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 an option to use converters for all well known types #278

Open
friscoMad opened this issue Jul 2, 2024 · 2 comments
Open

Add an option to use converters for all well known types #278

friscoMad opened this issue Jul 2, 2024 · 2 comments

Comments

@friscoMad
Copy link

I love the converters support in this lib but it would be nice to have an option to use the wrappers that you provide as an extension, in custom wrappers (like UUID) it makes sense to define it at the field level but if a team want to use the converters every time for timestamps they need to add the option to every single field which is not a great DevEx.
I understand this should not be enforced but having the option would be nice.

An alternative would be to provide an option to define a map<Type,Converter> to be applied during code generation, with that solution it will cover well-known types and more special cases like an API where all bytes should be UUIDs.

@andrewparmet
Copy link
Collaborator

andrewparmet commented Jul 10, 2024

I understand the friction of having to specify the wrapper option every time, and I've seen other code generator libraries use type mappings for this sort of thing. I hesitate to introduce that functionality for a few reasons:

  • By relying solely on protbouf extensions, protokt's protoc plugin is build-tool agnostic. We could introduce a configuration we pass as a protoc invocation option to remain platform independent, however, with some overhead to implement that serialization and parsing in whatever future build tools protokt supports.
  • The more concerning complication is that for conversion to the protobuf-java Message interface, which is required to support existing JSON and protovalidate implementations, we must have wrapper information embedded in the protobuf descriptor via the wrapper option. Without it we cannot unwrap fields at runtime for validation and JSON serialization. While this is a detail specific to the JSON/protovalidate implementations we currently have, I think any reflective implementation would require converter information to be present in the descriptor. If we migrate to code-generated traversal then this issue would go away, but that's a big lift and not necessarily desirable anyways.

@friscoMad
Copy link
Author

First of all thanks for the in deep reply.
Yes, I was thinking of a protoc option that gets passed to the plugin, but you are right and I was not thinking about the conversion to DynamicMessage feature. However, it should be possible to keep it working if you add the option artificially in the generated protokt descriptors when the option is set.

That said I have been working on a workaround implementing a descriptor set preprocessor that does exactly that, it adds an extra step, and sadly the gradle Protobuf plugin does not yet support the descriptors_in option but I think we can use the workaround if this is not implemented.

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

No branches or pull requests

2 participants