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

WebAPI: Treat the args dictionary as immutable #995

Merged
merged 4 commits into from
Jun 14, 2021
Merged

Conversation

yaakov-h
Copy link
Member

@yaakov-h yaakov-h commented Jun 13, 2021

This PR follows on from #992 by avoiding making any changes to the arguments dictionary in the first place.

I've rewritten the flow of the HTTP request builder function so that instead of updating the dictionary and then transforming the dictionary into text, we write out the text values directly, and then copy over the existing dictionary values.

cc/ @JustArchi @xPaw

}


var urlBuilder = new StringBuilder();
var paramBuilder = new StringBuilder();
Copy link
Member

@xPaw xPaw Jun 13, 2021

Choose a reason for hiding this comment

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

Is HttpValueCollection only in ASP? If .NET's uri/querystring stuff can be used, all this custom escaping code could be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to be an internal class only in both .NET Framework and Core, and does not appear as an available API anywhere on MS docs site. HttpUtility can parse but not build. FormUrlEncoded content can build but it requires having a full IEnumerable of all the values upfront, and is async.

Copy link
Member

Choose a reason for hiding this comment

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

That's lame.

Copy link
Contributor

@JustArchi JustArchi left a comment

Choose a reason for hiding this comment

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

We must pay special attention to #641 when rewriting this, as to not regress when user supplies byte array which causes different encoding to be used. I checked the current code and it looks OK. I also didn't find any problems in regards to this, thanks for following up on this matter @yaakov-h 😎.

@yaakov-h yaakov-h merged commit d85432f into master Jun 14, 2021
@yaakov-h yaakov-h deleted the webapi-immutable branch June 14, 2021 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants