feat: DIREGAPIC initial implementation #746

vam-google · 2021-06-02T06:56:25Z

Integration tests (compute_small) are not present in this commit, as they depend on #743 and #744. Also, as a prerequisite, at least a basic implementation of DIREGAPIC must be merged in gapic-generator-java, to integrate it with googleapis-discovery first (since integration test infra depends on the actual googleapis/googleapis-discovery targets). Please check vam-google@8983e23 to see how it would look like with compute integration tests not excluded.

compliance.proto is used as a basis for the REST composer tests. It was copied as is from showcase/compliance.proto.

Changes in WORKSPACE and repositories.bzl are necessary to make this repo work with gax-java 1.63.0 and above (gax-java vs gapic-generator-java java dependencies imports precedence). The other dependencies changes are either to bring deps in sync with the actual ones in googleapis, or to fix a specific import precedence issue.

I also added (in a form of bazel rules) a proto descriptor dumper and a runner from the dumped file (for debugging purposes). Not technically required here (but was very helpful for debugging purposes, so hopefully we can preserve it).

codecov · 2021-06-02T07:06:35Z

Codecov Report

Merging #746 (f0563bb) into master (72fa76f) will increase coverage by 0.35%.
The diff coverage is 92.84%.

@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
+ Coverage   91.30%   91.66%   +0.35%     
==========================================
  Files         133      140       +7     
  Lines       13900    14697     +797     
  Branches     1032     1051      +19     
==========================================
+ Hits        12692    13472     +780     
- Misses        927      941      +14     
- Partials      281      284       +3

Impacted Files	Coverage Δ
.../google/api/generator/gapic/composer/Composer.java	`10.11% <0.00%> (-2.57%)`	⬇️
...n/AbstractServiceCallableFactoryClassComposer.java	`100.00% <ø> (+16.42%)`	⬆️
...erator/gapic/protoparser/PluginArgumentParser.java	`51.61% <30.76%> (-17.96%)`	⬇️
...google/api/generator/gapic/protoparser/Parser.java	`43.21% <40.00%> (+0.10%)`	⬆️
.../api/generator/gapic/composer/store/TypeStore.java	`70.21% <50.00%> (-1.89%)`	⬇️
...pi/generator/gapic/protoparser/HttpRuleParser.java	`82.35% <81.08%> (-3.37%)`	⬇️
...poser/common/AbstractServiceStubClassComposer.java	`98.35% <87.50%> (+0.56%)`	⬆️
...google/api/generator/gapic/model/HttpBindings.java	`90.90% <90.90%> (ø)`
...api/generator/gapic/composer/rest/RestContext.java	`95.00% <95.00%> (ø)`
.../composer/rest/ServiceClientTestClassComposer.java	`97.39% <97.39%> (ø)`
... and 20 more

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 72fa76f...f0563bb. Read the comment docs.

miraleung

Took a first pass, will do a finer-grained review on the next one.

miraleung · 2021-06-02T16:06:25Z

src/main/java/com/google/api/generator/MainDumper.java

