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.

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

feat: Parser to consume the api-versioning value from proto #2630

Merged
merged 12 commits into from
Apr 24, 2024
Merged

feat: Parser to consume the api-versioning value from proto #2630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8772c51
chore: rename current apiVersion to apiMajorVersion.
zhumin8 Apr 8, 2024
9ec3531
continue: rename apiVersion used in sample regiontags to apiMajorVers…
zhumin8 Apr 8, 2024
3b35cd0
add new apiVersion field to Service model.
zhumin8 Apr 9, 2024
23f2ce7
add logic so Parser can handle optional api version.:
zhumin8 Apr 9, 2024
255c6f3
fix: simplify parser logic.
zhumin8 Apr 12, 2024
1ddb284
rename old apiVersion to packageVersion to disambiguous.
zhumin8 Apr 17, 2024
0e8705d
test: add additional service in echo.proto to test that api-version i…
zhumin8 Apr 18, 2024
8f1a131
Merge branch 'main' into api-version
zhumin8 Apr 18, 2024
f025a3f
fix: checkstyle
zhumin8 Apr 18, 2024
1bc4d64
test: revert changes to echo.proto, and create dedicated proto to tes…
zhumin8 Apr 23, 2024
0b57b3e
chore: rename test name and var references of old apiVersion to packa…
zhumin8 Apr 23, 2024
a28a8e8
test: add test case for service when apiVersion is empty.
zhumin8 Apr 23, 2024
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
2 changes: 1 addition & 1 deletion gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/Composer.java
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion ...va/com/google/api/generator/gapic/composer/common/AbstractServiceClientClassComposer.java
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion .../com/google/api/generator/gapic/composer/common/AbstractServiceSettingsClassComposer.java
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion .../google/api/generator/gapic/composer/common/AbstractServiceStubSettingsClassComposer.java
View file
Open in desktop
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
10 changes: 5 additions & 5 deletions gapic-generator-java/src/main/java/com/google/api/generator/gapic/model/GapicClass.java
View file
Open in desktop
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
25 changes: 17 additions & 8 deletions gapic-generator-java/src/main/java/com/google/api/generator/gapic/model/Service.java
View file
Open in desktop
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
5 changes: 5 additions & 0 deletions gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/ComposerTest.java
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions ...est/java/com/google/api/generator/gapic/composer/grpc/ServiceClientClassComposerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public static Collection<Object[]> data() {
public String apiShortNameExpected;

@Parameterized.Parameter(3)
public String apiVersionExpected;
public String packageVersionExpected;

@Test
public void generateServiceClientClasses() {
Expand All @@ -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(), packageVersionExpected);
}
}
4 changes: 2 additions & 2 deletions ...t/java/com/google/api/generator/gapic/composer/grpc/ServiceSettingsClassComposerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public static Collection<Object[]> data() {
public String apiShortNameExpected;

@Parameterized.Parameter(3)
public String apiVersionExpected;
public String packageVersionExpected;

@Test
public void generateServiceSettingsClasses() {
Expand All @@ -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(), packageVersionExpected);
}
}
4 changes: 2 additions & 2 deletions .../test/java/com/google/api/generator/gapic/composer/grpc/ServiceStubClassComposerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public static Collection<Object[]> data() {
public String apiShortNameExpected;

@Parameterized.Parameter(3)
public String apiVersionExpected;
public String packageVersionExpected;

@Test
public void generateServiceStubClasses() {
Expand All @@ -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(), packageVersionExpected);
}
}
4 changes: 2 additions & 2 deletions ...va/com/google/api/generator/gapic/composer/grpc/ServiceStubSettingsClassComposerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public static Collection<Object[]> data() {
public String apiShortNameExpected;

@Parameterized.Parameter(3)
public String apiVersionExpected;
public String packageVersionExpected;

@Test
public void generateServiceStubSettingsClasses() {
Expand All @@ -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(), packageVersionExpected);
}
}
56 changes: 44 additions & 12 deletions gapic-generator-java/src/test/java/com/google/api/generator/gapic/model/ServiceTest.java
View file
Open in desktop
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 @@ -79,17 +88,40 @@ public void apiShortName_shouldReturnHostIfNoPeriods() {
}

@Test
public void apiVersion_shouldReturnVersionIfMatch() {
public void packageVersion_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_shouldReturnEmptyIfNoMatch() {
public void packageVersion_shouldReturnEmptyIfNoMatch() {
String protoPackage = "com.google.showcase";
Service testService = testServiceBuilder.setProtoPakkage(protoPackage).build();
assertEquals("", 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_shouldReturnNoApiVersionWhenNull() {
Service testService = testServiceBuilder.build();
assertNull(testService.apiVersion());
assertFalse(testService.hasApiVersion());
}

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

@Test
Expand Down
Loading
Loading