From c5ed31efe7b67c4389b8b5916cd6ac343de9c11f Mon Sep 17 00:00:00 2001 From: ladeak Date: Mon, 1 Apr 2024 17:47:50 +0200 Subject: [PATCH 1/5] TagBuilder using SearchValues - Adding further tests and a new benchmark to compare before-after performance - Updating the implementation of TagBuilder to use SearchValues instead manual operation --- .../src/Rendering/TagBuilder.cs | 76 ++++++------------- .../test/Rendering/TagBuilderTest.cs | 27 +++++++ .../TagBuilderBenchmark.cs | 16 ++++ 3 files changed, 66 insertions(+), 53 deletions(-) create mode 100644 src/Mvc/perf/Microbenchmarks/Microsoft.AspNetCore.Mvc/TagBuilderBenchmark.cs diff --git a/src/Mvc/Mvc.ViewFeatures/src/Rendering/TagBuilder.cs b/src/Mvc/Mvc.ViewFeatures/src/Rendering/TagBuilder.cs index 255c6a7b0576..02425ffef153 100644 --- a/src/Mvc/Mvc.ViewFeatures/src/Rendering/TagBuilder.cs +++ b/src/Mvc/Mvc.ViewFeatures/src/Rendering/TagBuilder.cs @@ -3,6 +3,7 @@ #nullable enable +using System.Buffers; using System.Diagnostics; using System.Globalization; using System.Text; @@ -19,6 +20,11 @@ namespace Microsoft.AspNetCore.Mvc.Rendering; [DebuggerDisplay("{DebuggerToString()}")] public class TagBuilder : IHtmlContent { + // Note '.' is valid according to the HTML 4.01 specification. Disallowed here to avoid + // confusion with CSS class selectors or when using jQuery. + private static readonly SearchValues _html401IdChars = + SearchValues.Create("-0123456789:ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz"); + private AttributeDictionary? _attributes; private HtmlContentBuilder? _innerHtml; @@ -154,51 +160,39 @@ public static string CreateSanitizedId(string? name, string invalidCharReplaceme } // If there are no invalid characters in the string, then we don't have to create the buffer. - var firstIndexOfInvalidCharacter = 1; - for (; firstIndexOfInvalidCharacter < name.Length; firstIndexOfInvalidCharacter++) + var indexOfInvalidCharacter = name.AsSpan(1).IndexOfAnyExcept(_html401IdChars); + var firstChar = name[0]; + var startsWithAsciiLetter = char.IsAsciiLetter(firstChar); + if (startsWithAsciiLetter && indexOfInvalidCharacter < 0) { - if (!Html401IdUtil.IsValidIdCharacter(name[firstIndexOfInvalidCharacter])) - { - break; - } + return name; } - var firstChar = name[0]; - var startsWithAsciiLetter = char.IsAsciiLetter(firstChar); if (!startsWithAsciiLetter) { // The first character must be a letter according to the HTML 4.01 specification. firstChar = 'z'; } - if (firstIndexOfInvalidCharacter == name.Length && startsWithAsciiLetter) - { - return name; - } - var stringBuffer = new StringBuilder(name.Length); stringBuffer.Append(firstChar); + var remainingName = name.AsSpan(1); - // Characters until 'firstIndexOfInvalidCharacter' have already been checked for validity. - // So just copy them. This avoids running them through Html401IdUtil.IsValidIdCharacter again. - for (var index = 1; index < firstIndexOfInvalidCharacter; index++) - { - stringBuffer.Append(name[index]); - } - - for (var index = firstIndexOfInvalidCharacter; index < name.Length; index++) + // Copy values until an invalid character found. Replace the invalid character with the replacement string + // and search for the next invalid character. + while (remainingName.Length > 0) { - var thisChar = name[index]; - if (Html401IdUtil.IsValidIdCharacter(thisChar)) + if (indexOfInvalidCharacter < 0) { - stringBuffer.Append(thisChar); - } - else - { - stringBuffer.Append(invalidCharReplacement); + stringBuffer.Append(remainingName); + break; } - } + stringBuffer.Append(remainingName.Slice(0, indexOfInvalidCharacter)); + stringBuffer.Append(invalidCharReplacement); + remainingName = remainingName.Slice(indexOfInvalidCharacter + 1); + indexOfInvalidCharacter = remainingName.IndexOfAnyExcept(_html401IdChars); + } return stringBuffer.ToString(); } @@ -418,28 +412,4 @@ public void WriteTo(TextWriter writer, HtmlEncoder encoder) TagBuilder.WriteTo(_tagBuilder, writer, encoder, _tagRenderMode); } } - - private static class Html401IdUtil - { - public static bool IsValidIdCharacter(char testChar) - { - return char.IsAsciiLetterOrDigit(testChar) || IsAllowableSpecialCharacter(testChar); - } - - private static bool IsAllowableSpecialCharacter(char testChar) - { - switch (testChar) - { - case '-': - case '_': - case ':': - // Note '.' is valid according to the HTML 4.01 specification. Disallowed here to avoid - // confusion with CSS class selectors or when using jQuery. - return true; - - default: - return false; - } - } - } } diff --git a/src/Mvc/Mvc.ViewFeatures/test/Rendering/TagBuilderTest.cs b/src/Mvc/Mvc.ViewFeatures/test/Rendering/TagBuilderTest.cs index 1dc83a025870..8d040399abe8 100644 --- a/src/Mvc/Mvc.ViewFeatures/test/Rendering/TagBuilderTest.cs +++ b/src/Mvc/Mvc.ViewFeatures/test/Rendering/TagBuilderTest.cs @@ -95,6 +95,16 @@ public void WriteTo_IgnoresIdAttributeCase(TagRenderMode renderingMode, string e [InlineData("0", "z")] [InlineData("-", "z")] [InlineData(",", "z")] + [InlineData(",.", "z-")] + [InlineData("a😊", "a--")] + [InlineData("a😊.", "a---")] + [InlineData("😊", "z-")] + [InlineData("😊.", "z--")] + [InlineData(",a", "za")] + [InlineData("a,", "a-")] + [InlineData("a0,", "a0-")] + [InlineData("a,0,", "a-0-")] + [InlineData("a,0", "a-0")] [InlineData("00Hello,World", "z0Hello-World")] [InlineData(",,Hello,,World,,", "z-Hello--World--")] [InlineData("-_:Hello-_:Hello-_:", "z_:Hello-_:Hello-_:")] @@ -110,6 +120,23 @@ public void CreateSanitizedIdCreatesId(string input, string output) Assert.Equal(output, result); } + [Fact] + public void CreateSanitizedIdCreatesIdReplacesAllInvalidCharacters() + { + foreach (char c in Enumerable.Range(char.MinValue, char.MaxValue)) + { + var result = TagBuilder.CreateSanitizedId($"a{c}", "z"); + if (char.IsAsciiLetterOrDigit(c) || c == '-' || c == '_' || c == ':') + { + Assert.Equal($"a{c}", result); + } + else + { + Assert.Equal("az", result); + } + } + } + [Theory] [InlineData("attribute", "value", "

