GAPIC HttpJson additional_bindings annotation is not being used

[lqiu96]

Discovered while implementing Showcase testing for the ComplianceSuite. This fails the Compliance.RepeatDataPathTrailingResource test case: Testing group: Binding selection testing- RPC Name: Compliance.RepeatDataPathResourcewith Request Name:Binding testing additional binding`.

The path template for Compliance.RepeatDataPathResource:
/v1beta1/repeat/{info.fString=first/*}/{info.fChild.fString=second/*}/bool/{info.fBool}:pathresource. The data is

"info": {
            "fString": "second/greetings",
            "pBool": true,

            "fChild": {
              "fString": "first/hello"
            }
          }

Since info.fString exists, it spliced in as second/greetings and info.fChild.fString exists, so it's spliced in as first/hello. But the path template is expecting something matching first/* which second/greetings doesn't match. This is same issue with second/** not matching with first/hello.

The additional_binding for this RPC is /v1beta1/repeat/{info.fChild.fString=first/*}/{info.fString=second/*}/bool/{info.fBool}:childfirstpathresource and this will match the pathTemplate correctly.

Our logic is wrong in that:

1. It doesn't validate the match
2. Doesn't even have logic to check the additional_bindings

Error Logs:
Java side:

Caused by: com.google.api.client.http.HttpResponseException: 400 Bad Request
GET http://localhost:7469/v1beta1/repeat/second/greetings/first/hello/bool/false:pathresource?fInt64=0&info.pBool=true&intendedBindingUri=/v1beta1/repeat/%7Binfo.f_child.f_string%3Dfirst/*%7D/%7Binfo.f_string%3Dsecond/*%7D/bool/%7Binfo.f_bool%7D:childfirstpathresource&serverVerify=true&name=Binding%20testing%20additional%20binding&info.fString=second/greetings&fInt32=0&fDouble=0.0&info.fChild.fString=first/hello
{"error":{"code":400,"message":"unrecognized request: GET \"/v1beta1/repeat/second/greetings/first/hello/bool/false:pathresource?fInt64=0\u0026info.pBool=true\u0026intendedBindingUri=/v1beta1/repeat/%7Binfo.f_child.f_string%3Dfirst/*%7D/%7Binfo.f_string%3Dsecond/*%7D/bool/%7Binfo.f_bool%7D:childfirstpathresource\u0026serverVerify=true\u0026name=Binding%20testing%20additional%20binding\u0026info.fString=second/greetings\u0026fInt32=0\u0026fDouble=0.0\u0026info.fChild.fString=first/hello\"","details":null,"Body":"","Header":null,"Errors":null}}

Showcase Server:

2023/03/07 18:02:36 unrecognized request: GET "/v1beta1/repeat/second/greetings/first/hello/bool/false:pathresource?fInt64=0&info.pBool=true&intendedBindingUri=/v1beta1/repeat/%7Binfo.f_child.f_string%3Dfirst/*%7D/%7Binfo.f_string%3Dsecond/*%7D/bool/%7Binfo.f_bool%7D:childfirstpathresource&serverVerify=true&name=Binding%20testing%20additional%20binding&info.fString=second/greetings&fInt32=0&fDouble=0.0&info.fChild.fString=first/hello"

[blakeli0]

Thanks @lqiu96 for the finding! Can we confirm that we are expected to check the additional_binding? Is this implemented in other languages? I guess so if this is already implemented in the showcase server but want to confirm.
There are a lot of usages of additional_binding in googleapis, we should definitely fix it if this is confirmed to be a bug.

[lqiu96]

Thanks @lqiu96 for the finding! Can we confirm that we are expected to check the additional_binding? Is this implemented in other languages? I guess so if this is already implemented in the showcase server but want to confirm. There are a lot of usages of additional_binding in googleapis, we should definitely fix it if this is confirmed to be a bug.

I believe it is a bug since it's inside already inside the showcase test suite and it contains the input -> expected URI, but I'll need to reach out to other languages to confirm.

I see that Go currently isn't testing for this case: https://github.com/googleapis/gapic-generator-go/blob/main/showcase/compliance_test.go#L67

[lqiu96]

Took a look at how the Python team does it...

I see that Python have the overrides:
https://github.com/googleapis/python-dialogflow/blob/a03093dcf0a6fca77b2c9c9226d02458da1956cc/google/cloud/dialogflow_v2/services/versions/transports/rest.py#L769-L778

Similar to what we have in Java:
https://github.com/googleapis/google-cloud-java/blob/e7d648dd992a59df16cbdeb47623511f5ebb6d19/java-dialogflow/google-cloud-dialogflow/src/main/java/com/google/cloud/dialogflow/v2/stub/HttpJsonVersionsStub.java#L74-L83

---

Their parser:
https://github.com/googleapis/python-api-core/blob/5dfb6afda81bcc13e3432b020e144f7873fe1f4e/google/api_core/path_template.py#L250

• Loops through all the possible possible HttpRules, populates the path with the values, and tries validates validate the path.

Our Parser:
https://github.com/googleapis/gapic-generator-java/blob/79d986bd35b49a819dc875cfec69ec8685517930/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageRequestFormatter.java#L100

• Returns with the first path with whatever matches

---

I think there could be two steps to resolving this issue:

1. Loop through all the possible paths (first the initial PathTemplate and then through all the additionalPaths). Need to validate at each path and ensure that it matches correctly. If we find one that matches correctly, we can return the path. Throw an exception if nothing matches.
2. Add in logic to add custom pathVarsExtractors for each additional_binding. Each additionalPaths could take in their own unique FieldsExtractor as show in the showcase test above.

I think Step 2 could be left as a stretch for now given that most apis should have one FieldsExtractor that works for all additional_bindings.

[lqiu96]

Leaving this issue open. I have closed Part 2 of the fix, but can re-open in the future.

[alicejli]

@lqiu96 Just checking in on this - did we determine an approach we want to take with handing this annotation? Or is that still to be designed? Wondering whether to keep this open.

[lqiu96]

I believe #1565 has the initial code changes for the approach. Iirc, we decided to not pursue this (as of right now) since we have other priorities. I kept this open to track if we got an customer issue as a result of this behavior.

Probably can downgrade the priority to p3.