cleanup(generator): api version error handling (#14512)

googleapis · Jul 20, 2024 · a86e887 · a86e887
1 parent 4c40596
commit a86e887
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 29 deletions.
diff --git a/generator/internal/http_option_utils.cc b/generator/internal/http_option_utils.cc
@@ -342,7 +342,6 @@ absl::variant<absl::monostate, HttpSimpleInfo, HttpExtensionInfo>
 ParseHttpExtension(google::protobuf::MethodDescriptor const& method) {
   if (!method.options().HasExtension(google::api::http)) return {};
   auto api_version = FormatApiVersionFromPackageName(method);
-  if (!api_version) return {};
 
   HttpExtensionInfo info;
   google::api::HttpRule http_rule =
@@ -388,7 +387,7 @@ ParseHttpExtension(google::protobuf::MethodDescriptor const& method) {
             return absl::holds_alternative<PathTemplate::Variable>(s->value);
           }) == parsed_http_rule->segments.end()) {
     return HttpSimpleInfo{info.http_verb, url_pattern, http_rule.body(),
-                          *api_version};
+                          api_version};
   }
 
   info.body = http_rule.body();
@@ -409,7 +408,7 @@ ParseHttpExtension(google::protobuf::MethodDescriptor const& method) {
     out->append(absl::visit(SegmentAsStringVisitor{}, s->value));
   };
 
