Skip to content

Fix Request Charset Issues in Web Cmdlets#8742

Merged
iSazonov merged 3 commits intoPowerShell:masterfrom
markekraus:feature/FixRequestEncodingDetection
Jan 26, 2019
Merged

Fix Request Charset Issues in Web Cmdlets#8742
iSazonov merged 3 commits intoPowerShell:masterfrom
markekraus:feature/FixRequestEncodingDetection

Conversation

@markekraus
Copy link
Contributor

PR Summary

Closes #7618

Instantiating a new MediaTypeHeaderValue object fails when the -ContentType parameter includes a charset such as application/json; charset=utf-8. This makes it impossible to set the content encoding on web requests. Moving to Parse() ensures we actually get a proper MediaTypeHeaderValue when the charset is present, thus allowing users to set their request encoding via proper -ContentType values.

PR Context

#7618
#8736

PR Checklist

@markekraus
Copy link
Contributor Author

The codefactor error is nowhere near the code I touched. *shrugs

@iSazonov iSazonov self-assigned this Jan 25, 2019
try
{
var mediaTypeHeaderValue = new MediaTypeHeaderValue(ContentType);
var mediaTypeHeaderValue = MediaTypeHeaderValue.Parse(ContentType);
Copy link
Collaborator

@iSazonov iSazonov Jan 25, 2019

Choose a reason for hiding this comment

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

We could use MediaTypeHeaderValue.TryParse() to avoid exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need. we already have logic in place to handle the exceptions and perform the correct operations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should avoid exceptions if we can. If the exceptions come only from the Parse() method we should use TryParse().

Copy link
Contributor Author

@markekraus markekraus Jan 25, 2019

Choose a reason for hiding this comment

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

Except that we have different behavior depending on the type of exception. We would have to build our own logic to figure that out. When the type of exception determines behavior it is perfectly acceptable to use them in a try/catch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My comment is not blocking.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jan 25, 2019
@iSazonov iSazonov merged commit ff83206 into PowerShell:master Jan 26, 2019
@iSazonov
Copy link
Collaborator

@markekraus Thanks for fixing the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong encode in Invoke-RestMethod -Body (problem with cyrilic symbols)

4 participants