GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix Request Charset Issues in Web Cmdlets by markekraus · Pull Request #8742 · PowerShell/PowerShell

Conversation

@markekraus

@markekraus Mark Kraus (markekraus) commented Jan 25, 2019

Copy link
Copy Markdown
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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] to your commit messages if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@markekraus
[feature] Fix Rquest Charset Issues in Web Cmdlets
638e411
@markekraus
Mark Kraus (markekraus) requested review from James Truher (JamesWTruher) and Paul Higinbotham (PaulHigin) as code owners January 25, 2019 01:32
@markekraus

Mark Kraus (markekraus) commented Jan 25, 2019

Copy link
Copy Markdown
Contributor Author

The codefactor error is nowhere near the code I touched. *shrugs
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markekraus
[feature] Fix test name
70a1a0b
iSazonov
Ilya (iSazonov) reviewed Jan 25, 2019
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov Ilya (iSazonov) self-assigned this Jan 25, 2019
iSazonov
Ilya (iSazonov) reviewed Jan 25, 2019
View reviewed changes
Comment thread ...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
try
{
var mediaTypeHeaderValue = new MediaTypeHeaderValue(ContentType);
var mediaTypeHeaderValue = MediaTypeHeaderValue.Parse(ContentType);

@iSazonov Ilya (iSazonov) Jan 25, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markekraus Mark Kraus (markekraus) Jan 25, 2019

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jan 25, 2019

Copy link
Copy Markdown
Collaborator

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markekraus Mark Kraus (markekraus) Jan 25, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jan 25, 2019

Copy link
Copy Markdown
Collaborator

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.

My comment is not blocking.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markekraus and TravisEz13 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@iSazonov Ilya (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
@markekraus
Address PR feedback
e3ea4f1
@iSazonov
Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT) and Travis Plunk (TravisEz13) January 25, 2019 14:35
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Jan 25, 2019
View reviewed changes
TravisEz13
Travis Plunk (TravisEz13) approved these changes Jan 25, 2019
View reviewed changes
@iSazonov
Ilya (iSazonov) merged commit ff83206 into PowerShell:master Jan 26, 2019
@iSazonov

Ilya (iSazonov) commented Jan 26, 2019

Copy link
Copy Markdown
Collaborator

Mark Kraus (@markekraus) Thanks for fixing the issue!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov Ilya (iSazonov) mentioned this pull request Jan 26, 2019
invoke-restmethod and invoke-webrequest Content-Length incorrectly calculated #8736
Closed
@markekraus
Mark Kraus (markekraus) deleted the feature/FixRequestEncodingDetection branch January 26, 2019 14:19
@davidhaymond David Haymond (davidhaymond) mentioned this pull request Dec 5, 2022
Invoke-RestMethod charset not utf8 by default itglue/powershellwrapper#12
Closed
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@markekraus @iSazonov
Fix Request Charset Issues in Web Cmdlets (PowerShell#8742)
cb67de3 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@TravisEz13 Travis Plunk (TravisEz13) TravisEz13 approved these changes
@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (iSazonov)

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@markekraus @iSazonov @TravisEz13 @SteveL-MSFT
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.