Cleanup URL percent-encoding behavior.

[tomchristie]

Closes #2883

Cleanup test cases

• The first two commits here cleanup three test cases into an exactly equivalent parameterised test case.
• The next commit supplements the test cases with some extra cases (using with the current behaviour for the assertions).
• Drop a test case that's now been superseeded.

Determine correct behaviour

For each case here I'm listing...

• The URL under test.
• The URL once it's been percent-escaped by chrome.
• The URL we're currently percent-escaping into.

Test 1: Correct

# URL with unescaped chars in path.
https://example.com/!$&'()*+,;= abc ABC 123 :/[]@
https://example.com/!$&'()*+,;=%20abc%20ABC%20123%20:/[]@
https://example.com/!$&'()*+,;=%20abc%20ABC%20123%20:/[]@

Test 2: Correct

# URL with escaped chars in path.
https://example.com/!$&'()*+,;=%20abc%20ABC%20123%20:/[]@
https://example.com/!$&'()*+,;=%20abc%20ABC%20123%20:/[]@
https://example.com/!$&'()*+,;=%20abc%20ABC%20123%20:/[]@

Test 3: Incorrect. We're double-escaping here.

# URL with mix of unescaped and escaped chars in path.
https://example.com/ %97%98%99
https://example.com/%20%97%98%99
https://example.com/%20%2597%2598%2599

Test 4: Incorrect. We don't need to escape / in the query portion. We do need to percent escape '.

# URL with unescaped chars in query.
https://example.com/?!$&'()*+,;= abc ABC 123 :/[]@?
https://example.com/?!$&%27()*+,;=%20abc%20ABC%20123%20:/[]@?
https://example.com/?!$&'()*+,;=%20abc%20ABC%20123%20:%2F[]@?

Test 5: Correct.

# URL with escaped chars in query.
https://example.com/?!$&%27()*+,;=%20abc%20ABC%20123%20:%2F[]@?
https://example.com/?!$&%27()*+,;=%20abc%20ABC%20123%20:%2F[]@?
https://example.com/?!$&%27()*+,;=%20abc%20ABC%20123%20:%2F[]@?

Test 6: Correct.

# URL with mix of unescaped and escaped chars in query.
https://example.com/?%20%61%62%63
https://example.com/?%20%61%62%63
https://example.com/?%20%61%62%63

Test 7: Correct.

# URL encoding characters in fragment.
# (The fragment isn't sent as part of the HTTP request.)
https://example.com/#!$&'()*+,;= abc ABC 123 :/[]@?#
https://example.com/
https://example.com/

Resolve issue

• Update test cases to reflect desired behaviour instead of existing behaviour.
• Update behaviour so that double-escaping is not applied.
• Update behaviour so that / is treated as a safe character in the query portion of the URL.

[tomchristie]

At this point I've change the expectation in "test 3" to reflect the desired behaviour, not the existing behaviour.

[tomchristie]

This is once I've fixed the double escaping.

[tomchristie]

And now updated to also fix test 4.

(Well, almost except we're not encoding ' to %27 - spec seems to imply we're getting this correct here vs. chrome)

[Kludex]

• Fixes httpx 0.25.0 changes decoding behaviour of .query_params when using TestClient starlette#2282

[T-256]

I think we need minor version release for this merge, because it is behavior change.

[elupus]

This unnecessarily encode slashes if you pass them as query params. Which will not solve cases with servers needing stuff like: https://host?mimetype=application/data.

The library will not mutate and break an existing string like that, which is good. But you can not construct it using params={"mimetime": "application/data"}

These server are in theory broken since they should expect possible encoding inside the arguments, but feels like we should avoid overly encoding that. There is quite a lot that in theory is safe here: https://github.com/encode/httpx/pull/2980/files#diff-78d8d93b5dd4c77d99c3e2b46b7286ba71a8fd60e92d8bd4eee5fb200b4f87bfR51

[elupus]

Ps. Since it does solve most cases where the URL is in a pre-encoded form, its absolutely a good change. So i think something like this could go in to start with.

I think some optimization should be done with the use of sets for the reserved characters like i did in my change, since now it's doing linear scans of strings many times over during encoding, but its much less important.

[tomchristie]

I think we need minor version release for this merge, because it is behavior change.

We can commit to that yep.

I think some optimization should be done [...]

Yep, I have some follow-ups I'd like to make wrt...

• Optimization.
• Further test cleanup.
• Ensuring we've got the correct handling for params={...}

I'll treat those as independent follow-ups.

if you pass them as query params.

I'll discuss this separately.

[tomchristie]

@T-256 Thanks for the review. ☺️
If you'd like to be added as a maintainer ping me a private message on gitter.

[tomchristie]

Okay, I think we're good here now.

Just pending a review from @encode/maintainers. 🙏🏼

[T-256]

@T-256 Thanks for the review. ☺️ If you'd like to be added as a maintainer ping me a private message on gitter.

Thanks, I sent PM in gitter.

[karpetrosyan]

I believe we should also add unit tests for these functions, rather than simply testing them with httpx.URL.
This would be a more robust approach, in my opinion.

[T-256]

IMO current approach is fine.

This is clearly a code-smell, because our test cases ought to be tests against our public API, rather than testing implementation details. Perhaps there's some cases where it's a necessary hack, but... perhaps not?

from #2492 (comment)

[zanieb]

I agree testing the public API should be sufficient unless something private is particularly expensive to test via the public API.

[karpetrosyan]

It's a matter of preference, but if we encounter regression in our httpx.URL tests, we will go through these functions and find the method that isn't working properly, which is why unit tests are useful.

[T-256]

Thanks, LGTM!

[tomchristie]

thanks @karpetrosyan 🙏🏼