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

Update utils.py - check for PurePath instead of Path #1262

Closed
wants to merge 1 commit into from

Conversation

tomandersen
Copy link
Contributor

Should this check be for PurePath instead of Path? We had a crash when writing to S3 in django.

File "/usr/local/lib/python3.10/site-packages/storages/backends/s3boto3.py", line 596, in get_available_name
keellabs-django-1   |     name = clean_name(name)
keellabs-django-1   |   File "/usr/local/lib/python3.10/site-packages/storages/utils.py", line 47, in clean_name
keellabs-django-1   |     if name.endswith('/') and not clean_name.endswith('/'):
keellabs-django-1   | AttributeError: 'PurePosixPath' object has no attribute 'endswith'

See image here: https://docs.python.org/3/library/pathlib.html

NOTE - I am not a python / django expert, please look at this carefully.

Should this check be for PurePath instead of Path? We had a crash when writing to S3 in django. 

```
File "/usr/local/lib/python3.10/site-packages/storages/backends/s3boto3.py", line 596, in get_available_name
keellabs-django-1   |     name = clean_name(name)
keellabs-django-1   |   File "/usr/local/lib/python3.10/site-packages/storages/utils.py", line 47, in clean_name
keellabs-django-1   |     if name.endswith('/') and not clean_name.endswith('/'):
keellabs-django-1   | AttributeError: 'PurePosixPath' object has no attribute 'endswith'

```

See image here: https://docs.python.org/3/library/pathlib.html 

NOTE - I am not a python / django expert, please look at this carefully.
@tomandersen
Copy link
Contributor Author

Here they check and raise if not a str

https://github.com/boto/boto3/blob/develop/boto3/s3/transfer.py

Screenshot 2023-07-18 at 11 01 03 AM

@jschneier
Copy link
Owner

Thanks I failed to push to this branch but I've got you as the author in #1278, just added a test on top. Looks great.

@jschneier jschneier closed this Aug 26, 2023
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