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 target request extension #888

Merged
merged 6 commits into from
Feb 21, 2024
Merged

Conversation

karpetrosyan
Copy link
Member

Refs: encode/httpx#2949 (reply in thread)

This PR adds a target extension, which can be used to force HTTP target to a specific bytes-encoded string.

Check out https://developer.mozilla.org/en-US/docs/Web/HTTP/Messages#request_line to read more about HTTP message structure and what HTTP target is.

@tomchristie
Copy link
Member

tomchristie commented Feb 14, 2024

Okay... interesting. I was expecting this to be applied in httpx.HTTPTransport.

We actually support this in httpcore already, because the httpcore.URL() instances are a deliberately simple tuple of scheme, host, port, target.

Perhaps that distinction is subtle enough that it makes sense for us to apply this extension at the httpcore level even though it can be supported with the existing httpcore API?


Note to self our Request/Response/URL docs are missing here... https://github.com/encode/httpcore/blob/d4678e61b0af24747676c18fb0cb0b2027867bb0/docs/requests-responses-urls.md

@karpetrosyan
Copy link
Member Author

I thought it is more natural to have this kind of extension logic in the httpcore package + all the transports will support it.

@tomchristie
Copy link
Member

tomchristie commented Feb 15, 2024

How does this (does this?) effect (affect?) the proxies?

@karpetrosyan
Copy link
Member Author

I don't see any differences, If you like to see this at the httpx layer, I can close this one and open a new PR.

But, is there anything wrong with this one? Passing the target extension directly to the H11 target makes more sense to me.

@tomchristie
Copy link
Member

But, is there anything wrong with this one?

You're probably right that it's sensible to implement this directly in httpcore, since all the other extensions we support are also there. Good call.

I think the neatest way is implementing this elsewhere...

self.extensions = {} if extensions is None else extensions

if 'target' in extensions:
    self.url = URL(
        scheme=self.url.scheme,
        host=self.url.host,
        port=self.url.port,
        target=extensions['target'],
    )

That'll update the target once, and be passed through everywhere.

Reasonable?

@karpetrosyan
Copy link
Member Author

Reasonable?

Much neater, thanks.

docs/extensions.md Outdated Show resolved Hide resolved
@tomchristie
Copy link
Member

tomchristie commented Feb 20, 2024

Looks good, thanks. 😊

I'd suggest also updating the CONNECT requests docs...

httpcore/docs/extensions.md

Lines 212 to 219 in 908013c

# Formulate a CONNECT request...
#
# This will establish a connection to 127.0.0.1:8080, and then send the following...
#
# CONNECT http://www.example.com HTTP/1.1
# Host: 127.0.0.1:8080
url = httpcore.URL(b"http", b"127.0.0.1", 8080, b"http://www.example.com")
with httpcore.stream("CONNECT", url) as response:

Since we can now express this with...

url = "http://127.0.0.1:8080"
extensions = {"target: "http://www.example.com"}
with httpcore.stream("CONNECT", url, extensions=extensions) as response: 

Then, yep let's get this in.

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Made a couple of additional docs changes on top of this.
Fantastic addition, thanks @karpetrosyan. 🎉

@tomchristie tomchristie merged commit 80d21ee into master Feb 21, 2024
5 checks passed
@tomchristie tomchristie deleted the add-target-request-extension branch February 21, 2024 10:07
@tomchristie tomchristie mentioned this pull request Feb 21, 2024
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.

2 participants