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.

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 ci #4501

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

zjregee
Copy link
Contributor

@zjregee zjregee commented Apr 17, 2024

fix: #4484

There are several issues that need to be fixed here, related ci: https://github.com/apache/opendal/actions/runs/8682043318/job/23805843756.

Error caused by Dropbox service not supporting:

  • test_copy_overwrite
  • test_copy_file_with_non_ascii_name
  • test_rename_overwrite
  • test_list_prefix
  • test_list_file_with_recursive

Should return an error when it does not exist:

  • test_copy_non_existing_source
  • test_rename_non_existing_source

Returned by list does not contact the complete path:

  • test_list_dir
  • test_list_dir_with_metakey
  • test_list_dir_with_metakey_complete
  • test_list_rich_dir
  • test_list_nested_dir
  • test_list_dir_with_recursive
  • test_list_dir_with_recursive_no_trailing_slash

List dir should only return dir/

  • test_list_empty_dir
  • test_list_dir_with_file_path

@zjregee zjregee requested a review from PsiACE as a code owner April 17, 2024 07:05
@@ -558,6 +563,11 @@ pub async fn test_list_dir_with_recursive_no_trailing_slash(op: Operator) -> Res
}

pub async fn test_list_file_with_recursive(op: Operator) -> Result<()> {
// Dropbox does not support list file with recursive.
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make sense. All services that implement list_dir should have list prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this, I don't see it described in the API documentation, and in my practice Dropbox doesn't implement prefix matching.

Copy link
Member

Choose a reason for hiding this comment

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

prefix matching is simulated at

async fn complete_list(
&self,
path: &str,
args: OpList,
) -> Result<(RpList, CompleteLister<A, A::Lister>)> {
let cap = self.meta.full_capability();
if !cap.list {
return Err(self.new_unsupported_error(Operation::List));
}
let recursive = args.recursive();
match (recursive, cap.list_with_recursive) {
// - If service can list_with_recursive, we can forward list to it directly.
(_, true) => {
let (rp, p) = self.inner.list(path, args).await?;
Ok((rp, CompleteLister::One(p)))
}
// If recursive is true but service can't list_with_recursive
(true, false) => {
// Forward path that ends with /
if path.ends_with('/') {
let p = FlatLister::new(self.inner.clone(), path);
Ok((RpList::default(), CompleteLister::Two(p)))
} else {
let parent = get_parent(path);
let p = FlatLister::new(self.inner.clone(), parent);
let p = PrefixLister::new(p, path);
Ok((RpList::default(), CompleteLister::Four(p)))
}
}
// If recursive and service doesn't support list_with_recursive, we need to handle
// list prefix by ourselves.
(false, false) => {
// Forward path that ends with /
if path.ends_with('/') {
let (rp, p) = self.inner.list(path, args).await?;
Ok((rp, CompleteLister::One(p)))
} else {
let parent = get_parent(path);
let (rp, p) = self.inner.list(parent, args).await?;
let p = PrefixLister::new(p, path);
Ok((rp, CompleteLister::Three(p)))
}
}
}
}

I have a plan to refactor them later to make those behavior more clear.

@@ -178,6 +178,11 @@ pub async fn test_rename_nested(op: Operator) -> Result<()> {

/// Rename to a exist path should overwrite successfully.
pub async fn test_rename_overwrite(op: Operator) -> Result<()> {
// Dropbox does not support rename overwrite.
Copy link
Member

Choose a reason for hiding this comment

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

We can remove existing file before rename/copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropbox ci failed
2 participants