PR comments and some smallish cleanup

- `IRazorViewEngine.MakePathAbsolute()` -> `GetAbsolutePath()`
- set `IsPartial` on all `IRazorPage` instances
- improve consistency of methods in `HtmlHelperPartialExtensions`
  - a couple unnecessarily passed `htmlHelper.ViewData`
  - add missing tests of these extension methods
- restore parameter checks in `CompositeViewEngine`
- reduce `List<string>` and remove enumerator allocations in `CompositeViewEngine`

nits:
- correct a few comments
- use `<seealso/>`
This commit is contained in:
Doug Bunting 2015-11-15 22:42:07 -08:00
parent d1fe824b5d
commit cf30bb730f
10 changed files with 224 additions and 52 deletions

View File

@ -18,7 +18,7 @@ namespace Microsoft.AspNet.Mvc.Razor
/// <param name="pageName">The name of the page.</param>
/// <param name="isPartial">Determines if the page being found is a partial.</param>
/// <returns>The <see cref="RazorPageResult"/> of locating the page.</returns>
/// <remarks>Page search semantics match <see cref="IViewEngine.FindView"/>.</remarks>
/// <remarks><seealso cref="IViewEngine.FindView"/>.</remarks>
RazorPageResult FindPage(ActionContext context, string pageName, bool isPartial);
/// <summary>
@ -29,7 +29,7 @@ namespace Microsoft.AspNet.Mvc.Razor
/// <param name="pagePath">The path to the page.</param>
/// <param name="isPartial">Determines if the page being found is a partial.</param>
/// <returns>The <see cref="RazorPageResult"/> of locating the page.</returns>
/// <remarks>See also <see cref="IViewEngine.GetView"/>.</remarks>
/// <remarks><seealso cref="IViewEngine.GetView"/>.</remarks>
RazorPageResult GetPage(string executingFilePath, string pagePath, bool isPartial);
/// <summary>
@ -43,6 +43,6 @@ namespace Microsoft.AspNet.Mvc.Razor
/// <paramref name="pagePath"/> is a relative path. The <paramref name="pagePath"/> value (unchanged)
/// otherwise.
/// </returns>
string MakePathAbsolute(string executingFilePath, string pagePath);
string GetAbsolutePath(string executingFilePath, string pagePath);
}
}

View File

@ -174,7 +174,7 @@ namespace Microsoft.AspNet.Mvc.Razor
// Pass correct absolute path to next layout or the entry page if this view start set Layout to a
// relative path.
layout = _viewEngine.MakePathAbsolute(viewStart.Path, viewStart.Layout);
layout = _viewEngine.GetAbsolutePath(viewStart.Path, viewStart.Layout);
}
}
finally

View File

