Denormalize from_url_without_rest onto the redirects model

This should make querying faster and the logic to support it easier

Author: ericholscher

readthedocs/redirects/admin.py

@@ -17,6 +17,7 @@ class RedirectAdmin(admin.ModelAdmin):
         'from_url',
         'to_url',
     )
+    readonly_fields = ('from_url_without_rest',)
 
 
 admin.site.register(Redirect, RedirectAdmin)

readthedocs/redirects/migrations/0004_denormalize-from-url.py

@@ -0,0 +1,20 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.26 on 2020-03-14 14:15
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('redirects', '0003_add_default_redirect_http_status_to_302'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='redirect',
+            name='from_url_without_rest',
+            field=models.CharField(blank=True, db_index=True, help_text='Only for internal querying use', max_length=255, null=True),
+        ),
+    ]

readthedocs/redirects/models.py

@@ -77,6 +77,14 @@ class Redirect(models.Model):
         blank=True,
     )
 
+    from_url_without_rest = models.CharField(
+        max_length=255,
+        db_index=True,
+        help_text='Only for internal querying use',
+        blank=True,
+        null=True,
+    )
+
     to_url = models.CharField(
         _('To URL'),
         max_length=255,
@@ -102,6 +110,11 @@ class Meta:
         verbose_name_plural = _('redirects')
         ordering = ('-update_dt',)
 
+    def save(self, *args, **kwargs):  # pylint: disable=arguments-differ
+        if '$rest' in self.from_url:
+            self.from_url_without_rest = self.from_url.replace('$rest', '')
+        super().save(*args, **kwargs)
+
     def __str__(self):
         redirect_text = '{type}: {from_to_url}'
         if self.redirect_type in ['prefix', 'page', 'exact']:

readthedocs/redirects/querysets.py

@@ -2,9 +2,8 @@
 
 import logging
 
-from django.db import models, DataError
-from django.db.models import Value, CharField, IntegerField, Q, F, ExpressionWrapper
-from django.db.models.functions import Substr, Length
+from django.db import models
+from django.db.models import Value, CharField, Q, F
 
 from readthedocs.core.utils.extend import SettingsOverrideObject
 
@@ -42,27 +41,6 @@ def get_redirect_path_with_status(self, path, full_path=None, language=None, ver
                 full_path,
                 output_field=CharField(),
             ),
-
-            from_url_length=ExpressionWrapper(
-                Length('from_url'),
-                output_field=IntegerField(),
-            ),
-
-            # 1-indexed
-            from_url_without_rest=Substr(
-                'from_url',
-                1,
-                F('from_url_length') - 5,  # Strip "$rest"
-                output_field=CharField(),
-            ),
-
-            # 1-indexed
-            full_path_without_rest=Substr(
-                'full_path',
-                1,
-                F('from_url_length') - 5,  # Strip "$rest"
-                output_field=CharField(),
-            ),
         )
         prefix = Q(
             redirect_type='prefix',
@@ -76,10 +54,7 @@ def get_redirect_path_with_status(self, path, full_path=None, language=None, ver
             Q(
                 redirect_type='exact',
                 from_url__endswith='$rest',
-                # This works around a bug in Django doing a substr and an endswith,
-                # so instead we do 2 substrs and an exact
-                # https://code.djangoproject.com/ticket/29155
-                full_path_without_rest=F('from_url_without_rest'),
+                full_path__startswith=F('from_url_without_rest'),
             ) | Q(
                 redirect_type='exact',
                 full_path__iexact=F('from_url'),
@@ -99,20 +74,7 @@ def get_redirect_path_with_status(self, path, full_path=None, language=None, ver
             path__endswith='.html',
         )
 
-        try:
-            # Using the ``exact`` Q object we created here could cause an
-            # execption because it uses ``from_url_without_rest`` and
-            # ``full_path_without_rest`` which could produce a database error
-            # ("negative substring length not allowed") when ``from_url``'s
-            # length is less than 5 ('$rest' string substracted from it). We
-            # perform a query using ``exact`` here and if it fail we catch it
-            # and just ignore these redirects for this project.
-            queryset = queryset.filter(prefix | page | exact | sphinx_html | sphinx_htmldir)
-            queryset.select_related('project').count()
-        except DataError:
-            # Fallback to the query without using ``exact`` on it
-            log.warning('Failing Exact Redirects on this project. Ignoring them.')
-            queryset = queryset.filter(prefix | page | sphinx_html | sphinx_htmldir)
+        queryset = queryset.filter(prefix | page | exact | sphinx_html | sphinx_htmldir)
 
         # There should be one and only one redirect returned by this query. I
         # can't think in a case where there can be more at this point. I'm