Trim quotes from template #5838

ryanbrandenburg · 2017-02-22T00:47:45Z

Fixes #5835. Will add functional tests after #5837 is merged.

ryanbrandenburg · 2017-02-22T00:49:15Z

We shouldn't be enforcing the presense of quotes at this level, just removing them right?

rynowak · 2017-02-22T01:18:06Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

@@ -22,7 +22,7 @@ public static bool TryGetRouteTemplate(RazorProjectItem projectItem, out string
            if (content.StartsWith(PageDirective, StringComparison.Ordinal))
            {
                var newLineIndex = content.IndexOf(Environment.NewLine, PageDirective.Length);
-                template = content.Substring(PageDirective.Length, newLineIndex - PageDirective.Length).Trim();
+                template = content.Substring(PageDirective.Length, newLineIndex - PageDirective.Length).Trim().Trim('"');


What would happen if there's no newline?

rynowak · 2017-02-22T01:18:29Z

Please add unit tests as well as updating the functional tests, my PR has been merged.

ryanbrandenburg · 2017-02-23T00:19:47Z

🆙📅

ryanbrandenburg · 2017-02-23T00:21:05Z

test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Infrastructure/PageDirectiveFeatureTest.cs

+            Assert.Equal("Some/Path/{value}", template);
+        }
+
+        [Fact(Skip = "Re-evaluate this scenario after we use Razor to parse this stuff")]


My understanding was that we're not going to worry about this scenario for now, correct me if that's wrong.

Yeah let's ignore this for now

ryanbrandenburg · 2017-02-23T00:25:10Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

@@ -11,18 +11,43 @@ public static class PageDirectiveFeature
    {
        public static bool TryGetRouteTemplate(RazorProjectItem projectItem, out string template)


I feel like this function is kind of serving double duty to determing if it's a RazorPage (the bool return value) and extract the template (the out value). If that was intentional why, and if not might it be clearer with two functions?

Yeah this is really more like TryGetPageDirective

ryanbrandenburg · 2017-02-23T00:26:51Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

            const string PageDirective = "@page";

+            var stream = projectItem.Read();


I assume we have a guarantee that the underlying stream for projectItem.Read can be read twice?

It isn't through, it's read once here and then closed at line 32

rynowak · 2017-02-23T16:39:01Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

+            if (stream == null)
+            {
+                throw new ArgumentOutOfRangeException($"{nameof(projectItem)}.{nameof(projectItem.Read)} can't return null.");
+            }


This is kinda a strange check to put in

rynowak · 2017-02-23T16:40:29Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

            }

            if (content.StartsWith(PageDirective, StringComparison.Ordinal))
            {
-                var newLineIndex = content.IndexOf(Environment.NewLine, PageDirective.Length);
-                template = content.Substring(PageDirective.Length, newLineIndex - PageDirective.Length).Trim();
+                var endOfDirective = content.IndexOf(Environment.NewLine, PageDirective.Length);


We need to support all possible newlines on all possible systems. I would suggest creating a SourceDocument here and then using SourceDocument.Lines. We want to have explicit newline handling in as few places as possible.

rynowak · 2017-02-23T16:41:35Z

test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Infrastructure/PageDirectiveFeatureTest.cs

+The rest of the thing");
+
+            // Act & Assert
+            Assert.True(PageDirectiveFeature.TryGetRouteTemplate(projectItem, out template));


out var template 👍

A good cleanup item once we're on C#7.

rynowak · 2017-02-23T16:42:23Z

test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Infrastructure/PageDirectiveFeatureTest.cs

+        }
+
+        [Fact]
+        public void TryGetRouteTemplate_NoQuotesAroundPath()


Leave a comment here that in the future this will not be supported

rynowak · 2017-02-23T16:43:25Z

test/WebSites/RazorPagesWebSite/NoPage.cshtml

@@ -0,0 +1,6 @@
+@*
+    For more information on enabling MVC for empty projects, visit http://go.microsoft.com/fwlink/?LinkID=397860
+*@


Eww get rid of

Eilon · 2017-02-23T17:51:41Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

+                var endOfDirective = content.IndexOf(Environment.NewLine, PageDirective.Length);
+                if (endOfDirective < 0)
+                {
+                    var otherNewLine = Environment.NewLine == "/r/n" ? "/n" : "/r/n";


Ain't no slash gonna be a backslash if you ain't gonna slash the slash 🔫🔫 & 🌹🌹 .

Eilon · 2017-02-23T17:52:40Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

+                    endOfDirective = content.Length;
+                }
+
+                template = content.Substring(PageDirective.Length, endOfDirective - PageDirective.Length).Trim().Trim('"');


This will trim an arbitrary number of double-quotes from the start and end of the string. That is, the string """"""""hi, mom""""""""""""""""""" will become hi, mom. Is that intended here?

Hint: I strongly hope that this is not the intention - having too many quotes like this really ought to be an error.

ryanbrandenburg · 2017-02-24T20:55:19Z

🆙📅

Eilon · 2017-02-24T20:56:48Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

+                }
+                else
+                {
+                    template = template.Substring(1, template.Length - 2);


At this point is it guaranteed that there is a start quote and end quote?

I de-negafied the code the make this more clear.

Eilon · 2017-02-24T20:57:20Z

test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Infrastructure/PageDirectiveFeatureTest.cs

+            Assert.True(PageDirectiveFeature.TryGetPageDirective(projectItem, out template));
+            Assert.Equal("Some/Path/{value}", template);
+        }
+        [Fact]


