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

Use storage.exists on HEAD method #6751

Merged
merged 1 commit into from Mar 9, 2020
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Mar 5, 2020

When request method is HEAD we can't use storage.url because the signature calculated will consider GET as method. django-storages does not support passing the HTTP method into storage.url yet. Because of this, the response won't be exactly the same between GET and HEAD since we are not passing the headers returned by the storage itself.

We just use storage.exists and return 200 with an empty body or raise 404.

`storage.url` uses the HTTP method to calculate the signature, which
by default is GET and django-storages does not allow to change it.
@humitos humitos requested a review from ericholscher Mar 5, 2020
@humitos
Copy link
Member Author

humitos commented Mar 5, 2020

I opened and issue and a PR to fix this in django-storages itself: jschneier/django-storages#853

@jschneier
Copy link

jschneier commented Mar 6, 2020

Merged the corresponding PR

@ericholscher
Copy link
Member

ericholscher commented Mar 9, 2020

Thanks @jschneier!

@ericholscher
Copy link
Member

ericholscher commented Mar 9, 2020

@humitos should we merge this, or should we add a stoages dependency to the commit that fixes it?

@humitos
Copy link
Member Author

humitos commented Mar 9, 2020

@ericholscher the immediate fix I'd say is our PR. I'm not sure what else we are introducing if we upgrade django-storages right now. We are using 1.7.2 and the latest version is 1.9.1

I'm not really concern about incompatibility changes in the django-storage package itself, but on azure (mainly) and boto3 dependencies.

My plan would be to merge this PR and deploy tomorrow and then revert it, upgrade django-storage + azure/boto3 and test it for at least one week locally.

Copy link
Member

@ericholscher ericholscher left a comment

Makes sense. This at least gives us the right status code 👍

@ericholscher ericholscher merged commit 5691b4e into master Mar 9, 2020
3 checks passed
@ericholscher ericholscher deleted the humitos/handle-proxito-head branch Mar 9, 2020
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.

None yet

3 participants