-  auto rest_path_visitor = RestPathVisitor(*api_version, info.rest_path);
+  auto rest_path_visitor = RestPathVisitor(api_version, info.rest_path);
   for (auto const& s : parsed_http_rule->segments) {
     if (absl::holds_alternative<PathTemplate::Variable>(s->value)) {
       auto v = absl::get<PathTemplate::Variable>(s->value);
@@ -482,15 +481,15 @@ std::string FormatRequestResource(
 // For protos we generate from Discovery Documents the api version is always the
 // last part of the package name. Protos found in googleapis have package names
 // that mirror their directory path, which ends in the api version as well.
-StatusOr<std::string> FormatApiVersionFromPackageName(
+std::string FormatApiVersionFromPackageName(
     google::protobuf::MethodDescriptor const& method) {
   std::vector<std::string> parts =
       absl::StrSplit(method.file()->package(), '.');
   if (absl::StartsWith(parts.back(), "v")) return parts.back();
-  return internal::InvalidArgumentError(
-      absl::StrCat("Unrecognized API version in package name: ",
-                   method.file()->package()),
-      GCP_ERROR_INFO().WithMetadata("method", method.full_name()));
+  GCP_LOG(FATAL) << "Unrecognized API version in file: "
+                 << method.file()->name()
+                 << ", package: " << method.file()->package();
+  return {};  // Suppress clang-tidy warnings
 }
 
 }  // namespace generator_internal

diff --git a/generator/internal/http_option_utils.h b/generator/internal/http_option_utils.h
@@ -123,7 +123,7 @@ std::string FormatRequestResource(
 /**
  * Parses the package name of the method and returns its API version.
  */
-StatusOr<std::string> FormatApiVersionFromPackageName(
+std::string FormatApiVersionFromPackageName(
     google::protobuf::MethodDescriptor const& method);
 
 }  // namespace generator_internal

diff --git a/generator/internal/http_option_utils_test.cc b/generator/internal/http_option_utils_test.cc
@@ -15,7 +15,6 @@
 #include "generator/internal/http_option_utils.h"
 #include "generator/testing/error_collectors.h"
 #include "generator/testing/fake_source_tree.h"
-#include "google/cloud/testing_util/status_matchers.h"
 #include <google/protobuf/descriptor.pb.h>
 #include <google/protobuf/descriptor_database.h>
 #include <google/protobuf/text_format.h>
@@ -27,14 +26,11 @@ namespace generator_internal {
 namespace {
 
 using ::google::cloud::generator_testing::FakeSourceTree;
-using ::google::cloud::testing_util::IsOkAndHolds;
-using ::google::cloud::testing_util::StatusIs;
 using ::google::protobuf::DescriptorPool;
 using ::google::protobuf::FileDescriptor;
 using ::google::protobuf::FileDescriptorProto;
 using ::google::protobuf::MethodDescriptor;
 using ::testing::Eq;
-using ::testing::HasSubstr;
 using ::testing::SizeIs;
 
 char const* const kHttpProto =
@@ -683,15 +679,15 @@ TEST_F(HttpOptionUtilsTest, HasRoutingHeaderWrongUrlFormat) {
   /// @cond
   auto constexpr kServiceText = R"pb(
     name: "google/foo/v1/service.proto"
-    package: "google.protobuf"
+    package: "google.foo.v1"
     message_type { name: "Bar" }
     message_type { name: "Empty" }
     service {
       name: "Service"
       method {
         name: "Method0"
-        input_type: "google.protobuf.Bar"
-        output_type: "google.protobuf.Empty"
+        input_type: "google.foo.v1.Bar"
+        output_type: "google.foo.v1.Empty"
         options {
           [google.api.http] { post: "/v2/entries:list" body: "*" }
         }
@@ -781,19 +777,18 @@ TEST_F(HttpOptionUtilsTest, FormatApiVersionFromPackageName) {
       pool_.FindFileByName("google/foo/v1/service.proto");
   MethodDescriptor const* method =
       service_file_descriptor->service(0)->method(8);
-  EXPECT_THAT(FormatApiVersionFromPackageName(*method), IsOkAndHolds(Eq("v1")));
+  EXPECT_THAT(FormatApiVersionFromPackageName(*method), Eq("v1"));
 }
 
 TEST_F(HttpOptionUtilsTest, FormatApiVersionFromPackageNameError) {
   FileDescriptor const* service_file_descriptor =
       pool_.FindFileByName("google/foo/v1/service_without_version.proto");
   MethodDescriptor const* method =
       service_file_descriptor->service(0)->method(0);
-  EXPECT_THAT(
+  EXPECT_DEATH_IF_SUPPORTED(
       FormatApiVersionFromPackageName(*method),
-      StatusIs(
-          StatusCode::kInvalidArgument,
-          HasSubstr("Unrecognized API version in package name: my.service")));
+      "Unrecognized API version in file: "
+      "google/foo/v1/service_without_version.proto, package: my.service");
 }
 
 }  // namespace

diff --git a/generator/internal/longrunning.cc b/generator/internal/longrunning.cc
@@ -179,10 +179,6 @@ void SetLongrunningOperationServiceVars(
       auto operation_service_extension =
           method->options().GetExtension(google::cloud::operation_service);
       auto api_version = FormatApiVersionFromPackageName(*method);
-      if (!api_version) {
-        GCP_LOG(FATAL) << "Unrecognized API version in package name: "
-                       << method->file()->package();
-      }
 
       if (operation_service_extension == "GlobalOperations") {
         service_vars["longrunning_operation_include_header"] =
@@ -206,7 +202,7 @@ void SetLongrunningOperationServiceVars(
                               rest_internal::DetermineApiVersion("%s", *options),
                               "/projects/", request.project(),
                               "/global/operations/", request.operation()))""",
-            *api_version);
+            api_version);
         service_vars["longrunning_get_operation_path_rest"] = global_lro_path;
         service_vars["longrunning_cancel_operation_path_rest"] =
             global_lro_path;
@@ -231,7 +227,7 @@ void SetLongrunningOperationServiceVars(
             R"""(absl::StrCat("/compute/",
                               rest_internal::DetermineApiVersion("%s", *options),
                               "/locations/global/operations/", request.operation()))""",
-            *api_version);
+            api_version);
         service_vars["longrunning_get_operation_path_rest"] =
             global_org_lro_path;
         service_vars["longrunning_cancel_operation_path_rest"] =
@@ -261,7 +257,7 @@ void SetLongrunningOperationServiceVars(
                               "/projects/", request.project(),
                               "/regions/", request.region(),
                               "/operations/", request.operation()))""",
-            *api_version);
+            api_version);
         service_vars["longrunning_get_operation_path_rest"] = region_lro_path;
         service_vars["longrunning_cancel_operation_path_rest"] =
             region_lro_path;
@@ -290,7 +286,7 @@ void SetLongrunningOperationServiceVars(
                               "/projects/", request.project(),
                               "/zones/", request.zone(),
                               "/operations/", request.operation()))""",
-            *api_version);
+            api_version);
         service_vars["longrunning_get_operation_path_rest"] = zone_lro_path;
         service_vars["longrunning_cancel_operation_path_rest"] = zone_lro_path;
       } else {