Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add array of nested objects with guaranteed order #1957

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davidgrisham
Copy link

For proper deserialization of protobuf messages via the Confluent Schema Registry, we must have access to the messages in the order they appear in the schema so that we can select the target schema based on the message indices added during schema registry serialization. More info on the spec can be found here: https://docs.confluent.io/platform/current/schema-registry/fundamentals/serdes-develop/index.html#wire-format

This is part of a larger effort to fix a longstanding issue in the most widely used NodeJS Confluent Schema Registry client that does not follow the protobuf wire spec. I'll open a PR in that repo that implements the changes I've done here to properly encode/decode these messages.

I've added an additional array field to Namespaces/Types called orderedNestedObjects that achieves what I need here. I attempted to use the existing nestedArray/_nestedArray fields, but they were not populated when I need them (upon parsing a schema) and instead seemed to implement a more complicated caching mechanism while not guaranteeing the original ordering.

The implementation on this branch achieves my goals but may be missing something, it would be helpful if someone more familiar with the code could take a look + help me ensure my additions cleanly merge with the existing implementation. Happy to answer any questions / help iterate on this, thanks!

cc @alexander-fenster @bcoe

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.

None yet

1 participant