Skip to content

Conversation

@lovelydinosaur
Copy link
Contributor

@lovelydinosaur lovelydinosaur commented Dec 13, 2019

The RequestContent interface

This pull requests adds RequestContent as the internal interface for all request body construction. The RequestContent interface provides:

  • .get_headers() - Return the headers for the encoding.
  • .is_rewindable() - True if the content can be replayed, False otherwise.
  • async def __aiter__() -> bytes - Returns the byte stream for the encoded data.

There are a few concrete implementations.

  • RequestContent - An empty request content.
  • BytesRequestContent - Handles plain bytes and str content.
  • StreamingRequestContent - Handles byte async iterables. Not rewindable.
  • JSONRequestContent - Handles JSON data.
  • URLEncodedRequestContent - Handles URL encoded form data.
  • MultipartEncodedRequestContent - Handles Multipart encoded form data.

There is also a top level function for handling returning a RequestContent instance, given data, files, json arguments.

  • encode(data, files, json) -> RequestContent

Motivation

Short: Generally a much nicer seperation of logic & will allow us to support both sync+async cases on a single Request class.

  • All the request encoding logic is nicely seperated for the Request class itself, and testable in isolation.
  • We'll be able to have both async and sync variants by implementing __iter__ on the the interface alongside the existing __aiter__ - most encodings will support both. The StreamingRequestContent will end up as two distinct cases sync+async, which will only support one or the other case, and will error if used incorrectly. (Eg. passing an async iterable as a data= argument will raise an error when used with a sync client.)
  • This gets us much closer to being able to implement a streaming, rewindable multipart implementation.

@lovelydinosaur
Copy link
Contributor Author

NB. The get_headers() interface could potentially be plainer (eg. list of byte tuples) - we can consider any further tweaks like that if needed easily enough, since this is currently a private interface.

We'll want to think a little about exactly about if/how we want to expose any of this on a Request instance. On this pass I've wired the existing request.read() and request.stream() methods onto the new implementation. It's feasible that we'll want to drop that an instead expose request.content as a public interface. (Not that most folks will be working at the level, mind.)

@lovelydinosaur
Copy link
Contributor Author

Much design credit here is due to @sethmlarson.
Also ties in with how request/response content on any lower-level Dispatcher API would look.

Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really like the separation that this new interface enables too, yes!

Some minor comments/questions, but overall this is great. 😄

Co-Authored-By: Florimond Manca <[email protected]>
@florimondmanca florimondmanca self-requested a review December 16, 2019 20:20
@lovelydinosaur
Copy link
Contributor Author

Other thoughts...

  • I'm not sure about the module naming. encoders.py might be nicer than content.py. It doesn't really mirror the decoders (It's data structure encoding, rather than content-encoding) but it does encapsulate the functionality reasonably.
  • It'd probably be nice to bring the multipart implementation into the module, pehaps all encapsulated on the MultipartRequestContent class.

None of that is public API tho, so we could treat it incrementally.

@florimondmanca
Copy link
Contributor

florimondmanca commented Dec 17, 2019

encoders.py might be nicer than content.py

What about request_content.py? Simple and consistent. I might get used to encoders.py / Encoder, though, since we have decoders.py/Decoder for response content.

It'd probably be nice to bring the multipart implementation into the module

I haven't looked at our multipart implementation, so I can't say yet, but definitely in favor of treating that as a follow-up. :-)

Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me as-is, feel free to resolve the question on the content.py module name. :-) Exciting stuff in any case!

@lovelydinosaur
Copy link
Contributor Author

This looks good to me as-is, feel free to resolve the question on the content.py module name.

Gotcha. Not 100% sure yet, but I think that's one that is ok to tackle incrementally. 👍

@lovelydinosaur lovelydinosaur merged commit 8d4f182 into master Dec 18, 2019
@lovelydinosaur lovelydinosaur deleted the request-content branch December 18, 2019 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants