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

Error page optimisations #299

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions cloudflare/workers.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,10 @@ const REPLACE_STRIPPED_QUERYSTRING_ON_REDIRECT_LOCATION = false;
// Disabled by default, but highly recommended
const STRIP_VALUELESS_QUERYSTRING_KEYS = false;

// Only these status codes should be considered cacheable
// Only these status codes should be considered cacheable.
// (from https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.4)
const CACHABLE_HTTP_STATUS_CODES = [200, 203, 206, 300, 301, 410];
// 404 is added to reduce server load on spammed requests.
const CACHABLE_HTTP_STATUS_CODES = [200, 203, 206, 300, 301, 410, 404];

addEventListener('fetch', (event) => {
event.respondWith(main(event));
Expand Down
43 changes: 43 additions & 0 deletions tbx/core/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,46 @@ def test_setting_theme_on_one_site_sets_it_on_multiple_sites(self):
# check theme is set on new site
resp = self.client.get(f"http://{new_site.hostname}/")
self.assertEqual(resp.context["MODE"], mode)

class PageNotFoundTestCase(TestCase):
url = "/does-not-exist/"

def test_accept_html(self) -> None:
response = self.client.get(self.url, headers={"Accept": "text/html"})
self.assertEqual(response.status_code, 404)
self.assertIn("text/html", response.headers["content-type"])

def test_simple_when_doesnt_accept_html(self) -> None:
response = self.client.get(self.url, headers={"Accept": "text/css"})
self.assertEqual(response.status_code, 404)
self.assertIn("text/plain", response.headers["content-type"])

def test_simple_when_html_not_highest(self) -> None:
response = self.client.get(
self.url, headers={"Accept": "text/html;q=0.8,text/css"}
)
self.assertEqual(response.status_code, 404)
self.assertIn("text/plain", response.headers["content-type"])

def test_missing_accept_header(self) -> None:
response = self.client.get(self.url, headers={"Accept": ""})
self.assertEqual(response.status_code, 404)
self.assertIn("text/plain", response.headers["content-type"])

def test_wildcard_accept_header(self) -> None:
response = self.client.get(self.url, headers={"Accept": "*/*"})
self.assertEqual(response.status_code, 404)
self.assertIn("text/plain", response.headers["content-type"])

def test_browser_request(self) -> None:
"""
Test Accept header from Firefox 128
"""
response = self.client.get(
self.url,
headers={
"Accept": "text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/png,image/svg+xml,*/*;q=0.8" # noqa:E501
},
)
self.assertEqual(response.status_code, 404)
self.assertIn("text/html", response.headers["content-type"])
65 changes: 62 additions & 3 deletions tbx/core/utils/views.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,68 @@
from django.http import HttpResponseNotFound, HttpResponseServerError
from django.views import defaults
from django.views.decorators.cache import cache_control
from django.views.decorators.csrf import requires_csrf_token
from django.views.decorators.vary import vary_on_headers


def page_not_found(request, exception, template_name="patterns/pages/errors/404.html"):
return defaults.page_not_found(request, exception, template_name)
def get_quality(media_type):
return float(media_type.params.get("q", 1))


def show_html_error_page(request):
# If there is no `Accept` header, serve the simpler page
if not request.headers.get("Accept"):
return False

accepted_types = sorted(request.accepted_types, key=get_quality, reverse=True)

if len(accepted_types) == 1 and accepted_types[0].match("*/*"):
return False

html_type = next(
(
accepted_type
for accepted_type in accepted_types
if accepted_type.match("text/html")
),
None,
)

# If HTML isn't accepted, don't serve it
if html_type is None:
return False

max_quality = get_quality(accepted_types[0])

# If HTML isn't the highest quality, don't serve it
if get_quality(html_type) < max_quality:
return False

return True


@requires_csrf_token
Copy link
Member

Choose a reason for hiding this comment

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

Why do these need csrf? Doesn't that skip the cache completely?

@vary_on_headers("Accept")
@cache_control(max_age=900) # 15 minutes
Copy link
Member

Choose a reason for hiding this comment

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

Should this maybe set s_maxage too?

"s_maxage": s_maxage,

def page_not_found(
request, exception=None, template_name="patterns/pages/errors/404.html"
):
if show_html_error_page(request):
return defaults.page_not_found(request, exception, template_name)

# Serve a simpler, cheaper 404 page if possible
return HttpResponseNotFound(
"Page not found", content_type="text/plain; charset=utf-8"
)


@requires_csrf_token
@vary_on_headers("Accept")
def server_error(request, template_name="patterns/pages/errors/500.html"):
return defaults.server_error(request, template_name)
if show_html_error_page(request):
return defaults.server_error(request, template_name)

# Serve a simpler, cheaper 500 page if possible
return HttpResponseServerError(
"Server error", content_type="text/plain; charset=utf-8"
)
12 changes: 3 additions & 9 deletions tbx/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
get_default_cache_control_method_decorator,
)
from tbx.core.views import robots, switch_mode
from tbx.core.utils.views import page_not_found, server_error
from wagtail import urls as wagtail_urls
from wagtail.admin import urls as wagtailadmin_urls
from wagtail.contrib.sitemaps.views import sitemap
Expand All @@ -33,7 +34,6 @@
if settings.DEBUG:
from django.conf.urls.static import static
from django.contrib.staticfiles.urls import staticfiles_urlpatterns
from django.views.generic import TemplateView

# Serve static and media files from development server
urlpatterns += staticfiles_urlpatterns()
Expand All @@ -42,14 +42,8 @@
# Add views for testing 404 and 500 templates
urlpatterns += [
# Add views for testing 404 and 500 templates
path(
"test404/",
TemplateView.as_view(template_name="patterns/pages/errors/404.html"),
),
path(
"test500/",
TemplateView.as_view(template_name="patterns/pages/errors/500.html"),
),
path("test404/", page_not_found),
path("test500/", server_error),
]

# Django Debug Toolbar
Expand Down
Loading