Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(services/dropbox): fix dropbox test failed #4317

Closed
wants to merge 2 commits into from
Closed

fix(services/dropbox): fix dropbox test failed #4317

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79ad5a1
fix ci failed
zjregee Mar 5, 2024
84dabdc
add failure deserialization data structure for batch
zjregee Mar 6, 2024
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
13 changes: 8 additions & 5 deletions core/src/services/dropbox/core.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,11 +337,14 @@ impl DropboxCore {
(path, Ok(RpDelete::default().into()))
}
"failure" => {
let error = entry.error.expect("error should be present");
let err = Error::new(
ErrorKind::Unexpected,
&format!("delete failed with error {}", error.error_summary),
);
let err = if let Some(error) = entry.error {
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit odd that dropbox returns a failure without an error. Do you know the actual response from dropbox services?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for overlooking a potential issue.

Yes, it returns an error format that the current DropboxErrorResponse cannot parse. We should modify DropboxErrorResponse to support the new error format.

Due to the 1password settings, I cannot completely reproduce the test in CI. The following are the results of testing using a personal account.

When an error occurs during the stat call, Dropbox returns an error like this:

{
    "error_summary": "path/not_found/",
    "error": {
        ".tag": "path",
        "path": {
            ".tag": "not_found"
        }
    }
}

When an error occurs during batch call, Dropbox returns an error like this (causing an expect error):

{
    ".tag": "complete",
    "entries": [
        {
            ".tag": "failure",
            "failure": {
                ".tag": "path_lookup",
                "path_lookup": {
                    ".tag": "not_found"
                }
            }
        }
    ]
}

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Seems we need to parse the error response from dropbox correctly.

Error::new(
ErrorKind::Unexpected,
&format!("delete failed with error {}", error.error_summary),
)
} else {
Error::new(ErrorKind::Unexpected, "delete failed")
};
("".to_string(), Err(err))
}
_ => (
Expand Down
Loading