Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Server-side requests/responses #1091

Closed
tomchristie opened this issue Jul 27, 2020 · 15 comments
Closed

Server-side requests/responses #1091

tomchristie opened this issue Jul 27, 2020 · 15 comments

Comments

@tomchristie
Copy link
Member

So, not something that we need to support as a primary use-case, but something we ought to just think about a little before 1.0, is making sure that our Request and Response models can be used on the server side as well.

For instance...

  • It'd be beneficial if libraries like starlette were able to lean heavily on httpx.
  • It'd make for easier custom transport implementations, if users were able to eg. instantiate a request instance, given the incoming raw request info. (Since that's a bit easier to worth with that the raw byte-wise API.)

A couple of API/implementation notes to go along with this...

  • URL() should accept the raw bytes URL 4-tuple format that the httpcore API uses as one of it's options.
  • Request.prepare(...) shouldn't be called automatically on __init__. The Client should call it explicitly in build_request(). This ensures that we'd be able to instantiate a Request instance from the raw HTTP core method/url/headers/stream` information, without having additional headers automatically generated on the model.
  • We might want Request.raw, Response.raw, Headers.raw properties, to match up with the existing URL.raw property.
@florimondmanca
Copy link
Member

florimondmanca commented Jul 27, 2020

Hey hey,

Interesting stuff, but what would be some possible use cases for this?

I'm thinking a use case of a Starlette server side app where we proxy requests from ASGI through HTTPX and responses back again. Is this discussion related to making this kind of things easier?

@tomchristie
Copy link
Member Author

So heres an example of something you might use this for...

class MockTransport():
    """
    A transport class that simply always returns a single given response.
    """
    def __init__(self, response: Response):
        self.response = response

    def request(self, ...):
        return self.response.raw

@jcugat
Copy link
Member

jcugat commented Aug 2, 2020

Could this be used to build the httpx-equivalent of responses / aioresponses?

@florimondmanca
Copy link
Member

@jcugat There's already RESPX by @lundberg which I think was intended to be such an equivalent (although I believe the API has parted ways a bit). :-) But yes this MockTransport example totally rings a bell and might be a way to make RESPX even simpler in design and implementation…

@lundberg
Copy link
Contributor

lundberg commented Aug 2, 2020

True @florimondmanca , the API has parted somewhat, it's hard to keep up with the speed httpx evolves and improves, awesome work!

About the transports...latest version of respx is now implemented using the httpx transport API, and one can choose to use the respx sync/async mock transports directly if wanted. I have a small example in the docs.

I really like the idea of making the request, response and URL from httpx "easier" to use for an external lib @tomchristie . Currently I needed to have some wrappers in respx to fully support http_core "alone".

@johtso
Copy link
Contributor

johtso commented Sep 28, 2020

What's left to be done on this? I had a lot of awkward stuff going on in my custom transport that was alleviated by using a httpx.Request internally, very welcome change. I'm planning to do the same with Responses. Am I right in thinking this is already good to go?

@tomchristie
Copy link
Member Author

Yup you're good to go here. There are some bits & pieces that we might still pursue later.

For example... for accessing JSON or Form encoded data on a request instance, we might want to add .json() and .form() methods.

Aside to self: If we were going all the way with supporting server-side requests, then we'd also want to think about supporting ext containing http_version/client_addr/mount_path in order to give us parity with WSGI/ASGI interfaces.

@florimondmanca
Copy link
Member

Is there anything left related to building requests/responses from "raw" data?

Like…

request = httpx.Request(raw=(method, url, headers, stream, ext))
response = httpx.Response(raw=(status_code, headers, stream, ext))

I think I saw this discussed on another issue or PR, but can't remember where. I think @johtso's experiments with caching hint that there might be a use case there…? (Maybe we already support it, btw, and I'm lacking behind?)

https://github.com/johtso/httpx-caching/blob/54251ea1297c6e50deec1e6570d3da62e847cc18/httpx_caching/_models.py#L19-L24

@johtso
Copy link
Contributor

johtso commented Sep 28, 2020

Hah, please ignore that horrid code, was written in a fit of frustration at having to repeat the list of raw request value types umpteen times.

We're all groovy for easily using httpx.Request and httpx.Response in transports.

The only things are as you mention, having to explicitly unpack the raw Request values when making the Request and having to pluck out the relevant values from the Response when returning it.

Also, this may just be me misunderstanding the definition of ext, but you can't give an ext to a Request. Would be useful to be able to do that when my code is passing the Request around internally and wants to include things in the final ext.

I really like the idea of the raw kwarg, without that I'm going to have to have even more duplication of code that describes how Transport.request gets its arguments.

johtso added a commit to johtso/httpx that referenced this issue Sep 28, 2020
@tomchristie
Copy link
Member Author

@florimondmanca

I'm not sure that we want to go quite that far. (Maybe, but?)

Right now we've got...

def request(method, url, stream, ext):
    request = httpx.Request(method=method, url=url, stream=stream)
    ...
    response = httpx.Response(...)
    return (response.status_code, response.headers.raw, response.stream, response.ext)

Which is probably the right balance of:

  • Allowing easy integration between requests/responses and the transport API.
  • Without being too clever and obscuring stuff.

@tomchristie
Copy link
Member Author

tomchristie commented Sep 28, 2020

@johtso

Also, this may just be me misunderstanding the definition of ext, but you can't give an ext to a Request. Would be useful to be able to do that when my code is passing the Request around internally and wants to include things in the final ext.

We don't really want to be in the business of passing around arbitrary stuff in the extensions. We ought to define what extensions are supported, and guide users towards not treating it as an arbitrary value store. What are you considering using it for?

I can see a use case for us adding Request(..., ext=...), but it's not something that we need as things stand.

Supposing we decided to add a support minimal API for server support to httpcore in addition to the existing client support. Unlike ASGI and WSGI which are used solely for the server-side interface☩, our Transport API is actually perfectly well suitable to be used as the interface either for client -> outgoing transport handling requests or for server handling incoming requests -> app.

In that case, there's a couple of extra bits of information we'd need to pass in addition to the method, url, and headers in order to have parity with the information that ASGI and WSGI pass...

  • ext['http_version'] - The HTTP version with which the incoming request was handled.
  • ext['mount_path'] - Any path prefix that the server is mounted on.
  • ext['client_addr'] - The client IP address. Note that the url here would actually be treated as (scheme, remote_host, remote_port, target), and the Host header is what we'd actually want to use when constructing the URL() instance, given the raw URL.

Corresponding with those we'd also want to add the following properties to request instances...

  • http_version
  • client_addr
  • remote_addr
  • mount_path

☩: Actually that's not completely true. We could feasibly be using WSGI + ASGI instead of our transport API, for the dividing line between httpx and the transports themselves, but it'd be a bit unwieldy, particularly given that the sync + async cases have different styles.

@johtso
Copy link
Contributor

johtso commented Sep 28, 2020

@tomchristie I wanted to include 2 things in Response.ext.

An indication whether or not the response is from the cache.
The actual request that was sent by the caching transport.

Would it be so bad to allow attaching arbitrary stuff to Requests?

One use case is per request options you want to send to a custom transport.

Another use case is if for example you have some kind of multi step web scraping logic that needs context, and your handling of responses is separated from where the requests are sent. The only place that context can be is on the Response, and you couldn't just subclass Request and Response to avoid putting it in ext because the transport would need to pass it along. And even if you did we don't allow setting custom Request and Response classes on a Client I don't think..

Just brainstorming!

@tomchristie
Copy link
Member Author

Ah interesting...

An indication whether or not the response is from the cache.
The actual request that was sent by the caching transport.

We might want to fold that all into a test_info key, which we could also use for template_name/template_context.

(Eg. stay in line with a tentative ASGI proposal along those lines... django/asgiref#135)

One use case is per request options you want to send to a custom transport.

Yes maybe. I think we'd need to talk specifics?

I can see for example that ssl_context would allow us to support different per-request SSL configurations, which we might end up doing. But we ought to try to be as conservative as possible here.

Short version of this: Just like with any other API. Adding stuff is trivial. Removing something once added is nearly impossible. So in general we should tend towards trying to keep ext constrained for a limited set of well defined use-cases, with a clear motivation for each.

@johtso
Copy link
Contributor

johtso commented Sep 28, 2020

One use case is per request options you want to send to a custom transport.

Yes maybe. I think we'd need to talk specifics?

Was more of a general idea really. Potentially there could be an option to make the caching transport ignore a request, but that can also be indicated by adding a no-store header without it being an abuse of headers.

Short version of this: Just like with any other API. Adding stuff is trivial. Removing something once added is nearly impossible. So in general we should tend towards trying to keep ext constrained for a limited set of well defined use-cases, with a clear motivation for each.

I do feel like there's two separate issues.

  1. Avoiding 3rd party libraries and httpx internals having a free for all in ext, something that can be managed by case-by-case consideration and maybe a bit of namespacing.

  2. Not allowing users to attach something to a request that will make it through to the response, but not affect the data sent over the wire (e.g. abusing the headers).

@tomchristie
Copy link
Member Author

Going to push this into an open ended discussion now. I think we've largely resolved the points bought up here, though there's a few bits that might(?) come up later as specific issues.

@encode encode locked and limited conversation to collaborators Mar 23, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

5 participants