Fix #609 - Implements HTTP Caching for HTML routes #1335

karlcow · 2017-02-09T04:50:12Z

This currently doesn't fix all HTML resources served on our site, but just the issue view.
I would love to get it on staging, because of our nginx layer and see how it flies. Locally, things seem to go well.

→ nosetests -v

API access to comments greater than 30 returns pagination in Link ... ok
API issue for a non existent number returns JSON 404. ... ok
API issue search with bad keywords returns JSON 404. ... ok
API access to labels without auth returns JSON 200. ... ok
API with wrong parameter returns JSON 404. ... ok
API setting labels without auth returns JSON 403 error code. ... ok
API access to user activity without auth returns JSON 401. ... ok
API with wrong category returns JSON 404. ... ok
API with wrong route returns JSON 404. ... ok
test_domain_name (tests.test_form.TestForm) ... ok
Make sure wrap_metadata and get_metadata methods work. ... ok
test_normalize_url (tests.test_form.TestForm) ... ok
Test HTTP Links formating. ... ok
Test browser parsing via get_browser helper method. ... ok
Test browser name parsing via get_browser_name helper method. ... ok
Test OS parsing via get_os helper method. ... ok
Test that API params are correctly converted to Search API. ... ok
normalize_api_params shouldn't transform unknown params. ... ok
Test HTTP Links parsing for GitHub only. ... ok
test_rewrite_and_sanitize_link (tests.test_helpers.TestHelpers) ... ok
Test we're correctly rewriting the passed in link. ... ok
Test that we're removing access_token parameters. ... ok
Check Cache-Control for issues. ... ok
Check ETAG for issues. ... ok
Check if we receive a 304 Not Modified. ... ok
testBase64ScreenshotUploads (tests.test_uploads.TestUploads) ... ok
Test that /upload/ doesn't let you GET. ... ok
testRegularUploads (tests.test_uploads.TestUploads) ... ok
Test that /about exists. ... ok
Test that asks user to log in before displaying activity. ... ok
Test that the home page exists. ... ok
Test issues and integer for: ... ok
Test that the /issues/<number> exists, and does not redirect. ... ok
Test that the /issues route gets 200 and does not redirect. ... ok
Webhook related tests. ... ok
Test that the /login route 302s to GitHub. ... ok
Test that /issues/new exists. ... ok
Test that /privacy exists. ... ok
Rate Limit URI sends 200 OK and starts with Current user. ... ok
Test that the /tools/cssfixme route gets 200. ... ok
Test that the /tools/cssfixme route gets 200 with ?url query. ... ok
Test that the /tools/cssfixme route gets 200 with bad ?url query. ... ok

----------------------------------------------------------------------
Ran 42 tests in 9.196s

OK

And on the CLI.

First request:

→ http --print Hh GET http://localhost:5000/issues/100

GET /issues/100 HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate
Connection: keep-alive
Host: localhost:5000
User-Agent: HTTPie/0.9.3

The 1st response

HTTP/1.0 200 OK
Cache-Control: private, max-age=31536000
Content-Length: 9366
Content-Type: text/html; charset=utf-8
Date: Thu, 09 Feb 2017 04:46:16 GMT
ETag: "bce46f17692195b267de8026b3c5f66f"
Server: Werkzeug/0.10.4 Python/2.7.10

Then the second request with If-None-Match

→ http --print Hhb GET http://localhost:5000/issues/100 'If-None-Match:"bce46f17692195b267de8026b3c5f66f"'

GET /issues/100 HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate
Connection: keep-alive
Host: localhost:5000
If-None-Match: "bce46f17692195b267de8026b3c5f66f"
User-Agent: HTTPie/0.9.3

with the second response:

HTTP/1.0 304 NOT MODIFIED
Cache-Control: private, max-age=31536000
Connection: close
Date: Thu, 09 Feb 2017 04:46:56 GMT
ETag: "bce46f17692195b267de8026b3c5f66f"
Server: Werkzeug/0.10.4 Python/2.7.10

karlcow · 2017-02-09T04:52:57Z

ok let's ask
r? @miketaylr
for a preliminary review.

webcompat/views.py

