Skip to content

Fix WebClient url encoding regression #79975

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.

Sign up for GitHub

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

Jump to bottom
Merged
MihaZupan merged 1 commit into from
Dec 27, 2022
Merged

Fix WebClient url encoding regression #79975

MihaZupan merged 1 commit into from
Dec 27, 2022

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Dec 26, 2022

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 a team, stephentoub and wfurt 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
Closed
wfurt
wfurt approved these changes Dec 26, 2022
View reviewed changes
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

MihaZupan commented Dec 27, 2022

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

@MihaZupan
Copy link
Member Author

MihaZupan commented Dec 27, 2022

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Dec 27, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan MihaZupan merged commit 89b2740 into dotnet:main Dec 27, 2022
@MihaZupan MihaZupan mentioned this pull request Dec 29, 2022
Merged
@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.

Reviewers

@wfurt wfurt wfurt approved these changes

@stephentoub stephentoub Awaiting requested review from stephentoub

Assignees

@MihaZupan MihaZupan

Labels

area-System.Net.Http

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

TaskWebClientTest.UploadValues_Success test is failing

2 participants

@MihaZupan @wfurt