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.

Sign up for GitHub

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

Jump to bottom

Normalize content-type before extention computation in SimpleHTTPResolver #509

Open
wants to merge 2 commits into
base: development
Choose a base branch
from
Open

Normalize content-type before extention computation in SimpleHTTPResolver #509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff61dd7
Normalize content-type before extention computation in SimpleHTTPReso…
damooo Aug 22, 2020
397ebb0
content-type normalization in SimpleHTTPResolver using cgi; updated t…
damooo Aug 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,5 @@ todo.md
.hypothesis

.coverage

.idea/
4 changes: 3 additions & 1 deletion loris/resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
`resolver` -- Resolve Identifiers to Image Paths
================================================
"""
import cgi
from contextlib import closing
import glob
import json
Expand Down Expand Up @@ -298,7 +299,8 @@ def cached_file_for_ident(self, ident):
def cache_file_extension(self, ident, response):
if 'content-type' in response.headers:
try:
extension = self.get_format(ident, constants.FORMATS_BY_MEDIA_TYPE[response.headers['content-type']])
content_type = cgi.parse_header(response.headers['content-type'])[0]
extension = self.get_format(ident, constants.FORMATS_BY_MEDIA_TYPE[content_type])
except KeyError:
logger.warn('Your server may be responding with incorrect content-types. Reported %s for ident %s.',
response.headers['content-type'], ident)
Expand Down
4 changes: 2 additions & 2 deletions tests/simple_http_resolver_ut.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,14 @@ def set_responses(self):
responses.HEAD,
self.identifier_url,
status=200,
content_type='image/tiff'
content_type='image/tiff; charset=UTF-8'
)
responses.add(
responses.GET,
self.identifier_url,
body='II*\x00\x0c\x00\x00\x00\x80\x00 \x0e\x00\x00\x01\x03\x00\x01\x00\x00\x00\x01\x00\x00\x00\x01\x01\x03\x00\x01\x00\x00\x00\x01\x00\x00\x00\x02\x01\x03\x00\x01\x00\x00\x00\x08\x00\x00\x00\x03\x01\x03\x00\x01\x00\x00\x00\x05\x00\x00\x00\x06\x01\x03\x00\x01\x00\x00\x00\x03\x00\x00\x00\x11\x01\x04\x00\x01\x00\x00\x00\x08\x00\x00\x00\x15\x01\x03\x00\x01\x00\x00\x00\x01\x00\x00\x00\x16\x01\x03\x00\x01\x00\x00\x00\x08\x00\x00\x00\x17\x01\x04\x00\x01\x00\x00\x00\x04\x00\x00\x00\x1a\x01\x05\x00\x01\x00\x00\x00\xba\x00\x00\x00\x1b\x01\x05\x00\x01\x00\x00\x00\xc2\x00\x00\x00\x1c\x01\x03\x00\x01\x00\x00\x00\x01\x00\x00\x00(\x01\x03\x00\x01\x00\x00\x00\x02\x00\x00\x00@\x01\x03\x00\x00\x03\x00\x00\xca\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00H\x00\x00\x00\x01\x00\x00\x00H\x00\x00\x00\x01\x00\x00\x00\xff`\xe6q\x19\x08\x00\x00\x80\t\x00\x00\x80\n\x00\x00\x80\x0b\x00\x00\x80\x0c\x00\x00\x80\r',
status=200,
content_type='image/tiff'
content_type='image/tiff; charset=UTF-8'
Copy link
Contributor

@alexwlchan alexwlchan Aug 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This means we're no longer testing the old behaviour – we know the header image/tiff; charset=UTF-8 works, but not image/tiff. I'd rather have a test suite that covers both.

Can you add that, or would you rather I merge it and add the extra test myself?

Copy link
Author

Choose a reason for hiding this comment

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

I thought that simple case is just one of sub-cases of complex-headers case. Here it tests with any valid content-type in it's generic sense, than to check all possible header values.

I will add special simple case quickly if it is required.

Copy link
Author

Choose a reason for hiding this comment

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

at present, it has one self.identifier_url, around which test case runs. should i add another resource_identifier, to do this check? I seems bit lost in figuring out convention test cases following. I fear, i may make it more intrusive. if you don't mind, can you write the test case best suited with conventions?

)
responses.add(
responses.HEAD,
Expand Down