[Django] #20619: Missing call to super in `WSGIRequest.__init__()`
Django
-------------------------------+--------------------
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+--------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/20619>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by loic84):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
We can't just call the constructor, because setting the GET, POST, etc. in
`HttpRequest.__init__()` will clash with the @property definitions
in`WSGIRequest`.
That said, we need to ensure that future additions to
`HttpRequest.__init__()` will also find their way to`WSGIRequest`.
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:1>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by loic84):
* has_patch: 0 => 1
Comment:
I've got two solutions to offer:
1. The heavy machinery which enables WSGIRequest to call `super` in
`__init__()`:
https://github.com/loic/django/compare/ticket20619
2. A manual fix for the missing var initialization and a comment to
hopefully prevent this from happening again.
https://github.com/loic/django/compare/ticket20619_take2
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:2>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by loic84):
* severity: Normal => Release blocker
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:3>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by tomchristie):
* cc: tom@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:4>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by mjtamlyn):
+1 to approach 1, but not confident enough in this area of the code base
to merge it
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:5>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by apollo13):
I am not that happy with approach one since it:
* Adds stuff to HttpRequest which it doesn't need at all since it only
uses dicts
* Introduces an extra function call to __every__ GET/POST/etc access
That said, I don't have any better ideas.
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:6>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by loic84):
To be honest, I'm not totally happy showing up on git blame for such a
hack, that said it's the most compatible fix I've found.
Do we really need `WSGIRequest` to use properties? The comment in
`HttpRequest.encoding` seems to hint that we do, but I can't figure out
when it comes into play. Anyone familiar with this area of Django?
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:7>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by loic84):
I came up with a 3rd option:
A refactor of `HttpRequest` and `WSGIRequest` that removes all hacks
specific to `WSGIRequest` from `HttpRequest`:
https://github.com/loic/django/compare/ticket20619_take3
That's my favorite option by a big margin.
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:8>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by tomchristie):
@loic84 - It looks to me like that would change behavior. The lazy
evaluation of `request.POST`/`request.FILES` is important for cases such
as setting `request.upload_handlers`, as well as the effecting the
behavior of accessing `request.read()`. That change looks like it'd cause
`request.POST`/`request.FILES` to be loaded if `request.GET` is accessed,
which is problematic. In other words, accessing `request.GET` shouldn't
cause the request stream to be consumed and parsed.
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:9>
Django
Reporter: loic84 | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by loic84):
@tomchristie: You are absolutely right. I added another commit to the same
branch that should address this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:10>
Django
Reporter: loic84 | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by andrewgodwin):
* owner: nobody => andrewgodwin
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:11>
Django
Reporter: loic84 | Owner: andrewgodwin
Type: Bug | Status: assigned
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by loic84):
PR https://github.com/django/django/pull/1311
This should be enough to drop the priority on this ticket in view of
1.6b1. Note that the commit message won't close the ticket, we'll still
need a long term solution for the missing call to `super`.
While I'm at it, I'll mention that
https://github.com/loic/django/compare/ticket20619_take3 addresses two
outstanding issues:
- We initialize a mutable at the class level in
`HttpRequest._upload_handlers`.
- `WSGIRequest.REQUEST` would go stale if you access it and then change
the encoding.
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:12>
Django
Reporter: loic84 | Owner: andrewgodwin
Type: Bug | Status: assigned
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Loic Bistuer <loic.bistuer@…>):
In [changeset:"48ce167d895b7e2a9d00884a4a8679851fa890af"]:
{{{
#!CommitTicketReference repository=""
revision="48ce167d895b7e2a9d00884a4a8679851fa890af"
Fixed missing initializations in WSGIRequest. Refs #20619
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:13>
Django
Reporter: loic84 | Owner: andrewgodwin
Type: Bug | Status: assigned
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Andrew Godwin <andrew@…>):
In [changeset:"b21e96d00d12f8467d61b15a943994b303504e0e"]:
{{{
#!CommitTicketReference repository=""
revision="b21e96d00d12f8467d61b15a943994b303504e0e"
Merge pull request #1311 from loic/ticket20619_take2
Fixed missing initializations in WSGIRequest. Refs #20619
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:14>
Django
Reporter: loic84 | Owner: andrewgodwin
Type: Bug | Status: assigned
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by loic84):
* severity: Release blocker => Normal
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:15>
Django
Reporter: loic84 | Owner:
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by andrewgodwin):
* owner: andrewgodwin =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:16>
Django
Reporter: loic84 | Owner:
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by loic84):
* stage: Unreviewed => Accepted
Comment:
Rebased https://github.com/loic/django/compare/ticket20619_take3 to
account for [48ce167d895b7e2a9d00884a4a8679851fa890af].
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:17>
Django
Reporter: loic84 | Owner:
Type: Bug | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------+------------------------------------
Changes (by loic84):
* easy: 1 => 0
Comment:
PR https://github.com/django/django/pull/1335
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:18>
Django
Reporter: loic84 | Owner:
Type: Bug | Status: closed
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jezdez):
* status: new => closed
* resolution: => invalid
* stage: Accepted => Someday/Maybe
Comment:
I've talked to loic84 on IRC and think there is no evidence that
refactoring this piece of code will lead to improving the code. The call
to super may be missing, but there is no feature on the horizon to my
knowledge (and to loic84's knowledge) that would require it.
Given the central role the HttpRequest class plays in *any application*
I'm closing this ticket as invalid and ask to reopen it if an actual use
case presents itself to make a sensible decision of the usefulness of this
refactor.
TLDR; uneeded code churn with high risk
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:19>
Django
Reporter: loic84 | Owner:
Type: Bug | Status: closed
Keywords: | Triage Stage: Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+-----------------------------------------
Comment (by tomchristie):
@jezdez - Yup, seems like a good call to me. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/20619#comment:20>