Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup(generator): api version error handling #14512

Merged
merged 2 commits into from
Jul 20, 2024
Merged

cleanup(generator): api version error handling #14512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94536dc
cleanup(generator): api version error handling
dbolduc Jul 20, 2024
6fade38
address review comments
dbolduc Jul 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions generator/internal/http_option_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,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 =
Expand Down Expand Up @@ -393,7 +392,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();
Expand All @@ -414,7 +413,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);
Expand Down Expand Up @@ -487,15 +486,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
Expand Down
2 changes: 1 addition & 1 deletion generator/internal/http_option_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 7 additions & 12 deletions generator/internal/http_option_utils_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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 =
Expand Down Expand Up @@ -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: "*" }
}
Expand Down Expand Up @@ -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
Expand Down
12 changes: 4 additions & 8 deletions generator/internal/longrunning.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"] =
Expand All @@ -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;
Expand All @@ -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"] =
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
Loading