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

Fixed gzip automatic content-type assignment and added automatic compression header configuration #251

Merged
merged 3 commits into from
Aug 14, 2024
Merged

Fixed gzip automatic content-type assignment and added automatic compression header configuration #251

merged 3 commits into from
Aug 14, 2024

Conversation

lukaskremla
Copy link
Contributor

This implements the fix for detecting the proper content-type even when the file has the ".gz" extension. It further makes sure the compression headers are set properly if a "gz." file is detected, but the compression headers weren't explicitly set by the user.

lukaskremla and others added 2 commits August 13, 2024 16:11
…ression setting

This implements the fix for detecting the proper content-type even when the file has the ".gz" extension. It further makes sure the compression headers are set properly if a "gz." file is detected, but the compression headers weren't explicitly set by the user.
@@ -774,7 +774,10 @@ def send_file(cls, filename, status_code=200, content_type=None,
first.
"""
if content_type is None:
ext = filename.split('.')[-1]
if filename.endswith('.gz'):
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if filename.endswith('.gz'):
if compressed and filename.endswith('.gz'):

I think it is best to rely on the compression argument here.

@@ -786,7 +789,7 @@ def send_file(cls, filename, status_code=200, content_type=None,
if max_age is not None:
headers['Cache-Control'] = 'max-age={}'.format(max_age)

if compressed:
if compressed or filename.endswith('.gz'):
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if compressed or filename.endswith('.gz'):
if compressed:

I think this is fine as it was. If the user passes compressed=False then the response should not be compressed.

self.assertEqual(res.status_code, 200)
self.assertEqual(res.headers['Content-Type'], 'text/plain')

res = Response.send_file('tests/files/test.txt.gz')
Copy link
Owner

Choose a reason for hiding this comment

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

This test should use octet-stream as content file, because the compressed argument is False by default.

There should be another test that passes compressed=True and only then the we want the content type to be text and the content-encoding header to be present.

@miguelgrinberg
Copy link
Owner

miguelgrinberg commented Aug 13, 2024

I do not like the idea of making it "automatic". If the call has compressed=True, then when the file has a .gz extension it should be removed to determine the correct content type. But when compressed=False the file should be sent as is, even if it is a gzip file, which is a valid binary file that should go out with octet-stream content type.

@lukaskremla
Copy link
Contributor Author

Your suggestions make sense. I have modified the microdot.py and test files accordingly.

@miguelgrinberg miguelgrinberg merged commit 482ab6d into miguelgrinberg:main Aug 14, 2024
19 of 20 checks passed
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.

2 participants