Skip to content

Commit

Permalink
fix: allow empty services and java keywords as a method names (#985)
Browse files Browse the repository at this point in the history
This is specifically needed to enable `sqladmin` API, which has the following issues:
1) Using `import`, reserved java keyword, as a method name: https://github.com/googleapis/googleapis/blob/master/google/cloud/sql/v1/cloud_sql_instances.proto#L109
2) Has an empty service (no methods whatsoevver) defined: https://github.com/googleapis/googleapis/blob/master/google/cloud/sql/v1/cloud_sql_instance_names.proto#L31

I plan to add a full integration tests with sqladmin (which will also test pure REGAPIC case) once this is pushed (need to integrate these changes first for technical reasons).
  • Loading branch information
vam-google authored Apr 29, 2022
1 parent 04a6665 commit e37893c
Show file tree
Hide file tree
Showing 9 changed files with 248 additions and 5 deletions.
8 changes: 8 additions & 0 deletions BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,14 @@ GOLDEN_UPDATING_UNIT_TESTS = [
"com.google.api.generator.gapic.composer.grpc.ServiceSettingsClassComposerTest",
"com.google.api.generator.gapic.composer.grpc.ServiceStubClassComposerTest",
"com.google.api.generator.gapic.composer.grpc.ServiceStubSettingsClassComposerTest",
"com.google.api.generator.gapic.composer.grpcrest.GrpcServiceCallableFactoryClassComposerTest",
"com.google.api.generator.gapic.composer.grpcrest.GrpcServiceStubClassComposerTest",
"com.google.api.generator.gapic.composer.grpcrest.HttpJsonServiceCallableFactoryClassComposerTest",
"com.google.api.generator.gapic.composer.grpcrest.HttpJsonServiceClientTestClassComposerTest",
"com.google.api.generator.gapic.composer.grpcrest.HttpJsonServiceStubClassComposerTest",
"com.google.api.generator.gapic.composer.grpcrest.ServiceClientClassComposerTest",
"com.google.api.generator.gapic.composer.grpcrest.ServiceClientTestClassComposerTest",
"com.google.api.generator.gapic.composer.grpcrest.ServiceSettingsClassComposerTest",
"com.google.api.generator.gapic.composer.resourcename.ResourceNameHelperClassComposerTest",
"com.google.api.generator.gapic.composer.rest.HttpJsonServiceCallableFactoryClassComposerTest",
"com.google.api.generator.gapic.composer.rest.HttpJsonServiceStubClassComposerTest",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,7 @@ private static List<CommentStatement> createClassHeaderComments(
.filter(m -> m.stream() == Stream.NONE && !m.hasLro() && !m.isPaged())
.findFirst()
.orElse(service.methods().get(0)));
Optional<String> methodNameOpt =
methodOpt.isPresent() ? Optional.of(methodOpt.get().name()) : Optional.empty();
Optional<String> methodNameOpt = methodOpt.map(Method::name);
Optional<Sample> sampleCode =
SettingsSampleComposer.composeSettingsSample(
methodNameOpt, ClassNames.getServiceSettingsClassName(service), classType);
Expand Down Expand Up @@ -437,7 +436,7 @@ private static Map<String, VariableExpr> createMethodSettingsClassMemberVarExprs
!Objects.isNull(serviceConfig) && serviceConfig.hasBatchingSetting(service, method);
TypeNode settingsType =
getCallSettingsType(method, typeStore, hasBatchingSettings, isNestedClass);
String varName = JavaStyle.toLowerCamelCase(String.format("%sSettings", method.name()));
String varName = String.format("%sSettings", JavaStyle.toLowerCamelCase(method.name()));
if (method.isDeprecated()) {
deprecatedSettingVarNames.add(varName);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,18 @@ public static Sample composeClassHeaderSample(
TypeNode clientType,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes) {
if (service.methods().isEmpty()) {
return ServiceClientMethodSampleComposer.composeEmptyServiceSample(clientType);
}

// Use the first pure unary RPC method's sample code as showcase, if no such method exists, use
// the first method in the service's methods list.
Method method =
service.methods().stream()
.filter(m -> m.stream() == Method.Stream.NONE && !m.hasLro() && !m.isPaged())
.findFirst()
.orElse(service.methods().get(0));

if (method.stream() == Method.Stream.NONE) {
if (method.methodSignatures().isEmpty()) {
return ServiceClientMethodSampleComposer.composeCanonicalSample(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,36 @@
import java.util.stream.Collectors;

public class ServiceClientMethodSampleComposer {
// Creates an example for an empty service (no API methods), which is a corner case but can
// happen. Generated example will only show how to instantiate the client class but will not call
// any API methods (because there are no API methods).
public static Sample composeEmptyServiceSample(TypeNode clientType) {
VariableExpr clientVarExpr =
VariableExpr.withVariable(
Variable.builder()
.setName(JavaStyle.toLowerCamelCase(clientType.reference().name()))
.setType(clientType)
.build());

List<Statement> bodyStatements = new ArrayList<>();

RegionTag regionTag =
RegionTag.builder()
.setServiceName(clientVarExpr.variable().identifier().name())
.setRpcName("emtpy")
.build();

List<Statement> body =
Arrays.asList(
TryCatchStatement.builder()
.setTryResourceExpr(
SampleComposerUtil.assignClientVariableWithCreateMethodExpr(clientVarExpr))
.setTryBody(bodyStatements)
.setIsSampleCode(true)
.build());
return Sample.builder().setBody(body).setRegionTag(regionTag).setIsCanonical(true).build();
}

public static Sample composeCanonicalSample(
Method method,
TypeNode clientType,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.api.generator.gapic.utils;

import com.google.api.generator.engine.lexicon.Keyword;
import com.google.common.base.CaseFormat;
import com.google.common.base.Strings;
import java.util.stream.IntStream;
Expand All @@ -32,8 +33,13 @@ public static String toLowerCamelCase(String s) {
s = CaseFormat.LOWER_UNDERSCORE.to(CaseFormat.UPPER_CAMEL, s);
}

return capitalizeLettersAfterDigits(
String.format("%s%s", s.substring(0, 1).toLowerCase(), s.substring(1)));
String name =
capitalizeLettersAfterDigits(
String.format("%s%s", s.substring(0, 1).toLowerCase(), s.substring(1)));

// Some APIs use legit java keywords as method names. Both protobuf and gGRPC add an underscore
// in generated stubs to resolve name conflict, so we need to do the same.
return Keyword.isKeyword(name) ? name + '_' : name;
}

public static String toUpperCamelCase(String s) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,17 @@ public void generateServiceClasses() {
Path goldenFilePath = Paths.get(Utils.getGoldenDir(this.getClass()), "EchoClient.golden");
Assert.assertCodeEquals(goldenFilePath, visitor.write());
}

@Test
public void generateServiceClassesEmpty() {
GapicContext context = GrpcRestTestProtoLoader.instance().parseShowcaseEcho();
Service echoProtoService = context.services().get(1);
GapicClass clazz = ServiceClientClassComposer.instance().generate(context, echoProtoService);

JavaWriterVisitor visitor = new JavaWriterVisitor();
clazz.classDefinition().accept(visitor);
Utils.saveCodegenToFile(this.getClass(), "EchoEmpty.golden", visitor.write());
Path goldenFilePath = Paths.get(Utils.getGoldenDir(this.getClass()), "EchoEmpty.golden");
Assert.assertCodeEquals(goldenFilePath, visitor.write());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
package com.google.showcase.grpcrest.v1beta1;

import com.google.api.core.BetaApi;
import com.google.api.gax.core.BackgroundResource;
import com.google.showcase.grpcrest.v1beta1.stub.EchoEmpyStub;
import com.google.showcase.grpcrest.v1beta1.stub.EchoEmpyStubSettings;
import java.io.IOException;
import java.util.concurrent.TimeUnit;
import javax.annotation.Generated;

// AUTO-GENERATED DOCUMENTATION AND CLASS.
/**
* This class provides the ability to make remote calls to the backing service through method calls
* that map to API methods. Sample code to get started:
*
* <pre>{@code
* // This snippet has been automatically generated for illustrative purposes only.
* // It may require modifications to work in your environment.
* try (EchoEmpyClient echoEmpyClient = EchoEmpyClient.create()) {}
* }</pre>
*
* <p>Note: close() needs to be called on the EchoEmpyClient object to clean up resources such as
* threads. In the example above, try-with-resources is used, which automatically calls close().
*
* <p>The surface of this class includes several types of Java methods for each of the API's
* methods:
*
* <ol>
* <li>A "flattened" method. With this type of method, the fields of the request type have been
* converted into function parameters. It may be the case that not all fields are available as
* parameters, and not every API method will have a flattened method entry point.
* <li>A "request object" method. This type of method only takes one parameter, a request object,
* which must be constructed before the call. Not every API method will have a request object
* method.
* <li>A "callable" method. This type of method takes no parameters and returns an immutable API
* callable object, which can be used to initiate calls to the service.
* </ol>
*
* <p>See the individual methods for example code.
*
* <p>Many parameters require resource names to be formatted in a particular way. To assist with
* these names, this class includes a format method for each type of name, and additionally a parse
* method to extract the individual identifiers contained within names that are returned.
*
* <p>This class can be customized by passing in a custom instance of EchoEmpySettings to create().
* For example:
*
* <p>To customize credentials:
*
* <pre>{@code
* // This snippet has been automatically generated for illustrative purposes only.
* // It may require modifications to work in your environment.
* EchoEmpySettings echoEmpySettings =
* EchoEmpySettings.newBuilder()
* .setCredentialsProvider(FixedCredentialsProvider.create(myCredentials))
* .build();
* EchoEmpyClient echoEmpyClient = EchoEmpyClient.create(echoEmpySettings);
* }</pre>
*
* <p>To customize the endpoint:
*
* <pre>{@code
* // This snippet has been automatically generated for illustrative purposes only.
* // It may require modifications to work in your environment.
* EchoEmpySettings echoEmpySettings =
* EchoEmpySettings.newBuilder().setEndpoint(myEndpoint).build();
* EchoEmpyClient echoEmpyClient = EchoEmpyClient.create(echoEmpySettings);
* }</pre>
*
* <p>To use REST (HTTP1.1/JSON) transport (instead of gRPC) for sending an receiving requests over
* the wire:
*
* <pre>{@code
* // This snippet has been automatically generated for illustrative purposes only.
* // It may require modifications to work in your environment.
* EchoEmpySettings echoEmpySettings =
* EchoEmpySettings.newBuilder()
* .setTransportChannelProvider(
* EchoEmpySettings.defaultHttpJsonTransportProviderBuilder().build())
* .build();
* EchoEmpyClient echoEmpyClient = EchoEmpyClient.create(echoEmpySettings);
* }</pre>
*
* <p>Please refer to the GitHub repository's samples for more quickstart code snippets.
*/
@BetaApi
@Generated("by gapic-generator-java")
public class EchoEmpyClient implements BackgroundResource {
private final EchoEmpySettings settings;
private final EchoEmpyStub stub;

/** Constructs an instance of EchoEmpyClient with default settings. */
public static final EchoEmpyClient create() throws IOException {
return create(EchoEmpySettings.newBuilder().build());
}

/**
* Constructs an instance of EchoEmpyClient, using the given settings. The channels are created
* based on the settings passed in, or defaults for any settings that are not set.
*/
public static final EchoEmpyClient create(EchoEmpySettings settings) throws IOException {
return new EchoEmpyClient(settings);
}

/**
* Constructs an instance of EchoEmpyClient, using the given stub for making calls. This is for
* advanced usage - prefer using create(EchoEmpySettings).
*/
@BetaApi("A restructuring of stub classes is planned, so this may break in the future")
public static final EchoEmpyClient create(EchoEmpyStub stub) {
return new EchoEmpyClient(stub);
}

/**
* Constructs an instance of EchoEmpyClient, using the given settings. This is protected so that
* it is easy to make a subclass, but otherwise, the static factory methods should be preferred.
*/
protected EchoEmpyClient(EchoEmpySettings settings) throws IOException {
this.settings = settings;
this.stub = ((EchoEmpyStubSettings) settings.getStubSettings()).createStub();
}

@BetaApi("A restructuring of stub classes is planned, so this may break in the future")
protected EchoEmpyClient(EchoEmpyStub stub) {
this.settings = null;
this.stub = stub;
}

public final EchoEmpySettings getSettings() {
return settings;
}

@BetaApi("A restructuring of stub classes is planned, so this may break in the future")
public EchoEmpyStub getStub() {
return stub;
}

@Override
public final void close() {
stub.close();
}

@Override
public void shutdown() {
stub.shutdown();
}

@Override
public boolean isShutdown() {
return stub.isShutdown();
}

@Override
public boolean isTerminated() {
return stub.isTerminated();
}

@Override
public void shutdownNow() {
stub.shutdownNow();
}

@Override
public boolean awaitTermination(long duration, TimeUnit unit) throws InterruptedException {
return stub.awaitTermination(duration, unit);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,14 @@ public void acronyms() {
assertEquals("iamHttpXmlDog", JavaStyle.toLowerCamelCase(value));
assertEquals("IamHttpXmlDog", JavaStyle.toUpperCamelCase(value));
}

@Test
public void keyword() {
String value = "import";
assertEquals("import_", JavaStyle.toLowerCamelCase(value));
assertEquals("Import", JavaStyle.toUpperCamelCase(value));
value = "IMPORT_";
assertEquals("import_", JavaStyle.toLowerCamelCase(value));
assertEquals("Import", JavaStyle.toUpperCamelCase(value));
}
}
5 changes: 5 additions & 0 deletions src/test/proto/echo_grpcrest.proto
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ service Echo {
}
}

// Generator should not fail when encounter a service without methods
service EchoEmpy {
option (google.api.default_host) = "localhost:7469";
}

// A severity enum used to test enum capabilities in GAPIC surfaces
enum Severity {
UNNECESSARY = 0;
Expand Down

0 comments on commit e37893c

Please sign in to comment.