fix: fail loudly when --output receives JSON#732
fix: fail loudly when --output receives JSON#732lawrence3699 wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 3316a07 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 the CLI would silently succeed without writing any data when the --output flag was used against endpoints returning JSON instead of raw binary data. The changes ensure that the application now explicitly validates the response type and fails with a descriptive error message if a user attempts to use --output with a JSON response. 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 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. Footnotes
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![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 introduces a validation error when the --output flag is used with JSON responses, preventing the tool from silently failing to write a file. While the change includes a new test case, feedback highlights that the current implementation is inefficient because it reads and parses the entire response body before performing the check. The reviewer suggests refactoring the logic to fail earlier based on the Content-Type header and ensuring that non-JSON responses are properly handled when an output path is provided.
| if output_path.is_some() { | ||
| return Err(GwsError::Validation( | ||
| "--output only supports raw binary responses; this endpoint returned JSON instead. Use jq/base64 to extract the payload or rerun without --output.".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
This check occurs after the entire response body has been read into memory and parsed as JSON. This is inefficient for large JSON responses when the Content-Type header already indicates it's JSON.
Furthermore, if the response is NOT valid JSON (e.g., plain text or binary with a missing Content-Type), the function falls through to the else block (line 336) and prints to stdout. If --output was specified, this results in the output being dumped to the terminal instead of the file, which is likely not what the user intended.
Consider refactoring execute_method to fail early if is_json && output_path.is_some(), and to use handle_binary_response if content_type.is_empty() && output_path.is_some(). Please ensure these changes are focused on the primary goal to avoid scope creep, and acknowledge potential TOCTOU race conditions in file path validation as a known limitation.
References
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
- When implementing file path validation, acknowledge and document potential Time-of-check to time-of-use (TOCTOU) race conditions as a known limitation if a full mitigation (e.g., using
openat(O_NOFOLLOW)) is considered out of scope.
2 participants
Description
Fixes #714.
--outputis documented for raw binary downloads, but JSON endpoints likegmail.users.messages.attachments.getcurrently exit 0 without writing anything. This change makes that path fail loudly instead of silently succeeding.Before:
gws ... --output fileon a JSON response printed JSON or nothing and left no file behind.After:
--outputreturns a non-zero validation error when the endpoint responds with JSON instead of raw binary data.execute_method()against a JSON response with--outputset.Dry Run Output:
N/AChecklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.