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

Validation of FileDescriptorSets #5

Open
andrewhickman opened this issue Jan 27, 2022 · 6 comments
Open

Validation of FileDescriptorSets #5

andrewhickman opened this issue Jan 27, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@andrewhickman
Copy link
Owner

andrewhickman commented Jan 27, 2022

Currently FileDescriptor::new performs some sanity checking on the FileDescriptorSet, for example checking that all referenced types can be resolved. However there are many checks that protoc does and we do not.

@andrewhickman andrewhickman added the enhancement New feature or request label Jan 27, 2022
andrewhickman added a commit that referenced this issue Jan 1, 2023
This is in preparation for adding more checks for parity with protoc
itself. (see #5)
@andrewhickman
Copy link
Owner Author

andrewhickman commented Jan 4, 2023

Known error cases which we could check for but currently don't:

  • Invalid message/enum reserved ranges (invalid field number, start greater than end, overlapping ranges)
  • Use of reserved message field/enum value names
  • Re-use of the same field number for multiple extensions to the same message
  • Extending a non-options type in proto3
  • Using a proto2 enum in a proto3 message (Handle closed enums #44)
  • Use of message/enum names which are defined but not imported into the currently file (either directly or transitively through import public

@andrewhickman andrewhickman changed the title Use of potentially-invalid FileDescriptorSets Validation of FileDescriptorSets Jan 4, 2023
@mrpopogod
Copy link

I found that some of the work you've done on this item has caused the 0.10.x revisions to break for my use case. The validation fails on DuplicateFieldJsonName being the empty string.

DuplicateFieldJsonName { name: "", first: Label { file: "google/protobuf/descriptor.proto", path: [4, 1, 2, 0, 1], span: Some([60, 18, 60, 22]) }, second: Label { file: "google/protobuf/descriptor.proto", path: [4, 1, 2, 1, 1], span: Some([61, 18, 61, 25]) } }

It's a huge spam of seemingly every field in the proto. My understanding of the spec is that if a JSON field name is not specified it should default to the proto field name lowerCamelCase.

@andrewhickman
Copy link
Owner Author

@mrpopogod How are you generating the FileDescriptorSet? With protoc 3.19.1, it sets the json_name field in the descriptor even if the name isn't set explicitly in the .proto file

@mrpopogod
Copy link

In my case it's being generated by a schema registry owned by another team; you pass in a plaintext file and it will generate the FileDescriptorSet itself. I want to say it might still be proto2, but with no support for required fields (those get validated out).

@andrewhickman
Copy link
Owner Author

andrewhickman commented Feb 18, 2023

@mrpopogod I've published 0.10.2 which should resolve this by setting the json_name to the camel-cased field name if it isn't set

@mrpopogod
Copy link

Awesome, no longer getting the build error and my generated .rs files look good. Appreciate the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants