[Django] #26293: Warnings regarding 404s logged for URLs missing trailing slashes
Django
-------------------------+-------------------------------------------------
Reporter: jklaiho | Owner: nobody
Type: Bug | Status: new
Component: HTTP | Version: 1.9
handling | Keywords: CommonMiddleware 404 logging
Severity: Normal | regression
Triage Stage: | Has patch: 0
Unreviewed |
Easy pickings: 1 | UI/UX: 0
-------------------------+-------------------------------------------------
I recently deployed a project into production using Django 1.9.2 and
started getting strange logged warning messages from Sentry for 404's.
Looking into it, this occurred in
`django.core.handers.base.BaseHandler.get_response` and was related to
people visiting URLs without trailing slashes.
I compared the behaviour against an earlier, similarly configured project
still running Django 1.7.x and this didn't occur there.
Digging deeper, it seems that commit
[https://github.com/django/django/commit/434d309ef6dbecbfd2b322d3a1da78aa5cb05fa8
#diff-1f8be0eae49a1bf37d52829eaeda6a4e 434d309e] to fix [ticket:24720]
inside `CommonMiddleware` was causing this. In lines 56-66 (58-68 in
1.9.2), the path is only checked for a missing slash if the prerequisites
for `PREPEND_WWW` processing are met, since they are indented beneath it.
This doesn't really make sense, since the two settings are not
interdependent.
As a result, an `Http404` is raised after request middleware processing in
`BaseHandler.get_response`, at which point a warning is logged—for every
single request for a path without a trailing slash.
`APPEND_SLASH` still takes effect eventually, but only when
`CommonMiddleware` is called again, this time for `process_response`,
whereupon the normal redirect gets done. Thanks to this, everything
appears to function normally for the end user, but unnecessary 404
warnings end up getting logged. (Though if `APPEND_SLASH` was `False`,
you'd probably want them logged.)
It seems to me that `CommonMiddleware.process_request` needs a bit of
reworking to run the checks for `PREPEND_WWW` and `APPEND_SLASH`
independently, and to determine the need for a redirect based on whether
at least one of these is necessary, still fulfilling the purpose of
[ticket:24720]. I've provisionally marked the ticket as "easy pickings",
as it seems to be that way from my admittedly limited research into this.
--
Ticket URL: <https://code.djangoproject.com/ticket/26293>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jklaiho | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
404 logging regression | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by jklaiho):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
A further note: I'm naturally using URL reversing judiciously in all of my
templates so that the URLs that people actually click on have trailing
slashes already in place, but for some reason some people still type some
URLs by hand, leaving out the trailing slashes.
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:1>
Django
Reporter: jklaiho | Owner: the-kid89
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.9
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
404 logging regression | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by the-kid89):
* owner: nobody => the-kid89
* status: new => assigned
Old description:
New description:
I took a look over it and you are correct that APPEND_SLASH wil not run
unless PREPEND_WWW is also set. =
--
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:2>
Django
Reporter: jklaiho | Owner: the-kid89
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.9
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
404 logging regression | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by the-kid89):
* cc: the-kid89 (added)
Old description:
> I took a look over it and you are correct that APPEND_SLASH wil not run
> unless PREPEND_WWW is also set. =
>
New description:
--
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:3>
Django
Reporter: jklaiho | Owner: the-kid89
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.9
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
404 logging regression | Unreviewed
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by the-kid89):
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
I have created a fix: https://github.com/the-
kid89/django/tree/ticket_26293
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:4>
Django
Reporter: jklaiho | Owner: the-kid89
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.9
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
404 logging regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by the-kid89):
Pull request: https://github.com/django/django/pull/6227
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:5>
Django
Reporter: jklaiho | Owner: the-kid89
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.9
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage:
404 logging regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by jklaiho):
I haven't tested your changes, but by my reading of the code, it looks
incorrect. After your unindentation, `process_request` now '''always'''
returns a `HttpResponsePermanentRedirect`, as long as the forbidden UA
check at the top passes. It should only do that when either the slash has
been appended or "www" has been prepended.
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:6>
Django
Reporter: jklaiho | Owner: the-kid89
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.9
Severity: Normal | Resolution:
404 logging regression |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:7>
Django
Reporter: jklaiho | Owner: the-kid89
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.9
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage: Accepted
404 logging regression |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by the-kid89):
You are correct. My patch just breaks more things. My guess for the
logging is because it first has to 404 before it tries APPEND_SLASH. I
will have to look into this much further then I thought I would.
Replying to [comment:6 jklaiho]:
> I haven't tested your changes, but by my reading of the code, it looks
incorrect. After your unindentation, `process_request` now '''always'''
returns a `HttpResponsePermanentRedirect`, as long as the forbidden UA
check at the top passes. It should only do that when either the slash has
been appended or "www" has been prepended.
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:8>
Django
Reporter: jklaiho | Owner:
Type: Bug | Status: new
Component: HTTP handling | Version: 1.9
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage: Accepted
404 logging regression |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by the-kid89):
* status: assigned => new
* owner: the-kid89 =>
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:9>
Django
Reporter: jklaiho | Owner:
| ieatkittens
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.9
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage: Accepted
404 logging regression |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by ieatkittens):
* owner: => ieatkittens
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:10>
Django
Reporter: jklaiho | Owner:
| ieatkittens
Type: Bug | Status: assigned
Component: HTTP handling | Version: 1.9
Severity: Normal | Resolution:
Keywords: CommonMiddleware | Triage Stage: Accepted
404 logging regression |
Has patch: 1 | Needs documentation: 0
Changes (by ieatkittens):
* needs_tests: 1 => 0
Comment:
PR Submitted including regression test. Tests pass and docs build without
issue.
patch: https://github.com/ieatkittens/django/tree/ticket_26293
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:11>
Django
Reporter: jklaiho | Owner:
| ieatkittens
Component: HTTP handling | Version: 1.9
Keywords: CommonMiddleware | Triage Stage: Accepted
404 logging regression |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"9390da7fb6e251eaa9a785692f987296cb14523f" 9390da7]:
{{{
#!CommitTicketReference repository=""
revision="9390da7fb6e251eaa9a785692f987296cb14523f"
Fixed #26293 -- Fixed CommonMiddleware to process PREPEND_WWW and
APPEND_SLASH independently.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:12>
Django
Reporter: jklaiho | Owner:
| ieatkittens
Type: Bug | Status: closed
Component: HTTP handling | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: CommonMiddleware | Triage Stage: Accepted
404 logging regression |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"ccc367fd48655b8709a01653b224e5ffa19c9dee" ccc367fd]:
{{{
#!CommitTicketReference repository=""
revision="ccc367fd48655b8709a01653b224e5ffa19c9dee"
[1.9.x] Fixed #26293 -- Fixed CommonMiddleware to process PREPEND_WWW and
APPEND_SLASH independently.
Backport of 9390da7fb6e251eaa9a785692f987296cb14523f from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26293#comment:13>