Skip to content

[fix][test] Made ProtobufSchemaTest.testParsingInfoProperty order-independent#2

Open
LucasEby wants to merge 1 commit intomasterfrom
testParsingInfoPropertyNondeterministic
Open

[fix][test] Made ProtobufSchemaTest.testParsingInfoProperty order-independent#2
LucasEby wants to merge 1 commit intomasterfrom
testParsingInfoPropertyNondeterministic

Conversation

@LucasEby
Copy link
Owner

@LucasEby LucasEby commented Oct 1, 2025

Fixes apache#24806

Motivation

In ProtobufSchemaTest.testParsingInfoProperty, the json's contents and the PARSING_INFO field's contents do not have a deterministic order but the hardcoded string assertion assumes a deterministic order.

Serializing a map to a JSON with new ObjectMapper().writeValueAsString(schema.getSchemaInfo().getProperties()) can change the order of the keys since attribute order is not guaranteed in JSON. Additionally the inner JSON string order in PARSING_INFO can vary for the same reason. The ordering in both cases can change due to different environments producing the contents in different orders despite the logical contents being the same. Since the test compares the raw strings/trees "as-is", harmless re-ordering could flip the test from pass to fail despite the data being semantically the same.

Modifications

We no longer compare raw strings/trees "as-is". Instead, we compare a json structure that we derive. When we normalize the properties, we copy every key/value from the properties map so nothing at the top level is ignored. For __PARSING_INFO__ only we parse every string value into an ArrayNode, sort the array deterministically by (number, name) to remove element-order nondeterminism, and then store it back as a JSON array (instead of a string). For all other keys we keep their string values as-is.

When performing the Assert.assertEquals assertion, it is performed with the JsonNode equals method which is an important subtlety. This is because it compares field names and values not memory addresses. Additionally field order is not considered. This is the reason why we treated the __PARSING_INFO__ field differently before because this assertion is sensitive to the order of the contents of __PARSING_INFO__ (it does an as-is comparison instead of allowing different orders).

This change keeps the spirit of the original test while eliminating failures caused solely by allowed (but previously unexpected) reordering.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as org.apache.pulsar.client.impl.schema.ProtobufSchemaTest#testParsingInfoProperty.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@LucasEby LucasEby force-pushed the testParsingInfoPropertyNondeterministic branch 8 times, most recently from c8176ad to ae0b534 Compare October 3, 2025 16:16
@LucasEby LucasEby force-pushed the testParsingInfoPropertyNondeterministic branch from ae0b534 to 7ad6d73 Compare October 3, 2025 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] ProtobufSchemaTest.testParsingInfoProperty order-independent

1 participant