From 7e166295bac6f4f2d008c9c1db7a82bfca4ade0b Mon Sep 17 00:00:00 2001
From: Doug Bunting <dougbu@microsoft.com>
Date: Tue, 10 Feb 2015 19:01:30 -0800
Subject: [PATCH] Simplify `<select/>` tag helper `multiple` attribute handling
 - #1516 - `allowMultiple == true` when model has a collection type  - ignore
 any `multiple` attribute in Razor source when generating element - simplify
 tests too: fewer error cases

Note Razor author could allow browser user to submit values model binding
will likely ignore: Could mix a `multiple` attribute with a non-collection
`asp-for` expression.
---
 .../Properties/Resources.Designer.cs          |  16 --
 .../Resources.resx                            |   3 -
 .../SelectTagHelper.cs                        |  48 +-----
 .../SelectTagHelperTest.cs                    | 150 ++++--------------
 4 files changed, 40 insertions(+), 177 deletions(-)

diff --git a/src/Microsoft.AspNet.Mvc.TagHelpers/Properties/Resources.Designer.cs b/src/Microsoft.AspNet.Mvc.TagHelpers/Properties/Resources.Designer.cs
index 183d549f26..c46e3e4940 100644
--- a/src/Microsoft.AspNet.Mvc.TagHelpers/Properties/Resources.Designer.cs
+++ b/src/Microsoft.AspNet.Mvc.TagHelpers/Properties/Resources.Designer.cs
@@ -122,22 +122,6 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
             return string.Format(CultureInfo.CurrentCulture, GetString("SelectTagHelper_CannotDetermineContentWhenOnlyItemsSpecified"), p0, p1, p2);
         }
 
-        /// <summary>
-        /// Cannot parse '{1}' value '{2}' for {0}. Acceptable values are '{3}', '{4}' and '{5}'.
-        /// </summary>
-        internal static string TagHelpers_InvalidValue_ThreeAcceptableValues
-        {
-            get { return GetString("TagHelpers_InvalidValue_ThreeAcceptableValues"); }
-        }
-
-        /// <summary>
-        /// Cannot parse '{1}' value '{2}' for {0}. Acceptable values are '{3}', '{4}' and '{5}'.
-        /// </summary>
-        internal static string FormatTagHelpers_InvalidValue_ThreeAcceptableValues(object p0, object p1, object p2, object p3, object p4, object p5)
-        {
-            return string.Format(CultureInfo.CurrentCulture, GetString("TagHelpers_InvalidValue_ThreeAcceptableValues"), p0, p1, p2, p3, p4, p5);
-        }
-
         /// <summary>
         /// The {2} was unable to provide metadata about '{1}' expression value '{3}' for {0}.
         /// </summary>
diff --git a/src/Microsoft.AspNet.Mvc.TagHelpers/Resources.resx b/src/Microsoft.AspNet.Mvc.TagHelpers/Resources.resx
index 43bda290f1..ce38a162fc 100644
--- a/src/Microsoft.AspNet.Mvc.TagHelpers/Resources.resx
+++ b/src/Microsoft.AspNet.Mvc.TagHelpers/Resources.resx
@@ -138,9 +138,6 @@
   <data name="SelectTagHelper_CannotDetermineContentWhenOnlyItemsSpecified" xml:space="preserve">
     <value>Cannot determine body for {0}. '{2}' must be null if '{1}' is null.</value>
   </data>
-  <data name="TagHelpers_InvalidValue_ThreeAcceptableValues" xml:space="preserve">
-    <value>Cannot parse '{1}' value '{2}' for {0}. Acceptable values are '{3}', '{4}' and '{5}'.</value>
-  </data>
   <data name="TagHelpers_NoProvidedMetadata" xml:space="preserve">
     <value>The {2} was unable to provide metadata about '{1}' expression value '{3}' for {0}.</value>
   </data>
