feat: gapic-generator-java to perform a no-op when no services are detected #2460

diegomarquezp · 2024-02-12T22:08:19Z

Fixes #2050

Adds behavior to gracefully perform a NOOP if no services are contained in the generation request.

Confimation in hermetic build scripts

From generate_library.sh against google/cloud/alloydb/connectors/v1

+ /usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk3/sdk-platform-java/library_generation/output/protobuf-25.2/bin/protoc --experimental_allow_proto3_optional --plugin=protoc-gen-java_gapic=/usr/local/google/home/diegomarquezp/.pyenv/versions/3.11.0/lib/python3.11/site-packages/library_generation/gapic-generator-java-wrapper --java_gapic_out=metadata:/usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk3/sdk-platform-java/library_generation/output/temp_preprocessed/java_gapic_srcjar_raw.srcjar.zip --java_gapic_opt=transport=grpc,rest-numeric-enums,grpc-service-config=,gapic-config=,api-service-config=google/cloud/alloydb/connectors/v1/connectors_v1.yaml google/cloud/alloydb/connectors/v1/resources.proto google/cloud/common_resources.proto
Apr 05, 2024 9:33:22 PM com.google.api.generator.gapic.protoparser.Parser parse
WARNING: No services found to generate. This will produce an empty zip file
Apr 05, 2024 9:33:22 PM com.google.api.generator.gapic.composer.ClientLibraryPackageInfoComposer generatePackageInfo
WARNING: Generating empty package info since no services were found
+ did_generate_gapic=true
+ zipinfo -t /usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk3/sdk-platform-java/library_generation/output/temp_preprocessed/java_gapic_srcjar_raw.srcjar.zip
Empty zipfile. 
+ did_generate_gapic=false
+ [[ false == \t\r\u\e ]]

I made some changes to library_generation but I moved them to a follow up PR (#2599) so the checks can pass here.

This reverts commit 4bc092a.

This reverts commit a79329f.

This reverts commit d150747.

blakeli0 · 2024-02-28T15:13:52Z

Throwing an exception and catch it just to perform no-op and/or logging is not a good practice. It would be better if we can do the same thing gracefully. For example, can we remove this check, log the scenario, and return an empty GapicContext?

…rvice-noop

… into gapic-generator-no-service-noop

diegomarquezp · 2024-03-05T02:51:21Z

From https://github.com/googleapis/sdk-platform-java/actions/runs/8150253915/job/22276206879?pr=2460

+ /home/runner/work/sdk-platform-java/sdk-platform-java/output/protobuf-25.2/bin/protoc --experimental_allow_proto3_optional --plugin=protoc-gen-java_gapic=/home/runner/work/sdk-platform-java/sdk-platform-java/library_generation/gapic-generator-java-wrapper --java_gapic_out=metadata:/home/runner/work/sdk-platform-java/sdk-platform-java/output/temp_preprocessed/java_gapic_srcjar_raw.srcjar.zip --java_gapic_opt=transport=grpc,rest-numeric-enums,grpc-service-config=,gapic-config=,api-service-config=google/cloud/alloydb/connectors/v1/connectors_v1.yaml google/cloud/alloydb/connectors/v1/resources.proto google/cloud/common_resources.proto
Error: Exception in thread "main" java.lang.IllegalStateException: No services found to generate
	at com.google.common.base.Preconditions.checkState(Preconditions.java:512)
	at com.google.api.generator.gapic.protoparser.Parser.parse(Parser.java:178)
	at com.google.api.generator.gapic.Generator.generateGapic(Generator.java:30)
	at com.google.api.generator.Main.main(Main.java:28)

I confirmed they pass locally. Not sure how to prove it in a PR that updates both the generator and the hermetic build scripts

Reason: the hermetic build IT uses a published GGJ instead of compiling its own

EDIT: I decided to move all library_generation changes to #2599

...-generator-java/src/main/java/com/google/api/generator/engine/ast/PackageInfoDefinition.java

...c-generator-java/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java

.../src/main/java/com/google/api/generator/gapic/composer/ClientLibraryPackageInfoComposer.java

gapic-generator-java/src/main/java/com/google/api/generator/gapic/model/GapicPackageInfo.java

JoeWang1127 · 2024-04-09T17:20:02Z

gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/ComposerTest.java

@@ -154,6 +168,22 @@ public void composeSamples_parseProtoPackage() {
    }
  }

