Always try to append a slash (#37)

* Normalize URLs in model; Always try appending a slash
* unquote url before for cache invalidation
* Simplify test cases
* Explain leading/trailing slashes in docs

Co-authored-by: Iacopo Spalletti <[email protected]>

djangocms_redirect/admin.py

@@ -4,6 +4,7 @@
 from django.utils.translation import get_language
 
 from .models import Redirect
+from .utils import normalize_url
 
 
 class RedirectForm(ModelForm):
@@ -18,6 +19,9 @@ def __init__(self, *args, **kwargs):
         self.fields["old_path"].widget = widget
         self.fields["new_path"].widget = widget
 
+    def clean_old_path(self):
+        return normalize_url(self.cleaned_data.get("old_path"))
+
 
 @admin.register(Redirect)
 class RedirectAdmin(admin.ModelAdmin):

djangocms_redirect/middleware.py

@@ -8,7 +8,7 @@
 from django.core.exceptions import ImproperlyConfigured
 from django.db.models import Q
 from django.utils.deprecation import MiddlewareMixin
-from django.utils.http import urlunquote_plus
+from django.utils.encoding import escape_uri_path, iri_to_uri
 
 from .models import Redirect
 from .utils import get_key_from_path_and_site
@@ -32,22 +32,30 @@ def do_redirect(self, request, response=None):
         if getattr(settings, "DJANGOCMS_REDIRECT_404_ONLY", True) and response and response.status_code != 404:
             return response
 
-        full_path_quoted, part, querystring = request.get_full_path().partition("?")
-        possible_paths = [full_path_quoted]
-        full_path_unquoted = urlunquote_plus(full_path_quoted)
-        if full_path_unquoted != full_path_quoted:
-            possible_paths.append(urlunquote_plus(full_path_unquoted))
-        if not settings.APPEND_SLASH and not request.path.endswith("/"):
-            full_path_slash, __, __ = request.get_full_path(force_append_slash=True).partition("?")
-            possible_paths.append(full_path_slash)
-            full_path_slash_unquoted = urlunquote_plus(full_path_slash)
-            if full_path_slash_unquoted != full_path_slash:
-                possible_paths.append(full_path_slash_unquoted)
-        querystring = "{}{}".format(part, querystring)
+        req_path = request.path
+        # get the query string
+        querystring = request.META.get("QUERY_STRING", "")
+        if querystring:
+            querystring = "?%s" % iri_to_uri(querystring)
+        # start with the path as is
+        possible_paths = [req_path]
+        # add the unquoted path if it differs
+        req_path_quoted = escape_uri_path(req_path)
+        if req_path_quoted != req_path:
+            possible_paths.append(req_path_quoted)
+        # if a slash is missing, try to append it
+        if not req_path.endswith("/"):
+            req_path_slash = req_path + "/"
+            possible_paths.append(req_path_slash)
+            req_path_slash_quoted = escape_uri_path(req_path_slash)
+            if req_path_slash_quoted != req_path_slash:
+                possible_paths.append(req_path_slash_quoted)
+
         current_site = get_current_site(request)
         r = None
-        key = get_key_from_path_and_site(full_path_quoted, settings.SITE_ID)
+        key = get_key_from_path_and_site(req_path, settings.SITE_ID)
         cached_redirect = cache.get(key)
+
         if not cached_redirect:
             for path in possible_paths:
                 filters = dict(site=current_site, old_path=path)
@@ -58,6 +66,7 @@ def do_redirect(self, request, response=None):
                     r = self._match_substring(path)
                     if r:
                         break
+
             cached_redirect = {
                 "site": settings.SITE_ID,
                 "redirect": r.new_path if r else None,

djangocms_redirect/models.py

@@ -1,10 +1,14 @@
+from urllib.parse import unquote_plus
+
 from django.contrib.sites.models import Site
 from django.core.cache import cache
 from django.db import models
 from django.db.models.signals import post_delete, post_save
 from django.dispatch import receiver
 from django.utils.translation import ugettext_lazy as _
 
+from .utils import normalize_url
+
 RESPONSE_CODES = (
     ("301", _("301 - Permanent redirection")),
     (
@@ -59,6 +63,10 @@ class Meta:
         unique_together = (("site", "old_path"),)
         ordering = ("old_path",)
 
+    def clean(self):
+        self.old_path = normalize_url(self.old_path)
+        super().clean()
+
     def __str__(self):
         return "{} ---> {}".format(self.old_path, self.new_path)
 
@@ -68,5 +76,6 @@ def __str__(self):
 def clear_redirect_cache(**kwargs):
     from .utils import get_key_from_path_and_site
 
-    key = get_key_from_path_and_site(kwargs["instance"].old_path, kwargs["instance"].site_id)
+    path = unquote_plus(kwargs["instance"].old_path)
+    key = get_key_from_path_and_site(path, kwargs["instance"].site_id)
     cache.delete(key)

djangocms_redirect/utils.py

@@ -1,5 +1,7 @@
 import hashlib
 
+from django.conf import settings
+
 
 def get_key_from_path_and_site(path, site_id):
     """
@@ -12,3 +14,11 @@ def get_key_from_path_and_site(path, site_id):
     hashed_path = hashlib.sha224(path.encode("utf-8")).hexdigest()
     key = "CMSREDIRECT:{}:{}".format(hashed_path, site_id)
     return key
+
+
+def normalize_url(path):
+    if settings.APPEND_SLASH and not path.endswith("/"):
+        path = "%s/" % path
+    if not path.startswith("/"):
+        path = "/%s" % path
+    return path

docs/usage.rst

@@ -12,6 +12,17 @@ For each redirect you must provide:
 * **Redirect to**: The path to which the request will be redirected: you can type any URL or select an existing django CMS page.
 * **Response code**: You can select 3 types of status_code header: 301 (permanent redirect), 302 (temporary redirect) or 410 (permanent unavailable resource).
 
+Each **redirect from** URL must be unique and start with a slash. If you leave out the
+leading slash when creating a redirect, it is added automatically.
+
+If the user requests a page without a trailing slash and there is no redirect for that
+URL but there is one for the URL with a trailing slash, that redirect is used. For
+backwards-compatibility, this is also true when ``APPEND_SLASH=False``.
+
+If ``APPEND_SLASH=True`` (the default), a trailing slash is added automatically when
+creating a redirect. That way, there is only ever a single relevant redirect,
+whether there is a trailing slash or not.
+
 ****************
 Subpath matching
 ****************

tests/test_middleware.py

@@ -4,6 +4,7 @@
 from django.utils.encoding import force_text
 from django.utils.http import urlunquote_plus
 
+from djangocms_redirect.admin import RedirectForm
 from djangocms_redirect.middleware import RedirectMiddleware
 from djangocms_redirect.models import Redirect
 
@@ -208,6 +209,22 @@ def test_redirect_no_append_slash(self):
                 response = self.client.get(pages[1].get_absolute_url().rstrip("/"))
             self.assertRedirects(response, redirect.new_path, status_code=301)
 
+    def test_redirect_no_append_slash_unquoted(self):
+        pages = self.get_pages()
+
+        original_path = "/path (escaped)/"
+        with override_settings(APPEND_SLASH=False):
+            redirect = Redirect.objects.create(
+                site=self.site_1,
+                old_path=original_path,
+                new_path=pages[0].get_absolute_url(),
+                response_code="301",
+            )
+
+            with self.assertNumQueries(5):
+                response = self.client.get(original_path.rstrip("/"))
+            self.assertRedirects(response, redirect.new_path, status_code=301)
+
     def test_redirect_no_append_slash_quoted(self):
         pages = self.get_pages()
 
@@ -220,7 +237,7 @@ def test_redirect_no_append_slash_quoted(self):
                 response_code="301",
             )
 
-            with self.assertNumQueries(5):
+            with self.assertNumQueries(7):
                 response = self.client.get(original_path.rstrip("/"))
             self.assertRedirects(response, redirect.new_path, status_code=301)
 
@@ -319,6 +336,43 @@ def test_slash_append_slash(self):
             self.assertRedirects(response, "/en/b/", status_code=301)
 
 
+class TestClean(BaseRedirectTest):
+
+    _pages_data = (
+        {"en": {"title": "home page", "template": "page.html", "publish": True}},
+    )
+
+    def _make_form(self, old_path):
+        pages = self.get_pages()
+        return RedirectForm({
+            "site": self.site_1.pk,
+            "old_path": old_path,
+            "new_path": pages[0].get_absolute_url(),
+            "response_code": "301",
+        })
+
+    def _assert_clean(self, old_path, old_path_cleaned):
+        redirect_form = self._make_form(old_path)
+        redirect = redirect_form.save()
+        self.assertEqual(redirect.old_path, old_path_cleaned)
+
+    def test_clean_keep_slash(self):
+        self._assert_clean("/slash-bar/", "/slash-bar/")
+
+    def test_clean_prepend_slash(self):
+        self._assert_clean("slash-test/", "/slash-test/")
+
+    def test_clean_single_slash_root(self):
+        self._assert_clean("/", "/")
+
+    def test_clean_append_slash(self):
+        with override_settings(APPEND_SLASH=True):
+            self._assert_clean("/slash-foo", "/slash-foo/")
+
+    def test_clean_no_append_slash(self):
+        with override_settings(APPEND_SLASH=False):
+            self._assert_clean("/slash-baz", "/slash-baz")
+
 class TestPartialMatch(BaseRedirectTest):
     _pages_data = (
         {"en": {"title": "home page", "template": "page.html", "publish": True}},