Repository navigation
feat(generator): add debug_redact to key fields and enable redact in DebugString - #16398
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the discovery-to-proto generator to support mapping 'any' types to google.protobuf.Value or google.protobuf.Struct instead of always defaulting to google.protobuf.Any. It also introduces automatic redaction (debug_redact = true) for string/bytes fields containing 'key' in their name and updates DebugString to respect this option. The review feedback highlights several potential nlohmann::json::type_error exceptions that could occur if JSON Schema elements (like additionalProperties, items, or format) are not objects or strings as expected, recommending explicit type checks. Additionally, it suggests refining the redaction logic with a word-boundary check to prevent false positives on fields containing 'key' as a substring (e.g., 'monkey'), utilizing std::string_view per the repository style guide.
I am having trouble creating individual review comments. Click here to see my feedback.
generator/internal/discovery_to_proto.cc (384-395)
In JSON Schema, additionalProperties can be a boolean (e.g., false or true). If items["additionalProperties"] is a boolean, calling .value("type", "") or .contains("format") on it will throw a nlohmann::json::type_error exception (specifically type_error.306 because the JSON value is not an object). We should verify that items["additionalProperties"] is an object before calling these methods.
if (IsDiscoveryArrayType(*current)) {
auto const& items = (*current)["items"];
if (items.is_object() &&
items.contains("type") && items["type"] == "object" &&
items.contains("additionalProperties") &&
items["additionalProperties"].is_object() &&
items["additionalProperties"].value("type", "") == "any" &&
!items.contains("format") &&
!items["additionalProperties"].contains("format")) {
types_to_import.insert("google.protobuf.Struct");
} else {
worklist.push_back(&items);
}
}generator/internal/discovery_to_proto.cc (397-407)
Similar to the array items check, additional_properties can be a boolean value in JSON Schema. If it is not an object, calling .contains() on it will throw a nlohmann::json::type_error exception. We should ensure additional_properties is an object before querying its properties.
if (IsDiscoveryMapType(*current)) {
auto const& additional_properties = (*current)["additionalProperties"];
if (additional_properties.is_object() &&
additional_properties.contains("type") &&
additional_properties["type"] == "any" &&
!additional_properties.contains("format") &&
!current->contains("format")) {
types_to_import.insert("google.protobuf.Struct");
} else {
worklist.push_back(&additional_properties);
}
}generator/internal/discovery_to_proto.cc (363-372)
When checking the "format" field, we should ensure it is actually a string before assigning it to std::string const format. If "format" is present but is not a string (e.g., a boolean or an integer), the implicit conversion to std::string will throw a nlohmann::json::type_error exception.
if (current->contains("format") && (*current)["format"].is_string()) {
std::string const format = (*current)["format"];
if (absl::StartsWith(format, "google.protobuf.")) {
types_to_import.insert(format);
// This node is resolved as a protobuf message (e.g. google.protobuf.Any
// on Status.details items), so do not traverse into its internal
// additionalProperties or properties.
continue;
}generator/internal/discovery_type_vertex.cc (48-56)
If field_json["items"] is not an object, calling .value("type", "") on it will throw a nlohmann::json::type_error exception. We should ensure field_json["items"] is an object before attempting to retrieve its "type".
bool IsStringOrBytes(nlohmann::json const& field_json) {
std::string const type = field_json.value("type", "");
if (type == "string" || type == "bytes") return true;
if (type == "array" && field_json.contains("items") &&
field_json["items"].is_object()) {
std::string const item_type = field_json["items"].value("type", "");
if (item_type == "string" || item_type == "bytes") return true;
}
return false;
}generator/internal/discovery_type_vertex.cc (124-132)
Ensure that v["format"] is actually a string before assigning it to type to prevent a potential nlohmann::json::type_error exception.
if (type == "any") {
if (v.contains("format") && v["format"].is_string()) {
type = v["format"];
} else {
type = "google.protobuf.Value";
}
return TypeInfo{type, compare_package_name, properties_for_synthesis, false,
false};
}generator/internal/discovery_type_vertex.cc (165-174)
Ensure that additional_properties and v are objects and their "format" fields are strings before assigning them to map_type to prevent a potential nlohmann::json::type_error exception.
} else if (map_type == "any") {
if (additional_properties.is_object() &&
additional_properties.contains("format") &&
additional_properties["format"].is_string()) {
map_type = additional_properties["format"];
} else if (v.contains("format") && v["format"].is_string()) {
map_type = v["format"];
} else {
map_type = "google.protobuf.Struct";
}
return TypeInfo{map_type, compare_package_name,
properties_for_synthesis, true, is_message};generator/internal/discovery_type_vertex.cc (208-228)
Ensure that items and items["additionalProperties"] are objects and their "format" fields are strings before querying them or assigning them to type to prevent potential nlohmann::json::type_error exceptions.
} else if (type == "any") {
if (items.is_object() && items.contains("format") &&
items["format"].is_string()) {
type = items["format"];
} else {
type = "google.protobuf.Value";
}
return TypeInfo{type, compare_package_name, nullptr, false, false};
} else if (type == "object" && items.is_object() &&
items.contains("properties")) {
// Synthesize a nested type for this array.
type = CapitalizeFirstLetter(field_name + "Item");
return TypeInfo{type, compare_package_name, &items, false, true};
} else if (type == "object" && items.is_object() &&
items.contains("additionalProperties") &&
items["additionalProperties"].is_object() &&
items["additionalProperties"].value("type", "") == "any") {
if (items.contains("format") && items["format"].is_string()) {
type = items["format"];
} else if (items["additionalProperties"].contains("format") &&
items["additionalProperties"]["format"].is_string()) {
type = items["additionalProperties"]["format"];
} else {
type = "google.protobuf.Struct";
}
return TypeInfo{type, compare_package_name, nullptr, false, false};generator/internal/discovery_type_vertex.cc (510-514)
The current substring check absl::StrContains(field_name, "key") is highly prone to false positives and will over-redact fields that happen to contain "key" as a substring but are not actual keys (e.g., monkey, keyboard, hockey, keypad, keynote).
Since field_name is already formatted as snake_case by the generator, we can perform a precise word-boundary check to ensure "key" is matched only as a standalone word (e.g., kms_key_name, raw_key, rsa_encrypted_key, or key itself). This also eliminates the need to check json_field_name separately.
You can add a helper function ContainsKeyWord in the anonymous namespace:
bool ContainsKeyWord(std::string_view s) {
for (std::size_t pos = s.find("key"); pos != std::string_view::npos;
pos = s.find("key", pos + 1)) {
bool const prefix_ok = (pos == 0 || s[pos - 1] == '_');
bool const suffix_ok = (pos + 3 == s.size() || s[pos + 3] == '_');
if (prefix_ok && suffix_ok) return true;
}
return false;
} if (IsStringOrBytes(field_json) && ContainsKeyWord(field_name)) {
field_options.emplace_back("debug_redact", "true");
}References
- Prefer using
std::string_viewinstead ofabsl::string_viewin C++ code. (link)
… and JSON type safety
| } | ||
|
|
||
| if (current->contains("format")) { | ||
| if (current->contains("format") && (*current)["format"].is_string()) { |
There was a problem hiding this comment.
If the discovery document contains a format field that is not a string, this is a fundamental problem that the generator cannot recover from. We should emit an error and exit.
| auto const& items = (*current)["items"]; | ||
| if (items.contains("type") && items["type"] == "object" && | ||
| items.contains("additionalProperties") && | ||
| if (items.is_object() && items.contains("type") && |
There was a problem hiding this comment.
Similarly, if this isn't an object, then the discovery document is invalid. Emit an error and exit.
Same below. If checks to verify the structure of the document fail, these are non-recoverable.
…n across protobuf versions
…iscovery_type_vertex
…tRedactDebugString
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16398 +/- ##
==========================================
+ Coverage 92.24% 92.27% +0.02%
==========================================
Files 2246 2246
Lines 212136 212297 +161
==========================================
+ Hits 195692 195888 +196
+ Misses 16444 16409 -35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| bool ContainsKeyWord(std::string_view s) { | ||
| auto const tokens = absl::StrSplit(s, '_'); |
There was a problem hiding this comment.
Instead of auto, explicitly name the type, especially with absl::StrSplit as it can have several different return types. I would imagine we want a std::vector<std::string_view> here.
| bool ContainsKeyWord(std::string_view s) { | ||
| auto const tokens = absl::StrSplit(s, '_'); | ||
| return std::any_of(tokens.begin(), tokens.end(), | ||
| [](absl::string_view token) { return token == "key"; }); |
There was a problem hiding this comment.
Prefer std::string_view to absl::string_view. Any absl type that has a std type equivalent should usually be preferred.
| absl::StrCat("\"", field_name, "\"")); | ||
| } | ||
|
|
||
| if (IsStringOrBytes(field_json) && ContainsKeyWord(field_name)) { |
There was a problem hiding this comment.
Looking through the emitted protos, I think I underestimated how many fields named key would be false positives for redacting. It looks like there's a lot of types that contain both key and value fields that are simply a map of items. Let's see if we can improve our logic to determine when to redact, by NOT redacting fields named exactly key when the field value is present as a sibling field in the message type.
…g-redact-keys # Conflicts: # google/cloud/internal/debug_string_protobuf.cc
| } | ||
|
|
||
| Status | ||
| DiscoveryTypeVertex::FormatPropertiesHelper( // NOLINT(misc-no-recursion) |
There was a problem hiding this comment.
Let's create an internal cleanup issue to look at refactoring the parameters to this functions. 13 separate parameters is too many. We'll want to consider grouping some into structs.
Fixes: b/510919426