+    content = render_template('issue.html', number=number)
+    response = make_response(content)
+    # We can cache for one year. Etag is based on the content.
+    response = set_cache_control(response, 31536000)


miketaylr · 2017-02-09T16:52:38Z

Let me deploy this to staging so you can play around with it. I won't have time for review until tomorrow.

miketaylr · 2017-02-09T16:56:40Z

🍪 for new tests!

miketaylr · 2017-02-09T16:57:04Z

@karlcow deployed to staging.

karlcow · 2017-02-10T01:27:22Z

Thanks @miketaylr

https://staging.webcompat.com/issues/515

HTTP/1.1 200 OK
Server: nginx/1.1.19
Content-Type: text/html; charset=utf-8
Transfer-Encoding: chunked
Connection: keep-alive
Vary: Accept-Encoding
Cache-Control: private, max-age=31536000
Etag: "f9530168e98a2537ad066564ef2f38e5"
Date: Fri, 10 Feb 2017 01:09:30 GMT
Strict-Transport-Security: max-age=31536000; includeSubDomains
X-Content-Type-Options: nosniff
x-xss-protection: 1; mode=block
x-frame-options: DENY
Content-Encoding: gzip

Click reload

Request

GET /issues/515 HTTP/1.1
Host: staging.webcompat.com
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
Referer: https://staging.webcompat.com/
Cookie: session=****; _ga=GA1.2.1918118046.1486688948; _gat=1
Authorization: Basic czpz
Connection: keep-alive
Upgrade-Insecure-Requests: 1
If-None-Match: "f9530168e98a2537ad066564ef2f38e5"
Cache-Control: max-age=0

Response

HTTP/1.1 304 NOT MODIFIED
Server: nginx/1.1.19
Connection: keep-alive
Cache-Control: private, max-age=31536000
Etag: "f9530168e98a2537ad066564ef2f38e5"
Date: Fri, 10 Feb 2017 01:12:03 GMT
Strict-Transport-Security: max-age=31536000; includeSubDomains
X-Content-Type-Options: nosniff
x-xss-protection: 1; mode=block
x-frame-options: DENY

So this is working as expected. \o/

karlcow · 2017-02-10T07:18:22Z

I'm changing a bit the strategy to have more flexbility for other resources/routes.

I created a decorator defining the cache_policy
It also means that in the future we will be able to play a bit more with it and do fine tuning.

The routes without caching

/
/issues
/issues/nnnn DONE.
/about
/privacy
/contributors
/me
/activity/karlcow

karlcow · 2017-02-10T07:18:57Z

Don't review yet :)

miketaylr · 2017-02-10T17:44:28Z

Don't review yet :)

Deal!

webcompat/helpers.py

+
+    Adds Cache-Control headers.
+    Adds Etag based on HTTP Body.
+    Sends a 304 Not Modified in cas of If-None-Match.


karlcow · 2017-02-16T04:07:39Z

→ nosetests -v
/Users/karl/.virtualenvs/webcompatcom/lib/python2.7/site-packages/flask_limiter/extension.py:124: UserWarning: Use of the default `get_ipaddr` function is discouraged. Please refer to https://flask-limiter.readthedocs.org/#rate-limit-domain for the recommended configuration
  UserWarning
