Opened 11 years ago

Last modified 7 years ago

#21381 assigned New feature

Remove contrib.redirects dependency on contrib.sites

Reported by: Aymeric Augustin Owned by: Dmitry Pechnikov
Component: contrib.redirects Version: dev
Severity: Normal Keywords:
Cc: loic@…, Paul Oswald, cmawebsite@…, Dmitry Pechnikov Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Most websites aren't served on multiple domains. It'd be nice if redirects didn't depend on sites.

The Redirect model would omit the ForeignKey to Site entirely when the sites framework isn't installed. Redirections would use a RequestSite instead of a Site.

At first sight it looks like this can be achieved without breaking backwards-compatibility, as we're lifting a constraint.

Users will have to generate a migration to add / remove the ForeignKey if the enable / disable sites while redirects in installed.

Change History (20)

comment:1 by Anssi Kääriäinen, 11 years ago

Triage Stage: UnreviewedAccepted

If this is easy to do, why not.

comment:2 by Aymeric Augustin, 11 years ago

Has patch: set
Needs tests: set

Here's a pull request: https://github.com/django/django/pull/1885

Unfortunately, it's hard to add tests. The django_redirect table is initially created with a site_id column, which doesn't allow testing the feature this patch is adding.

The least awful idea I have is to disable sites, monkey-patch Site._meta.db_table, call syncdb to create the new table, and test with that.

Last edited 7 years ago by Tim Graham (previous) (diff)

comment:3 by Aymeric Augustin, 11 years ago

It turned out to be an utterly awful idea, because it requires heavy surgery on the Redirect model to remove the site field.

I gave up at that point. (The following diff doesn't work at all.)

diff --git a/django/contrib/redirects/tests.py b/django/contrib/redirects/tests.py
index dd3c0ac..e14c610 100644
--- a/django/contrib/redirects/tests.py
+++ b/django/contrib/redirects/tests.py
@@ -2,6 +2,7 @@ from django import http
 from django.conf import settings
 from django.contrib.sites.models import Site
 from django.core.exceptions import ImproperlyConfigured
+from django.core.management import call_command
 from django.test import TestCase
 from django.test.utils import override_settings
 from django.utils import six
@@ -57,6 +58,43 @@ class RedirectTests(TestCase):
         self.assertEqual(response.status_code, 410)
 
 
+@override_settings(
+    APPEND_SLASH=False,
+    INSTALLED_APPS=[app for app in settings.INSTALLED_APPS
+                        if app != 'django.contrib.sites'],
+    MIDDLEWARE_CLASSES=list(settings.MIDDLEWARE_CLASSES) +
+    ['django.contrib.redirects.middleware.RedirectFallbackMiddleware'],
+)
+class RedirectsWithoutSitesTests(TestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        Site._meta.installed = False
+        cls.redirect_site_field = Redirect._meta.fields.pop(1)
+        Redirect._meta.db_table = 'django_redirect_without_site'
+        Redirect._meta.unique_together = ()
+        call_command("migrate", verbosity=0)
+
+    @classmethod
+    def tearDownClass(cls):
+        Site._meta.installed = True
+        Redirect._meta.fields.insert(1, cls.redirect_site_field)
+        Redirect._meta.db_table = 'django_redirect'
+        Redirect._meta.unique_together = (('site', 'old_path'),)
+
+    def test_model(self):
+        r1 = Redirect.objects.create(
+            old_path='/initial', new_path='/new_target')
+        self.assertEqual(six.text_type(r1), "/initial ---> /new_target")
+
+    def test_redirect(self):
+        Redirect.objects.create(
+            old_path='/initial', new_path='/new_target')
+        response = self.client.get('/initial')
+        self.assertRedirects(response,
+            '/new_target', status_code=301, target_status_code=404)
+
+
 class OverriddenRedirectFallbackMiddleware(RedirectFallbackMiddleware):
     # Use HTTP responses different from the defaults
     response_gone_class = http.HttpResponseForbidden

We have to choose between rejecting this ticket or committing the patch without tests. redirects isn't a very complicated app...

comment:4 by Claude Paroz, 11 years ago

Looking quickly at your patch, I think that the call if Site._meta.installed at module import time (to define class attribute) is a bit fragile and might be dependent on the import order. But I didn't test anything yet...

comment:5 by Tim Graham, 11 years ago

My main concern is that it's not clear how migrations for contrib apps are designed to work. In particular, they are excluded from autodetection and obviously user-generated migrations cannot be stored in django/contrib/<app>/migrations/. I'm guessing the MIGRATION_MODULES setting may be of use here, but it's not even documented yet, so I think this change should wait until Andrew makes some more progress.

comment:6 by Aymeric Augustin, 11 years ago

Patch needs improvement: set

Claude : yes, this doesn't look very robust. The answer could be a swappable model, as suggested by loic84 on IRC.

Tim: yes, my docs are very hand-wavy. I couldn't figure out how to create such a migration.

I think we can work something out but this needs more work.

comment:8 by Paul Oswald, 11 years ago

Just a thought here: we've been bitten by the fact that sites depends on a config setting for SITE_ID (which is the pk of the table) which it then uses to look up the data from a table. This means your config file has to be kept in sync with the table's id's.

I think instead of having Site._meta.installed it should just be a configuration setting with a class name which can then be overridden by the developer. The default for new projects can be a the RequestSite object. In my case I was looking to create a class that simply maps to a dict() object in the settings file (rather than depend on a table or the upstream request) but there's currently no clean way to hook it in.

comment:9 by Paul Oswald, 11 years ago

Cc: Paul Oswald added

comment:10 by Collin Anderson, 10 years ago

Cc: cmawebsite@… added
Summary: Remove dependency on contrib.sitesRemove contrib.redirects dependency on contrib.sites

comment:11 by Dmitry Pechnikov, 7 years ago

Cc: Dmitry Pechnikov added

Is there a reason loic84's idea wasn't accepted? I'd really like to remove this dependency and looking into details. Just want to make sure I isn't for nothing :)

comment:12 by Dmitry Pechnikov, 7 years ago

Owner: changed from nobody to Dmitry Pechnikov
Status: newassigned

comment:13 by Dmitry Pechnikov, 7 years ago

comment:14 by Dmitry Pechnikov, 7 years ago

Needs documentation: set
Needs tests: unset
Patch needs improvement: unset

comment:15 by Dmitry Pechnikov, 7 years ago

I'd really appreciate PR review before updating documentation.

comment:16 by Dmitry Pechnikov, 7 years ago

Needs documentation: unset
Patch needs improvement: set

comment:17 by Dmitry Pechnikov, 7 years ago

Patch needs improvement: unset

comment:18 by Marten Kenbeek, 7 years ago

Patch needs improvement: set

comment:19 by Dmitry Pechnikov, 7 years ago

Patch needs improvement: unset

comment:20 by Carlton Gibson, 7 years ago

Needs tests: set
Patch needs improvement: set

Patch in PR looks OK in outline but needs tests demonstrating that the migrations work as hoped in all cases.

Note: See TracTickets for help on using tickets.
Back to Top