[Django] #35170: LocaleMiddleware unexpectedly causes messages to be consumed under certain circumstances
Django
circumstances
------------------------------------------------+------------------------
Reporter: Sylvain Fankhauser | Owner: nobody
Type: Bug | Status: new
Component: Internationalization | Version: 5.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
Adding a message (with the messages contrib module) to the request and
then redirecting the user to an internationalized route without the
language prefix and a custom 404 template that consumes messages will
consume messages from the request, even though no 404 response is sent
back to the client.
I have set up a minimal reproduction project here:
https://github.com/sephii/django-localemiddleware-bug
To reproduce this problem, you’ll need:
1. An internationalized route (eg. `urlpatterns +=
i18n_patterns(path("foo/", views.foo))`)
2. A view that adds a message to the request (eg.
`messages.success(request, "Hello world")`) and then redirects to the
internationalized route without a language prefix
3. A 404.html template that consumes the request messages (eg. `{% for
message in messages %}{{ message }}{% endfor %}`)
Visiting the view defined in point 2 will result in a 302 to /foo/, which,
because of the 404.html template, will consume the message added by the
view in point 2 before redirecting to /en/foo/ (which won’t have any
message left to consume).
The bug is not in LocaleMiddleware per se, but is related to the way
LocaleMiddleware works: by catching any 404 and checking if it matches an
internationalized route, and if so redirecting to the correct route, but
at this point the 404 response has been rendered.
Ideally the only response Django should create is the redirect, and no 404
should be rendered.
--
Ticket URL: <https://code.djangoproject.com/ticket/35170>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
circumstances
Reporter: Sylvain Fankhauser | Owner: nobody
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Natalia Bidart):
* status: new => closed
* type: Bug => Uncategorized
* version: 5.0 => dev
* resolution: => invalid
Comment:
Hello Sylvain Fankhauser!
Thank you for your interest in making Django better. I took a close look
at this report, I cloned the reproducer repo and followed your steps. I do
see what you are describing here, but I also noted a few cases that raise
concerns:
* In my experience, 404 templates should ideally be as static as possible.
Consuming/showing the messages from the request feels like a code smell.
* Redirecting to a hard-coded URL is an anti-pattern; ideally, the code
would use proper calls to `reverse`.
* The URL `/foobar/` is clearly a 404, so manually redirecting to this URL
is another unexpected aspect of the code. If hard-coding is essential, why
not redirecting to `/<lang>/foobar/`?
I made this slight change to the given code:
{{{#!diff
diff --git a/testdjango/urls.py b/testdjango/urls.py
index c89638c..088a45d 100644
--- a/testdjango/urls.py
+++ b/testdjango/urls.py
@@ -17,12 +17,12 @@ from django.conf.urls.i18n import i18n_patterns
from django.contrib import messages
from django.http import HttpResponse
from django.shortcuts import redirect
-from django.urls import path
+from django.urls import path, reverse
def home(request):
messages.error(request, "Hello world")
- return redirect("/foobar/")
+ return redirect(reverse("foobar"))
def foobar(request):
@@ -35,5 +35,5 @@ urlpatterns = [
]
urlpatterns += i18n_patterns(
- path("foobar/", foobar),
+ path("foobar/", foobar, name="foobar"),
)
}}}
and messages are not consumed any longer, because there is no 404 being
raised anywhere in the call chain. Honestly I don't see where Django is at
fault here, the way the i18n URLs are resolved involves raising a
`Resolver404` exception which triggers the creation of a
`HttpResponseNotFound` instance (which needs the template rendered to be
created).
I'll closing as `invalid` but if you have more information or if you
disagree with the resolution, please reach out to the
[https://forum.djangoproject.com/c/users/6 Django forum].
--
Ticket URL: <https://code.djangoproject.com/ticket/35170#comment:1>
Django
circumstances
-------------------------------------+-------------------------------------
Reporter: Sylvain Fankhauser | Owner: nobody
Type: Uncategorized | Status: closed
Component: | Version: dev
Internationalization |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Hello Natalia,
Thank you for taking the time to review the issue.
The reproduction code is indeed not using reverse on purpose, to mimic the
behaviour of `redirect_to_login` (which I’m actually using in my code,
instead of the `redirect(reverse("foobar"))` I put in the reproduction
code). `redirect_to_login` redirects to `settings.LOGIN_URL`, which
doesn’t have any language prefix and causes the initial 404. Perhaps the
proper fix in my case is to use a named URL for `settings.LOGIN_URL`
instead of the default, so that the language prefix is included in the URL
and no 404 is rendered.
I’m fine with keeping this as `invalid` as I don’t really see what else
could be done (except maybe change the default `settings.LOGIN_URL` to use
a named URL?).
--
Ticket URL: <https://code.djangoproject.com/ticket/35170#comment:2>