From 4c90bd0b01bb2a5b2632db04309417b66196437f Mon Sep 17 00:00:00 2001 From: campersau Date: Sat, 18 Jun 2022 14:34:09 +0200 Subject: [PATCH 1/4] Fix CollectionModelBinder BindSimpleCollection null value handling --- .../Binders/CollectionModelBinder.cs | 9 ++------- .../Binders/CollectionModelBinderTest.cs | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs index 763ad556fee3..64dab00fdbe6 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs @@ -276,13 +276,6 @@ internal async Task BindSimpleCollection( foreach (var value in values) { - bindingContext.ValueProvider = new CompositeValueProvider - { - // our temporary provider goes at the front of the list - new ElementalValueProvider(bindingContext.ModelName, value, values.Culture), - bindingContext.ValueProvider - }; - // Enter new scope to change ModelMetadata and isolate element binding operations. using (bindingContext.EnterNestedScope( elementMetadata, @@ -290,6 +283,8 @@ internal async Task BindSimpleCollection( modelName: bindingContext.ModelName, model: null)) { + bindingContext.ValueProvider = new ElementalValueProvider(bindingContext.ModelName, value, values.Culture); + await ElementBinder.BindModelAsync(bindingContext); if (bindingContext.Result.IsModelSet) diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs index 601fff8d1081..48d52d98d747 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs @@ -205,6 +205,25 @@ public async Task BindSimpleCollection_RawValueIsEmptyCollection_ReturnsEmptyLis Assert.Empty(boundCollection.Model); } + [Fact] + public async Task BindSimpleCollection_RawValueWithNull_ReturnsListWithoutNull() + { + // Arrange + var binder = new CollectionModelBinder(CreateIntBinder(), NullLoggerFactory.Instance); + var valueProvider = new SimpleValueProvider + { + { "someName", "420" }, + }; + var context = GetModelBindingContext(valueProvider); + + // Act + var boundCollection = await binder.BindSimpleCollection(context, new ValueProviderResult(new[] { null, "42", "100", null, "200" })); + + // Assert + Assert.NotNull(boundCollection.Model); + Assert.Equal(new[] { 42, 100, 200 }, boundCollection.Model); + } + private IActionResult ActionWithListParameter(List parameter) => null; [Theory] From 286e32559fd31a23726190786a2d809606cbe38a Mon Sep 17 00:00:00 2001 From: campersau Date: Sat, 18 Jun 2022 18:21:05 +0200 Subject: [PATCH 2/4] Add functional test --- .../Binders/CollectionModelBinderTest.cs | 3 +- .../CustomValueProviderTest.cs | 76 +++++++++++++++++++ .../CustomValueProviderController.cs | 22 ++++++ .../StartupWithCustomValueProvider.cs | 27 +++++++ .../CustomValueProviderFactory.cs | 44 +++++++++++ 5 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs create mode 100644 src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomValueProviderController.cs create mode 100644 src/Mvc/test/WebSites/BasicWebSite/StartupWithCustomValueProvider.cs create mode 100644 src/Mvc/test/WebSites/BasicWebSite/ValueProviders/CustomValueProviderFactory.cs diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs index 48d52d98d747..1c6fe8367508 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs @@ -215,9 +215,10 @@ public async Task BindSimpleCollection_RawValueWithNull_ReturnsListWithoutNull() { "someName", "420" }, }; var context = GetModelBindingContext(valueProvider); + var valueProviderResult = new ValueProviderResult(new[] { null, "42", "100", null, "200" }); // Act - var boundCollection = await binder.BindSimpleCollection(context, new ValueProviderResult(new[] { null, "42", "100", null, "200" })); + var boundCollection = await binder.BindSimpleCollection(context, valueProviderResult); // Assert Assert.NotNull(boundCollection.Model); diff --git a/src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs b/src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs new file mode 100644 index 000000000000..13e85d77429b --- /dev/null +++ b/src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs @@ -0,0 +1,76 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Net; +using System.Net.Http; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; + +namespace Microsoft.AspNetCore.Mvc.FunctionalTests; + +public class CustomValueProviderTest : IClassFixture> +{ + private IServiceCollection _serviceCollection; + + public CustomValueProviderTest(MvcTestFixture fixture) + { + var factory = fixture.Factories.FirstOrDefault() ?? fixture.WithWebHostBuilder(b => b.UseStartup()); + factory = factory.WithWebHostBuilder(b => b.ConfigureTestServices(serviceCollection => _serviceCollection = serviceCollection)); + + Client = factory.CreateDefaultClient(); + } + + public HttpClient Client { get; } + + [Fact] + public async Task CustomValueProvider_DisplayName() + { + // Arrange + var url = "http://localhost/CustomValueProvider/CustomValueProviderDisplayName"; + var request = new HttpRequestMessage(HttpMethod.Get, url); + + // Act + var response = await Client.SendAsync(request); + var content = await response.Content.ReadAsStringAsync(); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal("text/plain", response.Content.Headers.ContentType.MediaType); + Assert.Equal("BasicWebSite.Controllers.CustomValueProviderController.CustomValueProviderDisplayName (BasicWebSite)", content); + } + + [Fact] + public async Task CustomValueProvider_IntValues() + { + // Arrange + var url = "http://localhost/CustomValueProvider/CustomValueProviderIntValues"; + var request = new HttpRequestMessage(HttpMethod.Get, url); + + // Act + var response = await Client.SendAsync(request); + var content = await response.Content.ReadAsStringAsync(); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal("application/json", response.Content.Headers.ContentType.MediaType); + Assert.Equal("[42,100,200]", content); + } + + [Fact] + public async Task CustomValueProvider_StringValues() + { + // Arrange + var url = "http://localhost/CustomValueProvider/CustomValueProviderStringValues"; + var request = new HttpRequestMessage(HttpMethod.Get, url); + + // Act + var response = await Client.SendAsync(request); + var content = await response.Content.ReadAsStringAsync(); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal("application/json", response.Content.Headers.ContentType.MediaType); + Assert.Equal(@"[""foo"",null,""bar"",""baz""]", content); + } +} diff --git a/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomValueProviderController.cs b/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomValueProviderController.cs new file mode 100644 index 000000000000..ef23cf8a4425 --- /dev/null +++ b/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomValueProviderController.cs @@ -0,0 +1,22 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.ComponentModel; +using Microsoft.AspNetCore.Mvc; + +namespace BasicWebSite.Controllers; + +public class CustomValueProviderController : Controller +{ + [HttpGet] + public string CustomValueProviderDisplayName(string customValueProviderDisplayName) + => customValueProviderDisplayName; + + [HttpGet] + public int[] CustomValueProviderIntValues(int[] customValueProviderIntValues) + => customValueProviderIntValues; + + [HttpGet] + public string[] CustomValueProviderStringValues(string[] customValueProviderStringValues) + => customValueProviderStringValues; +} diff --git a/src/Mvc/test/WebSites/BasicWebSite/StartupWithCustomValueProvider.cs b/src/Mvc/test/WebSites/BasicWebSite/StartupWithCustomValueProvider.cs new file mode 100644 index 000000000000..dbdda780ebca --- /dev/null +++ b/src/Mvc/test/WebSites/BasicWebSite/StartupWithCustomValueProvider.cs @@ -0,0 +1,27 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using BasicWebSite.ValueProviders; + +namespace BasicWebSite; + +public class StartupWithCustomValueProvider +{ + public void ConfigureServices(IServiceCollection services) + { + services + .AddMvc(o => + { + o.ValueProviderFactories.Add(new CustomValueProviderFactory()); + }); + } + + public void Configure(IApplicationBuilder app) + { + app.UseDeveloperExceptionPage(); + + app.UseRouting(); + + app.UseEndpoints((endpoints) => endpoints.MapDefaultControllerRoute()); + } +} diff --git a/src/Mvc/test/WebSites/BasicWebSite/ValueProviders/CustomValueProviderFactory.cs b/src/Mvc/test/WebSites/BasicWebSite/ValueProviders/CustomValueProviderFactory.cs new file mode 100644 index 000000000000..effe9e7f7a76 --- /dev/null +++ b/src/Mvc/test/WebSites/BasicWebSite/ValueProviders/CustomValueProviderFactory.cs @@ -0,0 +1,44 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.Extensions.Primitives; + +namespace BasicWebSite.ValueProviders; + +public class CustomValueProviderFactory : IValueProviderFactory +{ + public Task CreateValueProviderAsync(ValueProviderFactoryContext context) + { + context.ValueProviders.Add(new CustomValueProvider(context)); + return Task.CompletedTask; + } + + private class CustomValueProvider : IValueProvider + { + private static readonly Dictionary> Values = new() + { + { "customValueProviderDisplayName", context => context.ActionContext.ActionDescriptor.DisplayName }, + { "customValueProviderIntValues", _ => new []{ null, "42", "100", null, "200" } }, + { "customValueProviderStringValues", _ => new []{ null, "foo", "", "bar", null, "baz" } }, + }; + + private readonly ValueProviderFactoryContext _context; + + public CustomValueProvider(ValueProviderFactoryContext context) + { + _context = context; + } + + public bool ContainsPrefix(string prefix) => Values.ContainsKey(prefix); + + public ValueProviderResult GetValue(string key) + { + if (Values.TryGetValue(key, out var fn)) + { + return new ValueProviderResult(fn(_context)); + } + return ValueProviderResult.None; + } + } +} From 035d2441e70dad3b2b0b95e7c8fcff43cd512896 Mon Sep 17 00:00:00 2001 From: campersau Date: Sun, 3 Jul 2022 13:49:28 +0200 Subject: [PATCH 3/4] PR feedback: keep CompositeValueProvider --- .../src/ModelBinding/Binders/CollectionModelBinder.cs | 8 +++++++- .../ModelBinding/Binders/CollectionModelBinderTest.cs | 2 +- .../test/Mvc.FunctionalTests/CustomValueProviderTest.cs | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs index 64dab00fdbe6..b05bde80f01f 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs @@ -273,6 +273,7 @@ internal async Task BindSimpleCollection( var boundCollection = new List(); var elementMetadata = bindingContext.ModelMetadata.ElementMetadata!; + var valueProvider = bindingContext.ValueProvider; foreach (var value in values) { @@ -283,7 +284,12 @@ internal async Task BindSimpleCollection( modelName: bindingContext.ModelName, model: null)) { - bindingContext.ValueProvider = new ElementalValueProvider(bindingContext.ModelName, value, values.Culture); + bindingContext.ValueProvider = new CompositeValueProvider + { + // our temporary provider goes at the front of the list + new ElementalValueProvider(bindingContext.ModelName, value, values.Culture), + valueProvider + }; await ElementBinder.BindModelAsync(bindingContext); diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs index 1c6fe8367508..a059ae91e321 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs @@ -222,7 +222,7 @@ public async Task BindSimpleCollection_RawValueWithNull_ReturnsListWithoutNull() // Assert Assert.NotNull(boundCollection.Model); - Assert.Equal(new[] { 42, 100, 200 }, boundCollection.Model); + Assert.Equal(new[] { 420, 42, 100, 420, 200 }, boundCollection.Model); } private IActionResult ActionWithListParameter(List parameter) => null; diff --git a/src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs b/src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs index 13e85d77429b..6d219a8016d5 100644 --- a/src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs +++ b/src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs @@ -71,6 +71,6 @@ public async Task CustomValueProvider_StringValues() // Assert Assert.Equal(HttpStatusCode.OK, response.StatusCode); Assert.Equal("application/json", response.Content.Headers.ContentType.MediaType); - Assert.Equal(@"[""foo"",null,""bar"",""baz""]", content); + Assert.Equal(@"[null,""foo"",null,""bar"",null,""baz""]", content); } } From 03a9a81a78392562b4ae50039b437b33e89b99f6 Mon Sep 17 00:00:00 2001 From: campersau Date: Fri, 8 Jul 2022 19:46:55 +0200 Subject: [PATCH 4/4] add NullableInt test --- .../Binders/CollectionModelBinderTest.cs | 2 +- .../CustomValueProviderTest.cs | 17 +++++++++++++++++ .../CustomValueProviderController.cs | 4 ++++ .../CustomValueProviderFactory.cs | 1 + 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs index a059ae91e321..ba6de816a883 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs @@ -215,7 +215,7 @@ public async Task BindSimpleCollection_RawValueWithNull_ReturnsListWithoutNull() { "someName", "420" }, }; var context = GetModelBindingContext(valueProvider); - var valueProviderResult = new ValueProviderResult(new[] { null, "42", "100", null, "200" }); + var valueProviderResult = new ValueProviderResult(new[] { null, "42", "", "100", null, "200" }); // Act var boundCollection = await binder.BindSimpleCollection(context, valueProviderResult); diff --git a/src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs b/src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs index 6d219a8016d5..c4fd51c23a6b 100644 --- a/src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs +++ b/src/Mvc/test/Mvc.FunctionalTests/CustomValueProviderTest.cs @@ -57,6 +57,23 @@ public async Task CustomValueProvider_IntValues() Assert.Equal("[42,100,200]", content); } + [Fact] + public async Task CustomValueProvider_NullableIntValues() + { + // Arrange + var url = "http://localhost/CustomValueProvider/CustomValueProviderNullableIntValues"; + var request = new HttpRequestMessage(HttpMethod.Get, url); + + // Act + var response = await Client.SendAsync(request); + var content = await response.Content.ReadAsStringAsync(); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal("application/json", response.Content.Headers.ContentType.MediaType); + Assert.Equal("[null,42,null,100,null,200]", content); + } + [Fact] public async Task CustomValueProvider_StringValues() { diff --git a/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomValueProviderController.cs b/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomValueProviderController.cs index ef23cf8a4425..d62337f519a8 100644 --- a/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomValueProviderController.cs +++ b/src/Mvc/test/WebSites/BasicWebSite/Controllers/CustomValueProviderController.cs @@ -16,6 +16,10 @@ public string CustomValueProviderDisplayName(string customValueProviderDisplayNa public int[] CustomValueProviderIntValues(int[] customValueProviderIntValues) => customValueProviderIntValues; + [HttpGet] + public int?[] CustomValueProviderNullableIntValues(int?[] customValueProviderNullableIntValues) + => customValueProviderNullableIntValues; + [HttpGet] public string[] CustomValueProviderStringValues(string[] customValueProviderStringValues) => customValueProviderStringValues; diff --git a/src/Mvc/test/WebSites/BasicWebSite/ValueProviders/CustomValueProviderFactory.cs b/src/Mvc/test/WebSites/BasicWebSite/ValueProviders/CustomValueProviderFactory.cs index effe9e7f7a76..8b259d2e50a9 100644 --- a/src/Mvc/test/WebSites/BasicWebSite/ValueProviders/CustomValueProviderFactory.cs +++ b/src/Mvc/test/WebSites/BasicWebSite/ValueProviders/CustomValueProviderFactory.cs @@ -20,6 +20,7 @@ private class CustomValueProvider : IValueProvider { { "customValueProviderDisplayName", context => context.ActionContext.ActionDescriptor.DisplayName }, { "customValueProviderIntValues", _ => new []{ null, "42", "100", null, "200" } }, + { "customValueProviderNullableIntValues", _ => new []{ null, "42", "", "100", null, "200" } }, { "customValueProviderStringValues", _ => new []{ null, "foo", "", "bar", null, "baz" } }, };