Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(core): fix list with recursive when the object doesn't exist #5732

Merged
merged 5 commits into from
Mar 12, 2025

Conversation

meteorgan
Copy link
Contributor

Which issue does this PR close?

None

Rationale for this change

when performing list_with_recursive, if the object doesn't exist and the underlying service does not support list_with_recursive, we may incorrectly return the non-existent object.

What changes are included in this PR?

Are there any user-facing changes?

@meteorgan meteorgan force-pushed the fix-lister-when-dir-not-exist branch 2 times, most recently from ca15d6c to 61d8e92 Compare March 11, 2025 15:49
@meteorgan meteorgan force-pushed the fix-lister-when-dir-not-exist branch from 16bd0c6 to 2f08d15 Compare March 12, 2025 12:39
@meteorgan meteorgan force-pushed the fix-lister-when-dir-not-exist branch from 2f08d15 to ffb37c6 Compare March 12, 2025 13:00
@meteorgan meteorgan marked this pull request as ready for review March 12, 2025 13:11
@meteorgan meteorgan requested a review from Xuanwo as a code owner March 12, 2025 13:11
let (_, l) = self.acc.list(de.path(), OpList::new()).await?;
self.active_lister.push((Some(de), l));
let (_, mut l) = self.acc.list(de.path(), OpList::new()).await?;
if let Some(v) = l.next().await? {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an interesting issue: since we include every directory that contain a subdirectory or file, if the underlying service doesn't return the directory itself in the list operation, we won't be able to detect it. I discovered this behavior in the webdav service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Listing an empty root path can cover this case, but we need clear the root path before testing it.

}
} else {
return Ok(Some(v));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally, we could return Ok(None) immediately when detecting that a directory doesn't exist. However, if another thread is deleting content in related directories, we may return too early. The behavior tests caught this issue.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Really nice, thank you @meteorgan!

@Xuanwo Xuanwo merged commit eace92d into main Mar 12, 2025
281 checks passed
@Xuanwo Xuanwo deleted the fix-lister-when-dir-not-exist branch March 12, 2025 13:36
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants