Skip to content

Commit

Permalink
feat: Add callable getters for non-eligible or non-enabled REST metho…
Browse files Browse the repository at this point in the history
…ds (#1211)

This is a PR for Part 1 of #1117 (Override method with clearer exception messages for non-eligible and non-enabled Service RPCs). Opening this while I look at other possible approaches: 

Other approaches looked at/ to revisit:
- Override createCallableClassMembers to return Map<String, Expr> (Return either VariableExpr or ThrowExpr)
- Create a duplicate createCallableClassMembers (i.e. createInvalidCallableClassMembers) that copies functionality of createCallableClassMembers but returns Map<String, ThrowExpr>

Both approaches above had an issue when setting the return type for the callableGetter. ThrowExpr's type is always set as `UnsupportedOperationException` but the MethodDefinition's return type is not (i.e. for Streams it would be ServerStreamCallable or ClientStreamCallable/ Unary is UnaryCallable vs. Operation, etc.). Would need potentially need another mapping between callableName and method return type for ThrowExprs.

This PR's implementation is copied from: https://togithub.com/googleapis/gapic-generator-java/blob/8c5e17ba325b7711f9ba9501992ab48e736ffc18/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractServiceStubClassComposer.java#L284-L302
- Use getCallableType(protoMethod) to get the correct return type for the RPC
- ThrowExpr's return type is set to `UnsupportedOperationException`
  • Loading branch information
lqiu96 authored Jan 10, 2023
1 parent 85e837c commit 84a1355
Show file tree
Hide file tree
Showing 7 changed files with 206 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import com.google.api.generator.gapic.model.Message;
import com.google.api.generator.gapic.model.Method;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.gapic.model.Transport;
import com.google.api.generator.gapic.utils.JavaStyle;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -89,7 +90,7 @@ public abstract class AbstractTransportServiceStubClassComposer implements Class
private static final String BACKGROUND_RESOURCES_MEMBER_NAME = "backgroundResources";
private static final String CALLABLE_NAME = "Callable";
private static final String CALLABLE_FACTORY_MEMBER_NAME = "callableFactory";
private static final String CALLABLE_CLASS_MEMBER_PATTERN = "%sCallable";
protected static final String CALLABLE_CLASS_MEMBER_PATTERN = "%sCallable";
private static final String OPERATION_CALLABLE_CLASS_MEMBER_PATTERN = "%sOperationCallable";
private static final String OPERATION_CALLABLE_NAME = "OperationCallable";
// private static final String OPERATIONS_STUB_MEMBER_NAME = "operationsStub";
Expand Down Expand Up @@ -126,7 +127,8 @@ private static TypeStore createStaticTypes() {
RequestParamsExtractor.class,
ServerStreamingCallable.class,
TimeUnit.class,
UnaryCallable.class);
UnaryCallable.class,
UnsupportedOperationException.class);
return new TypeStore(concreteClazzes);
}

Expand Down Expand Up @@ -190,6 +192,19 @@ public GapicClass generate(GapicContext context, Service service) {
messageTypes,
context.restNumericEnumsEnabled());

List<MethodDefinition> methodDefinitions =
createClassMethods(
context,
service,
typeStore,
classMemberVarExprs,
callableClassMemberVarExprs,
protoMethodNameToDescriptorVarExprs,
classStatements);
methodDefinitions.addAll(
createStubOverrideMethods(
classMemberVarExprs.get(BACKGROUND_RESOURCES_MEMBER_NAME), service));

StubCommentComposer commentComposer =
new StubCommentComposer(getTransportContext().transportNames().get(0));

Expand All @@ -204,22 +219,14 @@ public GapicClass generate(GapicContext context, Service service) {
.setName(className)
.setExtendsType(
typeStore.get(getTransportContext().classNames().getServiceStubClassName(service)))
.setMethods(
createClassMethods(
context,
service,
typeStore,
classMemberVarExprs,
callableClassMemberVarExprs,
protoMethodNameToDescriptorVarExprs,
classStatements))
.setMethods(methodDefinitions)
.setStatements(classStatements)
.build();
return GapicClass.create(kind, classDef);
}

