From a045324d3a7bd29f030a120d2edd90fedb3fb5be Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Sun, 23 Aug 2015 14:22:49 -0700 Subject: [PATCH] Do not include compiler-generated names in expression names - #2890 - add lots of `ExpressionHelper` tests using `IdFor()` and `NameFor()` (which are thin veneers) --- .../Rendering/Expressions/ExpressionHelper.cs | 23 +++- .../Rendering/HtmlHelperNameExtensionsTest.cs | 124 ++++++++++++++++-- 2 files changed, 132 insertions(+), 15 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.ViewFeatures/Rendering/Expressions/ExpressionHelper.cs b/src/Microsoft.AspNet.Mvc.ViewFeatures/Rendering/Expressions/ExpressionHelper.cs index c0f6e699be..b27caa123b 100644 --- a/src/Microsoft.AspNet.Mvc.ViewFeatures/Rendering/Expressions/ExpressionHelper.cs +++ b/src/Microsoft.AspNet.Mvc.ViewFeatures/Rendering/Expressions/ExpressionHelper.cs @@ -31,9 +31,9 @@ public static string GetExpressionText([NotNull] LambdaExpression expression) if (part.NodeType == ExpressionType.Call) { var methodExpression = (MethodCallExpression)part; - if (!IsSingleArgumentIndexer(methodExpression)) { + // Unsupported. break; } @@ -58,7 +58,18 @@ public static string GetExpressionText([NotNull] LambdaExpression expression) else if (part.NodeType == ExpressionType.MemberAccess) { var memberExpressionPart = (MemberExpression)part; - nameParts.Push("." + memberExpressionPart.Member.Name); + var name = memberExpressionPart.Member.Name; + + // If identifier contains "__", it is "reserved for use by the implementation" and likely compiler- + // or Razor-generated e.g. the name of a field in a delegate's generated class. + if (name.Contains("__")) + { + // Exit loop. Should have the entire name because previous MemberAccess has same name as the + // leftmost expression node (a variable). + break; + } + + nameParts.Push("." + name); part = memberExpressionPart.Expression; } else if (part.NodeType == ExpressionType.Parameter) @@ -67,15 +78,19 @@ public static string GetExpressionText([NotNull] LambdaExpression expression) // string onto the stack and stop evaluating. The extra empty string makes sure that // we don't accidentally cut off too much of m => m.Model. nameParts.Push(string.Empty); - part = null; + + // Exit loop. Have the entire name because the parameter cannot be used as an indexer; always the + // leftmost expression node. + break; } else { + // Unsupported. break; } } - // If it starts with "model", then strip that away + // If parts start with "model", then strip that part away. if (nameParts.Count > 0 && string.Equals(nameParts.Peek(), ".model", StringComparison.OrdinalIgnoreCase)) { nameParts.Pop(); diff --git a/test/Microsoft.AspNet.Mvc.ViewFeatures.Test/Rendering/HtmlHelperNameExtensionsTest.cs b/test/Microsoft.AspNet.Mvc.ViewFeatures.Test/Rendering/HtmlHelperNameExtensionsTest.cs index cc16e497ea..d3189337f2 100644 --- a/test/Microsoft.AspNet.Mvc.ViewFeatures.Test/Rendering/HtmlHelperNameExtensionsTest.cs +++ b/test/Microsoft.AspNet.Mvc.ViewFeatures.Test/Rendering/HtmlHelperNameExtensionsTest.cs @@ -1,9 +1,13 @@ // 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.Collections.Generic; +using System.Linq.Expressions; using Microsoft.AspNet.Mvc.ModelBinding; using Microsoft.AspNet.Mvc.ModelBinding.Metadata; using Microsoft.AspNet.Mvc.Rendering; +using Microsoft.AspNet.Testing; using Moq; using Xunit; @@ -14,6 +18,26 @@ namespace Microsoft.AspNet.Mvc.Core /// public class HtmlHelperNameExtensionsTest { + private static List _staticCollection = new List(); + private static int _staticIndex = 6; + + private List _collection = new List(); + private int _index = 7; + private List _nestedCollection = new List(); + private string _string = string.Empty; + + private static List StaticCollection { get; } + + private static int StaticIndex { get; } = 8; + + private List Collection { get; } + + private int Index { get; } = 9; + + private List NestedCollection { get; } + + private string StringProperty { get; } + [Fact] public void IdAndNameHelpers_ReturnEmptyForModel() { @@ -140,7 +164,7 @@ public void IdAndNameHelpers_DoNotConsultMetadataOrMetadataProvider() // Arrange var provider = new Mock(MockBehavior.Strict); var metadata = new Mock( - MockBehavior.Loose, + MockBehavior.Loose, ModelMetadataIdentity.ForType(typeof(DefaultTemplatesUtilities.ObjectTemplateModel))); provider .Setup(m => m.GetMetadataForType(typeof(DefaultTemplatesUtilities.ObjectTemplateModel))) @@ -223,28 +247,106 @@ public void IdForAndNameFor_ReturnEmpty_IfExpressionUnsupported() Assert.Empty(nameResult); } - [Fact] - public void IdForAndNameFor_ReturnVariableName() + // expression, expected name, expected id + public static TheoryData StaticExpressionNamesData + { + get + { + // Give expressions access to non-static fields and properties. + var test = new HtmlHelperNameExtensionsTest(); + return test.ExpressionNamesData; + } + } + + // expression, expected name, expected id + private TheoryData ExpressionNamesData + { + get + { + var collection = new List(); + var nestedCollection = new List(); + var index1 = 5; + var index2 = 23; + var unknownKey = "this is a dummy parameter value"; + + return new TheoryData, string>>, string, string> + { + { m => unknownKey, "unknownKey", "unknownKey" }, + { m => collection[index1], "collection[5]", "collection_5_" }, + { m => nestedCollection[index1][23], "nestedCollection[5][23]", "nestedCollection_5__23_" }, + { m => nestedCollection[index1][index2], "nestedCollection[5][23]", "nestedCollection_5__23_" }, + { m => nestedCollection[_index][Index], "nestedCollection[7][9]", "nestedCollection_7__9_" }, + { m => nestedCollection[Index][StaticIndex], "nestedCollection[9][8]", "nestedCollection_9__8_" }, + { m => _string, "_string", "zstring" }, + { m => _collection[_index], "_collection[7]", "zcollection_7_" }, + { m => _nestedCollection[_index][23], "_nestedCollection[7][23]", "znestedCollection_7__23_" }, + { m => _nestedCollection[_index][index2], "_nestedCollection[7][23]", "znestedCollection_7__23_" }, + { m => _nestedCollection[Index][_staticIndex], "_nestedCollection[9][6]", "znestedCollection_9__6_" }, + { m => _nestedCollection[StaticIndex][_index], "_nestedCollection[8][7]", "znestedCollection_8__7_" }, + { m => StringProperty, "StringProperty", "StringProperty" }, + { m => Collection[Index], "Collection[9]", "Collection_9_" }, + { m => NestedCollection[Index][23], "NestedCollection[9][23]", "NestedCollection_9__23_" }, + { m => NestedCollection[Index][index2], "NestedCollection[9][23]", "NestedCollection_9__23_" }, + { m => NestedCollection[_index][Index], "NestedCollection[7][9]", "NestedCollection_7__9_" }, + { m => NestedCollection[Index][StaticIndex], "NestedCollection[9][8]", "NestedCollection_9__8_" }, + { m => _staticCollection[_staticIndex], "_staticCollection[6]", "zstaticCollection_6_" }, + { m => _staticCollection[Index], "_staticCollection[9]", "zstaticCollection_9_" }, + { m => _staticCollection[_index], "_staticCollection[7]", "zstaticCollection_7_" }, + { m => StaticCollection[StaticIndex], "StaticCollection[8]", "StaticCollection_8_" }, + { m => StaticCollection[_staticIndex], "StaticCollection[6]", "StaticCollection_6_" }, + { m => StaticCollection[index1], "StaticCollection[5]", "StaticCollection_5_" }, + { m => m[index1].Inner.Name, "[5].Inner.Name", "z5__Inner_Name" }, + { m => m[_staticIndex].Inner.Name, "[6].Inner.Name", "z6__Inner_Name" }, + { m => m[_index].Inner.Name, "[7].Inner.Name", "z7__Inner_Name" }, + { m => m[StaticIndex].Inner.Name, "[8].Inner.Name", "z8__Inner_Name" }, + { m => m[Index].Inner.Name, "[9].Inner.Name", "z9__Inner_Name" }, + }; + } + } + + [Theory] + [MemberData(nameof(StaticExpressionNamesData))] + public void IdForAndNameFor_ReturnExpectedValues_WithVariablesInExpression( + Expression, string>> expression, + string expectedName, + string expectedId) { // Arrange - var unknownKey = "this is a dummy parameter value"; - var helper = DefaultTemplatesUtilities.GetHtmlHelper(); + var model = new List(); + var helper = DefaultTemplatesUtilities.GetHtmlHelper(model); // Act - var idResult = helper.IdFor(model => unknownKey); - var nameResult = helper.NameFor(model => unknownKey); + var idResult = helper.IdFor(expression); + var nameResult = helper.NameFor(expression); // Assert - Assert.Equal("unknownKey", idResult); - Assert.Equal("unknownKey", nameResult); + Assert.Equal(expectedId, idResult); + Assert.Equal(expectedName, nameResult); } - private sealed class InnerClass + [Fact] + public void IdForAndNameFor_Throws_WhenParameterUsedAsIndexer() + { + // Arrange + var collection = new List(); + var index = 24; + var helper = DefaultTemplatesUtilities.GetHtmlHelper(index); + var message = "The expression compiler was unable to evaluate the indexer expression 'm' because it " + + "references the model parameter 'm' which is unavailable."; + + // Act & Assert + ExceptionAssert.Throws(() => helper.IdFor(m => collection[m]), message); + ExceptionAssert.Throws(() => helper.NameFor(m => collection[m]), message); + } + + public sealed class InnerClass { public int Id { get; set; } + + public string Name { get; set; } } - private sealed class OuterClass + public sealed class OuterClass { public InnerClass Inner { get; set; } }