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: Parser to consume the api-versioning value from proto #2630

Merged
merged 12 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ static List<GapicClass> prepareExecutableSamples(List<GapicClass> clazzes) {
sample ->
samples.add(
addRegionTagAndHeaderToSample(
sample, gapicClass.apiShortName(), gapicClass.apiVersion())));
sample, gapicClass.apiShortName(), gapicClass.packageVersion())));
clazzesWithSamples.add(gapicClass.withSamples(samples));
});
return clazzesWithSamples;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ public GapicClass generate(GapicContext context, Service service) {
updateGapicMetadata(context, service, className, grpcRpcsToJavaMethodNames);
return GapicClass.create(kind, classDef, SampleComposerUtil.handleDuplicateSamples(samples))
.withApiShortName(service.apiShortName())
.withApiVersion(service.apiVersion());
.withPackageVersion(service.packageVersion());
}

private static List<AnnotationNode> createClassAnnotations(Service service, TypeStore typeStore) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public GapicClass generate(GapicContext context, Service service) {
.build();
return GapicClass.create(kind, classDef, SampleComposerUtil.handleDuplicateSamples(samples))
.withApiShortName(service.apiShortName())
.withApiVersion(service.apiVersion());
.withPackageVersion(service.packageVersion());
}

private static List<CommentStatement> createClassHeaderComments(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ public GapicClass generate(GapicContext context, Service service) {
return GapicClass.create(
GapicClass.Kind.STUB, classDef, SampleComposerUtil.handleDuplicateSamples(samples))
.withApiShortName(service.apiShortName())
.withApiVersion(service.apiVersion());
.withPackageVersion(service.packageVersion());
}

protected MethodDefinition createDefaultCredentialsProviderBuilderMethod() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public enum Kind {
public abstract String apiShortName();

// Only used for generating the region tag for samples; therefore only used in select Composers.
public abstract String apiVersion();
public abstract String packageVersion();

/**
* Create a GapicClass with minimal information. This is intended to be used for GapicClasses that
Expand Down Expand Up @@ -76,7 +76,7 @@ static Builder builder() {
return new AutoValue_GapicClass.Builder()
.setSamples(Collections.emptyList())
.setApiShortName("")
.setApiVersion("");
.setPackageVersion("");
}

abstract Builder toBuilder();
Expand All @@ -89,8 +89,8 @@ public final GapicClass withApiShortName(String apiShortName) {
return toBuilder().setApiShortName(apiShortName).build();
}

public final GapicClass withApiVersion(String apiVersion) {
return toBuilder().setApiVersion(apiVersion).build();
public final GapicClass withPackageVersion(String packageVersion) {
return toBuilder().setPackageVersion(packageVersion).build();
}

@AutoValue.Builder
Expand All @@ -103,7 +103,7 @@ abstract static class Builder {

abstract Builder setApiShortName(String apiShortName);

abstract Builder setApiVersion(String apiVersion);
abstract Builder setPackageVersion(String packageVersion);

abstract GapicClass build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
public abstract class Service {
public abstract String name();

@Nullable
public abstract String apiVersion();

public abstract String defaultHost();

public abstract ImmutableList<String> oauthScopes();
Expand All @@ -52,6 +55,10 @@ public boolean hasDescription() {
return !Strings.isNullOrEmpty(description());
}

public boolean hasApiVersion() {
return !Strings.isNullOrEmpty(apiVersion());
}
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved

public String hostServiceName() {
// Host Service Name is guaranteed to exist and be non-null and non-empty
// Parser will fail if the default host is not supplied
Expand All @@ -65,9 +72,9 @@ public String apiShortName() {
return "";
}

public String apiVersion() {
public String packageVersion() {
if (!Strings.isNullOrEmpty(protoPakkage())) {
return parseApiVersion(protoPakkage());
return parsePackageVersion(protoPakkage());
}
return "";
}
Expand Down Expand Up @@ -158,6 +165,8 @@ public abstract static class Builder {

public abstract Builder setOverriddenName(String overriddenName);

public abstract Builder setApiVersion(String apiVersion);

public abstract Builder setDefaultHost(String defaultHost);

public abstract Builder setOauthScopes(List<String> oauthScopes);
Expand All @@ -177,17 +186,17 @@ public abstract static class Builder {
public abstract Service build();
}

private static String parseApiVersion(String protoPackage) {
// parse protoPackage for apiVersion
private static String parsePackageVersion(String protoPackage) {
// parse protoPackage for packageVersion
String[] pakkage = protoPackage.split("\\.");
String apiVersion;
String packageVersion;
// e.g. v1, v2, v1beta1
if (pakkage[pakkage.length - 1].matches("v[0-9].*")) {
apiVersion = pakkage[pakkage.length - 1];
packageVersion = pakkage[pakkage.length - 1];
} else {
apiVersion = "";
packageVersion = "";
}
return apiVersion;
return packageVersion;
}

// Parse the service name from the default host configured in the protos
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,11 @@ public static List<Service> parseService(
}
}

if (serviceOptions.hasExtension(ClientProto.apiVersion)) {
String apiVersion = serviceOptions.getExtension(ClientProto.apiVersion);
serviceBuilder.setApiVersion(apiVersion);
}
Comment on lines +473 to +476
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

              String apiVersion = null;
              if (serviceOptions.hasExtension(ClientProto.apiVersion)) {
                apiVersion = serviceOptions.getExtension(ClientProto.apiVersion);
              }
...
              return serviceBuilder
                       .setName(serviceName)
                       ....
                       .setApiVersion(apiVersion)

I believe the Service class can take a null or empty ApiVersion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had similar concerns at first. But I believe both null and empty are covered. (I'll create additional test cases in ParserTest for it).
Note that If value is not set for serviceBuilder, it is null by default.


String serviceName = s.getName();
String overriddenServiceName = serviceName;
String pakkage = TypeParser.getPackage(fileDescriptor);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public class ComposerTest {
GrpcServiceCallableFactoryClassComposer.instance()
.generate(context, echoProtoService)
.withApiShortName(echoProtoService.apiShortName())
.withApiVersion(echoProtoService.apiVersion()));
.withPackageVersion(echoProtoService.packageVersion()));
private final Sample sample =
Sample.builder()
.setRegionTag(
Expand Down Expand Up @@ -160,7 +160,7 @@ private List<GapicClass> getTestClassListFromService(Service testService) {
.generate(context, testService)
.withSamples(ListofSamples)
.withApiShortName(testService.apiShortName())
.withApiVersion(testService.apiVersion());
.withPackageVersion(testService.packageVersion());
List<GapicClass> testClassList = Arrays.asList(new GapicClass[] {testClass});
return testClassList;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ public void codesDefinitionsBlock_noConfigsFound() {
List<Service> services =
Parser.parseService(
echoFileDescriptor, messageTypes, resourceNames, Optional.empty(), outputResourceNames);
assertEquals(1, services.size());
assertEquals(2, services.size());

Service service = services.get(0);

Expand Down Expand Up @@ -184,7 +184,7 @@ public void codesDefinitionsBlock_basic() {
List<Service> services =
Parser.parseService(
echoFileDescriptor, messageTypes, resourceNames, Optional.empty(), outputResourceNames);
assertEquals(1, services.size());
assertEquals(2, services.size());

Service service = services.get(0);

Expand Down Expand Up @@ -223,7 +223,7 @@ public void simpleBuilderExpr_basic() {
List<Service> services =
Parser.parseService(
echoFileDescriptor, messageTypes, resourceNames, Optional.empty(), outputResourceNames);
assertEquals(1, services.size());
assertEquals(2, services.size());

Service service = services.get(0);

Expand Down Expand Up @@ -304,7 +304,7 @@ public void lroBuilderExpr() {
List<Service> services =
Parser.parseService(
echoFileDescriptor, messageTypes, resourceNames, Optional.empty(), outputResourceNames);
assertEquals(1, services.size());
assertEquals(2, services.size());

Service service = services.get(0);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,26 @@
* EchoResponse response = echoClient.echo();
* }
* }</pre>
*
* <p>======================= EchoAgainClient =======================
*
* <p>Service Description: This service is used showcase the four main types of rpcs - unary, server
* side streaming, client side streaming, and bidirectional streaming. This service also exposes
* methods that explicitly implement server delay, and paginated calls. Set the 'showcase-trailer'
* metadata key on any method to have the values echoed in the response trailers.
*
* <p>Sample for EchoAgainClient:
*
* <pre>{@code
* // This snippet has been automatically generated and should be regarded as a code template only.
* // It will require modifications to work:
* // - It may require correct/in-range values for request initialization.
* // - It may require specifying regional endpoints when creating the service client as shown in
* // https://cloud.google.com/java/docs/setup#configure_endpoints_for_the_client_library
* try (EchoAgainClient echoAgainClient = EchoAgainClient.create()) {
* EchoResponse response = echoAgainClient.echo();
* }
* }</pre>
*/
@Generated("by gapic-generator-java")
package com.google.showcase.v1beta1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,6 @@ public void generateServiceClientClasses() {
Assert.assertGoldenSamples(
this.getClass(), name, clazz.classDefinition().packageString(), clazz.samples());
Assert.assertCodeEquals(clazz.apiShortName(), apiShortNameExpected);
Assert.assertCodeEquals(clazz.apiVersion(), apiVersionExpected);
Assert.assertCodeEquals(clazz.packageVersion(), apiVersionExpected);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,6 @@ public void generateServiceSettingsClasses() {
clazz.classDefinition().packageString(),
clazz.samples());
Assert.assertCodeEquals(clazz.apiShortName(), apiShortNameExpected);
Assert.assertCodeEquals(clazz.apiVersion(), apiVersionExpected);
Assert.assertCodeEquals(clazz.packageVersion(), apiVersionExpected);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,6 @@ public void generateServiceStubClasses() {
Assert.assertGoldenClass(this.getClass(), clazz, name + ".golden");
Assert.assertEmptySamples(clazz.samples());
Assert.assertCodeEquals(clazz.apiShortName(), apiShortNameExpected);
Assert.assertCodeEquals(clazz.apiVersion(), apiVersionExpected);
Assert.assertCodeEquals(clazz.packageVersion(), apiVersionExpected);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,6 @@ public void generateServiceStubSettingsClasses() {
clazz.classDefinition().packageString(),
clazz.samples());
Assert.assertCodeEquals(clazz.apiShortName(), apiShortNameExpected);
Assert.assertCodeEquals(clazz.apiVersion(), apiVersionExpected);
Assert.assertCodeEquals(clazz.packageVersion(), apiVersionExpected);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,7 @@ private static Service parseService(FileDescriptor fileDescriptor) {
List<Service> services =
Parser.parseService(
fileDescriptor, messageTypes, resourceNames, Optional.empty(), outputResourceNames);
assertEquals(1, services.size());

assertEquals(2, services.size());
return services.get(0);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,20 @@

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

import com.google.api.generator.engine.ast.TypeNode;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import java.util.Arrays;
import org.junit.Before;
import org.junit.Test;

public class ServiceTest {
private static final String SHOWCASE_PACKAGE_NAME = "com.google.showcase.v1beta1";
private static final Service.Builder testServiceBuilder =
Service.builder()
.setName("Echo")
.setDefaultHost("localhost:7469")
.setOauthScopes(Arrays.asList("https://www.googleapis.com/auth/cloud-platform"))
.setPakkage(SHOWCASE_PACKAGE_NAME)
.setProtoPakkage(SHOWCASE_PACKAGE_NAME)
.setOriginalJavaPackage(SHOWCASE_PACKAGE_NAME)
.setOverriddenName("Echo");
private Service.Builder testServiceBuilder;

private static final Method.Builder testMethodBuilder =
Method.builder()
Expand All @@ -50,6 +46,19 @@ public class ServiceTest {
.setIsAsteriskBody(false)
.setHttpVerb(HttpBindings.HttpVerb.GET);

@Before
public void init() {
testServiceBuilder =
Service.builder()
.setName("Echo")
.setDefaultHost("localhost:7469")
.setOauthScopes(Arrays.asList("https://www.googleapis.com/auth/cloud-platform"))
.setPakkage(SHOWCASE_PACKAGE_NAME)
.setProtoPakkage(SHOWCASE_PACKAGE_NAME)
.setOriginalJavaPackage(SHOWCASE_PACKAGE_NAME)
.setOverriddenName("Echo");
}
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved

@Test
public void apiShortName_shouldReturnApiShortNameIfHostContainsRegionalEndpoint() {
String defaultHost = "us-east1-pubsub.googleapis.com";
Expand Down Expand Up @@ -82,14 +91,29 @@ public void apiShortName_shouldReturnHostIfNoPeriods() {
public void apiVersion_shouldReturnVersionIfMatch() {
String protoPackage = "com.google.showcase.v1";
Service testService = testServiceBuilder.setProtoPakkage(protoPackage).build();
assertEquals("v1", testService.apiVersion());
assertEquals("v1", testService.packageVersion());
}

@Test
public void apiVersion_shouldReturnApiVersion() {
String apiVersion = "v1_20230601";
Service testService = testServiceBuilder.setApiVersion(apiVersion).build();
assertTrue(testService.hasApiVersion());
assertEquals(apiVersion, testService.apiVersion());
}

@Test
public void apiVersion_shouldReturnNullApiVersion() {
Service testService = testServiceBuilder.build();
assertNull(testService.apiVersion());
assertFalse(testService.hasApiVersion());
}

@Test
public void apiVersion_shouldReturnEmptyIfNoMatch() {
String protoPackage = "com.google.showcase";
Service testService = testServiceBuilder.setProtoPakkage(protoPackage).build();
assertEquals("", testService.apiVersion());
assertEquals("", testService.packageVersion());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;

Expand Down Expand Up @@ -607,6 +608,31 @@ public void parseNestedProtoTypeName() {
"google.ads.googleads.v3.resources.MutateJob.MutateJobMetadata"));
}

@Test
public void testServiceApiVersionParsed() {
Map<String, Message> messageTypes = Parser.parseMessages(echoFileDescriptor);
Map<String, ResourceName> resourceNames = Parser.parseResourceNames(echoFileDescriptor);
List<com.google.api.generator.gapic.model.Service> services =
Parser.parseService(
echoFileDescriptor, messageTypes, resourceNames, Optional.empty(), new HashSet<>());
com.google.api.generator.gapic.model.Service parsedEchoService = services.get(0);
assertEquals("v1_20230601", parsedEchoService.apiVersion());
com.google.api.generator.gapic.model.Service parsedEchoAgainService = services.get(1);
assertEquals("fake-version", parsedEchoAgainService.apiVersion());
}

@Test
public void testServiceWithoutApiVersionParsed() {
FileDescriptor bookshopFileDescriptor = BookshopProto.getDescriptor();
Map<String, Message> messageTypes = Parser.parseMessages(bookshopFileDescriptor);
Map<String, ResourceName> resourceNames = Parser.parseResourceNames(bookshopFileDescriptor);
List<com.google.api.generator.gapic.model.Service> services =
Parser.parseService(
bookshopFileDescriptor, messageTypes, resourceNames, Optional.empty(), new HashSet<>());
com.google.api.generator.gapic.model.Service parsedBookshopService = services.get(0);
assertNull(parsedBookshopService.apiVersion());
}

private void assertMethodArgumentEquals(
String name, TypeNode type, List<TypeNode> nestedFields, MethodArgument argument) {
assertEquals(name, argument.name());
Expand Down
Loading
Loading