From c08773487dfba63a77b6e24cd09863d0d118109f Mon Sep 17 00:00:00 2001 From: dbolduc Date: Thu, 19 Sep 2024 03:16:06 +0000 Subject: [PATCH] cleanup(generator): api version only from path --- generator/internal/http_option_utils.cc | 58 ++++---------------- generator/internal/http_option_utils.h | 10 +--- generator/internal/http_option_utils_test.cc | 49 ++--------------- generator/internal/longrunning.cc | 30 ++++------ 4 files changed, 27 insertions(+), 120 deletions(-) diff --git a/generator/internal/http_option_utils.cc b/generator/internal/http_option_utils.cc index 7fd9b8933455..7573b9057199 100644 --- a/generator/internal/http_option_utils.cc +++ b/generator/internal/http_option_utils.cc @@ -54,12 +54,12 @@ std::string FormatFieldAccessorCall( return absl::StrJoin(chunks, "()."); } -void RestPathVisitorHelper(std::string const& api_version, +void RestPathVisitorHelper(absl::optional const& api_version, PathTemplate::Segment const& s, std::vector& path); struct RestPathVisitor { - explicit RestPathVisitor(std::string api_version, + explicit RestPathVisitor(absl::optional api_version, std::vector& path) : api_version(std::move(api_version)), path(path) {} void operator()(PathTemplate::Match const&) {} @@ -68,13 +68,13 @@ struct RestPathVisitor { path.emplace_back( [piece = s, api = api_version]( google::protobuf::MethodDescriptor const&, bool is_async) { - if (piece != api) return absl::StrFormat("\"%s\"", piece); + if (!api || piece != *api) return absl::StrFormat("\"%s\"", piece); if (is_async) { return absl::StrFormat( - "rest_internal::DetermineApiVersion(\"%s\", *options)", api); + "rest_internal::DetermineApiVersion(\"%s\", *options)", *api); } return absl::StrFormat( - "rest_internal::DetermineApiVersion(\"%s\", options)", api); + "rest_internal::DetermineApiVersion(\"%s\", options)", *api); }); } void operator()(PathTemplate::Variable const& v) { @@ -89,12 +89,13 @@ struct RestPathVisitor { RestPathVisitorHelper(api_version, s, path); } - std::string api_version; + absl::optional api_version; std::vector& path; }; void RestPathVisitorHelper( - std::string const& api_version, PathTemplate::Segment const& s, + absl::optional const& api_version, + PathTemplate::Segment const& s, std::vector& path) { absl::visit(RestPathVisitor{api_version, path}, s.value); } @@ -144,15 +145,6 @@ std::string FormatQueryParameterCode( return code; } -std::string FormatApiVersionFromPackageNameString( - std::string const& package_name, std::string const& file_name) { - std::vector parts = absl::StrSplit(package_name, '.'); - if (absl::StartsWith(parts.back(), "v")) return parts.back(); - GCP_LOG(FATAL) << "Unrecognized API version in file: " << file_name - << ", package: " << package_name; - return {}; // Suppress clang-tidy warnings -} - } // namespace void SetHttpDerivedMethodVars(HttpExtensionInfo const& info, @@ -274,8 +266,6 @@ void SetHttpQueryParameters(HttpExtensionInfo const& info, HttpExtensionInfo ParseHttpExtension( google::protobuf::MethodDescriptor const& method, - absl::optional package_name_override, - absl::optional file_name_override, absl::optional method_override) { if (!method.options().HasExtension(google::api::http)) return {}; @@ -341,24 +331,7 @@ HttpExtensionInfo ParseHttpExtension( out->append(absl::visit(SegmentAsStringVisitor{}, s->value)); }; - // parse api version from url, if url doesn't include version info, - // parse api version from package name, if package name doesn't include - // version info, raise fatal error. For non-mixin method, package name is its - // package's name. For mixin method, package name is the target service's - // package's name. - auto file_name = - file_name_override ? *file_name_override : method.file()->name(); - auto api_version_opt = FormatApiVersionFromUrlPattern(url_pattern, file_name); - std::string api_version; - if (api_version_opt) { - api_version = *api_version_opt; - } else { - api_version = package_name_override - ? FormatApiVersionFromPackageNameString( - *package_name_override, file_name) - : FormatApiVersionFromPackageName(method); - } - + auto api_version = FormatApiVersionFromUrlPattern(url_pattern); auto rest_path_visitor = RestPathVisitor(api_version, info.rest_path); for (auto const& s : parsed_http_rule->segments) { if (absl::holds_alternative(s->value)) { @@ -415,19 +388,10 @@ std::string FormatRequestResource(google::protobuf::Descriptor const& request, return absl::StrCat("request.", field_name, "()"); } -// 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. -std::string FormatApiVersionFromPackageName( - google::protobuf::MethodDescriptor const& method) { - return FormatApiVersionFromPackageNameString(method.file()->package(), - method.file()->name()); -} - // Generate api version by extracting the version from the url pattern. // In some cases(i.e. location), there is no version in the package name. absl::optional FormatApiVersionFromUrlPattern( - std::string const& url_pattern, std::string const& file_name) { + std::string const& url_pattern) { std::vector const parts = absl::StrSplit(url_pattern, '/'); static auto const* const kVersion = new std::regex{R"(v\d+)"}; for (auto const& part : parts) { @@ -435,8 +399,6 @@ absl::optional FormatApiVersionFromUrlPattern( return part; } } - GCP_LOG(WARNING) << "Unrecognized API version in file: " << file_name - << ", url pattern: " << url_pattern; return absl::nullopt; } diff --git a/generator/internal/http_option_utils.h b/generator/internal/http_option_utils.h index 60c78d1d835b..8b25325737e7 100644 --- a/generator/internal/http_option_utils.h +++ b/generator/internal/http_option_utils.h @@ -48,8 +48,6 @@ struct HttpExtensionInfo { */ HttpExtensionInfo ParseHttpExtension( google::protobuf::MethodDescriptor const& method, - absl::optional package_name_override = absl::nullopt, - absl::optional file_name_override = absl::nullopt, absl::optional method_override = absl::nullopt); /** @@ -115,17 +113,11 @@ bool HasHttpAnnotation(google::protobuf::MethodDescriptor const& method); std::string FormatRequestResource(google::protobuf::Descriptor const& request, HttpExtensionInfo const& info); -/** - * Parses the package name of the method and returns its API version. - */ -std::string FormatApiVersionFromPackageName( - google::protobuf::MethodDescriptor const& method); - /** * Parses the url pattern of the method and returns its API version. */ absl::optional FormatApiVersionFromUrlPattern( - std::string const& url_pattern, std::string const& file_name); + std::string const& url_pattern); } // namespace generator_internal } // namespace cloud diff --git a/generator/internal/http_option_utils_test.cc b/generator/internal/http_option_utils_test.cc index 37dde9b466e7..c269935124af 100644 --- a/generator/internal/http_option_utils_test.cc +++ b/generator/internal/http_option_utils_test.cc @@ -30,7 +30,6 @@ using ::google::protobuf::DescriptorPool; using ::google::protobuf::FileDescriptor; using ::google::protobuf::FileDescriptorProto; using ::google::protobuf::MethodDescriptor; -using ::google::protobuf::ServiceDescriptor; using ::testing::AllOf; using ::testing::Contains; using ::testing::ElementsAre; @@ -592,22 +591,7 @@ TEST_F(HttpOptionUtilsTest, ParseHttpExtensionWithoutVersionInUrl) { EXPECT_THAT(info.url_path, Eq("/no/version/id/in/url")); } -TEST_F(HttpOptionUtilsTest, ParseHttpExtensionWithoutVersionInPackageAndUrl) { - FileDescriptor const* service_file = pool_.FindFileByName( - "google/foo/v1/service_without_version_in_package_and_url.proto"); - MethodDescriptor const* method = service_file->service(0)->method(0); - EXPECT_DEATH_IF_SUPPORTED( - ParseHttpExtension(*method), - "Unrecognized API version in file: " - "google/foo/v1/service_without_version_in_package_and_url.proto, " - "package: no.version.in.package"); -} - TEST_F(HttpOptionUtilsTest, ParseHttpExtensionWithMixinOverrides) { - FileDescriptor const* service_file = - pool_.FindFileByName("google/foo/v1/service.proto"); - ServiceDescriptor const* service = service_file->service(0); - FileDescriptor const* mixin_file = pool_.FindFileByName("google/cloud/location/locations.proto"); MethodDescriptor const* mixin_method = mixin_file->service(0)->method(0); @@ -615,9 +599,7 @@ TEST_F(HttpOptionUtilsTest, ParseHttpExtensionWithMixinOverrides) { auto mixin_method_override = MixinMethodOverride{"Post", "/v1/{name=projects/*/locations/*}", "*"}; - auto info = - ParseHttpExtension(*mixin_method, service->file()->package(), - service->file()->name(), mixin_method_override); + auto info = ParseHttpExtension(*mixin_method, mixin_method_override); EXPECT_THAT(info.url_path, Eq("/v1/{name=projects/*/locations/*}")); EXPECT_THAT(info.body, Eq("*")); EXPECT_THAT(info.http_verb, Eq("Post")); @@ -864,41 +846,18 @@ TEST_F(HttpOptionUtilsTest, FormatRequestResourceAnnotatedRequestField) { EXPECT_THAT(result, Eq("request.namespace_()")); } -TEST_F(HttpOptionUtilsTest, FormatApiVersionFromPackageName) { - FileDescriptor const* service_file_descriptor = - pool_.FindFileByName("google/foo/v1/service.proto"); - MethodDescriptor const* method = - service_file_descriptor->service(0)->method(8); - EXPECT_THAT(FormatApiVersionFromPackageName(*method), Eq("v1")); -} - -TEST_F(HttpOptionUtilsTest, FormatApiVersionFromPackageNameError) { - FileDescriptor const* service_file_descriptor = pool_.FindFileByName( - "google/foo/v1/service_without_version_in_package.proto"); - MethodDescriptor const* method = - service_file_descriptor->service(0)->method(0); - EXPECT_DEATH_IF_SUPPORTED( - FormatApiVersionFromPackageName(*method), - "Unrecognized API version in file: " - "google/foo/v1/service_without_version_in_package.proto, package: " - "my.service"); -} - TEST_F(HttpOptionUtilsTest, FormatApiVersionFromUrlPattern) { - std::string file_name = "google/foo/v1/service.proto"; std::string url_pattern_v1 = "/v1/foo/bar"; - EXPECT_THAT(FormatApiVersionFromUrlPattern(url_pattern_v1, file_name), + EXPECT_THAT(FormatApiVersionFromUrlPattern(url_pattern_v1), Optional(std::string("v1"))); std::string url_pattern_v2 = "/foo/v2/bar"; - EXPECT_THAT(FormatApiVersionFromUrlPattern(url_pattern_v2, file_name), + EXPECT_THAT(FormatApiVersionFromUrlPattern(url_pattern_v2), Optional(std::string("v2"))); } TEST_F(HttpOptionUtilsTest, FormatApiVersionFromUrlPatternNonExist) { - std::string file_name = "google/foo/v1/service.proto"; std::string url_pattern = "/foo/bar"; - EXPECT_THAT(FormatApiVersionFromUrlPattern(url_pattern, file_name), - Eq(absl::nullopt)); + EXPECT_THAT(FormatApiVersionFromUrlPattern(url_pattern), Eq(absl::nullopt)); } } // namespace diff --git a/generator/internal/longrunning.cc b/generator/internal/longrunning.cc index e8d674c4c78b..36e0dc4e0700 100644 --- a/generator/internal/longrunning.cc +++ b/generator/internal/longrunning.cc @@ -172,7 +172,6 @@ void SetLongrunningOperationServiceVars( *method, method->output_type()->full_name()))); auto operation_service_extension = method->options().GetExtension(google::cloud::operation_service); - auto api_version = FormatApiVersionFromPackageName(*method); if (operation_service_extension == "GlobalOperations") { service_vars["longrunning_operation_include_header"] = @@ -191,12 +190,11 @@ void SetLongrunningOperationServiceVars( r.set_project(info.project); r.set_operation(info.operation); )"""; - auto global_lro_path = absl::StrFormat( + std::string global_lro_path = R"""(absl::StrCat("/compute/", - rest_internal::DetermineApiVersion("%s", *options), + rest_internal::DetermineApiVersion("v1", *options), "/projects/", request.project(), - "/global/operations/", request.operation()))""", - api_version); + "/global/operations/", request.operation()))"""; service_vars["longrunning_get_operation_path_rest"] = global_lro_path; service_vars["longrunning_cancel_operation_path_rest"] = global_lro_path; @@ -217,11 +215,10 @@ void SetLongrunningOperationServiceVars( service_vars["longrunning_await_set_operation_fields"] = R"""( r.set_operation(info.operation); )"""; - auto global_org_lro_path = absl::StrFormat( + std::string global_org_lro_path = R"""(absl::StrCat("/compute/", - rest_internal::DetermineApiVersion("%s", *options), - "/locations/global/operations/", request.operation()))""", - api_version); + rest_internal::DetermineApiVersion("v1", *options), + "/locations/global/operations/", request.operation()))"""; service_vars["longrunning_get_operation_path_rest"] = global_org_lro_path; service_vars["longrunning_cancel_operation_path_rest"] = @@ -245,13 +242,12 @@ void SetLongrunningOperationServiceVars( r.set_region(info.region); r.set_operation(info.operation); )"""; - auto region_lro_path = absl::StrFormat( + std::string region_lro_path = R"""(absl::StrCat("/compute/", - rest_internal::DetermineApiVersion("%s", *options), + rest_internal::DetermineApiVersion("v1", *options), "/projects/", request.project(), "/regions/", request.region(), - "/operations/", request.operation()))""", - api_version); + "/operations/", request.operation()))"""; service_vars["longrunning_get_operation_path_rest"] = region_lro_path; service_vars["longrunning_cancel_operation_path_rest"] = region_lro_path; @@ -274,13 +270,11 @@ void SetLongrunningOperationServiceVars( r.set_zone(info.zone); r.set_operation(info.operation); )"""; - auto zone_lro_path = absl::StrFormat( - R"""(absl::StrCat("/compute/", - rest_internal::DetermineApiVersion("%s", *options), + std::string zone_lro_path = R"""(absl::StrCat("/compute/", + rest_internal::DetermineApiVersion("v1", *options), "/projects/", request.project(), "/zones/", request.zone(), - "/operations/", request.operation()))""", - api_version); + "/operations/", request.operation()))"""; service_vars["longrunning_get_operation_path_rest"] = zone_lro_path; service_vars["longrunning_cancel_operation_path_rest"] = zone_lro_path; } else {