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

Edits to media_downloaded in files.py to handle 201 response status (#1615 and #1806) #6309

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zoemyatt
Copy link

@zoemyatt zoemyatt commented Apr 7, 2024

#1615 and #1806 discussed the importance of handling a 201 response status in pipelines/files.py. We have attempted to correct this error by not only checking for 200 response status but also checking for 201 response status and location headers in the newly created image. If the new image has a 201 status and a location header, the new program schedules an extra request for the file data.

We then added a unit test to scrape a URL we knew would return a 201 status code and ran tox with the test_pipeline_media.py file.

tests/test_pipeline_media.py Outdated Show resolved Hide resolved
@zoemyatt
Copy link
Author

@wRAR let me know if these changes look better. Sorry about the extra file in there!

We improved the test case to check for a 201 status code without using a real URL and deleted the extra file.

@Gallaecio
Copy link
Member

@Gallaecio
Copy link
Member

Gallaecio commented Apr 22, 2024

Have you tried this fix in a real scenario?

The method you modified is meant to return a dict. You have modified it to return a Request for 201, but I don’t imagine the calling method that expects a dict will handle a Request. And your test verifies that the modified method returns a Request, it does not verify that the caller handles that request as needed.

I wonder if it would not be better to have some more general feature to follow redirects on empty 201 responses. Maybe modify the redirect middleware to do that if some new setting is enabled.

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