Allow unescaped equal signs in query parameter values #39
Conversation
…equals sign (=) is a reserved character for keys, but can appear unescaped in values. see #38 for details.
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
==========================================
+ Coverage 98.12% 98.15% +0.02%
==========================================
Files 8 8
Lines 1442 1462 +20
Branches 166 170 +4
==========================================
+ Hits 1415 1435 +20
Misses 13 13
Partials 14 14
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
The requested changes here are really just:
- Please land the Twisted test change (Twisted's tests don't need to test hyperlink this deeply, it's hyperlink's job to make this behavior correct) before resubmitting. The change itself is OK, but I don't want to leave it in a state where it could be accidentally merged until the prereqs are done :)
- Get rid of the comment lines in the test, since it can't be both;
to_textwould need to acquire an option here.
hyperlink/test/test_url.py
Outdated
| @@ -532,10 +532,18 @@ def test_parseEqualSignInParamValue(self): | |||
| """ | |||
| u = URL.from_text('http://localhost/?=x=x=x') | |||
| self.assertEqual(u.get(''), ['x=x=x']) | |||
| self.assertEqual(u.to_text(), 'http://localhost/?=x%3Dx%3Dx') | |||
| # TODO: see #38 | |||
There was a problem hiding this comment.
Why is this a TODO? Isn't #38 the bug that specifically asks this not to be done?
There was a problem hiding this comment.
test updated. basically this was a legacy twisted test that I was worried about breaking. The TODO was more like "TODO: fix this in Twisted's test suite". Does Twisted even still have the t.p.url tests duplicated in it? If so those will have to change/come out before updating to the 19.x hyperlink release. :)
There was a problem hiding this comment.
Yeah doesn't look like the Twisted test suite has had the deep tests removed yet: https://github.com/twisted/twisted/blob/8292869715d9471681ffff32296bc73b946868a3/src/twisted/python/test/test_url.py#L479
Not sure why that should block a hyperlink review/release though. Whomever does the upgrade on the Twisted side (i.e., me or you, but maybe someone else) can remove the corresponding tests based on info that'll be in the hyperlink changelog.
There was a problem hiding this comment.
In general I feel like one should feel badly about pip install of the latest version causing downstreams' tests to fail with ~no warning ;). That said, this might be what Twisted's own compatibility policy might call a "gross violation of specifications" and there's no compatible fix-forward way to handle this (i.e.: twisted is just directly testing the wrong thing); so I remove the objection unless you can think of a way to emit a warning rather than fail.
|
@mahmoud Are you planning to move this along so it can land? |
|
Time for my monthly bump here :) |
|
@glyph I dunno, I've started to have my doubts about this. It seems quite minor and maybe too clever. I'm not sure how to test its effects on interoperability. Have you seen any further precedent for this minor fork in handling? |
glyph
changed the title
|
I can't find the original citation, but I believe I had encountered a case where I built a URL with multiple =-signs in it for feeding to an API, and the API did not work with the escaped version. Per WHATWG, browsers (tested: Safari, Chrome, Firefox) definitely will not quote = signs in this position if they're present in the address bar. So I think this is the correct thing to do, interop-wise. |
|
I guess as long as web servers are ok with it, too, that's somewhat of a quorum. Kind of weird they wouldn't have a more general decoding policy; unreserved characters get encoded all the time. Speaking of time, I still don't have the time to make the Twisted change happen, but if someone wants to yank out some of those tests, I'll meet you halfway and fix merge conflicts, etc. get this mergeable again. |
|
@twisted/twisted-contributors anybody want to take a look at removing this overzealous test for library code from the Twisted suite? |
|
Erm. How does @-mentioning teams work |
|
https://blog.github.com/2012-05-09-introducing-team-mentions/ suggests that this should have worked? |
Oh, for sure. If the server behaves correctly, either representation should be OK. But this is exactly why hyperlink has to allow for multiple possible quoting representations ;). |
|
Bump… |
|
👉 |
|
|
|
Hey @mahmoud - any thoughts on fixing this up? |
|
🎶 |
|
lol ok today's your lucky day, I'm puttin everything else on hold takin a look! (for however long this train ride takes :P ) |
Fixed conflicts in hyperlink/_url.py
…es regime (see #38).
|
Hooray, the system works |
mahmoud
changed the title
I am holding off on merging myself because this comment is still in the PR description, but if your views have moved on from this perspective, please feel free to land yourself. |
Twisted should not test Hyperlink's behavior so extensively, as discussed at python-hyper/hyperlink#39
Specifically, allow unescaped equal signs in query parameter values. Values are terminated by ampersands (
&) or the fragment (#) or the end of the URL. See #38 for more details.This change requires changing Twisted's tests, which inherited the overzealous escaping of urllib.