HttpJson UpdateUserRequest's FieldMask is unable to be Unmarshalled

[lqiu96]

Java Gapic-Generator is implementing showcase coverage for the Identity client (modeled after golang). Draft PR with the full sample code: googleapis/sdk-platform-java#1431

Relevant Code for the test case:

CreateUserRequest createUserRequest = CreateUserRequest.newBuilder()
            .setUser(User.newBuilder().setDisplayName("Jane Doe").setEmail("janedoe@example.com").setNickname("Doe").setHeightFeet(5).build()).build();
    User user = httpjsonClient.createUser(createUserRequest);
    User expected = createUserRequest.getUser();

    UpdateUserRequest updateUserRequest = UpdateUserRequest.newBuilder()
            .setUser(User.newBuilder()
                    .setName(user.getName())
                    .setDisplayName(user.getDisplayName())
                    .setEmail("janedoe@jane.com")
                    .setHeightFeet(6.0)
                    .setEnableNotifications(true)
                    .build())
            .setUpdateMask(FieldMask.newBuilder().addAllPaths(Arrays.asList("email", "height_feet", "enable_notifications")).build())
            .build();
    User updatedUser = httpjsonClient.updateUser(updateUserRequest);

Results in this error:

Caused by: com.google.api.client.http.HttpResponseException: 400 Bad Request
POST http://localhost:7469/v1beta1/users/1?updateMask=email,heightFeet,enableNotifications
{"error":{"code":400,"message":"error reading query params: terminal field \"updateMask\" of field path \"updateMask\" is of type \"message\" with value string \"email,heightFeet,enableNotifications\", which could not be parsed: unable to unmarshal field \"update_mask\" in message \"google.showcase.v1beta1.UpdateUserRequest\": proto: syntax error (line 1:1): invalid value email","details":null,"Body":"","Header":null,"Errors":null}}

Logging the HttpRequest:

-------------- REQUEST  --------------
POST http://localhost:7469/v1beta1/users/1?updateMask=email,heightFeet,enableNotifications
Accept-Encoding: gzip
User-Agent: Google-HTTP-Java-Client/1.43.0 (gzip)
x-http-method-override: PATCH
x-goog-api-client: gl-java/11.0.16.1 gapic/0.0.1-SHAPSHOT gax/2.23.3-SNAPSHOT rest/
Content-Type: application/json; charset=utf-8
Content-Length: 115

curl -v --compressed -X POST -H 'Accept-Encoding: gzip' -H 'User-Agent: Google-HTTP-Java-Client/1.43.0 (gzip)' -H 'x-http-method-override: PATCH' -H 'x-goog-api-client: gl-java/11.0.16.1 gapic/0.0.1-SHAPSHOT gax/2.23.3-SNAPSHOT rest/' -H 'Content-Type: application/json; charset=utf-8' -d '@-' -- 'http://localhost:7469/v1beta1/users/12?updateMask=email,heightFeet,enableNotifications' << $$$
Total: 115 bytes
{"name":"users/12","displayName":"Jane Doe","email":"janedoe@jane.com","heightFeet":6.0,"enableNotifications":true}

The logs seems to show that the updateMask field is encodedproperly as a query param, but Showcase is unable to decode this query param:

2023/03/07 10:16:40 error reading query params: terminal field "updateMask" of field path "updateMask" is of type "message" with value string "email,heightFeet,enableNotifications", which could not be parsed: unable to unmarshal field "update_mask" in message "google.showcase.v1beta1.UpdateUserRequest": proto: syntax error (line 1:1): invalid value email

---

Thought this might be an error with Java's HttpJson implementation for FieldMask, but tested this with Java-Scheduler using HttpJson + FieldMask and this works properly.

-------------- REQUEST  --------------
POST https://cloudscheduler.googleapis.com:443/v1/projects/lawrence-test-project-2/locations/us-central1/jobs/72824232043972625121?$alt=json;enum-encoding%3Dint&updateMask=schedule,timeZone
Accept-Encoding: gzip
Authorization: Bearer {BEARER}
User-Agent: Google-HTTP-Java-Client/1.42.3 (gzip)
x-goog-user-project: {PROJECT}
x-http-method-override: PATCH
x-goog-api-client: gl-java/11.0.16.1 gapic/2.11.0 gax/2.23.1 rest/
Content-Type: application/json
Content-Length: 330

