web: fix whitespace-fragile extract_bool_value in request_handler#10139
web: fix whitespace-fragile extract_bool_value in request_handler#10139oharboe wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
Conversation
extract_bool_value searched for the literal string "value":true with no tolerance for whitespace after the colon, unlike every other extract_* helper in the same file. If a client sent pretty-printed JSON (space after ':'), the function silently fell through to the integer fallback path. Rewrite to skip whitespace consistently, matching the pattern used by extract_int and extract_float_or. Signed-off-by: Oyvind Harboe <oyvind.harboe@zylin.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
@maliberty just an experiment: I asked claude to find low churn maintainability issues in recent pull request merges. Close if not useful. |
|
clang-tidy review says "All clean, LGTM! 👍" |
![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 refactors the extract_bool_value function in request_handler.cpp to manually parse JSON values and include a fallback for integer-based booleans. The review feedback suggests extracting the shared parsing logic into a common helper function to reduce duplication and recommends verifying the full boolean literals ('true' and 'false') rather than just the first character to ensure parsing robustness.
| const std::string needle = "\"value\""; | ||
| auto pos = json.find(needle); | ||
| if (pos == std::string::npos) { | ||
| return false; | ||
| } | ||
| pos = json.find(':', pos + needle.size()); | ||
| if (pos == std::string::npos) { | ||
| return false; | ||
| } | ||
| // Skip whitespace after colon | ||
| pos++; | ||
| while (pos < json.size() && (json[pos] == ' ' || json[pos] == '\t')) { | ||
| pos++; | ||
| } |
There was a problem hiding this comment.
This logic for finding the key, locating the colon, and skipping whitespace is identical to the implementation in extract_int and extract_float_or. Consider extracting this into a common helper function (e.g., find_value_start) to reduce code duplication and improve maintainability. Note that helper logic should be defined as a free function in a namespace rather than as a local lambda.
References
- To improve maintainability and reduce redundancy, extract duplicated logic into a helper function.
- Helper logic should be defined as a free function in a namespace rather than as a local lambda within a function.
| if (pos < json.size() && json[pos] == 't') { | ||
| return true; | ||
| } | ||
| if (json.find("\"value\":false") != std::string::npos) { | ||
| if (pos < json.size() && json[pos] == 'f') { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The current implementation only checks the first character ('t' or 'f') to determine the boolean value. This is significantly less robust than the previous implementation and could lead to incorrect parsing if the value starts with these letters but isn't a valid JSON boolean (e.g., "value": "total" would be parsed as true). It is better to check for the full literal.
| if (pos < json.size() && json[pos] == 't') { | |
| return true; | |
| } | |
| if (json.find("\"value\":false") != std::string::npos) { | |
| if (pos < json.size() && json[pos] == 'f') { | |
| return false; | |
| } | |
| if (pos < json.size() && json.compare(pos, 4, "true") == 0) { | |
| return true; | |
| } | |
| if (pos < json.size() && json.compare(pos, 5, "false") == 0) { | |
| return false; | |
| } |
1 participant
extract_bool_value searched for the literal string "value":true with no tolerance for whitespace after the colon, unlike every other extract_* helper in the same file. If a client sent pretty-printed JSON (space after ':'), the function silently fell through to the integer fallback path. Rewrite to skip whitespace consistently, matching the pattern used by extract_int and extract_float_or.