Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#20619 closed Bug (invalid)

Missing call to super in `WSGIRequest.__init__()`

Reported by: loic84 Owned by:
Component: HTTP handling Version: master
Severity: Normal Keywords:
Cc: apollo13, tom@… Triage Stage: Someday/Maybe
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description


Change History (20)

comment:1 Changed 2 years ago by loic84

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

We can't just call the constructor, because setting the GET, POST, etc. in HttpRequest.__init__() will clash with the @property definitions inWSGIRequest.

That said, we need to ensure that future additions to HttpRequest.__init__() will also find their way toWSGIRequest.

comment:2 Changed 2 years ago by loic84

  • Has patch set

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

  1. 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

comment:3 Changed 2 years ago by loic84

  • Severity changed from Normal to Release blocker

comment:4 Changed 2 years ago by tomchristie

  • Cc tom@… added

comment:5 Changed 2 years ago by mjtamlyn

+1 to approach 1, but not confident enough in this area of the code base to merge it

comment:6 Changed 2 years ago 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.

comment:7 Changed 2 years ago 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?

comment:8 Changed 2 years ago 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.

comment:9 Changed 2 years ago 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.

comment:10 Changed 2 years ago by loic84

@tomchristie: You are absolutely right. I added another commit to the same branch that should address this issue.

comment:11 Changed 2 years ago by andrewgodwin

  • Owner changed from nobody to andrewgodwin
  • Status changed from new to assigned

comment:12 Changed 2 years ago 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.

comment:13 Changed 2 years ago by Loic Bistuer <loic.bistuer@…>

In 48ce167d895b7e2a9d00884a4a8679851fa890af:

Fixed missing initializations in WSGIRequest. Refs #20619

comment:14 Changed 2 years ago by Andrew Godwin <andrew@…>

In b21e96d00d12f8467d61b15a943994b303504e0e:

Merge pull request #1311 from loic/ticket20619_take2

Fixed missing initializations in WSGIRequest. Refs #20619

comment:15 Changed 2 years ago by loic84

  • Severity changed from Release blocker to Normal

comment:16 Changed 2 years ago by andrewgodwin

  • Owner andrewgodwin deleted
  • Status changed from assigned to new

comment:17 Changed 2 years ago by loic84

  • Triage Stage changed from Unreviewed to Accepted

comment:18 Changed 2 years ago by loic84

  • Easy pickings unset

comment:19 Changed 2 years ago by jezdez

  • Resolution set to invalid
  • Status changed from new to closed
  • Triage Stage changed from Accepted to Someday/Maybe

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

comment:20 Changed 2 years ago by tomchristie

@jezdez - Yup, seems like a good call to me. Thanks.

Note: See TracTickets for help on using tickets.
Back to Top