From d01a5ef8bee9e54580a529727b1ccf3b79cefe4f Mon Sep 17 00:00:00 2001 From: Nick Smith Date: Fri, 22 Apr 2016 10:32:06 +0100 Subject: [PATCH] #2281 add tests, and refactor redirects, to handle duplicates with query strings --- wagtail/wagtailredirects/middleware.py | 29 +++++---- wagtail/wagtailredirects/tests.py | 81 +++++++++++++++++++------- 2 files changed, 77 insertions(+), 33 deletions(-) diff --git a/wagtail/wagtailredirects/middleware.py b/wagtail/wagtailredirects/middleware.py index ad6c25492..ef5e53a5a 100644 --- a/wagtail/wagtailredirects/middleware.py +++ b/wagtail/wagtailredirects/middleware.py @@ -4,6 +4,16 @@ from django.utils.six.moves.urllib.parse import urlparse from wagtail.wagtailredirects import models +def get_redirect(request, path): + try: + return models.Redirect.get_for_site(request.site).get(old_path=path) + except models.Redirect.MultipleObjectsReturned: + # We have a site-specific and a site-ambivalent redirect; prefer the specific one + return models.Redirect.objects.get(site=request.site, old_path=path) + except models.Redirect.DoesNotExist: + return None + + # Originally pinched from: https://github.com/django/django/blob/master/django/contrib/redirects/middleware.py class RedirectMiddleware(object): def process_response(self, request, response): @@ -19,23 +29,18 @@ class RedirectMiddleware(object): # Get the path path = models.Redirect.normalise_path(request.get_full_path()) - # Get the path without the query string or params - path_without_query = urlparse(path).path - # Find redirect - try: - redirect = models.Redirect.get_for_site(request.site).get(old_path=path) - except models.Redirect.MultipleObjectsReturned: - # We have a site-specific and a site-ambivalent redirect; prefer the specific one - redirect = models.Redirect.objects.get(site=request.site, old_path=path) - except models.Redirect.DoesNotExist: + redirect = get_redirect(request, path) + if redirect is None: + # Get the path without the query string or params + path_without_query = urlparse(path).path + if path == path_without_query: # don't try again if we know we will get the same response return response - try: - redirect = models.Redirect.get_for_site(request.site).get(old_path=path_without_query) - except models.Redirect.DoesNotExist: + redirect = get_redirect(request, path_without_query) + if redirect is None: return response if redirect.is_permanent: diff --git a/wagtail/wagtailredirects/tests.py b/wagtail/wagtailredirects/tests.py index 14d6da459..258f6c7a4 100644 --- a/wagtail/wagtailredirects/tests.py +++ b/wagtail/wagtailredirects/tests.py @@ -159,17 +159,70 @@ class TestRedirects(TestCase): def test_duplicate_redirects_when_match_is_for_generic(self): contact_page = Page.objects.get(url_path='/home/contact-us/') site = Site.objects.create(hostname='other.example.com', port=80, root_page=contact_page) - christmas_page = Page.objects.get(url_path='/home/events/christmas/') # two redirects, one for any site, one for specific - models.Redirect.objects.create(old_path='/xmas', redirect_page=contact_page) - models.Redirect.objects.create(site=site, old_path='/xmas', redirect_page=christmas_page) + models.Redirect.objects.create(old_path='/xmas', redirect_link='/generic') + models.Redirect.objects.create(site=site, old_path='/xmas', redirect_link='/site-specific') - response = self.client.get('/xmas/', HTTP_HOST='localhost') - # the redirect which matched was the generic one, pointing to contact_page - self.assertRedirects(response, 'http://other.example.com/', status_code=301, fetch_redirect_response=False) + response = self.client.get('/xmas/') + # the redirect which matched was /generic + self.assertRedirects(response, '/generic', status_code=301, fetch_redirect_response=False) - def test_duplicate_redirects_across_sites_when_match_is_for_specific(self): + def test_duplicate_redirects_with_query_string_when_match_is_for_generic(self): + contact_page = Page.objects.get(url_path='/home/contact-us/') + site = Site.objects.create(hostname='other.example.com', port=80, root_page=contact_page) + + # two redirects, one for any site, one for specific, both with query string + models.Redirect.objects.create(old_path='/xmas?foo=Bar', redirect_link='/generic-with-query-string') + models.Redirect.objects.create(site=site, old_path='/xmas?foo=Bar', redirect_link='/site-specific-with-query-string') + + # and two redirects, one for any site, one for specific, without query strings + models.Redirect.objects.create(old_path='/xmas', redirect_link='/generic') + models.Redirect.objects.create(site=site, old_path='/xmas', redirect_link='/site-specific') + + response = self.client.get('/xmas/?foo=Bar') + # the redirect which matched was /generic-with-query-string + self.assertRedirects(response, '/generic-with-query-string', status_code=301, fetch_redirect_response=False) + + # now use a non-matching query string + response = self.client.get('/xmas/?foo=Baz') + # the redirect which matched was /generic + self.assertRedirects(response, '/generic', status_code=301, fetch_redirect_response=False) + + def test_duplicate_redirects_when_match_is_for_specific(self): + contact_page = Page.objects.get(url_path='/home/contact-us/') + site = Site.objects.create(hostname='other.example.com', port=80, root_page=contact_page) + + # two redirects, one for any site, one for specific + models.Redirect.objects.create(old_path='/xmas', redirect_link='/generic') + models.Redirect.objects.create(site=site, old_path='/xmas', redirect_link='/site-specific') + + response = self.client.get('/xmas/', HTTP_HOST='other.example.com') + # the redirect which matched was /site-specific + self.assertRedirects(response, 'http://other.example.com/site-specific', status_code=301, fetch_redirect_response=False) + + def test_duplicate_redirects_with_query_string_when_match_is_for_specific_with_qs(self): + contact_page = Page.objects.get(url_path='/home/contact-us/') + site = Site.objects.create(hostname='other.example.com', port=80, root_page=contact_page) + + # two redirects, one for any site, one for specific, both with query string + models.Redirect.objects.create(old_path='/xmas?foo=Bar', redirect_link='/generic-with-query-string') + models.Redirect.objects.create(site=site, old_path='/xmas?foo=Bar', redirect_link='/site-specific-with-query-string') + + # and two redirects, one for any site, one for specific, without query strings + models.Redirect.objects.create(old_path='/xmas', redirect_link='/generic') + models.Redirect.objects.create(site=site, old_path='/xmas', redirect_link='/site-specific') + + response = self.client.get('/xmas/?foo=Bar', HTTP_HOST='other.example.com') + # the redirect which matched was /site-specific-with-query-string + self.assertRedirects(response, 'http://other.example.com/site-specific-with-query-string', status_code=301, fetch_redirect_response=False) + + # now use a non-matching query string + response = self.client.get('/xmas/?foo=Baz', HTTP_HOST='other.example.com') + # the redirect which matched was /site-specific + self.assertRedirects(response, 'http://other.example.com/site-specific', status_code=301, fetch_redirect_response=False) + + def test_duplicate_page_redirects_when_match_is_for_specific(self): contact_page = Page.objects.get(url_path='/home/contact-us/') site = Site.objects.create(hostname='other.example.com', port=80, root_page=contact_page) christmas_page = Page.objects.get(url_path='/home/events/christmas/') @@ -182,20 +235,6 @@ class TestRedirects(TestCase): response = self.client.get('/xmas/', HTTP_HOST='other.example.com') self.assertRedirects(response, 'http://localhost/events/christmas/', status_code=301, fetch_redirect_response=False) - def test_duplicate_redirects_within_a_site_when_match_is_for_specific(self): - businessy_page = Page.objects.get(url_path='/home/events/businessy-events/') - site = Site.objects.create(hostname='other.example.com', port=80, root_page=businessy_page) - board_meetings_page = Page.objects.get(url_path='/home/events/businessy-events/board-meetings/') - christmas_page = Page.objects.get(url_path='/home/events/christmas/') - - # two redirects, one for any site, one for specific - models.Redirect.objects.create(old_path='/xmas', redirect_page=christmas_page) - models.Redirect.objects.create(site=site, old_path='/xmas', redirect_page=board_meetings_page) - - # request for specific site gets the meetings_page redirect, which is accessible from that site - response = self.client.get('/xmas/', HTTP_HOST='other.example.com') - self.assertRedirects(response, 'http://other.example.com/board-meetings/', status_code=301, fetch_redirect_response=False) - class TestRedirectsIndexView(TestCase, WagtailTestUtils): def setUp(self):