fix: Multiple REST transport related fixes (#997)

1) Stop failing for grpc+rest and rest transports in case if there are bidi- or client- streaming methods (generated a method throwing "Unimplemented" exception if called)
2) Make sure that the resource name suggested for wildcard resource names actually matches url pattern (most of the changes in goldens are caused by this change)
3) Fix the order of "actual" and "expected" in diff_gen_and_golden to match
4) Fix multipattern `of%Name` method resoluiton to make sure that the chosen pattern matches url pattern (the changes in `MessagingClient.golden` and related files are caused by this change).
5) Fix anonymous resource name class construction (add proper toString() method matching pattern)
6) Gracefully treat methods without http bindings as not supported in rest transport (instead of failing on generation)

Author: vam-google

scripts/diff_gen_and_golden.sh

@@ -23,5 +23,5 @@ find src -type f -name 'PlaceholderFile.java' -delete
 find src -type d -empty -delete
 # This will not print diff_output to the console unless `--test_output=all` option
 # is enabled, it only emits the comparison results to the test.log.
-diff -ru src test/integration/goldens/${API_NAME}/src
-diff -ru samples test/integration/goldens/${API_NAME}/samples
+diff -ru test/integration/goldens/${API_NAME}/src src
+diff -ru test/integration/goldens/${API_NAME}/samples samples

src/main/java/com/google/api/generator/gapic/composer/common/AbstractServiceClientTestClassComposer.java

@@ -130,6 +130,10 @@ protected GapicClass generate(String className, GapicContext context, Service se
     return GapicClass.create(kind, classDef);
   }
 
