RazorPages page directives missing quotes should alert user

Fixes #5868

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<Stream> streamFactory, out string template)
+        static bool TryGetPageDirective(
-        {
+            ILogger logger,
-            if (streamFactory == null)
+            RazorSourceDocument sourceDocument,
-            {
+            out string template)
-                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)
         {
             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;
+                }
+            }
+        }
     }
 }

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<ILogger, string, string[], ModelValidationState, Exception> _handlerMethodExecuting;
         private static readonly Action<ILogger, string, string, Exception> _handlerMethodExecuted;
         private static readonly Action<ILogger, object, Exception> _pageFilterShortCircuit;
+        private static readonly Action<ILogger, string, string[], Exception> _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<string, string[]>(
+                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<RazorDiagnostic> 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);
+            }
+        }
     }
 }

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<RazorPagesOptions> pagesOptionsAccessor)
+            IOptions<RazorPagesOptions> pagesOptionsAccessor,
+            ILoggerFactory loggerFactory)
         {
             _project = razorProject;
             _pagesOptions = pagesOptionsAccessor.Value;
+            _logger = loggerFactory.CreateLogger<RazorProjectPageApplicationModelProvider>();
         }
 
         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;

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);
-        }
+            var logs = sink.Writes.Select(w => w.State.ToString().Trim()).ToList();
-
+            Assert.Collection(sink.Writes,
-        [Fact]
+                log =>
-        public void TryGetPageDirective_WrongNewLine()
+                {
-        {
+                    Assert.Equal(LogLevel.Warning, log.LogLevel);
-            // Arrange
+                    Assert.Equal(expected, log.State.ToString());
-            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);
         }
 
         [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);
-        }
+            Assert.Empty(sink.Writes);
-
-        [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);
         }
 
         [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);
-        }
+            Assert.Empty(sink.Writes);
-
-        [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<ArgumentNullException>(() => 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<ArgumentNullException>(() => PageDirectiveFeature.TryGetPageDirective(projectItem, out template));
         }
     }
 
@@ -207,21 +166,9 @@ Nobody will use it");
             _content = content;
         }
 
-        public override string BasePath
+        public override string BasePath => throw new NotImplementedException();
-        {
-            get
-            {
-                throw new NotImplementedException();
-            }
-        }
 
-        public override bool Exists
+        public override bool Exists => throw new NotImplementedException();
-        {
-            get
-            {
-                throw new NotImplementedException();
-            }
-        }
 
         public override string Path => "Test.cshtml";
 

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<RazorPagesOptions>();
             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<RazorPagesOptions>();
             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<RazorPagesOptions>();
             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<RazorPagesOptions>();
             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<RazorPagesOptions>();
             optionsManager.Value.RootDirectory = "/Pages";
-            var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager);
+            var provider = new RazorProjectPageApplicationModelProvider(project, optionsManager, NullLoggerFactory.Instance);
             var context = new PageApplicationModelProviderContext();
 
             // Act