Skip to content

Commit 6d4ec5b

Browse files
scotthartscotthart
authored
refactor(generator): improve robustness of BuildNamespaces (#5169)
* refactor(generator): improve robustness of BuildNamespaces
1 parent 783fe3e commit 6d4ec5b

File tree

5 files changed

+105
-72
lines changed

5 files changed

+105
-72
lines changed

‎generator/internal/codegen_utils.cc

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -75,27 +75,15 @@ std::string ProtoNameToCppName(absl::string_view proto_name) {
7575
return "::" + absl::StrReplaceAll(proto_name, {{".", "::"}});
7676
}
7777

78-
StatusOr<std::vector<std::string>> BuildNamespaces(VarsDictionary const& vars,
79-
NamespaceType ns_type) {
80-
auto iter = vars.find("product_path");
81-
if (iter == vars.end()) {
82-
return Status(StatusCode::kNotFound,
83-
"product_path must be present in vars.");
84-
}
85-
std::string product_path = iter->second;
86-
if (product_path.back() != '/') {
87-
return Status(StatusCode::kInvalidArgument,
88-
"vars[product_path] must end with '/'.");
89-
}
90-
if (product_path.size() < 2) {
91-
return Status(StatusCode::kInvalidArgument,
92-
"vars[product_path] contain at least 2 characters.");
93-
}
94-
std::vector<std::string> v = absl::StrSplit(product_path, '/');
95-
auto name = v[v.size() - 2];
78+
std::vector<std::string> BuildNamespaces(std::string const& product_path,
79+
NamespaceType ns_type) {
80+
std::vector<std::string> v =
81+
absl::StrSplit(product_path, '/', absl::SkipEmpty());
82+
std::string name =
83+
absl::StrJoin(v.begin() + (v.size() > 2 ? 2 : 0), v.end(), "_");
9684
std::string inline_ns = absl::AsciiStrToUpper(name) + "_CLIENT_NS";
9785
if (ns_type == NamespaceType::kInternal) {
98-
name = absl::StrCat(name, "_internal");
86+
absl::StrAppend(&name, "_internal");
9987
}
10088

10189
return std::vector<std::string>{"google", "cloud", name, inline_ns};

‎generator/internal/codegen_utils.h

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,26 @@ enum class NamespaceType { kNormal, kInternal };
7171

7272
/**
7373
* Builds namespace hierarchy.
74+
*
75+
* Typically used with a product_path like to 'google/cloud/product/' and
76+
* returns {"google", "cloud", "product", "PRODUCT_CLIENT_NS"}.
77+
*
78+
* If the path contains fewer than two directories, they will be concatenated
79+
* to form the product value, e.g. 'unusual/product/' returns
80+
* {"google", "cloud", "unusual_product", "UNUSUAL_PRODUCT_CLIENT_NS"}.
81+
*
82+
* If the path contains more than three directories the third and subsequent
83+
* directories will be concatenated, e.g. 'google/cloud/foo/bar/baz/' returns
84+
* {"google", "cloud", "foo_bar_baz", "FOO_BAR_BAZ_CLIENT_NS"}.
85+
*
86+
* If ns_type is `NamespaceType::kInternal`, "_internal" is appended to the
87+
* product, e.g. 'google/cloud/product/' returns
88+
* {"google", "cloud", "product_internal", "PRODUCT_CLIENT_NS"}.
89+
*
7490
*/
75-
StatusOr<std::vector<std::string>> BuildNamespaces(
76-
VarsDictionary const& vars, NamespaceType ns_type = NamespaceType::kNormal);
91+
std::vector<std::string> BuildNamespaces(
92+
std::string const& product_path,
93+
NamespaceType ns_type = NamespaceType::kNormal);
7794

7895
/**
7996
* Validates command line arguments passed to the microgenerator.

‎generator/internal/codegen_utils_test.cc

Lines changed: 72 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -72,52 +72,78 @@ TEST(ProtoNameToCppName, Success) {
7272
ProtoNameToCppName("google.spanner.admin.database.v1.Request"));
7373
}
7474

75-
TEST(BuildNamespace, EmptyVars) {
76-
VarsDictionary vars;
77-
auto result = BuildNamespaces(vars, NamespaceType::kInternal);
78-
EXPECT_EQ(result.status().code(), StatusCode::kNotFound);
79-
EXPECT_EQ(result.status().message(), "product_path must be present in vars.");
80-
}
81-
82-
TEST(BuildNamespace, NoTrailingSlash) {
83-
VarsDictionary vars;
84-
vars["product_path"] = "google/cloud/spanner";
85-
auto result = BuildNamespaces(vars, NamespaceType::kInternal);
86-
EXPECT_EQ(result.status().code(), StatusCode::kInvalidArgument);
87-
EXPECT_EQ(result.status().message(), "vars[product_path] must end with '/'.");
88-
}
89-
90-
TEST(BuildNamespace, ProductPathTooShort) {
91-
VarsDictionary vars;
92-
vars["product_path"] = std::string("/");
93-
auto result = BuildNamespaces(vars, NamespaceType::kInternal);
94-
EXPECT_EQ(result.status().code(), StatusCode::kInvalidArgument);
95-
EXPECT_EQ(result.status().message(),
96-
"vars[product_path] contain at least 2 characters.");
97-
}
98-
99-
TEST(BuildNamespaces, Internal) {
100-
VarsDictionary vars;
101-
vars["product_path"] = "google/cloud/spanner/";
102-
auto result = BuildNamespaces(vars, NamespaceType::kInternal);
103-
ASSERT_TRUE(result.ok());
104-
ASSERT_EQ(result->size(), 4);
105-
EXPECT_EQ("google", (*result)[0]);
106-
EXPECT_EQ("cloud", (*result)[1]);
107-
EXPECT_EQ("spanner_internal", (*result)[2]);
108-
EXPECT_EQ("SPANNER_CLIENT_NS", (*result)[3]);
109-
}
110-
111-
TEST(BuildNamespaces, NotInternal) {
112-
VarsDictionary vars;
113-
vars["product_path"] = "google/cloud/translation/";
114-
auto result = BuildNamespaces(vars);
115-
ASSERT_TRUE(result.ok());
116-
ASSERT_EQ(result->size(), 4);
117-
EXPECT_EQ("google", (*result)[0]);
118-
EXPECT_EQ("cloud", (*result)[1]);
119-
EXPECT_EQ("translation", (*result)[2]);
120-
EXPECT_EQ("TRANSLATION_CLIENT_NS", (*result)[3]);
75+
TEST(BuildNamespaces, NoDirectoryPathInternal) {
76+
auto result = BuildNamespaces("/", NamespaceType::kInternal);
77+
ASSERT_EQ(result.size(), 4);
78+
EXPECT_EQ("google", result[0]);
79+
EXPECT_EQ("cloud", result[1]);
80+
EXPECT_EQ("_internal", result[2]);
81+
EXPECT_EQ("_CLIENT_NS", result[3]);
82+
}
83+
84+
TEST(BuildNamespaces, OneDirectoryPathInternal) {
85+
auto result = BuildNamespaces("one/", NamespaceType::kInternal);
86+
ASSERT_EQ(result.size(), 4);
87+
EXPECT_EQ("google", result[0]);
88+
EXPECT_EQ("cloud", result[1]);
89+
EXPECT_EQ("one_internal", result[2]);
90+
EXPECT_EQ("ONE_CLIENT_NS", result[3]);
91+
}
92+
93+
TEST(BuildNamespaces, TwoDirectoryPathInternal) {
94+
auto result = BuildNamespaces("unusual/product/", NamespaceType::kInternal);
95+
ASSERT_EQ(result.size(), 4);
96+
EXPECT_EQ("google", result[0]);
97+
EXPECT_EQ("cloud", result[1]);
98+
EXPECT_EQ("unusual_product_internal", result[2]);
99+
EXPECT_EQ("UNUSUAL_PRODUCT_CLIENT_NS", result[3]);
100+
}
101+
102+
TEST(BuildNamespaces, TwoDirectoryPathNotInternal) {
103+
auto result = BuildNamespaces("unusual/product/");
104+
ASSERT_EQ(result.size(), 4);
105+
EXPECT_EQ("google", result[0]);
106+
EXPECT_EQ("cloud", result[1]);
107+
EXPECT_EQ("unusual_product", result[2]);
108+
EXPECT_EQ("UNUSUAL_PRODUCT_CLIENT_NS", result[3]);
109+
}
110+
111+
TEST(BuildNamespaces, ThreeDirectoryPathInternal) {
112+
auto result =
113+
BuildNamespaces("google/cloud/spanner/", NamespaceType::kInternal);
114+
ASSERT_EQ(result.size(), 4);
115+
EXPECT_EQ("google", result[0]);
116+
EXPECT_EQ("cloud", result[1]);
117+
EXPECT_EQ("spanner_internal", result[2]);
118+
EXPECT_EQ("SPANNER_CLIENT_NS", result[3]);
119+
}
120+
121+
TEST(BuildNamespaces, ThreeDirectoryPathNotInternal) {
122+
auto result = BuildNamespaces("google/cloud/translation/");
123+
ASSERT_EQ(result.size(), 4);
124+
EXPECT_EQ("google", result[0]);
125+
EXPECT_EQ("cloud", result[1]);
126+
EXPECT_EQ("translation", result[2]);
127+
EXPECT_EQ("TRANSLATION_CLIENT_NS", result[3]);
128+
}
129+
130+
TEST(BuildNamespaces, FourDirectoryPathInternal) {
131+
auto result =
132+
BuildNamespaces("google/cloud/foo/bar/baz/", NamespaceType::kInternal);
133+
ASSERT_EQ(result.size(), 4);
134+
EXPECT_EQ("google", result[0]);
135+
EXPECT_EQ("cloud", result[1]);
136+
EXPECT_EQ("foo_bar_baz_internal", result[2]);
137+
EXPECT_EQ("FOO_BAR_BAZ_CLIENT_NS", result[3]);
138+
}
139+
140+
TEST(BuildNamespaces, FourDirectoryPathNotInternal) {
141+
auto result = BuildNamespaces("google/cloud/foo/bar/baz/");
142+
ASSERT_EQ(result.size(), 4);
143+
EXPECT_EQ("google", result[0]);
144+
EXPECT_EQ("cloud", result[1]);
145+
EXPECT_EQ("foo_bar_baz", result[2]);
146+
EXPECT_EQ("FOO_BAR_BAZ_CLIENT_NS", result[3]);
121147
}
122148

123149
TEST(ProcessCommandLineArgs, NoProductPath) {

‎generator/internal/service_code_generator.cc

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,12 @@ void ServiceCodeGenerator::GenerateSystemIncludes(
7070
}
7171

7272
Status ServiceCodeGenerator::OpenNamespaces(Printer& p, NamespaceType ns_type) {
73-
auto result = BuildNamespaces(service_vars_, ns_type);
74-
if (!result.ok()) {
75-
return result.status();
73+
auto result = service_vars_.find("product_path");
74+
if (result == service_vars_.end()) {
75+
return Status(StatusCode::kInternal,
76+
"product_path not found in service_vars_");
7677
}
77-
namespaces_ = result.value();
78+
namespaces_ = BuildNamespaces(service_vars_["product_path"], ns_type);
7879
for (auto const& nspace : namespaces_) {
7980
if (absl::EndsWith(nspace, "_CLIENT_NS")) {
8081
p.Print("inline namespace $namespace$ {\n", "namespace", nspace);

‎generator/internal/service_code_generator.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ class ServiceCodeGenerator : public GeneratorInterface {
5555
FileType file_type = FileType::kHeaderFile);
5656
static void GenerateSystemIncludes(Printer& p,
5757
std::vector<std::string> system_includes);
58-
Status OpenNamespaces(Printer& p, NamespaceType ns_type);
58+
Status OpenNamespaces(Printer& p,
59+
NamespaceType ns_type = NamespaceType::kNormal);
5960
void CloseNamespaces(Printer& p);
6061

6162
google::protobuf::ServiceDescriptor const* service_descriptor_;

0 commit comments

Comments
 (0)