curl -v --compressed -X POST -H 'Accept-Encoding: gzip' -H 'Authorization: Bearer {BEARER}' -H 'User-Agent: Google-HTTP-Java-Client/1.42.3 (gzip)' -H 'x-goog-user-project: lawrence-test-project-2' -H 'x-http-method-override: PATCH' -H 'x-goog-api-client: gl-java/11.0.16.1 gapic/2.11.0 gax/2.23.1 rest/' -H 'Content-Type: application/json' -d '@-' -- 'https://cloudscheduler.googleapis.com:443/v1/projects/{PROJECT}/locations/{LOCATION}/jobs/{ID}?$alt=json;enum-encoding%3Dint&updateMask=schedule,timeZone' << $$$
Total: 330 bytes
{"name":"projects/{PROJECT}/locations/{LOCATION}/jobs/{ID}","httpTarget":{"uri":"http://example.com/","httpMethod":2,"headers":{"User-Agent":"Google-Cloud-Scheduler"}},"userUpdateTime":"2023-03-07T15:57:34Z","state":1,"schedule":"1 10 * * FRI","timeZone":"America/New_York","attemptDeadline":"180s"}

[lqiu96]

Error seems to be stemming from this line:

gapic-showcase/util/genrest/resttools/populatefield.go

Line 257 in 656e5f4

err := protojson.Unmarshal([]byte(value), msgValue.Interface())

The fullName is found in the list:

gapic-showcase/util/genrest/resttools/populatefield.go

Lines 251 to 254 in 656e5f4

	fullName := string(fieldMsg.Descriptor().FullName())
	if !wellKnownTypes[fullName] {
	return nil, nil
	}

as google.protobuf.FieldMask.

Value passed in is email,heightFeet,enableNotifications. I'm not too familiar with the protojson and why this is causing an error.

[lqiu96]

Adding some logging to print out FieldMsg and MsgValue:

FieldMsg: &{GoReflectType:*fieldmaskpb.FieldMask Desc:MessageDescriptor{
	Syntax:   proto3
	FullName: google.protobuf.FieldMask
	Fields: [{
		Name:        paths
		Number:      1
		Cardinality: repeated
		Kind:        string
		HasJSONName: true
		JSONName:    "paths"
		IsList:      true
	}]
} Exporter:<nil> OneofWrappers:[] initMu:{state:0 sema:0} initDone:0 reflectMessageInfo:{fields:map[] oneofs:map[] fieldTypes:map[] denseFields:[] rangeInfos:[] getUnknown:<nil> setUnknown:<nil> extensionMap:<nil> nilMessage:{p:0x14000367790}} coderMessageInfo:{methods:{NoUnkeyedLiterals:{} Flags:0 Size:<nil> Marshal:<nil> Unmarshal:<nil> Merge:<nil> CheckInitialized:<nil>} orderedCoderFields:[] denseCoderFields:[] coderFields:map[] sizecacheOffset:0 unknownOffset:0 unknownPtrKind:false extensionOffset:0 needsInitCheck:false isMessageSet:false numRequiredFields:0}}

MsgValue: &{NoUnkeyedLiterals:{} DoNotCompare:[] DoNotCopy:[] atomicMessageInfo:0x1400014eb00}

2023/03/07 17:16:09 error reading query params: terminal field "updateMask" of field path "updateMask" is of type "message" with value string "email,heightFeet,enableNotifications", which could not be parsed: unable to unmarshal field "update_mask" in message "google.showcase.v1beta1.UpdateUserRequest": proto: syntax error (line 1:1): invalid value email

[noahdietz]

I can reproduce the error using just protojson code here: https://go.dev/play/p/Lmk7Vs6KbWF

The "fix" was to wrap the value in escaped double quotes e.g. \"email,heightFeet,enableNotifications\" because this is what Marshaling a FieldMask using the same protojson library looked like.

As such, it sounds like Go's protojson takes a more opinionated approach to JSON string serialization, whereas Java doesn't (that's not wrong, just not as opionated) and Google HTTP Transcoding accepts that same more lax value. This is a rather unfortunate situation.

One way to fix this would be to check if the field is explicitly of type google.protobuf.FieldMask and if so, wrap the value in escaped double quotes knowing it will be a string e.g. data := append(append([]byte{'"'}, []byte(value)...), '"'), but this is kind of wonky... @lqiu96 would you mind testing that change to gapic-showcase locally with your repro to see if it fixes it? If it does, that will doubly confirm it and we can talk to @vchudnov-g about how we should handle this.

[lqiu96]

Thanks @noahdietz! This makes sense. I was able to verify that escaping the value in double quotes works. I just ended up using value = fmt.Sprintf("\"%s\"", value) since I couldn't get the append() function to work even with a few variations of the []byte parameters (I'm not too familiar with golang unfortunately and just ended up creating a new string haha).