+  @Test
+  public void testEmptyGapicContext_succeeds() {


From what I read from the test, you want to verify no exception is thrown in Composer.composeServiceClasses(GapicContext.EMPTY)

How about write tests verifying the result, rather assert the exception is null? The test will fail if an exception is thrown.

Thanks, this simplifies the test. Done.

JoeWang1127 · 2024-04-09T17:23:27Z

gapic-generator-java/src/test/java/com/google/api/generator/gapic/protowriter/WriterTest.java

+  }
+
+  @Test
+  public void productionWrite_emptyGapicContext_succeeds() throws IOException {


Same as comment on testEmptyGapicContext_succeeds.

blakeli0 · 2024-04-09T22:29:45Z

What happens if you try to unzip temp-codegen.srcjar? Does it fail or produce an empty folder (I prefer this one because we can add logic to remove empty folders using remove_empty_files in utilities.sh, similarly to grpc plugin)?
+ zipinfo -t /usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk-platform-java/library_generation/output/temp_preprocessed/temp-codegen.srcjar
Empty zipfile. 
+ did_generate_gapic=false
We will now NOT generate a srcjar if no services are found. This means that the wrapper zip java_gapic_srcjar_raw.srcjar.zip will be empty. If we try to unzip it, it will fail (and the script will exit due to set -e), so we still need that zipinfo check. So to answer the question, it would not generate an empty folder, it would just fail.

What creates the java_gapic_srcjar_raw.srcjar.zip? The generator or the hermetic build scripts? Can we not generate anything at all, which would be a true no-op?
In general, I think the changes in GapicContext and Parser makes sense, but the we may not need changes in Composer and Writer. For example, can we return null CodeGeneratorResponse and skip writing to disk all together?

…rvice-noop # Conflicts: # gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/ComposerTest.java # gapic-generator-java/src/test/java/com/google/api/generator/gapic/protoparser/ParserTest.java # gapic-generator-java/src/test/java/com/google/api/generator/gapic/protowriter/WriterTest.java

diegomarquezp · 2024-05-28T22:37:47Z

What happens if you try to unzip temp-codegen.srcjar? Does it fail or produce an empty folder (I prefer this one because we can add logic to remove empty folders using remove_empty_files in utilities.sh, similarly to grpc plugin)?
+ zipinfo -t /usr/local/google/home/diegomarquezp/Desktop/sdk2/sdk-platform-java/library_generation/output/temp_preprocessed/temp-codegen.srcjar
Empty zipfile. 
+ did_generate_gapic=false
We will now NOT generate a srcjar if no services are found. This means that the wrapper zip java_gapic_srcjar_raw.srcjar.zip will be empty. If we try to unzip it, it will fail (and the script will exit due to set -e), so we still need that zipinfo check. So to answer the question, it would not generate an empty folder, it would just fail.
What creates the java_gapic_srcjar_raw.srcjar.zip? The generator or the hermetic build scripts? Can we not generate anything at all, which would be a true no-op? In general, I think the changes in GapicContext and Parser makes sense, but the we may not need changes in Composer and Writer. For example, can we return null CodeGeneratorResponse and skip writing to disk all together?

That sounds good. I modified Writer to return null if the context is empty, rendering the changes in package info unnecessary.

sonarcloud · 2024-05-29T00:33:07Z

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

Issues
18 New issues
0 Accepted issues

Measures
0 Security Hotspots
80.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarcloud · 2024-05-29T00:36:17Z

Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

JoeWang1127 · 2024-05-29T18:10:08Z

gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/ComposerTest.java

@@ -53,8 +56,13 @@ class ComposerTest {
          .build();
  private List<Sample> ListofSamples = Arrays.asList(new Sample[] {sample});

+  @Before


Could you change this tag to @BeforeEach?

JoeWang1127 · 2024-05-29T18:10:35Z

gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/ComposerTest.java

  @Test
-  void gapicClass_addApacheLicense() {
+  public void gapicClass_addApacheLicense_validInput_succeeds() {


Could you remove the public in the signature?

lqiu96 · 2024-05-29T18:22:59Z

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

@@ -175,7 +179,10 @@ public static GapicContext parse(CodeGeneratorRequest request) {
            mixinServices,
            transport);

-    Preconditions.checkState(!services.isEmpty(), "No services found to generate");
+    if (services.isEmpty()) {
+      LOGGER.warning("No services found to generate. This will produce an empty zip file");


qq, is this still correct? If

if (response != EMPTY_RESPONSE) { response.writeTo(System.out); }

doesn't write anything to System.out does it generate a zip file?

lqiu96 · 2024-05-29T18:25:24Z

gapic-generator-java/src/main/java/com/google/api/generator/gapic/protowriter/Writer.java

+    // package info may come as null if we have no services, but we will exit
+    // this function early if so.


qq, is this also still true? I don't see writePackageInfo() exiting early.

lqiu96 · 2024-05-29T18:27:48Z

gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/ComposerTest.java

@@ -154,6 +162,16 @@ void composeSamples_parseProtoPackage() {
    }
  }

+  @Test
+  public void testEmptyGapicContext_doesNotThrow() {
+    Composer.composeServiceClasses(GapicContext.EMPTY);


nit: Can this be something like assert empty list or assert null instead?

lqiu96 · 2024-05-29T18:29:04Z

gapic-generator-java/src/test/java/com/google/api/generator/gapic/protoparser/ParserTest.java

+    GapicContext result = Parser.parse(CodeGeneratorRequest.newBuilder().build());
+    assertFalse(result.containsServices());


nit: to match the test method, can this be assert that the result == GapicContext.Empty?

lqiu96

LGTM, added a few nits regarding the comments and tests

diegomarquezp added 5 commits February 12, 2024 20:11

feat(gapic-generator-java): Cause a noop when no services are found

00c1eb1

revert NoServicesFoundException

d150747

chore: fix owl-bot-staging unpacking

a79329f

Merge branch 'fix-owlbot-staging' into gapic-generator-no-service-noop

96b357b

try to use empty GapicPackageInfo

4bc092a

product-auto-label bot added the size: s Pull request size is small. label Feb 12, 2024

diegomarquezp changed the title ~~feat: gapic-generator-java to perform a no-op when no services are passed into~~ feat: gapic-generator-java to perform a no-op when no services are detected Feb 12, 2024

diegomarquezp added 5 commits February 14, 2024 21:26

Revert "try to use empty GapicPackageInfo"

0372f9e

This reverts commit 4bc092a.

Revert "chore: fix owl-bot-staging unpacking"

392305a

This reverts commit a79329f.

Revert "revert NoServicesFoundException"

97a38ad

This reverts commit d150747.

remove proto_only

204fff8

add a proto only library in integration test

4d85490

product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Feb 15, 2024

Merge branch 'main' into gapic-generator-no-service-noop

7e06287

diegomarquezp added 11 commits February 28, 2024 22:56

Merge remote-tracking branch 'origin/main' into gapic-generator-no-se…

45c1ff5

…rvice-noop

add empty() methods for no-op involved classes

6c8aa53

remove proto_only unit tests

c2ca534

fix warning messages

f5d595c

fix empty zip check

a58b4ef

extra handling for empty package info

0ff112e

add warning for empty package info

1639b61

fix integration test

69fce38

Merge branch 'main' into gapic-generator-no-service-noop

799b23e

use try with resources in Writer

4caa419

Merge remote-tracking branch 'origin/gapic-generator-no-service-noop'…

71cc3f3

… into gapic-generator-no-service-noop

diegomarquezp added 2 commits March 5, 2024 03:15

add coverage on parser and composer

827e27e

improve coverage 2

4eccad0

diegomarquezp requested review from blakeli0, JoeWang1127 and lqiu96 April 5, 2024 21:54

lqiu96 reviewed Apr 8, 2024

View reviewed changes

...-generator-java/src/main/java/com/google/api/generator/engine/ast/PackageInfoDefinition.java Outdated Show resolved Hide resolved

lqiu96 reviewed Apr 8, 2024

View reviewed changes

...c-generator-java/src/main/java/com/google/api/generator/engine/writer/JavaWriterVisitor.java Outdated Show resolved Hide resolved

lqiu96 reviewed Apr 8, 2024

View reviewed changes

.../src/main/java/com/google/api/generator/gapic/composer/ClientLibraryPackageInfoComposer.java Outdated Show resolved Hide resolved

lqiu96 reviewed Apr 8, 2024

View reviewed changes

gapic-generator-java/src/main/java/com/google/api/generator/gapic/model/GapicPackageInfo.java Outdated Show resolved Hide resolved

JoeWang1127 reviewed Apr 9, 2024

View reviewed changes

diegomarquezp added 4 commits May 28, 2024 17:04

Writer to return null response when no services are found

c5286e2

remove Composer changes

09f3722

revert unnecessary changes involving GapicPackageInfo

49b139a

product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels May 28, 2024

diegomarquezp added 2 commits May 28, 2024 18:38

Remove empty lines

0e73927

improve coverage

dc542b3

diegomarquezp requested review from lqiu96 and JoeWang1127 May 29, 2024 16:58

JoeWang1127 reviewed May 29, 2024

View reviewed changes

lqiu96 reviewed May 29, 2024

View reviewed changes

lqiu96 approved these changes May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: gapic-generator-java to perform a no-op when no services are detected #2460

feat: gapic-generator-java to perform a no-op when no services are detected #2460

diegomarquezp commented Feb 12, 2024 •

edited

blakeli0 commented Feb 28, 2024

diegomarquezp commented Mar 5, 2024 •

edited

JoeWang1127 Apr 9, 2024 •

edited

diegomarquezp May 28, 2024

JoeWang1127 Apr 9, 2024

diegomarquezp May 28, 2024

blakeli0 commented Apr 9, 2024

diegomarquezp commented May 28, 2024

sonarcloud bot commented May 29, 2024

sonarcloud bot commented May 29, 2024

JoeWang1127 May 29, 2024

JoeWang1127 May 29, 2024

lqiu96 May 29, 2024

lqiu96 May 29, 2024

lqiu96 May 29, 2024

lqiu96 May 29, 2024

lqiu96 left a comment

		// package info may come as null if we have no services, but we will exit
		// this function early if so.

		GapicContext result = Parser.parse(CodeGeneratorRequest.newBuilder().build());
		assertFalse(result.containsServices());

feat: gapic-generator-java to perform a no-op when no services are detected #2460

Are you sure you want to change the base?

feat: gapic-generator-java to perform a no-op when no services are detected #2460

Conversation

diegomarquezp commented Feb 12, 2024 • edited

Confimation in hermetic build scripts

blakeli0 commented Feb 28, 2024

diegomarquezp commented Mar 5, 2024 • edited

JoeWang1127 Apr 9, 2024 • 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

blakeli0 commented Apr 9, 2024

diegomarquezp commented May 28, 2024

sonarcloud bot commented May 29, 2024

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

sonarcloud bot commented May 29, 2024

Quality Gate passed for 'java_showcase_integration_tests'

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 left a comment

Choose a reason for hiding this comment

diegomarquezp commented Feb 12, 2024 •

edited

diegomarquezp commented Mar 5, 2024 •

edited

JoeWang1127 Apr 9, 2024 •

edited