8000 Initialize Headers Dictionary Only Once by markekraus · Pull Request #4853 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Initialize Headers Dictionary Only Once #4853

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

Merged
merged 4 commits into from
Sep 21, 2017
Merged

Initialize Headers Dictionary Only Once #4853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec28224
[Feature] Create Headers Only Once and Only If Called
markekraus Sep 16, 2017
fa0b8e1
{feature] headers -> _headers
markekraus Sep 16, 2017
e92ceca
[Feature] Move logic to WebResponseObject for reuse
markekraus Sep 17, 2017
283bdf4
Add comment to address PR Feedback.
markekraus Sep 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions ...werShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebResponseHelper.CoreClr.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
Copyright (c) Microsoft Corporation. All rights reserved.
--********************************************************************/

using System;
using System.Collections.Generic;
using System.Net.Http;
using System.Globalization;

Expand All @@ -16,6 +18,28 @@ internal static string GetCharacterSet(HttpResponseMessage response)
string characterSet = response.Content.Headers.ContentType.CharSet;
return characterSet;
}

internal static Dictionary<string, IEnumerable<string>> GetHeadersDictionary(HttpResponseMessage response)
{
var headers = new Dictionary<string, IEnumerable<string>>(StringComparer.OrdinalIgnoreCase);
foreach (var entry in response.Headers)
{
headers[entry.Key] = entry.Value;
}
// In CoreFX, HttpResponseMessage separates content related headers, such as Content-Type to
// HttpResponseMessage.Content.Headers. The remaining headers are in HttpResponseMessage.Headers.
// The keys in both should be unique with no duplicates between them.
// Added for backwards compatibility with PowerShell 5.1 and earlier.
if (response.Content != null)
{
foreach (var entry in response.Content.Headers)
{
headers[entry.Key] = entry.Value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting, can there be duplicate headers in response.Headers and response.Content.Headers ? Have second ones a high ptiority?

Copy link
Contributor Author
@markekraus markekraus Sep 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No. The headers in Response.Content.Headers will never exist in response.Headers. #4494 added this to address the lack of Content-Type in the WebResponseObject.Headers. I should also say that HttpResponseMessage has it's own conflict resolution for the headers, so we shouldn't have to do anything here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please clarify - my understanding from #4494 is that headers already contains response.Content.Headers, correct?

Copy link
Contributor Author
@markekraus markekraus Sep 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

That was added in #4494. Previous to that, WebResponseObject.Headers did not contain any of the headers in HttpResponseMessage.Content.Headers. CoreFX has split the response headers so that those related to content are always in HttpResponseMessage.Content.Headers and the rest of the response headers are in HttpResponseMessage.Headers.

To clarify, I'm not adding or changing any of the logic in this PR for the creation of the Dictionary. Just moving it so that a new dictionary is not created on every Get to Headers and to make the logic available outside of WebResponseObject. The existing logic is there on purpose and we have existing tests to ensure it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for clarify.
So foreach (var entry in response.Content.Headers) is duplicate code and could we remove it?
Sorry for late question - if CoreFX split the headers why we join them again? Only for backward compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are joining them for backwards compat. In 5.1 and earlier all headers are in WebResponseObject.Headers. In 6.0 the content headers are buried in WebResponseObject.BaseResponse.Content.Headers unless we promote them to the WebResponseObject.Headers dictionary. One would reasonably expect the Headers property to contain all headers.

Copy link
Member

Choose a reason for hiding this comment

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

@markekraus can you please add a comment to the code to clarify that there won't be duplicate headers in response.Headers and response.Content.Headers? It would be very helpful to other people who look at the code later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daxian-dbw should I also include a note about the content headers being added to the headers dictionary for backwards compatibility? or is the note about the distinct headers sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would also be helpful to mention the backward compatibility issue. Thanks @markekraus!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments added.

}
}

return headers;
}

internal static string GetProtocol(HttpResponseMessage response)
{
Expand Down
16 changes: 5 additions & 11 deletions ...werShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebResponseObject.CoreClr.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,23 +31,17 @@ public Dictionary<string, IEnumerable<string>> Headers
{
get
{
var headers = new Dictionary<string, IEnumerable<string>>(StringComparer.OrdinalIgnoreCase);
foreach (var entry in BaseResponse.Headers)
if(_headers == null)
{
headers[entry.Key] = entry.Value;
}
if (BaseResponse.Content != null)
{
foreach (var entry in BaseResponse.Content.Headers)
{
headers[entry.Key] = entry.Value;
}
_headers = WebResponseHelper.GetHeadersDictionary(BaseResponse);
}

return headers;
return _headers;
}
}

private Dictionary<string, IEnumerable<string>> _headers = null;

/// <summary>
/// gets the RelationLink property
/// </summary>
Expand Down
0