From 84a1355a6db4754404196f153958359f9ba55437 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Tue, 10 Jan 2023 15:54:12 -0500 Subject: [PATCH] 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 (Return either VariableExpr or ThrowExpr) - Create a duplicate createCallableClassMembers (i.e. createInvalidCallableClassMembers) that copies functionality of createCallableClassMembers but returns Map 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` --- ...ractTransportServiceStubClassComposer.java | 52 ++++++++------- .../HttpJsonServiceStubClassComposer.java | 63 +++++++++++++++++-- .../api/generator/gapic/model/Method.java | 19 ++++++ .../grpcrest/goldens/HttpJsonEchoStub.golden | 13 ++++ .../api/generator/gapic/model/MethodTest.java | 58 +++++++++++++++++ .../v1beta1/stub/HttpJsonEchoStub.java | 14 +++++ .../v1beta1/stub/HttpJsonMessagingStub.java | 16 +++++ 7 files changed, 206 insertions(+), 29 deletions(-) diff --git a/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java b/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java index c2f4fb7473..ebbaac698f 100644 --- a/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java +++ b/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java @@ -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; @@ -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"; @@ -126,7 +127,8 @@ private static TypeStore createStaticTypes() { RequestParamsExtractor.class, ServerStreamingCallable.class, TimeUnit.class, - UnaryCallable.class); + UnaryCallable.class, + UnsupportedOperationException.class); return new TypeStore(concreteClazzes); } @@ -190,6 +192,19 @@ public GapicClass generate(GapicContext context, Service service) { messageTypes, context.restNumericEnumsEnabled()); + List 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)); @@ -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( @@ -307,7 +314,7 @@ protected List createMethodDescriptorVariableDecls( Map messageTypes, boolean restNumericEnumsEnabled) { return service.methods().stream() - .filter(this::isSupportedMethod) + .filter(x -> x.isSupportedByTransport(getTransport())) .map( m -> createMethodDescriptorVariableDecl( @@ -336,7 +343,7 @@ private static List createClassMemberFieldDeclarations( protected Map createProtoMethodNameToDescriptorClassMembers( Service service, Class descriptorClass) { return service.methods().stream() - .filter(this::isSupportedMethod) + .filter(x -> x.isSupportedByTransport(getTransport())) .collect( Collectors.toMap( Method::name, @@ -368,7 +375,7 @@ private Map createCallableClassMembers( Map 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()); @@ -470,9 +477,6 @@ protected List createClassMethods( service, classMemberVarExprs.get(getTransportContext().transportOperationsStubNames().get(0)))); javaMethods.addAll(createCallableGetterMethods(callableClassMemberVarExprs)); - javaMethods.addAll( - createStubOverrideMethods( - classMemberVarExprs.get(BACKGROUND_RESOURCES_MEMBER_NAME), service)); return javaMethods; } @@ -660,7 +664,7 @@ protected List createConstructorMethods( // Transport settings local variables. Map javaStyleMethodNameToTransportSettingsVarExprs = service.methods().stream() - .filter(this::isSupportedMethod) + .filter(x -> x.isSupportedByTransport(getTransport())) .collect( Collectors.toMap( m -> JavaStyle.toLowerCamelCase(m.name()), @@ -684,7 +688,7 @@ protected List createConstructorMethods( secondCtorExprs.addAll( service.methods().stream() - .filter(this::isSupportedMethod) + .filter(x -> x.isSupportedByTransport(getTransport())) .map( m -> createTransportSettingsInitExpr( @@ -1055,7 +1059,7 @@ private List createStubOverrideMethods( private boolean checkOperationPollingMethod(Service service) { return service.methods().stream() - .filter(this::isSupportedMethod) + .filter(x -> x.isSupportedByTransport(getTransport())) .anyMatch(Method::isOperationPollingMethod); } @@ -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()), @@ -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: diff --git a/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java b/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java index 67e003f50f..277a5f2c7e 100644 --- a/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java +++ b/gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java @@ -45,6 +45,7 @@ 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; @@ -52,12 +53,13 @@ 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; @@ -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 @@ -1248,4 +1249,56 @@ protected List createTypeRegistry(Service service) { .setValueExpr(typeRegistryBuilderExpr) .build())); } + + @Override + protected List createClassMethods( + GapicContext context, + Service service, + TypeStore typeStore, + Map classMemberVarExprs, + Map callableClassMemberVarExprs, + Map protoMethodNameToDescriptorVarExprs, + List classStatements) { + List javaMethods = new ArrayList<>(); + javaMethods.addAll( + super.createClassMethods( + context, + service, + typeStore, + classMemberVarExprs, + callableClassMemberVarExprs, + protoMethodNameToDescriptorVarExprs, + classStatements)); + javaMethods.addAll(createInvalidClassMethods(service)); + return javaMethods; + } + + private List createInvalidClassMethods(Service service) { + List 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; + } } diff --git a/gapic-generator-java/src/main/java/com/google/api/generator/gapic/model/Method.java b/gapic-generator-java/src/main/java/com/google/api/generator/gapic/model/Method.java index dc2452f2d5..4fd5c4f384 100644 --- a/gapic-generator-java/src/main/java/com/google/api/generator/gapic/model/Method.java +++ b/gapic-generator-java/src/main/java/com/google/api/generator/gapic/model/Method.java @@ -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() { diff --git a/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/HttpJsonEchoStub.golden b/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/HttpJsonEchoStub.golden index 6d6d179d9b..d027d9ed10 100644 --- a/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/HttpJsonEchoStub.golden +++ b/gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/HttpJsonEchoStub.golden @@ -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; @@ -544,6 +545,18 @@ public class HttpJsonEchoStub extends EchoStub { return nestedBindingCallable; } + @Override + public BidiStreamingCallable chatCallable() { + throw new UnsupportedOperationException( + "Not implemented: chatCallable(). REST transport is not implemented for this method yet."); + } + + @Override + public UnaryCallable noBindingCallable() { + throw new UnsupportedOperationException( + "Not implemented: noBindingCallable(). REST transport is not implemented for this method yet."); + } + @Override public final void close() { try { diff --git a/gapic-generator-java/src/test/java/com/google/api/generator/gapic/model/MethodTest.java b/gapic-generator-java/src/test/java/com/google/api/generator/gapic/model/MethodTest.java index cecee806f9..00754113ae 100644 --- a/gapic-generator-java/src/test/java/com/google/api/generator/gapic/model/MethodTest.java +++ b/gapic-generator-java/src/test/java/com/google/api/generator/gapic/model/MethodTest.java @@ -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; @@ -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)); + } } diff --git a/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/HttpJsonEchoStub.java b/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/HttpJsonEchoStub.java index 3ddb5d3570..ed6ea0f01f 100644 --- a/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/HttpJsonEchoStub.java +++ b/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/HttpJsonEchoStub.java @@ -32,7 +32,9 @@ 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.ClientStreamingCallable; import com.google.api.gax.rpc.OperationCallable; import com.google.api.gax.rpc.ServerStreamingCallable; import com.google.api.gax.rpc.UnaryCallable; @@ -524,6 +526,18 @@ public UnaryCallable blockCallable() { return blockCallable; } + @Override + public ClientStreamingCallable collectCallable() { + throw new UnsupportedOperationException( + "Not implemented: collectCallable(). REST transport is not implemented for this method yet."); + } + + @Override + public BidiStreamingCallable chatCallable() { + throw new UnsupportedOperationException( + "Not implemented: chatCallable(). REST transport is not implemented for this method yet."); + } + @Override public final void close() { try { diff --git a/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/HttpJsonMessagingStub.java b/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/HttpJsonMessagingStub.java index 41ac603817..4eba21b87f 100644 --- a/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/HttpJsonMessagingStub.java +++ b/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/HttpJsonMessagingStub.java @@ -32,7 +32,9 @@ 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.ClientStreamingCallable; import com.google.api.gax.rpc.OperationCallable; import com.google.api.gax.rpc.ServerStreamingCallable; import com.google.api.gax.rpc.UnaryCallable; @@ -40,6 +42,7 @@ import com.google.protobuf.Empty; import com.google.protobuf.TypeRegistry; import com.google.showcase.v1beta1.Blurb; +import com.google.showcase.v1beta1.ConnectRequest; import com.google.showcase.v1beta1.CreateBlurbRequest; import com.google.showcase.v1beta1.CreateRoomRequest; import com.google.showcase.v1beta1.DeleteBlurbRequest; @@ -54,6 +57,7 @@ import com.google.showcase.v1beta1.SearchBlurbsMetadata; import com.google.showcase.v1beta1.SearchBlurbsRequest; import com.google.showcase.v1beta1.SearchBlurbsResponse; +import com.google.showcase.v1beta1.SendBlurbsResponse; import com.google.showcase.v1beta1.StreamBlurbsRequest; import com.google.showcase.v1beta1.StreamBlurbsResponse; import com.google.showcase.v1beta1.UpdateBlurbRequest; @@ -785,6 +789,18 @@ public ServerStreamingCallable stream return streamBlurbsCallable; } + @Override + public ClientStreamingCallable sendBlurbsCallable() { + throw new UnsupportedOperationException( + "Not implemented: sendBlurbsCallable(). REST transport is not implemented for this method yet."); + } + + @Override + public BidiStreamingCallable connectCallable() { + throw new UnsupportedOperationException( + "Not implemented: connectCallable(). REST transport is not implemented for this method yet."); + } + @Override public final void close() { try {