@ -276,7 +276,7 @@ namespace Microsoft.AspNet.Mvc.Razor
private ViewLocationCacheResult LocatePageFromPath(string executingFilePath, string pagePath, bool isPartial)
{
var applicationRelativePath = MakePathAbsolute(executingFilePath, pagePath);
var applicationRelativePath = GetAbsolutePath(executingFilePath, pagePath);
var cacheKey = new ViewLocationCacheKey(applicationRelativePath, isPartial);
ViewLocationCacheResult cacheResult;
if (!ViewLookupCache.TryGetValue(cacheKey, out cacheResult))
@ -350,7 +350,7 @@ namespace Microsoft.AspNet.Mvc.Razor
}
/// <inheritdoc />
public string MakePathAbsolute(string executingFilePath, string pagePath)
public string GetAbsolutePath(string executingFilePath, string pagePath)
{
if (string.IsNullOrEmpty(pagePath))
{
@ -505,11 +505,14 @@ namespace Microsoft.AspNet.Mvc.Razor
}
var page = result.ViewEntry.PageFactory();
page.IsPartial = isPartial;
var viewStarts = new IRazorPage[result.ViewStartEntries.Count];
for (var i = 0; i < viewStarts.Length; i++)
{
var viewStartItem = result.ViewStartEntries[i];
viewStarts[i] = viewStartItem.PageFactory();
viewStarts[i].IsPartial = true;
}
var view = new RazorView(

View File

@ -68,7 +68,7 @@ namespace Microsoft.AspNet.Mvc.Rendering
throw new ArgumentNullException(nameof(partialViewName));
}
return htmlHelper.PartialAsync(partialViewName, htmlHelper.ViewData.Model, viewData: viewData);
return htmlHelper.PartialAsync(partialViewName, htmlHelper.ViewData.Model, viewData);
}
/// <summary>
@ -259,8 +259,7 @@ namespace Microsoft.AspNet.Mvc.Rendering
throw new ArgumentNullException(nameof(partialViewName));
}
return htmlHelper.RenderPartialAsync(partialViewName, htmlHelper.ViewData.Model,
viewData: htmlHelper.ViewData);
return htmlHelper.RenderPartialAsync(partialViewName, htmlHelper.ViewData.Model, viewData: null);
}
/// <summary>
@ -290,7 +289,7 @@ namespace Microsoft.AspNet.Mvc.Rendering
throw new ArgumentNullException(nameof(partialViewName));
}
return htmlHelper.RenderPartialAsync(partialViewName, htmlHelper.ViewData.Model, viewData: viewData);
return htmlHelper.RenderPartialAsync(partialViewName, htmlHelper.ViewData.Model, viewData);
}
/// <summary>
@ -320,7 +319,7 @@ namespace Microsoft.AspNet.Mvc.Rendering
throw new ArgumentNullException(nameof(partialViewName));
}
return htmlHelper.RenderPartialAsync(partialViewName, model, htmlHelper.ViewData);
return htmlHelper.RenderPartialAsync(partialViewName, model, viewData: null);
}
}
}

View File

