fix: Allow custom HttpRules for REST LROs #1288

lqiu96 · 2023-01-31T23:16:31Z

This is both a gapic-generator and gax-java change:

Gapic Generator has a map of the Custom Http Bindings inside HttpJson{Service}Stub.java and passes it to the HttpJsonOperationStub. The generator parses the ServiceYaml file and recreates the HttpRules to be sent over.
Gax Java will update the rawPath and additional binding values for Operation (based on values received via HttpJsonOperationClient)

For example in Java-Speech generated client:

    this.httpJsonOperationsStub =
        HttpJsonOperationsStub.create(
            clientContext,
            callableFactory,
            typeRegistry,
            ImmutableMap.<String, HttpRule>builder()
                .put(
                    "google.longrunning.Operations.GetOperation",
                    HttpRule.newBuilder().setGet("/v1/operations/{name=**}").build())
                .put(
                    "google.longrunning.Operations.ListOperations",
                    HttpRule.newBuilder().setGet("/v1/operations").build())
                .build());
...

Output:

Running with REST...
Transcript: how old is the Brooklyn Bridge -> No longer errors out

…rations

blakeli0 · 2023-02-01T18:55:53Z

...test/java/com/google/api/generator/gapic/composer/rest/goldens/HttpJsonComplianceStub.golden

@@ -350,6 +351,9 @@ public class HttpJsonComplianceStub extends ComplianceStub {
                  .build())
          .build();

+  private static final Map<String, String> operationCustomHttpBindings =


We probably don't want to create unused variables if we can avoid it in the generator.

