fix(formatter): exit non-zero on JSON serialization failure instead of silent empty output#828
Conversation
…f silent empty output
🦋 Changeset detectedLatest commit: b069a27 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 addresses an issue where JSON serialization failures were being silently swallowed, resulting in empty output and a successful exit code. By implementing explicit error handling in the formatter, the CLI now provides actionable feedback to the user when serialization fails, improving overall robustness and debugging capabilities. Highlights
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 the 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 counterproductive. 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. Footnotes
|
googleworkspace-bot
added
the
area: core
![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 improves error handling during JSON serialization by printing an error message to stderr and exiting with a non-zero status code instead of silently returning an empty string. It also adds a corresponding unit test and a changeset file. The review feedback highlights a security concern regarding potential terminal escape sequence injection, recommending that the error messages printed to stderr be sanitized using crate::output::sanitize_for_terminal.
| OutputFormat::Json => match serde_json::to_string_pretty(value) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| eprintln!("error: failed to serialize response to JSON: {e}"); | ||
| std::process::exit(1); | ||
| } | ||
| }, |
There was a problem hiding this comment.
The error message from serde_json::to_string_pretty is printed directly to stderr without sanitization. To prevent potential terminal escape sequence injection from untrusted input contained within the error details, the error string must be sanitized using crate::output::sanitize_for_terminal before printing.
| OutputFormat::Json => match serde_json::to_string_pretty(value) { | |
| Ok(s) => s, | |
| Err(e) => { | |
| eprintln!("error: failed to serialize response to JSON: {e}"); | |
| std::process::exit(1); | |
| } | |
| }, | |
| OutputFormat::Json => match serde_json::to_string_pretty(value) { | |
| Ok(s) => s, | |
| Err(e) => { | |
| let err_msg = crate::output::sanitize_for_terminal(&e.to_string()); | |
| eprintln!("error: failed to serialize response to JSON: {err_msg}"); | |
| std::process::exit(1); | |
| } | |
| }, |
References
- Sanitize error strings printed to the terminal to prevent escape sequence injection.
There was a problem hiding this comment.
Fixed in 41f8a6f. Both error paths now route through \crate::output::sanitize_for_terminal\ before printing to stderr, so any embedded escape sequences in the serialization error text are stripped before they reach the terminal.
There was a problem hiding this comment.
The sanitize_for_terminal call in this path was addressed in the previous commit. With the Result refactor in b069a27, the error no longer gets printed in the formatter at all — it propagates to the caller and surfaces through the normal GwsError error path.
| OutputFormat::Json => match serde_json::to_string(value) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| eprintln!("error: failed to serialize response to JSON: {e}"); | ||
| std::process::exit(1); | ||
| } | ||
| }, |
There was a problem hiding this comment.
The error message from serde_json::to_string is printed directly to stderr without sanitization. To prevent potential terminal escape sequence injection from untrusted input contained within the error details, the error string must be sanitized using crate::output::sanitize_for_terminal before printing.
| OutputFormat::Json => match serde_json::to_string(value) { | |
| Ok(s) => s, | |
| Err(e) => { | |
| eprintln!("error: failed to serialize response to JSON: {e}"); | |
| std::process::exit(1); | |
| } | |
| }, | |
| OutputFormat::Json => match serde_json::to_string(value) { | |
| Ok(s) => s, | |
| Err(e) => { | |
| let err_msg = crate::output::sanitize_for_terminal(&e.to_string()); | |
| eprintln!("error: failed to serialize response to JSON: {err_msg}"); | |
| std::process::exit(1); | |
| } | |
| }, |
References
- Sanitize error strings printed to the terminal to prevent escape sequence injection.
There was a problem hiding this comment.
Fixed in 41f8a6f — same treatment as the sibling in \ ormat_value: the full error string is passed through \crate::output::sanitize_for_terminal\ before \eprintln!, covering both call sites.
There was a problem hiding this comment.
Same as the sibling comment — with b069a27 the error is now propagated via Result instead of printed, so the terminal sanitization concern is moot at this call site.
…jection Wrap the serialization error string with sanitize_for_terminal before printing to stderr, as the error text could theoretically contain user-controlled data with embedded escape sequences.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the JSON formatting functions (format_value and format_value_paginated) to print an error message to stderr and exit with a non-zero code when JSON serialization fails, rather than returning an empty string. The reviewer correctly identified that calling std::process::exit(1) inside these formatting utility functions is an anti-pattern because it bypasses RAII cleanup, makes the code untestable for error paths, and tightly couples formatting logic to the process lifecycle. The reviewer recommends refactoring both functions to return a Result instead, propagating the error to the caller.
| pub fn format_value(value: &Value, format: &OutputFormat) -> String { | ||
| match format { | ||
| OutputFormat::Json => serde_json::to_string_pretty(value).unwrap_or_default(), | ||
| OutputFormat::Json => match serde_json::to_string_pretty(value) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| eprintln!("{}", crate::output::sanitize_for_terminal(&format!("error: failed to serialize response to JSON: {e}"))); | ||
| std::process::exit(1); | ||
| } | ||
| }, | ||
| OutputFormat::Table => format_table(value), | ||
| OutputFormat::Yaml => format_yaml(value), | ||
| OutputFormat::Csv => format_csv(value), |
There was a problem hiding this comment.
Calling std::process::exit(1) inside a formatting utility function is a major anti-pattern in Rust.
Why this is an issue:
- Bypasses Cleanup (RAII):
std::process::exitimmediately terminates the process without running destructors (Drop) for any active objects on the stack. This can lead to resource leaks or incomplete state flushes. - Untestable: Any unit test that triggers this error path (e.g., trying to format an invalid JSON value containing
NaNorInfinity) will immediately terminate the entire test runner process, making it impossible to write proper assertions for error handling. - Tight Coupling: It tightly couples a pure formatting/presentation function to the process lifecycle, preventing the formatting module from being reused in other contexts (like a GUI, a daemon, or as a library crate) where exiting the process on a serialization error is unacceptable.
Recommendation:
Change the signature of format_value to return a Result<String, serde_json::Error>. This propagates the error to the caller (presumably the CLI command handler or main function), which can then print the sanitized error message and exit cleanly.
pub fn format_value(value: &Value, format: &OutputFormat) -> Result<String, serde_json::Error> {
match format {
OutputFormat::Json => serde_json::to_string_pretty(value),
OutputFormat::Table => Ok(format_table(value)),
OutputFormat::Yaml => Ok(format_yaml(value)),
OutputFormat::Csv => Ok(format_csv(value)),There was a problem hiding this comment.
Fixed in b069a27. Both format_value and format_value_paginated now return Result<String, serde_json::Error> — the match/exit pattern is gone. All callers propagate the error via .map_err(|e| GwsError::Other(anyhow::Error::from(e)))? so the cleanup happens at the right level.
| pub fn format_value_paginated(value: &Value, format: &OutputFormat, is_first_page: bool) -> String { | ||
| match format { | ||
| OutputFormat::Json => serde_json::to_string(value).unwrap_or_default(), | ||
| OutputFormat::Json => match serde_json::to_string(value) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| eprintln!("{}", crate::output::sanitize_for_terminal(&format!("error: failed to serialize response to JSON: {e}"))); | ||
| std::process::exit(1); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Just like in format_value, calling std::process::exit(1) inside format_value_paginated is an anti-pattern that prevents proper RAII cleanup, makes the function untestable for error cases, and tightly couples formatting logic to the process lifecycle.
Recommendation:
Propagate the error by changing the signature of format_value_paginated to return a Result<String, serde_json::Error>.
Since some arms of this function are outside the current diff hunk, a direct code suggestion cannot be applied automatically. Please update the function manually as follows:
pub fn format_value_paginated(value: &Value, format: &OutputFormat, is_first_page: bool) -> Result<String, serde_json::Error> {
match format {
OutputFormat::Json => serde_json::to_string(value),
OutputFormat::Csv => Ok(format_csv_page(value, is_first_page)),
OutputFormat::Table => Ok(format_table_page(value, is_first_page)),
// Prefix every page with a YAML document separator so that the
// concatenated stream is parseable as a multi-document YAML file.
OutputFormat::Yaml => Ok(format!("---\n{}", format_yaml(value))),
}
}There was a problem hiding this comment.
Fixed in b069a27. format_value_paginated now returns Result<String, serde_json::Error> too, with the same approach: serde_json::to_string(value) in the Json arm, Ok(...) for all other arms.
Change format_value and format_value_paginated to return Result<String, serde_json::Error> so that serialization failures propagate to callers rather than aborting the process mid-flight. All call sites now use .map_err(|e| GwsError::Other(...))? or (in format_and_print) return Result<(), GwsError>. This fixes the RAII bypass and makes both functions testable for error paths without killing the test runner. Resolves Gemini r2 comment on PR googleworkspace#828.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the CLI to exit with a non-zero code and a clear error message when JSON serialization fails, changing the formatting functions to return a Result and propagating these errors across the codebase. While the error handling is a great improvement, several UTF-8 characters—including table separators, ellipses, emojis, and accented test strings—were corrupted into Mojibake during the changes and must be restored to prevent broken terminal layouts and garbled output.
|
|
||
| // Separator | ||
| let sep: Vec<String> = widths.iter().map(|w| "─".repeat(*w)).collect(); | ||
| let sep: Vec<String> = widths.iter().map(|w| "─".repeat(*w)).collect(); |
There was a problem hiding this comment.
The table separator character has been corrupted into Mojibake (─ instead of ─). This will break the visual layout of tables printed to the terminal. Please restore the original box-drawing character ─.
| let sep: Vec<String> = widths.iter().map(|w| "─".repeat(*w)).collect(); | |
| let sep: Vec<String> = widths.iter().map(|w| "─".repeat(*w)).collect(); |
| // Safe char-boundary slice: take widths[i]-1 chars, then append ellipsis. | ||
| let truncated_str: String = c.chars().take(widths[i] - 1).collect(); | ||
| format!("{truncated_str}…") | ||
| format!("{truncated_str}…") |
There was a problem hiding this comment.
The ellipsis character has been corrupted into Mojibake (… instead of …). This will cause truncated table cells to display garbled characters in the terminal. Please restore the original … character.
| format!("{truncated_str}…") | |
| format!("{truncated_str}…") |
| // Column width cap is 60 chars, so a long string with multi-byte chars | ||
| // must be safely truncated without a byte-boundary panic. | ||
| let long_emoji = "😀".repeat(70); // each emoji is 4 bytes | ||
| let long_emoji = "😀".repeat(70); // each emoji is 4 bytes |
There was a problem hiding this comment.
The emoji in this test has been corrupted into Mojibake (😀 instead of 😀). Please restore the original grinning face emoji.
| let long_emoji = "😀".repeat(70); // each emoji is 4 bytes | |
| let long_emoji = "😀".repeat(70); // each emoji is 4 bytes |
| // Multi-byte chars at various positions must not panic or produce garbled output. | ||
| let val = json!([{"name": "café résumé naïve"}]); | ||
| let output = format_value(&val, &OutputFormat::Table); | ||
| let val = json!([{"name": "café résumé naïve"}]); |
There was a problem hiding this comment.
The test string has been corrupted into Mojibake (café résumé naïve instead of café résumé naïve). Please restore the original UTF-8 characters.
| let val = json!([{"name": "café résumé naïve"}]); | |
| let val = json!([{"name": "café résumé naïve"}]); |
2 participants
Summary
When
serde_json::to_string_prettyfailed (e.g. on values containing NaN/Infinity),format_valuereturned an empty string viaunwrap_or_default(). The CLI then printed nothing and exited 0, silently swallowing the API response.This change replaces the silent fallback with an explicit stderr error message and a non-zero exit code. The same fix is applied to
format_value_paginatedwhich had the identical issue.Fixes #740
Changes
format_value: replacedunwrap_or_default()with an explicitmatchthat printserror: failed to serialize response to JSON: {e}to stderr and exits with code 1format_value_paginated: same fix for the compact JSON pathtest_format_value_json_non_emptytest to assert valid values always produce non-empty outputChecklist
cargo fmt --allpassedcargo clippy -- -D warningspassedcargo testpassed