fix(mixins): Gate mixin RPC on HTTP rules, add yaml doc/http override…

…s [ggj] (#727)

* fix(mixins): Gate mixin RPC on HTTP rules, override with doc/http in yaml

* fix: comment, request params

src/main/java/com/google/api/generator/gapic/protoparser/HttpRuleParser.java

@@ -50,15 +50,24 @@ public static Optional<List<String>> parseHttpBindings(
       checkHttpFieldIsValid(httpRule.getBody(), inputMessage, true);
     }
 
+    return parseHttpRuleHelper(httpRule, Optional.of(inputMessage), messageTypes);
+  }
+
+  public static Optional<List<String>> parseHttpRule(HttpRule httpRule) {
+    return parseHttpRuleHelper(httpRule, Optional.empty(), Collections.emptyMap());
+  }
+
+  private static Optional<List<String>> parseHttpRuleHelper(
+      HttpRule httpRule, Optional<Message> inputMessageOpt, Map<String, Message> messageTypes) {
     // Get pattern.
-    Set<String> uniqueBindings = getPatternBindings(httpRule);
+    Set<String> uniqueBindings = getHttpVerbPattern(httpRule);
     if (uniqueBindings.isEmpty()) {
       return Optional.empty();
     }
 
     if (httpRule.getAdditionalBindingsCount() > 0) {
       for (HttpRule additionalRule : httpRule.getAdditionalBindingsList()) {
-        uniqueBindings.addAll(getPatternBindings(additionalRule));
+        uniqueBindings.addAll(getHttpVerbPattern(additionalRule));
       }
     }
 
@@ -69,27 +78,31 @@ public static Optional<List<String>> parseHttpBindings(
     for (String binding : bindings) {
       // Handle foo.bar cases by descending into the subfields.
       String[] descendantBindings = binding.split("\\.");
-      Message containingMessage = inputMessage;
+      Optional<Message> containingMessageOpt = inputMessageOpt;
       for (int i = 0; i < descendantBindings.length; i++) {
         String subField = descendantBindings[i];
+        if (!containingMessageOpt.isPresent()) {
+          continue;
+        }
+
         if (i < descendantBindings.length - 1) {
-          Field field = containingMessage.fieldMap().get(subField);
-          containingMessage = messageTypes.get(field.type().reference().fullName());
+          Field field = containingMessageOpt.get().fieldMap().get(subField);
+          containingMessageOpt = Optional.of(messageTypes.get(field.type().reference().fullName()));
           Preconditions.checkNotNull(
-              containingMessage,
+              containingMessageOpt.get(),
               String.format(
                   "No containing message found for field %s with type %s",
                   field.name(), field.type().reference().simpleName()));
         } else {
-          checkHttpFieldIsValid(subField, containingMessage, false);
+          checkHttpFieldIsValid(subField, containingMessageOpt.get(), false);
         }
       }
     }
 
     return Optional.of(bindings);
   }
 
-  private static Set<String> getPatternBindings(HttpRule httpRule) {
+  private static Set<String> getHttpVerbPattern(HttpRule httpRule) {
     String pattern = null;
     // Assign a temp variable to prevent the formatter from removing the import.
     PatternCase patternCase = httpRule.getPatternCase();

src/main/java/com/google/api/generator/gapic/protoparser/Parser.java

@@ -15,6 +15,8 @@
 package com.google.api.generator.gapic.protoparser;
 
 import com.google.api.ClientProto;
+import com.google.api.DocumentationRule;
+import com.google.api.HttpRule;
 import com.google.api.ResourceDescriptor;
 import com.google.api.ResourceProto;
 import com.google.api.generator.engine.ast.TypeNode;
@@ -239,6 +241,33 @@ public static List<Service> parseServices(
                 .filter(a -> MIXIN_ALLOWLIST.contains(a.getName()))
                 .map(a -> a.getName())
                 .collect(Collectors.toSet());
+    // Holds the methods to be mixed in.
+    // Key: proto_package.ServiceName.RpcName.
+    // Value: HTTP rules, which clobber those in the proto.
+    // Assumes that http.rules.selector always specifies RPC names in the above format.
+    Map<String, List<String>> mixedInMethodsToHttpRules = new HashMap<>();
+    Map<String, String> mixedInMethodsToDocs = new HashMap<>();
+    // Parse HTTP rules and documentation, which will override the proto.
+    if (serviceYamlProtoOpt.isPresent()) {
+      for (HttpRule httpRule : serviceYamlProtoOpt.get().getHttp().getRulesList()) {
+        Optional<List<String>> httpBindingsOpt = HttpRuleParser.parseHttpRule(httpRule);
+        if (!httpBindingsOpt.isPresent()) {
+          continue;
+        }
+        for (String rpcFullNameRaw : httpRule.getSelector().split(",")) {
+          String rpcFullName = rpcFullNameRaw.trim();
+          mixedInMethodsToHttpRules.put(rpcFullName, httpBindingsOpt.get());
+        }
+      }
+      for (DocumentationRule docRule :
+          serviceYamlProtoOpt.get().getDocumentation().getRulesList()) {
+        for (String rpcFullNameRaw : docRule.getSelector().split(",")) {
+          String rpcFullName = rpcFullNameRaw.trim();
+          mixedInMethodsToDocs.put(rpcFullName, docRule.getDescription());
+        }
+      }
+    }
+
     Set<String> apiDefinedRpcs = new HashSet<>();
     for (Service service : services) {
       if (blockedCodegenMixinApis.contains(service)) {
@@ -252,28 +281,55 @@ public static List<Service> parseServices(
     if (servicesContainBlocklistedApi && !mixedInApis.isEmpty()) {
       for (int i = 0; i < services.size(); i++) {
         Service originalService = services.get(i);
-        List<Method> updatedMethods = new ArrayList<>(originalService.methods());
+        List<Method> updatedOriginalServiceMethods = new ArrayList<>(originalService.methods());
         // If mixin APIs are present, add the methods to all other services.
         for (Service mixinService : blockedCodegenMixinApis) {
-          if (!mixedInApis.contains(
-              String.format("%s.%s", mixinService.protoPakkage(), mixinService.name()))) {
+          final String mixinServiceFullName = serviceFullNameFn.apply(mixinService);
+          if (!mixedInApis.contains(mixinServiceFullName)) {
             continue;
           }
-          mixinService
-              .methods()
+          Function<Method, String> methodToFullProtoNameFn =
+              m -> String.format("%s.%s", mixinServiceFullName, m.name());
+          // Filter mixed-in methods based on those listed in the HTTP rules section of
+          // service.yaml.
+          List<Method> updatedMixinMethods =
+              mixinService.methods().stream()
+                  // Mixin method inclusion is based on the HTTP rules list in service.yaml.
+                  .filter(
+                      m -> mixedInMethodsToHttpRules.containsKey(methodToFullProtoNameFn.apply(m)))
+                  .map(
+                      m -> {
+                        // HTTP rules and RPC documentation in the service.yaml file take
+                        // precedence.
+                        String fullMethodName = methodToFullProtoNameFn.apply(m);
+                        List<String> httpBindings =
+                            mixedInMethodsToHttpRules.containsKey(fullMethodName)
+                                ? mixedInMethodsToHttpRules.get(fullMethodName)
+                                : m.httpBindings();
+                        String docs =
+                            mixedInMethodsToDocs.containsKey(fullMethodName)
+                                ? mixedInMethodsToDocs.get(fullMethodName)
+                                : m.description();
+                        return m.toBuilder()
+                            .setHttpBindings(httpBindings)
+                            .setDescription(docs)
+                            .build();
+                      })
+                  .collect(Collectors.toList());
+          // Overridden RPCs defined in the protos take precedence.
+          updatedMixinMethods.stream()
+              .filter(m -> !apiDefinedRpcs.contains(m.name()))
               .forEach(
-                  m -> {
-                    // Overridden RPCs defined in the protos take precedence.
-                    if (!apiDefinedRpcs.contains(m.name())) {
-                      updatedMethods.add(
+                  m ->
+                      updatedOriginalServiceMethods.add(
                           m.toBuilder()
                               .setMixedInApiName(serviceFullNameFn.apply(mixinService))
-                              .build());
-                    }
-                  });
-          outputMixinServiceSet.add(mixinService);
+                              .build()));
+          outputMixinServiceSet.add(
+              mixinService.toBuilder().setMethods(updatedMixinMethods).build());
         }
-        services.set(i, originalService.toBuilder().setMethods(updatedMethods).build());
+        services.set(
+            i, originalService.toBuilder().setMethods(updatedOriginalServiceMethods).build());
       }
     }
 

test/integration/apis/kms/v1/cloudkms_test_mixins_v1.yaml

@@ -21,6 +21,7 @@ documentation:
       Gets the access control policy for a resource. Returns an empty policy
       if the resource exists and does not have a policy set.
 
+  # This RPC shouldn't appear in the proto even though the documentation field is set.
   - selector: google.iam.v1.IAMPolicy.SetIamPolicy
     description: |-
       Sets the access control policy on the specified resource. Replaces
@@ -29,31 +30,37 @@ documentation:
       Can return `NOT_FOUND`, `INVALID_ARGUMENT`, and `PERMISSION_DENIED`
       errors.
 
+  # Test the overriding of comments.
   - selector: google.iam.v1.IAMPolicy.TestIamPermissions
     description: |-
-      Returns permissions that a caller has on the specified resource. If the
-      resource does not exist, this will return an empty set of
-      permissions, not a `NOT_FOUND` error.
-
-      Note: This operation is designed to be used for building
-      permission-aware UIs and command-line tools, not for authorization
-      checking. This operation may "fail open" without warning.
+      This is a different comment for TestIamPermissions in the yaml file that should clobber the documentation in iam_policy.proto.
 
 http:
   rules:
+  - selector: google.cloud.location.Locations.GetLocation
+    get: "/v1/{name=locations/*}"
+    body: '*'
+  # Test different HTTP verbs.
+  - selector: google.cloud.location.Locations.ListLocations
+    get: "/v1/{filter=*/*}/locations"
+    body: '*'
+    additional_bindings:
+      - post: '/v1/{page_size=*}'
+        body: '*'
   - selector: google.iam.v1.IAMPolicy.GetIamPolicy
     get: '/v1/{resource=projects/*/locations/*/keyRings/*}:getIamPolicy'
     additional_bindings:
     - get: '/v1/{resource=projects/*/locations/*/keyRings/*/cryptoKeys/*}:getIamPolicy'
     - get: '/v1/{resource=projects/*/locations/*/keyRings/*/importJobs/*}:getIamPolicy'
-  - selector: google.iam.v1.IAMPolicy.SetIamPolicy
-    post: '/v1/{resource=projects/*/locations/*/keyRings/*}:setIamPolicy'
-    body: '*'
-    additional_bindings:
-    - post: '/v1/{resource=projects/*/locations/*/keyRings/*/cryptoKeys/*}:setIamPolicy'
-      body: '*'
-    - post: '/v1/{resource=projects/*/locations/*/keyRings/*/importJobs/*}:setIamPolicy'
-      body: '*'
+  # Test the omission of SetIamPolicy - this should no longer appear in the generated client.
+#  - selector: google.iam.v1.IAMPolicy.SetIamPolicy
+#    post: '/v1/{resource=projects/*/locations/*/keyRings/*}:setIamPolicy'
+#    body: '*'
+#    additional_bindings:
+#    - post: '/v1/{resource=projects/*/locations/*/keyRings/*/cryptoKeys/*}:setIamPolicy'
+#      body: '*'
+#    - post: '/v1/{resource=projects/*/locations/*/keyRings/*/importJobs/*}:setIamPolicy'
+#      body: '*'
   - selector: google.iam.v1.IAMPolicy.TestIamPermissions
     post: '/v1/{resource=projects/*/locations/*/keyRings/*}:testIamPermissions'
     body: '*'

test/integration/goldens/kms/com/google/cloud/kms/v1/KeyManagementServiceClient.java

@@ -36,7 +36,6 @@
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.iam.v1.GetIamPolicyRequest;
 import com.google.iam.v1.Policy;
-import com.google.iam.v1.SetIamPolicyRequest;
 import com.google.iam.v1.TestIamPermissionsRequest;
 import com.google.iam.v1.TestIamPermissionsResponse;
 import com.google.protobuf.ByteString;
@@ -3334,62 +3333,8 @@ public final UnaryCallable<GetLocationRequest, Location> getLocationCallable() {
 
   // AUTO-GENERATED DOCUMENTATION AND METHOD.
   /**
-   * Sets the access control policy on the specified resource. Replaces any existing policy.
-   *
-   * <p>Sample code:
-   *
-   * <pre>{@code
-   * try (KeyManagementServiceClient keyManagementServiceClient =
-   *     KeyManagementServiceClient.create()) {
-   *   SetIamPolicyRequest request =
-   *       SetIamPolicyRequest.newBuilder()
-   *           .setResource(KeyRingName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]").toString())
-   *           .setPolicy(Policy.newBuilder().build())
-   *           .build();
-   *   Policy response = keyManagementServiceClient.setIamPolicy(request);
-   * }
-   * }</pre>
-   *
-   * @param request The request object containing all of the parameters for the API call.
-   * @throws com.google.api.gax.rpc.ApiException if the remote call fails
-   */
-  public final Policy setIamPolicy(SetIamPolicyRequest request) {
-    return setIamPolicyCallable().call(request);
-  }
-
-  // AUTO-GENERATED DOCUMENTATION AND METHOD.
-  /**
-   * Sets the access control policy on the specified resource. Replaces any existing policy.
-   *
-   * <p>Sample code:
-   *
-   * <pre>{@code
-   * try (KeyManagementServiceClient keyManagementServiceClient =
-   *     KeyManagementServiceClient.create()) {
-   *   SetIamPolicyRequest request =
-   *       SetIamPolicyRequest.newBuilder()
-   *           .setResource(KeyRingName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]").toString())
-   *           .setPolicy(Policy.newBuilder().build())
-   *           .build();
-   *   ApiFuture<Policy> future =
-   *       keyManagementServiceClient.setIamPolicyCallable().futureCall(request);
-   *   // Do something.
-   *   Policy response = future.get();
-   * }
-   * }</pre>
-   */
-  public final UnaryCallable<SetIamPolicyRequest, Policy> setIamPolicyCallable() {
-    return stub.setIamPolicyCallable();
-  }
-
-  // AUTO-GENERATED DOCUMENTATION AND METHOD.
-  /**
-   * Returns permissions that a caller has on the specified resource. If the resource does not
-   * exist, this will return an empty set of permissions, not a NOT_FOUND error.
-   *
-   * <p>Note: This operation is designed to be used for building permission-aware UIs and
-   * command-line tools, not for authorization checking. This operation may "fail open" without
-   * warning.
+   * This is a different comment for TestIamPermissions in the yaml file that should clobber the
+   * documentation in iam_policy.proto.
    *
    * <p>Sample code:
    *
@@ -3414,12 +3359,8 @@ public final TestIamPermissionsResponse testIamPermissions(TestIamPermissionsReq
 
   // AUTO-GENERATED DOCUMENTATION AND METHOD.
   /**
-   * Returns permissions that a caller has on the specified resource. If the resource does not
-   * exist, this will return an empty set of permissions, not a NOT_FOUND error.
-   *
-   * <p>Note: This operation is designed to be used for building permission-aware UIs and
-   * command-line tools, not for authorization checking. This operation may "fail open" without
-   * warning.
+   * This is a different comment for TestIamPermissions in the yaml file that should clobber the
+   * documentation in iam_policy.proto.
    *
    * <p>Sample code:
    *