Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6251 enhancement closed fixed (fixed)

Add a helper function for getting the body of an HTTP request.

Reported by: Tom Prince Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight, Julian Berman Branch: branches/http-getBody-6251-2
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun, tomprince

Description

Very often, people just want the body of a request, and aren't going to handle it incrementally. Rather than making everyone write their own protocol, just provide a helper function to do that.

Attachments (1)

getBody.patch (3.8 KB) - added by Tom Prince 5 years ago.
Initial patch (lacks tests)

Download all attachments as: .zip

Change History (21)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: jknight added

Changed 5 years ago by Tom Prince

Attachment: getBody.patch added

Initial patch (lacks tests)

comment:2 Changed 5 years ago by Tom Prince

To expand on the description, the original client api was twisted.web.client.getPage, which just returned the body of the response. But that api is insufficient if you want any more information about the response.

The newer API is twisted.web.client.Agent, which returns an IResponse object, that has all the relevant information about the response. To get the body of the response, you need to pass a IProtocol object to IResponse.deliverBody, which will deliver the body incrementally to the protocol, as the response is recieved.

If the user isn't going to process the response body incrementally, they currently need to implement a protocol that buffers the body until it is complete. Rather than making everybody reimplement this logic, as function should be provided to call .deliverBody with an appropriate protocol and return a deferred that fires with the body, once it has been received.

comment:3 Changed 5 years ago by Julian Berman

Cc: Julian Berman added

I like this. There's some code to do this collection in treq but I also think it'd be nice if Agent could do so already.

Think it'd be better to move the accumulator out of proto_helpers and into the client module if that patch is going to be built upon.

comment:4 Changed 5 years ago by Jean-Paul Calderone

Think it'd be better to move the accumulator out of proto_helpers and into the client module

Not just better, of course. Completely necessary. Implementation code may never import code from a test module.

comment:5 Changed 5 years ago by Tom Prince

Author: tomprince
Branch: branches/http-getBody-6251

(In [36797]) Branching to http-getBody-6251

comment:6 Changed 5 years ago by Tom Prince

Owner: set to Tom Prince

treq's implementation

  • It uses a weak key dictionary to cache the content of a given response.
  • It also has helpers to parse json, and decode to unicode (respecting content-type headers).
  • It has a helper that injects a provided function as the dataReceived of a protocol, and returns a deferred that fires when the body is complete.

comment:7 Changed 5 years ago by Tom Prince

Keywords: review added
Owner: Tom Prince deleted

comment:8 Changed 5 years ago by therve

Keywords: review removed
Owner: set to Tom Prince
  1. Relevant twistedchecker issues:
    W9016:2097,0: Too many blank lines after docstring, found 2
    W9208:2109,8:GetBodyTests.FakeResponse.deliverBody: Missing docstring
    W9208:2113,4:GetBodyTests.test_simple: Missing docstring
    W9208:2122,4:GetBodyTests.test_withPotentialDataLoss: Missing docstring
    W9208:2140,4:GetBodyTests.test_withConnectionLost: Missing docstring
    W9012:2173,0: Expected 2 blank lines, found 3
    W9012:2190,0: Expected 2 blank lines, found 4
    
  1. _GetBodyProtocol checks ConnectionDone in connectionLost, but it should check ResponseDone instead.
  1. In the example, you need to return d from the main function for reactor to work.
  1. There is a typo in the example: 'Rsponse body:'
  1. "buf" is not a great variable name. "dataBuffer" maybe?

6.

+    This is a helper function, for clients that don't want to incrementally                                       
+    recieve the body of an HTTP response.

Typo: recieve.

Thanks, please put it back for review once fixed.

comment:9 Changed 5 years ago by Jean-Paul Calderone

+ This is a helper function, for clients that don't want to incrementally

That comma shouldn't be there, either. ;) A good rule of thumb for commas is "when in doubt leave it out".

comment:10 Changed 5 years ago by Julian Berman

getJSONBody is not really that useful as written. I presume that it was meant to use the encoding stuff that was added (getTextBody) rather than getBody directly?

For reference, requests does: https://github.com/kennethreitz/requests/blob/master/requests/models.py#L630 as per the JSON RPC for guessing encodings, which is useful if we're adding something JSON specific, but wouldn't devastate me personally if it only was present in treq, not in Twisted itself, as long as getBody was there.

comment:11 Changed 5 years ago by Jean-Paul Calderone

Owner: changed from Tom Prince to Jean-Paul Calderone
Status: newassigned

comment:12 Changed 5 years ago by Jean-Paul Calderone

Author: tomprinceexarkun, tomprince
Branch: branches/http-getBody-6251branches/http-getBody-6251-2

(In [38566]) Branching to 'http-getBody-6251-2'

comment:13 Changed 5 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

I took the JSON and text parsing stuff out. Those should come back as part of a follow-up ticket. I also renamed the function to readBody, sorry.

Build results - http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/http-getBody-6251-2

comment:14 Changed 5 years ago by therve

Owner: set to therve

comment:15 Changed 5 years ago by therve

Keywords: review removed
Owner: changed from therve to Jean-Paul Calderone
  1. There are 2 twistedchecker errors: http://buildbot.twistedmatrix.com/builders/twistedchecker/builds/722/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
  1. The howto doc should be formatted to fit in 79 columns.
  1. The reference to example says "Inspect the response", which is the same as the previous example. It should probably mention "and display the body".

Thanks, please merge once fixed!

comment:16 Changed 5 years ago by Tom Prince

  1. The howto doc should be formatted to fit in 79 columns.

There has been talk about switching to sentence per line, for docs. Doing this provides more sensible diffs, rather than having to rewrap each time.

comment:17 Changed 5 years ago by Jean-Paul Calderone

(In [38573]) Address review comments

refs #6251

comment:18 Changed 5 years ago by Jean-Paul Calderone

There has been talk about switching to sentence per line, for docs. Doing this provides more sensible diffs, rather than having to rewrap each time.

Indeed, the line lines were intentional. Hopefully this will work out better than what we've been doing. I suppose maybe this should be documented somewhere, though. :/

So, explicitly skipping that review point. Others addressed above.

comment:19 Changed 5 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [38574]) Merge http-getBody-6251-2

Author: tom.prince, exarkun Reviewer: therve Fixes: #6251

Introduce a Twisted Web client helper, twisted.web.client.readBody, for extracting the body of an HTTP response all at once.

comment:20 Changed 5 years ago by Tom Prince

I filed the line length issue as #6537, which also appears at TheWayWeDoThingsNow

Note: See TracTickets for help on using tickets.