…to v1.15.0 (#1278)

blakeli0 · 2023-02-02T00:35:56Z

...pjson/src/main/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStub.java

@@ -72,8 +72,14 @@
 public class HttpJsonOperationsStub extends OperationsStub {
  private static final Pattern CLIENT_PACKAGE_VERSION_PATTERN =
      Pattern.compile("\\.(?<version>v\\d+[a-zA-Z]*\\d*[a-zA-Z]*\\d*)\\.[\\w.]*stub");
-
-  private static final ApiMethodDescriptor<ListOperationsRequest, ListOperationsResponse>
+  private static final String LRO_LIST_OPERATIONS = "google.longrunning.Operations.ListOperations";


Are these all possible operations? Judging from the code in the composer, looks like there could be patch etc.?

There are only 5 operations (and 4 of them have http annotations): https://github.com/googleapis/googleapis/blob/c8a382b9980b59a241a561aef1599980209a5e87/google/longrunning/operations.proto#L67-L122

blakeli0 · 2023-02-02T00:36:47Z

...main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java

+  }
+
+  private String getValueBasedOnPatternCase(HttpRule httpRule) {
+    switch (httpRule.getPatternCase().getNumber()) {


It would be good if we can provide a reference to the http proto for these mappings.

I'll remember to add it in later. I'm not sure if this is the best approach yet, so I haven't been adding any docs or tests yet.

blakeli0 · 2023-02-02T00:43:56Z

...main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java

@@ -1103,6 +1143,43 @@ protected List<Expr> createOperationsStubInitExpr(
            .build());
  }

+  private Map<String, String> parseCustomHttpBindings(GapicContext context) {
+    Map<String, String> customHttpBindings = new HashMap<>();


We can add all the default operation -> url mapping to this map and override then if needed. Basically we need to move as much code as possible to the generator from gax, as gax is a runtime dependency and it would be much harder to make changes there later.

We can add the defaults for bunch of mixins (or for only Operation module). I just saw that it is possible to have way more mixins (https://github.com/googleapis/gapic-showcase/blob/b94ecfc51059b49770e5bdb6f0d7ea07903158e8/schema/google/showcase/v1beta1/showcase_v1beta1.yaml#L46-L85) so I didn't want to just create a default list in the gapic-generator that needed to be manually updated later on.

I see, I think that makes sense, we could do it the other way also, construct a map with all the bindings from the yaml, and populate default for the LRO operations. Or if we are confident about the list of mixins, we could pre-populate them as well, a related question, did you get a chance to see how mixins work for location and iam? Are we going to have similar problem for them?

From what I can see, I think only the Operation proto is currently being used. I would imagine if Location and IAM are used, they would be created the same way the Operations Client was in GAX: googleapis/gax-java#1456 and we would reference it via https://github.com/googleapis/google-cloud-java/blob/10bb0cb494f64b864408ede46834e1046351370c/java-speech/google-cloud-speech/src/main/java/com/google/cloud/speech/v1/SpeechClient.java#L164-L166.

I was running under the assumption that they would be added in sometime in the future, but I'm not sure about it. Plus each generated client (operation/ iam/ location) would have the initial defaults set from the proto file: https://github.com/googleapis/gapic-generator-java/blob/d1a16195937df041f65a52717ddf9dc6ceb09b4f/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStub.java#L84

blakeli0 · 2023-02-02T00:45:34Z

...ava/gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageRequestFormatter.java

@@ -147,6 +147,12 @@ public Builder<RequestT> setAdditionalPaths(String... rawAdditionalPaths) {
      return this;
    }

+    @InternalApi
+    public Builder<RequestT> updateRawPath(String rawPath) {


Is it possible to initialize the ProtoMessageRequestFormatter with the updated path instead of overriding it later?

The ProtoMessageRequestFormatter is created as part of the ApiMethodDescriptor static fields HttpJsonOperationStub: https://github.com/googleapis/gapic-generator-java/blob/b7ca95f12dfe8287c133e09534be1fc46882ce6c/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStub.java#L76-L108

I might be missing something obvious, but I'm not seeing a way to change this value without updating once I get the customHttpBindings map.

blakeli0 · 2023-02-02T00:48:42Z

...pjson/src/main/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStub.java

+  }
+
+  public static final HttpJsonOperationsStub create(
+      OperationsStubSettings settings, Map<String, String> customOperationHttpBindings)


This approach is fine but I would try to explore the possibility of add the customOperationHttpBindings to settings or clientContext. It would be cleaner and we don't have to have so many overloaded methods.

(From what I understand) The core of the logic which we can change in the generated library comes from:

protected HttpJsonSpeechStub( SpeechStubSettings settings, ClientContext clientContext, HttpJsonStubCallableFactory callableFactory) throws IOException { this.callableFactory = callableFactory; this.httpJsonOperationsStub = HttpJsonOperationsStub.create( clientContext, callableFactory, typeRegistry, ...

at https://github.com/googleapis/google-cloud-java/blob/9c0ddf2879b0e3ddf29b5d67cd44f0336f492134/java-speech/google-cloud-speech/src/main/java/com/google/cloud/speech/v1/stub/HttpJsonSpeechStub.java#L181-L188

The SpeechStubSettings settings parameter is different from OperationsStubSettings which is what HttpJsonOperationsStub.create() is expecting. The other approach I'm looking at is to try and add it either as part of the ClientContext (currently leaning towards not putting it here as it doesn't seem to be related) or the HttpJsonStubCallableFactory (Need to do a bit more digging into this).

The overloaded methods were the easiest way I could get the CI builds to pass. Ideally, I would just have each of the .create() methods take in another Map parameter (and not to duplicate the .create() methods), but I'm having issue with the build passing. I can show you this tomorrow.

…downstream test (#1291) * ci: use java-shared-dependencies in google-cloud-java * No need to modify google-cloud-jar-parent

…figurations for netty classes (#1290) * fix(java): initialize netty-shaded at run-time and add reflection configurations for netty classes

* ci(showcase): disable rest_numeric_enum feature in showcase tests

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

Pin Bazel version to 5.2.0 as googleapis already updated to 5.2.0

Bumps [cryptography](https://github.com/pyca/cryptography) from 38.0.3 to 39.0.1. - [Release notes](https://github.com/pyca/cryptography/releases) - [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst) - [Commits](pyca/cryptography@38.0.3...39.0.1) --- updated-dependencies: - dependency-name: cryptography dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Mridula <66699525+mpeddada1@users.noreply.github.com>

* chore: (cleanup) removing unused files * chore: working on comments

…a-common-protos, and java-iam (#1305) The build is Maven or Bazel. The Gradle files in the recently migrated repositories (api-common-java, gax-java, java-common-protos, and java-iam) are not used. Note that this pull request is not touching rules_java_gapic/resources/gradle which is still used to generate Gradle files for self-service client libraries.

blakeli0 · 2023-03-07T21:00:15Z

gapic-generator-java/src/test/resources/showcase_v1beta1.yaml

+    - selector: google.longrunning.Operations.ListOperations
+      get: '/v1beta1/operations'
+      additional_bindings:
+        - get: '/v1/operations'


Is it possible to have multiple additional bindings here? If yes, can we add more here to cover the test case?

In addition, do you know how additional_bindings are used in LROs or is it even used? If not, we probably can get rid of the duplicated logics.

Yep, I'll add more bindings for this. Thanks!

In addition, do you know how additional_bindings are used in LROs or is it even used? If not, we probably can get rid of the duplicated logics.

I've actually just been doing a bit of digging into as I've been running into some showcase issues with additional_bindings. For httpjson, I don't think there currently is any logic to handles checking other bindings once the first one doesn't match: https://github.com/googleapis/gapic-generator-java/blob/c23f981e2ac3c573bed51e725dc7061551179400/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpRequestRunnable.java#L184

I don't see any logic that handles checking the other paths/ additional_bindings which I think is a bug. ~~For grpc, I'll need to do some more digging.~~ It doesn't seem like grpc uses the paths.

Filed a bug here: #1440

Thanks Lawrence, that bug looks generic for all additional_bindings, anything special for the OperationClient? Or as long as it's REST, there is no differences between OperationClient and other clients?

Yeah, the bug is generic for all additional_bindings and it's a very specific edge case.

There is nothing special for OperationsClient, but I just added multiple additional_bindings to the Operations Client for showcase.

blakeli0 · 2023-03-08T20:44:18Z

...n/src/test/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStubTest.java

+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class HttpJsonOperationsStubTest {


This test can probably use some refactoring. One best practice for sharing test object is that we can create it every time in setUp and reference the object later, similar to this test file.

I originally had an HttpJsonOperationStub object inside this test, but the method to get the MethodDescriptors is static: https://github.com/googleapis/gapic-generator-java/blob/0ccf457a774bd3dbe3b101d82222bdc39f6e5b0e/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStub.java#L331 and I had to invoke it with HttpJsonOperationStub.getMethodDescriptors().

Is there a way to test this so that I can create an Stub object in the test and then reference it in my tests?

blakeli0 · 2023-03-08T20:45:40Z

...n/src/test/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStubTest.java

+                .getAdditionalPathTemplates()
+                .get(0)
+                .toRawString())
+        .isEqualTo("testList2");


Verifying a value that was set up in setUp method is also not a good practice, see here

This test tries to cover the updateDefaultApiMethodDescriptors function. Perhaps the easier/ cleaner way for this is to just open method as default scope and test it that way?

blakeli0 · 2023-03-08T20:51:29Z

showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/HttpJsonEchoStub.java

@@ -366,7 +368,24 @@ protected HttpJsonEchoStub(
      throws IOException {
    this.callableFactory = callableFactory;
    this.httpJsonOperationsStub =
-        HttpJsonOperationsStub.create(clientContext, callableFactory, typeRegistry);
+        HttpJsonOperationsStub.create(


I guess the actual showcase integration tests that verified the interactions will be done in this PR?

Going forward, the interactions will be covered in that PR. For this PR, I just generated the speech client locally and tested that Operations works.

blakeli0 · 2023-03-08T21:07:27Z

...main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java

+      case 5:
+        return httpRule.getDelete();
+      default:
+        throw new IllegalArgumentException(


I would probably write some true unit tests for this method and the method above, as the input and output are very easy to mock and verify, but I know they are mostly already covered in the golden unit tests, so as long as we covered all the cases, it's fine to me.
However, this branch couldn't be covered by golden tests, so we should add a unit test for it(by changing the scope of this method to default).

However, this branch couldn't be covered by golden tests, so we should add a unit test for it

I'm a bit lost of this part. The Echo goldens for rest should have it right?
https://github.com/googleapis/gapic-generator-java/blob/88487e38f86494e2ba18d4bf7c7f23c6671aeb24/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/HttpJsonEchoStub.golden#L360-L405

sonarcloud · 2023-03-09T22:06:56Z

Please retry analysis of this Pull-Request directly on SonarCloud.

blakeli0 · 2023-03-10T18:45:16Z

.../src/test/java/com/google/api/generator/gapic/composer/grpcrest/GrpcRestTestProtoLoader.java

@@ -58,6 +60,13 @@ public GapicContext parseShowcaseEcho() {
    ServiceDescriptor echoServiceDescriptor = echoFileDescriptor.getServices().get(0);
    assertEquals("Echo", echoServiceDescriptor.getName());

+    String serviceYamlFileName = "showcase_v1beta1.yaml";


I probably missed this in previous reviews, instead of showcase_v1beta1.yaml, can we rename it to reflect the fact that this yaml is only used by Echo at this point?

Sure, can rename to echo_v1beta.yaml. We can rename it back to showcase_v1beta1.yaml if other tests need to use this file again for other showcase protos.

blakeli0 · 2023-03-10T18:54:36Z

...pjson/src/main/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStub.java

-        .build();
+  /* This function returns the list of method descriptors (custom or default) */
+  @InternalApi
+  public List<ApiMethodDescriptor> getAllMethodDescriptors() {


We can make remove the public here since it's only used in tests, we should also make that clear in the comment.

MockHttpService requires this in OperationsClientTest. I'll add @VisibleForTesting annotation here to mark its intended use case.

blakeli0 · 2023-03-10T18:56:04Z

...pjson/src/main/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStub.java

-    if (apiVersion == null) {
-      return methodDescriptor;
+  /* OperationsClient's RPCs are mapped to GET/POST/DELETE and this function only expects those HttpVerbs to be used */
+  private static String getValueBasedOnPatternCase(HttpRule httpRule) {


This method does not have to be static now.

blakeli0 · 2023-03-10T18:59:07Z

...n/src/test/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStubTest.java

+    List<ApiMethodDescriptor> apiMethodDescriptorList =
+        httpJsonOperationsStub.getAllMethodDescriptors();
+    assertThat(apiMethodDescriptorList.get(0).getRequestFormatter().getPathTemplate().toRawString())
+        .isEqualTo("testList");


We could extract this string to a variable in the method or static field in the test class, since they are reused in the setup and verification. Same thing for the test below.

blakeli0

LGTM

sonarcloud · 2023-03-10T20:41:42Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

96.6% Coverage
0.0% Duplication

lqiu96 added 7 commits January 31, 2023 00:34

fix: Allow custom http bindings for LROs

4c14e48

fix: Update generator and GAX to support custom HTTP Bindings for ope…

8f5e313

…rations

fix: Use static Map for custom Operation REST Http Bindings

a693085

chore: Update golden test cases

f4baa0a

chore: Update showcase tests

7bde2cf

chore: Update golden ITs

a3527e1

chore: Add back origin HttpJsonOperationStub.create() methods

2e1acbc

blakeli0 reviewed Feb 1, 2023

View reviewed changes

renovate-bot and others added 11 commits February 1, 2023 14:47

fix(deps): update dependency com.google.auth:google-auth-library-bom …

5f3dfe2

…to v1.15.0 (#1278)

doc: Update DEVELOPMENT.md for local development. (#1237)

d8761bc

chore: Create a default mapping in OperationStub

d837f97

chore: Do not generate custom bindings if there are none

dfd1e5a

chore: Update golden units

0a891dc

Merge branch 'main' into main-fix_custom_LRO_httpbindings

51bde57

chore: Update all test cases

64fc726

chore: Fix format issues

09031c8

fix: remove constant operation binding field

568ad44

chore: Clean up code

3a72781

chore: Resolve sonar comments

d7d1b12

blakeli0 reviewed Feb 2, 2023

View reviewed changes

meltsufin and others added 10 commits February 23, 2023 12:42

chore: DEVELOPMENT.md formatting fix (#1289)

9b050d3

ci: use java-shared-dependencies in google-cloud-java repository for …

5b31d41

…downstream test (#1291) * ci: use java-shared-dependencies in google-cloud-java * No need to modify google-cloud-jar-parent

fix(java): initialize netty-shaded at run-time and add reflection con…

316621c

…figurations for netty classes (#1290) * fix(java): initialize netty-shaded at run-time and add reflection configurations for netty classes

ci(showcase): disable rest_numeric_enum for showcase testing (#1284)

d8e488f

* ci(showcase): disable rest_numeric_enum feature in showcase tests

chore(main): release 2.15.0 (#1269)

46156e0

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

chore(main): release 2.15.1-SNAPSHOT (#1292)

8a3ba93

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

chore: Pin Bazel version to 5.2.0 (#1304)

1cd51fb

Pin Bazel version to 5.2.0 as googleapis already updated to 5.2.0

chore: (cleanup) removing unused files (#1265)

443adec

* chore: (cleanup) removing unused files * chore: working on comments

blakeli0 reviewed Mar 7, 2023

View reviewed changes

lqiu96 added 2 commits March 7, 2023 16:43

chore: Update to have multiple additional_bindings

31932b1

Merge branch 'main' into main-fix_custom_LRO_httpbindings

88487e3

blakeli0 reviewed Mar 8, 2023

View reviewed changes

lqiu96 added 5 commits March 8, 2023 17:13

chore: Update grpcrest golden test to include httprule

4f6e39c

Merge branch 'main' into main-fix_custom_LRO_httpbindings

8d4d54c

chore: HttpJsonOperationsStub's MethodDescriptors are not static

9a4f437

chore: Add unit tests for HttpJson Operations logic

350b811

Merge branch 'main' into main-fix_custom_LRO_httpbindings

bfe3753

lqiu96 added 3 commits March 9, 2023 17:30

chore: Resolve lint issues

1d5d967

Merge branch 'main' into main-fix_custom_LRO_httpbindings

fe6c4c7

chore: Clean up test code

c15e2b5

blakeli0 reviewed Mar 10, 2023

View reviewed changes

lqiu96 added 3 commits March 10, 2023 15:19

chore: Resolve pr comments

2c489ff

chore: Add VisibleForTesting annotation

78be800

Merge branch 'main' into main-fix_custom_LRO_httpbindings

f179078

lqiu96 added the automerge Merge the pull request once unit tests and other checks pass. label Mar 10, 2023

blakeli0 approved these changes Mar 10, 2023

View reviewed changes

lqiu96 merged commit f8ccd2a into main Mar 10, 2023

lqiu96 deleted the main-fix_custom_LRO_httpbindings branch March 10, 2023 22:35

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Mar 10, 2023

release-please bot mentioned this pull request Mar 10, 2023

chore(main): release 2.15.3 #1445

Merged

fix: Allow custom HttpRules for REST LROs #1288

fix: Allow custom HttpRules for REST LROs #1288

Conversation

lqiu96 commented Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

lqiu96 Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 10, 2023

lqiu96 commented Jan 31, 2023 •

edited

Loading

lqiu96 Feb 2, 2023 •

edited

Loading

lqiu96 Feb 2, 2023 •

edited

Loading

lqiu96 Feb 2, 2023 •

edited

Loading

lqiu96 Feb 2, 2023 •

edited

Loading

lqiu96 Mar 7, 2023 •

edited

Loading

blakeli0 Mar 8, 2023 •

edited

Loading

lqiu96 Mar 8, 2023 •

edited

Loading

blakeli0 Mar 10, 2023 •

edited

Loading