Feat: Implement DIREGAPIC LRO annotations #832

GabrielGonzalezDiaz · 2021-08-30T19:42:53Z

Generated methods that were previously hard coded now use the new annotations.
Annotations implemented:
-operation_polling_service
-operation_service
-operation_field
-operation_request_field
-operation_response_field

The parts which appends fields for opName are in alphabetical order based on the field name and read from annotations
The .get[FIELD] methods are also read from annotations.

.setOperationSnapshotFactory(
    (DeleteAddressRequest request, Operation response) -> {
      StringBuilder opName = new StringBuilder(response.getName());
      opName.append(":").append(request.getProject());
      opName.append(":").append(request.getRegion());
      return HttpJsonOperationSnapshot.newBuilder()
          .setName(opName.toString())
          .setMetadata(response)
          .setDone(response.getStatus().equals(Status.DONE))
          .setResponse(response)
          .setError(response.getHttpErrorStatusCode(), response.getHttpErrorMessage())
          .build();

The .set[FIELD] methods are read from annotations. The order is as follows: First go the fields that have the operation_response_field annotation, and they are listed alphabetically based on the value of the annotation. The fields that do not have the annotation are listed alphabetically based on the name of the field itself.

.setPollingRequestFactory(
    compoundOperationId -> {
      List<String> idComponents = Arrays.asList(compoundOperationId.split(":"));
      return GetRegionOperationRequest.newBuilder()
          .setOperation(idComponents.get(0))
          .setProject(idComponents.get(1))
          .setRegion(idComponents.get(2))
          .build();
    })

Fixed logic to determine which methods require new LRO machinery using operation_polling_method and operation_service.

if (protoMethod.isOperationPollingMethod() || protoMethod.operationService() != null) {
  expr =
      methodMaker
          .apply(
              "setOperationSnapshotFactory",
              setOperationSnapshotFactoryExpr(protoMethod, messageTypes))
          .apply(expr);
}
if (protoMethod.isOperationPollingMethod()) {
  expr =
      methodMaker
          .apply(
              "setPollingRequestFactory",
              setPollingRequestFactoryExpr(protoMethod, messageTypes))
          .apply(expr);
}

Data model classes Message.java and Method.java were changed to hold the new data from the annotations

codecov · 2021-08-31T18:10:14Z

Codecov Report

❗ No coverage uploaded for pull request base (diregapic-lro@00d94ed). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             diregapic-lro     #832   +/-   ##
================================================
  Coverage                 ?   89.43%           
================================================
  Files                    ?      141           
  Lines                    ?    15518           
  Branches                 ?     1113           
================================================
  Hits                     ?    13878           
  Misses                   ?     1332           
  Partials                 ?      308

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 00d94ed...2bb8199. Read the comment docs.

vam-google

Overall looks good! A few initial comments. I will do a more thorough review later today.

vam-google · 2021-08-31T18:34:45Z

...om/google/api/generator/gapic/composer/rest/HttpJsonServiceCallableFactoryClassComposer.java

@@ -121,11 +120,21 @@ protected MethodDefinition createOperationCallableMethod(TypeStore typeStore) {

    List<Statement> createOperationCallableBody = new ArrayList<Statement>(2);

-    List<VariableExpr> arguments = method.arguments();
+    List<VariableExpr> arguments = new ArrayList<>(method.arguments());
+    // arguments.set(


Please remove all comments like this (debugging artifacts)

Still here...

vam-google · 2021-08-31T18:36:02Z

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

@@ -380,9 +384,74 @@ protected Expr createTransportSettingsInitExpr(
    return Collections.singletonList(expr);
  }

+  // Generates expr.methodName();
+  private MethodInvocationExpr invokeMethodfromVar(Expr expr, String methodName) {


YOu probably don't need this method and use the MethodInvocationExpr construction code directly where you need it It will make it a bit more verbose, but oddly enough it will make it more idiomatic for gapic-generator-java. The same applies to the staticBuilder() method below

vam-google · 2021-08-31T18:43:07Z

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

+    ImmutableSet<String> fieldNames = inputOperationMessage.fieldMap().keySet();
+    ArrayList<String> nonResponseAnnotationFields = new ArrayList<String>();
+    for (String fieldName : fieldNames) {
+      if (!responseFields.containsValue(fieldName)) {


This is not correct usage of maps. map.containsValue() will give you the linear time (do not confuse with containsKey(), which will give amortized O(1)).

This is not critical for this application, since there will never be many elements in the map, but basically if you need both keys and values to be indexed, you may consider using two-way index (direct and reverse, assuming that values are also unique).

I created a collection used regular contains. I tried to find the runtime for this and couldn't find it, but I believe it is O(1)

Collection is just an interface it can have many implementations. Here you did the same thing but in a more convoluded synax: responseFieldName = map.values() - which is still a non-indexed "array" of values. In other words, the proposed solution is exactly the same as it was, but a more verbose.

In IntelliJ you can go to the SDK library classes implementations the same way as you go to your code implementation (ctrl + lef_mouse_click in most basic case). Reading the code is usually the best way to asses how it works and which time properties it has.

Basically only key operations are O(1) in maps.

Side note: Please consider posting code as text instead of posting as a picture (it makes it mucheasier to work with

There is a way to post it higlighted:

public static void main(String[] args) {}

vam-google · 2021-08-31T18:43:43Z

src/main/java/com/google/api/generator/gapic/model/OperationResponse.java

+import com.google.auto.value.AutoValue;
+import javax.annotation.Nullable;
+
+// In composer:


Please remove this, and all similar artifacts in comments.

vam-google · 2021-08-31T18:44:48Z

src/main/java/com/google/api/generator/gapic/protoparser/Parser.java

@@ -663,6 +697,14 @@ private static boolean isMapType(Descriptor messageDescriptor) {
                      /* protoPakkage */ protoMethod.getFile().getPackage(),
                      serviceDescriptor.getName(),
                      protoMethod.getName());
+      boolean operationPollingMethod = false;
+      if (protoMethod.getOptions().hasExtension(ExtendedOperationsProto.operationPollingMethod))


Please never use if statements without {}. If you have ever used them like that elswere, please add curly brackets there too.

vam-google

For the polling stubs (HttpJsonRegionOprationStub, i.e. the ones marked with
operation_polling_method = true these parts should be generated too:

this.longRunningClient =
        new HttpJsonLongRunningClient<GetRegionOperationRequest, Operation>(
            getCallable,
            getMethodDescriptor.getOperationSnapshotFactory(),
            getMethodDescriptor.getPollingRequestFactory());

@Override
  public LongRunningClient longRunningClient() {
    return longRunningClient;
  }

private final LongRunningClient longRunningClient;

Apart from the stuff above and some other relatively minor comments, looks good!

vam-google · 2021-09-01T02:11:16Z

repositories.bzl

@@ -76,6 +76,15 @@ def gapic_generator_java_repositories():
        ],
    )

+    #    local(


Please remove

vam-google · 2021-09-01T02:11:24Z

...om/google/api/generator/gapic/composer/rest/HttpJsonServiceCallableFactoryClassComposer.java

@@ -121,11 +120,21 @@ protected MethodDefinition createOperationCallableMethod(TypeStore typeStore) {

    List<Statement> createOperationCallableBody = new ArrayList<Statement>(2);

-    List<VariableExpr> arguments = method.arguments();
+    List<VariableExpr> arguments = new ArrayList<>(method.arguments());
+    // arguments.set(


Still here...

vam-google · 2021-09-01T02:13:40Z

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

    TypeNode statusType =
        TypeNode.withReference(
            VaporReference.builder()
                .setName("Status")
-                .setPakkage("com.google.cloud.compute.v1")
+                .setPakkage("com.google.cloud.compute.v1.Operation")


All explicit mentions of "compute" must be gone. Why is this still needed?
Please check for all other mentions of compute in your codebase. There should be 0 mentions of compute in gapic-generator-java codebase.

So status is generated somewhere else in compute, and is used in this (genrated) class. I am not using compute to generate anything. This is a Vapor reference and that is just the package that will be imported in the generated class.

See: https://github.com/vam-google/google-cloud-compute-small-v1-lro/blob/7ff5244a579476088a222c50a7155e47e107a277/gapic-google-cloud-compute-small-v1-java/src/main/java/com/google/cloud/compute/v1/stub/HttpJsonAddressesStub.java#L46

vam-google · 2021-09-01T02:21:27Z

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

+    ImmutableSet<String> fieldNames = inputOperationMessage.fieldMap().keySet();
+    ArrayList<String> nonResponseAnnotationFields = new ArrayList<String>();
+    for (String fieldName : fieldNames) {
+      if (!responseFields.containsValue(fieldName)) {


Collection is just an interface it can have many implementations. Here you did the same thing but in a more convoluded synax: responseFieldName = map.values() - which is still a non-indexed "array" of values. In other words, the proposed solution is exactly the same as it was, but a more verbose.

In IntelliJ you can go to the SDK library classes implementations the same way as you go to your code implementation (ctrl + lef_mouse_click in most basic case). Reading the code is usually the best way to asses how it works and which time properties it has.

Basically only key operations are O(1) in maps.

Side note: Please consider posting code as text instead of posting as a picture (it makes it mucheasier to work with

There is a way to post it higlighted:

public static void main(String[] args) {}

vam-google · 2021-09-01T02:29:03Z

...tegration/goldens/compute/com/google/cloud/compute/v1/stub/HttpJsonRegionOperationsStub.java

-                    StringBuilder opName = new StringBuilder(response.getId());
-                    opName.append(":").append(request.getProject());
-                    opName.append(":").append(request.getRegion());
+                    StringBuilder opName = new StringBuilder(response.getName());


Sorry, I missed it during the previous review, can you generate the line 92 a bit differently. Instead of
response.getStatus().equals(Status.DONE)
do it in reverse instead?
Status.DONE.equals(response.getStatus())

This should give the same result but is safer, because the second variant will not produce null pointer exception if response.getStatus() is null (it is a general well known trick of doing comparisons in reverse)

vam-google · 2021-09-01T02:38:18Z

src/main/java/com/google/api/generator/gapic/protoparser/Parser.java

@@ -663,6 +697,16 @@ private static boolean isMapType(Descriptor messageDescriptor) {
                      /* protoPakkage */ protoMethod.getFile().getPackage(),
                      serviceDescriptor.getName(),
                      protoMethod.getName());
+      boolean operationPollingMethod = false;
+      if (protoMethod.getOptions().hasExtension(ExtendedOperationsProto.operationPollingMethod)) {


consider using ternary operator (?) for stuff like this (this is basically its mos common usage pattern).

vam-google

LGTM, assuming you will add those few missing pieces which we discussed offline

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [importlib-metadata](https://github.com/python/importlib_metadata) | `==4.8.3` -> `==4.12.0` | [![age](https://badges.renovateapi.com/packages/pypi/importlib-metadata/4.12.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/importlib-metadata/4.12.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/importlib-metadata/4.12.0/compatibility-slim/4.8.3)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/importlib-metadata/4.12.0/confidence-slim/4.8.3)](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. ⚠ **Warning**: custom changes will be lost. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-shared-dependencies).

The module itself has been removed in #820.

GabrielGonzalezDiaz added 14 commits August 17, 2021 17:26

fix ServiceStub Goldens

570bb33

fix Stub golden

cdb67da

fix Stub golden

2430989

fix CallableFactory golden

6b15e3b

fix java format

9ed928a

add annotation placement comments

edbe1a4

only add machinery to methods that return operation

e1d5e83

add grpc file that contained method that was edited on abstract class

703c123

update HttpJsonComplianceStub.golden

67b42bd

java format

703fc37

add initial methods from annotations

8dbd36b

add initial methods from annotations

f9f9c66

fix merge errors

772ca7e

Implement annotations

0657d8c

GabrielGonzalezDiaz requested a review from vam-google August 30, 2021 19:42

GabrielGonzalezDiaz requested review from miraleung and a team as code owners August 30, 2021 19:42

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 30, 2021

java format

5c4c626

GabrielGonzalezDiaz removed the request for review from miraleung August 30, 2021 19:52

GabrielGonzalezDiaz added 5 commits August 30, 2021 21:09

fix package for Status

b02303c

fix CallableFactory generics

f827fc6

java format

50815a9

initialize new fields in message for parser test

8867470

set default value for operation_polling_method

ae11f55

GabrielGonzalezDiaz changed the title ~~Chore: Implement annotations~~ Feat: Implement annotations Aug 31, 2021

GabrielGonzalezDiaz changed the title ~~Feat: Implement annotations~~ Feat: Implement LRO annotations Aug 31, 2021

vam-google reviewed Aug 31, 2021

View reviewed changes

add brackets to if statement

045c6da

remove comments and invoke methods

25bcb10

GabrielGonzalezDiaz changed the title ~~Feat: Implement LRO annotations~~ Feat: Implement DIREGAPIC LRO annotations Sep 1, 2021

GabrielGonzalezDiaz added 2 commits September 1, 2021 02:13

add brackets to if statements and remove invoke methods

85ac40b

java formet

3fbb746

vam-google reviewed Sep 1, 2021

View reviewed changes

vam-google approved these changes Sep 2, 2021

View reviewed changes

GabrielGonzalezDiaz added 2 commits September 3, 2021 16:15

add longRunningClient

85368d9

callablefactory fixes

2bb8199

GabrielGonzalezDiaz merged commit d7b29e0 into googleapis:diregapic-lro Sep 3, 2021

suztomo pushed a commit that referenced this pull request Mar 21, 2023

chore: remove native-image-support from bom (#832)

b5fbbe4

The module itself has been removed in #820.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Implement DIREGAPIC LRO annotations #832

Feat: Implement DIREGAPIC LRO annotations #832

GabrielGonzalezDiaz commented Aug 30, 2021 •

edited

Loading

codecov bot commented Aug 31, 2021 •

edited

Loading

vam-google left a comment

vam-google Aug 31, 2021

vam-google Sep 1, 2021

vam-google Aug 31, 2021

vam-google Aug 31, 2021

GabrielGonzalezDiaz Aug 31, 2021

vam-google Sep 1, 2021

vam-google Aug 31, 2021

vam-google Aug 31, 2021

vam-google left a comment

vam-google Sep 1, 2021

vam-google Sep 1, 2021

vam-google Sep 1, 2021

GabrielGonzalezDiaz Sep 1, 2021 •

edited

Loading

vam-google Sep 1, 2021

vam-google Sep 1, 2021

vam-google Sep 1, 2021

vam-google left a comment

Feat: Implement DIREGAPIC LRO annotations #832

Feat: Implement DIREGAPIC LRO annotations #832

Conversation

GabrielGonzalezDiaz commented Aug 30, 2021 • edited Loading

codecov bot commented Aug 31, 2021 • edited Loading

Codecov Report

vam-google left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google left a 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

GabrielGonzalezDiaz Sep 1, 2021 • 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

vam-google left a comment

Choose a reason for hiding this comment

GabrielGonzalezDiaz commented Aug 30, 2021 •

edited

Loading

codecov bot commented Aug 31, 2021 •

edited

Loading

GabrielGonzalezDiaz Sep 1, 2021 •

edited

Loading