+import com.google.protobuf.compiler.PluginProtos.CodeGeneratorResponse;
+import java.io.IOException;
+
+public class MainDumper {


This will be really useful. Could we move this into a debug-only subpackage (to separate this from the main generator logic) and rename this class to something like GeneratorRequestDumper?

Removed from this PR, will add in a separate one, incorporating the PR feedback.

miraleung · 2021-06-02T16:10:13Z

src/main/java/com/google/api/generator/MainDumper.java

+  }
+
+  /** Register all extensions needed to process API protofiles. */
+  private static void registerAllExtensions(ExtensionRegistry extensionRegistry) {


Since we're doing this in three places now, can we centralize this? We can have a minimal standalone class (e.g. in a generator.registry package) that has just this method.

Removed from this PR, will add in a separate one, incorporating the PR feedback.

miraleung · 2021-06-02T16:12:57Z

src/main/java/com/google/api/generator/MainFromFile.java

+import java.io.InputStream;
+import java.io.OutputStream;
+
+public class MainFromFile {


Same as the above comment, could we please move this into a debug-specific package and use a more descriptive name (e.g. GapicLibraryDumper?

Removed from this PR, will add in a separate one, incorporating the PR feedback.

miraleung · 2021-06-02T16:13:22Z

src/main/java/com/google/api/generator/MainFromFile.java

+import java.io.OutputStream;
+
+public class MainFromFile {
+  public static void main(String[] args)


Consider adding a usage example, same for the other debug class.

Removed from this PR, will add in a separate one, incorporating the PR feedback.

miraleung · 2021-06-02T16:13:43Z

BUILD.bazel

@@ -45,6 +45,32 @@ java_binary(
    ],
 )

+java_binary(


Since this is independent of DIREGAPIC, could we please split this out into a separate PR?

Removed from this PR, will add in a separate one, incorporating the PR feedback.

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

miraleung · 2021-06-02T17:34:28Z

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

-  private static Set<String> getHttpVerbPattern(HttpRule httpRule) {
-    String pattern = null;
-    // Assign a temp variable to prevent the formatter from removing the import.
+  private static String getPattern(HttpRule httpRule) {


getHttpRulePattern or similar?

I guess, originally it was called as getPattern, then in one of the recent PRs you renamed it to getHttpVerbPattern and on merge I overrode it back to getPattern. Renamed it to its recent updated name getHttpVerbPattern.

miraleung · 2021-06-02T18:03:32Z

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

+          Sets.difference(inputMessageOpt.get().fieldMap().keySet(), bodyBinidngsUnion);
+    }
+
+    HttpRuleBindings.Builder httpBindings = HttpRuleBindings.builder();


Nit: Can we use the chained builder pattern for consistency? e.g. builder().setFoo().set... .build().

Yes, sure. No idea why I used this style here. Refactored to inline return statement with chained builder.

miraleung · 2021-06-02T18:04:28Z

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

+
+    HttpRuleBindings.Builder httpBindings = HttpRuleBindings.builder();
+    httpBindings.setHttpVerb(httpRule.getPatternCase().toString());
+    httpBindings.setPattern(pattern);


What about a version of setPattern that takes a PatternCase? Then we can move getPattern into HttpRuleBindings, where it fits better anyway.

In that case, I think the whole parsing logic must be moved to HttpRuleBindings. I.e. the current architecture has a split between the class which simply contains the values (HttpRuleBindings, a simple POJO, javabean, whatever), and HttpRuleParser - one of potential classes, which knows how to populate HttpRuleBinding class from a HttpRule. So basically HttpRuleBindings class is detached from HttpRule per se, and may be populated from another source by some other sort of parser or manually. From my experience splitting this stuff into two classes (one contains values, the other one (or a set of, one for each source) knows how to populate those values from a source) usually works better in a long run (especially if a persistence layer is used, like a database, though this is irrelevant here). I can merge them, if you wish, though, I don't have a strong preference here.

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

…tes for existing integration tests. Will do that in next PR (the update of googleapis is needed for integration test for compute)

WORKSPACE

miraleung · 2021-06-04T19:11:04Z

rules_java_gapic/java_gapic.bzl

@@ -127,12 +127,14 @@ def java_gapic_library(
        service_yaml = None,
        deps = [],
        test_deps = [],
+        transport = None,
+        java_generator_name = "java_gapic",


SG, could we please add some documentation here? And since this is intended only for our use, could we please make this fail if the string does not match the generator or the dumpers' values?

miraleung · 2021-06-04T19:14:33Z

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

+  private static final HttpJsonServiceStubClassComposer INSTANCE =
+      new HttpJsonServiceStubClassComposer();
+
+  protected static final TypeStore FIXED_REST_TYPESTORE = createStaticTypes();


I meant FIXED_REST_TYPESTORE, but I see now they're actually different. In that case, could we make this one private?

miraleung · 2021-06-04T19:15:27Z

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

+    Expr returnExpr = null;
+    VariableExpr fieldsVarExpr = null;
+    Expr serializerExpr = null;
+    if (!extractorReturnType.isProtoPrimitiveType()) {


Could we please flip the if-blocks so the shorter one comes first?

Just curious, is there a best practice about this? conditional return/continue is preferred to go first but it is not quite the same as if-else block, after which method continues. Or is it about using not in if-else (as if else always covers both true and !true cases in any case) Well, anyways, flipped.

miraleung · 2021-06-04T19:17:08Z

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

+      paramsPutArgs.add(requestBuilderExpr);
+
+      if (fieldsVarExpr == null) {
+        Expr paramsPutExpr =


Perhaps I'm missing something, but this looks the same as the if-block. Can we move this assignment out, and leave just the returnExpr / bodyExprs.add() logic inside?

I guess centennially they were different, as since one of them was missing setReturnType(), but the one with setReturnType matches both cases, so moved it out.

miraleung · 2021-06-04T19:29:44Z

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

+        .setPattern("")
+        .setPathParameters(ImmutableSet.of())
+        .setQueryParameters(ImmutableSet.of())
+        .setBodyParameters(ImmutableSet.of());


In that case, we probably shouldn't be able to instantiate an HttpRuleBindings at all (or we'd be able to set an empty pattern and verb with non-empty parameter lists). Could we do the following instead?

Make HttpRuleBindings Nullable on Method

Remove the empty string setters here, and add checks in build() to fail on invalid cases (i.e. empty string, pattern has no slashes and is not PubSub's _deleted-topic_, etc).

Consider making the verb an enum, or check that the string is one of the accepted five values in build(). (Would prefer the enum, since the latter is more error-prone.)

Context: Using rigorous validity checks has really helped me with development in this codebase. For instance, adding type-checking on assignment expressions or requiring set types on methods has helped me catch those errors at compile-time, so I think more rigid checks would help us here as well.

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

miraleung · 2021-06-04T19:30:49Z

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

  // HttpBinding pending dotted reference support.
-  public abstract List<String> httpBindings();
+  public abstract HttpRuleBindings httpBindings();


Continuing the comment above; Can we put a @Nullable on this?

I really would rather not have nulls for things like this. Please check the other comments about HttpBindings. WDYT about renaming the HttpRuleBindings to just HttpBindings to make things clearer that this class is not strictly about the google.api.http option in proto files.

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

miraleung · 2021-06-04T19:34:01Z

src/test/java/com/google/api/generator/gapic/composer/rest/RestTestProtoLoader.java

+import java.util.Optional;
+import java.util.Set;
+
+public class RestTestProtoLoader extends TestProtoLoader {


Nit: Since all the classes are prefixed with HttpJson, should we do that here too for consistency?

This was intentional, and as consistent as i could make it. The generated files are named HttpJson<Smth> because they generate HttpJson<Smth> so it would comply with naming pattern of composer-generated_class. For everything else, the "rest" is used for naming.

miraleung

Leaving the HttpRuleBindings discussion as-is so we can discuss more next week.

miraleung · 2021-06-05T00:30:00Z

rules_java_gapic/java_gapic.bzl

@@ -127,12 +127,14 @@ def java_gapic_library(
        service_yaml = None,
        deps = [],
        test_deps = [],
+        transport = None,
+        java_generator_name = "java_gapic",


Sure, although some documentation would still be ideal.

Could you please check if the commits have been pushed? I'm still seeing the param without the leading underscore.

miraleung · 2021-06-05T00:31:28Z

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

+  private static final ServiceStubSettingsClassComposer INSTANCE =
+      new ServiceStubSettingsClassComposer();
+
+  protected static final TypeStore FIXED_REST_TYPESTORE = createStaticTypes();


Yes, I would prefer this to be privately-scoped, since it's meant to be used just in this class (so it is not consistent with FIXED_TYPESTORE by nature).

src/main/java/com/google/api/generator/gapic/composer/rest/ServiceClientTestClassComposer.java

miraleung

LGTM with the following addressed:

Can we flip these blocks (from the last round)?
PTAL at this comment from the last round
Can we make this FIXED_REST_TYPESTORE privately scoped as well?
Nit on L81 of HttpBindings - could we please use !pattern.isEmpty() instead of "".equals(pattern)?
PTAL at this comment on L290 of GrpcServiceStubComposer.java?

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

miraleung · 2021-06-05T03:51:40Z

src/test/java/com/google/api/generator/gapic/composer/rest/RestTestProtoLoader.java

+import java.util.Optional;
+import java.util.Set;
+
+public class RestTestProtoLoader extends TestProtoLoader {


src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java

vam-google · 2021-06-07T07:27:18Z

@miraleung Thanks for the reveiw!

More detailed responses are at each specific comment, but briefly:

Flipped the if blocks
The blocks were a bit different, but the bigger one worked for both cases, so moved it out as requested.
find/replacing it, I did not acutually replace the one, from which all started =). Fixed.
Intentional, to avoid NullPointer and to avoid duplicating NullPointer check here and in autovalue generated class.
Intentional, as it would be redundant, since the check is already done at the beginning of the whole method.

vam-google · 2021-06-07T07:33:15Z

@miraleung PTAL

miraleung

LGTM with commented-out code removed.

src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java

miraleung · 2021-06-07T19:06:49Z

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

+                FIXED_REST_TYPESTORE.get(ProtoMessageRequestFormatter.class.getSimpleName()))
+            .setMethodName("newBuilder")
+            .setGenerics(Collections.singletonList(protoMethod.inputType().reference()))
+            // .setArguments(Arrays.asList(m))


Nit: Remove?

…o v2 (#746)

vam-google added 2 commits June 1, 2021 23:16

feat: implement DIREGAPIC

8983e23

feat: implement DIREGAPIC without integration tests

d568db5

vam-google requested review from miraleung and a team as code owners June 2, 2021 06:56

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

miraleung reviewed Jun 2, 2021

View reviewed changes

vam-google added 3 commits June 2, 2021 12:08

fix: fix ordering of generated fields

8de3877

Address PR feedback.

b1d0660

Add a comment about order of things in WORKSPACE file

72a53c6

vam-google requested a review from miraleung June 4, 2021 04:41

vam-google added 2 commits June 4, 2021 11:50

Merge remote-tracking branch 'upstream/master'

2dcf807

revert update of googleapis hash, as it started picking up newer upda…

d99d8d0

…tes for existing integration tests. Will do that in next PR (the update of googleapis is needed for integration test for compute)

miraleung suggested changes Jun 4, 2021

View reviewed changes

vam-google requested a review from miraleung June 5, 2021 00:25

miraleung suggested changes Jun 5, 2021

View reviewed changes

vam-google added 2 commits June 4, 2021 19:29

address PR feedback

35b6dde

address PR feedback

393840e

vam-google requested a review from miraleung June 5, 2021 02:38

miraleung suggested changes Jun 5, 2021

View reviewed changes

vam-google added 2 commits June 7, 2021 00:30

address PR feedback

eaf7916

Merge remote-tracking branch 'upstream/master'

dc8a156

vam-google requested a review from miraleung June 7, 2021 07:32

miraleung approved these changes Jun 7, 2021

View reviewed changes

address PR feedback

f0563bb

vam-google merged commit 81f6737 into googleapis:master Jun 7, 2021

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

deps: update dependency com.google.api-client:google-api-client-bom t…

ef2b57a

…o v2 (#746)

feat: DIREGAPIC initial implementation #746

feat: DIREGAPIC initial implementation #746

Conversation

vam-google commented Jun 2, 2021 • edited

codecov bot commented Jun 2, 2021 • edited

Codecov Report

miraleung 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google Jun 7, 2021 • edited

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

miraleung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miraleung left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google commented Jun 7, 2021 • edited

vam-google commented Jun 7, 2021

miraleung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google commented Jun 2, 2021 •

edited

codecov bot commented Jun 2, 2021 •

edited

vam-google Jun 7, 2021 •

edited

miraleung left a comment •

edited

vam-google commented Jun 7, 2021 •

edited