If I'm understanding this correctly, the fix for this is to check for the type of google.protobuf.FieldMask and check that the value passed in is escaped with quotes. If not, then we'll have to manually escape it since that's that the protojson library expects for FieldMask.

[noahdietz]

I just ended up using value = fmt.Sprintf("\"%s\"", value)

Yeah that works better :)

check for the type of google.protobuf.FieldMask and check that the value passed in is escaped with quotes. If not, then we'll have to manually escape it since that's that the protojson library expects for FieldMask.

Yeah...which sucks. We can do that in the meantime, but the discrepancy in serialization behavior is not ideal.

Can I ask, what serialization library is the Java implementation using for proto-to-JSON? I think specifically for the code that handles query parameters.

[lqiu96]

Can I ask, what serialization library is the Java implementation using for proto-to-JSON? I think specifically for the code that handles query parameters.

I just took a look and I believe we have our own internal Proto-to-Json Serializer via ProtoRestSerializer: https://github.com/googleapis/gapic-generator-java/blob/main/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

Query Params set via: https://github.com/googleapis/gapic-generator-java/blob/d831bd29ae171ada8aa7f30724c1ece615e1f844/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java#L150

This is all setup via the ApiMethodDescriptor for each Client (i.e Compliance): https://github.com/googleapis/gapic-generator-java/blob/d831bd29ae171ada8aa7f30724c1ece615e1f844/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/HttpJsonComplianceStub.java#L106-L131

[lqiu96]

Seem to be running into the same issues with Duration and Timestamp as well:
proto: syntax error (line 1:1): invalid value 2009-11-10T23
proto: syntax error (line 1:1): invalid value 5s

[noahdietz]

Ok, thank you that is important, because we can fix those here too. Perhaps a helper function that mutates value for those well known types that are serialized as strings.

@vchudnov-g isn't the discrepancy in serialization for these types a little odd? Not sure if Go's impl is too strict, or if Java's is too lax. ESF transcoding doesn't seem to care!

[blakeli0]

Can I ask, what serialization library is the Java implementation using for proto-to-JSON? I think specifically for the code that handles query parameters.

I just took a look and I believe we have our own internal Proto-to-Json Serializer via ProtoRestSerializer: https://github.com/googleapis/gapic-generator-java/blob/main/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

In the end, we use the JsonFormat provided by Protobuf for proto-to-JSON, see here regarding how we use it.

[noahdietz]

Indeed @blakeli0 that's what I gathered from the links Lawrence provided. This seems to be yet another discrepancy in how the proto-JSON libraries are implemented across languages. The fact that a value serialized by Java proto library cannot be deserialized by Go proto library makes proto-JSON not language-agnostic like the binary format is.

[lqiu96]

@vchudnov-g Wanted to see if you had some thoughts on this

[vchudnov-g]

Apologies for not seeing this issue sooner. I'm looking into this and seeing how to resolve it. I need to investigate more, but I am inclined to believe that Go's implementation is indeed too strict given a related issue with query strings, the parsing of the semicolons (#1255 : the semicolon is no longer a query param separator, so sending it unencoded ought to have it parsed as just another character in the key or value, but Go assumes that one meant it as a separator and thus errors). Let me look some more into this particular error.

EDIT: To be clear, the two issues are in two distinct libraries, but they both affect query params, so maybe workarounds for both should occur in the same place. TBD.

[vchudnov-g]

@noahdietz https://go.dev/play/p/Lmk7Vs6KbWF is not reproducing the error for me. The first Unmarshal does not return an error. Did they code change since you posted it?

[vchudnov-g]

Ah, OK, nm. That has the already fixed code. I expanded the snippet in https://go.dev/play/p/gDWlykyKxA3 to show both the error and the solution.

[noahdietz]

Ah, OK, nm. That has the already fixed code. I expanded the snippet in https://go.dev/play/p/gDWlykyKxA3 to show both the error and the solution.

Yes! Thank you! You are quick :)

