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.
googleapis · Jun 9, 2022 · 2bed7cf · 2bed7cf
1 parent 7f186b5
commit 2bed7cf
Show file tree

Hide file tree

Showing 9 changed files with 108 additions and 3 deletions.
diff --git 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
@@ -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<TypeNode, TypeNode> BOXED_TYPE_MAP = createBoxedTypeMap();
 

diff --git a/...om/google/api/generator/gapic/composer/common/AbstractServiceClientTestClassComposer.java b/...om/google/api/generator/gapic/composer/common/AbstractServiceClientTestClassComposer.java
@@ -230,6 +230,7 @@ private List<MethodDefinition> createTestMethods(
     List<MethodDefinition> 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<Statement> createStreamingRpcExceptionTestStatements(
       Map<String, ResourceName> resourceNames,
       Map<String, Message> messageTypes);
 
+  protected MethodDefinition createUnsupportedTestMethod(Method method) {
+    String javaMethodName = JavaStyle.toLowerCamelCase(method.name());
+    String exceptionTestMethodName = String.format("%sUnsupportedMethodTest", javaMethodName);
+
+    List<Statement> 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<Statement> createRpcExceptionTestStatements(
       Method method,
       List<MethodArgument> 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
@@ -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/...t/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposerTest.java b/...t/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<String, Message> messageTypes = Parser.parseMessages(echoFileDescriptor);
+    messageTypes.putAll(Parser.parseMessages(StructProto.getDescriptor()));
+    Map<String, ResourceName> 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
@@ -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<String, Message> messageTypes = Parser.parseMessages(echoFileDescriptor);
-    Map<String, Message> operationMessageTypes =
-        Parser.parseMessages(OperationsProto.getDescriptor());
-    messageTypes.putAll(operationMessageTypes);
+    messageTypes.putAll(Parser.parseMessages(OperationsProto.getDescriptor()));
+    messageTypes.putAll(Parser.parseMessages(StructProto.getDescriptor()));
+
     Map<String, ResourceName> resourceNames = Parser.parseResourceNames(echoFileDescriptor);
     Set<ResourceName> outputResourceNames = new HashSet<>();
     List<Service> services =

diff --git a/...va/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoClientHttpJsonTest.golden b/...va/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
@@ -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
@@ -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.