protected boolean isSupportedMethod(Method method) {
return true;
protected Transport getTransport() {
return Transport.GRPC;
}

protected abstract Statement createMethodDescriptorVariableDecl(
Expand Down Expand Up @@ -307,7 +314,7 @@ protected List<Statement> createMethodDescriptorVariableDecls(
Map<String, Message> messageTypes,
boolean restNumericEnumsEnabled) {
return service.methods().stream()
.filter(this::isSupportedMethod)
.filter(x -> x.isSupportedByTransport(getTransport()))
.map(
m ->
createMethodDescriptorVariableDecl(
Expand Down Expand Up @@ -336,7 +343,7 @@ private static List<Statement> createClassMemberFieldDeclarations(
protected Map<String, VariableExpr> createProtoMethodNameToDescriptorClassMembers(
Service service, Class<?> descriptorClass) {
return service.methods().stream()
.filter(this::isSupportedMethod)
.filter(x -> x.isSupportedByTransport(getTransport()))
.collect(
Collectors.toMap(
Method::name,
Expand Down Expand Up @@ -368,7 +375,7 @@ private Map<String, VariableExpr> createCallableClassMembers(
Map<String, VariableExpr> callableClassMembers = new LinkedHashMap<>();
// Using a for-loop because the output cardinality is not a 1:1 mapping to the input set.
for (Method protoMethod : service.methods()) {
if (!isSupportedMethod(protoMethod)) {
if (!protoMethod.isSupportedByTransport(getTransport())) {
continue;
}
String javaStyleProtoMethodName = JavaStyle.toLowerCamelCase(protoMethod.name());
Expand Down Expand Up @@ -470,9 +477,6 @@ protected List<MethodDefinition> createClassMethods(
service,
classMemberVarExprs.get(getTransportContext().transportOperationsStubNames().get(0))));
javaMethods.addAll(createCallableGetterMethods(callableClassMemberVarExprs));
javaMethods.addAll(
createStubOverrideMethods(
classMemberVarExprs.get(BACKGROUND_RESOURCES_MEMBER_NAME), service));
return javaMethods;
}

Expand Down Expand Up @@ -660,7 +664,7 @@ protected List<MethodDefinition> createConstructorMethods(
// Transport settings local variables.
Map<String, VariableExpr> javaStyleMethodNameToTransportSettingsVarExprs =
service.methods().stream()
.filter(this::isSupportedMethod)
.filter(x -> x.isSupportedByTransport(getTransport()))
.collect(
Collectors.toMap(
m -> JavaStyle.toLowerCamelCase(m.name()),
Expand All @@ -684,7 +688,7 @@ protected List<MethodDefinition> createConstructorMethods(

secondCtorExprs.addAll(
service.methods().stream()
.filter(this::isSupportedMethod)
.filter(x -> x.isSupportedByTransport(getTransport()))
.map(
m ->
createTransportSettingsInitExpr(
Expand Down Expand Up @@ -1055,7 +1059,7 @@ private List<MethodDefinition> createStubOverrideMethods(

private boolean checkOperationPollingMethod(Service service) {
return service.methods().stream()
.filter(this::isSupportedMethod)
.filter(x -> x.isSupportedByTransport(getTransport()))
.anyMatch(Method::isOperationPollingMethod);
}

Expand Down Expand Up @@ -1094,7 +1098,7 @@ private TypeStore createDynamicTypes(Service service, String stubPakkage) {
typeStore.putAll(
service.pakkage(),
service.methods().stream()
.filter(this::isSupportedMethod)
.filter(x -> x.isSupportedByTransport(getTransport()))
.filter(Method::isPaged)
.map(m -> String.format(PAGED_RESPONSE_TYPE_NAME_PATTERN, m.name()))
.collect(Collectors.toList()),
Expand All @@ -1103,7 +1107,7 @@ private TypeStore createDynamicTypes(Service service, String stubPakkage) {
return typeStore;
}

private static TypeNode getCallableType(Method protoMethod) {
protected static TypeNode getCallableType(Method protoMethod) {
TypeNode callableType = FIXED_TYPESTORE.get("UnaryCallable");
switch (protoMethod.stream()) {
case CLIENT:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,19 +45,21 @@
import com.google.api.generator.engine.ast.Statement;
import com.google.api.generator.engine.ast.StringObjectValue;
import com.google.api.generator.engine.ast.ThisObjectValue;
import com.google.api.generator.engine.ast.ThrowExpr;
import com.google.api.generator.engine.ast.TypeNode;
import com.google.api.generator.engine.ast.ValueExpr;
import com.google.api.generator.engine.ast.VaporReference;
import com.google.api.generator.engine.ast.Variable;
import com.google.api.generator.engine.ast.VariableExpr;
import com.google.api.generator.gapic.composer.common.AbstractTransportServiceStubClassComposer;
import com.google.api.generator.gapic.composer.store.TypeStore;
import com.google.api.generator.gapic.model.GapicContext;
import com.google.api.generator.gapic.model.HttpBindings.HttpBinding;
import com.google.api.generator.gapic.model.Message;
import com.google.api.generator.gapic.model.Method;
import com.google.api.generator.gapic.model.Method.Stream;
import com.google.api.generator.gapic.model.OperationResponse;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.gapic.model.Transport;
import com.google.api.generator.gapic.utils.JavaStyle;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.BiMap;
Expand Down Expand Up @@ -115,10 +117,9 @@ private static TypeStore createStaticTypes() {
TypeRegistry.class));
}

protected boolean isSupportedMethod(Method method) {
return method.httpBindings() != null
&& method.stream() != Stream.BIDI
&& method.stream() != Stream.CLIENT;
@Override
protected Transport getTransport() {
return Transport.REST;
}

@Override
Expand Down Expand Up @@ -1248,4 +1249,56 @@ protected List<Statement> createTypeRegistry(Service service) {
.setValueExpr(typeRegistryBuilderExpr)
.build()));
}

@Override
protected List<MethodDefinition> createClassMethods(
GapicContext context,
Service service,
TypeStore typeStore,
Map<String, VariableExpr> classMemberVarExprs,
Map<String, VariableExpr> callableClassMemberVarExprs,
Map<String, VariableExpr> protoMethodNameToDescriptorVarExprs,
List<Statement> classStatements) {
List<MethodDefinition> javaMethods = new ArrayList<>();
javaMethods.addAll(
super.createClassMethods(
context,
service,
typeStore,
classMemberVarExprs,
callableClassMemberVarExprs,
protoMethodNameToDescriptorVarExprs,
classStatements));
javaMethods.addAll(createInvalidClassMethods(service));
return javaMethods;
}

private List<MethodDefinition> createInvalidClassMethods(Service service) {
List<MethodDefinition> methodDefinitions = new ArrayList<>();
for (Method protoMethod : service.methods()) {
if (protoMethod.isSupportedByTransport(getTransport())) {
continue;
}
String javaStyleProtoMethodName = JavaStyle.toLowerCamelCase(protoMethod.name());
String callableName = String.format(CALLABLE_CLASS_MEMBER_PATTERN, javaStyleProtoMethodName);
methodDefinitions.add(
MethodDefinition.builder()
.setIsOverride(true)
.setScope(ScopeNode.PUBLIC)
.setName(callableName)
.setReturnType(getCallableType(protoMethod))
.setBody(
Arrays.asList(
ExprStatement.withExpr(
ThrowExpr.builder()
.setType(FIXED_TYPESTORE.get("UnsupportedOperationException"))
.setMessageExpr(
String.format(
"Not implemented: %s(). %s transport is not implemented for this method yet.",
callableName, getTransport()))
.build())))
.build());
}
return methodDefinitions;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,25 @@ public boolean isOperationPollingMethod() {
return operationPollingMethod();
}

/**
* Determines if method is both eligible and enabled for the Transport. GRPC+REST Transport is not
* supported as each transport's sub composers will invoke this method the specific transport
* (GRPC or REST)
*
* @param transport Expects either GRPC or REST Transport
* @return boolean if method should be generated for the transport
*/
public boolean isSupportedByTransport(Transport transport) {
if (transport == Transport.REST) {
return httpBindings() != null && stream() != Stream.BIDI && stream() != Stream.CLIENT;
} else if (transport == Transport.GRPC) {
return true;
} else {
throw new IllegalArgumentException(
String.format("Invalid Transport: %s. Expecting GRPC or REST", transport.name()));
}
}

public abstract Builder toBuilder();

public static Builder builder() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import com.google.api.gax.httpjson.ProtoMessageResponseParser;
import com.google.api.gax.httpjson.ProtoRestSerializer;
import com.google.api.gax.httpjson.longrunning.stub.HttpJsonOperationsStub;
import com.google.api.gax.longrunning.OperationSnapshot;
import com.google.api.gax.rpc.BidiStreamingCallable;
import com.google.api.gax.rpc.ClientContext;
import com.google.api.gax.rpc.OperationCallable;
import com.google.api.gax.rpc.ServerStreamingCallable;
Expand Down Expand Up @@ -544,6 +545,18 @@ public class HttpJsonEchoStub extends EchoStub {
return nestedBindingCallable;
}

@Override
public BidiStreamingCallable<EchoRequest, EchoResponse> chatCallable() {
throw new UnsupportedOperationException(
"Not implemented: chatCallable(). REST transport is not implemented for this method yet.");
}

@Override
public UnaryCallable<EchoRequest, EchoResponse> noBindingCallable() {
throw new UnsupportedOperationException(
"Not implemented: noBindingCallable(). REST transport is not implemented for this method yet.");
}

@Override
public final void close() {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.api.generator.gapic.model;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;

import com.google.api.generator.engine.ast.TypeNode;
import com.google.api.generator.gapic.model.HttpBindings.HttpBinding;
Expand Down Expand Up @@ -112,4 +113,61 @@ public void shouldSetParamsExtractor_shouldReturnFalseIfHasNoHttpBindingsAndNoRo
Method method = METHOD.toBuilder().setHttpBindings(null).setRoutingHeaderRule(null).build();
assertThat(method.shouldSetParamsExtractor()).isFalse();
}

@Test
public void
isSupportedByTransport_shouldReturnTrueIfHasHttpBindingsAndIsRESTEligibleForRESTTransport() {
Method methodNoStreaming =
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.NONE).build();
assertThat(methodNoStreaming.isSupportedByTransport(Transport.REST)).isTrue();
Method methodServerSideStreaming =
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.SERVER).build();
assertThat(methodServerSideStreaming.isSupportedByTransport(Transport.REST)).isTrue();
}

@Test
public void isSupportedByTransport_shouldReturnFalseIfNoHttpBindingsForRESTTransport() {
Method methodNoStreaming =
METHOD.toBuilder().setHttpBindings(null).setStream(Method.Stream.NONE).build();
assertThat(methodNoStreaming.isSupportedByTransport(Transport.REST)).isFalse();
Method methodServerSideStreaming =
METHOD.toBuilder().setHttpBindings(null).setStream(Method.Stream.SERVER).build();
assertThat(methodServerSideStreaming.isSupportedByTransport(Transport.REST)).isFalse();
}

@Test
public void
isSupportedByTransport_shouldReturnFalseIfHasHttpBindingsAndIsNotRESTEligibleForRESTTransport() {
Method methodClientSideStreaming =
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.CLIENT).build();
assertThat(methodClientSideStreaming.isSupportedByTransport(Transport.REST)).isFalse();
Method methodBiDiStreaming =
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.BIDI).build();
assertThat(methodBiDiStreaming.isSupportedByTransport(Transport.REST)).isFalse();
}

@Test
public void isSupportedByTransport_shouldReturnTrueForGRPCTransport() {
Method methodNoStreaming =
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.NONE).build();
assertThat(methodNoStreaming.isSupportedByTransport(Transport.GRPC)).isTrue();
Method methodBiDiStreaming =
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.BIDI).build();
assertThat(methodBiDiStreaming.isSupportedByTransport(Transport.GRPC)).isTrue();
Method methodNoStreamingNoHttpBindings =
METHOD.toBuilder().setStream(Method.Stream.NONE).build();
assertThat(methodNoStreamingNoHttpBindings.isSupportedByTransport(Transport.GRPC)).isTrue();
Method methodBiDiStreamingNoHttpBindings =
METHOD.toBuilder().setStream(Method.Stream.BIDI).build();
assertThat(methodBiDiStreamingNoHttpBindings.isSupportedByTransport(Transport.GRPC)).isTrue();
}

@Test
public void isSupportedByTransport_shouldThrowExceptionIfPassedGRPCRESTTransport() {
Method methodClientStreaming =
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.CLIENT).build();
assertThrows(
IllegalArgumentException.class,
() -> methodClientStreaming.isSupportedByTransport(Transport.GRPC_REST));
}
}
Loading

0 comments on commit 84a1355

Please sign in to comment.