Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions crates/google-workspace-cli/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ mod tests {
message: "bad".to_string(),
reason: "r".to_string(),
enable_url: None,
retry_after: None,
};
let label = error_label(&api_err);
assert!(label.contains("error[api]:"));
Expand Down
41 changes: 40 additions & 1 deletion crates/google-workspace-cli/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,11 @@ pub async fn execute_method(
.and_then(|v| v.to_str().ok())
.unwrap_or("")
.to_string();
let retry_after = response
.headers()
.get(reqwest::header::RETRY_AFTER)
.and_then(|v| v.to_str().ok())
.map(str::to_string);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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


if !status.is_success() {
let error_body = response.text().await.unwrap_or_default();
Expand All @@ -472,7 +477,7 @@ pub async fn execute_method(
latency_ms = latency_ms,
"API error"
);
return handle_error_response(status, &error_body, &auth_method);
return handle_error_response(status, &error_body, &auth_method, retry_after);
}

tracing::debug!(
Expand Down Expand Up @@ -754,6 +759,7 @@ fn handle_error_response<T>(
status: reqwest::StatusCode,
error_body: &str,
auth_method: &AuthMethod,
retry_after: Option<String>,
) -> Result<T, GwsError> {
// If 401/403 and no auth was provided, give a helpful message
if (status.as_u16() == 401 || status.as_u16() == 403) && *auth_method == AuthMethod::None {
Expand Down Expand Up @@ -800,6 +806,7 @@ fn handle_error_response<T>(
message,
reason,
enable_url,
retry_after,
});
}
}
Expand All @@ -809,6 +816,7 @@ fn handle_error_response<T>(
message: error_body.to_string(),
reason: "httpError".to_string(),
enable_url: None,
retry_after,
})
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Add the shared helper function here to be used by other modules.

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


Expand Down Expand Up @@ -1947,6 +1955,7 @@ mod tests {
reqwest::StatusCode::UNAUTHORIZED,
"Unauthorized",
&AuthMethod::None,
None,
)
.unwrap_err();
match err {
Expand All @@ -1973,6 +1982,7 @@ mod tests {
reqwest::StatusCode::UNAUTHORIZED,
&json_err,
&AuthMethod::OAuth,
None,
)
.unwrap_err();
match err {
Expand Down Expand Up @@ -2008,6 +2018,7 @@ mod tests {
reqwest::StatusCode::BAD_REQUEST,
&json_err,
&AuthMethod::OAuth,
None,
)
.unwrap_err();
match err {
Expand All @@ -2024,6 +2035,31 @@ mod tests {
_ => panic!("Expected Api error"),
}
}

#[test]
fn test_handle_error_response_preserves_retry_after_header() {
let json_err = json!({
"error": {
"code": 429,
"message": "Quota exceeded",
"errors": [{ "reason": "rateLimitExceeded" }]
}
})
.to_string();

let err = handle_error_response::<()>(
reqwest::StatusCode::TOO_MANY_REQUESTS,
&json_err,
&AuthMethod::OAuth,
Some("120".to_string()),
)
.unwrap_err();

match err {
GwsError::Api { retry_after, .. } => assert_eq!(retry_after.as_deref(), Some("120")),
_ => panic!("Expected Api error"),
}
}
}

#[tokio::test]
Expand Down Expand Up @@ -2156,6 +2192,7 @@ fn test_handle_error_response_non_json() {
reqwest::StatusCode::INTERNAL_SERVER_ERROR,
"Internal Server Error Text",
&AuthMethod::OAuth,
None,
)
.unwrap_err();
match err {
Expand Down Expand Up @@ -2223,6 +2260,7 @@ fn test_handle_error_response_access_not_configured_with_url() {
reqwest::StatusCode::FORBIDDEN,
&json_err,
&AuthMethod::OAuth,
None,
)
.unwrap_err();

Expand Down Expand Up @@ -2260,6 +2298,7 @@ fn test_handle_error_response_access_not_configured_errors_array() {
reqwest::StatusCode::FORBIDDEN,
&json_err,
&AuthMethod::OAuth,
None,
)
.unwrap_err();

Expand Down
1 change: 1 addition & 0 deletions crates/google-workspace-cli/src/helpers/calendar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ async fn handle_agenda(matches: &ArgMatches) -> Result<(), GwsError> {
message: err,
reason: "calendarList_failed".to_string(),
enable_url: None,
retry_after: None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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
  1. Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.

});
}

