fix(executor): warn to stderr when 2xx response has empty body#752
fix(executor): warn to stderr when 2xx response has empty body#752hoyt-harness wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
When execute_method() receives a 2xx HTTP response but the body is empty, it previously fell through to handle_json_response() which silently discarded the input and returned Ok(false) — no output, no error, exit 0. Add a guard before the handle_json_response() call: if body_text is empty, print a Warning to stderr identifying the method and status code, then break out of the pagination loop. The exit code remains 0 because the HTTP call itself succeeded; the warning is the diagnostic for silent responses. Also add two unit tests for handle_json_response() covering the empty-body and valid-JSON paths, and collapse a pre-existing clippy::collapsible_match in helpers/script.rs. Fixes googleworkspace#740 (sub-issue: empty-body diagnostic)
🦋 Changeset detectedLatest commit: 612fcd5 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 a silent failure issue where successful API calls with empty response bodies provided no feedback to the user. By implementing a diagnostic warning, users are now alerted to these cases via stderr, ensuring better transparency without breaking existing output contracts. Additionally, the change includes regression testing and minor code quality improvements. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a diagnostic warning for successful API calls that return an empty response body. The execute_method function now checks for empty bodies, logs a warning to stderr, and breaks the execution loop. New unit tests verify this logic and ensure correct pagination behavior. Additionally, a match arm in script.rs was refactored for better readability using a match guard. I have no feedback to provide.
Problem
When an API call succeeds (2xx HTTP status) but returns an empty response
body,
execute_method()silently discards the response: no output, nowarning, exit 0. Users see nothing and have no indication of what happened.
This is tracked in #740 (sub-issue: empty-body diagnostic). The companion
fix for the
+read --formatglobal flag collision was submitted in #751.Root cause
In
execute_method(), after readingbody_text,handle_json_response()is called unconditionally. When
body_textis empty,serde_json::from_strfails, the else branch runs, and the condition
!capture_output && !body_text.is_empty()is false — nothing is printed,no error is returned, and the function exits with
Ok(None).Fix
Add a guard in
execute_method()between readingbody_textand callinghandle_json_response(): ifbody_textis empty on a 2xx response, printa
Warning:message to stderr (including the method ID and HTTP status) andbreak out of the pagination loop.
The exit code remains 0 because the HTTP call itself succeeded. A non-zero
exit was considered, but
GwsError::Otherwould also write{"error": {"code": 500, "reason": "internalError"}}to stdout, which ismisleading for a 200 OK response and breaks the stdout=data contract.
Scripts needing automated detection of empty-body responses should check
stderr for the
Warning:prefix.Tests
Two new unit tests for
handle_json_response():test_handle_json_response_empty_body_returns_no_continue— verifiesempty body returns
Ok(false)without panicking and does not incrementpages_fetched. This covers the underlying function behavior (the newguard in
execute_method()prevents it from being called with an emptybody in practice, but the function handles it gracefully as a fallback).
test_handle_json_response_valid_json_increments_pages— regression testfor the normal JSON path.
Testing the
execute_method()guard end-to-end would require a mock HTTPserver; there is no such infrastructure in this crate currently.
Other changes
Collapses a pre-existing
clippy::collapsible_matchinhelpers/script.rs(same warning present onupstream/main).