Skip to content

Commit

Permalink
cleanup(generator): api version only from path
Browse files Browse the repository at this point in the history
  • Loading branch information
@dbolduc
dbolduc committed Sep 19, 2024
1 parent 47b18cd commit c087734
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 120 deletions.
58 changes: 10 additions & 48 deletions generator/internal/http_option_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ std::string FormatFieldAccessorCall(
return absl::StrJoin(chunks, "().");
}

void RestPathVisitorHelper(std::string const& api_version,
void RestPathVisitorHelper(absl::optional<std::string> const& api_version,
PathTemplate::Segment const& s,
std::vector<HttpExtensionInfo::RestPathPiece>& path);

struct RestPathVisitor {
explicit RestPathVisitor(std::string api_version,
explicit RestPathVisitor(absl::optional<std::string> api_version,
std::vector<HttpExtensionInfo::RestPathPiece>& path)
: api_version(std::move(api_version)), path(path) {}
void operator()(PathTemplate::Match const&) {}
Expand All @@ -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) {
Expand All @@ -89,12 +89,13 @@ struct RestPathVisitor {
RestPathVisitorHelper(api_version, s, path);
}

std::string api_version;
absl::optional<std::string> api_version;
std::vector<HttpExtensionInfo::RestPathPiece>& path;
};

void RestPathVisitorHelper(
std::string const& api_version, PathTemplate::Segment const& s,
absl::optional<std::string> const& api_version,
PathTemplate::Segment const& s,
std::vector<HttpExtensionInfo::RestPathPiece>& path) {
absl::visit(RestPathVisitor{api_version, path}, s.value);
}
Expand Down Expand Up @@ -144,15 +145,6 @@ std::string FormatQueryParameterCode(
return code;
}

std::string FormatApiVersionFromPackageNameString(
std::string const& package_name, std::string const& file_name) {
std::vector<std::string> 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,
Expand Down Expand Up @@ -274,8 +266,6 @@ void SetHttpQueryParameters(HttpExtensionInfo const& info,

HttpExtensionInfo ParseHttpExtension(
google::protobuf::MethodDescriptor const& method,
absl::optional<std::string> package_name_override,
absl::optional<std::string> file_name_override,
absl::optional<MixinMethodOverride> method_override) {
if (!method.options().HasExtension(google::api::http)) return {};

Expand Down Expand Up @@ -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<PathTemplate::Variable>(s->value)) {
Expand Down Expand Up @@ -415,28 +388,17 @@ 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<std::string> FormatApiVersionFromUrlPattern(
std::string const& url_pattern, std::string const& file_name) {
std::string const& url_pattern) {
std::vector<std::string> const parts = absl::StrSplit(url_pattern, '/');
static auto const* const kVersion = new std::regex{R"(v\d+)"};
for (auto const& part : parts) {
if (std::regex_match(part, *kVersion)) {
return part;
}
}
GCP_LOG(WARNING) << "Unrecognized API version in file: " << file_name
<< ", url pattern: " << url_pattern;
return absl::nullopt;
}

Expand Down
10 changes: 1 addition & 9 deletions generator/internal/http_option_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ struct HttpExtensionInfo {
*/
HttpExtensionInfo ParseHttpExtension(
google::protobuf::MethodDescriptor const& method,
absl::optional<std::string> package_name_override = absl::nullopt,
absl::optional<std::string> file_name_override = absl::nullopt,
absl::optional<MixinMethodOverride> method_override = absl::nullopt);

/**
Expand Down Expand Up @@ -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<std::string> FormatApiVersionFromUrlPattern(
std::string const& url_pattern, std::string const& file_name);
std::string const& url_pattern);

} // namespace generator_internal
} // namespace cloud
Expand Down
49 changes: 4 additions & 45 deletions generator/internal/http_option_utils_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -592,32 +591,15 @@ 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);

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"));
Expand Down Expand Up @@ -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
Expand Down
30 changes: 12 additions & 18 deletions generator/internal/longrunning.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"] =
Expand All @@ -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;
Expand All @@ -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"] =
Expand All @@ -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;
Expand All @@ -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 {
Expand Down

0 comments on commit c087734

Please sign in to comment.