fix(errors): include retry-after in api errors#802
Conversation
|
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 improves the error handling mechanism for API interactions by capturing and propagating the 'Retry-After' header. This allows consumers of the CLI to better understand rate-limiting scenarios and respond appropriately, as the header is now included in the structured JSON error output. 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
area: http
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 introduces support for capturing the Retry-After header from Google API responses by adding a retry_after field to the GwsError::Api error variant. The changes include updating the HTTP executor to extract the header and modifying various CLI helpers to propagate this information. Feedback suggests that the build_api_error function in the Gmail module should be updated to accept an optional retry_after string instead of hardcoding it to None, ensuring the header is correctly captured in rate-limiting scenarios within that module.
| message: format!("{context}: {message}"), | ||
| reason, | ||
| enable_url, | ||
| retry_after: None, |
There was a problem hiding this comment.
The build_api_error function hardcodes retry_after to None, but it is used in contexts where the HTTP response is available (e.g., fetch_message_metadata at line 386). This omission prevents the Retry-After header from being captured and reported for rate-limited Gmail helper commands, which is the primary goal of this pull request. Please update build_api_error to accept an optional retry_after string and update its callers to pass the header from the response.
References
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
There was a problem hiding this comment.
Done in ba21290. build_api_error now accepts retry_after, and the Gmail helper/profile/attachment call sites capture the response header before consuming the body. Added test_build_api_error_preserves_retry_after for the regression.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for the Retry-After header by introducing a retry_after field to the GwsError::Api variant and updating error handling across the google-workspace and google-workspace-cli crates. Feedback identifies opportunities to reduce code duplication by refactoring header extraction into a shared utility. Additionally, the reviewer noted an inconsistency in the calendar helper where the header is not captured and a status code is hardcoded to zero.
| let retry_after = response | ||
| .headers() | ||
| .get(reqwest::header::RETRY_AFTER) | ||
| .and_then(|v| v.to_str().ok()) | ||
| .map(str::to_string); |
There was a problem hiding this comment.
The logic for extracting the Retry-After header is duplicated here and in crates/google-workspace-cli/src/helpers/gmail/mod.rs. It should be refactored into a shared helper function to ensure consistency and reduce duplication.
let retry_after = retry_after_header(&response);| enable_url: None, | ||
| retry_after, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add the shared helper function here to be used by other modules.
| } | |
| } | |
| pub fn retry_after_header(resp: &reqwest::Response) -> Option<String> { | |
| resp.headers() | |
| .get(reqwest::header::RETRY_AFTER) | |
| .and_then(|v| v.to_str().ok()) | |
| .map(str::to_string) | |
| } |
| pub(super) fn retry_after_header(resp: &reqwest::Response) -> Option<String> { | ||
| resp.headers() | ||
| .get(reqwest::header::RETRY_AFTER) | ||
| .and_then(|value| value.to_str().ok()) | ||
| .map(str::to_string) | ||
| } |
There was a problem hiding this comment.
This helper function is now duplicated. It should be removed in favor of the shared version in crate::executor.
pub(super) use crate::executor::retry_after_header;| message: err, | ||
| reason: "calendarList_failed".to_string(), | ||
| enable_url: None, | ||
| retry_after: None, |
There was a problem hiding this comment.
The Retry-After header is not being captured here, which is inconsistent with the PR's objective. It should be extracted from list_resp before the response body is consumed by .text().await. Additionally, the hardcoded code: 0 (on line 273) should be replaced with the actual HTTP status code from list_resp.status().
References
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for the Retry-After header in API error responses by updating the GwsError::Api variant and providing a helper function to extract the header from responses. While the core logic and some helpers now correctly propagate this information, feedback highlights several files—including subscribe.rs, triage.rs, watch.rs, workflows.rs, and timezone.rs—where the header is still hardcoded to None in error paths, preventing the capture of rate-limiting details.
| message: format!("Failed to create Pub/Sub topic: {body}"), | ||
| reason: "pubsubError".to_string(), | ||
| enable_url: None, | ||
| retry_after: None, |
There was a problem hiding this comment.
The Retry-After header is hardcoded to None here and in other error paths in this file (lines 251, 427). To fulfill the PR's objective of including retry information in API errors, the header should be captured from the response before the body is consumed, using the retry_after_header helper.
| message: err, | ||
| reason: "list_failed".to_string(), | ||
| enable_url: None, | ||
| retry_after: None, |
There was a problem hiding this comment.
The Retry-After header is not being captured from the response in this error path. It should be extracted using retry_after_header(&list_resp) before the response body is consumed at line 62, ensuring that rate-limiting information is preserved.
| message: format!("Failed to create Pub/Sub topic: {body}"), | ||
| reason: "pubsubError".to_string(), | ||
| enable_url: None, | ||
| retry_after: None, |
There was a problem hiding this comment.
The Retry-After header is hardcoded to None here and in several other error paths in this file (lines 137, 173, 308). It should be captured from the resp object before it is consumed using the retry_after_header helper.
| message: body, | ||
| reason: "workflow_request_failed".to_string(), | ||
| enable_url: None, | ||
| retry_after: None, |
There was a problem hiding this comment.
The Retry-After header should be captured from resp before it is consumed at line 248. Hardcoding it to None misses the opportunity to provide retry guidance for workflow-related API failures.
| reason: "timezone_fetch_failed".to_string(), | ||
| enable_url: None, | ||
| retry_after: None, | ||
| }); |
There was a problem hiding this comment.
The Retry-After header is not being captured from the response. It should be extracted using crate::executor::retry_after_header(&resp) before the response body is consumed at line 89.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the Retry-After HTTP header across various API interactions within the Google Workspace CLI. It updates the GwsError::Api error variant to include a retry_after field and implements a helper function to extract this header from responses. Error handling logic in several modules, including Gmail, Calendar, and Workflows, has been updated to propagate this information. Feedback was provided regarding watch.rs, where the response status should be checked before parsing the JSON body to ensure the Retry-After header is preserved even if the body is malformed.
| .await | ||
| .context("Failed to call gmail.users.watch")?; | ||
|
|
||
| let retry_after = super::retry_after_header(&resp); |
There was a problem hiding this comment.
The response status should be checked before attempting to parse the body as JSON. If the API returns a non-success status (e.g., 429 Too Many Requests) with a non-JSON body, resp.json().await will fail, resulting in a generic error that loses the Retry-After header and the actual HTTP status code. Consider checking status.is_success() and using super::build_api_error to ensure consistent error reporting and preservation of the retry_after value, similar to the pattern used in executor.rs or reply.rs.
2 participants
Fixes #777.
Summary
Retry-Afterresponse header before consuming error responsesGwsError::Apierror.retry_afterin JSON output when the API provides the headerTests
cargo fmt --checkcargo test -p google-workspace test_error_to_json_api_includes_retry_aftercargo test -p google-workspace-cli test_handle_error_response_preserves_retry_after_header