diff --git a/src/Microsoft.AspNet.Mvc.TagHelpers/SelectTagHelper.cs b/src/Microsoft.AspNet.Mvc.TagHelpers/SelectTagHelper.cs
index 9a2ebd45c2..e28fb59aa1 100644
--- a/src/Microsoft.AspNet.Mvc.TagHelpers/SelectTagHelper.cs
+++ b/src/Microsoft.AspNet.Mvc.TagHelpers/SelectTagHelper.cs
@@ -8,7 +8,6 @@ using System.Linq;
 using Microsoft.AspNet.Mvc.ModelBinding;
 using Microsoft.AspNet.Mvc.Rendering;
 using Microsoft.AspNet.Razor.Runtime.TagHelpers;
-using Microsoft.AspNet.Razor.TagHelpers;
 
 namespace Microsoft.AspNet.Mvc.TagHelpers
 {
@@ -44,16 +43,6 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
         [HtmlAttributeName(ForAttributeName)]
         public ModelExpression For { get; set; }
 
-        /// <summary>
-        /// Specifies that multiple options can be selected at once.
-        /// </summary>
-        /// <remarks>
-        /// Passed through to the generated HTML if value is <c>multiple</c>. Converted to <c>multiple</c> or absent if
-        /// value is <c>true</c> or <c>false</c>. Other values are not acceptable. Also used to determine the correct
-        /// "selected" attributes for generated &lt;option&gt; elements.
-        /// </remarks>
-        public string Multiple { get; set; }
-
         /// <summary>
         /// A collection of <see cref="SelectListItem"/> objects used to populate the &lt;select&gt; element with
         /// &lt;optgroup&gt; and &lt;option&gt; elements.
@@ -79,12 +68,6 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
                         ItemsAttributeName);
                     throw new InvalidOperationException(message);
                 }
-
-                // Pass through attribute that is also a well-known HTML attribute.
-                if (!string.IsNullOrEmpty(Multiple))
-                {
-                    output.CopyHtmlAttribute(nameof(Multiple), context);
-                }
             }
             else
             {
@@ -100,33 +83,12 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
                         For.Name));
                 }
 
-                bool allowMultiple;
-                if (string.IsNullOrEmpty(Multiple))
-                {
-                    // Base allowMultiple on the instance or declared type of the expression.
-                    var realModelType = For.Metadata.RealModelType;
-                    allowMultiple =
+                // Base allowMultiple on the instance or declared type of the expression to avoid a
+                // "SelectExpressionNotEnumerable" InvalidOperationException during generation.
+                // Metadata.IsCollectionType() is similar but does not take runtime type into account.
+                var realModelType = For.Metadata.RealModelType;
+                var allowMultiple =
                         typeof(string) != realModelType && typeof(IEnumerable).IsAssignableFrom(realModelType);
-                }
-                else if (string.Equals(Multiple, "multiple", StringComparison.OrdinalIgnoreCase))
-                {
-                    allowMultiple = true;
-
-                    // Copy exact attribute name and value the user entered. Must be done prior to any copying from a
-                    // TagBuilder. Not done in next case because "true" and "false" aren't valid for the HTML 5
-                    // attribute.
-                    output.CopyHtmlAttribute(nameof(Multiple), context);
-                }
-                else if (!bool.TryParse(Multiple.ToLowerInvariant(), out allowMultiple))
-                {
-                    throw new InvalidOperationException(Resources.FormatTagHelpers_InvalidValue_ThreeAcceptableValues(
-                        "<select>",
-                        nameof(Multiple).ToLowerInvariant(),
-                        Multiple,
-                        bool.FalseString.ToLowerInvariant(),
-                        bool.TrueString.ToLowerInvariant(),
-                        nameof(Multiple).ToLowerInvariant())); // acceptable value (as well as attribute name)
-                }
 
                 // Ensure GenerateSelect() _never_ looks anything up in ViewData.
                 var items = Items ?? Enumerable.Empty<SelectListItem>();
diff --git a/test/Microsoft.AspNet.Mvc.TagHelpers.Test/SelectTagHelperTest.cs b/test/Microsoft.AspNet.Mvc.TagHelpers.Test/SelectTagHelperTest.cs
index ff8036dbd0..cb14e38953 100644
--- a/test/Microsoft.AspNet.Mvc.TagHelpers.Test/SelectTagHelperTest.cs
+++ b/test/Microsoft.AspNet.Mvc.TagHelpers.Test/SelectTagHelperTest.cs
@@ -87,9 +87,9 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
             }
         }
 
