Skip to content

Commit 84a1355

Browse files
lqiu96lqiu96
authored
feat: Add callable getters for non-eligible or non-enabled REST methods (#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`
1 parent 85e837c commit 84a1355

File tree

7 files changed

+206
-29
lines changed

7 files changed

+206
-29
lines changed

‎gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java

+28-24
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
import com.google.api.generator.gapic.model.Message;
6060
import com.google.api.generator.gapic.model.Method;
6161
import com.google.api.generator.gapic.model.Service;
62+
import com.google.api.generator.gapic.model.Transport;
6263
import com.google.api.generator.gapic.utils.JavaStyle;
6364
import com.google.common.base.Preconditions;
6465
import com.google.common.collect.ImmutableList;
@@ -89,7 +90,7 @@ public abstract class AbstractTransportServiceStubClassComposer implements Class
8990
private static final String BACKGROUND_RESOURCES_MEMBER_NAME = "backgroundResources";
9091
private static final String CALLABLE_NAME = "Callable";
9192
private static final String CALLABLE_FACTORY_MEMBER_NAME = "callableFactory";
92-
private static final String CALLABLE_CLASS_MEMBER_PATTERN = "%sCallable";
93+
protected static final String CALLABLE_CLASS_MEMBER_PATTERN = "%sCallable";
9394
private static final String OPERATION_CALLABLE_CLASS_MEMBER_PATTERN = "%sOperationCallable";
9495
private static final String OPERATION_CALLABLE_NAME = "OperationCallable";
9596
// private static final String OPERATIONS_STUB_MEMBER_NAME = "operationsStub";
@@ -126,7 +127,8 @@ private static TypeStore createStaticTypes() {
126127
RequestParamsExtractor.class,
127128
ServerStreamingCallable.class,
128129
TimeUnit.class,
129-
UnaryCallable.class);
130+
UnaryCallable.class,
131+
UnsupportedOperationException.class);
130132
return new TypeStore(concreteClazzes);
131133
}
132134

@@ -190,6 +192,19 @@ public GapicClass generate(GapicContext context, Service service) {
190192
messageTypes,
191193
context.restNumericEnumsEnabled());
192194

195+
List<MethodDefinition> methodDefinitions =
196+
createClassMethods(
197+
context,
198+
service,
199+
typeStore,
200+
classMemberVarExprs,
201+
callableClassMemberVarExprs,
202+
protoMethodNameToDescriptorVarExprs,
203+
classStatements);
204+
methodDefinitions.addAll(
205+
createStubOverrideMethods(
206+
classMemberVarExprs.get(BACKGROUND_RESOURCES_MEMBER_NAME), service));
207+
193208
StubCommentComposer commentComposer =
194209
new StubCommentComposer(getTransportContext().transportNames().get(0));
195210

@@ -204,22 +219,14 @@ public GapicClass generate(GapicContext context, Service service) {
204219
.setName(className)
205220
.setExtendsType(
206221
typeStore.get(getTransportContext().classNames().getServiceStubClassName(service)))
207-
.setMethods(
208-
createClassMethods(
209-
context,
210-
service,
211-
typeStore,
212-
classMemberVarExprs,
213-
callableClassMemberVarExprs,
214-
protoMethodNameToDescriptorVarExprs,
215-
classStatements))
222+
.setMethods(methodDefinitions)
216223
.setStatements(classStatements)
217224
.build();
218225
return GapicClass.create(kind, classDef);
219226
}
220227

