Skip to content

Conversation

@campersau
Copy link
Contributor

@campersau campersau commented Jun 18, 2022

Fix CollectionModelBinder BindSimpleCollection null value handling

null is now ignored instead of receiving the previous value.
Because StringValues of null equals ValueProviderResult.None and the CompositeValueProvider contained the ElementalValueProvider from the previous iteration, it got used instead.

There was also an interesting case when null is the first value, because then the original IValueProvider might be called again, which returns the original array (or something else if it is computed) and because the array can not be bound to an element value it received actually null.

I don't think the CompositeValueProvider is needed at all, also the bindingContext.ValueProvider should be set inside of EnterNestedScope so the original bindingContext.ValueProvider is properly restored afterwards.

Input Before After
[42,100,null,200] [42,100,100,200] [42,100,200]
[null,42,100,200] [null,42,100,200] [42,100,200]

Fixes #40929

⚠Dangerous hack to get `null` values ⚠

Since ValueProviderResult.None is currently not readonly (I guess that's a bug?) you can set it to something else and then you would get null back instead of it being ignored:

ValueProviderResult.None = new ValueProviderResult(new[] { "please don't ignore null!" });

@campersau campersau requested a review from a team as a code owner June 18, 2022 13:43
@ghost ghost added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels community-contribution Indicates that the PR has been added by a community member labels Jun 18, 2022
@ghost
Copy link

ghost commented Jun 18, 2022

Thanks for your PR, @campersau. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

I'm fine w/ this version but defer to @brunolins16 for final approval.

One slight reservation: It feels odd to favour functional tests over unit tests here. Were we previously missing coverage of custom IValueProviderFactory / IValueProvider implementations❔

Copy link
Member

@brunolins16 brunolins16 left a comment

Choose a reason for hiding this comment

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

The change looks good. Thanks for that. I have a small ask for an additional test and we will be good to go.

@campersau campersau force-pushed the BindSimpleCollectionnullvalue branch from c84fd39 to 03a9a81 Compare July 9, 2022 12:02
@brunolins16
Copy link
Member

@campersau Thanks for your contribution.

@brunolins16 brunolins16 merged commit c565063 into dotnet:main Jul 19, 2022
@ghost ghost added this to the 7.0-rc1 milestone Jul 19, 2022
@campersau campersau deleted the BindSimpleCollectionnullvalue branch July 19, 2022 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Indicates that the PR has been added by a community member old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ValueProvider array of strings having a null value is processed wrong

4 participants