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

types/known/fieldmaskpb: special parsing behaviour for struct.proto types #1227

Closed
johanbrandhorst opened this issue Oct 22, 2020 · 5 comments

Comments

@johanbrandhorst
Copy link
Member

johanbrandhorst commented Oct 22, 2020

Is your feature request related to a problem? Please describe.
The grpc-gateway needs to be able to parse incoming JSON and generate a field mask from it. This works fine for most types, but specifically for the generic types google.protobuf.Struct and google.protobuf.Value, the existing fieldmaskpb.New and .Append behaviour does not allow generic uses of these types. We cannot use the fieldmask helpers in the grpc-gateway until this is supported.

For example, given a field named struct_field with a type google.protobuf.Struct and the incoming JSON:

{"struct_field": {"name":{"first": "bob"}, "amount": 2}}

We would like to produce a paths slice that looks like:

["struct_field.name.first", "struct_field.amount"]

Describe the solution you'd like
I'd like there to be a special case for these generic, well known types only, in the behaviour of the fieldmask helpers.

Describe alternatives you've considered
Keeping our own home grown field mask generation and validation logic.

Additional context
See here for our current field mask generation logic.

@neild
Copy link
Contributor

neild commented Oct 22, 2020

The field mask syntax is defined here, and is common across languages:
https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/field_mask.proto#L43

If I follow correctly, you're asking for a change to that syntax. Changes that affect multiple protobuf implementations are out of scope for this repository; you should raise the issue on the general protobuf discussion group.

Note that if you just want to add a path to a FieldMask without validation, fieldmask.New and FieldMask.Append aren't giving you anything useful. All they do is validate.

(Apologies if I misunderstand the change you're looking for.)

Edit: Fixed link to the field mask syntax.

@dsnet dsnet changed the title Special case fieldMask parsing behaviour for struct.proto types types/known/fieldmaskpb: special parsing behaviour for struct.proto types Oct 22, 2020
@dsnet
Copy link
Member

dsnet commented Oct 22, 2020

It sounds like you want a FieldMask-like message that is specific to JSON, which is an entirely reasonable use case. However, the FieldMask well-known type is documented to operate on the semantics of the protobuf type system, not arbitrary JSON objects. It's not clear to me that conflating these two semantics is necessarily a good idea. Either way, I think this needs to be discussed with the main protobuf project since semantics like this would affect all the language implementations of protobufs, not just Go.

@puellanivis
Copy link
Collaborator

Yeah, definitely sounds like a wider-discussion sort of thing.

@johanbrandhorst
Copy link
Member Author

I think I agree, this definitely conflates JSON and general field mask behaviour. Thanks for your thoughts on the topic, I'll put something out on the general protobuf discussion group, since I still think this would be a valuable addition.

@johanbrandhorst
Copy link
Member Author

I created a discussion on the protobuf mailing list: https://groups.google.com/g/protobuf/c/Lwm3X-uokBc.

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

4 participants