221-
protected boolean isSupportedMethod(Method method) {
222-
return true;
228+
protected Transport getTransport() {
229+
return Transport.GRPC;
223230
}
224231

225232
protected abstract Statement createMethodDescriptorVariableDecl(
@@ -307,7 +314,7 @@ protected List<Statement> createMethodDescriptorVariableDecls(
307314
Map<String, Message> messageTypes,
308315
boolean restNumericEnumsEnabled) {
309316
return service.methods().stream()
310-
.filter(this::isSupportedMethod)
317+
.filter(x -> x.isSupportedByTransport(getTransport()))
311318
.map(
312319
m ->
313320
createMethodDescriptorVariableDecl(
@@ -336,7 +343,7 @@ private static List<Statement> createClassMemberFieldDeclarations(
336343
protected Map<String, VariableExpr> createProtoMethodNameToDescriptorClassMembers(
337344
Service service, Class<?> descriptorClass) {
338345
return service.methods().stream()
339-
.filter(this::isSupportedMethod)
346+
.filter(x -> x.isSupportedByTransport(getTransport()))
340347
.collect(
341348
Collectors.toMap(
342349
Method::name,
@@ -368,7 +375,7 @@ private Map<String, VariableExpr> createCallableClassMembers(
368375
Map<String, VariableExpr> callableClassMembers = new LinkedHashMap<>();
369376
// Using a for-loop because the output cardinality is not a 1:1 mapping to the input set.
370377
for (Method protoMethod : service.methods()) {
371-
if (!isSupportedMethod(protoMethod)) {
378+
if (!protoMethod.isSupportedByTransport(getTransport())) {
372379
continue;
373380
}
374381
String javaStyleProtoMethodName = JavaStyle.toLowerCamelCase(protoMethod.name());
@@ -470,9 +477,6 @@ protected List<MethodDefinition> createClassMethods(
470477
service,
471478
classMemberVarExprs.get(getTransportContext().transportOperationsStubNames().get(0))));
472479
javaMethods.addAll(createCallableGetterMethods(callableClassMemberVarExprs));
473-
javaMethods.addAll(
474-
createStubOverrideMethods(
475-
classMemberVarExprs.get(BACKGROUND_RESOURCES_MEMBER_NAME), service));
476480
return javaMethods;
477481
}
478482

@@ -660,7 +664,7 @@ protected List<MethodDefinition> createConstructorMethods(
660664
// Transport settings local variables.
661665
Map<String, VariableExpr> javaStyleMethodNameToTransportSettingsVarExprs =
662666
service.methods().stream()
663-
.filter(this::isSupportedMethod)
667+
.filter(x -> x.isSupportedByTransport(getTransport()))
664668
.collect(
665669
Collectors.toMap(
666670
m -> JavaStyle.toLowerCamelCase(m.name()),
@@ -684,7 +688,7 @@ protected List<MethodDefinition> createConstructorMethods(
684688

685689
secondCtorExprs.addAll(
686690
service.methods().stream()
687-
.filter(this::isSupportedMethod)
691+
.filter(x -> x.isSupportedByTransport(getTransport()))
688692
.map(
689693
m ->
690694
createTransportSettingsInitExpr(
@@ -1055,7 +1059,7 @@ private List<MethodDefinition> createStubOverrideMethods(
10551059

10561060
private boolean checkOperationPollingMethod(Service service) {
10571061
return service.methods().stream()
1058-
.filter(this::isSupportedMethod)
1062+
.filter(x -> x.isSupportedByTransport(getTransport()))
10591063
.anyMatch(Method::isOperationPollingMethod);
10601064
}
10611065

@@ -1094,7 +1098,7 @@ private TypeStore createDynamicTypes(Service service, String stubPakkage) {
10941098
typeStore.putAll(
10951099
service.pakkage(),
10961100
service.methods().stream()
1097-
.filter(this::isSupportedMethod)
1101+
.filter(x -> x.isSupportedByTransport(getTransport()))
10981102
.filter(Method::isPaged)
10991103
.map(m -> String.format(PAGED_RESPONSE_TYPE_NAME_PATTERN, m.name()))
11001104
.collect(Collectors.toList()),
@@ -1103,7 +1107,7 @@ private TypeStore createDynamicTypes(Service service, String stubPakkage) {
11031107
return typeStore;
11041108
}
11051109

1106-
private static TypeNode getCallableType(Method protoMethod) {
1110+
protected static TypeNode getCallableType(Method protoMethod) {
11071111
TypeNode callableType = FIXED_TYPESTORE.get("UnaryCallable");
11081112
switch (protoMethod.stream()) {
11091113
case CLIENT:

‎gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java

+58-5
Original file line numberDiff line numberDiff line change
@@ -45,19 +45,21 @@
4545
import com.google.api.generator.engine.ast.Statement;
4646
import com.google.api.generator.engine.ast.StringObjectValue;
4747
import com.google.api.generator.engine.ast.ThisObjectValue;
48+
import com.google.api.generator.engine.ast.ThrowExpr;
4849
import com.google.api.generator.engine.ast.TypeNode;
4950
import com.google.api.generator.engine.ast.ValueExpr;
5051
import com.google.api.generator.engine.ast.VaporReference;
5152
import com.google.api.generator.engine.ast.Variable;
5253
import com.google.api.generator.engine.ast.VariableExpr;
5354
import com.google.api.generator.gapic.composer.common.AbstractTransportServiceStubClassComposer;
5455
import com.google.api.generator.gapic.composer.store.TypeStore;
56+
import com.google.api.generator.gapic.model.GapicContext;
5557
import com.google.api.generator.gapic.model.HttpBindings.HttpBinding;
5658
import com.google.api.generator.gapic.model.Message;
5759
import com.google.api.generator.gapic.model.Method;
58-
import com.google.api.generator.gapic.model.Method.Stream;
5960
import com.google.api.generator.gapic.model.OperationResponse;
6061
import com.google.api.generator.gapic.model.Service;
62+
import com.google.api.generator.gapic.model.Transport;
6163
import com.google.api.generator.gapic.utils.JavaStyle;
6264
import com.google.common.annotations.VisibleForTesting;
6365
import com.google.common.collect.BiMap;
@@ -115,10 +117,9 @@ private static TypeStore createStaticTypes() {
115117
TypeRegistry.class));
116118
}
117119

118-
protected boolean isSupportedMethod(Method method) {
119-
return method.httpBindings() != null
120-
&& method.stream() != Stream.BIDI
121-
&& method.stream() != Stream.CLIENT;
120+
@Override
121+
protected Transport getTransport() {
122+
return Transport.REST;
122123
}
123124

124125
@Override
@@ -1248,4 +1249,56 @@ protected List<Statement> createTypeRegistry(Service service) {
12481249
.setValueExpr(typeRegistryBuilderExpr)
12491250
.build()));
12501251
}
1252+
1253+
@Override
1254+
protected List<MethodDefinition> createClassMethods(
1255+
GapicContext context,
1256+
Service service,
1257+
TypeStore typeStore,
1258+
Map<String, VariableExpr> classMemberVarExprs,
1259+
Map<String, VariableExpr> callableClassMemberVarExprs,
1260+
Map<String, VariableExpr> protoMethodNameToDescriptorVarExprs,
1261+
List<Statement> classStatements) {
1262+
List<MethodDefinition> javaMethods = new ArrayList<>();
1263+
javaMethods.addAll(
1264+
super.createClassMethods(
1265+
context,
1266+
service,
1267+
typeStore,
1268+
classMemberVarExprs,
1269+
callableClassMemberVarExprs,
1270+
protoMethodNameToDescriptorVarExprs,
1271+
classStatements));
1272+
javaMethods.addAll(createInvalidClassMethods(service));
1273+
return javaMethods;
1274+
}
1275+
1276+
private List<MethodDefinition> createInvalidClassMethods(Service service) {
1277+
List<MethodDefinition> methodDefinitions = new ArrayList<>();
1278+
for (Method protoMethod : service.methods()) {
1279+
if (protoMethod.isSupportedByTransport(getTransport())) {
1280+
continue;
1281+
}
1282+
String javaStyleProtoMethodName = JavaStyle.toLowerCamelCase(protoMethod.name());
1283+
String callableName = String.format(CALLABLE_CLASS_MEMBER_PATTERN, javaStyleProtoMethodName);
1284+
methodDefinitions.add(
1285+
MethodDefinition.builder()
1286+
.setIsOverride(true)
1287+
.setScope(ScopeNode.PUBLIC)
1288+
.setName(callableName)
1289+
.setReturnType(getCallableType(protoMethod))
1290+
.setBody(
1291+
Arrays.asList(
1292+
ExprStatement.withExpr(
1293+
ThrowExpr.builder()
1294+
.setType(FIXED_TYPESTORE.get("UnsupportedOperationException"))
1295+
.setMessageExpr(
1296+
String.format(
1297+
"Not implemented: %s(). %s transport is not implemented for this method yet.",
1298+
callableName, getTransport()))
1299+
.build())))
1300+
.build());
1301+
}
1302+
return methodDefinitions;
1303+
}
12511304
}

‎gapic-generator-java/src/main/java/com/google/api/generator/gapic/model/Method.java

+19
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,25 @@ public boolean isOperationPollingMethod() {
9999
return operationPollingMethod();
100100
}
101101

102+
/**
103+
* Determines if method is both eligible and enabled for the Transport. GRPC+REST Transport is not
104+
* supported as each transport's sub composers will invoke this method the specific transport
105+
* (GRPC or REST)
106+
*
107+
* @param transport Expects either GRPC or REST Transport
108+
* @return boolean if method should be generated for the transport
109+
*/
110+
public boolean isSupportedByTransport(Transport transport) {
111+
if (transport == Transport.REST) {
112+
return httpBindings() != null && stream() != Stream.BIDI && stream() != Stream.CLIENT;
113+
} else if (transport == Transport.GRPC) {
114+
return true;
115+
} else {
116+
throw new IllegalArgumentException(
117+
String.format("Invalid Transport: %s. Expecting GRPC or REST", transport.name()));
118+
}
119+
}
120+
102121
public abstract Builder toBuilder();
103122

104123
public static Builder builder() {

‎gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/HttpJsonEchoStub.golden

+13
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import com.google.api.gax.httpjson.ProtoMessageResponseParser;
1616
import com.google.api.gax.httpjson.ProtoRestSerializer;
1717
import com.google.api.gax.httpjson.longrunning.stub.HttpJsonOperationsStub;
1818
import com.google.api.gax.longrunning.OperationSnapshot;
19+
import com.google.api.gax.rpc.BidiStreamingCallable;
1920
import com.google.api.gax.rpc.ClientContext;
2021
import com.google.api.gax.rpc.OperationCallable;
2122
import com.google.api.gax.rpc.ServerStreamingCallable;
@@ -544,6 +545,18 @@ public class HttpJsonEchoStub extends EchoStub {
544545
return nestedBindingCallable;
545546
}
546547

548+
@Override
549+
public BidiStreamingCallable<EchoRequest, EchoResponse> chatCallable() {
550+
throw new UnsupportedOperationException(
551+
"Not implemented: chatCallable(). REST transport is not implemented for this method yet.");
552+
}
553+
554+
@Override
555+
public UnaryCallable<EchoRequest, EchoResponse> noBindingCallable() {
556+
throw new UnsupportedOperationException(
557+
"Not implemented: noBindingCallable(). REST transport is not implemented for this method yet.");
558+
}
559+
547560
@Override
548561
public final void close() {
549562
try {

‎gapic-generator-java/src/test/java/com/google/api/generator/gapic/model/MethodTest.java

+58
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package com.google.api.generator.gapic.model;
1616

1717
import static com.google.common.truth.Truth.assertThat;
18+
import static org.junit.Assert.assertThrows;
1819

1920
import com.google.api.generator.engine.ast.TypeNode;
2021
import com.google.api.generator.gapic.model.HttpBindings.HttpBinding;
@@ -112,4 +113,61 @@ public void shouldSetParamsExtractor_shouldReturnFalseIfHasNoHttpBindingsAndNoRo
112113
Method method = METHOD.toBuilder().setHttpBindings(null).setRoutingHeaderRule(null).build();
113114
assertThat(method.shouldSetParamsExtractor()).isFalse();
114115
}
116+
117+
@Test
118+
public void
119+
isSupportedByTransport_shouldReturnTrueIfHasHttpBindingsAndIsRESTEligibleForRESTTransport() {
120+
Method methodNoStreaming =
121+
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.NONE).build();
122+
assertThat(methodNoStreaming.isSupportedByTransport(Transport.REST)).isTrue();
123+
Method methodServerSideStreaming =
124+
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.SERVER).build();
125+
assertThat(methodServerSideStreaming.isSupportedByTransport(Transport.REST)).isTrue();
126+
}
127+
128+
@Test
129+
public void isSupportedByTransport_shouldReturnFalseIfNoHttpBindingsForRESTTransport() {
130+
Method methodNoStreaming =
131+
METHOD.toBuilder().setHttpBindings(null).setStream(Method.Stream.NONE).build();
132+
assertThat(methodNoStreaming.isSupportedByTransport(Transport.REST)).isFalse();
133+
Method methodServerSideStreaming =
134+
METHOD.toBuilder().setHttpBindings(null).setStream(Method.Stream.SERVER).build();
135+
assertThat(methodServerSideStreaming.isSupportedByTransport(Transport.REST)).isFalse();
136+
}
137+
138+
@Test
139+
public void
140+
isSupportedByTransport_shouldReturnFalseIfHasHttpBindingsAndIsNotRESTEligibleForRESTTransport() {
141+
Method methodClientSideStreaming =
142+
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.CLIENT).build();
143+
assertThat(methodClientSideStreaming.isSupportedByTransport(Transport.REST)).isFalse();
144+
Method methodBiDiStreaming =
145+
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.BIDI).build();
146+
assertThat(methodBiDiStreaming.isSupportedByTransport(Transport.REST)).isFalse();
147+
}
148+
149+
@Test
150+
public void isSupportedByTransport_shouldReturnTrueForGRPCTransport() {
151+
Method methodNoStreaming =
152+
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.NONE).build();
153+
assertThat(methodNoStreaming.isSupportedByTransport(Transport.GRPC)).isTrue();
154+
Method methodBiDiStreaming =
155+
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.BIDI).build();
156+
assertThat(methodBiDiStreaming.isSupportedByTransport(Transport.GRPC)).isTrue();
157+
Method methodNoStreamingNoHttpBindings =
158+
METHOD.toBuilder().setStream(Method.Stream.NONE).build();
159+
assertThat(methodNoStreamingNoHttpBindings.isSupportedByTransport(Transport.GRPC)).isTrue();
160+
Method methodBiDiStreamingNoHttpBindings =
161+
METHOD.toBuilder().setStream(Method.Stream.BIDI).build();
162+
assertThat(methodBiDiStreamingNoHttpBindings.isSupportedByTransport(Transport.GRPC)).isTrue();
163+
}
164+
165+
@Test
166+
public void isSupportedByTransport_shouldThrowExceptionIfPassedGRPCRESTTransport() {
167+
Method methodClientStreaming =
168+
METHOD.toBuilder().setHttpBindings(HTTP_BINDINGS).setStream(Method.Stream.CLIENT).build();
169+
assertThrows(
170+
IllegalArgumentException.class,
171+
() -> methodClientStreaming.isSupportedByTransport(Transport.GRPC_REST));
172+
}
115173
}

0 commit comments

Comments
 (0)