API access to comments greater than 30 returns pagination in Link ... ok
API issue for a non existent number returns JSON 404. ... ok
API issue search with bad keywords returns JSON 404. ... ok
API access to labels without auth returns JSON 200. ... ok
API with wrong parameter returns JSON 404. ... ok
API setting labels without auth returns JSON 403 error code. ... ok
API access to user activity without auth returns JSON 401. ... ok
API with wrong category returns JSON 404. ... ok
API with wrong route returns JSON 404. ... ok
test_domain_name (tests.test_form.TestForm) ... ok
Make sure wrap_metadata and get_metadata methods work. ... ok
test_normalize_url (tests.test_form.TestForm) ... ok
Test HTTP Links formating. ... ok
Test browser parsing via get_browser helper method. ... ok
Test browser name parsing via get_browser_name helper method. ... ok
Test OS parsing via get_os helper method. ... ok
Test that API params are correctly converted to Search API. ... ok
normalize_api_params shouldn't transform unknown params. ... ok
Test HTTP Links parsing for GitHub only. ... ok
test_rewrite_and_sanitize_link (tests.test_helpers.TestHelpers) ... ok
Test we're correctly rewriting the passed in link. ... ok
Test that we're removing access_token parameters. ... ok
Check Cache-Control for issues. ... ok
Check ETAG for issues. ... ok
Checks if we receive a 304 Not Modified. ... ok
testBase64ScreenshotUploads (tests.test_uploads.TestUploads) ... ok
Test that /upload/ doesn't let you GET. ... ok
testRegularUploads (tests.test_uploads.TestUploads) ... ok
Test that /about exists. ... ok
Test that asks user to log in before displaying activity. ... ok
Test that the home page exists. ... ok
Test issues and integer for: ... ok
Test that the /issues/<number> exists, and does not redirect. ... ok
Test that the /issues route gets 200 and does not redirect. ... ok
Webhook related tests. ... ok
Test that the /login route 302s to GitHub. ... ok
Test that /issues/new exists. ... ok
Test that /privacy exists. ... ok
Rate Limit URI sends 200 OK and starts with Current user. ... ok
Test that the /tools/cssfixme route gets 200. ... ok
Test that the /tools/cssfixme route gets 200 with ?url query. ... ok
Test that the /tools/cssfixme route gets 200 with bad ?url query. ... ok

----------------------------------------------------------------------
Ran 42 tests in 6.190s

OK

Let's start a formal review r? @miketaylr

webcompat/db/__init__.py

@@ -45,6 +45,7 @@ def __init__(self, issue_id, summary, url, body):
        self.url = url
        self.body = body

+


webcompat/helpers.py

@@ -456,3 +459,29 @@ def api_request(method, path, params=None, data=None):
                get_response_headers(resource))
    else:
        abort(404)
+
+
+def cache_policy(private=True, uri_max_age=3600):


webcompat/views.py

-    return render_template('issue.html', number=number)
+    content = render_template('issue.html', number=number)
+    response = make_response(content)
+    return response


webcompat/views.py

@@ -143,6 +144,7 @@ def index():


 @app.route('/issues')
+@cache_policy(private=True, uri_max_age=86400)


miketaylr

Looks good!

One or two minor nits that are up to you, but it would be good to add the requested comments.

One other thing: can you clean up the history here? 17 commits is a bit much for what we're adding.

For example, all the test commits could be squashed into a single "tests for http caching" commit. And there's a few commits that add an idea (8cc5446) (512a68c), then remove that idea (47f18dc), etc.

A commit style like that is GOOD for development, but we can just land the working loaf of bread that you baked, rather than the breadcrumbs (if that makes sense, it's a terrible metaphor).

Issue webcompat#609 - Removes code dust

karlcow · 2017-02-17T01:15:37Z

@miketaylr Ok I squashed everything into one commit. :) because rebasing with intertwined modifications was probably not a good idea. If you prefer I can submit separate commits for testing, decorator and views in a new branch. ^_^ No issue.

miketaylr · 2017-02-17T16:15:22Z

@miketaylr Ok I squashed everything into one commit. :) because rebasing with intertwined modifications was probably not a good idea. If you prefer I can submit separate commits for testing, decorator and views in a new branch. ^_^ No issue.

That's fine too. Doing gigantic interactive rebases can easily get weird.

miketaylr · 2017-02-17T16:17:11Z

r=me, assuming you'll make the uri_max_age=86400 default change. Feel free to merge and add the "add to changelog" label after that -- thanks!

karlcow · 2017-02-28T06:04:50Z

r=me, assuming you'll make the uri_max_age=86400 default change. Feel free to merge and add the "add to changelog" label after that -- thanks!

It was done no? So assuming that @miketaylr had missed the change
https://github.com/webcompat/webcompat.com/pull/1335/files/bab6f6df1676b65f891ace94527d4596f176dcbf#diff-23e0a36bbee957147173901eb77913cbR463

I'll move forward.

Thanks for the review! Very helpful.

