aspnet / HttpAbstractions

[Archived] HTTP abstractions such as HttpRequest, HttpResponse, and HttpContext, as well as common web utilities. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
382 stars 193 forks source link

Do not rely on the implicit StringValues to array converter. #1009

Closed Tratcher closed 6 years ago

Tratcher commented 6 years ago

Fixes http://aspnetci/viewLog.html?buildId=425449&tab=buildResultsDiv&buildTypeId=Lite_UniverseTest

https://github.com/aspnet/Common/pull/323 made a subtle change that causes string[] myArray = new StringValues(new string[0]) to implicitly become null. This broke many components up stack that were expecting GetCommaSeparatedValues to return a non-null value even for an empty or missing header resulting in and caused 76 tests failures.

This should fix everything except the MVC test ValueProviderResultTest.Construct_With_None.

@benaadams @ryanbrandenburg

benaadams commented 6 years ago

Should StringValues change

public static implicit operator string[] (StringValues value)
{
    return value.GetArrayValue();
}

to

public static implicit operator string[] (StringValues value)
{
    return value.ToArray();
}

Previously string[] myArray = new StringValues() or string[] myArray = default(StringValues); would have same issue?

(Though is fixed by not relying on it being non-null)

Tratcher commented 6 years ago

The implicit converters should round trip, calling ToArray just inverts the problem.

halter73 commented 6 years ago

@Tratcher Is it critical for converters to round trip if they stabilize to a conical form?

I'm thinking that @benaadams might be right. Converting empty StringValues to an empty array instead of a null array seems safer. If we did this, would this get Universe passing?

benaadams commented 6 years ago

Prior to the change StringValues was ambiguous and had multiple ways to represent the low counts:

Count == 0

Count == 1

Count == 2

This means the check .Count == 0 becomes

if ((_value != null ? 1 : (_values?.Length ?? 0)) == 0)

So the cast of a .Count == 0 to an string[] could return become both string[0] and null anyway.

After https://github.com/aspnet/Common/pull/323 there is a single (internal) representation for each count

Count == 0

Count == 1

Count == 2

There is a fast path for .Count == 0 of .IsNull becoming

if (_value == null && _values == null)

Obviously the inverse for .Count > 0 being !value.IsNull

And the cast of a .Count == 0 to an string[] can only return null

Though, its easy enough to have it return Array.Empty<string>() instead

benaadams commented 6 years ago

I think there is a different change needed...

benaadams commented 6 years ago

Is this the change that's needed to fix Universe? https://github.com/aspnet/HttpAbstractions/pull/1010

benaadams commented 6 years ago

With https://github.com/aspnet/Common/pull/330