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

Draft: Selective api poc #3129

Closed

zhumin8 wants to merge 21 commits into main from selective-api-poc

Contributor

zhumin8 commented Aug 21, 2024 •

edited

Loading

DO NOT MERGE.
This is a draft pr as prototype to changes for selective api generation feature.
The changes in this pr include api-common-java changes copied over manually for testing purposes.

zhumin8 added 5 commits

August 12, 2024 16:55


          temp: local generate java-common-protos from googleapis with client.p…

c20c974

…roto changes.


          poc for typical cases on selective generation.

cbcd5bc


          minor fixes and add comments.

cf87622


          temp commit: needs cleanups - experimenting with resources.:

e877473


          add composer test to verify methods in client generated.

a89bb9f

product-auto-label bot added the size: xl label

zhumin8 added do not merge and removed size: xl labels

zhumin8 commented

View reviewed changes

gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/ComposerTest.java Outdated Show resolved Hide resolved

zhumin8 commented

View reviewed changes

gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java Outdated Show resolved Hide resolved

zhumin8 commented

View reviewed changes

gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java Outdated Show resolved Hide resolved


          fix service yaml library_settings.version. add verification that it m…

7dcdf56

…atches proto package in parser. other cleanups and minor fixes.

product-auto-label bot added the size: xl label

zhumin8 added 5 commits

August 23, 2024 10:55


          minor code cleanups.

f47c93a


          add note about cleanups for message and resource names parsing.


          rename method name.

c29a319


          minor fix: use parent interface.

7d8480a


          update test proto and config comments and namings.

395fa47

zhumin8 commented

View reviewed changes

gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java Outdated Show resolved Hide resolved

zhumin8 removed the do not merge label

blakeli0 reviewed

View reviewed changes

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

                   return fileDescriptor.getServices().stream()
                       .filter(
                           serviceDescriptor -> {
                             List<MethodDescriptor> methodsList = serviceDescriptor.getMethods();
-                            if (methodsList.isEmpty()) {
+                            List<MethodDescriptor> methodListSelected =

Collaborator

blakeli0 Aug 28, 2024

This scenario is in case that all methods from a service are excluded?

Contributor Author

zhumin8 Aug 29, 2024

Yes. And this is likely now e.g. in vertexai's requirement it only exposes methods from 2 services.

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

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

@@ @@ -425,6 +434,47 @@ public static List<Service> parseService( @@
                       Transport.GRPC);
                 }
+                private static Optional<List<String>> getInclusionMethodListFromServiceYaml(
+                    Optional<com.google.api.Service> serviceYamlProtoOpt, String protoPackage) {

Collaborator

blakeli0 Aug 28, 2024

It is usually preferred to not passing around Optional if we can check it before calling the method.

Contributor Author

zhumin8 Aug 30, 2024

I want to follow-up on this from our discussion earlier and get @blakeli0 your opinion on this. (FYI, I've combined this method into shouldIncludeMethodInGeneration() )
For now, I am slightly inclined to keep this Optional<com.google.api.Service> serviceYamlProtoOpt as input for 2 reasons:

The pattern of passing in this Optional is repeated in this Parser class, a larger scale refactor is needed to remove these.
Because this service yaml is an optional config file that only some libraries have, it sort of make sense to have this Optional representation. And I find the current way can better encapsulate this piece of logic: shouldIncludeMethodInGeneration() has all the information to decide if this method should or not be included. I could check serviceYamlProtoOpt before calling the method, but the 2 occurrences would repeat the logic in a reverse way, and it might be less readable for debugging in the future.

Does this makes sense to you? WDYT?

gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java Outdated Show resolved Hide resolved

zhumin8 added 5 commits

August 30, 2024 14:18


          review comments, combining methods for readability.

950069e


          minor cleanup.

fa63021


          update ComposerTest to check on non-included methods.

e013458


          update test.

5a89def


          move test for helper resource name from ComposerTest to ProtoTest.

a99af2c

zhumin8 commented

View reviewed changes

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

@@ @@ -705,6 +710,99 @@ void parseServiceWithNoMethodsTest() { @@
                   assertEquals("EchoWithMethods", services.get(0).overriddenName());
                 }
+                @Test
+                void selectiveGenerationTest_shouldExcludeUnusedResourceNames() {

Contributor Author

zhumin8 Aug 30, 2024

This test verifies that unused resource names (associated with methods not in inclusion list) will not be populated to GapicContext.

blakeli0 reviewed

View reviewed changes

gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java Outdated Show resolved Hide resolved

gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java Outdated Show resolved Hide resolved

gapic-generator-java/src/test/proto/selective_api_generation.proto Outdated Show resolved Hide resolved

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

@@ @@ -425,6 +434,39 @@ public static List<Service> parseService( @@
                       Transport.GRPC);
                 }
+                private static boolean shouldIncludeMethodInGeneration(

Collaborator

blakeli0 Sep 11, 2024

Can we make this a default scope method so we can test it directly? This would enabled us testing more scenarios like the non matching packages, it would also simplify the golden tests.

gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/ComposerTest.java Outdated Show resolved Hide resolved

gapic-generator-java/src/test/resources/selective_api_generation_no_java_settings_v1beta1.yaml Outdated

Collaborator

blakeli0 Sep 11, 2024

We probably don't need this yaml and selective_api_generation_no_publishing_v1beta1.yaml as the scenarios should be tested in ParserTest directly.

zhumin8 added 2 commits

September 13, 2024 16:18


          review feedback, log warning text.

e8f5388


          fix regression when combining logic in 950069e.

2012a82

zhumin8 added 3 commits

September 13, 2024 22:20


          trim proto for test. rm composer test.

7978c15


          expose private method to test, add test for version mismatch, add moc…

c2967e9

…kito dep.


          trying to refactor 2 parser tests with mockito mocks.

819bdd5

zhumin8 mentioned this pull request

feat: enable selective generation based on service config include list #3323

Merged

zhumin8 added a commit that referenced this pull request


          feat: enable selective generation based on service config include list (

0cddadb

#3323)

following changes in client.proto that propagated to `ClientProto.java`
([pr](https://github.com/googleapis/sdk-platform-java/pull/3309/files#diff-44c330ef5cfa380744be6c58a14aa543edceb6043d5b17da7b32bb728ef5d85f)),
apply changes from poc pr
(#3129).

For context:
[go/selective-api-gen-java-one-pager](http://goto.google.com/selective-api-gen-java-one-pager),
b/356380016

Contributor Author

zhumin8 commented Nov 7, 2024

implemented in #3323. closing

zhumin8 closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels