mennanov / fieldmask-utils

Protobuf Field Mask Go utils
MIT License
229 stars 26 forks source link

Example using oneof fields and StructToStruct #19

Closed mattnathan closed 2 years ago

mattnathan commented 2 years ago

I'm trying to validate/limit writes to a proto message that includes a oneof group but can't work out how to combine MaskFromProtoFieldMask and StructToStruct.

The issue I'm having is that fieldmaskpb.New correctly validates the oneof fields as if they were top level fields, but fieldmask-utils expects them to be nested inside the oneof group name.

Example

Given a proto like this

message Msg {
  oneof name {
    string formal = 1;
    string casual = 2;
  }
}

This does not return an error: fieldmaskpb.New(&pb.Msg{}, "formal") but passing "name" or "name.formal" does. On the util side, using fieldMaskUtils.MaskFromProtoFieldMask(fields, strcase.ToCamel) however results in a Mask that doesn't work with that proto.

Hope that all makes sense?

I've also submitted a ticket on the proto side to hopefully make this easier: https://github.com/golang/protobuf/issues/1344

mennanov commented 2 years ago

According to the field mask docs the name of the oneof field should not be included in the mask: https://developers.google.com/protocol-buffers/docs/reference/csharp/class/google/protobuf/well-known-types/field-mask

Oneof fields are not addressed properly in this repo according to the specification. I'd recommend taking a look at https://github.com/mennanov/fmutils which uses a new native protobuf reflection API and handles the case for oneof fields correctly, however it works with protobufs only and will not work with regular Go structs at this repo does.

mattnathan commented 2 years ago

I'll take a look, would it be worth adding something to some docs to explain this caveat when converting a FieldMask and using it with struct mode?

mennanov commented 2 years ago

It is definitely worth mentioning this in the docs (README.md file) as well as adding a test for oneof fields. Would you mind creating a pull request for that?

mattnathan commented 2 years ago

Sure, I'll have a go at updating the README.

I'm not sure what tests need making though. There are already copy tests that include oneof fields, and as we've established converting from FieldMask to Mask is incompatible with oneofs, so any test I write would fail (or test this failure)

mennanov commented 2 years ago

Yeah, you're right, there is already a test that involves a oneof field with a full name.

So the only thing that is left is the README.

Thanks!

mattnathan commented 2 years ago

PR created: #20

mennanov commented 2 years ago

Thanks for contributing!