")] [InlineData("attribute", null, "

")] diff --git a/src/Mvc/perf/Microbenchmarks/Microsoft.AspNetCore.Mvc/TagBuilderBenchmark.cs b/src/Mvc/perf/Microbenchmarks/Microsoft.AspNetCore.Mvc/TagBuilderBenchmark.cs new file mode 100644 index 000000000000..61918840b6a0 --- /dev/null +++ b/src/Mvc/perf/Microbenchmarks/Microsoft.AspNetCore.Mvc/TagBuilderBenchmark.cs @@ -0,0 +1,16 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using BenchmarkDotNet.Attributes; +using Microsoft.AspNetCore.Mvc.Rendering; + +namespace Microsoft.AspNetCore.Mvc.Microbenchmarks; + +public class TagBuilderBenchmark +{ + [Benchmark] + public string ValidFieldName() => TagBuilder.CreateSanitizedId("LongIdForFieldWithMaxLength", "z"); + + [Benchmark] + public string InvalidFieldName() => TagBuilder.CreateSanitizedId("LongIdForField$WithMaxLength", "z"); +} From 8e7b24a6de1eece1d903a44d37686883ce508a0f Mon Sep 17 00:00:00 2001 From: ladeak Date: Tue, 2 Apr 2024 17:58:32 +0200 Subject: [PATCH 2/5] Update comment triggering a build --- src/Mvc/Mvc.ViewFeatures/src/Rendering/TagBuilder.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Mvc/Mvc.ViewFeatures/src/Rendering/TagBuilder.cs b/src/Mvc/Mvc.ViewFeatures/src/Rendering/TagBuilder.cs index 02425ffef153..a5425a368897 100644 --- a/src/Mvc/Mvc.ViewFeatures/src/Rendering/TagBuilder.cs +++ b/src/Mvc/Mvc.ViewFeatures/src/Rendering/TagBuilder.cs @@ -20,8 +20,8 @@ namespace Microsoft.AspNetCore.Mvc.Rendering; [DebuggerDisplay("{DebuggerToString()}")] public class TagBuilder : IHtmlContent { - // Note '.' is valid according to the HTML 4.01 specification. Disallowed here to avoid - // confusion with CSS class selectors or when using jQuery. + // Note '.' is valid according to the HTML 4.01 specification. Disallowed here + // to avoid confusion with CSS class selectors or when using jQuery. private static readonly SearchValues _html401IdChars = SearchValues.Create("-0123456789:ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz"); From 1006cc15a4e9f35acf8bc732a76e5a53acae1d78 Mon Sep 17 00:00:00 2001 From: ladeak Date: Thu, 4 Apr 2024 23:11:02 +0200 Subject: [PATCH 3/5] Using SearchValues in UrlResolutionTagHelper. --- .../src/TagHelpers/UrlResolutionTagHelper.cs | 67 +++++-------------- .../TagHelpers/UrlResolutionTagHelperTest.cs | 7 ++ 2 files changed, 25 insertions(+), 49 deletions(-) diff --git a/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs b/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs index 8910c58f138e..3ff5efd006fa 100644 --- a/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs +++ b/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Buffers; using System.Diagnostics.CodeAnalysis; using System.Text.Encodings.Web; using Microsoft.AspNetCore.Html; @@ -49,8 +50,8 @@ namespace Microsoft.AspNetCore.Mvc.Razor.TagHelpers; public class UrlResolutionTagHelper : TagHelper { // Valid whitespace characters defined by the HTML5 spec. - private static readonly char[] ValidAttributeWhitespaceChars = - new[] { '\t', '\n', '\u000C', '\r', ' ' }; + private static readonly SearchValues ValidAttributeWhitespaceChars = SearchValues.Create("\t\n\u000C\r "); + private static readonly Dictionary ElementAttributeLookups = new(StringComparer.OrdinalIgnoreCase) { @@ -215,14 +216,11 @@ protected void ProcessUrlAttribute(string attributeName, TagHelperOutput output) protected bool TryResolveUrl([StringSyntax(StringSyntaxAttribute.Uri, UriKind.Relative)] string url, out string? resolvedUrl) { resolvedUrl = null; - var start = FindRelativeStart(url); - if (start == -1) + if (!TryCreateTrimmedString(url, out var trimmedUrl)) { return false; } - var trimmedUrl = CreateTrimmedString(url, start); - var urlHelper = UrlHelperFactory.GetUrlHelper(ViewContext); resolvedUrl = urlHelper.Content(trimmedUrl); @@ -241,14 +239,11 @@ protected bool TryResolveUrl([StringSyntax(StringSyntaxAttribute.Uri, UriKind.Re protected bool TryResolveUrl([StringSyntax(StringSyntaxAttribute.Uri, UriKind.Relative)] string url, [NotNullWhen(true)] out IHtmlContent? resolvedUrl) { resolvedUrl = null; - var start = FindRelativeStart(url); - if (start == -1) + if (!TryCreateTrimmedString(url, out var trimmedUrl)) { return false; } - var trimmedUrl = CreateTrimmedString(url, start); - var urlHelper = UrlHelperFactory.GetUrlHelper(ViewContext); var appRelativeUrl = urlHelper.Content(trimmedUrl); var postTildeSlashUrlValue = trimmedUrl.Substring(2); @@ -273,58 +268,32 @@ protected bool TryResolveUrl([StringSyntax(StringSyntaxAttribute.Uri, UriKind.Re return true; } - private static int FindRelativeStart(string url) + private static bool TryCreateTrimmedString(string input, [NotNullWhen(true)] out string? trimmed) { - if (url == null || url.Length < 2) + trimmed = null; + if (input == null || input.Length < 2) { - return -1; + return false; } - var maxTestLength = url.Length - 2; - - var start = 0; - for (; start < url.Length; start++) + var url = input.AsSpan(); + var start = url.IndexOfAnyExcept(ValidAttributeWhitespaceChars); + if (start < 0) { - if (start > maxTestLength) - { - return -1; - } - - if (!IsCharWhitespace(url[start])) - { - break; - } + return false; } // Before doing more work, ensure that the URL we're looking at is app-relative. - if (url[start] != '~' || url[start + 1] != '/') - { - return -1; - } - - return start; - } - - private static string CreateTrimmedString(string input, int start) - { - var end = input.Length - 1; - for (; end >= start; end--) + if (!url.Slice(start, 2).SequenceEqual("~/")) { - if (!IsCharWhitespace(input[end])) - { - break; - } + return false; } - var len = end - start + 1; + var end = url.Slice(start).LastIndexOfAnyExcept(ValidAttributeWhitespaceChars); // Substring returns same string if start == 0 && len == Length - return input.Substring(start, len); - } - - private static bool IsCharWhitespace(char ch) - { - return ValidAttributeWhitespaceChars.AsSpan().IndexOf(ch) != -1; + trimmed = input.Substring(start, end + 1); + return true; } private sealed class EncodeFirstSegmentContent : IHtmlContent diff --git a/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs b/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs index 712063062ea5..9282586d6bb7 100644 --- a/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs +++ b/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs @@ -20,10 +20,17 @@ public static TheoryData ResolvableUrlData { { "~/home/index.html", "/approot/home/index.html" }, { " ~/home/index.html", "/approot/home/index.html" }, + { "\u000C~/home/index.html\r\n", "/approot/home/index.html" }, + { "\t ~/home/index.html\n", "/approot/home/index.html" }, + { "\r\n~/home/index.html\u000C\t", "/approot/home/index.html" }, + { "\r~/home/index.html\t", "/approot/home/index.html" }, + { "\n~/home/index.html\u202F", "/approot/home/index.html\u202F" }, { "~/home/index.html ~/secondValue/index.html", "/approot/home/index.html ~/secondValue/index.html" }, + { " ~/ ", "/approot/" }, + { " ~/", "/approot/" }, }; } } From 52196db75a813f1e44ce08fe9de060639294fa0a Mon Sep 17 00:00:00 2001 From: ladeak Date: Fri, 5 Apr 2024 21:08:36 +0200 Subject: [PATCH 4/5] Review feedback + fixing outofrange exception for unresolveable urls --- .../src/TagHelpers/UrlResolutionTagHelper.cs | 11 +++++++---- .../test/TagHelpers/UrlResolutionTagHelperTest.cs | 2 ++ .../Mvc.ViewFeatures/test/Rendering/TagBuilderTest.cs | 8 ++++---- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs b/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs index 3ff5efd006fa..1609605a3f08 100644 --- a/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs +++ b/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs @@ -271,7 +271,7 @@ protected bool TryResolveUrl([StringSyntax(StringSyntaxAttribute.Uri, UriKind.Re private static bool TryCreateTrimmedString(string input, [NotNullWhen(true)] out string? trimmed) { trimmed = null; - if (input == null || input.Length < 2) + if (input == null) { return false; } @@ -283,16 +283,19 @@ private static bool TryCreateTrimmedString(string input, [NotNullWhen(true)] out return false; } + // Url without leading whitespace. + url = url.Slice(start); + // Before doing more work, ensure that the URL we're looking at is app-relative. - if (!url.Slice(start, 2).SequenceEqual("~/")) + if (url.Length < 2 || !url[..2].SequenceEqual("~/")) { return false; } - var end = url.Slice(start).LastIndexOfAnyExcept(ValidAttributeWhitespaceChars); + var endFromStart = url.LastIndexOfAnyExcept(ValidAttributeWhitespaceChars); // Substring returns same string if start == 0 && len == Length - trimmed = input.Substring(start, end + 1); + trimmed = input.Substring(start, endFromStart + 1); return true; } diff --git a/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs b/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs index 9282586d6bb7..411231c642bf 100644 --- a/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs +++ b/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs @@ -173,6 +173,8 @@ public static TheoryData UnresolvableUrlData { "/home/index.html ~/second/wontresolve.html" }, { " ~\\home\\index.html" }, { "~\\/home/index.html" }, + { " ~" }, + { " " }, }; } } diff --git a/src/Mvc/Mvc.ViewFeatures/test/Rendering/TagBuilderTest.cs b/src/Mvc/Mvc.ViewFeatures/test/Rendering/TagBuilderTest.cs index 8d040399abe8..075166681d2b 100644 --- a/src/Mvc/Mvc.ViewFeatures/test/Rendering/TagBuilderTest.cs +++ b/src/Mvc/Mvc.ViewFeatures/test/Rendering/TagBuilderTest.cs @@ -96,10 +96,10 @@ public void WriteTo_IgnoresIdAttributeCase(TagRenderMode renderingMode, string e [InlineData("-", "z")] [InlineData(",", "z")] [InlineData(",.", "z-")] - [InlineData("a😊", "a--")] - [InlineData("a😊.", "a---")] - [InlineData("😊", "z-")] - [InlineData("😊.", "z--")] + [InlineData("a\uD83D\uDE0A", "a--")] + [InlineData("a\uD83D\uDE0A.", "a---")] + [InlineData("\uD83D\uDE0A", "z-")] + [InlineData("\uD83D\uDE0A.", "z--")] [InlineData(",a", "za")] [InlineData("a,", "a-")] [InlineData("a0,", "a0-")] From e7c6979e2d65b88d720b44c76d03f45a1ae4ab00 Mon Sep 17 00:00:00 2001 From: ladeak Date: Fri, 5 Apr 2024 23:10:15 +0200 Subject: [PATCH 5/5] Using StartsWith instead of SequenceEqual --- src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs | 6 +++--- .../Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs b/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs index 1609605a3f08..b20c3b5c6300 100644 --- a/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs +++ b/src/Mvc/Mvc.Razor/src/TagHelpers/UrlResolutionTagHelper.cs @@ -287,15 +287,15 @@ private static bool TryCreateTrimmedString(string input, [NotNullWhen(true)] out url = url.Slice(start); // Before doing more work, ensure that the URL we're looking at is app-relative. - if (url.Length < 2 || !url[..2].SequenceEqual("~/")) + if (!url.StartsWith("~/")) { return false; } - var endFromStart = url.LastIndexOfAnyExcept(ValidAttributeWhitespaceChars); + var remainingLength = url.LastIndexOfAnyExcept(ValidAttributeWhitespaceChars) + 1; // Substring returns same string if start == 0 && len == Length - trimmed = input.Substring(start, endFromStart + 1); + trimmed = input.Substring(start, remainingLength); return true; } diff --git a/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs b/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs index 411231c642bf..a6e2a92b5529 100644 --- a/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs +++ b/src/Mvc/Mvc.Razor/test/TagHelpers/UrlResolutionTagHelperTest.cs @@ -19,6 +19,7 @@ public static TheoryData ResolvableUrlData return new TheoryData { { "~/home/index.html", "/approot/home/index.html" }, + { "~/home/index.html\r\n", "/approot/home/index.html" }, { " ~/home/index.html", "/approot/home/index.html" }, { "\u000C~/home/index.html\r\n", "/approot/home/index.html" }, { "\t ~/home/index.html\n", "/approot/home/index.html" },