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

Remove From header validation from System.Net.Http #52794

Merged
merged 2 commits into from
May 19, 2021

Conversation

eerhardt
Copy link
Member

The From header is not commonly used and this logic adds a decent amount of code to System.Net.Http that can't be trimmed.

Fix #52664

The From header is not commonly used and this logic adds a decent amount of code to System.Net.Http that can't be trimmed.

Fix dotnet#52664
@ghost
Copy link

ghost commented May 14, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

The From header is not commonly used and this logic adds a decent amount of code to System.Net.Http that can't be trimmed.

Fix #52664

Author: eerhardt
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@eerhardt eerhardt added the size-reduction Issues impacting final app size primary for size sensitive workloads label May 14, 2021
@ghost
Copy link

ghost commented May 14, 2021

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar, @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

The From header is not commonly used and this logic adds a decent amount of code to System.Net.Http that can't be trimmed.

Fix #52664

Author: eerhardt
Assignees: -
Labels:

area-System.Net.Http, size-reduction

Milestone: -

@@ -739,14 +749,15 @@ public void From_UseAddMethod_AddedValueCanBeRetrievedUsingProperty()
[Fact]
public void From_UseAddMethodWithInvalidValue_InvalidValueRecognized()
{
// values are not validated, so invalid values are accepted
Copy link
Contributor

Choose a reason for hiding this comment

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

But this one can be removed, it's calling GetParsedValues which is pointless if we don't have a parser.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, per my comments above about using TokenParser, we should keep this and use it to verify that multiple-valued headers are disallowed.

@eerhardt
Copy link
Member Author

Libraries Test Run release mono Linux arm64 Debug failure is #50300.

PGO failure is unrelated to this change.

@eerhardt eerhardt merged commit 9a91eeb into dotnet:main May 19, 2021
@eerhardt eerhardt deleted the RemoveFromHeaderParser branch May 19, 2021 01:51
@kasperk81
Copy link
Contributor

The title or description of this PR does not note one important thing, removal of public MailAddress class. Was it necessary to remove the From header validation from an internal class? It will unnecessarily break legacy code.

@eerhardt
Copy link
Member Author

removal of public MailAddress class. Was it necessary to remove the From header validation from an internal class? It will unnecessarily break legacy code.

See the answer here: #52664 (comment)

@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@DrewScoggins
Copy link
Member

We are seeing a 5.5KB size decrease from this change.

Benchmark Baseline Test Test/Base Test Quality Baseline IR Compare IR IR Ratio Baseline ETL Compare ETL
SOD - New Blazor Template - Publish - pub/wwwroot/_framework/System.Net.Http.dll 121.00 KB 115.50 KB 0.95 0.00

graph

<Compile Include="$(CommonPath)System\Net\Mail\QuotedStringFormatReader.cs"
Link="Common\System\Net\Mail\QuotedStringFormatReader.cs" />
<Compile Include="$(CommonPath)System\Net\Mail\WhitespaceReader.cs"
Link="Common\System\Net\Mail\WhitespaceReader.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

Yay :)

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Nice.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpClient: Consider removing MailAddressParser
6 participants