Code

Opened 5 months ago

Closed 5 months ago

#21447 closed Bug (fixed)

post parse error no longer correctly represented.

Reported by: tomchristie Owned by: nobody
Component: HTTP handling Version: master
Severity: Release blocker Keywords:
Cc: bmispelon Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

This line was unintentionally commented out: https://github.com/django/django/blob/master/django/http/request.py#L245 as part of ticket #21189

Attachments (0)

Change History (3)

comment:1 Changed 5 months ago by tomchristie

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

Pull request is https://github.com/django/django/pull/1924

As it happens, even with _mark_post_parse_error() commented out, the build_request_repr *does* correctly mark <could not parse>, *at a least* in the parse error case I've provided, as request.POST ends up re-parsing the request. However it's not clear if there are cases such as parse errors that only occur when the stream has been fully or partially read where it would simply end up repr as a blank request.POST. That's difficult to replicate as you need to provide a multipart content that contains a file field, with an invalid base64 transfer encoding. (I havn't quite figured out how to do that.)

Given that this is clearly an erroneous commenting out, I think we clearly need to revert it in any case.

comment:2 Changed 5 months ago by bmispelon

  • Cc bmispelon added
  • Has patch set
  • Patch needs improvement set
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

Yes, this was clearly a mistake on my part.

The patch looks good but as you said, the included test doesn't actually fail when self._mark_post_parse_error() is commented out which is kind of a problem.

I'll try to see if I can find a way to have the test fail and if I can't, I'll merge your pull request directly.

Thanks for catching this early.

comment:3 Changed 5 months ago by Baptiste Mispelon <bmispelon@…>

  • Resolution set to fixed
  • Status changed from new to closed

In ceecc962ad8f6bbbc2b989aec53eee6c6cca04b9:

Fixed #21447 -- Restored code erroneously removed in 20472aa827669d2b83b74e521504e88e18d086a1.

Also added some tests for HttpRequest.repr.
Note that the added tests don't actually catch the accidental code
removal (see ticket) but they do cover a codepath that wasn't tested
before.

Thanks to Tom Christie for the report and the original patch.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.