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

Supporting alternate schemes. #954

Closed
tomchristie opened this issue May 15, 2020 · 5 comments
Closed

Supporting alternate schemes. #954

tomchristie opened this issue May 15, 2020 · 5 comments

Comments

@tomchristie
Copy link
Member

Custom transport classes might want to support alternate schemes in addition to "http" and "https".
Issue #931 includes a couple of examples here...

  • Use of a test: scheme in unit tests for session handling.
  • Use of http+thing: custom schemes that enable additional behaviours at the transport level.

The question is how would we best want to support this. One thing that might be sensible here is to change the Transport API slightly, so that the url signature is Tuple[bytes, bytes, Optional[int], bytes] instead of the current Tuple[bytes, bytes, int, bytes].

We would then pass only any explicitly included port in the URL, and leave any protocol->port mapping to the responsibility of the transport class. The AsyncConnectionPool and SyncConnectionPool classes would enforce scheme=[b'http'|b'https'], but alternate transport classes could provide other behaviour.

A first step here would to to review where in the httpx codebase we're currently accessing the URL port.

@yeraydiazdiaz
Copy link
Contributor

I think we should scope this issue a bit.

  1. Are we strictly refering to choosing a port based the scheme?

If that's the case then we can change how port is derived from the scheme in the URL model.

Or,

  1. Are we including the selection of a transport as well? Which I think it's the objective behind Tweak scheme handling in client #931 makes things more complicated. To add some context, currently:
  • The client creates a default transport if the user does not provide one,
  • Depending on proxy configurations additional transports may be created,
  • Upon sending a request dispatch_for_url decides which transport to use.

Supporting arbitrary schemes introduces quite a few questions:

  • Which schemes and therefore transports do we create at client init time?
  • Do we create more transports at request time based on the scheme of the request?
  • Do we allow the user to provide their own scheme-to-transport map?
  • Transports are currently in the realm of httpcore, do we allow users to extend those or do we provide a higher level abstraction?

@tomchristie
Copy link
Member Author

tomchristie commented May 17, 2020

To rephrase this slightly, it's not exactly about "supporting alternate schemes" so much as "Enforcing http/https and determining default port mappings should be the responsibility of the transport layer."

Changes would be:

  • The transport API would switch to using a URL signature of Tuple[bytes, bytes, Optional[int], bytes].
  • URL.port should return an Optional[int] instead of int. Don't return a default-port-for-the-scheme if there's no port in the URL.
  • Origin(url) should use default port mappings for determining equivalence, but shouldn't error for schemes outside of {http,https}. We only use this for "should we strip auth headers on redirects", and we'd like the URLs https://example.com:443/ and https://example.com/ to be treated as the same origin for those purposes.
  • The implementation of Client.dispatcher_for_url would need a bit of rejigging if URL.port returned an Optional[int].

@florimondmanca
Copy link
Member

Didn't realize initially that there was already an initial PR about this in HTTPCore - encode/httpcore#92

@yeraydiazdiaz yeraydiazdiaz modified the milestones: v1.0, v0.13 May 21, 2020
@tomchristie
Copy link
Member Author

Okay so #967 and httpcore 0.9 address the only bits of this that we need for 0.13, so I'm going to milestone this as 1.0 now.

@tomchristie tomchristie modified the milestones: v0.13, v1.0 May 21, 2020
@tomchristie tomchristie mentioned this issue May 21, 2020
This was referenced Jul 16, 2020
@tomchristie
Copy link
Member Author

Closing this issue in favour of #1059

@tomchristie tomchristie removed this from the v1.0 milestone Jul 27, 2020
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

No branches or pull requests

3 participants