Regression in redirects middleware

[Aymeric Augustin]

After upgrading docs.djangoproject.com to 1.5 rc 1 static files fail to load.

Environment:


Request Method: GET
Request URL: http://localhost:8000/s/css/base.css/

Django Version: 1.5c1
Python Version: 2.7.2
Installed Applications:
['docs', 'haystack']
Installed Middleware:
['djangosecure.middleware.SecurityMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.contrib.flatpages.middleware.FlatpageFallbackMiddleware',
 'django.contrib.redirects.middleware.RedirectFallbackMiddleware']


Traceback:
File "/Users/myk/.virtualenvs/dp.com/lib/python2.7/site-packages/django/core/handlers/base.py" in get_response
  187.                 response = middleware_method(request, response)
File "/Users/myk/.virtualenvs/dp.com/lib/python2.7/site-packages/django/contrib/redirects/middleware.py" in process_response
  13.             r = Redirect.objects.get(site__id__exact=current_site.id, old_path=path)

Exception Type: AttributeError at /s/css/base.css/
Exception Value: 'RequestSite' object has no attribute 'id'

This regression was introduced in 6c2faaceb0482267cec19da0ff432984028f9d0c.

I'm not sure it's a good idea to use get_current_site(), which can return a RequestSite, in code that obviously needs a Site.

Possible solutions:

• rollback the change in the redirect middleware
• give RequestSite pk and id attributes returning settings.SITE_ID

[Aymeric Augustin]

The traceback is a bit different after 833ccd4b5b4b294e7ca8d32faa4461be7fafb13c but the underlying problem is the same:

Environment:


Request Method: GET
Request URL: http://localhost:8000/s/css/base.css/

Django Version: 1.5c1
Python Version: 2.7.2
Installed Applications:
['docs', 'haystack']
Installed Middleware:
['djangosecure.middleware.SecurityMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.contrib.flatpages.middleware.FlatpageFallbackMiddleware',
 'django.contrib.redirects.middleware.RedirectFallbackMiddleware']


Traceback:
File "/Users/myk/Documents/dev/django/django/core/handlers/base.py" in get_response
  187.                 response = middleware_method(request, response)
File "/Users/myk/Documents/dev/django/django/contrib/redirects/middleware.py" in process_response
  18.             r = Redirect.objects.get(site=current_site, old_path=full_path)
File "/Users/myk/Documents/dev/django/django/db/models/manager.py" in get
  143.         return self.get_query_set().get(*args, **kwargs)
File "/Users/myk/Documents/dev/django/django/db/models/query.py" in get
  379.         clone = self.filter(*args, **kwargs)
File "/Users/myk/Documents/dev/django/django/db/models/query.py" in filter
  655.         return self._filter_or_exclude(False, *args, **kwargs)
File "/Users/myk/Documents/dev/django/django/db/models/query.py" in _filter_or_exclude
  673.             clone.query.add_q(Q(*args, **kwargs))
File "/Users/myk/Documents/dev/django/django/db/models/sql/query.py" in add_q
  1259.                             can_reuse=used_aliases, force_having=force_having)
File "/Users/myk/Documents/dev/django/django/db/models/sql/query.py" in add_filter
  1190.                 connector)
File "/Users/myk/Documents/dev/django/django/db/models/sql/where.py" in add
  71.             value = obj.prepare(lookup_type, value)
File "/Users/myk/Documents/dev/django/django/db/models/sql/where.py" in prepare
  339.             return self.field.get_prep_lookup(lookup_type, value)
File "/Users/myk/Documents/dev/django/django/db/models/fields/related.py" in get_prep_lookup
  143.             return self._pk_trace(value, 'get_prep_lookup', lookup_type)
File "/Users/myk/Documents/dev/django/django/db/models/fields/related.py" in _pk_trace
  216.         v = getattr(field, prep_func)(lookup_type, v, **kwargs)
File "/Users/myk/Documents/dev/django/django/db/models/fields/__init__.py" in get_prep_lookup
  321.             return self.get_prep_value(value)
File "/Users/myk/Documents/dev/django/django/db/models/fields/__init__.py" in get_prep_value
  554.         return int(value)

Exception Type: TypeError at /s/css/base.css/
Exception Value: int() argument must be a string or a number, not 'RequestSite'

[Aymeric Augustin]

I searched for '.id' and '.pk' in the commit that introduced the regression.

I think the flatpages middleware is also affected, but nothing else.

[Claude Paroz]

The flatpages contrib docs explicitely require that contrib.sites should be installed, so I think we are safe on this side.

Looking at the redirect model, I see:

...
from django.contrib.sites.models import Site
...

class Redirect(models.Model):
    site = models.ForeignKey(Site)
...

How could that work without the sites framework being installed?

[Aymeric Augustin]

Currently Django doesn't forbid importing models from apps that aren't in INSTALLED_APPS.

Agreed, that's dubious, and we should probably fix it in app-loading.

[Aymeric Augustin]

Unlike the flatpages docs, the redirects docs don't say anything about contrib.sites.

[Aymeric Augustin]

After discussing the issue on IRC, I suggest to check in RedirectFallbackMiddleware.__init__ that django.contrib.sites is in INSTALLED_APPS — this allows us to keep the current implementation, a step towards #15089, and to show a clear error message at compile time rather than the error I posted above at run time.

It's still backwards-incompatible for people who enabled d.c.redirects but not d.c.sites. So it should be documented in the release notes as a backwards incompatible change:

Because of an implementation oversight, it was possible to use django.contrib.redirects without enabling django.contrib.sites. This isn't allowed any longer. If you're using d.c.redirects, make sure INSTALLED_APPS contains django.contrib.sites.

[Aymeric Augustin]

Is _ = necessary in _ = RedirectFallbackMiddleware()? Otherwise this looks perfect, thanks!

[Claude Paroz <claude@…>]

In 2ed90eac49b99d66ee4f2d59af8553274a4d095f:

Fixed #19779 -- Checked contrib.sites presence in RedirectFallbackMiddleware

Thanks Aymeric Augustin for the report and directions for the patch.

[Claude Paroz <claude@…>]

In b8c6de31a6f08321a6cf6e3ede2dab90c9f1febd:

[1.5.x] Fixed #19779 -- Checked contrib.sites presence in RedirectFallbackMiddleware

Thanks Aymeric Augustin for the report and directions for the patch.
Backport of 2ed90eac from master.