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

bug: the implement of list_with_deleted is not consistent with the RFC-5495 #5507

Closed
1 task
Tracked by #5515
meteorgan opened this issue Jan 3, 2025 · 3 comments · Fixed by #5518
Closed
1 task
Tracked by #5515

bug: the implement of list_with_deleted is not consistent with the RFC-5495 #5507

meteorgan opened this issue Jan 3, 2025 · 3 comments · Fixed by #5518
Labels
bug Something isn't working

Comments

@meteorgan
Copy link
Contributor

meteorgan commented Jan 3, 2025

Describe the bug

In RFC-5495:

Please note that `deleted` here means "including deleted files" rather than "only deleted files." Therefore, `list_with(path).deleted(true)` will list both current files and deleted ones.

But in the implement of S3, list_with_deleted only contains delete marker and common prefix, current files are not included.
https://github.com/apache/opendal/blob/c866d4bbae50f863b22ec2728011e80b633177a8/core/src/services/s3/lister.rs#L225-L250d

Steps to Reproduce

pub async fn test_list_files_with_deleted(op: Operator) -> Result<()> {
    if !op.info().full_capability().list_with_deleted {
        return Ok(());
    }

    let parent = TEST_FIXTURE.new_dir_path();
    let file_name = TEST_FIXTURE.new_file_path();
    let file_path = format!("{}{}", parent, file_name);
    op.write(file_path.as_str(), "1").await?;
    op.write(file_path.as_str(), "2").await?;
    op.delete(file_path.as_str()).await?;

    let file_path2 = format!("{}{}", parent, TEST_FIXTURE.new_file_path());
    op.write(file_path2.as_str(), "1").await?;

    let mut ds = op.list_with(&parent).deleted(true).await?;
    // this will fail !
    assert_eq!(ds.len(), 2);
}

Expected Behavior

the result of list_with_deleted includes the current files or change the RFC

Additional Context

No response

Are you willing to submit a PR to fix this bug?

  • Yes, I would like to submit a PR.
@meteorgan meteorgan added the bug Something isn't working label Jan 3, 2025
@Xuanwo
Copy link
Member

Xuanwo commented Jan 3, 2025

Yes, we should include the current file since list_with().deleted(true) is defined to be inclusive.

@hoslo
Copy link
Contributor

hoslo commented Jan 7, 2025

Yes, we should include the current file since list_with().deleted(true) is defined to be inclusive.

If the deleted file is not the latest, current file is should in output.version. And should list_with().deleted(true) include multi delete marker for same file?

@Xuanwo
Copy link
Member

Xuanwo commented Jan 7, 2025

If the deleted file is not the latest, current file is should in output.version.

@meteorgan's case mixed up two files, the most simple one should be:

pub async fn test_list_files_with_deleted(op: Operator) -> Result<()> {
    if !op.info().full_capability().list_with_deleted {
        return Ok(());
    }

    let parent = TEST_FIXTURE.new_dir_path();
    let file_name = TEST_FIXTURE.new_file_path();
    let file_path = format!("{}{}", parent, file_name);
    op.write(file_path.as_str(), "1").await?;

    let mut ds = op.list_with(&file_path).deleted(true).await?;
    assert_eq!(ds.len(), 1);
}

This means list_with().deleted(true) should always include the latest version of the file.

And should list_with().deleted(true) include multi delete marker for same file?

Yes, we should list all of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants