Opened 4 years ago

Closed 4 years ago

#18048 closed Bug (duplicate)

Test client implicitly assuming data to be querystring

Reported by: johan@… Owned by: s2hc_johan
Component: Testing framework Version: 1.4
Severity: Normal Keywords: test client
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


In some of the request methods (get, head, options, delete) the data input is assumed to be used as querystring input. This makes the client break when sending data such as a json string since urlencode assumes data to be a dict

This has been solved for some methods. In #11371 for the put request, where the solution was that urlencode was removed and querystring was only extracted from the path with urlparse.

The methods have a lot of redundant code, wehre as it is mostly the setting of content-length and such, this could be consolidated to one function wheras we could keep the possibility to send querystring as a dict into data with the content_type set to some dummy type such as "querystring". This aproach would be benefitial since we then allow the test client to send a body with all request, which is as far as I know not forbidden according to the rfc:2616 (I'm no expert in reading rfcs if I'm wrong pleas correct me).

Attachments (2)

test_patch.diff (2.1 KB) - added by Johan Sommerfeld <johan@…> 4 years ago.
tests showing problem
patch_and_tests.diff (6.1 KB) - added by Johan Sommerfeld <johan@…> 4 years ago.
patch of client, more and modified tests

Download all attachments as: .zip

Change History (6)

comment:1 Changed 4 years ago by Johan Sommerfeld <johan@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Bug

Changed 4 years ago by Johan Sommerfeld <johan@…>

tests showing problem

Changed 4 years ago by Johan Sommerfeld <johan@…>

patch of client, more and modified tests

comment:2 Changed 4 years ago by Johan Sommerfeld <johan@…>

  • Has patch set

upploaded patch and more tests. When I fixed the problem I saw one of my added tests was expecting a body on a head request.

the tests suite goes through.

I haven't written any document since the functionality hasn't changed. When I read the documentation I actually thought this would be supported. If anyone disagrees, pleas point me to where you would like me to add som documentation

comment:3 Changed 4 years ago by s2hc_johan

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

comment:4 Changed 4 years ago by aaugustin

  • Resolution set to duplicate
  • Status changed from assigned to closed

I committed a fix for #17371 after you opened this ticket, and I believe it addresses your concerns.

If it doesn't, could you open a new ticket specifically about the problems that still need fixing?

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