Opened 3 years ago

Closed 3 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

Description

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@…> 3 years ago.
tests showing problem
patch_and_tests.diff (6.1 KB) - added by Johan Sommerfeld <johan@…> 3 years ago.
patch of client, more and modified tests

Download all attachments as: .zip

Change History (6)

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

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

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

tests showing problem

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

patch of client, more and modified tests

comment:2 Changed 3 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 3 years ago by s2hc_johan

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

comment:4 Changed 3 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