mennanov / fieldmask-utils

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

Fix support for protobuf oneof fields to allow setting fields without… #15

Closed mohqas closed 3 years ago

mohqas commented 3 years ago

… providing the oneof typename

The protobuf fieldmask specifications (https://developers.google.com/protocol-buffers/docs/reference/csharp/class/google/protobuf/well-known-types/field-mask) state: Note that oneof type names ("test_oneof" in this case) cannot be used in paths.

This is currently not supported in fieldmask-utils. I've added support for setting fields of a oneof without having to provide the type name in the paths. This was causing issues when using GRPC clients against go that would automatically fill in field mask paths (e.g grpc-gateway).

I've kept the current state of supporting paths that contain the oneof type name as well.

codecov[bot] commented 3 years ago

Codecov Report

Merging #15 into master will decrease coverage by 0.08%. The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
- Coverage   86.75%   86.66%   -0.09%     
==========================================
  Files           2        2              
  Lines         317      330      +13     
==========================================
+ Hits          275      286      +11     
- Misses         22       23       +1     
- Partials       20       21       +1     
Impacted Files Coverage Δ
copy.go 81.60% <90.47%> (+0.19%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6776087...df9206e. Read the comment docs.

mohqas commented 3 years ago

I've also noticed that the library is not compatible with API v2 in go. There are new fields that are write protected, and the inverse field tests attempt to write them. We can simply skip destination fields that are write protected. Should I address this as part of this PR?

mohqas commented 3 years ago

It seems this is kinda tough to get right in go, and the specification are not clear for some cases. I have a failing test case commented right now since am not sure what is the expected behaviour there and how it would be implemented correctly.

I guess for now, I will use this hacky version in the fork since it would cover the requirements I have until go releases proper support for protobuf fieldmasks

mennanov commented 3 years ago

Things have changed for good since i created this repository. A new version of protobuf API has been released: https://blog.golang.org/protobuf-apiv2 which allows a way better introspection into proto messages via https://pkg.go.dev/google.golang.org/protobuf/reflect/protoreflect

If you only use this library for protobufs then try this tiny library https://github.com/mennanov/fmutils Your feedback is welcome.

Thanks!