Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions crates/google-workspace-cli/src/helpers/gmail/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,7 @@ pub(super) fn build_api_error(status: u16, body: &str, context: &str) -> GwsErro
message: format!("{context}: {message}"),
reason,
enable_url,
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 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
  1. Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

}
}

Expand Down Expand Up @@ -3614,6 +3615,7 @@ mod tests {
message,
reason,
enable_url,
..
} => {
assert_eq!(code, 403);
assert!(message.contains("Test context"));
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
1 change: 1 addition & 0 deletions crates/google-workspace-cli/src/timezone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ async fn fetch_account_timezone(client: &reqwest::Client, token: &str) -> Result
message: body,
reason: "timezone_fetch_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. It should be extracted using crate::executor::retry_after_header(&resp) before the response body is consumed at line 89.

}

Expand Down
23 changes: 23 additions & 0 deletions crates/google-workspace/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ pub enum GwsError {
reason: String,
/// For `accessNotConfigured` errors: the GCP console URL to enable the API.
enable_url: Option<String>,
/// Raw Retry-After response header, when provided by the API.
retry_after: Option<String>,
},

#[error("{0}")]
Expand Down Expand Up @@ -71,6 +73,7 @@ impl GwsError {
message,
reason,
enable_url,
retry_after,
} => {
let mut error_obj = json!({
"code": code,
Expand All @@ -80,6 +83,9 @@ impl GwsError {
if let Some(url) = enable_url {
error_obj["enable_url"] = json!(url);
}
if let Some(value) = retry_after {
error_obj["retry_after"] = json!(value);
}
json!({ "error": error_obj })
}
GwsError::Validation(msg) => json!({
Expand Down Expand Up @@ -125,6 +131,7 @@ mod tests {
message: "Not Found".to_string(),
reason: "notFound".to_string(),
enable_url: None,
retry_after: None,
};
assert_eq!(err.exit_code(), GwsError::EXIT_CODE_API);
}
Expand Down Expand Up @@ -185,6 +192,7 @@ mod tests {
message: "Not Found".to_string(),
reason: "notFound".to_string(),
enable_url: None,
retry_after: None,
};
let json = err.to_json();
assert_eq!(json["error"]["code"], 404);
Expand All @@ -193,6 +201,19 @@ mod tests {
assert!(json["error"]["enable_url"].is_null());
}

#[test]
fn test_error_to_json_api_includes_retry_after() {
let err = GwsError::Api {
code: 429,
message: "Quota exceeded".to_string(),
reason: "rateLimitExceeded".to_string(),
enable_url: None,
retry_after: Some("120".to_string()),
};
let json = err.to_json();
assert_eq!(json["error"]["retry_after"], "120");
}

#[test]
fn test_error_to_json_validation() {
let err = GwsError::Validation("Invalid input".to_string());
Expand Down Expand Up @@ -236,6 +257,7 @@ mod tests {
message: "Gmail API has not been used in project 549352339482 before or it is disabled.".to_string(),
reason: "accessNotConfigured".to_string(),
enable_url: Some("https://console.developers.google.com/apis/api/gmail.googleapis.com/overview?project=549352339482".to_string()),
retry_after: None,
};
let json = err.to_json();
assert_eq!(json["error"]["code"], 403);
Expand All @@ -253,6 +275,7 @@ mod tests {
message: "API not enabled.".to_string(),
reason: "accessNotConfigured".to_string(),
enable_url: None,
retry_after: None,
};
let json = err.to_json();
assert_eq!(json["error"]["code"], 403);
Expand Down
Loading