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

Make StaticRoute support Last-Modified and If-Modified-Since headers #386

Merged
merged 4 commits into from
Jun 5, 2015
Merged

Conversation

magv
Copy link
Contributor

@magv magv commented May 30, 2015

This change makes StaticRoute add Last-Modified headers to the responses and recognize If-Modified-Since headers in the requests. If If-Modified-Since header is present and is equal to the modification time of the file being served, StaticRoute will now respond with a 304 "Not Modified" response.

This is a small feature, but rather useful if you hit "Refresh" button often.

self.assertIsNotNone(lastmod)
resp.close()

resp = yield from request('GET', url, loop=self.loop,
Copy link
Member

Choose a reason for hiding this comment

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

Please split the test into several (five?) ones.

@asvetlov
Copy link
Member

The PR is good but I guess to add if_modified_since readonly property to Request and last_modified read-write property to StreamResponse (like content_length for example).

@asvetlov
Copy link
Member

See also #165

@magv
Copy link
Contributor Author

magv commented May 31, 2015

I've split the tests somewhat (some of them are superfluous though).

Would you like the if_modified_since property to be a timestamp or a string? I think such a property would make sense if it's value was the parsed timestamp, but my proposed code doesn't really parse the If-Modified-Since header, it just compares it char-by-char with the mtime string -- this is faster, simpler, and just as useful (because user agents usually just copy whatever you sent in the Last-Modified header without interpreting it); this behaviour also protects against the situation where your clock was rewound backwards, and the client has a timestamp far into the future.

@asvetlov
Copy link
Member

Yes, properties make sense only if they have datetime type (timezone aware BTW).
I think string parsing time is vanishing tiny comparing to overall response time.

By standard it is timestamp. If you need arbitrary data like hash please use ETAG header.

@magv
Copy link
Contributor Author

magv commented Jun 2, 2015

Something like this then? I think it's overengineering, to be honest.

@@ -65,6 +69,33 @@ def content_length(self, _CONTENT_LENGTH=hdrs.CONTENT_LENGTH):
else:
return int(l)

@property
def if_modified_since(self, _IF_MODIFIED_SINCE=hdrs.IF_MODIFIED_SINCE):
Copy link
Member

Choose a reason for hiding this comment

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

Move it into Request object: it's request-only header

@asvetlov
Copy link
Member

asvetlov commented Jun 2, 2015

I want to make properties for all common HTTP headers eventually.
if_modified_since and last_modified are not necessary for static file handling task but I like to have those as well as other absent fields.

asvetlov added a commit that referenced this pull request Jun 5, 2015
Make StaticRoute support Last-Modified and If-Modified-Since headers
@asvetlov asvetlov merged commit 8c3d16c into aio-libs:master Jun 5, 2015
@asvetlov
Copy link
Member

asvetlov commented Jun 5, 2015

Thanks!

@lock
Copy link

lock bot commented Oct 30, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants