-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Parsing from proto
should keep field ID.
#7645
Comments
would you mind if I do it ? |
@enum-class Sure, I always welcome PRs |
Ok, I have given this a bit more thought. We should map
We will support gaps in For conformity-sake, if someone adds a proto-field-id in an existing gap, To aid the user at this possibility of non-conformity, a new flag should be added: @enum-class Please see above for edits to your PR. @aardappel for any opinions. |
This generally seems ok, though I wonder if it is worth it to (by default) require the previous Alternatively, some clear message saying Or, if you specify |
What about oneof (union) that does not have id ? |
As we sort and map the ids, what is the point of this config flag ? |
I think we should just ignore that type. We don't have a similar concept in flatbuffers other than unions. But they are not the same thing.
This is to alert the user that they could introduce issues in the future. If you have:
And later do
That would lead to the following flatbuffer schema:
and
Those two schema are "valid" on their own, but are not conforming to each other. So the flag is alerting the user that this potential could happen in the future. |
Well, on the first conversion from proto->fbs, you wouldn't have a previous schema. That could probably be fixed with a flag to explicitly state "--first-proto-conversion" or whatnot. But, the later about forgetting to include
Yes, but this is kind of the purpose of the |
What do you mean by ignore them ? I mean what id should we assign to union (oneof) type ? |
It sounds reasonable. |
I think its a lot easier to run into incompatibility issues when the a |
While I am firmly in favour of addressing this issue, I realize that the changes to take proto field numbers into account may actually break binary backwards compatibility for existing users of the --proto flag, by generating a .fbs schema file that doesn't conform with the .fbs schema file generated by previous versions of flatbuffers. So I think we may need to have a --legacy-ignore-proto-field-numbers flag to preserve binary backwards compatibility in those cases? |
The fields within a one-of are just normal fields with their respective ids. Protobufs just ensures if that if one is set, the other fields are not-set. We don't have that mechanism in flatbuffers. So by ignoring that type, I just mean process the inner types as normal fields and disregard that they are wrapped in a oneof.
True, i'm fine having
Yeah, we have to make sure that existing users are not broken. I think the legacy flag is appropriate, though I would want to default to |
Rather than requiring it, with a flag to turn the error off, I suggested a clear warning (that you can ignore at your peril). |
In conclusion:
|
This one is designed to catch ABI breakage from possible future changes to the FlatBuffer compiler's proto to FlatBuffer schema conversion (e.g. see google/flatbuffers#7645). To do that, this CL checks in a snapshot of the .fbs file generated by the FlatBuffer compiler as of this CL, and then at build/test time we generate a new .fbs file, which will potentially use a different version of the FlatBuffer compiler (if/when the FlatBuffer compiler is updated). Then we use 'flatc --conform' to verify compatibility of those two. PiperOrigin-RevId: 493867469
* Parsing from proto should keep field ID. (fixes #7645) * Fix failed tests * Fix windows warning * Improve attribute generation in proto to fbs * Check if id is used twice. fix Some clang-format problems * Test if fake id can solve the test problem * Validate proto file in proto -> fbs generation. * Fix error messages * Ignore id in union * Add keep proto id for legacy and check gap flag have been added. Reserved id will be checked. * Add needed flags * unit tests * fix fromat problem. fix comments and error messages. * clear * More unit tests * Fix windows build * Fix include problems * Fake commit to invoke rebuild * Fix buzel build * Fix some issues * Fix comments, fix return value and sort for android NDK * Fix return type * Break down big function * Place todo --------- Co-authored-by: Derek Bailey <derekbailey@google.com>
When parsing from a proto that have explicit field identifiers, flatbuffers need to keep those identifiers stable by explicitly using the id attribute. This is because protos allow new field placement anywhere in their
message
, even before other fields, as long as they use a new field ID.Example:
The text was updated successfully, but these errors were encountered: