feat(docs): add +revisions helper to list document revision history#563
feat(docs): add +revisions helper to list document revision history#563khali wants to merge 11 commits intogoogleworkspace:mainfrom
Conversation
Adds a new `gws docs +revisions` helper command that lists revision metadata for a Google Docs document by calling the Drive v3 revisions API directly (drive.readonly scope). Flags: --document (required) Document ID --limit (optional, default 20) Number of revisions to return Each revision entry includes: id, modifiedTime, lastModifyingUser, keepForever, and size. A clear note documents the Google API limitation: content of past revisions is not accessible for native Docs files. Also adds the gws-docs-revisions SKILL.md and updates gws-docs SKILL.md to reference the new helper.
|
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. |
🦋 Changeset detectedLatest commit: 000b993 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 |
googleworkspace-bot
added
area: skills
area: core
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 enhances the Google Workspace CLI by adding a new helper command for Google Docs. The primary goal is to provide users with the ability to quickly retrieve the revision history metadata of any Google Docs document directly from the command line. This improves the utility of the 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
src/helpers/docs.rs
Outdated
| let scope = "https://www.googleapis.com/auth/drive.readonly"; | ||
| let token = auth::get_token(&[scope]) | ||
| .await | ||
| .map_err(|e| GwsError::Auth(format!("Docs auth failed: {e}")))?; |
There was a problem hiding this comment.
The error string Docs auth failed: {e} is printed to the terminal without sanitization. This could allow escape sequence injection if the underlying error e contains malicious escape sequences. Sanitize the error string before printing it to the terminal.
| .map_err(|e| GwsError::Auth(format!("Docs auth failed: {e}")))?; | |
| .map_err(|e| GwsError::Auth(format!("Docs auth failed: {}", crate::output::sanitize_for_terminal(&e.to_string())))? |
References
- Sanitize error strings printed to the terminal to prevent escape sequence injection.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new gws docs +revisions helper command to list revision history for a Google Docs document. The implementation correctly uses reqwest to call the Google Drive API directly and includes new documentation and tests. My review focuses on improving input validation and code maintainability. I've identified a missing validation for the --limit parameter and suggested refactoring a hardcoded string into a constant to improve readability and prevent potential errors.
|
|
||
| async fn handle_revisions(matches: &ArgMatches) -> Result<(), GwsError> { | ||
| let document_id = matches.get_one::<String>("document").unwrap(); | ||
| let limit = matches.get_one::<u32>("limit").copied().unwrap_or(20); |
There was a problem hiding this comment.
The limit parameter is not validated to be within the range of 1 to 1000, which is what the Google Drive API expects for pageSize. The command's own documentation in skills/gws-docs-revisions/SKILL.md also specifies this range. Providing a value outside this range (e.g., 0 or 1001) will result in an API error. Please add validation to ensure the limit is within the accepted range.
let limit = matches.get_one::<u32>("limit").copied().unwrap_or(20);
if !(1..=1000).contains(&limit) {
return Err(GwsError::Validation(
"Invalid limit: value must be between 1 and 1000.".to_string(),
));
}
src/helpers/docs.rs
Outdated
| .query(&[ | ||
| ( | ||
| "fields", | ||
| "revisions(id,modifiedTime,lastModifyingUser/displayName,keepForever,size)", |
There was a problem hiding this comment.
This long, hardcoded string for the fields query parameter is difficult to read and maintain. A typo could easily break the API call. I suggest defining it as a constant at the top of the handle_revisions function, like this:
const REVISION_FIELDS: &str = "revisions(id,modifiedTime,lastModifyingUser/displayName,keepForever,size)";Then you can use the constant here.
REVISION_FIELDS
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new helper command gws docs +revisions to list the revision history of a Google Docs document. The implementation makes a direct call to the Google Drive API. The changes include the command logic, tests, and documentation.
My review has identified a critical security vulnerability related to URL injection, as well as opportunities to improve error handling and input validation for a more robust and user-friendly experience. Please see the detailed comments for suggestions.
src/helpers/docs.rs
Outdated
| .get(format!( | ||
| "https://www.googleapis.com/drive/v3/files/{}/revisions", | ||
| document_id | ||
| )) |
There was a problem hiding this comment.
The document_id is used directly in the URL without proper encoding. This is a critical security risk that can lead to URL injection or path traversal vulnerabilities if a malicious document ID is provided. For example, an ID like ../... could alter the request path.
You should percent-encode the document_id before inserting it into the URL string. You can use the percent_encoding crate, which is already a dependency in the project.
| .get(format!( | |
| "https://www.googleapis.com/drive/v3/files/{}/revisions", | |
| document_id | |
| )) | |
| .get(format!( | |
| "https://www.googleapis.com/drive/v3/files/{}/revisions", | |
| percent_encoding::utf8_percent_encode(document_id, percent_encoding::NON_ALPHANUMERIC) | |
| )) |
src/helpers/docs.rs
Outdated
| .long("limit") | ||
| .help("Maximum number of revisions to return (default: 20)") | ||
| .value_name("N") | ||
| .value_parser(clap::value_parser!(u32)), |
There was a problem hiding this comment.
The help text for --limit correctly states the accepted range is 1-1000, but this is not enforced by the argument parser. An invalid value (e.g., 0 or 2000) will be sent to the API, likely resulting in an API error. It's better to validate this on the client-side using clap's range validator for a better user experience.
| .value_parser(clap::value_parser!(u32)), | |
| .value_parser(clap::value_parser!(u32).range(1..=1000)), |
src/helpers/docs.rs
Outdated
|
|
||
| if !resp.status().is_success() { | ||
| let status = resp.status(); | ||
| let body = resp.text().await.unwrap_or_default(); |
There was a problem hiding this comment.
Using unwrap_or_default() here can hide the true cause of an error. If the request fails and reading the error response body also fails, this will result in an empty error message, making debugging difficult. It would be more robust to handle the potential error from resp.text().await.
| let body = resp.text().await.unwrap_or_default(); | |
| let body = resp.text().await.unwrap_or_else(|e| format!("Failed to read error response body: {e}")); |
- Add .range(1..=1000) to --limit clap value_parser so invalid values are rejected client-side before reaching the API - Extract REVISION_FIELDS constant to avoid hardcoded string duplication and make field list easier to maintain - Percent-encode document_id in the Drive API URL to prevent URL injection / path traversal (uses existing percent-encoding crate) - Replace unwrap_or_default() on error body with unwrap_or_else so secondary failures include a meaningful error message Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new gws docs +revisions helper command to list revision history for a Google Docs document. The implementation directly calls the Google Drive API. The changes include the new command logic, associated documentation, and tests. My review focuses on ensuring consistency with existing commands. I've found one high-severity issue related to the handling of the --dry-run flag, which is inconsistent with other helpers and could lead to unexpected network requests.
src/helpers/docs.rs
Outdated
| async fn handle_revisions(matches: &ArgMatches) -> Result<(), GwsError> { | ||
| const REVISION_FIELDS: &str = | ||
| "revisions(id,modifiedTime,lastModifyingUser/displayName,keepForever,size)"; | ||
|
|
||
| let document_id = matches.get_one::<String>("document").unwrap(); | ||
| let limit = matches.get_one::<u32>("limit").copied().unwrap_or(20); | ||
|
|
||
| let scope = "https://www.googleapis.com/auth/drive.readonly"; | ||
| let token = auth::get_token(&[scope]).await.map_err(|e| { | ||
| GwsError::Auth(format!( | ||
| "Docs auth failed: {}", | ||
| crate::output::sanitize_for_terminal(&e.to_string()) | ||
| )) | ||
| })?; | ||
|
|
||
| let client = crate::client::build_client()?; | ||
| let limit_str = limit.to_string(); | ||
| let encoded_id = | ||
| percent_encoding::utf8_percent_encode(document_id, percent_encoding::NON_ALPHANUMERIC); | ||
|
|
||
| let resp = client | ||
| .get(format!( | ||
| "https://www.googleapis.com/drive/v3/files/{}/revisions", | ||
| encoded_id | ||
| )) | ||
| .query(&[ | ||
| ("fields", REVISION_FIELDS), | ||
| ("pageSize", limit_str.as_str()), | ||
| ]) | ||
| .bearer_auth(&token) | ||
| .send() | ||
| .await | ||
| .map_err(|e| GwsError::Other(anyhow::anyhow!("HTTP request failed: {e}")))?; | ||
|
|
||
| if !resp.status().is_success() { | ||
| let status = resp.status(); | ||
| let body = resp | ||
| .text() | ||
| .await | ||
| .unwrap_or_else(|e| format!("Failed to read error response body: {e}")); | ||
| return Err(GwsError::Api { | ||
| code: status.as_u16(), | ||
| message: body, | ||
| reason: "revisions_request_failed".to_string(), | ||
| enable_url: None, | ||
| }); | ||
| } | ||
|
|
||
| let value: Value = resp | ||
| .json() | ||
| .await | ||
| .map_err(|e| GwsError::Other(anyhow::anyhow!("JSON parse failed: {e}")))?; | ||
|
|
||
| let fmt = matches | ||
| .get_one::<String>("format") | ||
| .map(|s| crate::formatter::OutputFormat::from_str(s)) | ||
| .unwrap_or_default(); | ||
| println!("{}", crate::formatter::format_value(&value, &fmt)); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The new +revisions helper does not handle the global --dry-run flag. This is inconsistent with other helpers like +write and can lead to unexpected behavior, as the command will attempt to make a real API call even when a dry run is requested.
While the PR description mentions that dry-run is 'N/A' because it doesn't produce a local JSON request body, it should still prevent network activity and provide informative output about what it would have done. I suggest implementing dry-run logic to improve consistency and user experience.
async fn handle_revisions(matches: &ArgMatches) -> Result<(), GwsError> {
const REVISION_FIELDS: &str =
"revisions(id,modifiedTime,lastModifyingUser/displayName,keepForever,size)";
let document_id = matches.get_one::<String>("document").unwrap();
let limit = matches.get_one::<u32>("limit").copied().unwrap_or(20);
let dry_run = matches.get_flag("dry-run");
let scope = "https://www.googleapis.com/auth/drive.readonly";
let token = if dry_run {
None
} else {
Some(auth::get_token(&[scope]).await.map_err(|e| {
GwsError::Auth(format!(
"Docs auth failed: {}",
crate::output::sanitize_for_terminal(&e.to_string())
))
})?)
};
let limit_str = limit.to_string();
let encoded_id =
percent_encoding::utf8_percent_encode(document_id, percent_encoding::NON_ALPHANUMERIC);
let url = format!(
"https://www.googleapis.com/drive/v3/files/{}/revisions",
encoded_id
);
if dry_run {
let dry_run_info = json!({
"dry_run": true,
"url": url,
"method": "GET",
"query_params": {
"fields": REVISION_FIELDS,
"pageSize": limit_str,
},
});
println!("{}", serde_json::to_string_pretty(&dry_run_info).unwrap_or_default());
return Ok(());
}
let client = crate::client::build_client()?;
let resp = client
.get(url)
.query(&[
("fields", REVISION_FIELDS),
("pageSize", limit_str.as_str()),
])
.bearer_auth(token.unwrap()) // Safe to unwrap due to dry_run check
.send()
.await
.map_err(|e| GwsError::Other(anyhow::anyhow!("HTTP request failed: {e}")))?;
if !resp.status().is_success() {
let status = resp.status();
let body = resp
.text()
.await
.unwrap_or_else(|e| format!("Failed to read error response body: {e}"));
return Err(GwsError::Api {
code: status.as_u16(),
message: body,
reason: "revisions_request_failed".to_string(),
enable_url: None,
});
}
let value: Value = resp
.json()
.await
.map_err(|e| GwsError::Other(anyhow::anyhow!("JSON parse failed: {e}")))?;
let fmt = matches
.get_one::<String>("format")
.map(|s| crate::formatter::OutputFormat::from_str(s))
.unwrap_or_default();
println!("{}", crate::formatter::format_value(&value, &fmt));
Ok(())
}All other helpers respect --dry-run. Without it, +revisions makes a real API call even when the user passes --dry-run, which is inconsistent with +write and the rest of the codebase. When --dry-run is set: skip auth, print the URL + query params as JSON, return early. Token is now Option<String> to avoid fetching it unnecessarily. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a gws docs +revisions helper command to list revision history for a Google Docs document. The implementation is clean, with corresponding documentation and tests. I have one high-severity suggestion to improve error handling by reusing existing logic, which will enhance consistency and provide better error details to the user.
| if !resp.status().is_success() { | ||
| let status = resp.status(); | ||
| let body = resp | ||
| .text() | ||
| .await | ||
| .unwrap_or_else(|e| format!("Failed to read error response body: {e}")); | ||
| return Err(GwsError::Api { | ||
| code: status.as_u16(), | ||
| message: body, | ||
| reason: "revisions_request_failed".to_string(), | ||
| enable_url: None, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This custom error handling for failed API requests duplicates logic from executor::handle_error_response. Reusing the shared error handler will provide more consistent and detailed error messages to the user, such as automatically parsing Google API error structures and extracting helpful information like the URL to enable a disabled API.
You can replace this block with a call to crate::executor::handle_error_response. The authentication method is OAuth because this code path is only taken when not in dry_run mode and an auth token has been successfully retrieved.
if !resp.status().is_success() {
let status = resp.status();
let body = resp
.text()
.await
.unwrap_or_else(|e| format!("Failed to read error response body: {e}"));
return crate::executor::handle_error_response(status, &body, &executor::AuthMethod::OAuth);
}Replace bare GwsError::Api construction with a local build_api_error helper that parses the Google JSON error format (message, reason, accessNotConfigured enable URL) - consistent with the same pattern in helpers/gmail/mod.rs and mirroring executor::handle_error_response. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new gws docs +revisions helper command to list revision history for a Google Docs document. The implementation calls the Drive v3 API directly. The changes include adding the new subcommand, its documentation, and tests. My main feedback is regarding code duplication in error handling. A new function build_api_error is introduced which largely duplicates logic from executor.rs. I've suggested refactoring this into a shared function to improve maintainability.
src/helpers/docs.rs
Outdated
| /// Build a from an HTTP error response, parsing the Google | ||
| /// JSON error format when available. Mirrors the logic in . | ||
| fn build_api_error(status: u16, body: &str) -> 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 | ||
| .and_then(|e| e.get("message")) | ||
| .and_then(|m| m.as_str()) | ||
| .unwrap_or(body) | ||
| .to_string(); | ||
| let reason = err_obj | ||
| .and_then(|e| e.get("errors")) | ||
| .and_then(|e| e.as_array()) | ||
| .and_then(|arr| arr.first()) | ||
| .and_then(|e| e.get("reason")) | ||
| .and_then(|r| r.as_str()) | ||
| .or_else(|| { | ||
| err_obj | ||
| .and_then(|e| e.get("reason")) | ||
| .and_then(|r| r.as_str()) | ||
| }) | ||
| .unwrap_or("unknown") | ||
| .to_string(); | ||
| let enable_url = if reason == "accessNotConfigured" { | ||
| crate::executor::extract_enable_url(&message) | ||
| } else { | ||
| None | ||
| }; | ||
| GwsError::Api { | ||
| code: status, | ||
| message, | ||
| reason, | ||
| enable_url, | ||
| } | ||
| } |
There was a problem hiding this comment.
This function build_api_error duplicates a significant amount of logic from executor::handle_error_response. This creates a maintainability issue, as any changes to Google's error format would need to be updated in two places.
To avoid this, consider refactoring the shared error parsing logic into a new public function, for example in error.rs, and call it from both executor.rs and here. This would improve code reuse and maintainability.
For example, you could create a function like GwsError::from_api_error(status: u16, body: &str) -> GwsError.
Also, the doc comment for this function seems incomplete.
…pi_response Add GwsError::from_api_response(status, body) to error.rs as the canonical shared method for parsing Google API error responses. Remove the local build_api_error function from docs.rs that duplicated this logic. Both executor::handle_error_response and the revisions helper now use the same parsing path, eliminating the duplication flagged in code review.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a gws docs +revisions helper to list document revision history. The implementation correctly uses reqwest to call the Drive API directly. However, I've identified two high-severity issues: a regression in the Cargo.lock file format version that could cause build inconsistencies, and the new helper is missing retry logic for API rate limiting, which could affect its reliability. Please see the detailed comments for suggestions on how to address these.
Cargo.lock
Outdated
| # This file is automatically @generated by Cargo. | ||
| # It is not intended for manual editing. | ||
| version = 4 | ||
| version = 3 |
There was a problem hiding this comment.
The Cargo.lock file format version has been downgraded from 4 to 3. This is a regression and can cause issues for developers using newer versions of Cargo (1.70+), potentially leading to inconsistent dependency resolution or build failures. Please revert this change and ensure the lock file is generated with a modern Cargo version.
src/helpers/docs.rs
Outdated
| ("pageSize", limit_str.as_str()), | ||
| ]) | ||
| .bearer_auth(token.unwrap()) // safe: dry_run path already returned above | ||
| .send() |
There was a problem hiding this comment.
The direct use of .send().await bypasses the repository's existing retry logic for handling rate limits (HTTP 429), which can make this new helper unreliable. The crate::client::send_with_retry function should be used to ensure consistent and robust error handling.
This will require restructuring the request building slightly. Here is an example of how it could be implemented:
let token = token.unwrap(); // safe: dry_run path already returned above
let resp = crate::client::send_with_retry(|| {
client
.get(url.clone())
.query(&[
("fields", REVISION_FIELDS),
("pageSize", limit_str.as_str()),
])
.bearer_auth(token.clone())
})
.await
.map_err(|e| GwsError::Other(anyhow::anyhow!("HTTP request failed: {e}")))?;….lock v4 Switch +revisions from direct .send().await to crate::client::send_with_retry so HTTP 429 rate-limit responses are retried consistently with all other helpers. Restore Cargo.lock format version from 3 to 4 to match upstream; no dependency changes, only the version header was different.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a useful gws docs +revisions helper command to view document revision history. The implementation is straightforward, making a direct call to the Drive API. The accompanying documentation and tests are well-written.
My main feedback is on the new GwsError::from_api_response helper function. While extracting this logic is a good idea, the current implementation has some inconsistencies with existing error handling in the codebase and contains incomplete documentation. I've left a detailed comment with a suggestion to improve it.
src/error.rs
Outdated
| /// Build a [] by parsing a Google API error response body. | ||
| /// | ||
| /// Extracts , , and (for errors) the | ||
| /// GCP console URL to enable the API. Falls back to the raw body when the | ||
| /// response is not well-formed Google JSON. | ||
| pub fn from_api_response(status: u16, body: &str) -> Self { | ||
| let err_json: Option<serde_json::Value> = serde_json::from_str(body).ok(); | ||
| let err_obj = err_json.as_ref().and_then(|v| v.get("error")); | ||
| let message = err_obj | ||
| .and_then(|e| e.get("message")) | ||
| .and_then(|m| m.as_str()) | ||
| .unwrap_or(body) | ||
| .to_string(); | ||
| let reason = err_obj | ||
| .and_then(|e| e.get("errors")) | ||
| .and_then(|e| e.as_array()) | ||
| .and_then(|arr| arr.first()) | ||
| .and_then(|e| e.get("reason")) | ||
| .and_then(|r| r.as_str()) | ||
| .or_else(|| { | ||
| err_obj | ||
| .and_then(|e| e.get("reason")) | ||
| .and_then(|r| r.as_str()) | ||
| }) | ||
| .unwrap_or("unknown") | ||
| .to_string(); | ||
| let enable_url = if reason == "accessNotConfigured" { | ||
| crate::executor::extract_enable_url(&message) | ||
| } else { | ||
| None | ||
| }; | ||
| GwsError::Api { | ||
| code: status, | ||
| message, | ||
| reason, | ||
| enable_url, | ||
| } | ||
| } |
There was a problem hiding this comment.
This new function from_api_response is a good step towards centralizing API error handling. However, there are a couple of issues:
-
Incomplete Documentation: The documentation comments contain placeholders like
[],, ,, and(for errors), which seem to be remnants of a template. This should be corrected to accurately describe the function's behavior. -
Inconsistent
codeHandling: This implementation differs from the existing logic inexecutor::handle_error_response. Specifically, it doesn't attempt to parse thecodefield from the JSON error body, instead relying solely on the HTTP status code passed in. The logic inexecutor.rsis more robust as it prefers thecodefrom the body and falls back to the HTTP status. It would be best to align this new function with the existing, more robust pattern.
Here's a suggested implementation that addresses these points:
/// Build a `GwsError::Api` by parsing a Google API error response body.
///
/// Extracts `code`, `message`, `reason`, and (for `accessNotConfigured` errors) the
/// GCP console URL to enable the API. Falls back to the raw body when the
/// response is not well-formed Google JSON.
pub fn from_api_response(status: u16, body: &str) -> Self {
let err_json: Option<serde_json::Value> = serde_json::from_str(body).ok();
let err_obj = err_json.as_ref().and_then(|v| v.get("error"));
let code = err_obj
.and_then(|e| e.get("code"))
.and_then(|c| c.as_u64())
.map(|c| c as u16)
.unwrap_or(status);
let message = err_obj
.and_then(|e| e.get("message"))
.and_then(|m| m.as_str())
.unwrap_or(body)
.to_string();
let reason = err_obj
.and_then(|e| e.get("errors"))
.and_then(|e| e.as_array())
.and_then(|arr| arr.first())
.and_then(|e| e.get("reason"))
.and_then(|r| r.as_str())
.or_else(|| {
err_obj
.and_then(|e| e.get("reason"))
.and_then(|r| r.as_str())
})
.unwrap_or("unknown")
.to_string();
let enable_url = if reason == "accessNotConfigured" {
crate::executor::extract_enable_url(&message)
} else {
None
};
GwsError::Api {
code,
message,
reason,
enable_url,
}
}Fix mangled rustdoc backtick references in from_api_response doc comment. Prefer the code field from the Google API JSON error body over the HTTP status code, falling back to status when absent — consistent with executor::handle_error_response.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new gws docs +revisions helper command to list revision history for a Google Docs document. The implementation correctly calls the Google Drive API, handles authentication, and provides a dry-run mode. The associated documentation is clear and helpful. My feedback focuses on improving test coverage for the new logic and addressing code duplication in error handling to enhance maintainability.
| /// Build a [`GwsError::Api`] by parsing a Google API error response body. | ||
| /// | ||
| /// Extracts `code`, `message`, `reason`, and (for `accessNotConfigured` errors) the | ||
| /// GCP console URL to enable the API. Falls back to the raw body when the | ||
| /// response is not well-formed Google JSON. | ||
| pub fn from_api_response(status: u16, body: &str) -> Self { | ||
| let err_json: Option<serde_json::Value> = serde_json::from_str(body).ok(); | ||
| let err_obj = err_json.as_ref().and_then(|v| v.get("error")); | ||
| let code = err_obj | ||
| .and_then(|e| e.get("code")) | ||
| .and_then(|c| c.as_u64()) | ||
| .map(|c| c as u16) | ||
| .unwrap_or(status); | ||
| let message = err_obj | ||
| .and_then(|e| e.get("message")) | ||
| .and_then(|m| m.as_str()) | ||
| .unwrap_or(body) | ||
| .to_string(); | ||
| let reason = err_obj | ||
| .and_then(|e| e.get("errors")) | ||
| .and_then(|e| e.as_array()) | ||
| .and_then(|arr| arr.first()) | ||
| .and_then(|e| e.get("reason")) | ||
| .and_then(|r| r.as_str()) | ||
| .or_else(|| { | ||
| err_obj | ||
| .and_then(|e| e.get("reason")) | ||
| .and_then(|r| r.as_str()) | ||
| }) | ||
| .unwrap_or("unknown") | ||
| .to_string(); | ||
| let enable_url = if reason == "accessNotConfigured" { | ||
| crate::executor::extract_enable_url(&message) | ||
| } else { | ||
| None | ||
| }; | ||
| GwsError::Api { | ||
| code, | ||
| message, | ||
| reason, | ||
| enable_url, | ||
| } | ||
| } |
There was a problem hiding this comment.
This new function from_api_response is a good addition for parsing API errors. However, its logic is nearly identical to the existing executor::handle_error_response function. This code duplication can become a maintenance issue, as changes to error parsing logic would need to be applied in two places, potentially leading to inconsistencies. To improve maintainability, please consider refactoring executor::handle_error_response to use this new GwsError::from_api_response function. This would centralize the API error parsing logic in one place.
src/helpers/docs.rs
Outdated
| async fn handle_revisions(matches: &ArgMatches) -> Result<(), GwsError> { | ||
| const REVISION_FIELDS: &str = | ||
| "revisions(id,modifiedTime,lastModifyingUser/displayName,keepForever,size)"; | ||
|
|
||
| let document_id = matches.get_one::<String>("document").unwrap(); | ||
| let limit = matches.get_one::<u32>("limit").copied().unwrap_or(20); | ||
| let dry_run = matches.get_flag("dry-run"); | ||
|
|
||
| let scope = "https://www.googleapis.com/auth/drive.readonly"; | ||
| let token = if dry_run { | ||
| None | ||
| } else { | ||
| Some(auth::get_token(&[scope]).await.map_err(|e| { | ||
| GwsError::Auth(format!( | ||
| "Docs auth failed: {}", | ||
| crate::output::sanitize_for_terminal(&e.to_string()) | ||
| )) | ||
| })?) | ||
| }; | ||
|
|
||
| let limit_str = limit.to_string(); | ||
| let encoded_id = | ||
| percent_encoding::utf8_percent_encode(document_id, percent_encoding::NON_ALPHANUMERIC); | ||
| let url = format!( | ||
| "https://www.googleapis.com/drive/v3/files/{}/revisions", | ||
| encoded_id | ||
| ); | ||
|
|
||
| if dry_run { | ||
| let dry_run_info = json!({ | ||
| "dry_run": true, | ||
| "url": url, | ||
| "method": "GET", | ||
| "query_params": { | ||
| "fields": REVISION_FIELDS, | ||
| "pageSize": limit_str, | ||
| }, | ||
| }); | ||
| println!( | ||
| "{}", | ||
| serde_json::to_string_pretty(&dry_run_info).unwrap_or_default() | ||
| ); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let token = token.unwrap(); // safe: dry_run path already returned above | ||
| let client = crate::client::build_client()?; | ||
| let resp = crate::client::send_with_retry(|| { | ||
| client | ||
| .get(url.clone()) | ||
| .query(&[ | ||
| ("fields", REVISION_FIELDS), | ||
| ("pageSize", limit_str.as_str()), | ||
| ]) | ||
| .bearer_auth(token.clone()) | ||
| }) | ||
| .await | ||
| .map_err(|e| GwsError::Other(anyhow::anyhow!("HTTP request failed: {e}")))?; | ||
|
|
||
| if !resp.status().is_success() { | ||
| let status = resp.status(); | ||
| let body = resp | ||
| .text() | ||
| .await | ||
| .unwrap_or_else(|e| format!("Failed to read error response body: {e}")); | ||
| return Err(GwsError::from_api_response(status.as_u16(), &body)); | ||
| } | ||
|
|
||
| let value: Value = resp | ||
| .json() | ||
| .await | ||
| .map_err(|e| GwsError::Other(anyhow::anyhow!("JSON parse failed: {e}")))?; | ||
|
|
||
| let fmt = matches | ||
| .get_one::<String>("format") | ||
| .map(|s| crate::formatter::OutputFormat::from_str(s)) | ||
| .unwrap_or_default(); | ||
| println!("{}", crate::formatter::format_value(&value, &fmt)); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The new handle_revisions function contains important business logic (URL construction, API parameters, dry-run output) but lacks unit tests. This makes it hard to verify its correctness and could lead to regressions during future refactoring.
Please consider refactoring handle_revisions to improve testability. For example, you could extract the logic for building the request details into a separate, testable function, similar to build_write_request. Then, you could add unit tests to cover:
- Correct URL and query parameter construction for a given document ID and limit.
- The structure and content of the JSON output in
--dry-runmode.
This would align with the testing strategy used for the +write helper and increase confidence in the new functionality.
Extract URL construction logic into build_revisions_request() for testability, following the build_write_request pattern. Add four unit tests covering: - correct URL construction for a plain document ID - percent-encoding of special characters in document IDs - limit_str formatting - dry-run JSON output structure and field values
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new gws docs +revisions helper command to list revision history for a Google Docs document. The implementation involves a direct call to the Drive API using reqwest. The changes also include adding a new helper function GwsError::from_api_response for parsing API errors. The implementation of the new command is solid, but the introduction of from_api_response creates duplication with existing error handling logic in src/executor.rs. I've left a comment suggesting a refactoring to consolidate this logic and reduce technical debt.
| pub fn from_api_response(status: u16, body: &str) -> Self { | ||
| let err_json: Option<serde_json::Value> = serde_json::from_str(body).ok(); | ||
| let err_obj = err_json.as_ref().and_then(|v| v.get("error")); | ||
| let code = err_obj | ||
| .and_then(|e| e.get("code")) | ||
| .and_then(|c| c.as_u64()) | ||
| .map(|c| c as u16) | ||
| .unwrap_or(status); | ||
| let message = err_obj | ||
| .and_then(|e| e.get("message")) | ||
| .and_then(|m| m.as_str()) | ||
| .unwrap_or(body) | ||
| .to_string(); | ||
| let reason = err_obj | ||
| .and_then(|e| e.get("errors")) | ||
| .and_then(|e| e.as_array()) | ||
| .and_then(|arr| arr.first()) | ||
| .and_then(|e| e.get("reason")) | ||
| .and_then(|r| r.as_str()) | ||
| .or_else(|| { | ||
| err_obj | ||
| .and_then(|e| e.get("reason")) | ||
| .and_then(|r| r.as_str()) | ||
| }) | ||
| .unwrap_or("unknown") | ||
| .to_string(); | ||
| let enable_url = if reason == "accessNotConfigured" { | ||
| crate::executor::extract_enable_url(&message) | ||
| } else { | ||
| None | ||
| }; | ||
| GwsError::Api { | ||
| code, | ||
| message, | ||
| reason, | ||
| enable_url, | ||
| } | ||
| } |
There was a problem hiding this comment.
This new function from_api_response is a good abstraction for parsing Google API error responses. However, it duplicates logic that already exists in src/executor.rs within the handle_error_response function.
To improve maintainability and ensure consistent error handling, handle_error_response should be refactored to use this new, centralized function. This will prevent bugs from being fixed in one place but not the other and make future updates easier.
Since executor.rs is not part of this PR, I recommend creating a high-priority follow-up task to address this technical debt.
2 participants
Summary
Adds a new
gws docs +revisionshelper command that lists revision metadata for a Google Docs document.Output per revision: id, modifiedTime, lastModifyingUser, keepForever, size
Implementation: Calls the Drive v3 revisions API directly (
drive.readonlyscope) using the same cross-service HTTP pattern as other helpers (reqwest + bearer_auth).Limitation documented: Content of past revisions is not accessible for native Docs files via the Google API. The help text makes this explicit so users are not surprised.
Dry Run
N/A -- this helper calls the Drive API directly via reqwest rather than through the gws schema/discovery system, so
--dry-rundoes not produce a local JSON request body.Changes
src/helpers/docs.rs-- new+revisionssubcommandskills/gws-docs-revisions/SKILL.md-- new AI agent skillskills/gws-docs/SKILL.md-- updated helper commands tableChecklist
google-*crates -- uses reqwest + bearer_auth directlycargo fmt --all-- run and appliedcargo clippy -- -D warnings-- zero warnings.changeset/docs-revisions-helper.md,minor)