-
Notifications
You must be signed in to change notification settings - Fork 143
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
nicer JSON for standard wrappers #100
Comments
@siderakis No thoughts? |
What is Wrappers.proto in this context? |
In protobuf-java there is a file wrappers.proto in the package google.protobuf containing wrappers for primitive types when nullability/optionality is required. It contains messages like FloatValue or UInt64Value. It is the equivalent of java wrapper classes. |
Oh interesting, for JSON I have been converting the data Map<String,Object> to JSON directly using GSON. Would that work for your use case? or could you tell me about your use case? |
I'll give a simple example: a gRPC service returns painting which may have an artistID. The artistID is simply an long, or null in case of an unknown artist. In proto3 you have to wrap this long in an Int64 to make it nullable. At this time that would mean the output of the query contains either:
Nicer would be if artist were left out if unknown and the id would simply be given as a number instead of an object. |
Is this functionality specific to GraphQL/Rejoiner or just general proto3 JSON conversion & proto message design? |
With a long ID 0 could be a valid id so I see the issue (unless you never use 0 and treat it as null in the clients). A the default value for a string wouldn't have that issue and is what's used as resource identifiers in the related AIP https://aip.dev/122 |
If you created a stand-alone package we could use it in one of the examples, but I'm not sure if it should be in the rejoiner repo. |
If your aim is to conform to that proto/json mapping, then the current behaviour is a bug. At least that's how I read it. |
@siderakis We actually have the same problem, as we want to implement rejoiner for transmitting measurement data updates. For example it might be valid to have:
Is there a way to override this behaviour as an addition globally for these types? |
@sheepdreamofandroids I would also contribute to a PR here
|
Wrappers.proto defines types for nullable scalars. When those are mapped to JSON you get a nullable object with a value field. That's a bit awkward and unnecessary.
It would be nicer if the wrapper would be mapped to a simple nullable JSON field.
This could be special cased or maybe there is some way of marking wrappers. Initially just the well-known ones would be OK, I guess.
I could make a PR if you like.
The text was updated successfully, but these errors were encountered: