Skip to content

Update IDNA encoding to 2008 spec - #161

Merged
lovelydinosaur merged 7 commits into
encode:masterfrom
StephenBrown2:idna_2008
Jul 31, 2019
Merged

Update IDNA encoding to 2008 spec#161
lovelydinosaur merged 7 commits into
encode:masterfrom
StephenBrown2:idna_2008

Conversation

@StephenBrown2

Copy link
Copy Markdown
Contributor

Resolves #150 and adds tests based on deviations from 2003 spec: https://unicode.org/reports/tr46/#Deviations

This also enables Unicode庐 Technical Standard #46 to normalize capital letters and such, and a test based on the example given in the module documentation.

Since the idna encode() function chokes on colons, I've split and rejoined the authority string and added to the tests to make sure the port stays as expected.

Comment thread tests/models/test_url.py Outdated
Comment thread httpx/models.py Outdated

@sethmlarson sethmlarson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for opening this @StephenBrown2! I've marked a few issues, I think we should look at how urrlib3 handles this case or explore using rfc3986.IRIReference to handle it for us.

Comment thread httpx/models.py Outdated
@StephenBrown2

Copy link
Copy Markdown
Contributor Author

Looks like rfc3986.IRIReference will in fact do exactly what we want, including handling colons in the authority. I've rebased on master to resolve the conflict with hsts... and switched to using rfc3986.api.iri_reference(url).encode() which will handle the IDNA Domain and return a URLReference object as expected.

Ref: convenience function - https://github.com/python-hyper/rfc3986/blob/df21c4b09078580c1cb10c728dc18c3111fcf603/src/rfc3986/api.py#L41
Relevant bit of IRIReference.encode() - https://github.com/python-hyper/rfc3986/blob/df21c4b09078580c1cb10c728dc18c3111fcf603/src/rfc3986/iri.py#L97-L129

Comment thread httpx/models.py Outdated
) -> None:
if isinstance(url, rfc3986.uri.URIReference):
self.components = url
elif isinstance(url, rfc3986.iri.IRIReference):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should be exposing extra types as part of this pull request.

(In fact I don't think we should be exposing rfc3986.uri.URIReference as part of our public interface in the first place - it'd be much better if we could keep that as an internal implementation detail, in order to allow us to completely switch implementations if needed.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See #170

@lovelydinosaur

Copy link
Copy Markdown
Member

I guess we might also choose to switch our dependency listing to rfc3986[idna], instead of listing the two packages seperately?

That'd stop devs from having to do a "wait where are you actually using the idna package?" double take.

@sethmlarson sethmlarson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I rebased and removed a test case that's no longer valid due to interface changes.

@lovelydinosaur

Copy link
Copy Markdown
Member

Yas - good stuff!

@lovelydinosaur
lovelydinosaur merged commit 40849bf into encode:master Jul 31, 2019
@StephenBrown2
StephenBrown2 deleted the idna_2008 branch November 20, 2019 16:34
Kludex referenced this pull request in pydantic/httpx2 May 11, 2026
Co-authored-by: Josep Cugat <jcugat@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Use IDNA 2008 instead of IDNA 2003

4 participants