From 2bed7cf4f4168977ccb413125c6bcb43422061fd Mon Sep 17 00:00:00 2001 From: Vadym Matsishevskyi <25311427+vam-google@users.noreply.github.com> Date: Thu, 9 Jun 2022 12:53:19 -0700 Subject: [PATCH] fix: More REST transport fixes (#1003) 1) Generate an empty unit test method even if the method is not supported in a particular transport. Bazel fails test execution, if the test class has no `@Test` methods. In case if there is a service with only unsupported methods, and we do not generate unit tests for those, we would get an empty unit test, which will fail execution in our automatic bazel build pipeline. 2) For fields of `google.protobuf.Value` type set an actual value for it (to avoid non-set vs null value differences after json serialization/deserializaiton roudtrip). This looks like an issue in protobuf support of `google.protobuf.Value` type in JSON and not specific to GAPIC. --- BUILD.bazel | 1 + .../api/generator/engine/ast/TypeNode.java | 3 +++ ...bstractServiceClientTestClassComposer.java | 26 +++++++++++++++++++ .../defaultvalue/DefaultValueComposer.java | 14 ++++++++++ .../DefaultValueComposerTest.java | 23 ++++++++++++++++ .../grpcrest/GrpcRestTestProtoLoader.java | 7 ++--- .../goldens/EchoClientHttpJsonTest.golden | 21 +++++++++++++++ .../grpcrest/goldens/EchoClientTest.golden | 12 +++++++++ src/test/proto/echo_grpcrest.proto | 4 +++ 9 files changed, 108 insertions(+), 3 deletions(-) diff --git a/BUILD.bazel b/BUILD.bazel index 7d7e512dad..e8e17887a3 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -79,6 +79,7 @@ proto_library( "@com_google_protobuf//:empty_proto", "@com_google_protobuf//:field_mask_proto", "@com_google_protobuf//:timestamp_proto", + "@com_google_protobuf//:struct_proto", ], ) java_proto_library( diff --git a/src/main/java/com/google/api/generator/engine/ast/TypeNode.java b/src/main/java/com/google/api/generator/engine/ast/TypeNode.java index 31cd5141c8..1814be11aa 100644 --- a/src/main/java/com/google/api/generator/engine/ast/TypeNode.java +++ b/src/main/java/com/google/api/generator/engine/ast/TypeNode.java @@ -70,6 +70,9 @@ public enum TypeKind { public static final TypeNode BYTESTRING = TypeNode.withReference(ConcreteReference.withClazz(ByteString.class)); + public static final TypeNode VALUE = + withReference( + VaporReference.builder().setName("Value").setPakkage("com.google.protobuf").build()); private static final Map BOXED_TYPE_MAP = createBoxedTypeMap(); diff --git a/src/main/java/com/google/api/generator/gapic/composer/common/AbstractServiceClientTestClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/common/AbstractServiceClientTestClassComposer.java index 11786d29ab..896607982c 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/common/AbstractServiceClientTestClassComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/common/AbstractServiceClientTestClassComposer.java @@ -230,6 +230,7 @@ private List createTestMethods( List javaMethods = new ArrayList<>(); for (Method method : service.methods()) { if (!isSupportedMethod(method)) { + javaMethods.add(createUnsupportedTestMethod(method)); continue; } Service matchingService = service; @@ -760,6 +761,31 @@ protected abstract List createStreamingRpcExceptionTestStatements( Map resourceNames, Map messageTypes); + protected MethodDefinition createUnsupportedTestMethod(Method method) { + String javaMethodName = JavaStyle.toLowerCamelCase(method.name()); + String exceptionTestMethodName = String.format("%sUnsupportedMethodTest", javaMethodName); + + List methodBody = + Collections.singletonList( + CommentStatement.withComment( + LineComment.withComment( + "The " + + javaMethodName + + "() method is not supported in " + + String.join("+", getTransportContext().transportNames()) + + " transport.\n" + + "This empty test is generated for technical reasons."))); + + return MethodDefinition.builder() + .setAnnotations(Arrays.asList(TEST_ANNOTATION)) + .setScope(ScopeNode.PUBLIC) + .setReturnType(TypeNode.VOID) + .setName(exceptionTestMethodName) + .setThrowsExceptions(Arrays.asList(TypeNode.withExceptionClazz(Exception.class))) + .setBody(methodBody) + .build(); + } + protected List createRpcExceptionTestStatements( Method method, List methodSignature, diff --git a/src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java b/src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java index bf1c26dae4..54fc1dad57 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java @@ -168,6 +168,20 @@ public static Expr createValue( .setStaticReferenceType(field.type()) .setMethodName("newBuilder") .build(); + if (field.type().equals(TypeNode.VALUE)) { + newBuilderExpr = + MethodInvocationExpr.builder() + .setExprReferenceExpr(newBuilderExpr) + .setMethodName("setBoolValue") + .setArguments( + ValueExpr.withValue( + PrimitiveValue.builder() + .setType(TypeNode.BOOLEAN) + .setValue("true") + .build())) + .build(); + } + return MethodInvocationExpr.builder() .setExprReferenceExpr(newBuilderExpr) .setMethodName("build") diff --git a/src/test/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposerTest.java index 9f84b34ad0..298d6e27d8 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposerTest.java +++ b/src/test/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposerTest.java @@ -29,6 +29,7 @@ import com.google.api.generator.testutils.LineFormatter; import com.google.protobuf.ByteString; import com.google.protobuf.Descriptors.FileDescriptor; +import com.google.protobuf.StructProto; import com.google.showcase.v1beta1.EchoOuterClass; import com.google.testgapic.v1beta1.LockerProto; import java.util.Arrays; @@ -410,6 +411,28 @@ public void createSimpleMessage_containsMessagesEnumsAndResourceName() { writerVisitor.write()); } + @Test + public void createSimpleMessage_valueField() { + FileDescriptor echoFileDescriptor = + com.google.showcase.grpcrest.v1beta1.EchoGrpcrest.getDescriptor(); + Map messageTypes = Parser.parseMessages(echoFileDescriptor); + messageTypes.putAll(Parser.parseMessages(StructProto.getDescriptor())); + Map typeStringsToResourceNames = + Parser.parseResourceNames(echoFileDescriptor); + Message message = messageTypes.get("com.google.showcase.grpcrest.v1beta1.EchoResponse"); + Expr expr = + DefaultValueComposer.createSimpleMessageBuilderValue( + message, typeStringsToResourceNames, messageTypes, null); + expr.accept(writerVisitor); + assertEquals( + "EchoResponse.newBuilder()" + + ".setContent(\"content951530617\")" + + ".setSeverity(Severity.forNumber(0))" + + ".setValueField(Value.newBuilder().setBoolValue(true).build())" + + ".build()", + writerVisitor.write()); + } + @Test public void createSimpleMessage_containsRepeatedField() { FileDescriptor echoFileDescriptor = EchoOuterClass.getDescriptor(); diff --git a/src/test/java/com/google/api/generator/gapic/composer/grpcrest/GrpcRestTestProtoLoader.java b/src/test/java/com/google/api/generator/gapic/composer/grpcrest/GrpcRestTestProtoLoader.java index 375204be59..05c0241327 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/grpcrest/GrpcRestTestProtoLoader.java +++ b/src/test/java/com/google/api/generator/gapic/composer/grpcrest/GrpcRestTestProtoLoader.java @@ -29,6 +29,7 @@ import com.google.longrunning.OperationsProto; import com.google.protobuf.Descriptors.FileDescriptor; import com.google.protobuf.Descriptors.ServiceDescriptor; +import com.google.protobuf.StructProto; import com.google.showcase.grpcrest.v1beta1.EchoGrpcrest; import java.nio.file.Path; import java.nio.file.Paths; @@ -57,9 +58,9 @@ public GapicContext parseShowcaseEcho() { assertEquals("Echo", echoServiceDescriptor.getName()); Map messageTypes = Parser.parseMessages(echoFileDescriptor); - Map operationMessageTypes = - Parser.parseMessages(OperationsProto.getDescriptor()); - messageTypes.putAll(operationMessageTypes); + messageTypes.putAll(Parser.parseMessages(OperationsProto.getDescriptor())); + messageTypes.putAll(Parser.parseMessages(StructProto.getDescriptor())); + Map resourceNames = Parser.parseResourceNames(echoFileDescriptor); Set outputResourceNames = new HashSet<>(); List services = diff --git a/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoClientHttpJsonTest.golden b/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoClientHttpJsonTest.golden index b3a5f9c63d..e3a030ccd9 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoClientHttpJsonTest.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoClientHttpJsonTest.golden @@ -18,6 +18,7 @@ import com.google.longrunning.Operation; import com.google.protobuf.Any; import com.google.protobuf.Duration; import com.google.protobuf.Timestamp; +import com.google.protobuf.Value; import com.google.rpc.Status; import com.google.showcase.grpcrest.v1beta1.stub.HttpJsonEchoStub; import java.io.IOException; @@ -72,6 +73,7 @@ public class EchoClientHttpJsonTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockService.addResponse(expectedResponse); @@ -121,6 +123,7 @@ public class EchoClientHttpJsonTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockService.addResponse(expectedResponse); @@ -166,6 +169,7 @@ public class EchoClientHttpJsonTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockService.addResponse(expectedResponse); @@ -211,6 +215,7 @@ public class EchoClientHttpJsonTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockService.addResponse(expectedResponse); @@ -256,6 +261,7 @@ public class EchoClientHttpJsonTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockService.addResponse(expectedResponse); @@ -301,6 +307,7 @@ public class EchoClientHttpJsonTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockService.addResponse(expectedResponse); @@ -346,6 +353,7 @@ public class EchoClientHttpJsonTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockService.addResponse(expectedResponse); @@ -391,6 +399,7 @@ public class EchoClientHttpJsonTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockService.addResponse(expectedResponse); @@ -805,4 +814,16 @@ public class EchoClientHttpJsonTest { // Expected exception. } } + + @Test + public void chatUnsupportedMethodTest() throws Exception { + // The chat() method is not supported in REST transport. + // This empty test is generated for technical reasons. + } + + @Test + public void noBindingUnsupportedMethodTest() throws Exception { + // The noBinding() method is not supported in REST transport. + // This empty test is generated for technical reasons. + } } diff --git a/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoClientTest.golden b/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoClientTest.golden index 014add9c3a..aed3486918 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoClientTest.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoClientTest.golden @@ -22,6 +22,7 @@ import com.google.protobuf.AbstractMessage; import com.google.protobuf.Any; import com.google.protobuf.Duration; import com.google.protobuf.Timestamp; +import com.google.protobuf.Value; import com.google.rpc.Status; import io.grpc.StatusRuntimeException; import java.io.IOException; @@ -82,6 +83,7 @@ public class EchoClientTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockEcho.addResponse(expectedResponse); @@ -124,6 +126,7 @@ public class EchoClientTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockEcho.addResponse(expectedResponse); @@ -163,6 +166,7 @@ public class EchoClientTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockEcho.addResponse(expectedResponse); @@ -202,6 +206,7 @@ public class EchoClientTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockEcho.addResponse(expectedResponse); @@ -241,6 +246,7 @@ public class EchoClientTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockEcho.addResponse(expectedResponse); @@ -280,6 +286,7 @@ public class EchoClientTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockEcho.addResponse(expectedResponse); @@ -319,6 +326,7 @@ public class EchoClientTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockEcho.addResponse(expectedResponse); @@ -358,6 +366,7 @@ public class EchoClientTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockEcho.addResponse(expectedResponse); @@ -400,6 +409,7 @@ public class EchoClientTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockEcho.addResponse(expectedResponse); ExpandRequest request = @@ -781,6 +791,7 @@ public class EchoClientTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockEcho.addResponse(expectedResponse); EchoRequest request = @@ -839,6 +850,7 @@ public class EchoClientTest { EchoResponse.newBuilder() .setContent("content951530617") .setSeverity(Severity.forNumber(0)) + .setValueField(Value.newBuilder().setBoolValue(true).build()) .build(); mockEcho.addResponse(expectedResponse); diff --git a/src/test/proto/echo_grpcrest.proto b/src/test/proto/echo_grpcrest.proto index 4b56bca9e3..61efb8fd65 100644 --- a/src/test/proto/echo_grpcrest.proto +++ b/src/test/proto/echo_grpcrest.proto @@ -20,6 +20,7 @@ import "google/api/field_behavior.proto"; import "google/api/resource.proto"; import "google/longrunning/operations.proto"; import "google/protobuf/duration.proto"; +import "google/protobuf/struct.proto"; import "google/protobuf/timestamp.proto"; import "google/rpc/status.proto"; @@ -210,6 +211,9 @@ message EchoResponse { // The severity specified in the request. Severity severity = 2; + + // Value field to test special case in Value type serialization. + google.protobuf.Value value_field = 3; } // Tests name collisions with java.lang.Object.