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

make hyperlink handle + like an HTML form post by default #145

Merged
merged 5 commits into from
Dec 30, 2020

Conversation

glyph
Copy link
Collaborator

@glyph glyph commented Dec 28, 2020

Per the interminable discussion already present there, fixes #129.

@codecov
Copy link

codecov bot commented Dec 28, 2020

Codecov Report

Merging #145 (b06994b) into master (e362c53) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
+ Coverage   98.67%   98.69%   +0.02%     
==========================================
  Files          13       13              
  Lines        1809     1840      +31     
  Branches      209      212       +3     
==========================================
+ Hits         1785     1816      +31     
  Misses         12       12              
  Partials       12       12              
Impacted Files Coverage Δ
src/hyperlink/test/test_url.py 99.82% <ø> (ø)
src/hyperlink/_url.py 96.98% <100.00%> (+0.06%) ⬆️
src/hyperlink/test/test_decoded_url.py 100.00% <100.00%> (ø)
src/hyperlink/test/test_scheme_registration.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e362c53...b06994b. Read the comment docs.

Copy link
Contributor

@wsanchez wsanchez left a comment

Choose a reason for hiding this comment

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

Looks good.

One question: do we really want to default to this for all schemes? I'm worried in particular about urn: schemes, though even in that case, perhaps the right answer is just "don't use + in URLs".

twm added a commit to twisted/treq that referenced this pull request Dec 28, 2020
twm added a commit to twisted/treq that referenced this pull request Dec 28, 2020
@twm
Copy link
Collaborator

twm commented Dec 28, 2020

Unfortunately it's not just DecodedURLURL also corrupts +. This PR demonstrates the issue: #146

twm and others added 2 commits December 28, 2020 21:43
Before:

>>> URL(scheme='https', host='foo', query={'f+o o': 'b+a r'}).to_text()
'https://foo/?f%2Bo o=b%2Ba r'

After:

>>> URL(scheme='https', host='foo', query={'f+o o': 'b+a r'}).to_text()
'https://foo/?f+o o=b+a r'

If spaces pass unencoded, surely + can.
(Encoded)URL round-tripping of + in query
twm added a commit to twisted/treq that referenced this pull request Dec 29, 2020
@twm
Copy link
Collaborator

twm commented Dec 30, 2020

👍 Thank you for this! I'll update and release treq to depend on new Hyperlink version as soon as this has been released. That'll unblock Klein.

@twm twm mentioned this pull request Dec 30, 2020
@glyph
Copy link
Collaborator Author

glyph commented Dec 30, 2020

Looks good.

One question: do we really want to default to this for all schemes? I'm worried in particular about urn: schemes, though even in that case, perhaps the right answer is just "don't use + in URLs".

The relevant specification for URNs seems to be https://tools.ietf.org/html/rfc8141#section-2.3.2 - previously, query parameters basically weren't used in URNs; https://tools.ietf.org/html/rfc2141 specifically punts on the issue. And the bananas new syntax in 8141 is going to require extensive changes in hyperlink to accommodate- for example, the URN equivalent of query components are now introduced by "?=" or "?+", not just "?".

In any event, URNs are not URLs so are in some sense out of scope for this library ;-).

I think if we want to handle these properly, we may want a new "URN" wrapper class which inherits this behavior and introduces the appropriate constraints (no netloc, for example) for URNs.

Given all this knock-on complexity, I'm going to go ahead and land it. I'm by no means opposed to supporting URNs, "q-component" and all, but we don't before this change and adding it won't change anything. (Arguably it will make it easier to build proper support on top of URL, although it will admittedly make DecodedURL a little more of a pain.)

@glyph glyph merged commit 1949b07 into master Dec 30, 2020
@glyph glyph deleted the ne-plus-ultra branch December 30, 2020 03:29
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.

Hyperlink should treat + in query as %20
3 participants