Skip to content

feat: Selective gapic generation phase II #3730

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 27 commits into from
Apr 17, 2025
Merged

feat: Selective gapic generation phase II #3730

merged 27 commits into from
Apr 17, 2025

Conversation

cindy-peng
Copy link
Contributor

@cindy-peng cindy-peng commented Apr 2, 2025 •
edited
Loading

Implement Selective GAPIC Generation (Phase II)

This PR implements Phase II of selective GAPIC generation within the gapic-generator-java project. This allows for finer control over the intended usage of generated client methods (public, internal, or hidden) by providing selective gapic generation configuration in service yaml.

Key Changes:

1. Model Updates

  • Added a isInternalApi attribute to the internal representation of methods to track their intended visibility (e.g., public, internal).

2. Parser Logic

  • Introduced the getMethodSelectiveGapicType() method responsible for parsing the selective generation configuration for each method.
  • Modified service filtering logic: Service classes will not be generated if the service definition contains no methods or includes only methods marked as HIDDEN.
  • Enhanced parseService() to determine and assign the appropriate SelectiveGapicType to each service method and its corresponding generated variants (e.g., overloaded methods).

3. Composer (Code Generation) Updates

  • Method Annotations: For all method variants designated as INTERNAL, generate an @InternalApi annotation accompanied by a warning message discouraging external use.
  • Method Header Comments: For methods marked as INTERNAL, generate a specific comment in the method's header indicating its intended internal-only usage.
  • Sample Generation: Adjusted the logic for generating package-info.java samples to prevent the usage of any methods marked as INTERNAL.

4. Tests

  • Added unit tests covering the new parser logic and comment generation changes related to selective generation types.
  • Added/updated golden unit/integration tests to verify the correct code output for various selective generation scenarios, including services with:
    • All public methods.
    • A mix of public, INTERNAL, and/or HIDDEN methods.
    • No public methods (verifying that the service class is not generated).

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Apr 2, 2025
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Apr 2, 2025
@zhumin8
Copy link
Contributor

zhumin8 commented Apr 10, 2025

/gcbrun

zhumin8
zhumin8 reviewed Apr 11, 2025
View reviewed changes
Copy link
Contributor

@zhumin8 zhumin8 left a comment

Choose a reason for hiding this comment

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

Looks good to me so far. Left a few small comments and I will take another pass.

...test/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposerTest.java Outdated
Service service = context.services().get(1);
GapicClass clazz = GrpcServiceStubClassComposer.instance().generate(context, service);

Assert.assertGoldenClass(this.getClass(), clazz, "SelectiveGapicStub.golden");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any @InternalApi in the golden file for this test. Is this test necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this should not impact the stubclass. I have removed the test and the golden file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Am I missing it or there is only test setup to verify -Client changes, but not -Stub, -StubSettings or -ClientSettings ?
For example, in below snippet of golden file, I expect chatShouldGenerateAsInternalCallable() also be marked as internal, and I believe changes in AbstractServiceStubClassComposer.java should do it. Do you have any test coverages for it?

sdk-platform-java/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/EchoServiceSelectiveGapicClient.golden

Lines 568 to 571 in a87d3b0

@InternalApi("Internal API. This API is not intended for public consumption.")
public final UnaryCallable<EchoRequest, EchoResponse> chatShouldGenerateAsInternalCallable() {
return stub.chatShouldGenerateAsInternalCallable();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I didn't add those tests earlier. I just updated the test files to include coverage for service settings, stub and stub settings. Please take a look:)

zhumin8
zhumin8 reviewed Apr 11, 2025
View reviewed changes
...com/google/api/generator/gapic/composer/common/AbstractServiceStubSettingsClassComposer.java Outdated
@@ -140,6 +141,8 @@ public abstract class AbstractServiceStubSettingsClassComposer implements ClassC
private static final String SETTINGS_LITERAL = "Settings";

private static final String DOT = ".";
private static final String INTERNAL_API_WARNING =
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: perhaps extract INTERNAL_API_WARNING to a somewhere to be accessible for each of these composes.