[vchudnov-g]

I am able to reproduce the problem. Command-line repro steps:

• start the local server on the standard port (7469)

• issue the following commands:

  # works; note that if we set the field "name" it gets overriden by the server
  curl -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" -H 'Content-Type: application/json' -i -X POST http://localhost:7469/v1beta1/users -d'{"user":{"displayName":"JaneDoe","email":"janedoe@example.com","nickname":"Doe","heightFeet":5.0,"enableNotifications":true}}'
  
  # works
  curl -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" -H 'Content-Type: application/json' -i -X GET http://localhost:7469/v1beta1/users 
  
  # fails: reproduces failure in #1263
  curl -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" -H 'Content-Type: application/json' -i -X PATCH http://localhost:7469/v1beta1/users/0?updateMask=email,heightFeet,enableNotifications -d'{"displayName":"JaneDoe","email":"janedoe@jane.com","heightFeet":6.0,"enableNotifications":true}'
  
  # works
  curl -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" -H 'Content-Type: application/json' -i -X PATCH http://localhost:7469/v1beta1/users/0?updateMask=\"email,heightFeet,enableNotifications\" -d'{"displayName":"JaneDoe","email":"janedoe@jane.com","heightFeet":6.0,"enableNotifications":true}'      
  
  # panics because of missing field mask (see https://github.com/googleapis/gapic-showcase/issues/1281)
  curl -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" -H 'Content-Type: application/json' -i -X PATCH http://localhost:7469/v1beta1/users/0 -d'{"displayName":"JanetDoe","email":"janetdoe@jane.com","heightFeet":7.0,"enableNotifications":true}'
  

I suspect the fix may involve using ParseQuery, though we should also pre-process the query string to deal with unescaped semicolons (#1255). I am still investigating why the failure occurs here for FieldMask and not with all query params.

[vchudnov-g]

Some more information on the cause of this problem.

Summary: FieldMask consists of just a repeated field, but it is JSON- and query-string encoded different than normal repeated fields:

• JSON Mapping of protobufs: FieldMask has a different JSON encoding than repeated fields:
  • repeated fields are arrays: [v,...]
  • FieldMask paths are comma-separated strings: "f.fooBar,h"
    • see field_mask.proto
• query params
  • in general (ref): repeated primitives are specified by repeating the field name as needed: ?param=A&param=b
  • I haven't yet found a canonical reference on the query string representation of FieldMask in the query string, but:
    • A Slides doc page shows that in query string, FieldMask paths are also represented as a single comma-separated string, just like in JSON

So: it does look like google.protobuf.FieldMask may need to be special-cased in Showcase, since protojson appears to not handle it. Given the snippets like https://go.dev/play/p/gDWlykyKxA3, this is a bug in protojson: it can't deserialize what other language implementations of the same spec serialized. We should file a bug there. (EDIT: fixed the preceding description)

Anyway, I think the fix/workaround for Showcase is straightforward now. Expect it shortly. (Though I need to work on some other projects, so wall-time ETA may be O(day).)

[noahdietz]

protojson: it can't deserialize what it serialized

I think it can do this (the first example in https://go.dev/play/p/gDWlykyKxA3 is exactly that), but it cannot deserialize what other language implementations of the same spec i.e. Java's serialized or one more generally, one that is not serialized exactly as its own implementation would serialize it.

We should file a bug there.

Would you like to or shall I? I appreciate all the time you have spent digging into this as our REGAPIC expert :)

[vchudnov-g]

it cannot deserialize what other language implementations of the same spec

Right, good clarification. I was too hasty in my writing.

I can file the bug; that way I'm more likely to notice once it gets fixed. ;-)

[noahdietz]

I can file the bug; that way I'm more likely to notice once it gets fixed. ;-)

Great thank you :) Happy to review any interim showcase fix too, just assign me as a reviewer.

[vchudnov-g]

TBH, I'm wondering about whether this really is a bug in protojson. I'm verifying with JSON validators that nanos is not valid JSON, but "nanos" is. So since Showcase is passing the query string value to the JSON unmarshaller, it's being correct in rejecting the invalid JSON. My workaround is to quote the value for FieldMask, but maybe I should do it for more/all well-known types. I'll need to check their JSON encodings. But I can do that as a follow-up so as to not delay this fix.

[lqiu96]

Thanks @noahdietz and @vchudnov-g!