From e14dbdf9be27f23456e87b124b8c2c7c47326e8b Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Wed, 11 Feb 2015 17:15:42 -0800 Subject: [PATCH] Add parser error for empty TagHelper bound attributes. - Errors are only created for TagHelper bound attributes that are not bound to string. - Added tests to validate proper errors for expected input. #289 --- .../TagHelpers/TagHelperBlockRewriter.cs | 74 ++++- .../Properties/RazorResources.Designer.cs | 16 + .../RazorResources.resx | 3 + .../TagHelperParseTreeRewriterTest.cs | 297 +++++++++++++++++- 4 files changed, 387 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlockRewriter.cs b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlockRewriter.cs index 49023e37c2..410fdc9ad4 100644 --- a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlockRewriter.cs +++ b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlockRewriter.cs @@ -15,6 +15,8 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers.Internal { public static class TagHelperBlockRewriter { + private static readonly string StringTypeName = typeof(string).FullName; + public static TagHelperBlockBuilder Rewrite(string tagName, bool validStructure, Block tag, @@ -68,6 +70,23 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers.Internal // Only want to track the attribute if we succeeded in parsing its corresponding Block/Span. if (succeeded) { + // Check if it's a bound attribute that is not of type string and happens to be null or whitespace. + string attributeValueType; + if (attributeValueTypes.TryGetValue(attribute.Key, out attributeValueType) && + !IsStringAttribute(attributeValueType) && + IsNullOrWhitespaceAttributeValue(attribute.Value)) + { + var errorLocation = GetAttributeNameStartLocation(child); + + errorSink.OnError( + errorLocation, + RazorResources.FormatRewriterError_EmptyTagHelperBoundAttribute( + attribute.Key, + tagName, + attributeValueType), + attribute.Key.Length); + } + attributes[attribute.Key] = attribute.Value; } } @@ -363,6 +382,34 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers.Internal return builder.Build(); } + private static SourceLocation GetAttributeNameStartLocation(SyntaxTreeNode node) + { + Span span; + + if (node.IsBlock) + { + span = ((Block)node).FindFirstDescendentSpan(); + } + else + { + span = (Span)node; + } + + // Span should never be null here, this should only ever be called if an attribute was successfully parsed. + Debug.Assert(span != null); + + var nodeStart = span.Parent.Start; + + // Attributes must have at least one non-whitespace character to represent the tagName (even if its a C# + // expression). + var firstNonWhitespaceSymbol = span + .Symbols + .OfType() + .First(sym => sym.Type != HtmlSymbolType.WhiteSpace && sym.Type != HtmlSymbolType.NewLine); + + return nodeStart + firstNonWhitespaceSymbol.Start; + } + private static KeyValuePair CreateMarkupAttribute( string name, SpanBuilder builder, @@ -374,7 +421,7 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers.Internal // its value as code. Any non-string value can be any C# value so we need to ensure the SyntaxTreeNode // reflects that. if (attributeValueTypes.TryGetValue(name, out attributeTypeName) && - !string.Equals(attributeTypeName, typeof(string).FullName, StringComparison.OrdinalIgnoreCase)) + !IsStringAttribute(attributeTypeName)) { builder.Kind = SpanKind.Code; } @@ -382,6 +429,31 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers.Internal return new KeyValuePair(name, builder.Build()); } + private static bool IsNullOrWhitespaceAttributeValue(SyntaxTreeNode attributeValue) + { + if (attributeValue.IsBlock) + { + foreach (var span in ((Block)attributeValue).Flatten()) + { + if (!string.IsNullOrWhiteSpace(span.Content)) + { + return false; + } + } + + return true; + } + else + { + return string.IsNullOrWhiteSpace(((Span)attributeValue).Content); + } + } + + private static bool IsStringAttribute(string attributeTypeName) + { + return string.Equals(attributeTypeName, StringTypeName, StringComparison.OrdinalIgnoreCase); + } + private static bool IsQuote(HtmlSymbol htmlSymbol) { return htmlSymbol.Type == HtmlSymbolType.DoubleQuote || diff --git a/src/Microsoft.AspNet.Razor/Properties/RazorResources.Designer.cs b/src/Microsoft.AspNet.Razor/Properties/RazorResources.Designer.cs index a238ee5343..9312ed766f 100644 --- a/src/Microsoft.AspNet.Razor/Properties/RazorResources.Designer.cs +++ b/src/Microsoft.AspNet.Razor/Properties/RazorResources.Designer.cs @@ -1530,6 +1530,22 @@ namespace Microsoft.AspNet.Razor return string.Format(CultureInfo.CurrentCulture, GetString("TagHelpers_InlineMarkupBlocks_NotSupported_InAttributes"), p0); } + /// + /// Attribute '{0}' on tag helper element '{1}' requires a value. Tag helper bound attributes of type '{2}' cannot be empty or contain only whitespace. + /// + internal static string RewriterError_EmptyTagHelperBoundAttribute + { + get { return GetString("RewriterError_EmptyTagHelperBoundAttribute"); } + } + + /// + /// Attribute '{0}' on tag helper element '{1}' requires a value. Tag helper bound attributes of type '{2}' cannot be empty or contain only whitespace. + /// + internal static string FormatRewriterError_EmptyTagHelperBoundAttribute(object p0, object p1, object p2) + { + return string.Format(CultureInfo.CurrentCulture, GetString("RewriterError_EmptyTagHelperBoundAttribute"), p0, p1, p2); + } + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNet.Razor/RazorResources.resx b/src/Microsoft.AspNet.Razor/RazorResources.resx index 748e14ef5d..ffcaec714e 100644 --- a/src/Microsoft.AspNet.Razor/RazorResources.resx +++ b/src/Microsoft.AspNet.Razor/RazorResources.resx @@ -422,4 +422,7 @@ Instead, wrap the contents of the block in "{{}}": Inline markup blocks (e.g. @<p>content</p>) must not appear in non-string tag helper attribute values. Expected a '{0}' attribute value, not a string. + + Attribute '{0}' on tag helper element '{1}' requires a value. Tag helper bound attributes of type '{2}' cannot be empty or contain only whitespace. + \ No newline at end of file diff --git a/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs b/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs index 27b9d61d84..4ca19e04a5 100644 --- a/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs +++ b/test/Microsoft.AspNet.Razor.Test/TagHelpers/TagHelperParseTreeRewriterTest.cs @@ -19,6 +19,299 @@ namespace Microsoft.AspNet.Razor.Test.TagHelpers { public class TagHelperParseTreeRewriterTest : CsHtmlMarkupParserTestBase { + public static TheoryData EmptyTagHelperBoundAttributeData + { + get + { + var factory = CreateDefaultSpanFactory(); + var emptyAttributeError = + "Attribute '{0}' on tag helper element '{1}' requires a value. Tag helper bound attributes of " + + "type '{2}' cannot be empty or contain only whitespace."; + var boolTypeName = typeof(bool).FullName; + + // documentContent, expectedOutput, expectedErrors + return new TheoryData + { + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "bound", new MarkupBlock() } + })), + new[] + { + new RazorError( + string.Format(emptyAttributeError, "bound", "myth", boolTypeName), + absoluteIndex: 6, lineIndex: 0, columnIndex: 6, length: 5) + } + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "bound", factory.CodeMarkup(" true") } + })), + new RazorError[0] + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "bound", factory.CodeMarkup(" ") } + })), + new[] + { + new RazorError( + string.Format(emptyAttributeError, "bound", "myth", boolTypeName), + absoluteIndex: 6, lineIndex: 0, columnIndex: 6, length: 5) + } + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "bound", new MarkupBlock() } + })), + new[] + { + new RazorError( + string.Format(emptyAttributeError, "bound", "myth", boolTypeName), + absoluteIndex: 6, lineIndex: 0, columnIndex: 6, length: 5), + new RazorError( + string.Format(emptyAttributeError, "bound", "myth", boolTypeName), + absoluteIndex: 16, lineIndex: 0, columnIndex: 16, length: 5) + } + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "bound", factory.CodeMarkup(" ") } + })), + new[] + { + new RazorError( + string.Format(emptyAttributeError, "bound", "myth", boolTypeName), + absoluteIndex: 6, lineIndex: 0, columnIndex: 6, length: 5), + new RazorError( + string.Format(emptyAttributeError, "bound", "myth", boolTypeName), + absoluteIndex: 17, lineIndex: 0, columnIndex: 17, length: 5) + } + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "bound", factory.CodeMarkup(string.Empty).With(SpanCodeGenerator.Null) } + })), + new[] + { + new RazorError( + string.Format(emptyAttributeError, "bound", "myth", boolTypeName), + absoluteIndex: 19, lineIndex: 0, columnIndex: 19, length: 5) + } + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "bound", factory.CodeMarkup(string.Empty).With(SpanCodeGenerator.Null) }, + { "name", new MarkupBlock() } + })), + new[] + { + new RazorError( + string.Format(emptyAttributeError, "bound", "myth", boolTypeName), + absoluteIndex: 6, lineIndex: 0, columnIndex: 6, length: 5), + } + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "bound", factory.CodeMarkup(string.Empty).With(SpanCodeGenerator.Null) }, + { "name", factory.Markup(" ") } + })), + new[] + { + new RazorError( + string.Format(emptyAttributeError, "bound", "myth", boolTypeName), + absoluteIndex: 6, lineIndex: 0, columnIndex: 6, length: 5), + } + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "bound", factory.CodeMarkup(string.Empty).With(SpanCodeGenerator.Null) }, + { "name", factory.Markup(string.Empty).With(SpanCodeGenerator.Null) } + })), + new[] + { + new RazorError( + string.Format(emptyAttributeError, "bound", "myth", boolTypeName), + absoluteIndex: 31, lineIndex: 0, columnIndex: 31, length: 5), + } + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "BouND", new MarkupBlock() } + })), + new[] + { + new RazorError( + string.Format(emptyAttributeError, "BouND", "myth", boolTypeName), + absoluteIndex: 6, lineIndex: 0, columnIndex: 6, length: 5), + } + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "BOUND", new MarkupBlock() } + })), + new[] + { + new RazorError( + string.Format(emptyAttributeError, "BOUND", "myth", boolTypeName), + absoluteIndex: 6, lineIndex: 0, columnIndex: 6, length: 5), + new RazorError( + string.Format(emptyAttributeError, "bOUnd", "myth", boolTypeName), + absoluteIndex: 18, lineIndex: 0, columnIndex: 18, length: 5) + } + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { "BOUND", factory.CodeMarkup(string.Empty).With(SpanCodeGenerator.Null) }, + { "nAMe", factory.Markup("john") } + })), + new[] + { + new RazorError( + string.Format(emptyAttributeError, "BOUND", "myth", boolTypeName), + absoluteIndex: 6, lineIndex: 0, columnIndex: 6, length: 5) + } + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { + "bound", + new MarkupBlock( + new MarkupBlock( + factory.Markup(" "), + new ExpressionBlock( + factory.CodeTransition(), + factory.Code("true") + .AsImplicitExpression(CSharpCodeParser.DefaultKeywords) + .Accepts(AcceptedCharacters.NonWhiteSpace))), + factory.Markup(" ")) + } + })), + new RazorError[0] + }, + { + "", + new MarkupBlock( + new MarkupTagHelperBlock( + "myth", + new Dictionary + { + { + "bound", + new MarkupBlock( + new MarkupBlock( + factory.Markup(" "), + new ExpressionBlock( + factory.CodeTransition(), + factory.MetaCode("(").Accepts(AcceptedCharacters.None), + factory.Code("true").AsExpression(), + factory.MetaCode(")").Accepts(AcceptedCharacters.None))), + factory.Markup(" ")) + } + })), + new RazorError[0] + }, + }; + } + } + + [Theory] + [MemberData(nameof(EmptyTagHelperBoundAttributeData))] + public void Rewrite_CreatesErrorForEmptyTagHelperBoundAttributes( + string documentContent, + MarkupBlock expectedOutput, + RazorError[] expectedErrors) + { + // Arrange + var descriptors = new TagHelperDescriptor[] + { + new TagHelperDescriptor( + tagName: "myth", + typeName: "mythTagHelper", + assemblyName: "SomeAssembly", + attributes: new[] + { + new TagHelperAttributeDescriptor( + name: "bound", + propertyName: "Bound", + typeName: typeof(bool).FullName), + new TagHelperAttributeDescriptor( + name: "name", + propertyName: "Name", + typeName: typeof(string).FullName) + }) + }; + var descriptorProvider = new TagHelperDescriptorProvider(descriptors); + + // Act & Assert + EvaluateData(descriptorProvider, documentContent, expectedOutput, expectedErrors); + } + public static TheoryData OptOut_WithAttributeTextTagData { get @@ -80,7 +373,7 @@ namespace Microsoft.AspNet.Razor.Test.TagHelpers errorMatchingBrace, absoluteIndex: 1, lineIndex: 0, columnIndex: 1), new RazorError( - string.Format(errorFormatNormalUnclosed, "!text"), + string.Format(errorFormatNormalUnclosed, "!text"), absoluteIndex: 2, lineIndex: 0, columnIndex: 2) } }, @@ -1654,7 +1947,7 @@ namespace Microsoft.AspNet.Razor.Test.TagHelpers { RunParseTreeRewriterTest(documentContent, expectedOutput, expectedErrors, "strong", "p"); } - + public static TheoryData EmptyAttributeTagHelperData { get