feat: Numeric enums in routing headers

[ddixit14]

Solves - #2368 (Numeric enums in implicit routing headers)
FYI, only implicit routing headers can be fixed currently because explicit routing headers are implemented as strings by design. See this doc for more details.

[ddixit14]

After commit d63e854, the local testing shows this O/P. Getting "getFKingdomValue" in the method name.

[ddixit14]

Code changes description:
createRequestFieldGetterExpr() is being called in both cases (routing_header rule present v/s absent) to map the parameters.

In first case (routingHeaderRule() == null, createRequestParamsExtractorBodyForHttpBindings() is called), createRequestFieldGetterExpr() is modified to accept an extra boolean parameter, isFieldEnum(). And if the field is enum, I am adding an extra “Value” to the fieldMethod name (https://github.com/googleapis/sdk-platform-java/pull/2328/files#diff-13cc1d69966fca5c433b2c243aa4e8414520cf05e295f02ed52f2fe5097cac21R1474) during the last iteration of the descendant fields.

In the 2nd case when routingHeaderRule()!=null, I am sending false in isFieldEnum() because routingHeaderParam does not have anything to check if it is an enum or not (or is there? I am not able to find it).

[ddixit14]

Since this only solves the implicit routing headers, can you please create a separate issue to track it and update the description of this PR accordingly? Leaving a comment at #1966 would be great as well.

Done. Here is the issue created - #2368

[lqiu96]

FYI @ddixit14, we have Showcase ITs for Dynamic Routing Headers at https://github.com/googleapis/sdk-platform-java/blob/762c125abadb5e56682224c9f1587c71e5c6e653/showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITDynamicRoutingHeaders.java

I think the compliance.proto has a enum field that you can possibly test against. It may be possible to add a test to intercept the the enum value is sent as an int value in the headers.

[lqiu96]

nit: Rename this to be a bit more descriptive. Something like testGrpc_implicitHeaders_enumsEncodedasInt() to describe that this test covers specifically enums -> int.

[lqiu96]

Also, can you add an httpjson test case as well. I think it affects both transports. Ignore this comment if it only affects gRPC.

[blakeli0]

nit: Rename this to be a bit more descriptive. Something like testGrpc_implicitHeaders_enumsEncodedasInt() to describe that this test covers specifically enums -> int.

+1000 to this. See unit tests naming for details.

[ddixit14]

renamed the tests and added httpjson case as well. thank you.

[lqiu96]

Also, we talked earlier about possibly enhancing echo.proto to also test for some explicit + implicit dynamic routing enum use cases. I took another look into echo.proto and I think it makes sense since a bulk of the routing header test cases are there: https://github.com/googleapis/gapic-showcase/blob/330a3a25eb8c4d0948860f1898fe7dcc05dc4ea5/schema/google/showcase/v1beta1/echo.proto#L48-L86

I think you can create an issue in the gapic-showcase repo and someone can pick it up in the future.

[lqiu96]

I added a few more nits but overall LGTM to me. Please resolve the nits and let @blakeli0 to take another look before merging. Thanks for the PR!

[blakeli0]

Also, we talked earlier about possibly enhancing echo.proto to also test for some explicit + implicit dynamic routing enum use cases. I took another look into echo.proto and I think it makes sense since a bulk of the routing header test cases are there: https://github.com/googleapis/gapic-showcase/blob/330a3a25eb8c4d0948860f1898fe7dcc05dc4ea5/schema/google/showcase/v1beta1/echo.proto#L48-L86

I think you can create an issue in the gapic-showcase repo and someone can pick it up in the future.

If we already have a test case in compliance.proto, what is the advantage of adding one to echo.proto? Is it mostly for simplifying our showcase tests, so that we don't have to create a separate ComplianceClient? If it's for latter, I don't think we have to add one.
Ideally the showcase protos should be grouped into features, so that routing header test cases can be in a dedicated proto. Before that happens, I think having implicit routing header test case in a separate proto is OK.

[lqiu96]

Also, we talked earlier about possibly enhancing echo.proto to also test for some explicit + implicit dynamic routing enum use cases. I took another look into echo.proto and I think it makes sense since a bulk of the routing header test cases are there: https://github.com/googleapis/gapic-showcase/blob/330a3a25eb8c4d0948860f1898fe7dcc05dc4ea5/schema/google/showcase/v1beta1/echo.proto#L48-L86
I think you can create an issue in the gapic-showcase repo and someone can pick it up in the future.

If we already have a test case in compliance.proto, what is the advantage of adding one to echo.proto? Is it mostly for simplifying our showcase tests, so that we don't have to create a separate ComplianceClient? If it's for latter, I don't think we have to add one. Ideally the showcase protos should be grouped into features, so that routing header test cases can be in a dedicated proto. Before that happens, I think having implicit routing header test case in a separate proto is OK.

For now, it's mainly for the latter reason. Agreed that ideally we would have a separate showcase proto for routing headers, but it looks like the explicit headers are mostly contained in the echo proto. It's not urgent to add an enum to the echo proto, but a smaller task to enable showcase tests for more routing headers use cases is to add it there (especially for any future explicit enhancements/ improvements). In the future/whenever someone has the time, we can create a new proto specifically for routing headers.

[sonarcloud]

Quality Gate passed for 'gapic-generator-java-root'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
94.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[sonarcloud]

Quality Gate passed for 'java_showcase_integration_tests'

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud