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

Add support for Mount API #1362

Merged
merged 9 commits into from
Nov 24, 2020
Merged

Add support for Mount API #1362

merged 9 commits into from
Nov 24, 2020

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Oct 13, 2020

Closes #977

Allows users to do this sort of thing:

# Redirect all HTTP requests to HTTPS.
mounts = {"http://": HTTPSRedirectTransport()}
client = httpx.Client(mounts=mounts)

Or...

# Mock out the example.org domain.
mounts = {"all://example.org": MockTransport()}
client = httpx.Client(mounts=mounts)

Or...

# Support URLs like "file:///Users/tomchristie/GitHub/encode/httpx/README.md"
mounts = {"file://": FileSystemTransport()}
client = httpx.Client(mounts=mounts)

Once we've got a nice httpx.HTTPTransport() it'll also be a good route for use-cases that require different configuration across different hosts.

Still to do:

  • Documentation.
  • Test case that actually makes a request to a mount point.
  • Potentially add support for eg. {"https://www.example.com/prefix": transport} (?)
  • Make sure we support httpx.Client(mounts={"custom://"}), since I've a suspicion there's possibly something in the cookie handling or elsewhere that'll currently fail on non-http/https schemes.

Also what do we want to do about the gnarly case where a user does the following?...

# Same instance mounted in two places.
mounts = {"http://": transport, "https://": transport}
client = httpx.Client(mounts=mounts)

Note that if the same transport is mounted twice then it'll end up have __enter__/__exit__/close called multiple times.

Perhaps that's not actually an issue? Possible that we could just defer thinking about that unless/until we've got an actual live case to consider where it's problematic? After-all, we do already have a similar issue if a user does the following...

transport = ...
client_1 = httpx.Client(transport=transport)
client_2 = httpx.Client(transport=transport)

@tomchristie tomchristie added the enhancement New feature or request label Oct 13, 2020
@cdeler
Copy link
Member

cdeler commented Oct 16, 2020

# Same instance mounted in two places.
mounts = {"http://": transport, "https://": transport}
client = httpx.Client(mounts=mounts)

It might be a silly idea, but can we make our transports reentrant? I dont know, may be to add some reference counters? From another side, it doesn't look pythonic.

@@ -626,7 +627,12 @@ def __init__(
)
for key, proxy in proxy_map.items()
}
self._proxies = dict(sorted(self._proxies.items()))
if mounts is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly that the mounts overrides proxies? Like:

client = Client(..., proxies={"http://": SomeTransport()}, ..., mounts={"http://": SomeOtherTransport())
assert isinstance(client._mounts["http://"], SomeOtherTransport)

Do we need to outline it somewhere?

@@ -681,7 +687,7 @@ def _transport_for_url(self, url: URL) -> httpcore.SyncHTTPTransport:
Returns the transport instance that should be used for a given URL.
This will either be the standard connection pool, or a proxy.
"""
for pattern, transport in self._proxies.items():
for pattern, transport in self._mounts.items():
Copy link
Member

@cdeler cdeler Oct 16, 2020

Choose a reason for hiding this comment

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

So only the thing, why we introduce self._mounts is a lookup over the dict items.

Then why it's a dictionary? May we have a list with tuples there?

Update: the only benefit we get from self._mounts presented by dict is the keys uniques.

httpx/_models.py Outdated Show resolved Hide resolved
@tomchristie
Copy link
Member Author

Okay, docs are up to scratch on this now, and I think it's good to go...

  • Supporting path prefixes, for mounts={"https://www.example.com/prefix": transport} and proxies={"https://www.example.com/prefix": proxy} is something that we could potentially support, but there's no need for us to treat that as in-scope here.
  • I'm okay with transports having to gracefully deal with 1 or more close operations being called at this point in time. It's not actually a problem anywhere, and again it's something that we could iterate on later if we chose to.

@tomchristie tomchristie requested a review from a team November 13, 2020 13:58
Copy link
Member

@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.

Looking "dope"! Very exciting.

Seconding @cdeler's note about whether there's any confusion to be anticipated re: proxies. What do we expect this to do…?

Client(proxies={"http://": "..."}, mounts={"http://": "..."})

@lundberg
Copy link
Contributor

lundberg commented Nov 16, 2020

Sweet @tomchristie!

Just one thing about mocking...it's quite common that you don't have access to the actual client in the test cases, i.e. testing some remote API behind your integration API. Meaning, it's not really possible to mount a mock transport during a test case.

One way I see that doable is by having specific client mounts picked up from somewhere else in your app/project, e.g. django settings or alike, so that they could differ between production and tests. Works but maybe not so straightforward IMO.

From a RESPX perspective, HTTPX is currently patched globally to mock requests/responses, using a decorator or context manager in your tests. It would be really nice to allow this without patching and to use the mount-feature instead.

Just an idea...what about adding support for global/default mounts? Maybe httpx.configure(mounts=...), picked up as default by the Client, much like the request defaults the client are merging when building a request.

@lundberg
Copy link
Contributor

One more thought ;-)

If mounts were able to be configured globally, i.e. httpx config/settings, then the get_environment_proxies could be moved from the client to the config, used as default mounts by the client, but overridable by the client like now.

@tomchristie
Copy link
Member Author

@lundberg That's a really interesting idea yup! We would need to be super careful with being able to override-at-a-distance, so there might be something to think about there.

@tomchristie
Copy link
Member Author

@florimondmanca @cdeler I'd been a bit slow on responding to the "mounts that precedence over proxies" in the case of two identical keys being used, because I wasn't sure if we ought to document that as expected behaviour, or if it ought to be treated as ambiguous. (Ie. "hey user, probably don't do that, because it's non-obvious what you're expecting")

I'd suggest we get this in as is, but happy to consider any docs follow ups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mount API
4 participants