Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-sheets-json-error.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@googleworkspace/cli": patch
---

Report validation error for invalid --json-values instead of silent empty append
53 changes: 42 additions & 11 deletions src/helpers/sheets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ TIPS:
) -> Pin<Box<dyn Future<Output = Result<bool, GwsError>> + Send + 'a>> {
Box::pin(async move {
if let Some(matches) = matches.subcommand_matches("+append") {
let config = parse_append_args(matches);
let config = parse_append_args(matches)?;
let (params_str, body_str, scopes) = build_append_request(&config, doc)?;

let scope_strs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect();
Expand Down Expand Up @@ -257,6 +257,7 @@ fn build_read_request(
/// Configuration for appending values to a spreadsheet.
///
/// Holds the parsed arguments for the `+append` subcommand.
#[derive(Debug)]
pub struct AppendConfig {
/// The ID of the spreadsheet to append to.
pub spreadsheet_id: String,
Expand All @@ -267,7 +268,8 @@ pub struct AppendConfig {
/// Parses arguments for the `+append` command.
///
/// Supports both `--values` (single row) and `--json-values` (single or multi-row).
pub fn parse_append_args(matches: &ArgMatches) -> AppendConfig {
/// Returns a validation error if `--json-values` contains invalid JSON.
pub fn parse_append_args(matches: &ArgMatches) -> Result<AppendConfig, GwsError> {
let values = if let Some(json_str) = matches.get_one::<String>("json-values") {
// Try parsing as array-of-arrays (multi-row) first
if let Ok(parsed) = serde_json::from_str::<Vec<Vec<String>>>(json_str) {
Expand All @@ -276,21 +278,20 @@ pub fn parse_append_args(matches: &ArgMatches) -> AppendConfig {
// Single flat array — treat as one row
vec![parsed]
} 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\"]]'."
)));
Comment on lines +281 to +283
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, added the serde_json error details to the message so users can see exactly what went wrong

}
Comment on lines 280 to 284
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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\"]]'."
            )));
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if let Some(values_str) = matches.get_one::<String>("values") {
vec![values_str.split(',').map(|s| s.to_string()).collect()]
} else {
Vec::new()
};

AppendConfig {
Ok(AppendConfig {
spreadsheet_id: matches.get_one::<String>("spreadsheet").unwrap().clone(),
values,
}
})
}

/// Configuration for reading values from a spreadsheet.
Expand Down Expand Up @@ -395,7 +396,7 @@ mod tests {
#[test]
fn test_parse_append_args_values() {
let matches = make_matches_append(&["test", "--spreadsheet", "123", "--values", "a,b,c"]);
let config = parse_append_args(&matches);
let config = parse_append_args(&matches).unwrap();
assert_eq!(config.spreadsheet_id, "123");
assert_eq!(config.values, vec![vec!["a", "b", "c"]]);
}
Expand All @@ -409,7 +410,7 @@ mod tests {
"--json-values",
r#"["a","b","c"]"#,
]);
let config = parse_append_args(&matches);
let config = parse_append_args(&matches).unwrap();
assert_eq!(config.values, vec![vec!["a", "b", "c"]]);
}

Expand All @@ -422,7 +423,7 @@ mod tests {
"--json-values",
r#"[["Alice","100"],["Bob","200"]]"#,
]);
let config = parse_append_args(&matches);
let config = parse_append_args(&matches).unwrap();
assert_eq!(
config.values,
vec![vec!["Alice", "100"], vec!["Bob", "200"]]
Expand All @@ -447,6 +448,36 @@ mod tests {
assert_eq!(values[1], json!(["Bob", "200"]));
}

#[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"]]);
}
Comment on lines +451 to +462
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, removed the duplicate test


#[test]
fn test_parse_append_args_json_invalid_returns_error() {
let matches = make_matches_append(&[
"test",
"--spreadsheet",
"123",
"--json-values",
"not valid json",
]);
let err = parse_append_args(&matches).unwrap_err();
let msg = err.to_string();
assert!(
msg.contains("--json-values is not valid JSON"),
"expected JSON error message, got: {msg}"
);
}

#[test]
fn test_parse_read_args() {
let matches = make_matches_read(&["test", "--spreadsheet", "123", "--range", "A1:B2"]);
Expand Down
Loading