Expand Down
3 changes: 3 additions & 0 deletions crates/google-workspace-cli/src/helpers/events/subscribe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ pub(super) async fn handle_subscribe(
message: format!("Failed to create Pub/Sub topic: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after: None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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.

});
}

Expand All @@ -246,6 +247,7 @@ pub(super) async fn handle_subscribe(
message: format!("Failed to create Pub/Sub subscription: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after: None,
});
}

Expand Down Expand Up @@ -421,6 +423,7 @@ async fn pull_loop(
message: format!("Pub/Sub pull failed: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after: None,
});
}

Expand Down
48 changes: 42 additions & 6 deletions crates/google-workspace-cli/src/helpers/gmail/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ pub(super) async fn fetch_message_metadata(

if !resp.status().is_success() {
let status = resp.status().as_u16();
let retry_after = retry_after_header(&resp);
let body = resp
.text()
.await
Expand All @@ -393,6 +394,7 @@ pub(super) async fn fetch_message_metadata(
status,
&body,
&format!("Failed to fetch message {message_id}"),
retry_after,
));
}

Expand All @@ -407,7 +409,12 @@ pub(super) async fn fetch_message_metadata(
/// Build a `GwsError::Api` from an HTTP error response body, parsing the
/// Google JSON error format when possible. Modeled after the executor's
/// `handle_error_response`, extracting message, reason, and enable URL.
pub(super) fn build_api_error(status: u16, body: &str, context: &str) -> GwsError {
pub(super) fn build_api_error(
status: u16,
body: &str,
context: &str,
retry_after: Option<String>,
) -> GwsError {
let err_json: Option<Value> = serde_json::from_str(body).ok();
let err_obj = err_json.as_ref().and_then(|v| v.get("error"));
let message = err_obj
Expand Down Expand Up @@ -438,9 +445,17 @@ pub(super) fn build_api_error(status: u16, body: &str, context: &str) -> GwsErro
message: format!("{context}: {message}"),
reason,
enable_url,
retry_after,
}
}

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)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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;


