cleanup(mixin): refactor mixin utils to use HttpRule (#14723)

* cleanup(mixin): refactor mixin utils to use HttpRule * complete refactor * format * add comment * revise * revise * nit
googleapis · Sep 23, 2024 · 6d70fe8 · 6d70fe8
1 parent 4c8c2a5
commit 6d70fe8
Show file tree

Hide file tree

Showing 8 changed files with 225 additions and 216 deletions.
diff --git a/generator/BUILD.bazel b/generator/BUILD.bazel
@@ -77,6 +77,7 @@ cc_library(
     deps = [
         ":google_cloud_cpp_generator",
         ":google_cloud_cpp_generator_testing",
+        "//google/cloud/testing_util:google_cloud_cpp_testing_grpc_private",
         "@com_google_googletest//:gtest",
     ],
 ) for test in google_cloud_cpp_generator_unit_tests]

diff --git a/generator/internal/descriptor_utils.cc b/generator/internal/descriptor_utils.cc
@@ -35,6 +35,7 @@
 #include "absl/strings/str_split.h"
 #include "absl/strings/strip.h"
 #include "absl/types/variant.h"
+#include <google/api/annotations.pb.h>
 #include <google/api/routing.pb.h>
 #include <google/longrunning/operations.pb.h>
 #include <google/protobuf/compiler/code_generator.h>
@@ -842,7 +843,11 @@ std::map<std::string, VarsDictionary> CreateMethodVars(
     SetLongrunningOperationMethodVars(method, method_vars);
     AssignPaginationMethodVars(method, method_vars);
     SetMethodSignatureMethodVars(service, method, omitted_rpcs, method_vars);
-    auto parsed_http_info = ParseHttpExtension(method);
+    HttpExtensionInfo parsed_http_info;
+    if (method.options().HasExtension(google::api::http)) {
+      parsed_http_info =
+          ParseHttpExtension(method.options().GetExtension(google::api::http));
+    }
     method_vars["request_resource"] =
         FormatRequestResource(*method.input_type(), parsed_http_info);
     SetHttpDerivedMethodVars(parsed_http_info, method, method_vars);

diff --git a/generator/internal/http_option_utils.cc b/generator/internal/http_option_utils.cc
@@ -263,57 +263,42 @@ void SetHttpQueryParameters(HttpExtensionInfo const& info,
       FormatQueryParameterCode(method, std::move(param_field_names));
 }
 
-HttpExtensionInfo ParseHttpExtension(
-    google::protobuf::MethodDescriptor const& method,
-    absl::optional<MixinMethodOverride> method_override) {
-  if (!method.options().HasExtension(google::api::http)) return {};
-
+HttpExtensionInfo ParseHttpExtension(google::api::HttpRule const& http_rule) {
   HttpExtensionInfo info;
-  google::api::HttpRule http_rule =
-      method.options().GetExtension(google::api::http);
-
-  std::string url_pattern;
-  if (method_override) {
-    url_pattern = method_override->http_path;
-    info.http_verb = method_override->http_verb;
-    info.body = method_override->http_body ? *method_override->http_body : "";
-  } else {
-    switch (http_rule.pattern_case()) {
-      case google::api::HttpRule::kGet:
-        info.http_verb = "Get";
-        url_pattern = http_rule.get();
-        break;
-      case google::api::HttpRule::kPut:
-        info.http_verb = "Put";
-        url_pattern = http_rule.put();
-        break;
-      case google::api::HttpRule::kPost:
-        info.http_verb = "Post";
-        url_pattern = http_rule.post();
-        break;
-      case google::api::HttpRule::kDelete:
-        info.http_verb = "Delete";
-        url_pattern = http_rule.delete_();
-        break;
-      case google::api::HttpRule::kPatch:
-        info.http_verb = "Patch";
-        url_pattern = http_rule.patch();
-        break;
-      default:
-        GCP_LOG(FATAL) << __FILE__ << ":" << __LINE__
-                       << ": google::api::HttpRule not handled";
-    }
-    info.body = http_rule.body();
+  switch (http_rule.pattern_case()) {
+    case google::api::HttpRule::kGet:
+      info.http_verb = "Get";
+      info.url_path = http_rule.get();
+      break;
+    case google::api::HttpRule::kPut:
+      info.http_verb = "Put";
+      info.url_path = http_rule.put();
+      break;
+    case google::api::HttpRule::kPost:
+      info.http_verb = "Post";
+      info.url_path = http_rule.post();
+      break;
+    case google::api::HttpRule::kDelete:
+      info.http_verb = "Delete";
+      info.url_path = http_rule.delete_();
+      break;
+    case google::api::HttpRule::kPatch:
+      info.http_verb = "Patch";
+      info.url_path = http_rule.patch();
+      break;
+    default:
+      GCP_LOG(FATAL) << __FILE__ << ":" << __LINE__
+                     << ": google::api::HttpRule not handled";
   }
 
-  auto parsed_http_rule = ParsePathTemplate(url_pattern);
+  auto parsed_http_rule = ParsePathTemplate(info.url_path);
   if (!parsed_http_rule) {
     GCP_LOG(FATAL) << __FILE__ << ":" << __LINE__
                    << " failure in ParsePathTemplate: "
                    << parsed_http_rule.status();
   }
 
-  info.url_path = url_pattern;
+  info.body = http_rule.body();
 
   struct SegmentAsStringVisitor {
     std::string operator()(PathTemplate::Match const&) { return "*"; }
@@ -330,7 +315,7 @@ HttpExtensionInfo ParseHttpExtension(
     out->append(absl::visit(SegmentAsStringVisitor{}, s->value));
   };
 
-  auto api_version = FormatApiVersionFromUrlPattern(url_pattern);
+  auto api_version = FormatApiVersionFromUrlPattern(info.url_path);
   auto rest_path_visitor =
       RestPathVisitor(std::move(api_version), info.rest_path);
   for (auto const& s : parsed_http_rule->segments) {
@@ -352,7 +337,9 @@ HttpExtensionInfo ParseHttpExtension(
 }
 
 bool HasHttpRoutingHeader(MethodDescriptor const& method) {
-  auto result = ParseHttpExtension(method);
+  if (!method.options().HasExtension(google::api::http)) return false;
+  auto result =
+      ParseHttpExtension(method.options().GetExtension(google::api::http));
   return !result.field_substitutions.empty();
 }
 

diff --git a/generator/internal/http_option_utils.h b/generator/internal/http_option_utils.h
@@ -19,6 +19,7 @@
 #include "generator/internal/mixin_utils.h"
 #include "generator/internal/printer.h"
 #include "absl/types/optional.h"
+#include <google/api/http.pb.h>
 #include <google/protobuf/descriptor.h>
 #include <string>
 
@@ -41,14 +42,8 @@ struct HttpExtensionInfo {
  * Parses the http extension providing resource routing info, if present,
  * for the provided method per AIP-4222. Output is also used for gRPC/HTTP
  * transcoding and REST transport.
- * Mixin method should use the target service's package and file names instead
- * of its own.
- * Mixin method' url, verb and body are overridden by the definition in target
- * service's YAML.
  */
-HttpExtensionInfo ParseHttpExtension(
-    google::protobuf::MethodDescriptor const& method,
-    absl::optional<MixinMethodOverride> method_override = absl::nullopt);
+HttpExtensionInfo ParseHttpExtension(google::api::HttpRule const& http_rule);
 
 /**
  * Sets the following method_vars based on the provided parsed_http_info: