From 0dfffd45c25bfc663d0984af2ffbc8fb98bb0575 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Thu, 22 Jun 2017 11:56:16 -0700 Subject: [PATCH] RazorPages page directives missing quotes should alert user Fixes #5868 --- .../Infrastructure/PageDirectiveFeature.cs | 52 +++--- .../Internal/PageLoggerExtensions.cs | 22 +++ ...azorProjectPageApplicationModelProvider.cs | 8 +- .../PageDirectiveFeatureTest.cs | 161 ++++++------------ ...ProjectPageApplicationModelProviderTest.cs | 11 +- 5 files changed, 121 insertions(+), 133 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs index 8a657ab010..5e963893c3 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs @@ -2,9 +2,11 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.IO; using Microsoft.AspNetCore.Mvc.Razor.Extensions; +using Microsoft.AspNetCore.Mvc.RazorPages.Internal; using Microsoft.AspNetCore.Razor.Language; +using Microsoft.AspNetCore.Razor.Language.Intermediate; +using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure { @@ -26,7 +28,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure builder.Features.Add(new PageDirectiveParserOptionsFeature()); }); - public static bool TryGetPageDirective(RazorProjectItem projectItem, out string template) + public static bool TryGetPageDirective(ILogger logger, RazorProjectItem projectItem, out string template) { if (projectItem == null) { @@ -34,35 +36,34 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure } var sourceDocument = RazorSourceDocument.ReadFrom(projectItem); - return TryGetPageDirective(sourceDocument, out template); + return TryGetPageDirective(logger, sourceDocument, out template); } - public static bool TryGetPageDirective(Func streamFactory, out string template) - { - if (streamFactory == null) - { - throw new ArgumentNullException(nameof(streamFactory)); - } - - using (var stream = streamFactory()) - { - var sourceDocument = RazorSourceDocument.ReadFrom(stream, fileName: "Parse.cshtml"); - return TryGetPageDirective(sourceDocument, out template); - } - } - - private static bool TryGetPageDirective(RazorSourceDocument sourceDocument, out string template) + static bool TryGetPageDirective( + ILogger logger, + RazorSourceDocument sourceDocument, + out string template) { var codeDocument = RazorCodeDocument.Create(sourceDocument); PageDirectiveEngine.Process(codeDocument); - if (PageDirective.TryGetPageDirective(codeDocument.GetDocumentIntermediateNode(), out var pageDirective)) + var documentIRNode = codeDocument.GetDocumentIntermediateNode(); + if (PageDirective.TryGetPageDirective(documentIRNode, out var pageDirective)) { template = pageDirective.RouteTemplate; return true; } template = null; + + var visitor = new Visitor(); + visitor.Visit(documentIRNode); + if (visitor.MalformedPageDirective != null) + { + logger.MalformedPageDirective(sourceDocument.FilePath, visitor.MalformedPageDirective.Diagnostics); + return true; + } + return false; } @@ -75,5 +76,18 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure options.ParseOnlyLeadingDirectives = true; } } + + private class Visitor : IntermediateNodeWalker + { + public MalformedDirectiveIntermediateNode MalformedPageDirective { get; private set; } + + public override void VisitMalformedDirective(MalformedDirectiveIntermediateNode node) + { + if (node.Descriptor == PageDirective.Directive) + { + MalformedPageDirective = node; + } + } + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageLoggerExtensions.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageLoggerExtensions.cs index 31ca582430..7289f0da2c 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageLoggerExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/PageLoggerExtensions.cs @@ -2,9 +2,11 @@ // 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 Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure; +using Microsoft.AspNetCore.Razor.Language; using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal @@ -14,6 +16,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal private static readonly Action _handlerMethodExecuting; private static readonly Action _handlerMethodExecuted; private static readonly Action _pageFilterShortCircuit; + private static readonly Action _malformedPageDirective; static PageLoggerExtensions() { @@ -33,6 +36,11 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal LogLevel.Debug, 3, "Request was short circuited at page filter '{PageFilter}'."); + + _malformedPageDirective = LoggerMessage.Define( + LogLevel.Warning, + 104, + "The page directive at '{FilePath}' is malformed. Please fix the following issues: {Diagnostics}"); } public static void ExecutingHandlerMethod(this ILogger logger, PageContext context, HandlerMethodDescriptor handler, object[] arguments) @@ -76,5 +84,19 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal { _pageFilterShortCircuit(logger, filter, null); } + + public static void MalformedPageDirective(this ILogger logger, string filePath, IList diagnostics) + { + if (logger.IsEnabled(LogLevel.Warning)) + { + var messages = new string[diagnostics.Count]; + for (var i = 0; i < diagnostics.Count; i++) + { + messages[i] = diagnostics[i].GetMessage(); + } + + _malformedPageDirective(logger, filePath, messages, null); + } + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/RazorProjectPageApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/RazorProjectPageApplicationModelProvider.cs index 367d99d564..d80651ad27 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/RazorProjectPageApplicationModelProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/RazorProjectPageApplicationModelProvider.cs @@ -4,6 +4,7 @@ using Microsoft.AspNetCore.Mvc.ApplicationModels; using Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure; using Microsoft.AspNetCore.Razor.Language; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal @@ -12,13 +13,16 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal { private readonly RazorProject _project; private readonly RazorPagesOptions _pagesOptions; + private readonly ILogger _logger; public RazorProjectPageApplicationModelProvider( RazorProject razorProject, - IOptions pagesOptionsAccessor) + IOptions pagesOptionsAccessor, + ILoggerFactory loggerFactory) { _project = razorProject; _pagesOptions = pagesOptionsAccessor.Value; + _logger = loggerFactory.CreateLogger(); } public int Order => -1000; @@ -37,7 +41,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal continue; } - if (!PageDirectiveFeature.TryGetPageDirective(item, out var routeTemplate)) + if (!PageDirectiveFeature.TryGetPageDirective(_logger, item, out var routeTemplate)) { // .cshtml pages without @page are not RazorPages. continue; diff --git a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Infrastructure/PageDirectiveFeatureTest.cs b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Infrastructure/PageDirectiveFeatureTest.cs index 164e214633..596847d5a6 100644 --- a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Infrastructure/PageDirectiveFeatureTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Infrastructure/PageDirectiveFeatureTest.cs @@ -3,8 +3,12 @@ using System; using System.IO; +using System.Linq; using System.Text; using Microsoft.AspNetCore.Razor.Language; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Logging.Testing; using Xunit; namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure @@ -15,126 +19,110 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure public void TryGetPageDirective_FindsTemplate() { // Arrange - string template; var projectItem = new TestRazorProjectItem(@"@page ""Some/Path/{value}"" The rest of the thing"); + var sink = new TestSink(); + var logger = new TestLogger("logger", sink, enabled: true); // Act & Assert - Assert.True(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); + Assert.True(PageDirectiveFeature.TryGetPageDirective(logger, projectItem, out var template)); Assert.Equal("Some/Path/{value}", template); + Assert.Empty(sink.Writes); } [Fact] public void TryGetPageDirective_NoNewLine() { // Arrange - string template; var projectItem = new TestRazorProjectItem(@"@page ""Some/Path/{value}"""); + var sink = new TestSink(); + var logger = new TestLogger("logger", sink, enabled: true); // Act & Assert - Assert.True(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); + Assert.True(PageDirectiveFeature.TryGetPageDirective(logger, projectItem, out var template)); Assert.Equal("Some/Path/{value}", template); + Assert.Empty(sink.Writes); } [Fact] public void TryGetPageDirective_JunkBeforeDirective() { // Arrange - string template; var projectItem = new TestRazorProjectItem(@"Not a directive @page ""Some/Path/{value}"""); + var sink = new TestSink(); + var logger = new TestLogger("logger", sink, enabled: true); // Act & Assert - Assert.False(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); + Assert.False(PageDirectiveFeature.TryGetPageDirective(logger, projectItem, out var template)); Assert.Null(template); + Assert.Empty(sink.Writes); } [Theory] [InlineData(@"""Some/Path/{value}")] [InlineData(@"Some/Path/{value}""")] - public void TryGetPageDirective_RequiresBothQuotes(string inTemplate) + public void TryGetPageDirective_WithoutBothQuotes_LogsWarning(string inTemplate) { // Arrange - string template; + var expected = "The page directive at 'Test.cshtml' is malformed. Please fix the following issues: The 'page' directive expects a string surrounded by double quotes."; + var sink = new TestSink(); + var logger = new TestLogger("logger", sink, enabled: true); var projectItem = new TestRazorProjectItem($@"@page {inTemplate}"); // Act & Assert - Assert.False(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); + Assert.True(PageDirectiveFeature.TryGetPageDirective(logger, projectItem, out var template)); Assert.Null(template); + Assert.Collection(sink.Writes, + log => + { + Assert.Equal(LogLevel.Warning, log.LogLevel); + Assert.Equal(expected, log.State.ToString()); + }); } [Fact] - public void TryGetPageDirective_NoQuotesAroundPath_IsNotTemplate() + public void TryGetPageDirective_NoQuotesAroundPath_LogsWarning() { // Arrange - string template; + var expected = "The page directive at 'Test.cshtml' is malformed. Please fix the following issues: The 'page' directive expects a string surrounded by double quotes."; + var sink = new TestSink(); + var logger = new TestLogger("logger", sink, enabled: true); var projectItem = new TestRazorProjectItem(@"@page Some/Path/{value}"); // Act & Assert - Assert.False(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); + Assert.True(PageDirectiveFeature.TryGetPageDirective(logger, projectItem, out var template)); Assert.Null(template); - } - - [Fact] - public void TryGetPageDirective_WrongNewLine() - { - // Arrange - var wrongNewLine = Environment.NewLine == "\r\n" ? "\n" : "\r\n"; - - string template; - var projectItem = new TestRazorProjectItem($"@page \"Some/Path/{{value}}\" {wrongNewLine}"); - - // Act & Assert - Assert.True(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); - Assert.Equal("Some/Path/{value}", template); + var logs = sink.Writes.Select(w => w.State.ToString().Trim()).ToList(); + Assert.Collection(sink.Writes, + log => + { + Assert.Equal(LogLevel.Warning, log.LogLevel); + Assert.Equal(expected, log.State.ToString()); + }); } [Fact] public void TryGetPageDirective_NewLineBeforeDirective() { // Arrange - string template; var projectItem = new TestRazorProjectItem("\n @page \"Some/Path/{value}\""); + var sink = new TestSink(); + var logger = new TestLogger("logger", sink, enabled: true); // Act - Assert.True(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); + Assert.True(PageDirectiveFeature.TryGetPageDirective(logger, projectItem, out var template)); Assert.Equal("Some/Path/{value}", template); - } - - [Fact] - public void TryGetPageDirective_WhitespaceBeforeDirective() - { - // Arrange - string template; - var projectItem = new TestRazorProjectItem(@" @page ""Some/Path/{value}"" -"); - - // Act & Assert - Assert.True(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); - Assert.Equal("Some/Path/{value}", template); - } - - [Fact] - public void TryGetPageDirective_JunkBeforeNewline() - { - // Arrange - string template; - var projectItem = new TestRazorProjectItem(@"@page ""Some/Path/{value}"" things that are not the path -a new line"); - - // Act & Assert - Assert.False(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); - Assert.Null(template); + Assert.Empty(sink.Writes); } [Fact] public void TryGetPageDirective_Directive_WithoutPathOrContent() { // Arrange - string template; var projectItem = new TestRazorProjectItem(@"@page"); // Act & Assert - Assert.True(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); + Assert.True(PageDirectiveFeature.TryGetPageDirective(NullLogger.Instance, projectItem, out var template)); Assert.Null(template); } @@ -142,59 +130,30 @@ a new line"); public void TryGetPageDirective_DirectiveWithContent_WithoutPath() { // Arrange - string template; var projectItem = new TestRazorProjectItem(@"@page Non-path things"); + var sink = new TestSink(); + var logger = new TestLogger("logger", sink, enabled: true); // Act & Assert - Assert.True(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); + Assert.True(PageDirectiveFeature.TryGetPageDirective(logger, projectItem, out var template)); Assert.Null(template); + Assert.Empty(sink.Writes); } [Fact] public void TryGetPageDirective_NoDirective() { // Arrange - string template; var projectItem = new TestRazorProjectItem(@"This is junk Nobody will use it"); + var sink = new TestSink(); + var logger = new TestLogger("logger", sink, enabled: true); // Act & Assert - Assert.False(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); + Assert.False(PageDirectiveFeature.TryGetPageDirective(logger, projectItem, out var template)); Assert.Null(template); - } - - [Fact] - public void TryGetPageDirective_EmptyStream() - { - // Arrange - string template; - var projectItem = new TestRazorProjectItem(string.Empty); - - // Act - Assert.False(PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); - Assert.Null(template); - } - - [Fact] - public void TryGetPageDirective_NullProject() - { - // Arrange - string template; - - // Act & Assert - Assert.Throws(() => PageDirectiveFeature.TryGetPageDirective(projectItem: null, template: out template)); - } - - [Fact] - public void TryGetPageDirective_NullStream() - { - // Arrange - string template; - var projectItem = new TestRazorProjectItem(content: null); - - // Act & Assert - Assert.Throws(() => PageDirectiveFeature.TryGetPageDirective(projectItem, out template)); + Assert.Empty(sink.Writes); } } @@ -207,21 +166,9 @@ Nobody will use it"); _content = content; } - public override string BasePath - { - get - { - throw new NotImplementedException(); - } - } + public override string BasePath => throw new NotImplementedException(); - public override bool Exists - { - get - { - throw new NotImplementedException(); - } - } + public override bool Exists => throw new NotImplementedException(); public override string Path => "Test.cshtml"; diff --git a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/RazorProjectPageApplicationModelProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/RazorProjectPageApplicationModelProviderTest.cs index 3f8487e579..a4f9995f37 100644 --- a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/RazorProjectPageApplicationModelProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/RazorProjectPageApplicationModelProviderTest.cs @@ -5,6 +5,7 @@ using System; using Microsoft.AspNetCore.Mvc.ApplicationModels; using Microsoft.AspNetCore.Mvc.Razor; using Microsoft.Extensions.FileProviders; +using Microsoft.Extensions.Logging.Abstractions; using Xunit; namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal @@ -26,7 +27,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal var optionsManager = new TestOptionsManager(); optionsManager.Value.RootDirectory = "/"; - var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager); + var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager, NullLoggerFactory.Instance); var context = new PageApplicationModelProviderContext(); // Act @@ -60,7 +61,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal var optionsManager = new TestOptionsManager(); optionsManager.Value.RootDirectory = "/"; - var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager); + var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager, NullLoggerFactory.Instance); var context = new PageApplicationModelProviderContext(); // Act @@ -98,7 +99,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal var optionsManager = new TestOptionsManager(); optionsManager.Value.RootDirectory = "/"; - var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager); + var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager, NullLoggerFactory.Instance); var context = new PageApplicationModelProviderContext(); // Act & Assert @@ -124,7 +125,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal var optionsManager = new TestOptionsManager(); optionsManager.Value.RootDirectory = "/"; - var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager); + var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager, NullLoggerFactory.Instance); var context = new PageApplicationModelProviderContext(); // Act @@ -162,7 +163,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal var optionsManager = new TestOptionsManager(); optionsManager.Value.RootDirectory = "/Pages"; - var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager); + var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager, NullLoggerFactory.Instance); var context = new PageApplicationModelProviderContext(); // Act