-        // Items value, Multiple value, expected items value (passed to generator), expected allowMultiple.
-        // Provides cross product of Items and Multiple values. These attribute values should not interact.
-        public static TheoryData<IEnumerable<SelectListItem>, string, IEnumerable<SelectListItem>, bool>
+        // Items property value, attribute name, attribute value, expected items value (passed to generator). Provides
+        // cross product of Items and attributes. These values should not interact.
+        public static TheoryData<IEnumerable<SelectListItem>, string, string, IEnumerable<SelectListItem>>
             ItemsAndMultipleDataSet
         {
             get
@@ -106,24 +106,34 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
                     new[] { multiItems, multiItems },
                     new[] { selectItems, selectItems },
                 };
-                var mutlipleData = new[]
+                var attributeData = new Dictionary<string, string>(StringComparer.Ordinal)
                 {
-                    new Tuple<string, bool>(null, false), // allowMultiple determined by string datatype.
-                    new Tuple<string, bool>("", false),   // allowMultiple determined by string datatype.
-                    new Tuple<string, bool>("true", true),
-                    new Tuple<string, bool>("false", false),
-                    new Tuple<string, bool>("multiple", true),
-                    new Tuple<string, bool>("Multiple", true),
-                    new Tuple<string, bool>("MULTIPLE", true),
+                    // SelectTagHelper ignores all "multiple" attribute values.
+                    { "multiple", null },
+                    { "mUltiple", string.Empty },
+                    { "muLtiple", "true" },
+                    { "Multiple", "false" },
+                    { "MUltiple", "multiple" },
+                    { "MuLtiple", "Multiple" },
+                    { "mUlTiPlE", "mUlTiPlE" },
+                    { "mULTiPlE", "MULTIPLE" },
+                    { "mUlTIPlE", "Invalid" },
+                    { "MULTiPLE", "0" },
+                    { "MUlTIPLE", "1" },
+                    { "MULTIPLE", "__true" },
+                    // SelectTagHelper also ignores non-"multiple" attributes.
+                    { "multiple_", "multiple" },
+                    { "not-multiple", "multiple" },
+                    { "__multiple", "multiple" },
                 };
 
                 var theoryData =
-                    new TheoryData<IEnumerable<SelectListItem>, string, IEnumerable<SelectListItem>, bool>();
+                    new TheoryData<IEnumerable<SelectListItem>, string, string, IEnumerable<SelectListItem>>();
                 foreach (var items in itemsData)
                 {
-                    foreach (var multiples in mutlipleData)
+                    foreach (var attribute in attributeData)
                     {
-                        theoryData.Add(items[0], multiples.Item1, items[1], multiples.Item2);
+                        theoryData.Add(items[0], attribute.Key, attribute.Value, items[1]);
                     }
                 }
 
@@ -315,19 +325,22 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
 
         [Theory]
         [MemberData(nameof(ItemsAndMultipleDataSet))]
-        public async Task ProcessAsync_CallsGeneratorWithExpectedValues_ItemsAndMultiple(
+        public async Task ProcessAsync_CallsGeneratorWithExpectedValues_ItemsAndAttribute(
             IEnumerable<SelectListItem> inputItems,
-            string multiple,
-            IEnumerable<SelectListItem> expectedItems,
-            bool expectedAllowMultiple)
+            string attributeName,
+            string attributeValue,
+            IEnumerable<SelectListItem> expectedItems)
         {
             // Arrange
             var contextAttributes = new Dictionary<string, object>
             {
-                // Attribute will be restored if value matches "multiple".
-                { "multiple", multiple },
+                // Provided for completeness. Select tag helper does not confirm AllAttributes set is consistent.
+                { attributeName, attributeValue },
+            };
+            var originalAttributes = new Dictionary<string, string>
+            {
+                { attributeName, attributeValue },
             };
-            var originalAttributes = new Dictionary<string, string>();
             var propertyName = "Property1";
             var expectedTagName = "select";
 
@@ -356,7 +369,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
                     null,         // optionLabel
                     string.Empty, // name
                     expectedItems,
-                    expectedAllowMultiple,
+                    false,        // allowMultiple
                     null,         // htmlAttributes
                     out selectedValues))
                 .Returns((TagBuilder)null)
