Fix auto generated fieldmask case in patch requests #1049

william-plano-oxb · 2019-09-25T08:34:09Z

Fix up tests to check expected behaviour
Fix up documentation to match new behaviour

Fix up tests to check expected behaviour Fix up documentation to match new behaviour

examples/server/a_bit_of_everything.go

william-plano-oxb · 2019-09-25T08:34:58Z

examples/server/a_bit_of_everything.go

 func (s *_ABitOfEverythingServer) PatchWithFieldMaskInBody(ctx context.Context, request *examples.UpdateV2Request) (*empty.Empty, error) {
-	// low-effort attempt to modify the field mask to only include paths for the ABE struct. Since this is only for the


This method could be removed entirely and the rest endpoint annotation in the proto file added an additional binding to the UpdateV2 method. But it's perhaps useful for documentation to have it separate. 🤷‍♂

I think it'd be nicer to remove this and show that it's possible to use an additional binding instead. Would you mind changing it?

Sure thing, I was very much on-the-fence about whether to keep it.

examples/server/fieldmask_helper.go

examples/server/fieldmask_helper_test.go

protoc-gen-grpc-gateway/gengateway/template.go

protoc-gen-grpc-gateway/gengateway/template_test.go

johanbrandhorst

This is a great PR - thanks for all the comments.

Something I'm curious about, what happens if someone uses non-standard casing in their protobuf field names. Will using camelCase for your fields produce camelCased field masks?

examples/server/a_bit_of_everything.go

johanbrandhorst · 2019-09-25T08:43:36Z

examples/server/a_bit_of_everything.go

 func (s *_ABitOfEverythingServer) PatchWithFieldMaskInBody(ctx context.Context, request *examples.UpdateV2Request) (*empty.Empty, error) {
-	// low-effort attempt to modify the field mask to only include paths for the ABE struct. Since this is only for the


I think it'd be nicer to remove this and show that it's possible to use an additional binding instead. Would you mind changing it?

johanbrandhorst · 2019-09-25T08:53:13Z

The bazel failure is real btw: https://circleci.com/gh/grpc-ecosystem/grpc-gateway/4409. We'll need to run something to re-generate the bazel files I think (@achew22, @drigz?).

william-plano-oxb · 2019-09-25T09:01:00Z

This is a great PR - thanks for all the comments.

Thanks 😄

Something I'm curious about, what happens if someone uses non-standard casing in their protobuf field names. Will using camelCase for your fields produce camelCased field masks?

The intention is that the field masks are taken from the json keys in the request as-is. So as long as the user passes camelCase names matching the protobuf field names in the request, it should result in camelCase field masks too. That said, this seems like a reasonable thing to add a test for.

william-plano-oxb · 2019-09-25T09:03:09Z

The bazel failure is real btw: https://circleci.com/gh/grpc-ecosystem/grpc-gateway/4409. We'll need to run something to re-generate the bazel files I think (@achew22, @drigz?).

I did have a look at that, but I've not used Bazel before and I wasn't sure what it was telling me (though there is a command in the error output that I'll try running locally and see if it makes any changes).
Oddly the diff that it seems to be complaining about doesn't look like a diff at all, maybe it's a line ending thing?

drigz · 2019-09-25T09:11:40Z

Oddly the diff that it seems to be complaining about doesn't look like a diff at all, maybe it's a line ending thing?

You should be able to run the command or manually apply the diff, eg:

patch -p0 << EOF
--- examples/server/BUILD.bazel	1970-01-01 00:00:00.000000000 +0000
+++ examples/server/BUILD.bazel	1970-01-01 00:00:00.000000000 +0000
@@ -20,6 +20,7 @@
         "//examples/proto/sub2:go_default_library",
         "@com_github_golang_glog//:go_default_library",
         "@com_github_golang_protobuf//proto:go_default_library",
+        "@com_github_golang_protobuf//protoc-gen-go/generator:go_default_library_gen",
         "@com_github_rogpeppe_fastuuid//:go_default_library",
         "@go_googleapis//google/rpc:errdetails_go_proto",
         "@io_bazel_rules_go//proto/wkt:duration_go_proto",
--- runtime/BUILD.bazel	1970-01-01 00:00:00.000000000 +0000
+++ runtime/BUILD.bazel	1970-01-01 00:00:00.000000000 +0000
@@ -29,7 +29,6 @@
         "//utilities:go_default_library",
         "@com_github_golang_protobuf//jsonpb:go_default_library_gen",
         "@com_github_golang_protobuf//proto:go_default_library",
-        "@com_github_golang_protobuf//protoc-gen-go/generator:go_default_library_gen",
         "@go_googleapis//google/api:httpbody_go_proto",
         "@io_bazel_rules_go//proto/wkt:any_go_proto",
         "@io_bazel_rules_go//proto/wkt:duration_go_proto",
EOF

The diff reflects the fact the changes in the import "github.com/golang/protobuf/protoc-gen-go/generator" lines.

johanbrandhorst · 2019-09-25T09:11:40Z

Something I'm curious about, what happens if someone uses non-standard casing in their protobuf field names. Will using camelCase for your fields produce camelCased field masks?

The intention is that the field masks are taken from the json keys in the request as-is. So as long as the user passes camelCase names matching the protobuf field names in the request, it should result in camelCase field masks too. That said, this seems like a reasonable thing to add a test for.

Hmm, I wonder how this will interact with the JSON marshaler OrigName setting, and if we need to do something about it. A proto field's JSON representation can be field_name or fieldName depending on the marshaller setting. Could we add a test for the latter being transformed correctly as well?

william-plano-oxb · 2019-09-25T09:41:05Z

The diff reflects the fact the changes in the import "github.com/golang/protobuf/protoc-gen-go/generator" lines

🤦‍♂ Of course, it's 2 diffs. I was just reading it wrong.

william-plano-oxb · 2019-09-25T09:41:32Z

Could we add a test for the latter being transformed correctly as well?

Will do, fingers crossed it "just works"™️

Replaced with an additional binding to UpdateV2

william-plano-oxb · 2019-09-25T15:46:16Z

Could we add a test for the latter being transformed correctly as well?

Will do, fingers crossed it "just works"™️

Unfortunately, if somewhat predictably, it doesn't work very well. In the simple cases it works just fine, however there a few edge cases that my testing has picked up where JSON's camelCasing and protoc-gen-go/generator's camelCasing disagree, for a concrete example "foo_Bar" becomes "fooBar" with the former, but remains "foo_Bar" with the latter.

The more serious issue is however that if you specify [json_name] options in the proto file, then all bets are off since you can completely rename things rather than just changing the case.

I'll have to have a bit of a think about how to sort this out.

johanbrandhorst · 2019-09-25T15:52:04Z

Unfortunately, if somewhat predictably, it doesn't work very well. In the simple cases it works just fine, however there a few edge cases that my testing has picked up where JSON's camelCasing and protoc-gen-go/generator's camelCasing disagree, for a concrete example "foo_Bar" becomes "fooBar" with the former, but remains "foo_Bar" with the latter.

The more serious issue is however that if you specify [json_name] options in the proto file, then all bets are off since you can completely rename things rather than just changing the case.

I'll have to have a bit of a think about how to sort this out.

I don't think we're going to be able to support all the cases, and that's okay. It's possible for users to turn off the FieldMask implementation, and for the vast majority of cases it should work. I think this fix is better than what we have at the moment as it should be closer to the source of truth (the proto field name). I'm OK with these shortcomings as long as we document them, of course.

codecov-io · 2019-09-26T15:36:40Z

Codecov Report

Merging #1049 into master will increase coverage by 0.1%.
The diff coverage is 10%.

@@            Coverage Diff             @@
##           master    #1049      +/-   ##
==========================================
+ Coverage   53.86%   53.97%   +0.11%     
==========================================
  Files          40       41       +1     
  Lines        4127     4126       -1     
==========================================
+ Hits         2223     2227       +4     
+ Misses       1661     1657       -4     
+ Partials      243      242       -1

Impacted Files	Coverage Δ
protoc-gen-grpc-gateway/gengateway/template.go	`80.8% <ø> (+15.15%)`	⬆️
examples/server/a_bit_of_everything.go	`0% <0%> (ø)`	⬆️
examples/server/non_standard_names.go	`0% <0%> (ø)`
examples/server/main.go	`0% <0%> (ø)`	⬆️
runtime/fieldmask.go	`94.11% <100%> (-2.32%)`	⬇️
examples/server/fieldmask_helper.go	`92% <100%> (ø)`	⬆️

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 6527435...84008e0. Read the comment docs.

william-plano-oxb · 2019-09-26T15:41:29Z

Okay, sorry, I went down a bit of a rabbit hole trying to support the json names. I have left that out of this PR for now.
I do have something that works, but it's not very clean. Shall I check it in as a separate PR on my fork?

johanbrandhorst

Could we add another section to the _docs/patch.md documenting the supported json <-> proto field name mappings? Just something like "If you define custom JSON names, you're SOL with regards to the automatic PATCH feature".

johanbrandhorst · 2019-09-26T15:48:02Z

Okay, sorry, I went down a bit of a rabbit hole trying to support the json names. I have left that out of this PR for now.
I do have something that works, but it's not very clean. Shall I check it in as a separate PR on my fork?

Hm, no, go ahead and check it into this one. I'm curious now :). This is already one of the best PRs I've reviewed so the anticipation is great!

william-plano-oxb · 2019-09-26T15:51:17Z

Hm, no, go ahead and check it into this one. I'm curious now :). This is already one of the best PRs I've reviewed so the anticipation is great!

I'm a bit reluctant to mess this PR up if we decide not to take the change though. I guess we could always revert it with a force push to my branch, not sure if that would break the world though.

johanbrandhorst · 2019-09-26T15:52:25Z

Hm, no, go ahead and check it into this one. I'm curious now :). This is already one of the best PRs I've reviewed so the anticipation is great!

I'm a bit reluctant to mess this PR up if we decide not to take the change though. I guess we could always revert it with a force push to my branch, not sure if that would break the world though.

Alright fine, lets leave it out for now and open a separate PR.

william-plano-oxb · 2019-09-26T16:29:19Z

Alright fine, lets leave it out for now and open a separate PR.

Okay, all synced up with master now, and I've created https://github.com/william-plano-oxb/grpc-gateway/pull/1 on my fork with my attempt at getting the json names supported.

johanbrandhorst · 2019-09-26T16:36:35Z

Could we add another section to the _docs/patch.md documenting the supported json <-> proto field name mappings? Just something like "If you define custom JSON names, you're SOL with regards to the automatic PATCH feature".

Cool, as I said in the other PR, it LGTM, but we can still merge it separately. I'd be happy to merge this after ^ has been addressed.

william-plano-oxb · 2019-09-27T11:06:33Z

Hmm, as I was fixing up the documentation for this I've noticed something that didn't seem right: The second case where we say

The FieldMask is exposed to the REST request (as in the PatchWithFieldMaskInBody example). For this case, a check is made as to whether the FieldMask is nil/empty prior to populating with the request body

Isn't true at all, we just don't do anything for this case. I've checked the code before I started poking at it, and all we did then was the case translation, it didn't take anything from the Body of the message. And I have confirmed this by modifying the TestABEPatchBody test to add an extra field on the originalValues that isn't in the input values, in the case of nil/empty field mask we're just falling through to the full Update/PUT semantics.

The question is; what do we want to do here? Shall I just fix up the docs, or modify the code to support this case properly? (And if the latter, should that be in another separate PR?)

william-plano-oxb · 2019-09-27T11:08:30Z

In fact, I'm not sure how we would know what the resource is called in that case. Maybe we can just say the request body can only have 2 fields, and one of them must be the field_mask so the resource is necessarily the other one?

johanbrandhorst · 2019-09-27T11:13:56Z

In fact, I'm not sure how we would know what the resource is called in that case. Maybe we can just say the request body can only have 2 fields, and one of them must be the field_mask so the resource is necessarily the other one?

Yeah I think a prerequisite of using this feature is to design your PATCH requests following the Google API design convention (https://cloud.google.com/apis/design/standard_methods#update). I don't particularly care about the fieldmask-in-body request since then you can also just not use this feature, so we can remove it I think.

william-plano-oxb · 2019-09-27T11:25:40Z

following the Google API design convention

The request message field containing the resource must map to the request body.

I don't think I realised it was this strict, seems fair to exclude this case then. I'll fix up the docs while I'm there.

Add benchmark for patching ABE

examples/integration/integration_test.go

johanbrandhorst · 2019-09-27T12:12:39Z

examples/integration/integration_test.go

+	for i := 0; i < b.N; i++ {
+		testABEPatch(b)
+	}
+}


Hm, this benchmark actually fires requests at a real server? The cost of the fieldmask function is surely going to be lost in the variance of I/O cost? Could we make the benchmark run the transformation directly or is this due to the circular dependency problem?

Yes, you're right, let me have a go at aiming this more directly at the function in question.

Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com>

and just measure the FieldMaskFromRequestBody method

Fix typos (again) Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com>

runtime/fieldmask_test.go

johanbrandhorst

Fantastic, thanks for this brilliant PR and for being such a great contributor! I'm beyond impressed. I hope we can work together again in the future :).

william-plano-oxb · 2019-09-27T14:07:23Z

Thank you for being so responsive to the contribution. It's a real pleasure to help out with a great project like this.

RJPercival · 2019-11-14T15:48:45Z

runtime/fieldmask.go

-
-// CamelCaseFieldMask updates the given FieldMask by converting all of its paths to CamelCase, using the same heuristic
-// that's used for naming protobuf fields in Go.
-func CamelCaseFieldMask(mask *field_mask.FieldMask) {


This func being removed, and subsequently released as a patch release (v1.11.3), broke semantic versioning guarantees (our code no longer builds simply by updating from v1.11.2 to v1.11.3). Could you please avoid doing this in the future?

Terribly sorry @RJPercival, you're right about this breaking users using a generator from version v1.11.2 and a runtime library with version v1.11.3. However, we do always encourage our users to ensure their generator versions and runtime library versions match up, so this shouldn't be a problem for users that can upgrade both at the same time. Sorry for the inconvenience caused.

I don't think this can be guaranteed with Go Modules. You may import a dependency that used an older version of the generator and Go Modules will happily use a newer runtime library, so long as it's at the same major version, since that's supposed to mean it's backwards compatible.

This can (and should) be guaranteed by go modules. Define a tool dependency for the generator, and run go install github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway before invoking protoc.

I agree that it's not ideal though, and I apologize for the inconvenience caused.

I don't think that guarantees what you think it guarantees. We already define a tool dependency, and have grpc-gateway listed in our go.mod file with a pre-v1.11.3 version number. However, according to the version selection rules of Go Modules, anyone can import our module and require a newer v1 version of grpc-gateway in their go.mod file and it'll use that newer version (because it's supposed to be backwards-compatible), resulting in a build error. They then have to figure out that v1.11.3 of grpc-gateway broke them and pin to an earlier version.

Thanks for the apology. I don't mean to labour the point, but I just want to make it clear that violating semantic versioning rules will break your users and so we'd very much appreciate you avoiding it, thanks! 🙂

) * Fix fieldmask case Fix up tests to check expected behaviour Fix up documentation to match new behaviour * Remove PatchWithFieldMaskInBody Replaced with an additional binding to UpdateV2 * Regenerated code * `bazel run :gazelle` * Added tests for proto file with non snake_case field names * tabs -> spaces * Update patch documentation Add benchmark for patching ABE * Apply suggestions from code review Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com> * Move benchmark to fieldmask_test and just measure the FieldMaskFromRequestBody method * Apply suggestions from code review Fix typos (again) Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com> * Add more representative benchmark

Fix fieldmask case

9bde734

Fix up tests to check expected behaviour Fix up documentation to match new behaviour

googlebot added the cla: yes label Sep 25, 2019