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 explicit field IDs to flatbuffer schemas #148

Merged
merged 1 commit into from
May 22, 2024

Conversation

jameskuszmaul-brt
Copy link
Contributor

Changelog

Added explicit field IDs to flatbuffer schemas.

Docs

None

Description

This allows the schemas to be compiled with --require-explicit-ids turned on in flatc, which can be nice to help prevent people from accidentally making breaking changes to flatbuffer schemas if they choose to reorder fields (or insert new fields in the middle of the schemas). Because of how flatc works with includes, if downstream users want to use --require-explicit-ids on their own schemas that happen to include foxglove schema types, they must update the foxglove schemas that they use to include IDs.

This allows the schemas to be compiled with --require-explicit-ids
turned on in flatc, which can be nice to help prevent people from
accidentally making breaking changes to flatbuffer schemas if they
choose to reorder fields (or insert new fields in the middle of the
schemas).
@jameskuszmaul-brt
Copy link
Contributor Author

Waiting on #147 for CI....

Copy link
Member

@jtbandes jtbandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something worth noting is that we have a mechanism here for specifying field ids explicitly for protobuf (protobufFieldNumber). This was added in #61 because we wanted to add a field, but put it in a different order in the IDL, without changing the ids of existing fields.

This isn't currently an issue for flatbuffers because we created these .fbs files after making that change (#70) and these ids you've added should correspond with the existing field order today. But anyway, since the Foxglove app uses the binary schemas from the MCAP file, this probably won't matter for our purposes — only for external users who are associating this schema with some pre-existing data.

...and actually, while writing this comment I discovered we added a new field in a different order in #108 after the .fbs files were first published (cc @defunctzombie). It was probably a miss that we didn't do this field numbering thing back then. (Amusingly, we discussed the issue in #108 (comment) for protobuf but forgot about flatbuffers.) Anyway, what's done is done :)

@jtbandes jtbandes merged commit a4b0b35 into foxglove:main May 22, 2024
9 of 10 checks passed
@jameskuszmaul-brt jameskuszmaul-brt deleted the explicit_field_ids branch May 23, 2024 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants