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

Added httpx.BaseTransport and httpx.AsyncBaseTransport. #1515

Closed
wants to merge 10 commits into from

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Mar 17, 2021

We have a little niggle in our API. Almost all the way through httpcore is treated as an implementation detail, that just happens to be used by the default transport implementation, httpx.HTTPTransport.

The one place where this isn't the case is in our "Transport API". In that case we're explicitly documenting "subclass httpcore.SyncHTTPTransport and implement it's API".

To make this clearer, here's how the "custom transports" docs look after this change...


Writing custom transports

A transport instance must implement the low-level Transport API, which deals
with sending a single request, and returning a response. You should either
subclass httpx.BaseTransport to implement a transport to use with Client,
or subclass httpx.AsyncBaseTransport to implement a transport to
use with AsyncClient.

A complete example of a custom transport implementation would be:

import json
import httpx


class HelloWorldTransport(httpx.BaseTransport):
    """
    A mock transport that always returns a JSON "Hello, world!" response.
    """

    def request(self, method, url, headers=None, stream=None, ext=None):
        message = {"text": "Hello, world!"}
        content = json.dumps(message).encode("utf-8")
        stream = [content]
        headers = [(b"content-type", b"application/json")]
        ext = {"http_version": b"HTTP/1.1"}
        return 200, headers, stream, ext

Which we can use in the same way:

>>> import httpx
>>> client = httpx.Client(transport=HelloWorldTransport())
>>> response = client.get("https://example.org/")
>>> response.json()
{"text": "Hello, world!"}

I do have some further thoughts on progressing on from this point, but from a review perspective we probably want to treat this in isolation.

Nonetheless, I think the potential follow through points are worth talking through...

  • We currently have a slightly awkward byte stream API, that includes a .close() method at the httpcore level, but which makes for an awkward base .request() signature. A neater alternative would be to switch to having close be an optional key on the returned ext dict. Then request and response streams are just plain old byte iterables. Yay. (There's a couple of type: ignores in this PR that sweep this under the table.)
  • We might want to have the low-level method that is overridden be something like handle_request(...) instead of request(...). Why? Because then we could implement a couple of convenience methods on the base classes, allowing for easy usage of sending a single request with a transport. Eg...
t = httpx.HTTPTransport()
r = t.request("GET", "https://www.example.com")

This kinda gives us the best of both worlds, of having a strict low-level point in the API where all the request/response models drop away to plain primitives, while also being able to use transports directly. (Which for the purposes of debugging, narrowing down issues etc. is actually super useful. Especially if we start doing a really good job in __repr__ of showing exactly which transports are mounted on a client, and exactly what configuration each transport has setup.)

  • With a little tweaking we could drop the HTTPCORE_EXC_MAP out of the client, and strictly inside the httpx.HTTPTransport implementation, which is nice from the perspective of getting any reference to httpcore entirely out of httpx, except in the sole case of the concrete httpx.HTTPTransport implementation. Not worth discussing the details of that tweak here.
  • Annnnddddd... kinda related to all this, although not absolutely a necessity... I think we probably look at considering dropping our context-managed Transport API into the heap of "interesting idea, but actually not necessarily worth the complexity trade-offs". We've been held up on getting to a neat 1.0 for a while largely because of that one, and I think it might well be worth us nope'ing on it. Wouldn't absolutely preclude us from choosing that route in a 2.x series at some point, but also I'm just not convinced it looks like a graceful road for us, from a user-perspective. Now... that's a potentially loaded decision to make, but I don't think a final decision on that needs to hold us up from progressing with an httpx.BaseTransport API in either case, because I think this way around feels more user-centred regardless of if we had a close-callback or a context-managed API. (Tho would need to take another look at the streaming case for ASGITransport?)

Either ways. This particular PR rubs out one of our last sets of API niggles, and necessarily put us on path for a 0.18.x series, which I think would be the last prior to a 1.0 release.

@tomchristie
Copy link
Member Author

We probably don't want to jump into anything here too hastily, but the core piece of "The base Transport API should be an httpx interface I think is really solid. It makes so much more sense to me from a user perspective than the existing "oh by the way, here's this one bit of interface that happens to be some other package."

@tomchristie
Copy link
Member Author

Updating this into #1522

@tomchristie tomchristie deleted the basetransport branch March 22, 2021 12:31
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.

1 participant