@@ -367,7 +380,6 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
                 For = modelExpression,
                 Items = inputItems,
                 Generator = htmlGenerator.Object,
-                Multiple = multiple,
                 ViewContext = viewContext,
             };
 
@@ -443,58 +455,6 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
             Assert.Same(selectedValues, keyValuePair.Value);
         }
 
-        [Theory]
-        [InlineData("multiple")]
-        [InlineData("mUlTiPlE")]
-        [InlineData("MULTIPLE")]
-        public async Task ProcessAsync_RestoresMultiple_IfForNotBound(string attributeName)
-        {
-            // Arrange
-            var contextAttributes = new Dictionary<string, object>
-            {
-                { attributeName, "I'm more than one" },
-            };
-            var originalAttributes = new Dictionary<string, string>
-            {
-                { "class", "form-control" },
-                { "size", "2" },
-            };
-            var expectedAttributes = new Dictionary<string, string>(originalAttributes);
-            expectedAttributes[attributeName] = (string)contextAttributes[attributeName];
-            var expectedPreContent = "original pre-content";
-            var expectedContent = "original content";
-            var expectedPostContent = "original post-content";
-            var expectedTagName = "select";
-
-            var tagHelperContext = new TagHelperContext(
-                contextAttributes,
-                uniqueId: "test",
-                getChildContentAsync: () => Task.FromResult("Something"));
-            var output = new TagHelperOutput(expectedTagName, originalAttributes)
-            {
-                PreContent = expectedPreContent,
-                Content = expectedContent,
-                PostContent = expectedPostContent,
-                SelfClosing = true,
-            };
-
-            var tagHelper = new SelectTagHelper
-            {
-                Multiple = "I'm more than one",
-            };
-
-            // Act
-            await tagHelper.ProcessAsync(tagHelperContext, output);
-
-            // Assert
-            Assert.Equal(expectedAttributes, output.Attributes);
-            Assert.Equal(expectedPreContent, output.PreContent);
-            Assert.Equal(expectedContent, output.Content);
-            Assert.Equal(expectedPostContent, output.PostContent);
-            Assert.True(output.SelfClosing);
-            Assert.Equal(expectedTagName, output.TagName);
-        }
-
         [Fact]
         public async Task ProcessAsync_Throws_IfForNotBoundButItemsIs()
         {
@@ -520,46 +480,6 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
             Assert.Equal(expectedMessage, exception.Message);
         }
 
-        [Theory]
-        [InlineData("Invalid")]
-        [InlineData("0")]
-        [InlineData("1")]
-        [InlineData("__true")]
-        [InlineData("false__")]
-        [InlineData("__Multiple")]
-        [InlineData("Multiple__")]
-        public async Task ProcessAsync_Throws_IfMultipleInvalid(string multiple)
-        {
-            // Arrange
-            var contextAttributes = new Dictionary<string, object>();
-            var originalAttributes = new Dictionary<string, string>();
-            var expectedTagName = "select";
-            var expectedMessage = "Cannot parse 'multiple' value '" + multiple +
-                "' for <select>. Acceptable values are 'false', 'true' and 'multiple'.";
-
-            var tagHelperContext = new TagHelperContext(
-                contextAttributes,
-                uniqueId: "test",
-                getChildContentAsync: () => Task.FromResult("Something"));
-            var output = new TagHelperOutput(expectedTagName, originalAttributes);
-
-            var metadataProvider = new EmptyModelMetadataProvider();
-            string model = null;
-            var metadata = metadataProvider.GetMetadataForType(() => model, typeof(string));
-            var modelExpression = new ModelExpression("Property1", metadata);
-
-            var tagHelper = new SelectTagHelper
-            {
-                For = modelExpression,
-                Multiple = multiple,
-            };
-
-            // Act & Assert
-            var exception = await Assert.ThrowsAsync<InvalidOperationException>(
-                () => tagHelper.ProcessAsync(tagHelperContext, output));
-            Assert.Equal(expectedMessage, exception.Message);
-        }
-
         public class NameAndId
         {
             public NameAndId(string name, string id)