ValueProvider array of strings having a null value is processed wrong

[alphons]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

public class SomeValueProvider : IValueProvider
{
  public bool ContainsPrefix(string prefix)
  {
    return true;
  }
  public ValueProviderResult GetValue(string key)
  {
    return new ValueProviderResult(new string[] { "a", "b", null, "c" });
  }
}

Controller method

public async Task<IActionResult> Index(List<string> list)
{
  await Task.Yield();

  // Returned list "a", b", "b",  "c"
  // Expected list "a", b", null, "c"

  return Content("<pre>returns "+string.Join(",", list)+ " expected a,b,,c</pre>", "text/html");
}

Expected Behavior

When using an Array of strings having null values in it, it must not skip, nor must it repeat previous element values.

Steps To Reproduce

The bug is visible in this small test project:

https://github.com/alphons/ValueProviderBug

Exceptions (if any)

No exceptions, bug procudes wrong processing of Array elements having null values.

.NET Version

6.0.102

Anything else?

ASP.NET 6.0.2

[TanayParikh]

@dougbu, @Tratcher, @BrennanConroy just confirming whether this is still expected behavior or if we want to take action here.

The decision not to support null values as part of StringValues was made here.

https://github.com/dotnet/aspnetcore/blame/b9fcd82fe9d0883e9828864a8e8b2231f469254f/src/Mvc/Mvc.Abstractions/src/ModelBinding/ValueProviderResult.cs#L160-L163

Regardless, { "a", "b", null, "c" } yielding a, b, b, c (repeated value) and { null, "b", "c" } yielding b, c (skipped value) seems unexpected.

[Tratcher]

StringValues come from headers and query strings where null has no meaning. Worst case you'd end up with string.Empty instead.

That said, yes, returning b twice seems wrong.

[dougbu]

That said, yes, returning b twice seems wrong.

Agreed. I thought my updates in #35547 were limited to simply skipping null values. May need to debug things to find the root cause of the repetition.

I can't think of a ValueProvider than could provide a null value and have that be useful anywhere. @alphons what led you to suggest skipping null was incorrect in some way❔

More generally, your repo is basically test code. Have you found a real-world case where null matters❔

[alphons]

what led you to suggest skipping null was incorrect in some way

I have build a full working Json value provider and came across this strange behaviour.
Using this for posting Json data to a controller having: string values . empty strings (""), and null values. Can be values in a single dim array but also in multi dim arrays. Position of the array item is important.
So skipping null values is a no go and clearly an error, to my opinion.

For my JsonParametersValueProvider, have a look at:

..../JsonParametersValueProvider.cs

[alphons]

To elaborate on the null values, and a read-world (?!) example ...
Maybe in forms or querystring request strings null has no much of a meaning, but when posting json data to a MVC controller, and binding its parameters, we have to use the famous ValueProviderResult. And string.Empty has a complete different meaning then null. Turning string.Empty into null has also devastating effects, mostly gaining nothing, i call it a weird feature or artifact.

Here is some of my javascript code, which posts json to a controller. It can be bind to method parameters, if we all want it ;-)
netproxy is a simple javascript function to make json post calls to an url.
Source ..../wwwroot/Scripts/netproxy.js

 netproxy("./api/ComplexListNullableDouble", { list: [1.2, 3.4, 5.6, null, 7.8] });

To this controller method:

[HttpPost]
[Route("~/api/ComplexListNullableDouble")]
public async Task<IActionResult> ComplexListNullableDouble(List<double?> list)
{
	await Task.Yield();
	return Ok();
}

Results in the repeat bug: 1.2, 3.4, 5.6, 5.6, 7.8

Concluding, for having nullable values, this means also nullable strings, but also empty strings, and they are, clearly, not the same.

[ghost]

Thanks for contacting us.
We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[alphons]

I pinpointed the location of the repeat error. It is in the collection binder.

Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs

I cleared out all the other options, and the error stil exists :-)

builder.Services.AddMvcCore().AddMvcOptions(options =>
{
 options.OutputFormatters.Clear();
 options.InputFormatters.Clear();
 options.ValueProviderFactories.Clear();
 options.ModelValidatorProviders.Clear();
 options.Conventions.Clear();
 options.Filters.Clear();
 options.ModelMetadataDetailsProviders.Clear();
 options.ModelValidatorProviders.Clear();
 options.ModelMetadataDetailsProviders.Clear();
 options.ModelBinderProviders.Clear();

options.ModelBinderProviders.Add(new CollectionModelBinderProvider()); // Repeat previous value on null value
options.ModelBinderProviders.Add(new SimpleTypeModelBinderProvider()); 

options.ValueProviderFactories.Add(new TestWeb.SomeValueProviderFactory());
});

I hope this can be fixed.

[dougbu]

I pinpointed the location of the repeat error. It is in the collection binder.

That's a good start but there are a few layers that need to be debugged to find the root cause. CollectionModelBinder is a rather complex class that sits on top of a fair amount of complex infrastructure.

[alphons]

That's a good start but there are a few layers that need to be debugged to find the root cause. CollectionModelBinder is a rather complex class that sits on top of a fair amount of complex infrastructure.

You are absolutely right, it is rather complex, however, the solution is simpler than we think ;-)
When binding to a simple collection, which is an internal method of CollectionModelBinder class,
every value is bind (nested) also when the value is null.
Why not skip the null values, these are really a dead-end (especially when nested).
The extra line boundCollection.Add(default) and continue on this condition will do fine.....

Example:

internal async Task<CollectionResult> BindSimpleCollection(
		ModelBindingContext bindingContext,
		ValueProviderResult values)
{
	var boundCollection = new List<TElement?>();
	var elementMetadata = bindingContext.ModelMetadata.ElementMetadata!;
	foreach (var value in values)
	{
		if (value == null)
		{
			//boundCollection.Add(ModelBindingHelper.CastOrDefault<TElement>(null));
			boundCollection.Add(default);
			continue;
		}
.....
.....

[dougbu]

The interesting part of this bug is the repeated value and filling in null values probably wouldn't change anything because the value providers have likely skipped null by the time that code is hit. Still curious…

[alphons]

The interesting part of this bug is the repeated value and filling in null values probably wouldn't change anything because the value providers have likely skipped null by the time that code is hit. Still curious…

Sorry to disappoint you ;-) The null values added to the BindSimpleCollection actually, as proposed, do work! So this has to be fixed a.s.a.p.

But thats half of the story. After doing some days of intensive debugging there is a far greater problem.
Therefore I made a proposal.
Proposal
Has been moved to https://github.com/alphons/ValueProviderBug

[dougbu]

Other than removing a number of layers of abstraction, it seems you're proposing something like an IInputFormatter. Let's go back to using an IValueProvider in the first place. Is there a reason you took that approach❔

More specifically, what SystemTextJsonInputFormatter or NewtonsoftJsonInputFormatter issue led you to create a new IValueProvider❔

[alphons]

Let's go back to using an in the first place. Is there a reason you took that approach❔

Inputformatters are of no use. They are made to deliver 1 model InputFormatterResult, which a binder can only use if, and only if, there is 1 model as parameter to bind to. When using for example controllers having multiple parameters, the InputFormatter is a dead end, clearly. This is the same problem al lot of programmers having when using [FromBody] and having multiple paramaters to bind to.

[TanayParikh]

Hi all, thanks for the discussion here! We've currently assigned this to the .NET 7 Planning milestone. This means we may investigate this issue in the .NET 7 timeframe based on where it stands in priority with other issues, as well as overall time constraints. In the meantime, we'll await further feedback from members in the community.

[TanayParikh]

Let's keep this issue open for now.