search

thumbor-community / aws

Thumbor AWS extensions
MIT License
155 stars 70 forks source link

fix delete file scenario. Was missing normalization from path to abspath #141

Closed eitanshapiro closed 4 years ago

eitanshapiro commented 4 years ago

Thumbor report successful delete while in practice the delete fails.

Branch Master Also release 6.2.12

Copy/paste the result of the pip freeze command here. We'll be most interested in the versions of 

boto==2.49.0
boto3==1.11.6
botocore==1.14.6
thumbor==6.7.0
tornado==5.1.1
tornado-botocore==1.5.0
tornado-pyvows==0.6.1

Configuration

TC_AWS_STORAGE_BUCKET = 'vo-thumbor-bucket' # S3 bucket for Storage
TC_AWS_STORAGE_ROOT_PATH = 'testDeleteImage' # S3 path prefix for Storage bucket
TC_AWS_REGION = 'us-east-1'

Steps to reproduce

curl -i -H "Content-Type: image/png" -H "Slug: test.png" -XPOST http://localhost:8080/image --data-binary "@Avatar-85-256.png"

curl -i -XDELETE http://localhost:8080/image/1e98c320dd714290a33403e5c8e75b86/test.png

Delete is returning 204 but the file is not deleted in S3 :(

Adding this code to s3_storage.py solves the issue

@return_future
def remove(self, path, callback=None):
"""
Deletes data at path
:param string path: Path to delete
"""
super(Storage, self).remove(self._normalize_path(path), callback)
eitanshapiro commented 4 years ago

Looking at https://github.com/thumbor-community/aws/pull/131 it actually fixes the same thing

eitanshapiro commented 4 years ago

I think the tests fail because the other pull request is not in master - https://github.com/thumbor-community/aws/pull/139

Bladrak commented 4 years ago

@eitanshapiro thanks for your submission! Could you add a test regarding the removing of a file, as well as rebase it?

Bladrak commented 4 years ago

@kkopachev if you can take a look at this as well :)

eitanshapiro commented 4 years ago

Adding TC_AWS_STORAGE_ROOT_PATH='nana' to the test is showing the issue that was fixed by calling normalization.

MrLesh commented 4 years ago

Is there a target release for this pull request? we really need it in our project

Bladrak commented 4 years ago

I'll release this next week, I'll let you know

Bladrak commented 4 years ago

Hi, @MrLesh you should be able to use the latest release (6.2.15).

@kkopachev I've fixed the publishing process which needed to migrate to twine for some reason. If you need to release, you can simply create a release from GitHub with an unexisting version, and it should publish a new release to pypi.