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

Read json_name from protobuf field annotations for serialization and deserialization #2700

Closed
jabagawee opened this issue Jun 14, 2024 · 1 comment · Fixed by #2701
Closed
Assignees

Comments

@jabagawee
Copy link
Contributor

Background

We would like to have custom names for keys in JSON strings that do not match the names of the proto fields. Such a feature was added to the GSON library in 2015 via #710, which allowed developers to register custom field options in the ProtoTypeAdapter to read annotations for custom field names from the proto definition. Unfortunately some users use the json_name annotation described at https://protobuf.dev/programming-guides/proto3/#json, and it does not appear in the field options list in the proto field descriptor. Instead, it is set as its own first-class field in the field descriptor.

Design decisions

Backwards compatibility

One important criteria in making changes to GSON is backwards compatibility, because any changes we make must not break existing users. The first thing I want to establish is that the functionality of reading the json_name field should absolutely not be enabled by default. Doing so has the potential to break many users, some of whom would not even realize that such a thing was happening. As such, I believe that we should add a new boolean flag named something like shouldUseJsonNameFieldOption to the ProtoTypeAdapter and its Builder in order to configure whether or not the json_name field option should be read or not.

Interaction with other existing features

Assuming that we add such a shouldUseJsonNameFieldOption boolean, the next question is what should happen if it is turned on (in other words, set to true) in the presence of other features like field name serialization/deserialization formats and custom serialized name extensions.

Field name serialization and deserialization formats

ProtoTypeAdapter allows the user to set the expected "case format" of the names of both the proto and the JSON fields. By default, it expects proto field names to be written in lower_underscore and JSON field names to be written in lowerCamelCase. During serialization and deserialization, it converts between the "from" and "to" case formats. I propose that we should ignore the field name serialization format configuration when reading the json_name field option for three reasons:

  1. The json_name annotation is an explicit decision written by the user.
  2. This matches existing behavior from the Protobuf library's JsonFormat class in Java and implementations in other languages.
  3. This matches the behavior displayed when ProtoTypeAdapter is configured to read other custom serialized field names via its Builder's addSerializedNameExtension method.

Custom serialized name extensions

Due to the aforementioned PR #710, the ProtoTypeAdapter class already features a way to register a custom field option such that annotations are used to determine JSON field names during serialization and deserialization. If the user registers such a custom field option and also enables reading the json_name field, we need to determine what the ProtoTypeAdapter will do when it serializes and deserializes a field that features both annotations. I believe that we have several options here, though I personally advocate for one particular approach noted below.

Don't allow them both to be set at the ProtoTypeAdapter level

One option would be to throw an error when a ProtoTypeAdapter is built with both options configured. This may solve our issue by disallowing invalid states, but it may be excessively restrictive for users. A protobuf message definition can be effectively arbitrarily complex, and it's even possible for no single team to have ownership of every field all the way down the message definition hierarchy, so it's very possible that a protobuf may need both options to be set.

One counter-argument to this example would be that users should register a separate ProtoTypeAdapter for different subclasses of Message.class with different needs.

Don't allow them both to be set on a particular field

Instead of throwing an error at the point where a ProtoTypeAdapter is built, we could simply allow it and then throw an error when encountering a field that has both the specially registered field option and a json_name option as annotations on itself.

Allow both to be set but prefer the one that was set earlier in the ProtoTypeAdapter.Builder

This option leaves control to the user about what to do when both options are enabled in the ProtoTypeAdapter and both annotations are present on a particular proto field. Similar to how custom field options are chosen in the order that they are added in the Builder, we could track the order in which a call to setShouldUseJsonNameFieldOption was made relative to calls to addSerializedNameExtension and then read the annotation that corresponds to the earliest call for a given field. I am not a huge fan of this option because:

  • it is very implicit and quite unintuitive, and
  • it complicates the code implementation greatly for what I believe to be not very much benefit.

Allow both to be set but prefer one or the other

The last reasonable option would be to allow both options to be enabled in the ProtoTypeAdapter. When encountering a field for serialization or deserialization that features both annotations, we should prefer either:

  • the json_name annotation: This annotation exists as a first-class concept in the FieldDescriptor, so it might make sense to prefer this instead of treating it with the same priority as an arbitrary field option.
  • the custom serialized name annotation: This annotation would be explicitly set by the user and may be more domain-specific, meaning that it might make sense to give it a higher priority. This is especially true if we ever change the default behavior to respect the json_name annotation by default, in which case ProtoTypeAdapter would be reading the json_name annotation implicitly and reading the custom annotation explicitly. I believe this is the option that we should go with.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@jabagawee @eamonnmcmanus and others