#19779 closed Bug (fixed)
Regression in redirects middleware
Reported by: | Aymeric Augustin | Owned by: | nobody |
---|---|---|---|
Component: | contrib.redirects | Version: | 1.5-rc-1 |
Severity: | Release blocker | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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
andid
attributes returningsettings.SITE_ID
Attachments (1)
Change History (11)
comment:1 by , 12 years ago
comment:2 by , 12 years ago
I searched for '.id' and '.pk' in the commit that introduced the regression.
I think the flatpages middleware is also affected, but nothing else.
comment:3 by , 12 years ago
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?
comment:4 by , 12 years ago
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.
comment:5 by , 12 years ago
Unlike the flatpages docs, the redirects docs don't say anything about contrib.sites.
comment:6 by , 12 years ago
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.
by , 12 years ago
Attachment: | 19779-1.diff added |
---|
comment:7 by , 12 years ago
Has patch: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:8 by , 12 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Is _ =
necessary in _ = RedirectFallbackMiddleware()
? Otherwise this looks perfect, thanks!
comment:9 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
The traceback is a bit different after 833ccd4b5b4b294e7ca8d32faa4461be7fafb13c but the underlying problem is the same: