-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
Cleanup URL percent-encoding behavior. #2990
Conversation
At this point I've change the expectation in "test 3" to reflect the desired behaviour, not the existing behaviour. |
This is once I've fixed the double escaping. |
And now updated to also fix test 4. (Well, almost except we're not encoding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need minor version release for this merge, because it is behavior change.
This unnecessarily encode slashes if you pass them as query params. Which will not solve cases with servers needing stuff like: 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 |
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. |
We can commit to that yep.
Yep, I have some follow-ups I'd like to make wrt...
I'll treat those as independent follow-ups.
I'll discuss this separately. |
@T-256 Thanks for the review. |
Okay, I think we're good here now. Just pending a review from @encode/maintainers. 🙏🏼 |
Thanks, I sent PM in gitter. |
@@ -449,6 +446,39 @@ def quote(string: str, safe: str = "/") -> str: | |||
) | |||
|
|||
|
|||
def quote(string: str, safe: str = "/") -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree testing the public API should be sufficient unless something private is particularly expensive to test via the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
thanks @karpetrosyan 🙏🏼 |
Closes #2883
Cleanup test cases
Determine correct behaviour
For each case here I'm listing...
Test 1: Correct
Test 2: Correct
Test 3: Incorrect. We're double-escaping here.
Test 4: Incorrect. We don't need to escape
/
in the query portion. We do need to percent escape'
.Test 5: Correct.
Test 6: Correct.
Test 7: Correct.
Resolve issue
/
is treated as a safe character in the query portion of the URL.