Add optional support for FieldMask wkt #576
Conversation
|
@donaldgraham @jmarais please let me know if I can do anything to get this reviewed and hopefully merged. |
|
Ping on this PR! We'd like to get this reviewed when you get a chance :) |
|
Hi. Thanks for contribution.
Why did you remove it in a recent commit? Since this is a breaking change I do not just want to merge it without any guard in place. |
jsonpb/jsonpb.go
Outdated
|
|
||
| // To correctly support marshaling field masks, the following is largely copied from the `golang/protobuf` v2 implementation | ||
| // https://github.com/golang/protobuf/blob/a3369c5dc28ac197f8ec8de40a7291be4dea85b5/encoding/protojson/well_known_types.go#L884 | ||
| if proto.MessageName(v) == "google.protobuf.FieldMask" { |
There was a problem hiding this comment.
You can generate a XXX_WellKnownType() method for the FieldMask message here:
https://github.com/gogo/protobuf/blob/master/protoc-gen-gogo/generator/generator.go#L2032
and then you can add the "FieldMask" case here:
https://github.com/gogo/protobuf/blob/master/jsonpb/jsonpb.go#L816
And same with the unmarshal
Hi, yes i removed it since we were just going to apply the patch internally if we could not get it accepted. I can add it back if you think that you will be able to accept/merge the pull request. I will address other comments. |
Apply suggestions from code review Co-Authored-By: Alex Peats-Bond <[email protected]>
ae35ac2 to
4ea7d02
Compare
|
I think this change is fine. As you pointed out, the current marshaled format is not inline with the proto spec. |
Added option on the marshaler/unmarshaler struct |
|
Any news about this one? :( |
Go implementation of JSONPB does not support well-known proto type FieldMask, which has special JSON encoding.
The spec is defined as:
mask { paths: "user.display_name" paths: "photo" }JSON:
{ mask: "user.displayName,photo" }What jsonpb actually serializes it as:
mask: {paths: ["user.displayName","photo"]}While this has been fixed in v2 as described in this issue golang/protobuf#745 , v1 is still widely in use and v2 does not have published timelines.
What does this pull request do
This PR adds support for correctly serializing FieldMask. But since this is a breaking change, this fix is gated by an env variable.
Protobuf implementations in other languages like python/cpp already support FieldMask. This allows gogo JSONPB generated JSON to be compatible.