You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "yunchipang (via GitHub)" <gi...@apache.org> on 2025/10/16 17:01:10 UTC
[PR] feat(services/gdrive): Implement write returns metadata [opendal]
yunchipang opened a new pull request, #6683:
URL: https://github.com/apache/opendal/pull/6683
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
part of #5693
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
- Adds the `fields` parameter which tells the gdrive API to include additional metadata in the response, and now returns `id` ,`size`, `modifiedTime`, `md5Checksum`, `version`.
- Relevant metadata is picked from [docs](https://developers.google.com/workspace/drive/api/reference/rest/v3/files).
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking-changes` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2438495350
##########
core/src/services/gdrive/core.rs:
##########
@@ -468,22 +492,63 @@ pub struct GdriveTokenResponse {
expires_in: u64,
}
-/// This is the file struct returned by the Google Drive API.
-/// This is a complex struct, but we only add the fields we need.
-/// refer to https://developers.google.com/drive/api/reference/rest/v3/files#File
+/// File struct for Google Drive API
+/// We select a few arbitrary fields.
+/// When update fields, keep `DRIVE_FILE_FIELDS` in sync to fetch related data.
+/// Read more [here](https://developers.google.com/drive/api/reference/rest/v3/files#File)
#[derive(Deserialize, Debug)]
#[serde(rename_all = "camelCase")]
pub struct GdriveFile {
pub mime_type: String,
pub id: String,
pub name: String,
+ // Size may be null for folders or shortcuts
pub size: Option<String>,
- // The modified time is not returned unless the `fields`
- // query parameter contains `modifiedTime`.
- // As we only need the modified time when we do `stat` operation,
- // if other operations(such as search) do not specify the `fields` query parameter,
- // try to access this field, it will be `None`.
- pub modified_time: Option<String>,
+ pub modified_time: String,
+ // Only applicable to files with binary content in Google Drive.
+ pub md5_checksum: Option<String>,
+ // A short-lived link to the file's version.
+ pub version: Option<String>,
+}
+
+impl GdriveFile {
+ /// Converts Google Drive file metadata to OpenDAL Metadata.
+ ///
+ /// This method parses the Google Drive API response fields and maps them
+ /// to OpenDAL's standard metadata fields.
+ pub(crate) fn to_metadata(&self) -> Result<Metadata> {
+ let mut metadata = Metadata::default();
+
+ let file_type = if self.mime_type == GDRIVE_FOLDER_MIME_TYPE {
+ EntryMode::DIR
+ } else {
+ EntryMode::FILE
+ };
+ metadata.set_mode(file_type);
Review Comment:
Nitpick: use Metadata::new(file_type)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #6683:
URL: https://github.com/apache/opendal/pull/6683#issuecomment-3415928004
Thanks for working on this.
Let me know if you are interested in fixing the service for tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2437635509
##########
core/src/services/gdrive/writer.rs:
##########
@@ -43,6 +43,32 @@ impl GdriveWriter {
file_id,
}
}
+
+ fn parse_metadata(file: GdriveFile) -> Result<Metadata> {
Review Comment:
fixed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #6683:
URL: https://github.com/apache/opendal/pull/6683#issuecomment-3417035148
@Xuanwo We have indeed a timing issue from the bug issue (#6684). I would accept this PR with one change: modified_time should remain optional.
The PR will pass the CI. But I know it doesn't mean we have strong consistency guarantee.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2437560442
##########
core/src/services/gdrive/writer.rs:
##########
@@ -43,6 +43,32 @@ impl GdriveWriter {
file_id,
}
}
+
+ fn parse_metadata(file: GdriveFile) -> Result<Metadata> {
+ let mut metadata = Metadata::default();
Review Comment:
See a comment in the backend.
##########
core/src/services/gdrive/backend.rs:
##########
@@ -74,11 +74,9 @@ impl Access for GdriveBackend {
Error::new(ErrorKind::Unexpected, "parse content length").set_source(e)
})?);
}
- if let Some(v) = gdrive_file.modified_time {
- meta = meta.with_last_modified(v.parse::<Timestamp>().map_err(|e| {
- Error::new(ErrorKind::Unexpected, "parse last modified time").set_source(e)
- })?);
- }
+ meta = meta.with_last_modified(gdrive_file.modified_time.parse::<Timestamp>().map_err(
Review Comment:
This is a good change addressing the comment.
Now looking at code structure, since you built parse metadata which is good, move these logic into your function.
The other is that most of this function should be moved into core.rs. It's okay the core have two stat functions. The newer function can simply be called gdrive_do_stat, or gdrive_get_metadata. The function returns this metadata.
Remember to extract the mime above as a constant.
##########
core/src/services/gdrive/writer.rs:
##########
@@ -43,6 +43,32 @@ impl GdriveWriter {
file_id,
}
}
+
+ fn parse_metadata(file: GdriveFile) -> Result<Metadata> {
Review Comment:
Move this to core.rs as an associated function or a model.rs. The struct is entirely encapsulated in core.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2440284786
##########
core/src/services/gdrive/core.rs:
##########
@@ -468,22 +492,63 @@ pub struct GdriveTokenResponse {
expires_in: u64,
}
-/// This is the file struct returned by the Google Drive API.
-/// This is a complex struct, but we only add the fields we need.
-/// refer to https://developers.google.com/drive/api/reference/rest/v3/files#File
+/// File struct for Google Drive API
+/// We select a few arbitrary fields.
+/// When update fields, keep `DRIVE_FILE_FIELDS` in sync to fetch related data.
+/// Read more [here](https://developers.google.com/drive/api/reference/rest/v3/files#File)
#[derive(Deserialize, Debug)]
#[serde(rename_all = "camelCase")]
pub struct GdriveFile {
pub mime_type: String,
pub id: String,
pub name: String,
+ // Size may be null for folders or shortcuts
pub size: Option<String>,
- // The modified time is not returned unless the `fields`
- // query parameter contains `modifiedTime`.
- // As we only need the modified time when we do `stat` operation,
- // if other operations(such as search) do not specify the `fields` query parameter,
- // try to access this field, it will be `None`.
- pub modified_time: Option<String>,
+ pub modified_time: String,
+ // Only applicable to files with binary content in Google Drive.
+ pub md5_checksum: Option<String>,
+ // A short-lived link to the file's version.
+ pub version: Option<String>,
+}
+
+impl GdriveFile {
+ /// Converts Google Drive file metadata to OpenDAL Metadata.
+ ///
+ /// This method parses the Google Drive API response fields and maps them
+ /// to OpenDAL's standard metadata fields.
+ pub(crate) fn to_metadata(&self) -> Result<Metadata> {
+ let mut metadata = Metadata::default();
+
+ let file_type = if self.mime_type == GDRIVE_FOLDER_MIME_TYPE {
+ EntryMode::DIR
+ } else {
+ EntryMode::FILE
+ };
+ metadata.set_mode(file_type);
Review Comment:
fixed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on PR #6683:
URL: https://github.com/apache/opendal/pull/6683#issuecomment-3415922389
@erickguan thanks so much for reviewing! yes i'd like to merge this first if possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #6683:
URL: https://github.com/apache/opendal/pull/6683#issuecomment-3415997963
Hitting errors like:
```shell
---- behavior::test_write_only ----
Unexpected (persistent) at write, context: { service: gdrive, path: 2598aedf-0a47-4feb-9509-0e2be776fae1 } => deserialize json, source: missing field `modifiedTime` at line 8 column 5
---- behavior::test_write_with_special_chars ----
Unexpected (persistent) at write, context: { service: gdrive, path: 6123d42b-5265-4ec7-874b-53167d218067 !@#$%^&()_+-=;',.txt } => deserialize json, source: missing field `modifiedTime` at line 8 column 5
---- behavior::test_write_returns_metadata ----
Unexpected (persistent) at write, context: { service: gdrive, path: 906b4dbb-08fb-4047-a859-ebb5bfcbb34f } => deserialize json, source: missing field `modifiedTime` at line 8 column 5
---- behavior::test_writer_write_with_overwrite ----
Unexpected (persistent) at write, context: { service: gdrive, path: 68b61cb3-9ec0-4c79-9694-83b56d38e9ea } => deserialize json, source: missing field `modifiedTime` at line 8 column 5
---- behavior::test_writer_abort ----
test panicked: assertion `left == right` failed
left: Unexpected
right: Unsupported
---- behavior::test_writer_abort_with_concurrent ----
test panicked: assertion `left == right` failed
left: Unexpected
right: Unsupported
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan merged PR #6683:
URL: https://github.com/apache/opendal/pull/6683
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2437224209
##########
core/src/services/gdrive/core.rs:
##########
Review Comment:
Remove since unused.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #6683:
URL: https://github.com/apache/opendal/pull/6683#issuecomment-3416191884
I will look over it firstly and ping you back for the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2437224209
##########
core/src/services/gdrive/core.rs:
##########
Review Comment:
Remove since unused.
##########
core/src/services/gdrive/core.rs:
##########
Review Comment:
```suggestion
/// File struct for Google Drive API
/// We select a few arbitrary fields.
/// When update fields, keep `DRIVE_FILE_FIELDS` in sync to fetch related data.
/// Read more [here](https://developers.google.com/drive/api/reference/rest/v3/files#File)
```
##########
core/src/services/gdrive/core.rs:
##########
Review Comment:
This comment is outdated.
I recommend adding these at the top of core.
```rust
// we append this as part of URL to request Google Drive file fields
// must be sync with `GdriveFile` struct fields
// read more [here](https://developers.google.com/workspace/drive/api/guides/fields-parameter)
pub(crate) const DRIVE_FILE_FIELDS: &'static str =
"id,size,modifiedTime,md5Checksum,version";
```
Then this can be used when needed when building URLs in `format!()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on PR #6683:
URL: https://github.com/apache/opendal/pull/6683#issuecomment-3416124864
hi @Xuanwo @erickguan thank you both for taking a look and noticing errors! i might not be able to help today (sorry!) but will follow up on the issue later if it has not been fixed yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2437305558
##########
core/src/services/gdrive/core.rs:
##########
Review Comment:
See a comment above near the top of the core.
Here is a design choice how to keep the fields and requested data in sync. On the spectrum,
- prefer consistency? Then we use more `String` in the model
- prefer lower amount of allocations? Then we use more `Option<String>`.
What do you think about this design choice? My reasoning is that Google probably loads some metadata in memory so I would always requests some data, like `id`, `name`, `size`, `modified_time`. Fine to keep `md5_checksum` and `version` as optional.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2437356440
##########
core/src/services/gdrive/core.rs:
##########
Review Comment:
@erickguan thanks for pointing this out! yea i'd prefer consistency in this case. however, based on the gdrive docs, `size` can be `null` or missing for folders or shortcuts, should we keep it `Optional` for prevent deserialization failures?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2437356440
##########
core/src/services/gdrive/core.rs:
##########
Review Comment:
@erickguan thanks for pointing this out! yea i'd prefer consistency in this case. however, based on the gdrive docs, `size` can be `null` or missing for folders or shortcuts, should we keep it `Optional` to prevent deserialization failures?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on PR #6683:
URL: https://github.com/apache/opendal/pull/6683#issuecomment-3416024314
@Xuanwo thanks for noticing. It’s my bad suggesting modified time changes. But I noticed a few more failures so I took the initiative to merge this PR firstly and will address bugs in the follow up ticket #6684. If I didn’t see Yun’s response in half an hour, I’ll start fixing this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2437234888
##########
core/src/services/gdrive/core.rs:
##########
Review Comment:
This comment is outdated. Maybe at least change it to:
```
only load a few fields to get smaller response
```
It's fine by me adding fields needed. Hence the other route is to be consistent in the module. Then we can add these at the top of core.
```rust
// we append this as part of URL to request Google Drive file fields
// must be sync with `GdriveFile` struct fields
// read more [here](https://developers.google.com/workspace/drive/api/guides/fields-parameter)
pub(crate) const DRIVE_FILE_FIELDS: &'static str =
"id,size,modifiedTime,md5Checksum,version";
```
Perhaps also add `name,mimeType`.
Then this can be used when needed when building URLs in `format!()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2437551571
##########
core/src/services/gdrive/core.rs:
##########
Review Comment:
Great. Please document your decision briefly in the structure comment.
For your question, good detail. No, we must use Optional for size if API omits the field. It also implies the API doesn't return the field once the struct is consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/gdrive): Implement write returns metadata [opendal]
Posted by "yunchipang (via GitHub)" <gi...@apache.org>.
yunchipang commented on code in PR #6683:
URL: https://github.com/apache/opendal/pull/6683#discussion_r2437637699
##########
core/src/services/gdrive/backend.rs:
##########
@@ -74,11 +74,9 @@ impl Access for GdriveBackend {
Error::new(ErrorKind::Unexpected, "parse content length").set_source(e)
})?);
}
- if let Some(v) = gdrive_file.modified_time {
- meta = meta.with_last_modified(v.parse::<Timestamp>().map_err(|e| {
- Error::new(ErrorKind::Unexpected, "parse last modified time").set_source(e)
- })?);
- }
+ meta = meta.with_last_modified(gdrive_file.modified_time.parse::<Timestamp>().map_err(
Review Comment:
fixed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org