Skip to content

Replace call to self._clean_name with clean_name#252

Merged
codingjoe merged 2 commits intocodingjoe:mainfrom
agsimmons:patch-1
Mar 31, 2023
Merged

Replace call to self._clean_name with clean_name#252
codingjoe merged 2 commits intocodingjoe:mainfrom
agsimmons:patch-1

Conversation

@agsimmons
Copy link
Copy Markdown
Contributor

@agsimmons agsimmons commented Mar 27, 2023

Fixes #251

This utility function was moved in April 2017, so it should be supported on all recent versions of django-storages, but it would break support for versions older than that. Should a minimum django-storages version be specified for django-s3file?

@agsimmons
Replace call to self._clean_name with clean_name
19000ad
@agsimmons
Require django-storages>=1.6
c2036fe 
clean_name utility function was first made available in version 1.6
@codingjoe codingjoe self-requested a review March 29, 2023 06:42
@github-advanced-security
Copy link
Copy Markdown

github-advanced-security bot commented Mar 29, 2023

You have successfully added a new CodeQL configuration /language:javascript. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 29, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (df1b4df) 98.02% compared to head (c2036fe) 98.03%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #252   +/-   ##
=======================================
  Coverage   98.02%   98.03%           
=======================================
  Files           8        8           
  Lines         203      204    +1     
=======================================
+ Hits          199      200    +1     
  Misses          4        4
Impacted Files Coverage Δ
s3file/middleware.py 93.61% <ø> (ø)
s3file/storages_optimized.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-advanced-security
Copy link
Copy Markdown

github-advanced-security bot commented Mar 29, 2023

You have successfully added a new CodeQL configuration /language:python. As part of the setup process, we have scanned this repository and found 1 existing alert. Please check the repository Security tab to see all alerts.

codingjoe
codingjoe reviewed Mar 29, 2023
View reviewed changes
Copy link
Copy Markdown
Owner

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @agsimmons,

Thanks for the patch. I believe we can safely limit the django-storages dependency. After all, your package manager will inform you about compatibility, and we're not forcing users to a new major version.

I am curious, that we didn't notice this before. Would you be so kind and add a test that covers this better?

Thanks again!

Cheers,
Joe

@agsimmons
Copy link
Copy Markdown
Contributor Author

agsimmons commented Mar 30, 2023

storages.utils.clean_name was first made available in django-storages 1.6 but storages.backends.s3boto3.S3Boto3Storage._clean_name was just removed in the most recent django-storages 1.13.2 which only came out 3 months ago.

The dependabot PRs from the past 2 months have all had failing tests because of this. Here's one example: https://github.com/codingjoe/django-s3file/actions/runs/3839867555/jobs/6538228693

I increased the minimum django-storages version to 1.6 in this PR, but I'm wondering if maybe more minimum dependencies should be set. Django 3.2 is the oldest version receiving security updates, it's the earliest version being tested against this project, and support for it was added in django-storages 1.12. Maybe these potential changes are better suited for a separate PR.

codingjoe
codingjoe approved these changes Mar 31, 2023
View reviewed changes
Copy link
Copy Markdown
Owner

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agsimmons thanks for the swift response. I didn't notice tests failing. I am probably maintaining too many things. Therefore, I really appreciate people like you, making excellent contributions. 🥇

@codingjoe codingjoe merged commit 660d079 into codingjoe:main Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@codingjoe codingjoe codingjoe approved these changes

Assignees

@agsimmons agsimmons

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

S3OptimizedUploadStorage: self._clean_name does not exist

3 participants

@agsimmons @github-advanced-security @codingjoe