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 issue where empty dir name in s3 key breaks listdir api. #771

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

varunmittal91
Copy link

Tried writing this test case, but moto_service i think ignores these slashes anyway. Maybe somebody else will have a better luck with this test case.

def test_simple_with_empty_folder_name(s3):
    e_base_path = f"{test_bucket_name}//test"
    e = e_base_path + "/" + "e"

    data = b"e" * (10 * 2**20)

    with s3.open(e, "wb") as f:
        f.write(data)

    with s3.open(e, "rb") as f:
        out = f.read(len(data))
        assert len(data) == len(out)
        assert out == data

    dir_list = s3.listdir(f"s3://{test_bucket_name}//test")
    assert len(dir_list) > 0

@martindurant
Copy link
Member

@ianthomas23 , the tests here pass, but it does seem like this change ought to break something somewhere!

@varunmittal91
Copy link
Author

@ianthomas23 , the tests here pass, but it does seem like this change ought to break something somewhere!

I am sorry, I know what you mean. I wish there was a better way. If there is something you want me to look into, I will be happy to,

@martindurant
Copy link
Member

Maybe I should have phrased it thus: I imagine that the lstrip you removed was there for a reason ;). It may be that changes elsewhere made it unnecessary.

@varunmittal91
Copy link
Author

So s3 has a wiered thing, one of our customers have key s3://bucket_name//test//some_data

With lstrip it removes those '/' but without that we can do a listdir.

@martindurant
Copy link
Member

Note that .find() should I think still work in these odder cases.

@varunmittal91
Copy link
Author

oh okay, I can try and see if that resolves the usecase. Mahor problem is that i am using it with pyarrow and fspec which will do a listdir.

@martindurant
Copy link
Member

Totally understand... you can maybe still pass a list of absolute filenames taken from find(), though.

@varunmittal91
Copy link
Author

okay I will give it a try, thanks for looking into it.

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

Successfully merging this pull request may close these issues.

2 participants