+  protected boolean isSupportedMethod(Method method) {
+    return true;
+  }
+
   private List<AnnotationNode> createClassAnnotations() {
     return Arrays.asList(
         AnnotationNode.builder()
@@ -225,6 +229,9 @@ private List<MethodDefinition> createTestMethods(
     Map<String, Message> messageTypes = context.messages();
     List<MethodDefinition> javaMethods = new ArrayList<>();
     for (Method method : service.methods()) {
+      if (!isSupportedMethod(method)) {
+        continue;
+      }
       Service matchingService = service;
       if (method.isMixin()) {
         int dotIndex = method.mixedInApiName().lastIndexOf(".");
@@ -388,7 +395,8 @@ private MethodDefinition createRpcTestMethod(
             DefaultValueComposer.createSimpleMessageBuilderValue(
                 messageTypes.get(methodOutputType.reference().fullName()),
                 resourceNames,
-                messageTypes);
+                messageTypes,
+                method.httpBindings());
       } else {
         // Wrap this in a field so we don't have to split the helper into lots of different methods,
         // or duplicate it for VariableExpr.
@@ -461,9 +469,14 @@ private MethodDefinition createRpcTestMethod(
       if (getTransportContext().useValuePatterns() && method.hasHttpBindings()) {
         pathParamValuePatterns = method.httpBindings().getPathParametersValuePatterns();
       }
+
       Expr valExpr =
           DefaultValueComposer.createSimpleMessageBuilderValue(
-              requestMessage, resourceNames, messageTypes, pathParamValuePatterns);
+              requestMessage,
+              resourceNames,
+              messageTypes,
+              pathParamValuePatterns,
+              method.httpBindings());
       methodExprs.add(
           AssignmentExpr.builder()
               .setVariableExpr(requestVarExpr.toBuilder().setIsDecl(true).build())
@@ -482,7 +495,7 @@ private MethodDefinition createRpcTestMethod(
         argExprs.add(varExpr);
         Expr valExpr =
             DefaultValueComposer.createMethodArgValue(
-                methodArg, resourceNames, messageTypes, valuePatterns);
+                methodArg, resourceNames, messageTypes, valuePatterns, method.httpBindings());
         methodExprs.add(
             AssignmentExpr.builder()
                 .setVariableExpr(varExpr.toBuilder().setIsDecl(true).build())
@@ -770,7 +783,7 @@ protected List<Statement> createRpcExceptionTestStatements(
       }
       Expr valExpr =
           DefaultValueComposer.createSimpleMessageBuilderValue(
-              requestMessage, resourceNames, messageTypes, valuePatterns);
+              requestMessage, resourceNames, messageTypes, valuePatterns, method.httpBindings());
       tryBodyExprs.add(
           AssignmentExpr.builder()
               .setVariableExpr(varExpr.toBuilder().setIsDecl(true).build())
@@ -789,7 +802,7 @@ protected List<Statement> createRpcExceptionTestStatements(
         argVarExprs.add(varExpr);
         Expr valExpr =
             DefaultValueComposer.createMethodArgValue(
-                methodArg, resourceNames, messageTypes, valuePatterns);
+                methodArg, resourceNames, messageTypes, valuePatterns, method.httpBindings());
         tryBodyExprs.add(
             AssignmentExpr.builder()
                 .setVariableExpr(varExpr.toBuilder().setIsDecl(true).build())

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

@@ -217,6 +217,10 @@ public GapicClass generate(GapicContext context, Service service) {
     return GapicClass.create(kind, classDef);
   }
 
+  protected boolean isSupportedMethod(Method method) {
+    return true;
+  }
+
   protected abstract Statement createMethodDescriptorVariableDecl(
       Service service,
       Method protoMethod,
@@ -299,6 +303,7 @@ protected List<Statement> createMethodDescriptorVariableDecls(
       Map<String, VariableExpr> protoMethodNameToDescriptorVarExprs,
       Map<String, Message> messageTypes) {
     return service.methods().stream()
+        .filter(this::isSupportedMethod)
         .map(
             m ->
                 createMethodDescriptorVariableDecl(
@@ -323,6 +328,7 @@ private static List<Statement> createClassMemberFieldDeclarations(
   protected Map<String, VariableExpr> createProtoMethodNameToDescriptorClassMembers(
       Service service, Class<?> descriptorClass) {
     return service.methods().stream()
+        .filter(this::isSupportedMethod)
         .collect(
             Collectors.toMap(
                 Method::name,
@@ -354,6 +360,9 @@ 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)) {
+        continue;
+      }
       String javaStyleProtoMethodName = JavaStyle.toLowerCamelCase(protoMethod.name());
       String callableName = String.format(CALLABLE_CLASS_MEMBER_PATTERN, javaStyleProtoMethodName);
       callableClassMembers.put(
@@ -643,6 +652,7 @@ protected List<MethodDefinition> createConstructorMethods(
     // Transport settings local variables.
     Map<String, VariableExpr> javaStyleMethodNameToTransportSettingsVarExprs =
         service.methods().stream()
+            .filter(this::isSupportedMethod)
             .collect(
                 Collectors.toMap(
                     m -> JavaStyle.toLowerCamelCase(m.name()),
@@ -666,6 +676,7 @@ protected List<MethodDefinition> createConstructorMethods(
 
     secondCtorExprs.addAll(
         service.methods().stream()
+            .filter(this::isSupportedMethod)
             .map(
                 m ->
                     createTransportSettingsInitExpr(
@@ -1035,7 +1046,9 @@ private List<MethodDefinition> createStubOverrideMethods(
   }
 
   private boolean checkOperationPollingMethod(Service service) {
-    return service.methods().stream().anyMatch(Method::isOperationPollingMethod);
+    return service.methods().stream()
+        .filter(this::isSupportedMethod)
+        .anyMatch(Method::isOperationPollingMethod);
   }
 
   protected List<MethodDefinition> createLongRunningClientGetters() {
@@ -1073,6 +1086,7 @@ private TypeStore createDynamicTypes(Service service, String stubPakkage) {
     typeStore.putAll(
         service.pakkage(),
         service.methods().stream()
+            .filter(this::isSupportedMethod)
             .filter(Method::isPaged)
             .map(m -> String.format(PAGED_RESPONSE_TYPE_NAME_PATTERN, m.name()))
             .collect(Collectors.toList()),

src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java

@@ -32,6 +32,7 @@
 import com.google.api.generator.engine.ast.VariableExpr;
 import com.google.api.generator.gapic.composer.resourcename.ResourceNameTokenizer;
 import com.google.api.generator.gapic.model.Field;
+import com.google.api.generator.gapic.model.HttpBindings;
 import com.google.api.generator.gapic.model.Message;
 import com.google.api.generator.gapic.model.MethodArgument;
 import com.google.api.generator.gapic.model.ResourceName;
@@ -65,7 +66,8 @@ public static Expr createMethodArgValue(
       MethodArgument methodArg,
       Map<String, ResourceName> resourceNames,
       Map<String, Message> messageTypes,
-      Map<String, String> valuePatterns) {
+      Map<String, String> valuePatterns,
+      HttpBindings bindings) {
     if (methodArg.isResourceNameHelper()) {
       Preconditions.checkState(
           methodArg.field().hasResourceReference(),
@@ -84,7 +86,8 @@ public static Expr createMethodArgValue(
               resourceName,
               methodArg.field().resourceReference().isChildType(),
               resourceNames.values().stream().collect(Collectors.toList()),
-              methodArg.field().name());
+              methodArg.field().name(),
+              bindings);
 
       if (!methodArg.isResourceNameHelper() && methodArg.field().hasResourceReference()) {
         defValue =
@@ -156,7 +159,7 @@ public static Expr createValue(
         Message nestedMessage = messageTypes.get(field.type().reference().fullName());
         if (nestedMessage != null) {
           return createSimpleMessageBuilderValue(
-              nestedMessage, resourceNames, messageTypes, nestedValuePatterns);
+              nestedMessage, resourceNames, messageTypes, nestedValuePatterns, null);
         }
       }
 
@@ -209,8 +212,10 @@ public static Expr createResourceHelperValue(
       ResourceName resourceName,
       boolean isChildType,
       List<ResourceName> resnames,
-      String fieldOrMessageName) {
-    return createResourceHelperValue(resourceName, isChildType, resnames, fieldOrMessageName, true);
+      String fieldOrMessageName,
+      HttpBindings bindings) {
+    return createResourceHelperValue(
+        resourceName, isChildType, resnames, fieldOrMessageName, true, bindings);
   }
 
   private static Optional<ResourceName> findParentResource(
@@ -239,7 +244,8 @@ static Expr createResourceHelperValue(
       boolean isChildType,
       List<ResourceName> resnames,
       String fieldOrMessageName,
-      boolean allowAnonResourceNameClass) {
+      boolean allowAnonResourceNameClass,
+      HttpBindings bindings) {
 
     if (isChildType) {
       resourceName = findParentResource(resourceName, resnames).orElse(resourceName);
@@ -249,13 +255,15 @@ static Expr createResourceHelperValue(
       List<ResourceName> unexaminedResnames = new ArrayList<>(resnames);
       for (ResourceName resname : resnames) {
         unexaminedResnames.remove(resname);
-        if (!resname.isOnlyWildcard()) {
-          return createResourceHelperValue(resname, false, unexaminedResnames, fieldOrMessageName);
+        if (!resname.isOnlyWildcard()
+            && (bindings == null || resname.getMatchingPattern(bindings) != null)) {
+          return createResourceHelperValue(
+              resname, false, unexaminedResnames, fieldOrMessageName, null);
         }
       }
 
       return allowAnonResourceNameClass
-          ? createAnonymousResourceNameClassValue(fieldOrMessageName)
+          ? createAnonymousResourceNameClassValue(fieldOrMessageName, bindings)
           : ValueExpr.withValue(
               StringObjectValue.withValue(
                   String.format("%s%s", fieldOrMessageName, fieldOrMessageName.hashCode())));
@@ -272,15 +280,22 @@ static Expr createResourceHelperValue(
     if (containsOnlyDeletedTopic) {
       ofMethodName = "ofDeletedTopic";
     } else {
-      for (String pattern : resourceName.patterns()) {
-        if (pattern.equals(ResourceNameConstants.WILDCARD_PATTERN)
-            || pattern.equals(ResourceNameConstants.DELETED_TOPIC_LITERAL)) {
-          continue;
+      String matchingPattern = null;
+      if (bindings != null) {
+        matchingPattern = resourceName.getMatchingPattern(bindings);
+      }
+      if (matchingPattern == null) {
+        for (String pattern : resourceName.patterns()) {
+          if (pattern.equals(ResourceNameConstants.WILDCARD_PATTERN)
+              || pattern.equals(ResourceNameConstants.DELETED_TOPIC_LITERAL)) {
+            continue;
+          }
+          matchingPattern = pattern;
+          break;
         }
-        patternPlaceholderTokens.addAll(
-            ResourceNameTokenizer.parseTokenHierarchy(Arrays.asList(pattern)).get(0));
-        break;
       }
+      patternPlaceholderTokens.addAll(
+          ResourceNameTokenizer.parseTokenHierarchy(Arrays.asList(matchingPattern)).get(0));
     }
 
     boolean hasOnePattern = resourceName.patterns().size() == 1;
@@ -312,16 +327,20 @@ static Expr createResourceHelperValue(
   }
 
   public static Expr createSimpleMessageBuilderValue(
-      Message message, Map<String, ResourceName> resourceNames, Map<String, Message> messageTypes) {
+      Message message,
+      Map<String, ResourceName> resourceNames,
+      Map<String, Message> messageTypes,
+      HttpBindings bindings) {
     return createSimpleMessageBuilderValue(
-        message, resourceNames, messageTypes, Collections.emptyMap());
+        message, resourceNames, messageTypes, Collections.emptyMap(), bindings);
   }
 
   public static Expr createSimpleMessageBuilderValue(
       Message message,
       Map<String, ResourceName> resourceNames,
       Map<String, Message> messageTypes,
-      Map<String, String> valuePatterns) {
+      Map<String, String> valuePatterns,
+      HttpBindings bindings) {
     MethodInvocationExpr builderExpr =
         MethodInvocationExpr.builder()
             .setStaticReferenceType(message.type())
@@ -350,7 +369,8 @@ public static Expr createSimpleMessageBuilderValue(
                 field.resourceReference().isChildType(),
                 resourceNames.values().stream().collect(Collectors.toList()),
                 message.name(),
-                /* allowAnonResourceNameClass = */ false);
+                /* allowAnonResourceNameClass = */ false,
+                bindings);
         defaultExpr =
             MethodInvocationExpr.builder()
                 .setExprReferenceExpr(defaultExpr)
@@ -507,7 +527,8 @@ public static Expr createSimplePagedResponseValue(
   }
 
   @VisibleForTesting
-  static AnonymousClassExpr createAnonymousResourceNameClassValue(String fieldOrMessageName) {
+  static AnonymousClassExpr createAnonymousResourceNameClassValue(
+      String fieldOrMessageName, HttpBindings matchedBindings) {
     TypeNode stringMapType =
         TypeNode.withReference(
             ConcreteReference.builder()
@@ -525,12 +546,18 @@ static AnonymousClassExpr createAnonymousResourceNameClassValue(String fieldOrMe
     //   fieldValuesMap.put("resource", "resource-12345");
     //   return fieldValuesMap;
     // }
+
+    String toStringValue = String.format("%s%s", fieldOrMessageName, fieldOrMessageName.hashCode());
+    if (matchedBindings != null) {
+      Map<String, String> valuePatterns = matchedBindings.getPathParametersValuePatterns();
+      toStringValue =
+          constructValueMatchingPattern(fieldOrMessageName, valuePatterns.get(fieldOrMessageName));
+    }
+
     VariableExpr fieldValuesMapVarExpr =
         VariableExpr.withVariable(
             Variable.builder().setType(stringMapType).setName("fieldValuesMap").build());
-    StringObjectValue fieldOrMessageStringValue =
-        StringObjectValue.withValue(
-            String.format("%s%s", fieldOrMessageName, fieldOrMessageName.hashCode()));
+    StringObjectValue fieldOrMessageStringValue = StringObjectValue.withValue(toStringValue);
 
     List<Expr> bodyExprs =
         Arrays.asList(
@@ -586,15 +613,24 @@ static AnonymousClassExpr createAnonymousResourceNameClassValue(String fieldOrMe
                     .build())
             .build();
 
+    MethodDefinition toStringMethod =
+        MethodDefinition.builder()
+            .setIsOverride(true)
+            .setScope(ScopeNode.PUBLIC)
+            .setReturnType(TypeNode.STRING)
+            .setName("toString")
+            .setReturnExpr(ValueExpr.withValue(StringObjectValue.withValue(toStringValue)))
+            .build();
+
     return AnonymousClassExpr.builder()
         .setType(
             TypeNode.withReference(
                 ConcreteReference.withClazz(com.google.api.resourcenames.ResourceName.class)))
-        .setMethods(Arrays.asList(getFieldValuesMapMethod, getFieldValueMethod))
+        .setMethods(Arrays.asList(getFieldValuesMapMethod, getFieldValueMethod, toStringMethod))
         .build();
   }
 
-  private static String constructValueMatchingPattern(String fieldName, String pattern) {
+  public static String constructValueMatchingPattern(String fieldName, String pattern) {
     if (pattern == null || pattern.isEmpty()) {
       return fieldName + fieldName.hashCode();
     }

src/main/java/com/google/api/generator/gapic/composer/grpc/ServiceClientTestClassComposer.java

@@ -551,7 +551,8 @@ protected MethodDefinition createStreamingRpcTestMethod(
           DefaultValueComposer.createSimpleMessageBuilderValue(
               messageTypes.get(methodOutputType.reference().fullName()),
               resourceNames,
-              messageTypes);
+              messageTypes,
+              method.httpBindings());
     } else {
       // Wrap this in a field so we don't have to split the helper into lots of different methods,
       // or duplicate it for VariableExpr.
@@ -608,7 +609,7 @@ protected MethodDefinition createStreamingRpcTestMethod(
     Preconditions.checkNotNull(requestMessage);
     Expr valExpr =
         DefaultValueComposer.createSimpleMessageBuilderValue(
-            requestMessage, resourceNames, messageTypes);
+            requestMessage, resourceNames, messageTypes, method.httpBindings());
     methodExprs.add(
         AssignmentExpr.builder()
             .setVariableExpr(requestVarExpr.toBuilder().setIsDecl(true).build())
@@ -881,7 +882,7 @@ protected List<Statement> createStreamingRpcExceptionTestStatements(
     Preconditions.checkNotNull(requestMessage);
     Expr valExpr =
         DefaultValueComposer.createSimpleMessageBuilderValue(
-            requestMessage, resourceNames, messageTypes);
+            requestMessage, resourceNames, messageTypes, method.httpBindings());
 
     List<Statement> statements = new ArrayList<>();
     statements.add(