Skip to content

Commit 7bc9991

Browse files
colinmoycolinmoy
committed
fix(generator): do not redact key field when sibling value field is present
1 parent c74ae87 commit 7bc9991

118 files changed

Lines changed: 346 additions & 264 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎generator/internal/discovery_type_vertex.cc‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,9 @@ bool IsStringOrBytes(nlohmann::json const& field_json) {
5656
}
5757

5858
bool ContainsKeyWord(std::string_view s) {
59-
auto const tokens = absl::StrSplit(s, '_');
59+
std::vector<std::string_view> const tokens = absl::StrSplit(s, '_');
6060
return std::any_of(tokens.begin(), tokens.end(),
61-
[](absl::string_view token) { return token == "key"; });
61+
[](std::string_view token) { return token == "key"; });
6262
}
6363

6464
} // namespace
@@ -337,8 +337,8 @@ DiscoveryTypeVertex::FormatPropertiesHelper( // NOLINT(misc-no-recursion)
337337
std::string json_field_name, int indent_level,
338338
MessageProperties& message_properties,
339339
google::protobuf::Descriptor const* message_descriptor,
340-
std::set<std::string>& current_field_names,
341-
std::string const& indent) const {
340+
std::set<std::string>& current_field_names, std::string const& indent,
341+
bool has_sibling_value) const {
342342
try {
343343
if (field.contains("id")) {
344344
json_field_name = field["id"];
@@ -381,7 +381,8 @@ DiscoveryTypeVertex::FormatPropertiesHelper( // NOLINT(misc-no-recursion)
381381
message_properties.lines.push_back(absl::StrFormat(
382382
"%s%s%s%s %s = %d%s;", FormatMessageDescription(field, indent_level),
383383
indent, introducer, type_name, field_name, *field_number,
384-
FormatFieldOptions(field_name, json_field_name, field)));
384+
FormatFieldOptions(field_name, json_field_name, field,
385+
has_sibling_value)));
385386
if (*field_number == message_properties.next_available_field_number) {
386387
++message_properties.next_available_field_number;
387388
}
@@ -417,13 +418,14 @@ DiscoveryTypeVertex::FormatProperties( // NOLINT(misc-no-recursion)
417418
std::set<std::string> current_field_names;
418419
if (json.contains("properties")) {
419420
auto const& properties = json.find("properties");
421+
bool const has_sibling_value = properties->contains("value");
420422
for (auto p = properties->begin(); p != properties->end(); ++p) {
421423
auto const& field = p.value();
422424
auto const& field_key = p.key();
423425
auto result = FormatPropertiesHelper(
424426
types, message_name, qualified_message_name, file_package_name,
425427
field, field_key, indent_level, message_properties,
426-
message_descriptor, current_field_names, indent);
428+
message_descriptor, current_field_names, indent, has_sibling_value);
427429
if (!result.ok()) return result;
428430
}
429431
}
@@ -434,7 +436,7 @@ DiscoveryTypeVertex::FormatProperties( // NOLINT(misc-no-recursion)
434436
auto result = FormatPropertiesHelper(
435437
types, message_name, qualified_message_name, file_package_name, json,
436438
message_name, indent_level, message_properties, message_descriptor,
437-
current_field_names, indent);
439+
current_field_names, indent, false);
438440
if (!result.ok()) return result;
439441
}
440442

@@ -502,7 +504,7 @@ std::string DiscoveryTypeVertex::FormatMessageDescription(
502504

503505
std::string DiscoveryTypeVertex::FormatFieldOptions(
504506
std::string const& field_name, std::string const& json_field_name,
505-
nlohmann::json const& field_json) {
507+
nlohmann::json const& field_json, bool has_sibling_value) {
506508
std::vector<std::pair<std::string, std::string>> field_options;
507509
if (field_json.value("required", false)) {
508510
field_options.emplace_back("google.api.field_behavior", "REQUIRED");
@@ -513,7 +515,9 @@ std::string DiscoveryTypeVertex::FormatFieldOptions(
513515
absl::StrCat("\"", field_name, "\""));
514516
}
515517

516-
if (IsStringOrBytes(field_json) && ContainsKeyWord(field_name)) {
518+
auto const is_key_value_pair = field_name == "key" && has_sibling_value;
519+
if (IsStringOrBytes(field_json) && ContainsKeyWord(field_name) &&
520+
!is_key_value_pair) {
517521
field_options.emplace_back("debug_redact", "true");
518522
}
519523

‎generator/internal/discovery_type_vertex.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,8 @@ class DiscoveryTypeVertex {
124124
// Formats any field options as indicated by the field_json.
125125
static std::string FormatFieldOptions(std::string const& field_name,
126126
std::string const& json_field_name,
127-
nlohmann::json const& field_json);
127+
nlohmann::json const& field_json,
128+
bool has_sibling_value);
128129

129130
// Determines the correct field_number to use for the specified field.
130131
static StatusOr<int> GetFieldNumber(
@@ -153,8 +154,8 @@ class DiscoveryTypeVertex {
153154
std::string json_field_name, int indent_level,
154155
MessageProperties& message_properties,
155156
google::protobuf::Descriptor const* message_descriptor,
156-
std::set<std::string>& current_field_names,
157-
std::string const& indent) const;
157+
std::set<std::string>& current_field_names, std::string const& indent,
158+
bool has_sibling_value) const;
158159

159160
std::string name_;
160161
std::string package_name_;

‎generator/internal/discovery_type_vertex_test.cc‎

Lines changed: 109 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,9 @@ TEST(DiscoveryTypeVertexTest, FormatFieldOptionsEmpty) {
112112
auto constexpr kOptionalEmptyFieldJson = R"""({})""";
113113
auto json = nlohmann::json::parse(kOptionalEmptyFieldJson, nullptr, false);
114114
ASSERT_TRUE(json.is_object());
115-
EXPECT_THAT(
116-
DiscoveryTypeVertex::FormatFieldOptions("test_field", "testField", json),
117-
Eq(" [json_name=\"testField\"]"));
115+
EXPECT_THAT(DiscoveryTypeVertex::FormatFieldOptions("test_field", "testField",
116+
json, false),
117+
Eq(" [json_name=\"testField\"]"));
118118
}
119119

120120
TEST(DiscoveryTypeVertexTest, FormatFieldOptionsRequired) {
@@ -127,7 +127,8 @@ TEST(DiscoveryTypeVertexTest, FormatFieldOptionsRequired) {
127127
auto json = nlohmann::json::parse(kOptionalRequiredFieldJson, nullptr, false);
128128
ASSERT_TRUE(json.is_object());
129129
EXPECT_THAT(
130-
DiscoveryTypeVertex::FormatFieldOptions("test_field", "testField", json),
130+
DiscoveryTypeVertex::FormatFieldOptions("test_field", "testField", json,
131+
false),
131132
Eq(" [(google.api.field_behavior) = REQUIRED,json_name=\"testField\"]"));
132133
}
133134

@@ -141,10 +142,10 @@ TEST(DiscoveryTypeVertexTest, FormatFieldOptionsOperationRequestField) {
141142
auto json =
142143
nlohmann::json::parse(kOptionalOperationRequestFieldJson, nullptr, false);
143144
ASSERT_TRUE(json.is_object());
144-
EXPECT_THAT(
145-
DiscoveryTypeVertex::FormatFieldOptions("test_field", "testField", json),
146-
Eq(" [(google.cloud.operation_request_field) = "
147-
"\"test_field\",json_name=\"testField\"]"));
145+
EXPECT_THAT(DiscoveryTypeVertex::FormatFieldOptions("test_field", "testField",
146+
json, false),
147+
Eq(" [(google.cloud.operation_request_field) = "
148+
"\"test_field\",json_name=\"testField\"]"));
148149
}
149150

150151
TEST(DiscoveryTypeVertexTest, FormatFieldOptionsRequiredOperationRequestField) {
@@ -158,11 +159,11 @@ TEST(DiscoveryTypeVertexTest, FormatFieldOptionsRequiredOperationRequestField) {
158159
auto json =
159160
nlohmann::json::parse(kOptionalOperationRequestFieldJson, nullptr, false);
160161
ASSERT_TRUE(json.is_object());
161-
EXPECT_THAT(
162-
DiscoveryTypeVertex::FormatFieldOptions("test_field", "testField", json),
163-
Eq(" [(google.api.field_behavior) = "
164-
"REQUIRED,(google.cloud.operation_request_field) = "
165-
"\"test_field\",json_name=\"testField\"]"));
162+
EXPECT_THAT(DiscoveryTypeVertex::FormatFieldOptions("test_field", "testField",
163+
json, false),
164+
Eq(" [(google.api.field_behavior) = "
165+
"REQUIRED,(google.cloud.operation_request_field) = "
166+
"\"test_field\",json_name=\"testField\"]"));
166167
}
167168

168169
TEST(DiscoveryTypeVertexTest, FormatFieldOptionsRequiredIsResource) {
@@ -176,10 +177,10 @@ TEST(DiscoveryTypeVertexTest, FormatFieldOptionsRequiredIsResource) {
176177
auto json =
177178
nlohmann::json::parse(kRequiredIsResourceFieldJson, nullptr, false);
178179
ASSERT_TRUE(json.is_object());
179-
EXPECT_THAT(
180-
DiscoveryTypeVertex::FormatFieldOptions("test_field", "testField", json),
181-
Eq(" [(google.api.field_behavior) = "
182-
"REQUIRED,json_name=\"__json_request_body\"]"));
180+
EXPECT_THAT(DiscoveryTypeVertex::FormatFieldOptions("test_field", "testField",
181+
json, false),
182+
Eq(" [(google.api.field_behavior) = "
183+
"REQUIRED,json_name=\"__json_request_body\"]"));
183184
}
184185

185186
TEST(DiscoveryTypeVertexTest, FormatFieldOptionsDebugRedactString) {
@@ -191,7 +192,7 @@ TEST(DiscoveryTypeVertexTest, FormatFieldOptionsDebugRedactString) {
191192
auto json = nlohmann::json::parse(kFieldJson, nullptr, false);
192193
ASSERT_TRUE(json.is_object());
193194
EXPECT_THAT(
194-
DiscoveryTypeVertex::FormatFieldOptions("raw_key", "rawKey", json),
195+
DiscoveryTypeVertex::FormatFieldOptions("raw_key", "rawKey", json, false),
195196
Eq(" [debug_redact = true,json_name=\"rawKey\"]"));
196197
}
197198

@@ -204,7 +205,7 @@ TEST(DiscoveryTypeVertexTest, FormatFieldOptionsDebugRedactBytes) {
204205
auto json = nlohmann::json::parse(kFieldJson, nullptr, false);
205206
ASSERT_TRUE(json.is_object());
206207
EXPECT_THAT(
207-
DiscoveryTypeVertex::FormatFieldOptions("raw_key", "rawKey", json),
208+
DiscoveryTypeVertex::FormatFieldOptions("raw_key", "rawKey", json, false),
208209
Eq(" [debug_redact = true,json_name=\"rawKey\"]"));
209210
}
210211

@@ -220,7 +221,7 @@ TEST(DiscoveryTypeVertexTest, FormatFieldOptionsDebugRedactArrayString) {
220221
auto json = nlohmann::json::parse(kFieldJson, nullptr, false);
221222
ASSERT_TRUE(json.is_object());
222223
EXPECT_THAT(
223-
DiscoveryTypeVertex::FormatFieldOptions("raw_key", "rawKey", json),
224+
DiscoveryTypeVertex::FormatFieldOptions("raw_key", "rawKey", json, false),
224225
Eq(" [debug_redact = true,json_name=\"rawKey\"]"));
225226
}
226227

@@ -236,7 +237,7 @@ TEST(DiscoveryTypeVertexTest, FormatFieldOptionsDebugRedactArrayBytes) {
236237
auto json = nlohmann::json::parse(kFieldJson, nullptr, false);
237238
ASSERT_TRUE(json.is_object());
238239
EXPECT_THAT(
239-
DiscoveryTypeVertex::FormatFieldOptions("raw_key", "rawKey", json),
240+
DiscoveryTypeVertex::FormatFieldOptions("raw_key", "rawKey", json, false),
240241
Eq(" [debug_redact = true,json_name=\"rawKey\"]"));
241242
}
242243

@@ -248,15 +249,18 @@ TEST(DiscoveryTypeVertexTest, FormatFieldOptionsDebugRedactNonMatches) {
248249
)""";
249250
auto json = nlohmann::json::parse(kFieldJson, nullptr, false);
250251
ASSERT_TRUE(json.is_object());
251-
EXPECT_THAT(DiscoveryTypeVertex::FormatFieldOptions("monkey", "monkey", json),
252-
Eq(" [json_name=\"monkey\"]"));
253252
EXPECT_THAT(
254-
DiscoveryTypeVertex::FormatFieldOptions("keyboard", "keyboard", json),
255-
Eq(" [json_name=\"keyboard\"]"));
256-
EXPECT_THAT(DiscoveryTypeVertex::FormatFieldOptions("hockey", "hockey", json),
257-
Eq(" [json_name=\"hockey\"]"));
258-
EXPECT_THAT(DiscoveryTypeVertex::FormatFieldOptions("keypad", "keypad", json),
259-
Eq(" [json_name=\"keypad\"]"));
253+
DiscoveryTypeVertex::FormatFieldOptions("monkey", "monkey", json, false),
254+
Eq(" [json_name=\"monkey\"]"));
255+
EXPECT_THAT(DiscoveryTypeVertex::FormatFieldOptions("keyboard", "keyboard",
256+
json, false),
257+
Eq(" [json_name=\"keyboard\"]"));
258+
EXPECT_THAT(
259+
DiscoveryTypeVertex::FormatFieldOptions("hockey", "hockey", json, false),
260+
Eq(" [json_name=\"hockey\"]"));
261+
EXPECT_THAT(
262+
DiscoveryTypeVertex::FormatFieldOptions("keypad", "keypad", json, false),
263+
Eq(" [json_name=\"keypad\"]"));
260264
}
261265

262266
TEST(DiscoveryTypeVertexTest,
@@ -268,8 +272,9 @@ TEST(DiscoveryTypeVertexTest,
268272
)""";
269273
auto json = nlohmann::json::parse(kFieldJson, nullptr, false);
270274
ASSERT_TRUE(json.is_object());
271-
EXPECT_THAT(DiscoveryTypeVertex::FormatFieldOptions("key_id", "keyId", json),
272-
Eq(" [json_name=\"keyId\"]"));
275+
EXPECT_THAT(
276+
DiscoveryTypeVertex::FormatFieldOptions("key_id", "keyId", json, false),
277+
Eq(" [json_name=\"keyId\"]"));
273278
}
274279

275280
TEST(DiscoveryTypeVertexTest, FormatFieldOptionsDebugRedactRequired) {
@@ -282,11 +287,52 @@ TEST(DiscoveryTypeVertexTest, FormatFieldOptionsDebugRedactRequired) {
282287
auto json = nlohmann::json::parse(kFieldJson, nullptr, false);
283288
ASSERT_TRUE(json.is_object());
284289
EXPECT_THAT(
285-
DiscoveryTypeVertex::FormatFieldOptions("raw_key", "rawKey", json),
290+
DiscoveryTypeVertex::FormatFieldOptions("raw_key", "rawKey", json, false),
286291
Eq(" [(google.api.field_behavior) = "
287292
"REQUIRED,debug_redact = true,json_name=\"rawKey\"]"));
288293
}
289294

295+
TEST(DiscoveryTypeVertexTest,
296+
FormatFieldOptionsDebugRedactKeyWithSiblingValueNotRedacted) {
297+
auto constexpr kFieldJson = R"""(
298+
{
299+
"type": "string"
300+
}
301+
)""";
302+
auto json = nlohmann::json::parse(kFieldJson, nullptr, false);
303+
ASSERT_TRUE(json.is_object());
304+
EXPECT_THAT(DiscoveryTypeVertex::FormatFieldOptions("key", "key", json, true),
305+
Eq(" [json_name=\"key\"]"));
306+
}
307+
308+
TEST(DiscoveryTypeVertexTest,
309+
FormatFieldOptionsDebugRedactKeyWithoutSiblingValueRedacted) {
310+
auto constexpr kFieldJson = R"""(
311+
{
312+
"type": "string"
313+
}
314+
)""";
315+
auto json = nlohmann::json::parse(kFieldJson, nullptr, false);
316+
ASSERT_TRUE(json.is_object());
317+
EXPECT_THAT(
318+
DiscoveryTypeVertex::FormatFieldOptions("key", "key", json, false),
319+
Eq(" [debug_redact = true,json_name=\"key\"]"));
320+
}
321+
322+
TEST(DiscoveryTypeVertexTest,
323+
FormatFieldOptionsDebugRedactKeyCompoundWithSiblingValueRedacted) {
324+
auto constexpr kFieldJson = R"""(
325+
{
326+
"type": "string"
327+
}
328+
)""";
329+
auto json = nlohmann::json::parse(kFieldJson, nullptr, false);
330+
ASSERT_TRUE(json.is_object());
331+
EXPECT_THAT(
332+
DiscoveryTypeVertex::FormatFieldOptions("raw_key", "rawKey", json, true),
333+
Eq(" [debug_redact = true,json_name=\"rawKey\"]"));
334+
}
335+
290336
struct DetermineTypesSuccess {
291337
std::string name;
292338
std::string json;
@@ -1458,6 +1504,37 @@ TEST_F(DiscoveryTypeVertexDescriptorTest, JsonToProtobufCustomerEncryptionKey) {
14581504
EXPECT_THAT(*result, Eq(kExpectedProto));
14591505
}
14601506

1507+
TEST_F(DiscoveryTypeVertexDescriptorTest, JsonToProtobufMapItemsKeyValuePair) {
1508+
auto constexpr kSchemaJson = R"""(
1509+
{
1510+
"id": "DataItem",
1511+
"properties": {
1512+
"key": {
1513+
"type": "string"
1514+
},
1515+
"value": {
1516+
"type": "string"
1517+
}
1518+
}
1519+
}
1520+
)""";
1521+
1522+
auto constexpr kExpectedProto = R"""(message DataItem {
1523+
optional string key = 1 [json_name="key"];
1524+
1525+
optional string value = 2 [json_name="value"];
1526+
}
1527+
)""";
1528+
1529+
auto json = nlohmann::json::parse(kSchemaJson, nullptr, false);
1530+
ASSERT_TRUE(json.is_object());
1531+
DiscoveryTypeVertex t("DataItem", "test.package", json, &pool());
1532+
std::map<std::string, DiscoveryTypeVertex> types;
1533+
auto result = t.JsonToProtobufMessage(types, "test.package");
1534+
ASSERT_THAT(result, ::google::cloud::testing_util::IsOk());
1535+
EXPECT_THAT(*result, Eq(kExpectedProto));
1536+
}
1537+
14611538
} // namespace
14621539
} // namespace generator_internal
14631540
} // namespace cloud

‎protos/google/cloud/compute/v1/internal/common_000.proto‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1554,7 +1554,7 @@ message BackendServiceList {
15541554
// warning about invalid network settings (for example, if an instance
15551555
// attempts to perform IP forwarding but is not enabled for IP
15561556
// forwarding).
1557-
optional string key = 1 [debug_redact = true, json_name = "key"];
1557+
optional string key = 1 [json_name = "key"];
15581558

15591559
// [Output Only] A warning data value corresponding to the key.
15601560
optional string value = 2 [json_name = "value"];
@@ -1685,7 +1685,7 @@ message BackendServiceListUsable {
16851685
// warning about invalid network settings (for example, if an instance
16861686
// attempts to perform IP forwarding but is not enabled for IP
16871687
// forwarding).
1688-
optional string key = 1 [debug_redact = true, json_name = "key"];
1688+
optional string key = 1 [json_name = "key"];
16891689

16901690
// [Output Only] A warning data value corresponding to the key.
16911691
optional string value = 2 [json_name = "value"];

‎protos/google/cloud/compute/v1/internal/common_002.proto‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,7 @@ message ReservationSubBlocksListResponse {
278278
// warning about invalid network settings (for example, if an instance
279279
// attempts to perform IP forwarding but is not enabled for IP
280280
// forwarding).
281-
optional string key = 1 [debug_redact = true, json_name = "key"];
281+
optional string key = 1 [json_name = "key"];
282282

283283
// [Output Only] A warning data value corresponding to the key.
284284
optional string value = 2 [json_name = "value"];

‎protos/google/cloud/compute/v1/internal/common_003.proto‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ message AcceleratorTypeAggregatedList {
182182
// warning about invalid network settings (for example, if an instance
183183
// attempts to perform IP forwarding but is not enabled for IP
184184
// forwarding).
185-
optional string key = 1 [debug_redact = true, json_name = "key"];
185+
optional string key = 1 [json_name = "key"];
186186

187187
// [Output Only] A warning data value corresponding to the key.
188188
optional string value = 2 [json_name = "value"];
@@ -312,7 +312,7 @@ message AcceleratorTypeList {
312312
// warning about invalid network settings (for example, if an instance
313313
// attempts to perform IP forwarding but is not enabled for IP
314314
// forwarding).
315-
optional string key = 1 [debug_redact = true, json_name = "key"];
315+
optional string key = 1 [json_name = "key"];
316316

317317
// [Output Only] A warning data value corresponding to the key.
318318
optional string value = 2 [json_name = "value"];
@@ -424,7 +424,7 @@ message AcceleratorTypesScopedList {
424424
// warning about invalid network settings (for example, if an instance
425425
// attempts to perform IP forwarding but is not enabled for IP
426426
// forwarding).
427-
optional string key = 1 [debug_redact = true, json_name = "key"];
427+
optional string key = 1 [json_name = "key"];
428428

429429
// [Output Only] A warning data value corresponding to the key.
430430
optional string value = 2 [json_name = "value"];

‎protos/google/cloud/compute/v1/internal/common_005.proto‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,7 @@ message AddressList {
354354
// warning about invalid network settings (for example, if an instance
355355
// attempts to perform IP forwarding but is not enabled for IP
356356
// forwarding).
357-
optional string key = 1 [debug_redact = true, json_name = "key"];
357+
optional string key = 1 [json_name = "key"];
358358

359359
// [Output Only] A warning data value corresponding to the key.
360360
optional string value = 2 [json_name = "value"];

0 commit comments

Comments
 (0)