Eilon · 2017-02-24T20:59:11Z

test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Infrastructure/PageDirectiveFeatureTest.cs

+        public void TryGetPageDirective_WrongNewLine()
+        {
+            // Arrange
+            string wrongNewLine = "\r\n";


And can just use a ternary expression here?

Eilon · 2017-02-24T21:01:20Z

test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Infrastructure/PageDirectiveFeatureTest.cs

+
+namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure
+{
+    public class PageDirectiveFeatureTest


Now that the quote trimming has changed, should there be a test to make sure that extraneous quotes are invalid? Or at least to see what happens?

It's my understanding that @rynowak wasn't wanting to mess with parsing and verifying these too much since we're eventually going to transition to using razor parsing which would deal with a lot of this for us. As written if you have multiple quotes it will include the extras as part of the template and I've added a test which proves that. I think this boils down to how much verifying of the template we want to do in this function. What if we have "this/is/"a/template" or "this/is/'a/template", both of these are invalid url parts, but is it this functions job to ensure that out template is a valid part of a url?

Yeah that'd be good to discuss with @rynowak

Eilon

I'm signing off on the part I reviewed about quote trimming.

NTaylorMullen · 2017-02-27T19:54:57Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

            {
-                var newLineIndex = content.IndexOf(Environment.NewLine, PageDirective.Length);
-                template = content.Substring(PageDirective.Length, newLineIndex - PageDirective.Length).Trim();
+                template = content.Substring(PageDirective.Length, content.Length - PageDirective.Length).Trim();


No need to trim again

This second trim will execute after the Substring which removes "@page" and will trim off any spaces between @page and the first ". Should I make it TrimStart to make that more explicit?

Ah, I see. Ya TrimSTart would help.

NTaylorMullen · 2017-02-27T19:56:21Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

            }

-            if (content.StartsWith(PageDirective, StringComparison.Ordinal))
+            if (content != null && content.StartsWith(PageDirective, StringComparison.Ordinal))


Easier to read if you just do:

if (content == null || !content.StartsWith(PageDirective, StringComparison.Ordinal)) { return false; }

and then un-nest the rest.

NTaylorMullen · 2017-02-27T20:07:43Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

+                // If it's not in quotes it's not our template
+                else
+                {
+                    template = string.Empty;


This is an interesting case. If a user writes: @page foo in bar.cshtml we'll map the Razor page to /bar. However, the user will then try to access their Razor page at /bar/foo with no luck (it'll 404). Ideally we could raise @page errors on startup so they could be fixed without a user wondering why they can't hit their page.

Not much we can do in this PR so mostly just throwing this out to thinking about it. What's everyone's thoughts?

I agree that silently ignoring this is going to cause confusion. We could probably log this.

ryanbrandenburg · 2017-02-28T18:53:03Z

🆙📅

NTaylorMullen

Looks good, just wanted to raise awareness for everyone else on #5838 (comment)

ajaybhargavb

Just one minor comment

ajaybhargavb · 2017-02-28T18:54:45Z

src/Microsoft.AspNetCore.Mvc.RazorPages/Infrastructure/PageDirectiveFeature.cs

+                {
+                    content = streamReader.ReadLine();
+                } while (content != null && string.IsNullOrWhiteSpace(content));
+                content = content != null ? content.Trim() : content;


content = content?.Trim()

Could also bring this outside Using block

ryanbrandenburg · 2017-02-28T22:48:13Z

I filed #5868 for that comment @NTaylorMullen.

dnfclas added the cla-already-signed label Feb 22, 2017

rynowak reviewed Feb 22, 2017

View reviewed changes

ryanbrandenburg force-pushed the RazorPagesTrimQuotes branch from 26369e9 to 57f5641 Compare February 22, 2017 23:52

ryanbrandenburg commented Feb 23, 2017

View reviewed changes

rynowak reviewed Feb 23, 2017

View reviewed changes

Eilon suggested changes Feb 23, 2017

View reviewed changes

ryanbrandenburg force-pushed the RazorPagesTrimQuotes branch from fd45e3d to 6b5e4c5 Compare February 24, 2017 18:42

Eilon suggested changes Feb 24, 2017

View reviewed changes

Eilon approved these changes Feb 24, 2017

View reviewed changes

ryanbrandenburg requested review from ajaybhargavb and NTaylorMullen February 27, 2017 19:48

NTaylorMullen suggested changes Feb 27, 2017

View reviewed changes

ryanbrandenburg force-pushed the RazorPagesTrimQuotes branch from b88b29f to af6eb81 Compare February 28, 2017 18:20

NTaylorMullen approved these changes Feb 28, 2017

View reviewed changes

ajaybhargavb approved these changes Feb 28, 2017

View reviewed changes

ryanbrandenburg mentioned this pull request Feb 28, 2017

RazorPages page directives missing quotes should alert user #5868

Closed

ryanbrandenburg force-pushed the RazorPagesTrimQuotes branch 2 times, most recently from e8c41b6 to 45d3f5f Compare March 3, 2017 01:07

Trim quotes from template

c9e122f

ryanbrandenburg force-pushed the RazorPagesTrimQuotes branch from 45d3f5f to c9e122f Compare March 3, 2017 17:21

ryanbrandenburg merged commit 85e28ae into dev Mar 3, 2017

ryanbrandenburg deleted the RazorPagesTrimQuotes branch March 3, 2017 17:52

		@@ -11,18 +11,43 @@ public static class PageDirectiveFeature
		{
		public static bool TryGetRouteTemplate(RazorProjectItem projectItem, out string template)

		const string PageDirective = "@page";

		var stream = projectItem.Read();

Trim quotes from template #5838

Trim quotes from template #5838

Conversation

ryanbrandenburg commented Feb 22, 2017

ryanbrandenburg commented Feb 22, 2017

Choose a reason for hiding this comment

rynowak commented Feb 22, 2017

ryanbrandenburg commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eilon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg commented Feb 28, 2017

NTaylorMullen left a comment

Choose a reason for hiding this comment

ajaybhargavb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg commented Feb 28, 2017