-
Notifications
You must be signed in to change notification settings - Fork 1
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
Set HTTP headers to enable client-side caching #75
Conversation
Adds a new key to the results JSON that gives the latest modification time (the date of indexing) listed in the database. Usable for cache-control.
Changes the internal representation of times to use Python's native datetime objects. Conversion to a string only happens right before serializing the response for the web server.
Looks good. I think the switch to a native representation for dates until response is generated is a good idea in any case. A couple of minor issues, mostly to do with testing. |
candidates.append(obj['modtime']) | ||
candidates = [ x for x in candidates if isinstance(x, datetime) ] | ||
if candidates: | ||
return max(candidates) |
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.
Slick.
'end_date': '1985-04-15T00:00:00Z', | ||
'start_date': 'datetime.datetime(1985, 1, 15, 0, 0), | ||
'end_date': datetime.datetime(1985, 4, 15, 0, 0), | ||
'modtime': datetime.datetime(2010, 1, 1, 17, 30, 4) |
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.
I really approve of keeping datetimes in native representation until final conversion.
@@ -32,6 +35,23 @@ def test_api_endpoints_are_callable(test_client, cleandb, endpoint, query_params | |||
url = '/api/' + endpoint | |||
response = test_client.get(url, query_string=query_params) | |||
assert response.status_code == 200 | |||
assert response.cache_control.public == True | |||
assert response.cache_control.max_age > 0 |
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.
Do you want to be a bit more specific about max_age
?
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.
Meh. We only set it in one and only one place, and the resolution of caching age is so coarse as to be trivial. I'm really only concerned that it got properly set to be anything aside from zero.
ce/tests/test_api.py
Outdated
assert key in file_metadata | ||
|
||
times = file_metadata['times'] | ||
assert len(times) > 0 | ||
|
||
# Are the values converible into times? | ||
# Are the values propert datetimes? |
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.
"proper"?
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.
Yep, good catch.
@@ -119,6 +140,7 @@ def test_multimeta(populateddb, model): | |||
# make sure start_date and end_date are present | |||
assert 'start_date' in rv[unique_id] | |||
assert 'end_date' in rv[unique_id] | |||
assert 'modtime' in rv[unique_id] |
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.
Should you be asserting the types (datetime) of these?
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.
Yes, good suggestion.
@@ -176,6 +199,7 @@ def test_stats_bad_params(populateddb, unique_id, var): | |||
assert math.isnan(rv[unique_id]['max']) | |||
assert 'time' not in rv[unique_id] | |||
assert 'units' not in rv[unique_id] | |||
assert 'modtime' not in rv[unique_id] |
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.
Assert types (datetime) as well?
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.
This is a not in
check, so there's no type to assert.
This PR fixes #74 by setting two HTTP caching headers, allowing clients to forgo the need to refresh rarely changing content.
Cache-Control
We set
cache-control
to "public", essentially turning it on, and we set amax-age
of 24 hours (climate data will rarely-if-never be updated more often than that).Last-Modified
For requests that are tied to one or more
DataFile
entries, we set the last-modified header to be the most recent update seen on any of the data files that comprise the response. The update times are taken from theindex_time
field of theDataFile
entries.Byproducts
To implement this, I ended up changing the internal representation of date times. Rather than using
strftime()
to format them when they come right out of the database, I opted to use the Pythondatetime
object to pass values through the code, only serializing right before its necessary to encode to JSON. I think this makes the code a bit cleaner (and it was necessary for testing).Demo
Backend demo is here.
Frontend demo using this backend is here. Watch your dev tool's network requests and note that it shouldn't make a re-request when you switch back-and-forth between two options.
The demo isn't using the production Gunicorn work pool for servicing requests, so it may appear a bit sluggish if requests back up behind each other. But just pay attention to what your browser is requesting and more importantly, what it isn't.
Comments/questions/concerns welcome.