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

Fix WebClient url encoding regression #79975

Merged
merged 1 commit into from
Dec 27, 2022

Conversation

MihaZupan
Copy link
Member

Fixes #79731

#75896 introduced two subtle changes to how we encoded values in WebClient:

  1. We are now encoding the ' character.
  2. We started using upper-case letters for percent-encoding values instead of lower-case
    • It turns out WebUtility.UrlEncode and HttpUtility.UrlEncode differ only in this casing behavior ... because reasons

I updated the test to expect ' to be encoded to fix nr. 1, and swapped the implementation to use HttpUtility instead for nr. 2.

@MihaZupan MihaZupan added this to the 8.0.0 milestone Dec 26, 2022
@MihaZupan MihaZupan requested review from stephentoub, wfurt and a team December 26, 2022 14:52
@ghost
Copy link

ghost commented Dec 26, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #79731

#75896 introduced two subtle changes to how we encoded values in WebClient:

  1. We are now encoding the ' character.
  2. We started using upper-case letters for percent-encoding values instead of lower-case
    • It turns out WebUtility.UrlEncode and HttpUtility.UrlEncode differ only in this casing behavior ... because reasons

I updated the test to expect ' to be encoded to fix nr. 1, and swapped the implementation to use HttpUtility instead for nr. 2.

Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http

Milestone: 8.0.0

@ghost ghost assigned MihaZupan Dec 26, 2022
@build-analysis build-analysis bot mentioned this pull request Dec 26, 2022
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.
So we still have duplicate UrlEncode Implementations right? We may look at it as followup.

@MihaZupan
Copy link
Member Author

Yeah there's duplication in HttpUtility and WebUtility implementations.
I never understood the point of having both of these.

@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan MihaZupan merged commit 89b2740 into dotnet:main Dec 27, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TaskWebClientTest.UploadValues_Success test is failing
2 participants