From a86e887fe2756935a51d4d93b42e45a0fdde7812 Mon Sep 17 00:00:00 2001 From: Darren Bolduc Date: Sat, 20 Jul 2024 12:35:55 -0400 Subject: [PATCH] cleanup(generator): api version error handling (#14512) --- generator/internal/http_option_utils.cc | 15 +++++++-------- generator/internal/http_option_utils.h | 2 +- generator/internal/http_option_utils_test.cc | 19 +++++++------------ generator/internal/longrunning.cc | 12 ++++-------- 4 files changed, 19 insertions(+), 29 deletions(-) diff --git a/generator/internal/http_option_utils.cc b/generator/internal/http_option_utils.cc index 3490663fecced..0a54152e5fcc1 100644 --- a/generator/internal/http_option_utils.cc +++ b/generator/internal/http_option_utils.cc @@ -342,7 +342,6 @@ absl::variant 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(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(s->value)) { auto v = absl::get(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 FormatApiVersionFromPackageName( +std::string FormatApiVersionFromPackageName( google::protobuf::MethodDescriptor const& method) { std::vector 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 index cc43fc4362788..7e3ccf0c8a2a4 100644 --- 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 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 index 20f04af74463c..c3a646fc1e062 100644 --- 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 #include #include @@ -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,7 +777,7 @@ 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) { @@ -789,11 +785,10 @@ TEST_F(HttpOptionUtilsTest, FormatApiVersionFromPackageNameError) { 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 index 96b16063ec43f..d8aef984ca6af 100644 --- 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 {