huahaofeng
/
aspnetcore
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Check encoded and unencoded values against element body in `OptionTagHelper` 

- #3386
- initialize comparison `HashSet` with unencoded values to ensure both are checked
- address perf and correctness issues in this code
  - `context.Items[typeof(SelectTagHelper)]` entry read as `ICollection` but written as `IReadOnlyCollection`
    - `IReadOnlyCollection` worse because it does not include `Contains()`, causing Linq use
  - every `<option>` element recalculated the encoded values and created a `HashSet` to contain them
    - add `CurrentValues` type to cache this `HashSet` in `context.Items`
  - each `OptionTagHelper` created the additional `HashSet` even if `Value` was bound
This commit is contained in:
Doug Bunting 2016-01-07 09:09:16 -08:00
parent 82381d97c2
commit fa8c2eac3e
7 changed files with 107 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
src/Microsoft.AspNet.Mvc.TagHelpers/Internal/CurrentValues.cs Normal file
View File

@ -0,0 +1,21 @@
// 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.Collections.Generic;
using System.Diagnostics;
namespace Microsoft.AspNet.Mvc.TagHelpers.Internal
{
public class CurrentValues
{
public CurrentValues(ICollection<string> values)
{
Debug.Assert(values != null);
Values = values;
}
public ICollection<string> Values { get; }
public ICollection<string> ValuesAndEncodedValues { get; set; }
}
}

54
src/Microsoft.AspNet.Mvc.TagHelpers/OptionTagHelper.cs
View File

@ -5,6 +5,7 @@ using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using Microsoft.AspNet.Mvc.Rendering;
using Microsoft.AspNet.Mvc.TagHelpers.Internal;
using Microsoft.AspNet.Mvc.ViewFeatures;
using Microsoft.AspNet.Razor.TagHelpers;
@ -55,8 +56,8 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
/// <inheritdoc />
/// <remarks>
/// Does nothing unless <see cref="TagHelperContext.Items"/> contains a
/// <see cref="SelectTagHelper"/> <see cref="Type"/> entry and that entry is a non-empty
/// <see cref="ICollection{string}"/> instance. Also does nothing if the associated &lt;option&gt; is already
/// <see cref="SelectTagHelper"/> <see cref="Type"/> entry and that entry is a non-<c>null</c>
/// <see cref="CurrentValues"/> instance. Also does nothing if the associated &lt;option&gt; is already
/// selected.
/// </remarks>
public override async Task ProcessAsync(TagHelperContext context, TagHelperOutput output)
@ -85,32 +86,43 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
context.Items.TryGetValue(typeof(SelectTagHelper), out formDataEntry);
// ... And did the SelectTagHelper determine any selected values?
var selectedValues = formDataEntry as ICollection<string>;
if (selectedValues != null && selectedValues.Count != 0)
var currentValues = formDataEntry as CurrentValues;
if (currentValues?.Values != null && currentValues.Values.Count != 0)
{
// Encode all selected values for comparison with element content.
var encodedValues = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
foreach (var selectedValue in selectedValues)
{
encodedValues.Add(Generator.Encode(selectedValue));
}
// Select this <option/> element if value attribute or content matches a selected value. Callers
// encode values as-needed while executing child content. But TagHelperOutput itself
// encodes attribute values later, when start tag is generated.
// Select this <option/> element if value attribute or (if no value attribute) body matches a
// selected value. Body is encoded as-needed while executing child content. But TagHelperOutput
// itself encodes attribute values later, when start tag is generated.
bool selected;
if (Value != null)
{
selected = selectedValues.Contains(Value);
}
else if (output.IsContentModified)
{
selected = encodedValues.Contains(output.Content.GetContent());
selected = currentValues.Values.Contains(Value);
}
else
{
var childContent = await output.GetChildContentAsync();
selected = encodedValues.Contains(childContent.GetContent());
if (currentValues.ValuesAndEncodedValues == null)
{
// Include encoded versions of all selected values when comparing with body.
var allValues = new HashSet<string>(currentValues.Values, StringComparer.OrdinalIgnoreCase);
foreach (var selectedValue in currentValues.Values)
{
allValues.Add(Generator.Encode(selectedValue));
}
currentValues.ValuesAndEncodedValues = allValues;
}
TagHelperContent childContent;
if (output.IsContentModified)
{
// Another tag helper has modified the body. Use what they wrote.
childContent = output.Content;
}
else
{
childContent = await output.GetChildContentAsync();
}
selected = currentValues.ValuesAndEncodedValues.Contains(childContent.GetContent());
}
if (selected)

6
src/Microsoft.AspNet.Mvc.TagHelpers/SelectTagHelper.cs
View File

@ -8,6 +8,7 @@ using System.Linq;
using System.Reflection;
using Microsoft.AspNet.Mvc.ModelBinding;
using Microsoft.AspNet.Mvc.Rendering;
using Microsoft.AspNet.Mvc.TagHelpers.Internal;
using Microsoft.AspNet.Mvc.ViewFeatures;
using Microsoft.AspNet.Razor.TagHelpers;
@ -22,7 +23,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
private const string ForAttributeName = "asp-for";
private const string ItemsAttributeName = "asp-items";
private bool _allowMultiple;
private IReadOnlyCollection<string> _currentValues;
private ICollection<string> _currentValues;
/// <summary>
/// Creates a new <see cref="SelectTagHelper"/>.
@ -94,7 +95,8 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
// Whether or not (not being highly unlikely) we generate anything, could update contained <option/>
// elements. Provide selected values for <option/> tag helpers.
context.Items[typeof(SelectTagHelper)] = _currentValues;
var currentValues = _currentValues == null ? null : new CurrentValues(_currentValues);
context.Items[typeof(SelectTagHelper)] = currentValues;
}
/// <inheritdoc />

