fix(sheets): report error for invalid --json-values instead of silent empty append#463
fix(sheets): report error for invalid --json-values instead of silent empty append#463anshul-garg27 wants to merge 2 commits intogoogleworkspace:mainfrom
Conversation
googleworkspace-bot
added
the
area: core
🦋 Changeset detectedLatest commit: d3d3a51 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness and user experience of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 correctly addresses an issue where invalid JSON for --json-values was silently ignored. The change to parse_append_args to return a Result and propagate the error is a good solution. I've included one suggestion to further improve the JSON parsing logic by using a serde untagged enum, which would make it more efficient and maintainable.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the silent failure when parsing invalid JSON for --json-values by returning a validation error. The changes to propagate the error and the addition of tests are well-implemented. However, I've identified a potential issue in the new parsing logic that could lead to silent data loss in cases of mixed-type JSON arrays. My suggestion aims to make the parsing stricter and more robust to prevent this.
|
Ready for review — all tests passing, Gemini feedback addressed. @jpoehnelt |
|
need to resolve conflicts |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
==========================================
+ Coverage 67.22% 67.28% +0.05%
==========================================
Files 38 38
Lines 16679 16721 +42
==========================================
+ Hits 11213 11250 +37
- Misses 5466 5471 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… empty append parse_append_args() used unwrap_or_default() when JSON parsing failed, silently converting invalid input into an empty Vec. This caused +append --json-values with malformed JSON to append an empty row without any error message, making debugging very difficult. - Change parse_append_args() to return Result<AppendConfig, GwsError> - Surface the serde_json error message with guidance on expected format - Add #[derive(Debug)] to AppendConfig for Result unwrap in tests - Add tests for valid JSON and invalid JSON error paths
anshul-garg27
force-pushed
the
fix/sheets-json-parse-error
branch
from
ca1a241 to
d13d206
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a silent failure where invalid JSON for --json-values was ignored. The change to propagate a Result and return a validation error is a significant improvement. The new tests effectively cover the error case. I've added one suggestion to further enhance the error reporting by distinguishing between JSON syntax errors and incorrect data structures, which will provide more precise feedback to the user.
| } else { | ||
| eprintln!( | ||
| "Warning: --json-values is not valid JSON; expected an array or array-of-arrays" | ||
| ); | ||
| Vec::new() | ||
| return Err(GwsError::Validation(format!( | ||
| "--json-values is not valid JSON: expected an array like '[\"a\",\"b\"]' or array-of-arrays like '[[\"a\",\"b\"],[\"c\",\"d\"]]'." | ||
| ))); | ||
| } |
There was a problem hiding this comment.
The current error message for invalid --json-values can be misleading. It reports "not valid JSON" even for inputs that are syntactically correct JSON but have the wrong structure (e.g., an object instead of an array). To provide more accurate feedback, it's better to first attempt to parse the input as a generic serde_json::Value. This allows you to distinguish between a syntax error and a structure/type error, and include the specific parsing error from serde_json in the message, as alluded to by the <details> in the pull request description.
} else {
// Check if it's a JSON syntax error or just a type mismatch.
let err_msg = if let Err(e) = serde_json::from_str::<serde_json::Value>(json_str) {
format!("--json-values is not valid JSON: {e}")
} else {
"--json-values has an incorrect structure".to_string()
};
return Err(GwsError::Validation(format!(
"{err_msg}. Expected an array like '[\"a\",\"b\"]' or array-of-arrays like '[[\"a\",\"b\"],[\"c\",\"d\"]]'."
)));
}There was a problem hiding this comment.
you're right, updated the error to first check if it's valid JSON at all vs valid JSON but wrong structure, so the message is more accurate
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where invalid JSON for --json-values failed silently. The change to return a validation error is a significant improvement. My review includes a suggestion to make the new error message more detailed by including the specific parsing error, which would further improve usability. I also noticed a duplicated test case that should be removed to maintain a clean test suite.
| return Err(GwsError::Validation(format!( | ||
| "--json-values is not valid JSON: expected an array like '[\"a\",\"b\"]' or array-of-arrays like '[[\"a\",\"b\"],[\"c\",\"d\"]]'." | ||
| ))); |
There was a problem hiding this comment.
The PR description mentions including error details (<details>) for invalid JSON, but the current implementation provides a generic error message. To improve the user experience and align with the PR's goal, consider including the specific parsing error from serde_json. This will help users debug their input more effectively.
| return Err(GwsError::Validation(format!( | |
| "--json-values is not valid JSON: expected an array like '[\"a\",\"b\"]' or array-of-arrays like '[[\"a\",\"b\"],[\"c\",\"d\"]]'." | |
| ))); | |
| let err = serde_json::from_str::<serde_json::Value>(json_str).unwrap_err(); | |
| let msg = format!("--json-values is not valid JSON: {err}. Expected a JSON array like '[\"a\",\"b\"]' or array-of-arrays like '[[\"a\",\"b\"],[\"c\",\"d\"]]'."); | |
| return Err(GwsError::Validation(msg)); |
There was a problem hiding this comment.
good point, added the serde_json error details to the message so users can see exactly what went wrong
| #[test] | ||
| fn test_parse_append_args_json_valid() { | ||
| let matches = make_matches_append(&[ | ||
| "test", | ||
| "--spreadsheet", | ||
| "123", | ||
| "--json-values", | ||
| r#"["a","b","c"]"#, | ||
| ]); | ||
| let config = parse_append_args(&matches).unwrap(); | ||
| assert_eq!(config.values, vec![vec!["a", "b", "c"]]); | ||
| } |
There was a problem hiding this comment.
This test, test_parse_append_args_json_valid, is a duplicate of the existing test_parse_append_args_json_single_row test (lines 404-415). To keep the test suite clean and maintainable, this redundant test should be removed.
There was a problem hiding this comment.
yep, removed the duplicate test
3 participants
Summary
+append --json-values "bad input"silently appended an empty row becauseunwrap_or_default()discarded the parse error--json-values is not valid JSON: <details>. Expected a JSON array like '["a","b"]' or '[["a","b"],["c","d"]]'.Changes
parse_append_args()returnsResult<AppendConfig, GwsError>instead of bareAppendConfig?at the call site inhandle()#[derive(Debug)]toAppendConfig(required forResult::unwrap()in tests)test_parse_append_args_json_validandtest_parse_append_args_json_invalid_returns_errorTest plan
cargo test— 586 passed, 0 failedcargo clippy -- -D warnings— cleanGenerated with Claude Code