From a23307e2b1e7934e2c8d69774cda1b36778f697b Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Sun, 25 Jun 2017 20:40:42 -0700 Subject: [PATCH] Check for properties that can't befound If you give ModelExpressionProvider a lambda with a private property you'll end up here. This wasn't common before, but it seems like users are more likely to try it with pages. Model Metadata and Model Binding don't handle private properties, so supporting it in Model Expressions seems less than useful. This isn't a breaking change because this case would have resulted in a null-ref. Addresses #6400 --- .../Internal/ExpressionMetadataProvider.cs | 36 +++++++--- .../HtmlGenerationTest.cs | 26 +++++++ .../ExpressionMetadataProviderTest.cs | 72 +++++++++++++++++++ .../HtmlGeneration_WeirdExpressions.cs | 22 ++++++ .../Models/WeirdModel.cs | 14 ++++ .../GetWeirdWithHtmlHelpers.cshtml | 27 +++++++ .../GetWeirdWithTagHelpers.cshtml | 27 +++++++ 7 files changed, 213 insertions(+), 11 deletions(-) create mode 100644 test/WebSites/HtmlGenerationWebSite/Controllers/HtmlGeneration_WeirdExpressions.cs create mode 100644 test/WebSites/HtmlGenerationWebSite/Models/WeirdModel.cs create mode 100644 test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_WeirdExpressions/GetWeirdWithHtmlHelpers.cshtml create mode 100644 test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_WeirdExpressions/GetWeirdWithTagHelpers.cshtml diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionMetadataProvider.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionMetadataProvider.cs index 0649c8033c..38d89f80bc 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionMetadataProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ExpressionMetadataProvider.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Diagnostics; using System.Globalization; using System.Linq.Expressions; using System.Reflection; @@ -60,7 +61,12 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal return FromModel(viewData, metadataProvider); } - containerType = memberExpression.Expression.Type; + // memberExpression.Expression can be null when this is a static field or property. + // + // This can be the case if the expression is like (m => Person.Name) where Name is a static field + // or property on the Person type. + containerType = memberExpression.Expression?.Type; + legalExpression = true; break; @@ -86,16 +92,8 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal } }; - ModelMetadata metadata; - if (propertyName == null) - { - // Ex: - // m => 5 (arbitrary expression) - // m => foo (arbitrary expression) - // m => m.Widgets[0] (expression ending with non-property-access) - metadata = metadataProvider.GetMetadataForType(typeof(TResult)); - } - else + ModelMetadata metadata = null; + if (containerType != null && propertyName != null) { // Ex: // m => m.Color (simple property access) @@ -103,6 +101,22 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal // m => m.Widgets[0].Size (expression ending with property-access) metadata = metadataProvider.GetMetadataForType(containerType).Properties[propertyName]; } + + if (metadata == null) + { + // Ex: + // m => 5 (arbitrary expression) + // m => foo (arbitrary expression) + // m => m.Widgets[0] (expression ending with non-property-access) + // + // This can also happen for any case where we cannot retrieve a model metadata. + // This will happen for: + // - fields + // - statics + // - non-visibility (internal/private) + metadata = metadataProvider.GetMetadataForType(typeof(TResult)); + Debug.Assert(metadata != null); + } return viewData.ModelExplorer.GetExplorerForExpression(metadata, modelAccessor); } diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/HtmlGenerationTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/HtmlGenerationTest.cs index 3773ced1be..d2d6a1349d 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/HtmlGenerationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/HtmlGenerationTest.cs @@ -523,6 +523,32 @@ Products: Music Systems, Televisions (3)"; Assert.Equal(expected, response, ignoreLineEndingDifferences: true); } + // We want to make sure that for 'wierd' model expressions involving: + // - fields + // - statics + // - private + // + // These tests verify that we don't throw, and can evaluate the expression to get the model + // value. One quirk of behavior for these cases is that we can't return a correct model metadata + // instance (this is true for anything other than a public instance property). We're not overly + // concerned with that, and so the accuracy of the model metadata is is not verified by the test. + [Theory] + [InlineData("GetWeirdWithHtmlHelpers")] + [InlineData("GetWeirdWithTagHelpers")] + public async Task WeirdModelExpressions_CanAccessModelValues(string action) + { + // Arrange + var url = "http://localhost/HtmlGeneration_WeirdExpressions/" + action; + + // Act + var response = await Client.GetStringAsync(url); + + // Assert + Assert.Contains("Hello, Field World!", response); + Assert.Contains("Hello, Static World!", response); + Assert.Contains("Hello, Private World!", response); + } + private static HttpRequestMessage RequestWithLocale(string url, string locale) { var request = new HttpRequestMessage(HttpMethod.Get, url); diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionMetadataProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionMetadataProviderTest.cs index 2b912cfa67..44d421c376 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionMetadataProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/ExpressionMetadataProviderTest.cs @@ -10,6 +10,12 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal { public class ExpressionMetadataProviderTest { + private string PrivateProperty { get; set; } + + public static string StaticProperty { get; set; } + + public string Field = "Hello"; + [Fact] public void FromLambdaExpression_GetsExpectedMetadata_ForIdentityExpression() { @@ -148,6 +154,72 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal Assert.Same(myModel, metadata.Container.Model); } + // A private property can't be found by the model metadata provider, so return the property's type + // as a best-effort mechanism. + [Fact] + public void FromLambdaExpression_ForPrivateProperty_ReturnsMetadataOfExpressionType() + { + // Arrange + var provider = new EmptyModelMetadataProvider(); + var viewData = new ViewDataDictionary(provider); + + // Act + var explorer = ExpressionMetadataProvider.FromLambdaExpression( + m => m.PrivateProperty, + viewData, + provider); + + // Assert + Assert.NotNull(explorer); + Assert.NotNull(explorer.Metadata); + Assert.Equal(ModelMetadataKind.Type, explorer.Metadata.MetadataKind); + Assert.Equal(typeof(string), explorer.ModelType); + } + + // A static property can't be found by the model metadata provider, so return the property's type + // as a best-effort mechanism. + [Fact] + public void FromLambdaExpression_ForStaticProperty_ReturnsMetadataOfExpressionType() + { + // Arrange + var provider = new EmptyModelMetadataProvider(); + var viewData = new ViewDataDictionary(provider); + + // Act + var explorer = ExpressionMetadataProvider.FromLambdaExpression( + m => ExpressionMetadataProviderTest.StaticProperty, + viewData, + provider); + + // Assert + Assert.NotNull(explorer); + Assert.NotNull(explorer.Metadata); + Assert.Equal(ModelMetadataKind.Type, explorer.Metadata.MetadataKind); + Assert.Equal(typeof(string), explorer.ModelType); + } + + // A field can't be found by the model metadata provider, so return the field's type + // as a best-effort mechanism. + [Fact] + public void FromLambdaExpression_ForField_ReturnsMetadataOfExpressionType() + { + // Arrange + var provider = new EmptyModelMetadataProvider(); + var viewData = new ViewDataDictionary(provider); + + // Act + var explorer = ExpressionMetadataProvider.FromLambdaExpression( + m => m.Field, + viewData, + provider); + + // Assert + Assert.NotNull(explorer); + Assert.NotNull(explorer.Metadata); + Assert.Equal(ModelMetadataKind.Type, explorer.Metadata.MetadataKind); + Assert.Equal(typeof(string), explorer.ModelType); + } + private class TestModel { public Category SelectedCategory { get; set; } diff --git a/test/WebSites/HtmlGenerationWebSite/Controllers/HtmlGeneration_WeirdExpressions.cs b/test/WebSites/HtmlGenerationWebSite/Controllers/HtmlGeneration_WeirdExpressions.cs new file mode 100644 index 0000000000..7281b6cce1 --- /dev/null +++ b/test/WebSites/HtmlGenerationWebSite/Controllers/HtmlGeneration_WeirdExpressions.cs @@ -0,0 +1,22 @@ +// 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 HtmlGenerationWebSite.Models; +using Microsoft.AspNetCore.Mvc; + +namespace HtmlGenerationWebSite.Controllers +{ + public class HtmlGeneration_WeirdExpressionsController : Controller + { + public IActionResult GetWeirdWithHtmlHelpers() + { + return View(new WeirdModel()); + } + + public IActionResult GetWeirdWithTagHelpers() + { + return View(new WeirdModel()); + } + } +} \ No newline at end of file diff --git a/test/WebSites/HtmlGenerationWebSite/Models/WeirdModel.cs b/test/WebSites/HtmlGenerationWebSite/Models/WeirdModel.cs new file mode 100644 index 0000000000..8dce437889 --- /dev/null +++ b/test/WebSites/HtmlGenerationWebSite/Models/WeirdModel.cs @@ -0,0 +1,14 @@ +// 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.ComponentModel.DataAnnotations; + +namespace HtmlGenerationWebSite.Models +{ + public class WeirdModel + { + public string Field = "Hello, Field World!"; + + public static string StaticProperty { get; set; } = "Hello, Static World!"; + } +} \ No newline at end of file diff --git a/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_WeirdExpressions/GetWeirdWithHtmlHelpers.cshtml b/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_WeirdExpressions/GetWeirdWithHtmlHelpers.cshtml new file mode 100644 index 0000000000..f192c5d84f --- /dev/null +++ b/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_WeirdExpressions/GetWeirdWithHtmlHelpers.cshtml @@ -0,0 +1,27 @@ +@addTagHelper *, Microsoft.AspNetCore.Mvc.TagHelpers +@using HtmlGenerationWebSite.Models +@model WeirdModel + +@functions{ + private string PrivateProperty { get; set; } = "Hello, Private World!"; +} + + + +
+

Field

+ @Html.LabelFor(m => m.Field) + @Html.TextBoxFor(m => m.Field) +
+
+

Static Property

+ @Html.LabelFor(m => WeirdModel.StaticProperty) + @Html.TextBoxFor(m => WeirdModel.StaticProperty) +
+
+

Private Property

+ @Html.LabelFor(m => PrivateProperty) + @Html.TextBoxFor(m => PrivateProperty) +
+ + \ No newline at end of file diff --git a/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_WeirdExpressions/GetWeirdWithTagHelpers.cshtml b/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_WeirdExpressions/GetWeirdWithTagHelpers.cshtml new file mode 100644 index 0000000000..8bf8575cdc --- /dev/null +++ b/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_WeirdExpressions/GetWeirdWithTagHelpers.cshtml @@ -0,0 +1,27 @@ +@addTagHelper *, Microsoft.AspNetCore.Mvc.TagHelpers +@using HtmlGenerationWebSite.Models +@model WeirdModel + +@functions{ + private string PrivateProperty { get; set; } = "Hello, Private World!"; +} + + + +
+

Field

+
+
+

Static Property

+
+
+

Private Property

+
+ + \ No newline at end of file