Addressed code review comments.

NTaylorMullen · NTaylorMullen · commit 4d221a140d89 · 2014-10-14T23:48:39.000-07:00
diff --git a/src/Microsoft.AspNet.Mvc.TagHelpers/FormTagHelper.cs b/src/Microsoft.AspNet.Mvc.TagHelpers/FormTagHelper.cs
@@ -55,44 +55,37 @@ public override void Process(TagHelperContext context, TagHelperOutput output)
         {
             bool antiForgeryDefault = true;
 
-            var routeValues = GetRouteValues(output);
+            var routePrefixedAttributes = output.FindPrefixedAttributes(RouteAttributePrefix);
 
             // If Action contains a '/' it means the user is attempting to use the FormTagHelper as a normal form.
             if (Action != null && Action.Contains('/'))
             {
-                if (Controller != null || routeValues != null)
+                if (Controller != null || routePrefixedAttributes.Any())
                 {
                     // We don't know how to generate a form action since a Controller attribute was also provided.
                     throw new InvalidOperationException(
                         Resources.FormatFormTagHelper_CannotDetermineAction(
                             "<form>",
-                            nameof(Action),
-                            nameof(Controller),
+                            nameof(Action).ToLowerInvariant(),
+                            nameof(Controller).ToLowerInvariant(),
                             RouteAttributePrefix));
                 }
 
-                // If the anti-forgery token was not specified then don't assume that it's on (user is using the
-                // FormTagHelper like a normal <form> tag).
+                // User is using the FormTagHelper like a normal <form> tag, anti-forgery default should be false to 
+                // not force the anti-forgery token onto the user.
                 antiForgeryDefault = false;
 
                 // Restore Action, Method and Route HTML attributes if they were provided, user wants non-TagHelper <form>.
-                output.RestoreBoundHtmlAttribute(nameof(Action), context);
+                output.CopyHtmlAttribute(nameof(Action), context);
 
                 if (Method != null)
                 {
-                    output.RestoreBoundHtmlAttribute(nameof(Method), context);
-                }
-
-                if (routeValues != null)
-                {
-                    foreach(var routeKeys in routeValues.Keys)
-                    {
-                        output.RestoreBoundHtmlAttribute("route-" + routeKeys, context);
-                    }
+                    output.CopyHtmlAttribute(nameof(Method), context);
                 }
             }
             else
             {
+                var routeValues = GetRouteValues(output, routePrefixedAttributes);
                 var tagBuilder = Generator.GenerateForm(ViewContext,
                                                         Action,
                                                         Controller,
@@ -102,15 +95,14 @@ public override void Process(TagHelperContext context, TagHelperOutput output)
 
                 if (tagBuilder != null)
                 {
-                    // We don't want to do a full merge because we want the TagHelper content to take presedence.
-                    output.MergeAttributes(tagBuilder);
+                    // We don't want to do a full merge because we want the TagHelper content to take precedence.
+                    output.Merge(tagBuilder);
                 }
             }
 
             if (AntiForgery ?? antiForgeryDefault)
             {
                 var antiForgeryTag = Generator.GenerateAntiForgery(ViewContext);
-
                 if (antiForgeryTag != null)
                 {
                     output.Content += antiForgeryTag.ToString(TagRenderMode.SelfClosing);
@@ -119,15 +111,17 @@ public override void Process(TagHelperContext context, TagHelperOutput output)
         }
 
         // TODO: We will not need this method once https://github.com/aspnet/Razor/issues/89 is completed.
-        private static Dictionary<string, object> GetRouteValues(TagHelperOutput output)
+        private static Dictionary<string, object> GetRouteValues(
+            TagHelperOutput output, IEnumerable<KeyValuePair<string, string>> routePrefixedAttributes)
         {
-            var prefixedValues = output.PullPrefixedAttributes(RouteAttributePrefix);
-
             Dictionary<string, object> routeValues = null;
-            if (prefixedValues.Any())
+            if (routePrefixedAttributes.Any())
             {
+                // Prefixed values should be treated as bound attributes, remove them from the output.
+                output.RemoveRange(routePrefixedAttributes);
+
                 // Generator.GenerateForm does not accept a Dictionary<string, string> for route values.
-                routeValues = prefixedValues.ToDictionary(
+                routeValues = routePrefixedAttributes.ToDictionary(
                     attribute => attribute.Key.Substring(RouteAttributePrefix.Length),
                     attribute => (object)attribute.Value);
             }
diff --git a/test/Microsoft.AspNet.Mvc.TagHelpers.Test/FormTagHelperTest.cs b/test/Microsoft.AspNet.Mvc.TagHelpers.Test/FormTagHelperTest.cs
@@ -50,14 +50,14 @@ public async Task ProcessAsync_GeneratesExpectedOutput()
                 },
                 content: "Something");
             var urlHelper = new Mock<IUrlHelper>();
-            var setup = urlHelper.Setup(mock => 
-                mock.Action(It.IsAny<string>(), 
-                            It.IsAny<string>(), 
-                            It.IsAny<object>(),
-                            It.IsAny<string>(),
-                            It.IsAny<string>(),
-                            It.IsAny<string>()));
-            setup.Returns("home/index");
+            urlHelper
+                .Setup(mock =>  mock.Action(It.IsAny<string>(), 
+                    It.IsAny<string>(), 
+                    It.IsAny<object>(),
+                    It.IsAny<string>(),
+                    It.IsAny<string>(),
+                    It.IsAny<string>()))
+                .Returns("home/index");
 
             var htmlGenerator = new TestableHtmlGenerator(metadataProvider, urlHelper.Object);
             var viewContext = TestableHtmlGenerator.GetViewContext(model: null, 
@@ -72,6 +72,7 @@ public async Task ProcessAsync_GeneratesExpectedOutput()
             await formTagHelper.ProcessAsync(tagHelperContext, output);
 
             // Assert
+            Assert.Equal(3, output.Attributes.Count);
             var attribute = Assert.Single(output.Attributes, kvp => kvp.Key.Equals("id"));
             Assert.Equal("myform", attribute.Value);
             attribute = Assert.Single(output.Attributes, kvp => kvp.Key.Equals("method"));
@@ -89,7 +90,7 @@ public async Task ProcessAsync_GeneratesExpectedOutput()
         public async Task ProcessAsync_GeneratesAntiForgeryCorrectly(bool? antiForgery, string expectedContent)
         {
             // Arrange
-            var expectedViewContext = CreateViewContext();
+            var viewContext = CreateViewContext();
             var formTagHelper = new FormTagHelper
             {
                 Action = "Index",
@@ -102,18 +103,20 @@ public async Task ProcessAsync_GeneratesAntiForgeryCorrectly(bool? antiForgery,
                 attributes: new Dictionary<string, string>(),
                 content: string.Empty);
             var generator = new Mock<IHtmlGenerator>(MockBehavior.Strict);
-            generator.Setup(mock =>
-                mock.GenerateForm(It.IsAny<ViewContext>(),
-                                  It.IsAny<string>(),
-                                  It.IsAny<string>(),
-                                  It.IsAny<object>(),
-                                  It.IsAny<string>(),
-                                  It.IsAny<object>()))
-                    .Returns(new TagBuilder("form"));
-            generator.Setup(mock => mock.GenerateAntiForgery(expectedViewContext))
+            generator
+                .Setup(mock => mock.GenerateForm(
+                    It.IsAny<ViewContext>(),
+                    It.IsAny<string>(),
+                    It.IsAny<string>(),
+                    It.IsAny<object>(),
+                    It.IsAny<string>(),
+                    It.IsAny<object>()))
+                .Returns(new TagBuilder("form"));
+
+            generator.Setup(mock => mock.GenerateAntiForgery(viewContext))
                      .Returns(new TagBuilder("input"));
 
-            SetViewContextAndGenerator(formTagHelper, expectedViewContext, generator.Object);
+            SetViewContextAndGenerator(formTagHelper, viewContext, generator.Object);
 
             // Act
             await formTagHelper.ProcessAsync(context, output);
@@ -125,7 +128,7 @@ public async Task ProcessAsync_GeneratesAntiForgeryCorrectly(bool? antiForgery,
         }
 
         [Fact]
-        public async Task ProcessAsync_UnderstandsRouteValues()
+        public async Task ProcessAsync_BindsRouteValuesFromTagHelperOutput()
         {
             // Arrange
             var testViewContext = CreateViewContext();
@@ -136,39 +139,42 @@ public async Task ProcessAsync_UnderstandsRouteValues()
             };
             var context = new TagHelperContext(
                 allAttributes: new Dictionary<string, object>());
+            var expectedAttribute = new KeyValuePair<string, string>("ROUTEE-NotRoute", "something");
             var output = new TagHelperOutput(
                 "form",
                 attributes: new Dictionary<string, string>()
                 {
                     { "route-val", "hello" },
-                    { "roUte--Foo", "bar" },
-                    { "ROUTEE-NotRoute", "something" }
+                    { "roUte--Foo", "bar" }
                 },
                 content: string.Empty);
-            var generator = new Mock<IHtmlGenerator>(MockBehavior.Strict);
-            var setup = generator.Setup(mock =>
-                mock.GenerateForm(It.IsAny<ViewContext>(),
-                                  It.IsAny<string>(),
-                                  It.IsAny<string>(),
-                                  It.IsAny<object>(),
-                                  It.IsAny<string>(),
-                                  It.IsAny<object>()));
-            setup.Callback<ViewContext, string, string, object, string, object>(
-            (viewContext, actionName, controllerName, routeValues, method, htmlAttributes) =>
-            {
-                // Fixes Roslyn bug with lambdas
-                generator.ToString();
+            output.Attributes.Add(expectedAttribute);
 
-                var routeValueDictionary = (Dictionary<string, object>)routeValues;
-
-                Assert.Equal(2, routeValueDictionary.Count);
-                var routeValue = Assert.Single(routeValueDictionary, kvp => kvp.Key.Equals("val"));
-                Assert.Equal("hello", routeValue.Value);
-                routeValue = Assert.Single(routeValueDictionary, kvp => kvp.Key.Equals("-Foo"));
-                Assert.Equal("bar", routeValue.Value);
-            });
-            setup.Returns(new TagBuilder("form"));
-            setup.Verifiable();
+            var generator = new Mock<IHtmlGenerator>(MockBehavior.Strict);
+            generator
+                .Setup(mock =>mock.GenerateForm(
+                    It.IsAny<ViewContext>(),
+                    It.IsAny<string>(),
+                    It.IsAny<string>(),
+                    It.IsAny<object>(),
+                    It.IsAny<string>(),
+                    It.IsAny<object>()))
+                .Callback<ViewContext, string, string, object, string, object>(
+                    (viewContext, actionName, controllerName, routeValues, method, htmlAttributes) =>
+                    {
+                        // Fixes Roslyn bug with lambdas
+                        generator.ToString();
+
+                        var routeValueDictionary = (Dictionary<string, object>)routeValues;
+
+                        Assert.Equal(2, routeValueDictionary.Count);
+                        var routeValue = Assert.Single(routeValueDictionary, kvp => kvp.Key.Equals("val"));
+                        Assert.Equal("hello", routeValue.Value);
+                        routeValue = Assert.Single(routeValueDictionary, kvp => kvp.Key.Equals("-Foo"));
+                        Assert.Equal("bar", routeValue.Value);
+                    })
+                .Returns(new TagBuilder("form"))
+                .Verifiable();
 
             SetViewContextAndGenerator(formTagHelper, testViewContext, generator.Object);
 
@@ -177,7 +183,7 @@ public async Task ProcessAsync_UnderstandsRouteValues()
 
             Assert.Equal("form", output.TagName);
             var attribute = Assert.Single(output.Attributes);
-            Assert.Equal("something", attribute.Value);
+            Assert.Equal(expectedAttribute, attribute);
             Assert.Empty(output.Content);
             generator.Verify();
         }
@@ -186,7 +192,7 @@ public async Task ProcessAsync_UnderstandsRouteValues()
         public async Task ProcessAsync_CallsIntoGenerateFormWithExpectedParameters()
         {
             // Arrange
-            var expectedViewContext = CreateViewContext();
+            var viewContext = CreateViewContext();
             var formTagHelper = new FormTagHelper
             {
                 Action = "Index",
@@ -201,13 +207,13 @@ public async Task ProcessAsync_CallsIntoGenerateFormWithExpectedParameters()
                 attributes: new Dictionary<string, string>(),
                 content: string.Empty);
             var generator = new Mock<IHtmlGenerator>(MockBehavior.Strict);
-            var setup = generator.Setup(mock =>
-                mock.GenerateForm(expectedViewContext, "Index", "Home", null, "POST", null));
-            setup.Returns(new TagBuilder("form"));
-            setup.Verifiable();
+            generator
+                .Setup(mock => mock.GenerateForm(viewContext, "Index", "Home", null, "POST", null))
+                .Returns(new TagBuilder("form"))
+                .Verifiable();
 
             SetViewContextAndGenerator(formTagHelper,
-                                       expectedViewContext,
+                                       viewContext,
                                        generator.Object);
 
             // Act & Assert
@@ -243,6 +249,7 @@ public async Task ProcessAsync_RestoresBoundAttributesIfActionIsURL()
 
             // Assert
             Assert.Equal("form", output.TagName);
+            Assert.Equal(2, output.Attributes.Count);
             var attribute = Assert.Single(output.Attributes, kvp => kvp.Key.Equals("aCTiON"));
             Assert.Equal("http://www.contoso.com", attribute.Value);
             attribute = Assert.Single(output.Attributes, kvp => kvp.Key.Equals("METhod"));
@@ -254,10 +261,10 @@ public async Task ProcessAsync_RestoresBoundAttributesIfActionIsURL()
         [InlineData(true, "<input />")]
         [InlineData(false, "")]
         [InlineData(null, "")]
-        public async Task ProcessAsync_AllowsAntiForgeryIfActionIsURL(bool? antiForgery, string expectedContent)
+        public async Task ProcessAsync_SupportsAntiForgeryIfActionIsURL(bool? antiForgery, string expectedContent)
         {
             // Arrange
-            var expectedViewContext = CreateViewContext();
+            var viewContext = CreateViewContext();
             var generator = new Mock<IHtmlGenerator>();
             generator.Setup(mock => mock.GenerateAntiForgery(It.IsAny<ViewContext>()))
                      .Returns(new TagBuilder("input"));
@@ -267,7 +274,7 @@ public async Task ProcessAsync_AllowsAntiForgeryIfActionIsURL(bool? antiForgery,
                 AntiForgery = antiForgery,
             };
             SetViewContextAndGenerator(formTagHelper,
-                                       expectedViewContext,
+                                       viewContext,
                                        generator.Object);
             var output = new TagHelperOutput("form",
                                              attributes: new Dictionary<string, string>(),
@@ -283,8 +290,8 @@ public async Task ProcessAsync_AllowsAntiForgeryIfActionIsURL(bool? antiForgery,
 
             // Assert
             Assert.Equal("form", output.TagName);
-            var attribute = Assert.Single(output.Attributes, kvp => kvp.Key.Equals("aCTiON"));
-            Assert.Equal("http://www.contoso.com", attribute.Value);
+            var attribute = Assert.Single(output.Attributes);
+            Assert.Equal(new KeyValuePair<string, string>("aCTiON", "http://www.contoso.com"), attribute);
             Assert.Equal(expectedContent, output.Content);
         }
 
@@ -298,8 +305,8 @@ public async Task ProcessAsync_ThrowsIfActionIsUrlWithSpecifiedController()
                 Controller = "Home",
                 Method = "POST"
             };
-            var expectedErrorMessage = "Cannot determine an Action for <form>. A <form> with a URL-based Action " +
-                                       "must not have attributes starting with route- or a Controller attribute.";
+            var expectedErrorMessage = "Cannot determine an action for <form>. A <form> with a URL-based action " +
+                                       "must not have attributes starting with route- or a controller attribute.";
             var tagHelperOutput = new TagHelperOutput(
                 "form",
                 attributes: new Dictionary<string, string>(),
@@ -323,8 +330,8 @@ public async Task ProcessAsync_ThrowsIfActionIsUrlWithSpecifiedRoutes()
                 Action = "http://www.contoso.com",
                 Method = "POST"
             };
-            var expectedErrorMessage = "Cannot determine an Action for <form>. A <form> with a URL-based Action " +
-                                       "must not have attributes starting with route- or a Controller attribute.";
+            var expectedErrorMessage = "Cannot determine an action for <form>. A <form> with a URL-based action " +
+                                       "must not have attributes starting with route- or a controller attribute.";
             var tagHelperOutput = new TagHelperOutput(
                 "form",
                 attributes: new Dictionary<string, string>

Original file line number	Diff line number	Diff line change
`@@ -55,44 +55,37 @@ public override void Process(TagHelperContext context, TagHelperOutput output)`
`55`	`55`	`{`
`56`	`56`	`bool antiForgeryDefault = true;`
`57`	`57`
`58`		`- var routeValues = GetRouteValues(output);`
	`58`	`+ var routePrefixedAttributes = output.FindPrefixedAttributes(RouteAttributePrefix);`
`59`	`59`
`60`	`60`	`// If Action contains a '/' it means the user is attempting to use the FormTagHelper as a normal form.`
`61`	`61`	`if (Action != null && Action.Contains('/'))`
`62`	`62`	`{`
`63`		`- if (Controller != null \|\| routeValues != null)`
	`63`	`+ if (Controller != null \|\| routePrefixedAttributes.Any())`
`64`	`64`	`{`
`65`	`65`	`// We don't know how to generate a form action since a Controller attribute was also provided.`
`66`	`66`	`throw new InvalidOperationException(`
`67`	`67`	`Resources.FormatFormTagHelper_CannotDetermineAction(`
`68`	`68`	`"<form>",`
`69`		`- nameof(Action),`
`70`		`- nameof(Controller),`
	`69`	`+ nameof(Action).ToLowerInvariant(),`
	`70`	`+ nameof(Controller).ToLowerInvariant(),`
`71`	`71`	`RouteAttributePrefix));`
`72`	`72`	`}`
`73`	`73`
`74`		`- // If the anti-forgery token was not specified then don't assume that it's on (user is using the`
`75`		`- // FormTagHelper like a normal <form> tag).`
	`74`	`+ // User is using the FormTagHelper like a normal <form> tag, anti-forgery default should be false to`
	`75`	`+ // not force the anti-forgery token onto the user.`
`76`	`76`	`antiForgeryDefault = false;`
`77`	`77`
`78`	`78`	`// Restore Action, Method and Route HTML attributes if they were provided, user wants non-TagHelper <form>.`
`79`		`- output.RestoreBoundHtmlAttribute(nameof(Action), context);`
	`79`	`+ output.CopyHtmlAttribute(nameof(Action), context);`
`80`	`80`
`81`	`81`	`if (Method != null)`
`82`	`82`	`{`
`83`		`- output.RestoreBoundHtmlAttribute(nameof(Method), context);`
`84`		`- }`
`85`		`-`
`86`		`- if (routeValues != null)`
`87`		`- {`
`88`		`- foreach(var routeKeys in routeValues.Keys)`
`89`		`- {`
`90`		`- output.RestoreBoundHtmlAttribute("route-" + routeKeys, context);`
`91`		`- }`
	`83`	`+ output.CopyHtmlAttribute(nameof(Method), context);`
`92`	`84`	`}`
`93`	`85`	`}`
`94`	`86`	`else`
`95`	`87`	`{`
	`88`	`+ var routeValues = GetRouteValues(output, routePrefixedAttributes);`
`96`	`89`	`var tagBuilder = Generator.GenerateForm(ViewContext,`
`97`	`90`	`Action,`
`98`	`91`	`Controller,`
`@@ -102,15 +95,14 @@ public override void Process(TagHelperContext context, TagHelperOutput output)`
`102`	`95`
`103`	`96`	`if (tagBuilder != null)`
`104`	`97`	`{`
`105`		`- // We don't want to do a full merge because we want the TagHelper content to take presedence.`
`106`		`- output.MergeAttributes(tagBuilder);`
	`98`	`+ // We don't want to do a full merge because we want the TagHelper content to take precedence.`
	`99`	`+ output.Merge(tagBuilder);`
`107`	`100`	`}`
`108`	`101`	`}`
`109`	`102`
`110`	`103`	`if (AntiForgery ?? antiForgeryDefault)`
`111`	`104`	`{`
`112`	`105`	`var antiForgeryTag = Generator.GenerateAntiForgery(ViewContext);`
`113`		`-`
`114`	`106`	`if (antiForgeryTag != null)`
`115`	`107`	`{`
`116`	`108`	`output.Content += antiForgeryTag.ToString(TagRenderMode.SelfClosing);`
`@@ -119,15 +111,17 @@ public override void Process(TagHelperContext context, TagHelperOutput output)`
`119`	`111`	`}`
`120`	`112`
`121`	`113`	`// TODO: We will not need this method once https://github.com/aspnet/Razor/issues/89 is completed.`
`122`		`- private static Dictionary<string, object> GetRouteValues(TagHelperOutput output)`
	`114`	`+ private static Dictionary<string, object> GetRouteValues(`
	`115`	`+ TagHelperOutput output, IEnumerable<KeyValuePair<string, string>> routePrefixedAttributes)`
`123`	`116`	`{`
`124`		`- var prefixedValues = output.PullPrefixedAttributes(RouteAttributePrefix);`
`125`		`-`
`126`	`117`	`Dictionary<string, object> routeValues = null;`
`127`		`- if (prefixedValues.Any())`
	`118`	`+ if (routePrefixedAttributes.Any())`
`128`	`119`	`{`
	`120`	`+ // Prefixed values should be treated as bound attributes, remove them from the output.`
	`121`	`+ output.RemoveRange(routePrefixedAttributes);`
	`122`	`+`
`129`	`123`	`// Generator.GenerateForm does not accept a Dictionary<string, string> for route values.`
`130`		`- routeValues = prefixedValues.ToDictionary(`
	`124`	`+ routeValues = routePrefixedAttributes.ToDictionary(`
`131`	`125`	`attribute => attribute.Key.Substring(RouteAttributePrefix.Length),`
`132`	`126`	`attribute => (object)attribute.Value);`
`133`	`127`	`}`