Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

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

Merged
Tratcher merged 1 commit into
devfrom
tratcher/nulls
Mar 21, 2018
Merged

Do not rely on the implicit StringValues to array converter.#1009
Tratcher merged 1 commit into
devfrom
tratcher/nulls

Conversation

@Tratcher
Copy link
Copy Markdown
Member

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

dotnet/extensions#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

@Tratcher Tratcher added this to the 2.1.0 milestone Mar 21, 2018
@Tratcher Tratcher self-assigned this Mar 21, 2018
@Tratcher Tratcher requested review from JunTaoLuo and halter73 March 21, 2018 16:43
JunTaoLuo
JunTaoLuo approved these changes Mar 21, 2018
@Tratcher Tratcher merged commit e5cffe6 into dev Mar 21, 2018
@Tratcher Tratcher deleted the tratcher/nulls branch March 21, 2018 16:50
@benaadams
Copy link
Copy Markdown
Contributor

benaadams commented Mar 21, 2018

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
Copy link
Copy Markdown
Member Author

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

@halter73
Copy link
Copy Markdown
Member

@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
Copy link
Copy Markdown
Contributor

benaadams commented Mar 21, 2018

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

Count == 0

  • null
  • string[0]

Count == 1

  • string
  • string[1]

Count == 2

  • string[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 dotnet/extensions#323 there is a single (internal) representation for each count

Count == 0

  • null

Count == 1

  • string

Count == 2

  • string[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
Copy link
Copy Markdown
Contributor

I think there is a different change needed...

@benaadams
Copy link
Copy Markdown
Contributor

Is this the change that's needed to fix Universe? #1010

@benaadams
Copy link
Copy Markdown
Contributor

With dotnet/extensions#330

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants