Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 65 commits into from
Jun 10, 2024

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Feb 12, 2024

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.

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Feb 12, 2024
@diegomarquezp 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
@product-auto-label 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
@blakeli0
Copy link
Collaborator

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?

@diegomarquezp
Copy link
Contributor Author

diegomarquezp commented Mar 5, 2024

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

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label May 28, 2024
@diegomarquezp
Copy link
Contributor Author

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.

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

@Before
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change this tag to @BeforeEach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Test
void gapicClass_addApacheLicense() {
public void gapicClass_addApacheLicense_validInput_succeeds() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove the public in the signature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 72 to 73
// package info may come as null if we have no services, but we will exit
// this function early if so.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not true anymore either. I removed the comment.

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

@Test
public void testEmptyGapicContext_doesNotThrow() {
Composer.composeServiceClasses(GapicContext.EMPTY);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 616 to 617
GapicContext result = Parser.parse(CodeGeneratorRequest.newBuilder().build());
assertFalse(result.containsServices());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@lqiu96 lqiu96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, added a few nits regarding the comments and tests

@product-auto-label 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 diegomarquezp enabled auto-merge (squash) June 8, 2024 00:48
@diegomarquezp diegomarquezp disabled auto-merge June 8, 2024 00:48
@diegomarquezp diegomarquezp requested a review from JoeWang1127 June 8, 2024 00:49
Copy link

sonarqubecloud bot commented Jun 8, 2024

Copy link

sonarqubecloud bot commented Jun 8, 2024

Quality Gate Passed 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

@diegomarquezp diegomarquezp merged commit c0b5646 into main Jun 10, 2024
33 checks passed
@diegomarquezp diegomarquezp deleted the gapic-generator-no-service-noop branch June 10, 2024 21:22
JoeWang1127 added a commit that referenced this pull request Jun 25, 2024
🤖 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>
zhumin8 added a commit that referenced this pull request Jul 23, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gapic-generator-java exits with IllegalStateException when there's no service in protos
5 participants