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

S3Path.exists() returns True on partial matches #208

Closed
maciejb opened this issue Feb 7, 2022 · 4 comments · Fixed by #244 or #245
Closed

S3Path.exists() returns True on partial matches #208

maciejb opened this issue Feb 7, 2022 · 4 comments · Fixed by #244 or #245
Labels
bug Something isn't working

Comments

@maciejb
Copy link

maciejb commented Feb 7, 2022

S3Path.exists() is returning True for anything that begins with the same as a real file. For instance, if we have a file named foobar, we get a false positive when querying for foo.

>>> S3Path('s3://my-bucket/foobar').exists()  # This file is really there
True
>>> S3Path('s3://my-bucket/foo').exists()  # This file does NOT exist
True
@jayqi
Copy link
Member

jayqi commented Feb 7, 2022

Looks like this is a bug in S3Client._s3_file_query.

# else, confirm it is a dir by filtering to the first item under the prefix
except ClientError:
return next(
(
obj
for obj in (
self.s3.Bucket(cloud_path.bucket)
.objects.filter(Prefix=cloud_path.key)
.limit(1)
)
),
None,
)

This branch of logic is too permissive and needs to also check if there's a /.

Also a blind spot in our tests that this isn't caught.

@jayqi jayqi added the bug Something isn't working label Feb 7, 2022
@pjbull
Copy link
Member

pjbull commented Feb 7, 2022

This branch of logic is too permissive and needs to also check if there's a /.

I think append a / to the key if there isn't one in this fallback path—that said, we'll need to double check if that handles the fake directory scenario as well.

@jayqi jayqi added the review label Feb 8, 2022
@jayqi jayqi closed this as completed Feb 8, 2022
@pjbull pjbull removed the review label Feb 8, 2022
@jayqi
Copy link
Member

jayqi commented Feb 8, 2022

Whoops, closed this by accident.

@frndmg
Copy link
Contributor

frndmg commented Aug 4, 2022

Hi. Looking for some eyes on #244 if anybody has the time. We stumbled upon the same problem and fixed it by patching it but wanted to submit a fix.

I think append a / to the key if there isn't one in this fallback path—that said, we'll need to double check if that handles the fake directory scenario as well.

It is also checking that condition as well.

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
4 participants