(string[])Request.Headers["NonExistentHeader"] is null while (IEnumerable<string>)Request.Headers["NonExistentHeader"] is not

[hrumhurum]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

Consider the following code:

string value = string.Join("\n", Request.Headers["NonExistentHeader"]);

It causes the following exception in runtime at method string.Join(string separator, string[] value):

ArgumentNullException: Value cannot be null. (Parameter 'value')

For a very strange reason, the implicit cast of an empty StringValues struct to string[] type returns null:

string[] arr = default(StringValues);
Console.WriteLine("string[] is null: {0}", arr is null);  // prints 'string[] is null: True' message

While a similar cast to IEnumerable returns an expected non-null value:

var seq = (IEnumerable<string>)default(StringValues);
Console.WriteLine("IEnumerable<string> is null: {0}", seq is null);  // prints 'IEnumerable<string> is null: False' message

When C# compiler compiles string.Join("\n", Request.Headers["NonExistentHeader"]) expression, it selects string.Join(string separator, string[] value) overload and this makes the expression to crash at runtime. While the project has nullable checks enabled, C# compiler is not able to catch that null reference error at compile time.

It looks like a bug because structs are never expected to be null and thus their cast operators should avoid producing null values as well, unless there is a very good reason for that.

The following approach allows to workaround the issue:

string value = string.Join("\n", (IEnumerable<string>)Request.Headers["NonExistentHeader"]);

It would be nice to have a consistent behavior here (non-null string[] is expected and preferred). But even if there is a null then C# compiler should be able to catch it at compile time.

Expected Behavior

(string[])default(StringValues) aways returns a non-null value.

Steps To Reproduce

string value = string.Join("\n", Request.Headers["NonExistentHeader"]);

Exceptions (if any)

ArgumentNullException: Value cannot be null. (Parameter 'value') at string.Join(string separator, string[] value)

.NET Version

6.0.401

Anything else?

Server: Kestrel

[dotnet-issue-labeler[bot]]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[Tratcher]

Which server was this on, IIS, Kestrel, or HttpSys? They each have their own header collection implementation.

[hrumhurum]

This behavior is spotted on Kestrel. (I've just added that info to the original message)

[snickler]

For a very strange reason, the implicit cast of an empty StringValues struct to string[] type returns null:

This looks to be by design

https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Primitives/src/StringValues.cs#L83 https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Primitives/src/StringValues.cs#L280

May be better to do Request.Headers["NonExistentHeader"].ToArray() since it returns an empty Array<string> if the value is null.

[hrumhurum]

It seems that the definition was updated in .NET 7 to include the correct nullability annotation for the implicit cast operator.

However, it does not generally look like a good design decision IMHO. I guess there may be some compatibility concerns, but the current behavior causes a friction that could be absent otherwise.

[adityamandaleeka]

@hrumhurum yea, I guess this is unexpected behavior.

May be better to do Request.Headers["NonExistentHeader"].ToArray() since it returns an empty Array if the value is null.

@snickler, this seems like a reasonable suggestion. We would consider a PR to unify this.

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[vadrsa]

I'd like to contribute and can work on this issue, but idk if I can take it?

[adityamandaleeka]

@vadrsa Sure, go for it.

[adityamandaleeka]

I'm not sure if you've contributed to this repo before, but do let us know if you have any questions/issues getting set up. Look forward to your PR :)

[vadrsa]

@adityamandaleeka It will be my first contribution to this repo, so I might ask a dumb question or two))

[vadrsa]

So, a few questions. I should create PR in runtime repo, right? Should I open an issue in runtime repo, or this issue is enough? Should I go straight to PR or create some kind of design proposal?

[adityamandaleeka]

@vadrsa Good question. Yes the PR will be in the runtime repo. I'll go ahead and transfer this issue to the runtime repo so you don't need to create another one.

[ghost]

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

Issue Details

---

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

Consider the following code:

string value = string.Join("\n", Request.Headers["NonExistentHeader"]);

It causes the following exception in runtime at method string.Join(string separator, string[] value):

ArgumentNullException: Value cannot be null. (Parameter 'value')

For a very strange reason, the implicit cast of an empty StringValues struct to string[] type returns null:

string[] arr = default(StringValues);
Console.WriteLine("string[] is null: {0}", arr is null);  // prints 'string[] is null: True' message

While a similar cast to IEnumerable returns an expected non-null value:

var seq = (IEnumerable<string>)default(StringValues);
Console.WriteLine("IEnumerable<string> is null: {0}", seq is null);  // prints 'IEnumerable<string> is null: False' message

When C# compiler compiles string.Join("\n", Request.Headers["NonExistentHeader"]) expression, it selects string.Join(string separator, string[] value) overload and this makes the expression to crash at runtime. While the project has nullable checks enabled, C# compiler is not able to catch that null reference error at compile time.

It looks like a bug because structs are never expected to be null and thus their cast operators should avoid producing null values as well, unless there is a very good reason for that.

The following approach allows to workaround the issue:

string value = string.Join("\n", (IEnumerable<string>)Request.Headers["NonExistentHeader"]);

It would be nice to have a consistent behavior here (non-null string[] is expected and preferred). But even if there is a null then C# compiler should be able to catch it at compile time.

Expected Behavior

(string[])default(StringValues) aways returns a non-null value.

Steps To Reproduce

string value = string.Join("\n", Request.Headers["NonExistentHeader"]);

Exceptions (if any)

ArgumentNullException: Value cannot be null. (Parameter 'value') at string.Join(string separator, string[] value)

.NET Version

6.0.401

Anything else?

Server: Kestrel

Author:	hrumhurum
Assignees:	vadrsa
Labels:	help wanted, untriaged, area-Extensions-Primitives
Milestone:	-

