-
Notifications
You must be signed in to change notification settings - Fork 214
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
Bugfix to ensure image type is correctly extracted from content type #4062
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
….parametrize for test_get_extension_from_content_type()
openverse-bot
added
🟨 priority: medium
Not blocking but should be addressed soon
🛠 goal: fix
Bug fix
💻 aspect: code
Concerns the software code in the repository
🚦 status: awaiting triage
Has not been triaged & therefore, not ready for work
labels
Apr 8, 2024
AetherUnbound
removed
the
🚦 status: awaiting triage
Has not been triaged & therefore, not ready for work
label
Apr 8, 2024
…modified get_file_extension_from_content_type() to utilize mimetypes.guess_extension() to automatically find file extensions in strings
…that return None when passed to mimetypes.guess_extension() to compare to None. Also, call mimetypes.guess_extension() in conditional header, assign it to variable, and then only strip it of '.' if it isn't None
sarayourfriend
approved these changes
Apr 9, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks very much for the contribution @thegreendrinker and for adding a new unit test 🚀
stacimc
approved these changes
Apr 10, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic, lgtm 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
💻 aspect: code
Concerns the software code in the repository
🛠 goal: fix
Bug fix
🟨 priority: medium
Not blocking but should be addressed soon
🧱 stack: api
Related to the Django API
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #3750 by @obulat
In
api/utils/image_proxy/extension.py
,_get_file_extension_from_content_type()
was modified to split the returnedcontent_type
by;
in order to remove undesirable appended section of string to the file type. I also made sure to strip any preceding.
that occurred when passingcontent_type
tomimetypes.guess_extension()
. Finally,None
was returned anytime the call tomimetypes.guess_extension()
returnedNone
.Description
When you attempt to go to the following link, the following 415 error is returned:
{"detail":"Unsupported media type \"Image extension png;charset=UTF-8 is not supported by the thumbnail proxy.\" in request."}
charset=UTF-8
is incorrectly appended topng
. To solve this, I wrote a function to remove the undesirable appended section of the content file type. I also wrote a@pytest.mark.parametrize
decorator that contains tests for various file extensions.Testing Instructions
Created a draft pull request so that I can get assistance on running the tests utilizing the Docker container.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin