feat: gapic-generator-java to perform a no-op when no services are detected by diegomarquezp · Pull Request #2460 · googleapis/sdk-platform-java

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

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.

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

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

          .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


-    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?

It's not correct anymore. I corrected the log entry. Thanks for the catch.

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.

Not true anymore either. I removed the comment.

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

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


+  @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

sonarqubecloud · 2024-06-08T00:57:34Z

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

Issues
7 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

sonarqubecloud · 2024-06-08T01:00:45Z

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

🤖 I have created a release *beep* *boop* --- <details><summary>2.42.0</summary> ## [2.42.0](v2.41.0...v2.42.0) (2024-06-25) ### Features * Allow Adding Client Level Attributes to MetricsTracerFactory ([#2614](#2614)) ([f122c6f](f122c6f)) * gapic-generator-java to perform a no-op when no services are detected ([#2460](#2460)) ([c0b5646](c0b5646)) * Make Layout Parser generally available in V1 ([e508ae6](e508ae6)) * populate `.repo-metadata.json` from highest version ([#2890](#2890)) ([f587541](f587541)) * push SNAPSHOT versions of the hermetic build docker image ([#2888](#2888)) ([81df866](81df866)) ### Bug Fixes * **deps:** update the Java code generator (gapic-generator-java) to 1.2.3 ([e508ae6](e508ae6)) * Expose Gax meter name ([#2865](#2865)) ([6c5d6ce](6c5d6ce)) * Move the logic of getting systemProductName from static block to static method ([#2874](#2874)) ([536f1eb](536f1eb)) * Update default Otel Attribute from method_name to method ([#2833](#2833)) ([af10a9e](af10a9e)) ### Dependencies * update dependency com.google.auto.value:auto-value to v1.11.0 ([#2842](#2842)) ([dd27fdf](dd27fdf)) * update dependency com.google.auto.value:auto-value-annotations to v1.11.0 ([#2843](#2843)) ([bf8e67f](bf8e67f)) * update dependency com.google.cloud:grpc-gcp to v1.6.1 ([#2943](#2943)) ([9f16b40](9f16b40)) * update dependency org.checkerframework:checker-qual to v3.44.0 ([#2848](#2848)) ([7a99c50](7a99c50)) * update dependency org.easymock:easymock to v5.3.0 ([#2871](#2871)) ([c243f7d](c243f7d)) * update google api dependencies ([#2846](#2846)) ([b5ef698](b5ef698)) * update googleapis/java-cloud-bom digest to 17cc5ec ([#2882](#2882)) ([d6abd8e](d6abd8e)) * update netty dependencies to v4.1.111.final ([#2877](#2877)) ([b5f10b9](b5f10b9)) * update opentelemetry-java monorepo to v1.39.0 ([#2863](#2863)) ([9d1f3a8](9d1f3a8)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Joe Wang <joewa@google.com>

fixes #2750 Skip parsing a service if no RPCs found for. In the scenario that only one service with no RPCs or all services have no RPCs, falls back to #2460 This pr also reverts a change brought by #985, and removes the relevant tests. For more context, this has been discussed [here](#2750 (comment)). --------- Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>

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

product-auto-label bot added the size: m Pull request size is medium. label May 28, 2024

diegomarquezp added 2 commits May 28, 2024 18:38

Remove empty lines

0e73927

improve coverage

dc542b3

diegomarquezp requested review from JoeWang1127 and lqiu96 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

diegomarquezp added 2 commits June 7, 2024 20:20

fix from comments

0838d76

more test fixes

2abdfb5

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

diegomarquezp enabled auto-merge (squash) June 8, 2024 00:48

diegomarquezp disabled auto-merge June 8, 2024 00:48

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

93270e6

diegomarquezp requested a review from JoeWang1127 June 8, 2024 00:49

blakeli0 approved these changes Jun 10, 2024

View reviewed changes

diegomarquezp merged commit c0b5646 into main Jun 10, 2024

diegomarquezp deleted the gapic-generator-no-service-noop branch June 10, 2024 21:22

release-please bot mentioned this pull request Jun 10, 2024

chore(main): release 2.42.0 #2847

Merged

zhumin8 mentioned this pull request Jul 16, 2024

fix: Generator to skip generation for empty services. #3051

Merged

		// 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());

Conversation

diegomarquezp commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confimation in hermetic build scripts

Uh oh!

blakeli0 commented Feb 28, 2024

Uh oh!

diegomarquezp commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diegomarquezp commented May 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqiu96 left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 8, 2024

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

Uh oh!

sonarqubecloud bot commented Jun 8, 2024

Quality Gate passed for 'java_showcase_integration_tests'

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

diegomarquezp commented Feb 12, 2024 •

edited

Loading

diegomarquezp commented Mar 5, 2024 •

edited

Loading