Xuanwo commented on code in PR #2892:
URL:
https://github.com/apache/incubator-opendal/pull/2892#discussion_r1300128912
##########
core/src/services/gdrive/backend.rs:
##########
@@ -221,12 +221,23 @@ impl GdriveBackend {
"application/vnd.google-apps.folder" => EntryMode::DIR,
_ => EntryMode::FILE,
});
- meta =
meta.with_content_length(metadata.size.parse::<u64>().unwrap_or(0));
+ meta = meta.with_content_length(
+ metadata
+ .size
+ .unwrap_or(String::from("0"))
Review Comment:
Although it's okay to implement it like this, we need to keep in mind that
we should respect the server's response. Our implementation should align with
server's correct behavior.
For this place:
> When will server return a metadata without `size`?
There are possible reasons:
- Gdrive doesn't return size for folder: we should make it clear by set
content length to `0` when we meet a folder.
- Gdrive only return size when we query it: It's a bug, we should fix it.
- Gdrive should return `0` but not: We should return an error instead
because it's violate service API Specs.
If we write `unwrap_or(String::from("0"))`, we are ignoring the reasons
behind it, which can create maintenance issues.
##########
core/src/services/gdrive/backend.rs:
##########
@@ -221,12 +221,23 @@ impl GdriveBackend {
"application/vnd.google-apps.folder" => EntryMode::DIR,
_ => EntryMode::FILE,
});
- meta =
meta.with_content_length(metadata.size.parse::<u64>().unwrap_or(0));
+ meta = meta.with_content_length(
+ metadata
+ .size
+ .unwrap_or(String::from("0"))
+ .parse::<u64>()
+ .map_err(|e| {
+ Error::new(ErrorKind::Unexpected, "parse content
length").set_source(e)
+ })?,
+ );
meta = meta.with_last_modified(
metadata
.modified_time
+ .unwrap_or_default()
Review Comment:
The same. We should know what we are expected.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