[adityamandaleeka]

@maryamariyan

[tarekgh]

@hrumhurum is this a blocking issue for you or you can work around this for now and we fix it in future releases?

[eerhardt]

I'm not sure this suggestion is inline with the original design of StringValues. Reading https://github.com/aspnet/Announcements/issues/60, it says:

The value is implicitly convertable to and from string and string[]

Changing the implicit conversion to string[] is going to lose the "nullness" of it. Take the following code as an example:

string? a = null;
StringValues values = a;

string? b = values;
Console.WriteLine(b is null); // prints 'true'

string?[]? aArray = null;
values = aArray;

string?[]? bArray = values;
Console.WriteLine(bArray is null); // prints 'true' today, but with this change will print 'false'

This seems pretty inconsistent to be implicitly convertible to and from a Type, but lose information when you do.

To back up this point, see https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/operator-overloads

❌ DO NOT provide an implicit conversion operator if the conversion is potentially lossy.

For example, there should not be an implicit conversion from Double to Int32 because Double has a wider range than Int32. An explicit conversion operator can be provided even if the conversion is potentially lossy.

[hrumhurum]

@eerhardt, the HTTP spec has no notion of null headers, so trying to shove it there is quite meaningless IMHO.

It also means that the conversion from (string[]?)null to StringValues may be disallowed:

string[]? a = null;
StringValues values = a; // nullability warning during compilation (but no error in runtime for compatibility reasons)

If anybody ever needs the notion of (string[]?)null with StringValues, it can be achieved in a more natural and mathematically sound way:

string[]? a = null;
StringValues? values = a;

string[]? b = values;
Console.WriteLine(b is null); // prints 'true'

On a general note, I think that the original idea of having a (string[]?)null in StringValues might well be just to enable the following approach within the header collection:

Request.Headers["SomeHeader"] = "example"; // sets "SomeHeader" to "example" value

string[]? value = Request.Headers["SomeHeader"]; // retrieves the value of "SomeHeader"
if (value != null)
{
    foreach (var i in value)
        // ...
}

The suggested change does not violate that original idea, actually it even improves it making it more tidy and less prone to errors. Consider the following code (after the change):

Request.Headers["SomeHeader"] = "example"; // sets "SomeHeader" to "example" value

string[] value = Request.Headers["SomeHeader"]; // retrieves the value of "SomeHeader"
foreach (var i in value)
    // ...

[hrumhurum]

@tarekgh, this is not a blocking issue in any way as it can be workarounded.

[eerhardt]

the HTTP spec has no notion of null headers

Note that StringValues is a primitive type that is usable outside of HTTP headers.

string value = string.Join("\n", Request.Headers["NonExistentHeader"]);

From your original issue report, you get a nullability warning on this line:

CS8604 - Possible null reference argument for parameter 'value' in 'string string.Join(string? separator, params string?[] value)'.

So it shouldn't be surprising that you need to handle the case that Request.Headers["NonExistentHeader"] turns into a null string array.

At one point in time there was a proposal to add IsNull to StringValues: https://github.com/dotnet/extensions/pull/323. But that was reverted, since it broke the build.

cc @Tratcher

[davit-asryan-sveasolar]

I'm not sure this suggestion is inline with the original design of StringValues. Reading https://github.com/aspnet/Announcements/issues/60, it says:

The value is implicitly convertable to and from string and string[]

Changing the implicit conversion to string[] is going to lose the "nullness" of it. Take the following code as an example:

string? a = null;

StringValues values = a;



string? b = values;

Console.WriteLine(b is null); // prints 'true'



string?[]? aArray = null;

values = aArray;



string?[]? bArray = values;

Console.WriteLine(bArray is null); // prints 'true' today, but with this change will print 'false'

This seems pretty inconsistent to be implicitly convertible to and from a Type, but lose information when you do.

To back up this point, see https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/operator-overloads

❌ DO NOT provide an implicit conversion operator if the conversion is potentially lossy.

For example, there should not be an implicit conversion from Double to Int32 because Double has a wider range than Int32. An explicit conversion operator can be provided even if the conversion is potentially lossy.

I agree that lossy implicit conversion is bad.

As I see it the problem here is implicit conversions of string?[]? to and from StringValues. It doesn't make sense to me. My suggestion would be to only have implicit conversions for string?[]. And for null values it only makes sense to have null in case of implicit conversion from string?.

[halter73]

I agree we shouldn't change StringValues. I'm going to move this back to the aspnetcore repo and remove the "help wanted" label until we agree on exactly what we want. We can consider using new StringValues(Array.Empty<string>()) instead of new StringValues(null) since there isn't much of a reason to distinguish between 0 and null header values.

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[halter73]

I think we can change Kestrel's KnownHeaders to default to StringValues.Empty instead of default. This will align better with both the plain HeaderDictionary type and QueryCollection type.

https://github.com/dotnet/aspnetcore/blob/de8b5afeb180779c2c16f22cc2e392cb4cf59ed6/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.cs#L47-L48

https://github.com/dotnet/aspnetcore/blob/de8b5afeb180779c2c16f22cc2e392cb4cf59ed6/src/Servers/Kestrel/shared/KnownHeaders.cs#L958-L960

https://github.com/dotnet/aspnetcore/blob/de8b5afeb180779c2c16f22cc2e392cb4cf59ed6/src/Http/Http/src/HeaderDictionary.cs#L63-L77

https://github.com/dotnet/aspnetcore/blob/de8b5afeb180779c2c16f22cc2e392cb4cf59ed6/src/Http/Http/src/QueryCollection.cs#L64-L79

This will make minimal APIs handling of string[] header parameters just as bad (or good) as string[] query string parameters, but at least it will be consistent. See #45956