#[derive(Debug)]
struct SendAsIdentity {
mailbox: Mailbox,
Expand All @@ -462,6 +477,7 @@ async fn fetch_send_as_identities(

if !resp.status().is_success() {
let status = resp.status().as_u16();
let retry_after = retry_after_header(&resp);
let body = resp
.text()
.await
Expand All @@ -470,6 +486,7 @@ async fn fetch_send_as_identities(
status,
&body,
"Failed to fetch sendAs settings",
retry_after,
));
}

Expand Down Expand Up @@ -656,11 +673,17 @@ async fn fetch_profile_display_name(

if !resp.status().is_success() {
let status = resp.status().as_u16();
let retry_after = retry_after_header(&resp);
let body = resp
.text()
.await
.unwrap_or_else(|_| "(error body unreadable)".to_string());
return Err(build_api_error(status, &body, "People API request failed"));
return Err(build_api_error(
status,
&body,
"People API request failed",
retry_after,
));
}

let body: Value = resp.json().await.map_err(|e| {
Expand Down Expand Up @@ -703,6 +726,7 @@ async fn fetch_attachment_data(

if !resp.status().is_success() {
let status = resp.status().as_u16();
let retry_after = retry_after_header(&resp);
let err = resp
.text()
.await
Expand All @@ -711,6 +735,7 @@ async fn fetch_attachment_data(
status,
&err,
&format!("Failed to fetch attachment {attachment_id} from message {message_id}"),
retry_after,
));
}

Expand Down Expand Up @@ -3607,13 +3632,14 @@ mod tests {
#[test]
fn test_build_api_error_parses_google_json_format() {
let body = r#"{"error":{"code":403,"message":"Insufficient Permission","errors":[{"reason":"insufficientPermissions","domain":"global","message":"Insufficient Permission"}]}}"#;
let err = build_api_error(403, body, "Test context");
let err = build_api_error(403, body, "Test context", None);
match err {
GwsError::Api {
code,
message,
reason,
enable_url,
..
} => {
assert_eq!(code, 403);
assert!(message.contains("Test context"));
Expand All @@ -3627,7 +3653,7 @@ mod tests {

#[test]
fn test_build_api_error_falls_back_to_raw_body() {
let err = build_api_error(500, "Internal Server Error", "Test context");
let err = build_api_error(500, "Internal Server Error", "Test context", None);
match err {
GwsError::Api {
code,
Expand All @@ -3643,10 +3669,20 @@ mod tests {
}
}

#[test]
fn test_build_api_error_preserves_retry_after() {
let body = r#"{"error":{"code":429,"message":"Quota exceeded","errors":[{"reason":"rateLimitExceeded"}]}}"#;
let err = build_api_error(429, body, "ctx", Some("60".to_string()));
match err {
GwsError::Api { retry_after, .. } => assert_eq!(retry_after.as_deref(), Some("60")),
_ => panic!("Expected GwsError::Api"),
}
}

#[test]
fn test_build_api_error_extracts_top_level_reason() {
let body = r#"{"error":{"code":404,"message":"Not Found","reason":"notFound"}}"#;
let err = build_api_error(404, body, "ctx");
let err = build_api_error(404, body, "ctx", None);
match err {
GwsError::Api { reason, .. } => assert_eq!(reason, "notFound"),
_ => panic!("Expected GwsError::Api"),
Expand All @@ -3656,7 +3692,7 @@ mod tests {
#[test]
fn test_build_api_error_access_not_configured_extracts_url() {
let body = r#"{"error":{"code":403,"message":"People API has not been used in project 123 before or it is disabled. Enable it by visiting https://console.developers.google.com/apis/api/people.googleapis.com/overview?project=123 then retry.","errors":[{"reason":"accessNotConfigured"}]}}"#;
let err = build_api_error(403, body, "ctx");
let err = build_api_error(403, body, "ctx", None);
match err {
GwsError::Api {
reason, enable_url, ..
Expand Down
2 changes: 2 additions & 0 deletions crates/google-workspace-cli/src/helpers/gmail/reply.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ async fn fetch_user_email(client: &reqwest::Client, token: &str) -> Result<Strin

if !resp.status().is_success() {
let status = resp.status().as_u16();
let retry_after = super::retry_after_header(&resp);
let body = resp
.text()
.await
Expand All @@ -193,6 +194,7 @@ async fn fetch_user_email(client: &reqwest::Client, token: &str) -> Result<Strin
status,
&body,
"Failed to fetch user profile",
retry_after,
));
}

Expand Down
1 change: 1 addition & 0 deletions crates/google-workspace-cli/src/helpers/gmail/triage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pub async fn handle_triage(matches: &ArgMatches) -> Result<(), GwsError> {
message: err,
reason: "list_failed".to_string(),
enable_url: None,
retry_after: None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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.

});
}

Expand Down
4 changes: 4 additions & 0 deletions crates/google-workspace-cli/src/helpers/gmail/watch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ pub(super) async fn handle_watch(
message: format!("Failed to create Pub/Sub topic: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after: None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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.

});
}

Expand Down Expand Up @@ -132,6 +133,7 @@ pub(super) async fn handle_watch(
message: format!("Failed to create Pub/Sub subscription: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after: None,
});
}

Expand Down Expand Up @@ -168,6 +170,7 @@ pub(super) async fn handle_watch(
),
reason: "gmailError".to_string(),
enable_url: None,
retry_after: None,
});
}

Expand Down Expand Up @@ -301,6 +304,7 @@ async fn watch_pull_loop(
message: format!("Pub/Sub pull failed: {body}"),
reason: "pubsubError".to_string(),
enable_url: None,
retry_after: None,
});
}

Expand Down
3 changes: 3 additions & 0 deletions crates/google-workspace-cli/src/helpers/workflows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ async fn get_json(
message: body,
reason: "workflow_request_failed".to_string(),
enable_url: None,
retry_after: None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

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.

});
}

Expand Down Expand Up @@ -517,6 +518,7 @@ async fn handle_email_to_task(matches: &ArgMatches) -> Result<(), GwsError> {
message: body,
reason: "task_create_failed".to_string(),
enable_url: None,
retry_after: None,
});
}

Expand Down Expand Up @@ -676,6 +678,7 @@ async fn handle_file_announce(matches: &ArgMatches) -> Result<(), GwsError> {
message: body,
reason: "chat_send_failed".to_string(),
enable_url: None,
retry_after: None,
});
}

Expand Down
Loading
Loading