miketaylr · 2017-02-28T15:00:49Z

It was done no? So assuming that @miketaylr had missed the change

I wasn't even looking -- I assumed you would merge it after you made the change. 😄

Happy this landed!

karlcow added the status: in progress label Feb 9, 2017

karlcow requested a review from miketaylr February 9, 2017 04:52

karlcow commented Feb 9, 2017

View reviewed changes

webcompat/views.py Outdated

content = render_template('issue.html', number=number)

response = make_response(content)

# We can cache for one year. Etag is based on the content.

response = set_cache_control(response, 31536000)

This comment was marked as abuse.

Sign in to view

miketaylr reviewed Feb 10, 2017

View reviewed changes

webcompat/helpers.py Outdated

Adds Cache-Control headers.

Adds Etag based on HTTP Body.

Sends a 304 Not Modified in cas of If-None-Match.

This comment was marked as abuse.

Sign in to view

karlcow changed the title ~~Implements HTTP Caching for issues/nnnn view~~ Fix #609 - Implements HTTP Caching for HTML routes Feb 16, 2017

miketaylr reviewed Feb 16, 2017

View reviewed changes

webcompat/db/__init__.py Outdated

@@ -45,6 +45,7 @@ def __init__(self, issue_id, summary, url, body):

self.url = url

self.body = body

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

miketaylr reviewed Feb 16, 2017

View reviewed changes

miketaylr requested changes Feb 16, 2017

View reviewed changes

Issue webcompat#609 - Implement Cache-Policy decorator

bab6f6d

Issue webcompat#609 - Removes code dust

karlcow force-pushed the 609/2 branch from bf2ac21 to bab6f6d Compare February 17, 2017 01:12

miketaylr approved these changes Feb 17, 2017

View reviewed changes

karlcow merged commit 8aefae1 into webcompat:master Feb 28, 2017

karlcow added the status: add to changelog label Feb 28, 2017

miketaylr removed the status: in progress label Mar 2, 2017

miketaylr removed the status: add to changelog label Mar 2, 2017

karlcow deleted the 609/2 branch March 30, 2017 05:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #609 - Implements HTTP Caching for HTML routes #1335

Fix #609 - Implements HTTP Caching for HTML routes #1335

karlcow commented Feb 9, 2017

karlcow commented Feb 9, 2017

This comment was marked as abuse.

miketaylr commented Feb 9, 2017

miketaylr commented Feb 9, 2017

miketaylr commented Feb 9, 2017

karlcow commented Feb 10, 2017 •

edited

Loading

karlcow commented Feb 10, 2017

karlcow commented Feb 10, 2017

miketaylr commented Feb 10, 2017

This comment was marked as abuse.

karlcow commented Feb 16, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr left a comment

karlcow commented Feb 17, 2017

miketaylr commented Feb 17, 2017

miketaylr commented Feb 17, 2017

karlcow commented Feb 28, 2017

miketaylr commented Feb 28, 2017

		@@ -45,6 +45,7 @@ def __init__(self, issue_id, summary, url, body):
		self.url = url
		self.body = body

		@@ -143,6 +144,7 @@ def index():


		@app.route('/issues')
		@cache_policy(private=True, uri_max_age=86400)

Fix #609 - Implements HTTP Caching for HTML routes #1335

Fix #609 - Implements HTTP Caching for HTML routes #1335

Conversation

karlcow commented Feb 9, 2017

karlcow commented Feb 9, 2017

This comment was marked as abuse.

miketaylr commented Feb 9, 2017

miketaylr commented Feb 9, 2017

miketaylr commented Feb 9, 2017

karlcow commented Feb 10, 2017 • edited Loading

karlcow commented Feb 10, 2017

karlcow commented Feb 10, 2017

miketaylr commented Feb 10, 2017

This comment was marked as abuse.

karlcow commented Feb 16, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr left a comment

Choose a reason for hiding this comment

karlcow commented Feb 17, 2017

miketaylr commented Feb 17, 2017

miketaylr commented Feb 17, 2017

karlcow commented Feb 28, 2017

miketaylr commented Feb 28, 2017

karlcow commented Feb 10, 2017 •

edited

Loading