[Bug] The test `testSchemaIncompatibility` is failed in some certain version protobuf

[RobertIndie]

Search before asking

• I searched in the issues and found nothing similar.

Version

3.1 and master branch

Minimal reproduce step

• Use macos x86_64
• Install protobuf 21.12
• Run test ./pulsar-tests --gtest_filter="ProtobufNativeSchemaTest.testSchemaIncompatibility"

What did you expect to see?

The test passed

What did you see instead?

The test failed:

➜  tests ./pulsar-tests --gtest_filter="ProtobufNativeSchemaTest.testSchemaIncompatibility"
Note: Google Test filter = ProtobufNativeSchemaTest.testSchemaIncompatibility
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from ProtobufNativeSchemaTest
[ RUN      ] ProtobufNativeSchemaTest.testSchemaIncompatibility
2023-02-14 15:23:04.272 INFO  [0x10d003600] ClientConnection:190 | [<none> -> pulsar://localhost:6650] Create ClientConnection, timeout=10000
2023-02-14 15:23:04.273 INFO  [0x10d003600] ConnectionPool:97 | Created connection for pulsar://localhost:6650
2023-02-14 15:23:04.275 WARN  [0x700009e8a000] ClientConnection:456 | [<none> -> pulsar://localhost:6650] Failed to establish connection: Connection refused
2023-02-14 15:23:04.275 INFO  [0x700009e8a000] ClientConnection:388 | [127.0.0.1:64503 -> 127.0.0.1:6650] Connected to broker
2023-02-14 15:23:04.314 INFO  [0x700009e8a000] HandlerBase:72 | [persistent://public/default/ProtobufNativeSchemaTest-testSchemaIncompatibility, ] Getting connection from pool
2023-02-14 15:23:04.356 INFO  [0x700009e8a000] ProducerImpl:202 | [persistent://public/default/ProtobufNativeSchemaTest-testSchemaIncompatibility, ] Created producer on broker [127.0.0.1:64503 -> 127.0.0.1:6650]
2023-02-14 15:23:04.356 INFO  [0x10d003600] ProducerImpl:740 | [persistent://public/default/ProtobufNativeSchemaTest-testSchemaIncompatibility, standalone-120-9] Closing producer for topic persistent://public/default/ProtobufNativeSchemaTest-testSchemaIncompatibility
2023-02-14 15:23:04.356 INFO  [0x700009e8a000] ProducerImpl:704 | [persistent://public/default/ProtobufNativeSchemaTest-testSchemaIncompatibility, standalone-120-9] Closed producer 0
2023-02-14 15:23:04.357 INFO  [0x700009e8a000] HandlerBase:72 | [persistent://public/default/ProtobufNativeSchemaTest-testSchemaIncompatibility, ] Getting connection from pool
2023-02-14 15:23:04.366 INFO  [0x700009e8a000] ProducerImpl:202 | [persistent://public/default/ProtobufNativeSchemaTest-testSchemaIncompatibility, ] Created producer on broker [127.0.0.1:64503 -> 127.0.0.1:6650]
2023-02-14 15:23:04.366 INFO  [0x10d003600] ProducerImpl:695 | Producer - [persistent://public/default/ProtobufNativeSchemaTest-testSchemaIncompatibility, standalone-120-9] , [batchMessageContainer = { BatchMessageContainer [size = 0] [bytes = 0] [maxSize = 1000] [maxBytes = 131072] [topicName = persistent://public/default/ProtobufNativeSchemaTest-testSchemaIncompatibility] [numberOfBatchesSent_ = 1] [averageBatchSize_ = 0] }]
/Users/aaronrobert/codebase/cpp-verify/apache-pulsar-client-cpp-3.1.2/tests/ProtobufNativeSchemaTest.cc:109: Failure
Expected equality of these values:
  ResultIncompatibleSchema
    Which is: IncompatibleSchema
  createProducerResult(getExternalMessageDescriptor())
    Which is: Ok
2023-02-14 15:23:04.366 INFO  [0x10d003600] ProducerImpl:695 | Producer - [persistent://public/default/ProtobufNativeSchemaTest-testSchemaIncompatibility, standalone-120-10] , [batchMessageContainer = { BatchMessageContainer [size = 0] [bytes = 0] [maxSize = 1000] [maxBytes = 131072] [topicName = persistent://public/default/ProtobufNativeSchemaTest-testSchemaIncompatibility] [numberOfBatchesSent_ = 0] [averageBatchSize_ = 0] }]
2023-02-14 15:23:04.366 INFO  [0x10d003600] ClientConnection:1600 | [127.0.0.1:64503 -> 127.0.0.1:6650] Connection closed with ConnectError
2023-02-14 15:23:04.366 INFO  [0x10d003600] ClientConnection:269 | [127.0.0.1:64503 -> 127.0.0.1:6650] Destroyed connection
[  FAILED  ] ProtobufNativeSchemaTest.testSchemaIncompatibility (96 ms)
[----------] 1 test from ProtobufNativeSchemaTest (96 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (96 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] ProtobufNativeSchemaTest.testSchemaIncompatibility

 1 FAILED TEST

Anything else?

The root cause may be here:

pulsar-client-cpp/tests/ProtobufNativeSchemaTest.cc

Lines 41 to 48 in c3e3e8a

	const ::google::protobuf::Descriptor* getExternalMessageDescriptor() {
	#if GOOGLE_PROTOBUF_VERSION < 3020000
	::proto::external::ExternalMessage msg;
	return msg.GetDescriptor();
	#else
	return ::proto::TestMessage::GetDescriptor();
	#endif
	}

If GOOGLE_PROTOBUF_VERSION >= 3020000, it will return the same message descriptor, which results in the assertion failing:

pulsar-client-cpp/tests/ProtobufNativeSchemaTest.cc

Line 109 in c3e3e8a

ASSERT_EQ(ResultIncompatibleSchema, createProducerResult(getExternalMessageDescriptor()));

Are you willing to submit a PR?

• I'm willing to submit a PR!

[tisonkun]

@merlimat @BewareMyPower what is the reason we use this conditional compile tech here? Is it a valid issue or by design?

[BewareMyPower]

@tisonkun In the original PR (apache/pulsar#11388), the descriptor is retrieved from the class (ExternalMessage).

    ASSERT_EQ(ResultIncompatibleSchema,
              createProducerResult(::proto::external::ExternalMessage::GetDescriptor()));

I just verified that my local protobuf (3.21.12) also generates the static method.

#define GOOGLE_PROTOBUF_VERSION 3021012

  static const ::PROTOBUF_NAMESPACE_ID::Descriptor* GetDescriptor() {
    return default_instance().GetMetadata().descriptor;
  }

Then I tried generating the files with Protobuf 3.19.2, the generated GetDescriptor method is still static. So I think it is not necessary to do conditional compile here.

[BewareMyPower]

I just checked Protobuf 3.1.0 and found no GetDescriptor method generated. It seems that we should use descriptor() method for backward compatibility. See https://protobuf.dev/reference/cpp/cpp-generated/#message

static const Descriptor* descriptor(): Returns the type’s descriptor. This contains information about the type, including what fields it has and what their types are. This can be used with reflection to inspect fields programmatically.