From 0c084fa28a60edd2a39139f1a41b942b3e3b3246 Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Thu, 22 Mar 2018 14:42:05 -0700 Subject: [PATCH] [Fixes #7518] NullReferenceException thrown when Controller method uses Guid parameter default value --- build/dependencies.props | 1 + .../Internal/ParameterDefaultValues.cs | 42 ++++++++----------- .../Microsoft.AspNetCore.Mvc.Core.csproj | 1 + .../Internal/PageActionInvoker.cs | 8 ++-- ...Microsoft.AspNetCore.Mvc.RazorPages.csproj | 1 + .../Internal/ParameterDefaultValuesTest.cs | 26 ++++++++++++ .../DefaultValuesTest.cs | 31 ++++++++++++++ .../RazorPagesTest.cs | 19 +++++++++ .../Controllers/DefaultValuesController.cs | 9 ++++ .../ModelHandlerTestModel.cs | 10 +++++ 10 files changed, 118 insertions(+), 30 deletions(-) diff --git a/build/dependencies.props b/build/dependencies.props index bf8a0978e1..bb3afdae9b 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -65,6 +65,7 @@ 2.1.0-preview2-30355 2.1.0-preview2-30355 2.1.0-preview2-30355 + 2.1.0-preview2-30355 2.1.0-preview2-30355 2.1.0-preview2-30355 2.1.0-preview2-30355 diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ParameterDefaultValues.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ParameterDefaultValues.cs index 948638f247..0d4a641e5f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ParameterDefaultValues.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ParameterDefaultValues.cs @@ -4,6 +4,7 @@ using System; using System.ComponentModel; using System.Reflection; +using Microsoft.Extensions.Internal; namespace Microsoft.AspNetCore.Mvc.Internal { @@ -21,34 +22,25 @@ namespace Microsoft.AspNetCore.Mvc.Internal for (var i = 0; i < parameters.Length; i++) { - var parameterInfo = parameters[i]; - object defaultValue; - - if (parameterInfo.HasDefaultValue) - { - defaultValue = parameterInfo.DefaultValue; - } - else - { - var defaultValueAttribute = parameterInfo - .GetCustomAttribute(inherit: false); - - if (defaultValueAttribute?.Value == null) - { - defaultValue = parameterInfo.ParameterType.GetTypeInfo().IsValueType - ? Activator.CreateInstance(parameterInfo.ParameterType) - : null; - } - else - { - defaultValue = defaultValueAttribute.Value; - } - } - - values[i] = defaultValue; + values[i] = GetParameterDefaultValue(parameters[i]); } return values; } + + private static object GetParameterDefaultValue(ParameterInfo parameterInfo) + { + if (!ParameterDefaultValue.TryGetDefaultValue(parameterInfo, out var defaultValue)) + { + var defaultValueAttribute = parameterInfo.GetCustomAttribute(inherit: false); + defaultValue = defaultValueAttribute?.Value; + + if (defaultValue == null && parameterInfo.ParameterType.IsValueType) + { + defaultValue = Activator.CreateInstance(parameterInfo.ParameterType); + } + } + return defaultValue; + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Microsoft.AspNetCore.Mvc.Core.csproj b/src/Microsoft.AspNetCore.Mvc.Core/Microsoft.AspNetCore.Mvc.Core.csproj index f5263f4479..8014ae9cc2 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Microsoft.AspNetCore.Mvc.Core.csproj +++ b/src/Microsoft.AspNetCore.Mvc.Core/Microsoft.AspNetCore.Mvc.Core.csproj @@ -34,6 +34,7 @@ Microsoft.AspNetCore.Mvc.RouteAttribute + diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvoker.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvoker.cs index 49eb71c523..a6a0de16fe 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvoker.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageActionInvoker.cs @@ -15,6 +15,7 @@ using Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure; using Microsoft.AspNetCore.Mvc.Rendering; using Microsoft.AspNetCore.Mvc.ViewFeatures; using Microsoft.AspNetCore.Mvc.ViewFeatures.Internal; +using Microsoft.Extensions.Internal; using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal @@ -194,11 +195,8 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal { // Do nothing, already set the value. } - else if (parameter.ParameterInfo.HasDefaultValue) - { - value = parameter.ParameterInfo.DefaultValue; - } - else if (parameter.ParameterInfo.ParameterType.IsValueType) + else if (!ParameterDefaultValue.TryGetDefaultValue(parameter.ParameterInfo, out value) && + parameter.ParameterInfo.ParameterType.IsValueType) { value = Activator.CreateInstance(parameter.ParameterInfo.ParameterType); } diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/Microsoft.AspNetCore.Mvc.RazorPages.csproj b/src/Microsoft.AspNetCore.Mvc.RazorPages/Microsoft.AspNetCore.Mvc.RazorPages.csproj index 94cc12fe4c..f0be433ea5 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/Microsoft.AspNetCore.Mvc.RazorPages.csproj +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/Microsoft.AspNetCore.Mvc.RazorPages.csproj @@ -13,6 +13,7 @@ + diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ParameterDefaultValuesTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ParameterDefaultValuesTest.cs index 79f72a53c7..0b9b2f7880 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ParameterDefaultValuesTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ParameterDefaultValuesTest.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.ComponentModel; using Xunit; @@ -25,6 +26,21 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Equal(expectedValues, actualValues); } + [Fact] + public void GetParameterDefaultValues_ReturnsExpectedValues_ForStructTypes() + { + // Arrange + var methodInfo = typeof(TestObject).GetMethod("DefaultValuesOfStructTypes"); + + // Act + var actualValues = ParameterDefaultValues.GetParameterDefaultValues(methodInfo); + + // Assert + Assert.Equal( + new object[] { default(Guid), default(TimeSpan), default(DateTime), default(DateTimeOffset) }, + actualValues); + } + private class TestObject { public void DefaultAttributes( @@ -54,6 +70,16 @@ namespace Microsoft.AspNetCore.Mvc.Internal TestObject input4) { } + + // Note that default value for DateTime currently throws a FormatException + // https://github.com/dotnet/corefx/issues/12338 + public void DefaultValuesOfStructTypes( + Guid guid = default(Guid), + TimeSpan timeSpan = default(TimeSpan), + DateTime dateTime = default(DateTime), + DateTimeOffset dateTimeOffset = default(DateTimeOffset)) + { + } } } } diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/DefaultValuesTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/DefaultValuesTest.cs index cfca842fee..da39f5f4ca 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/DefaultValuesTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/DefaultValuesTest.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Net.Http; using System.Threading.Tasks; using Xunit; @@ -71,5 +72,35 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests // Assert Assert.Equal(expected, response); } + + [Fact] + public async Task Controller_WithDefaultParameterValues_ForStructs_ReturnsDefaults() + { + // Arrange + var expected = $"{default(Guid)}, {default(TimeSpan)}"; + var url = "http://localhost/DefaultValues/EchoValue_DefaultParameterValue_ForStructs"; + + // Act + var response = await Client.GetStringAsync(url); + + // Assert + Assert.Equal(expected, response); + } + + [Fact] + public async Task Controller_WithDefaultParameterValues_ForStructs_ReturnsBoundValues() + { + // Arrange + Guid guid = Guid.NewGuid(); + TimeSpan timeSpan = new TimeSpan(10, 10, 10); + var expected = $"{guid}, {timeSpan}"; + var url = $"http://localhost/DefaultValues/EchoValue_DefaultParameterValue_ForStructs?guid={guid}×pan={timeSpan}"; + + // Act + var response = await Client.GetStringAsync(url); + + // Assert + Assert.Equal(expected, response); + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs index 4958aeaa54..18ba7a5df6 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Globalization; using System.Linq; using System.Net; using System.Net.Http; @@ -10,6 +11,7 @@ using System.Net.Http.Headers; using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Testing; using Xunit; namespace Microsoft.AspNetCore.Mvc.FunctionalTests @@ -1251,6 +1253,23 @@ Microsoft.AspNetCore.Mvc.ViewFeatures.ViewDataDictionary`1[AspNetCore.InjectedPa Assert.Equal("

Hey, it's Mr. totally custom here!

", content.Trim()); } + [Fact] + public async Task Page_Handler_BindsToDefaultValues() + { + // Arrange + string expected; + using (new CultureReplacer(CultureInfo.InvariantCulture, CultureInfo.InvariantCulture)) + { + expected = $"id: 10, guid: {default(Guid)}, boolean: {default(bool)}, dateTime: {default(DateTime)}"; + } + + // Act + var content = await Client.GetStringAsync("http://localhost/ModelHandlerTestPage/DefaultValues"); + + // Assert + Assert.Equal(expected, content); + } + private async Task AddAntiforgeryHeaders(HttpRequestMessage request) { var getResponse = await Client.GetAsync(request.RequestUri); diff --git a/test/WebSites/BasicWebSite/Controllers/DefaultValuesController.cs b/test/WebSites/BasicWebSite/Controllers/DefaultValuesController.cs index f0b2f166fe..3bf963c3eb 100644 --- a/test/WebSites/BasicWebSite/Controllers/DefaultValuesController.cs +++ b/test/WebSites/BasicWebSite/Controllers/DefaultValuesController.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.ComponentModel; using Microsoft.AspNetCore.Mvc; @@ -19,5 +20,13 @@ namespace BasicWebSite.Controllers { return input; } + + [HttpGet] + public string EchoValue_DefaultParameterValue_ForStructs( + Guid guid = default(Guid), + TimeSpan timeSpan = default(TimeSpan)) + { + return $"{guid}, {timeSpan}"; + } } } diff --git a/test/WebSites/RazorPagesWebSite/ModelHandlerTestModel.cs b/test/WebSites/RazorPagesWebSite/ModelHandlerTestModel.cs index 95e9e5ecfc..a5d25a001e 100644 --- a/test/WebSites/RazorPagesWebSite/ModelHandlerTestModel.cs +++ b/test/WebSites/RazorPagesWebSite/ModelHandlerTestModel.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.RazorPages; @@ -35,6 +36,15 @@ namespace RazorPagesWebSite MethodName = nameof(OnGetViewCustomerAsync); } + public IActionResult OnGetDefaultValues( + bool boolean, + int id = 10, + Guid guid = default(Guid), + DateTime dateTime = default(DateTime)) + { + return Content($"id: {id}, guid: {guid}, boolean: {boolean}, dateTime: {dateTime}"); + } + public async Task OnPostCustomActionResult() { await Task.CompletedTask;