10
src/Microsoft.AspNet.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs
View File

@ -511,7 +511,7 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures
string optionLabel,
string expression,
IEnumerable<SelectListItem> selectList,
IReadOnlyCollection<string> currentValues,
ICollection<string> currentValues,
bool allowMultiple,
object htmlAttributes)
{
@ -886,7 +886,7 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures
}
/// <inheritdoc />
public virtual IReadOnlyCollection<string> GetCurrentValues(
public virtual ICollection<string> GetCurrentValues(
ViewContext viewContext,
ModelExplorer modelExplorer,
string expression,
@ -1026,9 +1026,7 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures
}
}
// HashSet<> implements IReadOnlyCollection<> as of 4.6, but does not for 4.5.1. If the runtime cast succeeds,
// avoid creating a new collection.
return (currentValues as IReadOnlyCollection<string>) ?? currentValues.ToArray();
return currentValues;
}
internal static string EvalString(ViewContext viewContext, string key, string format)
@ -1407,7 +1405,7 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures
private static IEnumerable<SelectListItem> UpdateSelectListItemsWithDefaultValue(
ModelExplorer modelExplorer,
IEnumerable<SelectListItem> selectList,
IReadOnlyCollection<string> currentValues)
ICollection<string> currentValues)
{
// Perform deep copy of selectList to avoid changing user's Selected property values.
var newSelectList = new List<SelectListItem>();

10
src/Microsoft.AspNet.Mvc.ViewFeatures/ViewFeatures/IHtmlGenerator.cs
View File

@ -273,7 +273,7 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures
/// <c>ViewContext.ViewData[expression]</c>.
/// </param>
/// <param name="currentValues">
/// An <see cref="IReadOnlyCollection{string}"/> containing values for &lt;option&gt; elements to select. If
/// An <see cref="ICollection{string}"/> containing values for &lt;option&gt; elements to select. If
/// <c>null</c>, selects &lt;option&gt; elements based on <see cref="SelectListItem.Selected"/> values in
/// <paramref name="selectList"/>.
/// </param>
@ -303,7 +303,7 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures
string optionLabel,
string expression,
IEnumerable<SelectListItem> selectList,
IReadOnlyCollection<string> currentValues,
ICollection<string> currentValues,
bool allowMultiple,
object htmlAttributes);
@ -361,8 +361,8 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures
/// </param>
/// <returns>
/// <para>
/// <c>null</c> if no <paramref name="expression"/> result is found. Otherwise an
/// <see cref="IReadOnlyCollection{string}"/> containing current values for the given
/// <c>null</c> if no <paramref name="expression"/> result is found. Otherwise a
/// <see cref="ICollection{string}"/> containing current values for the given
/// <paramref name="expression"/>.
/// </para>
/// <para>
@ -380,7 +380,7 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures
/// <remarks>
/// See <see cref="GenerateSelect"/> for information about how the return value may be used.
/// </remarks>
IReadOnlyCollection<string> GetCurrentValues(
ICollection<string> GetCurrentValues(
ViewContext viewContext,
ModelExplorer modelExplorer,
string expression,

64
test/Microsoft.AspNet.Mvc.TagHelpers.Test/OptionTagHelperTest.cs
View File

@ -5,6 +5,7 @@ using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNet.Mvc.ModelBinding;
using Microsoft.AspNet.Mvc.TagHelpers.Internal;
using Microsoft.AspNet.Razor.TagHelpers;
using Xunit;
@ -14,11 +15,11 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
{
// Original content, selected attribute, value attribute, selected values (to place in TagHelperContext.Items)
// and expected tag helper output.
public static TheoryData<string, string, string, IEnumerable<string>, TagHelperOutput> GeneratesExpectedDataSet
public static TheoryData<string, string, string, CurrentValues, TagHelperOutput> GeneratesExpectedDataSet
{
get
{
return new TheoryData<string, string, string, IEnumerable<string>, TagHelperOutput>
return new TheoryData<string, string, string, CurrentValues, TagHelperOutput>
{
// original content, selected, value, selected values,
// expected tag helper output - attributes, content
@ -53,7 +54,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"")
},
{
null, null, "value", Enumerable.Empty<string>(),
null, null, "value", new CurrentValues(new HashSet<string>()),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -63,7 +64,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"")
},
{
null, null, "value", new [] { string.Empty, },
null, null, "value", new CurrentValues(new HashSet<string>(new [] { string.Empty, })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -73,7 +74,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"")
},
{
null, string.Empty, "value", new [] { string.Empty, },
null, string.Empty, "value", new CurrentValues(new HashSet<string>(new [] { string.Empty, })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -83,7 +84,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"")
},
{
null, null, "value", new [] { "value", },
null, null, "value", new CurrentValues(new HashSet<string>(new [] { "value", })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -93,7 +94,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"")
},
{
null, null, "value", new [] { string.Empty, "value", },
null, null, "value", new CurrentValues(new HashSet<string>(new [] { string.Empty, "value", })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -133,7 +134,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"")
},
{
string.Empty, null, null, Enumerable.Empty<string>(),
string.Empty, null, null, new CurrentValues(new HashSet<string>()),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -143,7 +144,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"")
},
{
string.Empty, null, null, new [] { string.Empty, },
string.Empty, null, null, new CurrentValues(new HashSet<string>(new [] { string.Empty, })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -153,7 +154,8 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"")
},
{
string.Empty, string.Empty, null, new [] { string.Empty, },
string.Empty, string.Empty, null,
new CurrentValues(new HashSet<string>(new [] { string.Empty, })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -163,7 +165,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"")
},
{
string.Empty, null, null, new [] { "text", },
string.Empty, null, null, new CurrentValues(new HashSet<string>(new [] { "text", })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -173,7 +175,8 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"")
},
{
string.Empty, null, null, new [] { string.Empty, "text", },
string.Empty, null, null,
new CurrentValues(new HashSet<string>(new [] { string.Empty, "text", })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -213,7 +216,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"text")
},
{
"text", null, null, Enumerable.Empty<string>(),
"text", null, null, new CurrentValues(new HashSet<string>()),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -223,7 +226,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"text")
},
{
"text", null, null, new [] { string.Empty, },
"text", null, null, new CurrentValues(new HashSet<string>(new [] { string.Empty, })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -233,7 +236,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"text")
},
{
"HtmlEncode[[text]]", null, null, new [] { "text", },
"HtmlEncode[[text]]", null, null, new CurrentValues(new HashSet<string>(new [] { "text", })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -243,7 +246,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"HtmlEncode[[text]]")
},
{
"text", string.Empty, null, new [] { "text", },
"text", string.Empty, null, new CurrentValues(new HashSet<string>(new [] { "text", })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -253,7 +256,8 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"text")
},
{
"HtmlEncode[[text]]", null, null, new [] { string.Empty, "text", },
"HtmlEncode[[text]]", null, null,
new CurrentValues(new HashSet<string>(new [] { string.Empty, "text", })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -283,7 +287,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"text")
},
{
"text", null, "value", Enumerable.Empty<string>(),
"text", null, "value", new CurrentValues(new HashSet<string>()),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -293,7 +297,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"text")
},
{
"text", null, "value", new [] { string.Empty, },
"text", null, "value", new CurrentValues(new HashSet<string>(new [] { string.Empty, })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -303,7 +307,8 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"text")
},
{
"text", string.Empty, "value", new [] { string.Empty, },
"text", string.Empty, "value",
new CurrentValues(new HashSet<string>(new [] { string.Empty, })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -313,7 +318,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"text")
},
{
"text", null, "value", new [] { "text", },
"text", null, "value", new CurrentValues(new HashSet<string>(new [] { "text", })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -323,7 +328,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"text")
},
{
"text", null, "value", new [] { "value", },
"text", null, "value", new CurrentValues(new HashSet<string>(new [] { "value", })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -333,7 +338,8 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
"text")
},
{
"text", null, "value", new [] { string.Empty, "value", },
"text", null, "value",
new CurrentValues(new HashSet<string>(new [] { string.Empty, "value", })),
GetTagHelperOutput(
"not-option",
new TagHelperAttributeList
@ -354,7 +360,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
get
{
return GeneratesExpectedDataSet.Where(
entry => (entry[1] != null || entry[3] == null || (entry[3] as ICollection<string>).Count == 0));
entry => (entry[1] != null || entry[3] == null || ((CurrentValues)(entry[3])).Values.Count == 0));
}
}
@ -375,7 +381,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
string originalContent,
string selected,
string value,
IEnumerable<string> selectedValues,
CurrentValues currentValues,
TagHelperOutput expectedTagHelperOutput)
{
// Arrange
@ -420,7 +426,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
model: null,
htmlGenerator: htmlGenerator,
metadataProvider: metadataProvider);
tagHelperContext.Items[typeof(SelectTagHelper)] = selectedValues;
tagHelperContext.Items[typeof(SelectTagHelper)] = currentValues;
var tagHelper = new OptionTagHelper(htmlGenerator)
{
Value = value,
@ -446,7 +452,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
string originalContent,
string selected,
string value,
IEnumerable<string> selectedValues,
CurrentValues currentValues,
TagHelperOutput ignored)
{
// Arrange
@ -491,7 +497,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
model: null,
htmlGenerator: htmlGenerator,
metadataProvider: metadataProvider);
tagHelperContext.Items[typeof(SelectTagHelper)] = selectedValues;
tagHelperContext.Items[typeof(SelectTagHelper)] = currentValues;
var tagHelper = new OptionTagHelper(htmlGenerator)
{
Value = value,
@ -509,7 +515,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
string originalContent,
string selected,
string value,
IEnumerable<string> ignoredValues,
CurrentValues ignoredValues,
TagHelperOutput ignoredOutput)
{
// Arrange

7
test/Microsoft.AspNet.Mvc.TagHelpers.Test/SelectTagHelperTest.cs
View File

@ -8,6 +8,7 @@ using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNet.Mvc.ModelBinding;
using Microsoft.AspNet.Mvc.Rendering;
using Microsoft.AspNet.Mvc.TagHelpers.Internal;
using Microsoft.AspNet.Mvc.TestCommon;
using Microsoft.AspNet.Mvc.ViewFeatures;
using Microsoft.AspNet.Razor.TagHelpers;
@ -544,7 +545,8 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
var keyValuePair = Assert.Single(
tagHelperContext.Items,
entry => (Type)entry.Key == typeof(SelectTagHelper));
Assert.Same(currentValues, keyValuePair.Value);
var actualCurrentValues = Assert.IsType<CurrentValues>(keyValuePair.Value);
Assert.Same(currentValues, actualCurrentValues.Values);
}
[Theory]
@ -618,7 +620,8 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
var keyValuePair = Assert.Single(
tagHelperContext.Items,
entry => (Type)entry.Key == typeof(SelectTagHelper));
Assert.Same(currentValues, keyValuePair.Value);
var actualCurrentValues = Assert.IsType<CurrentValues>(keyValuePair.Value);
Assert.Same(currentValues, actualCurrentValues.Values);
}
public class NameAndId