Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose content streams in public API #872

Closed
Colin-b opened this issue Mar 23, 2020 · 6 comments
Closed

Expose content streams in public API #872

Colin-b opened this issue Mar 23, 2020 · 6 comments
Labels
question Further information is requested

Comments

@Colin-b
Copy link
Contributor

Colin-b commented Mar 23, 2020

Hello,

Would it be possible to foresee the addition of classes (and maybe custom types) and encode function defined in https://github.com/encode/httpx/blob/master/httpx/_content_streams.py as part of the public API?

I am using encode function in pytest-httpx (and JSONStream in test cases) and I wonder if it shouldn't be exposed to ease creation of responses.

If not, would you advise to copy this code or to keep relying on internals?

As a side note, this request is not to add SyncDispatcher or AsyncDispatcher classes to public API, I mock them in pytest-httpx but it would make no sense to expose them to public.

@tomchristie
Copy link
Member

The ContentStream class could feasibly become part of the public API at some point yes.
I don't know if we'd want to expose the concrete implementations.

We should probably hold off on making it public for a bit. Can you give a simple example of how you'd be using it?

@florimondmanca florimondmanca added the question Further information is requested label Apr 13, 2020
@Colin-b
Copy link
Contributor Author

Colin-b commented May 24, 2020

Apologies for the delay in the reply.

Thanks to your developments on httpcore, and the new release of httpx 0.13, I was able to get rid of almost every internal httpx dependency within pytest-httpx.

However, to allow clients to mock a JSON response, a multipart response or a custom body, I would like to avoid duplicating the httpx._content_streams.encode function and the underlying stream classes.

Do you think it can be exposed at some point?

Thanks again for all your work on httpcore and httpx !

@tomchristie
Copy link
Member

Heya! I think I’d probably need to understand the use case a little better here - I’ve taken a bit of a look over and I think this might be a case for tweaking things in pytest-httpx instead.

The biggest thing I noticed was that you’re using encode in order to write the data/files for HTTP responses. As it happens, that’s probably not what you want to do, since there’s a disparity in the encodings used in HTTP requests and responses...

HTTP requests use multipart and URL-encoded data for HTML form submissions, but they’re not used in HTTP responses.

If you do want to automagically determine an encoding type for HTTP responses, you’d probable want to instead just go with string, bytes -> Plain bytes, and “anything else” -> JSON encoded. (In which case httpx’s encode function isn’t way at you want.)

@Colin-b
Copy link
Contributor Author

Colin-b commented May 25, 2020

I don't understand what you mean by difference in encoding between request and response, could you elaborate (if you are talking about the fact that the accept header is not read to know what content-type to use, that's on purpose)?

URL encoded data makes indeed no sense for responses, however I assumed Multipart could be sent as a response?

In any case, even if pytest-httpx would rewrote encode, it would still require to expose JSONStream / BytesStream and (Async)IteratorStream (to avoid duplicating this code).

Unless you advocate for a copy paste of this code as you don't plan on exposing it?

@tomchristie
Copy link
Member

however I assumed Multipart could be sent as a response?

In theory it could, although in practise it isn't.
Multipart encoding is essentially only used in uploading HTML form data.

For response data you probably want to use something like this...

if isinstance(data, bytes):
    content = data
elif isinstance(data, str):
    content = data.encode("utf-8")
else:
    content = json.dumps(data).encode("utf-8")

@Colin-b
Copy link
Contributor Author

Colin-b commented May 25, 2020

Thx for the input @tomchristie I will then stop relying on this internal func and implement one of my own.

@Colin-b Colin-b closed this as completed May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants