Xuanwo commented on code in PR #2600:
URL:
https://github.com/apache/incubator-opendal/pull/2600#discussion_r1254254331
##########
core/src/services/dropbox/core.rs:
##########
@@ -49,6 +49,13 @@ impl Debug for DropboxCore {
}
impl DropboxCore {
+ fn build_path(&self, path: &str) -> String {
+ let path = build_rooted_abs_path(&self.root, path);
+ // For dropbox, even the path is a directory,
+ // we still need to remove the trailing slash.
+ path.strip_suffix('/').unwrap_or(&path).to_string()
Review Comment:
We can use `trim` instead of `strip`.
##########
core/src/services/dropbox/builder.rs:
##########
@@ -114,6 +114,7 @@ impl Builder for DropboxBuilder {
fn from_map(map: HashMap<String, String>) -> Self {
let mut builder = Self::default();
+ builder.root(map.get("root").map(|v| v.as_str()).unwrap_or_default());
Review Comment:
How about keeping the same style that `map.get("root").map(|v|
builder.root(v));`
##########
core/src/services/dropbox/error.rs:
##########
@@ -38,12 +38,34 @@ pub async fn parse_error(resp: Response<IncomingAsyncBody>)
-> Result<Error> {
| StatusCode::GATEWAY_TIMEOUT => (ErrorKind::Unexpected, true),
_ => (ErrorKind::Unexpected, false),
};
+
+ // We cannot get the error type from the response header when the status
code is 409.
+ // Because Dropbox API v2 will put error summary in the response body, we
need to parse it
+ // to get the correct error type and then error kind.
+ // See
https://www.dropbox.com/developers/documentation/http/documentation#error-handling
let dropbox_error =
serde_json::from_slice::<DropboxErrorResponse>(&bs).map_err(new_json_deserialize_error);
match dropbox_error {
Ok(dropbox_error) => {
- let mut err = Error::new(kind,
dropbox_error.error_summary.as_ref())
- .with_context("response", format!("{parts:?}"));
+ let error_summary = dropbox_error.error_summary.as_str();
+ let mut err = Error::new(
Review Comment:
Can we make those code more readable?
##########
core/src/services/dropbox/core.rs:
##########
@@ -98,7 +108,23 @@ impl DropboxCore {
pub async fn dropbox_delete(&self, path: &str) ->
Result<Response<IncomingAsyncBody>> {
let url = "https://api.dropboxapi.com/2/files/delete_v2".to_string();
let args = DropboxDeleteArgs {
- path: build_rooted_abs_path(&self.root, path),
+ path: self.build_path(path),
+ };
+
+ let bs =
Bytes::from(serde_json::to_string(&args).map_err(new_json_serialize_error)?);
+
+ let request = self
+ .build_auth_header(Request::post(&url))
Review Comment:
Maybe we should set content length?
##########
.github/workflows/service_test_dropbox.yml:
##########
Review Comment:
> What's your meaning? Its expiration time is quite short and I can't set it.
I'm talking about `Before merging, I will xxx`. If those content is not
ready for merging, how about removing them and start a new issue to track them.
##########
core/src/services/dropbox/core.rs:
##########
@@ -82,6 +89,9 @@ impl DropboxCore {
}
if let Some(mime) = content_type {
request_builder = request_builder.header(header::CONTENT_TYPE,
mime);
+ } else {
Review Comment:
We can use `request_builder = request_builder.header(header::CONTENT_TYPE,
mime.unwrap_or("xxxx"));` instead.
##########
core/src/services/dropbox/backend.rs:
##########
@@ -109,12 +137,17 @@ impl Accessor for DropboxBackend {
_ => EntryMode::Unknown,
};
let mut metadata = Metadata::new(entry_mode);
- let last_modified = decoded_response.client_modified;
- let date_utc_last_modified =
parse_datetime_from_rfc3339(&last_modified)?;
- metadata.set_last_modified(date_utc_last_modified);
- if decoded_response.size.is_some() {
- let size = decoded_response.size.unwrap();
- metadata.set_content_length(size);
+ // Only set last_modified and size if entry_mode is FILE,
because Dropbox API
+ // returns last_modified and size only for files.
+ // FYI:
https://www.dropbox.com/developers/documentation/http/documentation#files-get_metadata
+ if entry_mode == EntryMode::FILE {
+ let last_modified = decoded_response.client_modified;
+ let date_utc_last_modified =
parse_datetime_from_rfc3339(&last_modified)?;
+ metadata.set_last_modified(date_utc_last_modified);
+ if decoded_response.size.is_some() {
+ let size = decoded_response.size.unwrap();
Review Comment:
I don't like code `if xxx.is_some() { let ttt = xxx.unwrap() }`. How about
use `if let Some(size) = xxx` instead?
And it's should be an error if content length is not set for file.
--
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]