@ -14,7 +14,7 @@ namespace Microsoft.AspNet.Mvc.Rendering
/// </summary>
public class ViewContext : ActionContext
{
// We need a default FormContext if the user uses html <form> instead of an MvcForm
// We need a default FormContext if the user uses HTML <form> instead of an MvcForm
private readonly FormContext _defaultFormContext = new FormContext();
private FormContext _formContext;

View File

@ -1,8 +1,10 @@
// Copyright (c) .NET Foundation. All rights reserved.
// 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 System.Linq;
using Microsoft.AspNet.Mvc.ViewFeatures;
using Microsoft.Extensions.OptionsModel;
namespace Microsoft.AspNet.Mvc.ViewEngines
@ -10,8 +12,6 @@ namespace Microsoft.AspNet.Mvc.ViewEngines
/// <inheritdoc />
public class CompositeViewEngine : ICompositeViewEngine
{
private const string ViewExtension = ".cshtml";
/// <summary>
/// Initializes a new instance of <see cref="CompositeViewEngine"/>.
/// </summary>
@ -27,10 +27,22 @@ namespace Microsoft.AspNet.Mvc.ViewEngines
/// <inheritdoc />
public ViewEngineResult FindView(ActionContext context, string viewName, bool isPartial)
{
List<string> searchedLocations = null;
foreach (var engine in ViewEngines)
if (context == null)
{
var result = engine.FindView(context, viewName, isPartial);
throw new ArgumentNullException(nameof(context));
}
if (string.IsNullOrEmpty(viewName))
{
throw new ArgumentException(Resources.ArgumentCannotBeNullOrEmpty, nameof(viewName));
}
// Do not allocate in the common cases: ViewEngines contains one entry or initial attempt is successful.
IEnumerable<string> searchedLocations = null;
List<string> searchedList = null;
for (var index = 0; index < ViewEngines.Count; index++)
{
var result = ViewEngines[index].FindView(context, viewName, isPartial);
if (result.Success)
{
return result;
@ -38,11 +50,19 @@ namespace Microsoft.AspNet.Mvc.ViewEngines
if (searchedLocations == null)
{
searchedLocations = new List<string>(result.SearchedLocations);
// First failure.
searchedLocations = result.SearchedLocations;
}
else
{
searchedLocations.AddRange(result.SearchedLocations);
if (searchedList == null)
{
// Second failure.
searchedList = new List<string>(searchedLocations);
searchedLocations = searchedList;
}
searchedList.AddRange(result.SearchedLocations);
}
}
@ -52,10 +72,17 @@ namespace Microsoft.AspNet.Mvc.ViewEngines
/// <inheritdoc />
public ViewEngineResult GetView(string executingFilePath, string viewPath, bool isPartial)
{
List<string> searchedLocations = null;
foreach (var engine in ViewEngines)
if (string.IsNullOrEmpty(viewPath))
{
var result = engine.GetView(executingFilePath, viewPath, isPartial);
throw new ArgumentException(Resources.ArgumentCannotBeNullOrEmpty, nameof(viewPath));
}
// Do not allocate in the common cases: ViewEngines contains one entry or initial attempt is successful.
IEnumerable<string> searchedLocations = null;
List<string> searchedList = null;
for (var index = 0; index < ViewEngines.Count; index++)
{
var result = ViewEngines[index].GetView(executingFilePath, viewPath, isPartial);
if (result.Success)
{
return result;
@ -63,11 +90,19 @@ namespace Microsoft.AspNet.Mvc.ViewEngines
if (searchedLocations == null)
{
searchedLocations = new List<string>(result.SearchedLocations);
// First failure.
searchedLocations = result.SearchedLocations;
}
else
{
searchedLocations.AddRange(result.SearchedLocations);
if (searchedList == null)
{
// Second failure.
searchedList = new List<string>(searchedLocations);
searchedLocations = searchedList;
}
searchedList.AddRange(result.SearchedLocations);
}
}

View File

@ -46,7 +46,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
" The following locations were searched:" + PlatformNormalizer.GetNewLinesAsUnderscores(1) +
"/Areas/Users/Views/Home/Index.cshtml" + PlatformNormalizer.GetNewLinesAsUnderscores(1) +
"/Areas/Users/Views/Shared/Index.cshtml" + PlatformNormalizer.GetNewLinesAsUnderscores(1) +
"/Views/Shared/Index.cshtml.",
"/Views/Shared/Index.cshtml",
exception.ExceptionMessage);
}

View File

@ -1306,13 +1306,13 @@ namespace Microsoft.AspNet.Mvc.Razor.Test
[InlineData("/Home/Index.cshtml", "Page")]
[InlineData("/Home/Index.cshtml", "Folder/Page")]
[InlineData("/Home/Index.cshtml", "Folder1/Folder2/Page")]
public void MakePathAbsolute_ReturnsPagePathUnchanged_IfNotAPath(string executingFilePath, string pagePath)
public void GetAbsolutePath_ReturnsPagePathUnchanged_IfNotAPath(string executingFilePath, string pagePath)
{
// Arrange
var viewEngine = CreateViewEngine();
// Act
var result = viewEngine.MakePathAbsolute(executingFilePath, pagePath);
var result = viewEngine.GetAbsolutePath(executingFilePath, pagePath);
// Assert
Assert.Same(pagePath, result);
@ -1325,13 +1325,13 @@ namespace Microsoft.AspNet.Mvc.Razor.Test
[InlineData("/Home/Index.cshtml", "~/Page")]
[InlineData("/Home/Index.cshtml", "/Folder/Page.cshtml")]
[InlineData("/Home/Index.cshtml", "~/Folder1/Folder2/Page.rzr")]
public void MakePathAbsolute_ReturnsPageUnchanged_IfAppRelative(string executingFilePath, string pagePath)
public void GetAbsolutePath_ReturnsPageUnchanged_IfAppRelative(string executingFilePath, string pagePath)
{
// Arrange
var viewEngine = CreateViewEngine();
// Act
var result = viewEngine.MakePathAbsolute(executingFilePath, pagePath);
var result = viewEngine.GetAbsolutePath(executingFilePath, pagePath);
// Assert
Assert.Same(pagePath, result);
@ -1341,14 +1341,14 @@ namespace Microsoft.AspNet.Mvc.Razor.Test
[InlineData("Page.cshtml")]
[InlineData("Folder/Page.cshtml")]
[InlineData("../../Folder1/Folder2/Page.cshtml")]
public void MakePathAbsolute_ResolvesRelativeToExecutingPage(string pagePath)
public void GetAbsolutePath_ResolvesRelativeToExecutingPage(string pagePath)
{
// Arrange
var expectedPagePath = "/Home/" + pagePath;
var viewEngine = CreateViewEngine();
// Act
var result = viewEngine.MakePathAbsolute("/Home/Page.cshtml", pagePath);
var result = viewEngine.GetAbsolutePath("/Home/Page.cshtml", pagePath);
// Assert
Assert.Equal(expectedPagePath, result);
@ -1358,14 +1358,14 @@ namespace Microsoft.AspNet.Mvc.Razor.Test
[InlineData("Page.cshtml")]
[InlineData("Folder/Page.cshtml")]
[InlineData("../../Folder1/Folder2/Page.cshtml")]
public void MakePathAbsolute_ResolvesRelativeToAppRoot_IfNoPageExecuting(string pagePath)
public void GetAbsolutePath_ResolvesRelativeToAppRoot_IfNoPageExecuting(string pagePath)
{
// Arrange
var expectedPagePath = "/" + pagePath;
var viewEngine = CreateViewEngine();
// Act
var result = viewEngine.MakePathAbsolute(executingFilePath: null, pagePath: pagePath);
var result = viewEngine.GetAbsolutePath(executingFilePath: null, pagePath: pagePath);
// Assert
Assert.Equal(expectedPagePath, result);
@ -1649,7 +1649,7 @@ namespace Microsoft.AspNet.Mvc.Razor.Test
}
}
// Return RazorViewEngine with factories that always successfully create instances.
// Return RazorViewEngine with a page factory provider that is always successful.
private RazorViewEngine CreateSuccessfulViewEngine()
{
var pageFactory = new Mock<IRazorPageFactoryProvider>(MockBehavior.Strict);

View File

@ -135,7 +135,7 @@ namespace Microsoft.AspNet.Mvc.Razor
var activator = Mock.Of<IRazorPageActivator>();
var viewEngine = new Mock<IRazorViewEngine>();
viewEngine
.Setup(p => p.MakePathAbsolute("_ViewStart", LayoutPath))
.Setup(p => p.GetAbsolutePath("_ViewStart", LayoutPath))
.Returns(LayoutPath);
viewEngine
.Setup(v => v.GetPage(pagePath, LayoutPath, /*isPartial*/ true))
@ -345,10 +345,10 @@ namespace Microsoft.AspNet.Mvc.Razor
var viewEngine = new Mock<IRazorViewEngine>(MockBehavior.Strict);
viewEngine
.Setup(engine => engine.MakePathAbsolute(/*executingFilePath*/ null, "/fake-layout-path"))
.Setup(engine => engine.GetAbsolutePath(/*executingFilePath*/ null, "/fake-layout-path"))
.Returns("/fake-layout-path");
viewEngine
.Setup(engine => engine.MakePathAbsolute(/*executingFilePath*/ null, layoutPath))
.Setup(engine => engine.GetAbsolutePath(/*executingFilePath*/ null, layoutPath))
.Returns(layoutPath);
var view = new RazorView(
@ -1588,10 +1588,10 @@ namespace Microsoft.AspNet.Mvc.Razor
});
var viewEngine = new Mock<IRazorViewEngine>(MockBehavior.Strict);
viewEngine
.Setup(engine => engine.MakePathAbsolute(/*executingFilePath*/ null, expectedViewStart))
.Setup(engine => engine.GetAbsolutePath(/*executingFilePath*/ null, expectedViewStart))
.Returns(expectedViewStart);
viewEngine
.Setup(engine => engine.MakePathAbsolute(/*executingFilePath*/ null, expectedPage))
.Setup(engine => engine.GetAbsolutePath(/*executingFilePath*/ null, expectedPage))
.Returns(expectedPage);
var view = new RazorView(
@ -1646,10 +1646,10 @@ namespace Microsoft.AspNet.Mvc.Razor
var viewEngine = new Mock<IRazorViewEngine>(MockBehavior.Strict);
viewEngine
.Setup(engine => engine.MakePathAbsolute("~/_ViewStart.cshtml", "_Layout.cshtml"))
.Setup(engine => engine.GetAbsolutePath("~/_ViewStart.cshtml", "_Layout.cshtml"))
.Returns("~/_Layout.cshtml");
viewEngine
.Setup(engine => engine.MakePathAbsolute("~/Home/_ViewStart.cshtml", "_Layout.cshtml"))
.Setup(engine => engine.GetAbsolutePath("~/Home/_ViewStart.cshtml", "_Layout.cshtml"))
.Returns("~/Home/_Layout.cshtml");
var view = new RazorView(
@ -1691,10 +1691,10 @@ namespace Microsoft.AspNet.Mvc.Razor
});
var viewEngine = new Mock<IRazorViewEngine>(MockBehavior.Strict);
viewEngine
.Setup(engine => engine.MakePathAbsolute(/*executingFilePath*/ null, "Layout"))
.Setup(engine => engine.GetAbsolutePath(/*executingFilePath*/ null, "Layout"))
.Returns("Layout");
viewEngine
.Setup(engine => engine.MakePathAbsolute(/*executingFilePath*/ null, /*pagePath*/ null))
.Setup(engine => engine.GetAbsolutePath(/*executingFilePath*/ null, /*pagePath*/ null))
.Returns<string>(null);
var view = new RazorView(
@ -1737,7 +1737,7 @@ namespace Microsoft.AspNet.Mvc.Razor
});
var viewEngine = new Mock<IRazorViewEngine>(MockBehavior.Strict);
viewEngine
.Setup(p => p.MakePathAbsolute(/*executingFilePath*/ null, "/Layout.cshtml"))
.Setup(p => p.GetAbsolutePath(/*executingFilePath*/ null, "/Layout.cshtml"))
.Returns("/Layout.cshtml");
viewEngine
.Setup(p => p.GetPage(/*executingFilePath*/ null, "/Layout.cshtml", /*isPartial*/ true))

View File

@ -17,24 +17,62 @@ namespace Microsoft.AspNet.Mvc.Rendering
{
public class HtmlHelperPartialExtensionsTest
{
public static TheoryData<Func<IHtmlHelper, IHtmlContent>> PartialExtensionMethods
// Func<IHtmlHelper, IHtmlContent>, expected Model, expected ViewDataDictionary
public static TheoryData<Func<IHtmlHelper, IHtmlContent>, object, ViewDataDictionary> PartialExtensionMethods
{
get
{
var vdd = new ViewDataDictionary(new EmptyModelMetadataProvider());
return new TheoryData<Func<IHtmlHelper, IHtmlContent>>
var viewData = new ViewDataDictionary(new EmptyModelMetadataProvider());
var model = new object();
return new TheoryData<Func<IHtmlHelper, IHtmlContent>, object, ViewDataDictionary>
{
helper => helper.Partial("test"),
helper => helper.Partial("test", new object()),
helper => helper.Partial("test", vdd),
helper => helper.Partial("test", new object(), vdd)
{ helper => helper.Partial("test"), null, null },
{ helper => helper.Partial("test", model), model, null },
{ helper => helper.Partial("test", viewData), null, viewData },
{ helper => helper.Partial("test", model, viewData), model, viewData },
};
}
}
[Theory]
[MemberData(nameof(PartialExtensionMethods))]
public void PartialMethods_DoesNotWrapThrownException(Func<IHtmlHelper, IHtmlContent> partialMethod)
public void PartialMethods_CallHtmlHelperWithExpectedArguments(
Func<IHtmlHelper, IHtmlContent> partialMethod,
object expectedModel,
ViewDataDictionary expectedViewData)
{
// Arrange
var htmlContent = Mock.Of<IHtmlContent>();
var helper = new Mock<IHtmlHelper>(MockBehavior.Strict);
if (expectedModel == null)
{
// Extension methods without model parameter use ViewData.Model to get Model.
var viewData = expectedViewData ?? new ViewDataDictionary(new EmptyModelMetadataProvider());
helper
.SetupGet(h => h.ViewData)
.Returns(viewData)
.Verifiable();
}
helper
.Setup(h => h.PartialAsync("test", expectedModel, expectedViewData))
.Returns(Task.FromResult(htmlContent))
.Verifiable();
// Act
var result = partialMethod(helper.Object);
// Assert
Assert.Same(htmlContent, result);
helper.VerifyAll();
}
[Theory]
[MemberData(nameof(PartialExtensionMethods))]
public void PartialMethods_DoesNotWrapThrownException(
Func<IHtmlHelper, IHtmlContent> partialMethod,
object unusedModel,
ViewDataDictionary unusedViewData)
{
// Arrange
var expected = new InvalidOperationException();
@ -54,6 +92,103 @@ namespace Microsoft.AspNet.Mvc.Rendering
Assert.Same(expected, actual);
}
// Func<IHtmlHelper, IHtmlContent>, expected Model, expected ViewDataDictionary
public static TheoryData<Func<IHtmlHelper, Task<IHtmlContent>>, object, ViewDataDictionary> PartialAsyncExtensionMethods
{
get
{
var viewData = new ViewDataDictionary(new EmptyModelMetadataProvider());
var model = new object();
return new TheoryData<Func<IHtmlHelper, Task<IHtmlContent>>, object, ViewDataDictionary>
{
{ helper => helper.PartialAsync("test"), null, null },
{ helper => helper.PartialAsync("test", model), model, null },
{ helper => helper.PartialAsync("test", viewData), null, viewData },
};
}
}
[Theory]
[MemberData(nameof(PartialAsyncExtensionMethods))]
public async Task PartialAsyncMethods_CallHtmlHelperWithExpectedArguments(
Func<IHtmlHelper, Task<IHtmlContent>> partialAsyncMethod,
object expectedModel,
ViewDataDictionary expectedViewData)
{
// Arrange
var htmlContent = Mock.Of<IHtmlContent>();
var helper = new Mock<IHtmlHelper>(MockBehavior.Strict);
if (expectedModel == null)
{
// Extension methods without model parameter use ViewData.Model to get Model.
var viewData = expectedViewData ?? new ViewDataDictionary(new EmptyModelMetadataProvider());
helper
.SetupGet(h => h.ViewData)
.Returns(viewData)
.Verifiable();
}
helper
.Setup(h => h.PartialAsync("test", expectedModel, expectedViewData))
.Returns(Task.FromResult(htmlContent))
.Verifiable();
// Act
var result = await partialAsyncMethod(helper.Object);
// Assert
Assert.Same(htmlContent, result);
helper.VerifyAll();
}
// Func<IHtmlHelper, IHtmlContent>, expected Model, expected ViewDataDictionary
public static TheoryData<Func<IHtmlHelper, Task>, object, ViewDataDictionary> RenderPartialAsyncExtensionMethods
{
get
{
var viewData = new ViewDataDictionary(new EmptyModelMetadataProvider());
var model = new object();
return new TheoryData<Func<IHtmlHelper, Task>, object, ViewDataDictionary>
{
{ helper => helper.RenderPartialAsync("test"), null, null },
{ helper => helper.RenderPartialAsync("test", model), model, null },
{ helper => helper.RenderPartialAsync("test", viewData), null, viewData },
};
}
}
[Theory]
[MemberData(nameof(RenderPartialAsyncExtensionMethods))]
public async Task RenderPartialAsyncMethods_CallHtmlHelperWithExpectedArguments(
Func<IHtmlHelper, Task> renderPartialAsyncMethod,
object expectedModel,
ViewDataDictionary expectedViewData)
{
// Arrange
var htmlContent = Mock.Of<IHtmlContent>();
var helper = new Mock<IHtmlHelper>(MockBehavior.Strict);
if (expectedModel == null)
{
// Extension methods without model parameter use ViewData.Model to get Model.
var viewData = expectedViewData ?? new ViewDataDictionary(new EmptyModelMetadataProvider());
helper
.SetupGet(h => h.ViewData)
.Returns(viewData)
.Verifiable();
}
helper
.Setup(h => h.RenderPartialAsync("test", expectedModel, expectedViewData))
.Returns(Task.FromResult(true))
.Verifiable();
// Act
await renderPartialAsyncMethod(helper.Object);
// Assert
helper.VerifyAll();
}
[Fact]
public void Partial_InvokesPartialAsyncWithCurrentModel()
{