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

Is a Null Header Value a Set header value? #4754

Closed
benaadams opened this issue Mar 17, 2018 · 5 comments · Fixed by #41542
Closed

Is a Null Header Value a Set header value? #4754

benaadams opened this issue Mar 17, 2018 · 5 comments · Fixed by #41542
Assignees
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. feature-kestrel help wanted Up for grabs. We would accept a PR to help resolve this issue severity-nice-to-have This label is used by an internal tool
Milestone

Comments

@benaadams
Copy link
Member

Setting a Header to null

  • doesn't change its count (stays at zero)
  • doesn't change the Response output (header is not output)

However it will cause an error if a real value is added after:

app.Run(async (context) =>
{
    var resposne = context.Response;

    // Prints zero
    Console.WriteLine(resposne.Headers["warning"].Count);

    // Does not change response
    resposne.Headers.Add("warning", new StringValues((string)null));

    // Prints zero
    Console.WriteLine(resposne.Headers["warning"].Count);

    // Throws: System.ArgumentException: An item with the same key has already been added.
    resposne.Headers.Add("warning", new StringValues("Hello"));

    await context.Response.WriteAsync("Hello World!");
});

If you can't smell it, see it, taste it, does it exist?

Should it not throw, but set the value? (as no value is set)

From aspnet/KestrelHttpServer#2347 (which was treating it as not set for perf reasons; i.e. don't expend extra cycles tracking nothing for no effect other than to just to throw an error on a second add)

/cc @halter73 @davidfowl

@benaadams
Copy link
Member Author

benaadams commented Mar 17, 2018

Clearly the correct answer is null value is not set 😉
As it acts in every measurable way as not set other than throwing exception on a second .Add

So not throwing would be be restoring sanity (bug fix); rather than a behavior change (break)

@benaadams
Copy link
Member Author

benaadams commented Mar 17, 2018

Setting to empty string by contrast, will return 1 for count and output a header (with no value); so throwing in that case makes sense.

The only wrinkle/corner case in the logic is

resposne.Headers.Add("warning", new StringValues(new string[3]));

Will return 3 as count and output no headers; but if you do that you are just being very strange.

So should StringValues remove nulls when setting to a string[] so that would also return count 0?

@benaadams
Copy link
Member Author

Added change to remove nulls from arrays to cover the corner case.

@benaadams
Copy link
Member Author

/cc @Tratcher

@halter73
Copy link
Member

It think we've decided we don't care about the array-of-nulls corner case here.

@aspnet-hello aspnet-hello transferred this issue from aspnet/KestrelHttpServer Dec 13, 2018
@aspnet-hello aspnet-hello added this to the Backlog milestone Dec 13, 2018
@aspnet-hello aspnet-hello added area-servers bug This issue describes a behavior which is not expected - a bug. feature-kestrel labels Dec 13, 2018
@jkotalik jkotalik added affected-very-few This issue impacts very few customers severity-nice-to-have This label is used by an internal tool labels Nov 13, 2020 — with ASP.NET Core Issue Ranking
@davidfowl davidfowl added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Mar 28, 2021
@Tratcher Tratcher self-assigned this Apr 19, 2022
@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 5, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. feature-kestrel help wanted Up for grabs. We would accept a PR to help resolve this issue severity-nice-to-have This label is used by an internal tool
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants