RazorViewEngine should cache transformed paths when view cannot be found

Fixes #3034

src/Microsoft.AspNet.Mvc.Razor/DefaultViewLocationCache.cs

@@ -5,7 +5,6 @@ using System;
 using System.Collections.Concurrent;
 using System.Linq;
 using System.Text;
-using Microsoft.AspNet.Mvc.Razor.Internal;
 using Microsoft.Framework.Internal;
 
 namespace Microsoft.AspNet.Mvc.Razor
@@ -18,28 +17,33 @@ namespace Microsoft.AspNet.Mvc.Razor
         private const char CacheKeySeparator = ':';
 
         // A mapping of keys generated from ViewLocationExpanderContext to view locations.
-        private readonly ConcurrentDictionary<string, string> _cache;
+        private readonly ConcurrentDictionary<string, ViewLocationCacheResult> _cache;
 
         /// <summary>
         /// Initializes a new instance of <see cref="DefaultViewLocationCache"/>.
         /// </summary>
         public DefaultViewLocationCache()
         {
-            _cache = new ConcurrentDictionary<string, string>(StringComparer.Ordinal);
+            _cache = new ConcurrentDictionary<string, ViewLocationCacheResult>(StringComparer.Ordinal);
         }
 
         /// <inheritdoc />
-        public string Get([NotNull] ViewLocationExpanderContext context)
+        public ViewLocationCacheResult Get([NotNull] ViewLocationExpanderContext context)
         {
             var cacheKey = GenerateKey(context);
-            string result;
+            ViewLocationCacheResult result;
-            _cache.TryGetValue(cacheKey, out result);
+            if (_cache.TryGetValue(cacheKey, out result))
-            return result;
+            {
+                return result;
+            }
+
+            return ViewLocationCacheResult.None;
         }
 
         /// <inheritdoc />
-        public void Set([NotNull] ViewLocationExpanderContext context,
+        public void Set(
-                        [NotNull] string value)
+            [NotNull] ViewLocationExpanderContext context,
+            [NotNull] ViewLocationCacheResult value)
         {
             var cacheKey = GenerateKey(context);
             _cache.TryAdd(cacheKey, value);

src/Microsoft.AspNet.Mvc.Razor/IViewLocationCache.cs

@@ -14,7 +14,7 @@ namespace Microsoft.AspNet.Mvc.Razor
         /// <param name="context">The <see cref="ViewLocationExpanderContext"/> for the current view location
         /// expansion.</param>
         /// <returns>The cached location, if available, <c>null</c> otherwise.</returns>
-        string Get(ViewLocationExpanderContext context);
+        ViewLocationCacheResult Get(ViewLocationExpanderContext context);
 
         /// <summary>
         /// Adds a cache entry for values specified by <paramref name="context"/>.
@@ -22,6 +22,6 @@ namespace Microsoft.AspNet.Mvc.Razor
         /// <param name="context">The <see cref="ViewLocationExpanderContext"/> for the current view location
         /// expansion.</param>
         /// <param name="value">The view location that is to be cached.</param>
-        void Set(ViewLocationExpanderContext context, string value);
+        void Set(ViewLocationExpanderContext context, ViewLocationCacheResult value);
     }
 }

src/Microsoft.AspNet.Mvc.Razor/RazorViewEngine.cs

@@ -216,9 +216,10 @@ namespace Microsoft.AspNet.Mvc.Razor
             }
         }
 
-        private RazorPageResult LocatePageFromViewLocations(ActionContext context,
+        private RazorPageResult LocatePageFromViewLocations(
-                                                            string pageName,
+            ActionContext context,
-                                                            bool isPartial)
+            string pageName,
+            bool isPartial)
         {
             // Initialize the dictionary for the typical case of having controller and action tokens.
             var areaName = GetNormalizedRouteValue(context, AreaKey);
@@ -240,47 +241,64 @@ namespace Microsoft.AspNet.Mvc.Razor
             }
 
             // 2. With the values that we've accumumlated so far, check if we have a cached result.
-            var pageLocation = _viewLocationCache.Get(expanderContext);
+            IEnumerable<string> locationsToSearch = null;
-            if (!string.IsNullOrEmpty(pageLocation))
+            var cachedResult = _viewLocationCache.Get(expanderContext);
+            if (!cachedResult.Equals(ViewLocationCacheResult.None))
             {
-                var page = _pageFactory.CreateInstance(pageLocation);
+                if (cachedResult.IsFoundResult)
-
-                if (page != null)
                 {
-                    // 2a. We found a IRazorPage at the cached location.
+                    var page = _pageFactory.CreateInstance(cachedResult.ViewLocation);
-                    return new RazorPageResult(pageName, page);
+
+                    if (page != null)
+                    {
+                        // 2a We have a cache entry where a view was previously found.
+                        return new RazorPageResult(pageName, page);
+                    }
+                }
+                else
+                {
+                    locationsToSearch = cachedResult.SearchedLocations;
                 }
             }
 
-            // 2b. We did not find a cached location or did not find a IRazorPage at the cached location.
+            if (locationsToSearch == null)
-            // The cached value has expired and we need to look up the page.
-            foreach (var expander in _viewLocationExpanders)
             {
-                viewLocations = expander.ExpandViewLocations(expanderContext, viewLocations);
+                // 2b. We did not find a cached location or did not find a IRazorPage at the cached location.
+                // The cached value has expired and we need to look up the page.
+                foreach (var expander in _viewLocationExpanders)
+                {
+                    viewLocations = expander.ExpandViewLocations(expanderContext, viewLocations);
+                }
+
+                var controllerName = GetNormalizedRouteValue(context, ControllerKey);
+
+                locationsToSearch = viewLocations.Select(
+                    location => string.Format(
+                        CultureInfo.InvariantCulture,
+                        location,
+                        pageName,
+                        controllerName,
+                        areaName
+                    ));
             }
 
             // 3. Use the expanded locations to look up a page.
-            var controllerName = GetNormalizedRouteValue(context, ControllerKey);
             var searchedLocations = new List<string>();
-            foreach (var path in viewLocations)
+            foreach (var path in locationsToSearch)
             {
-                var transformedPath = string.Format(CultureInfo.InvariantCulture,
+                var page = _pageFactory.CreateInstance(path);
-                                                    path,
-                                                    pageName,
-                                                    controllerName,
-                                                    areaName);
-                var page = _pageFactory.CreateInstance(transformedPath);
                 if (page != null)
                 {
                     // 3a. We found a page. Cache the set of values that produced it and return a found result.
-                    _viewLocationCache.Set(expanderContext, transformedPath);
+                    _viewLocationCache.Set(expanderContext, new ViewLocationCacheResult(path, searchedLocations));
                     return new RazorPageResult(pageName, page);
                 }
 
-                searchedLocations.Add(transformedPath);
+                searchedLocations.Add(path);
             }
 
             // 3b. We did not find a page for any of the paths.
+            _viewLocationCache.Set(expanderContext, new ViewLocationCacheResult(searchedLocations));
             return new RazorPageResult(pageName, searchedLocations);
         }
 

src/Microsoft.AspNet.Mvc.Razor/ViewLocationCacheResult.cs

@@ -0,0 +1,121 @@
+// 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.Framework.Internal;
+
+namespace Microsoft.AspNet.Mvc.Razor
+{
+    /// <summary>
+    /// Result of <see cref="IViewLocationCache"/> lookups.
+    /// </summary>
+    public struct ViewLocationCacheResult : IEquatable<ViewLocationCacheResult>
+    {
+        /// <summary>
+        /// Initializes a new instance of <see cref="ViewLocationCacheResult"/>
+        /// for a view that was successfully found at the specified location.
+        /// </summary>
+        /// <param name="foundLocation">The view location.</param>
+        /// <param name="searchedLocations">Locations that were searched
+        /// in addition to <paramref name="foundLocation"/>.</param>
+        public ViewLocationCacheResult(
+            [NotNull] string foundLocation,
+            [NotNull] IEnumerable<string> searchedLocations)
+            : this (searchedLocations)
+        {
+            ViewLocation = foundLocation;
+            SearchedLocations = searchedLocations;
+            IsFoundResult = true;
+        }
+
+        /// <summary>
+        /// Initializes a new instance of <see cref="ViewLocationCacheResult"/> for a
+        /// failed view lookup.
+        /// </summary>
+        /// <param name="searchedLocations">Locations that were searched.</param>
+        public ViewLocationCacheResult([NotNull] IEnumerable<string> searchedLocations)
+        {
+            SearchedLocations = searchedLocations;
+            ViewLocation = null;
+            IsFoundResult = false;
+        }
+
+        /// <summary>
+        /// A <see cref="ViewLocationCacheResult"/> that represents a cache miss.
+        /// </summary>
+        public static readonly ViewLocationCacheResult None = new ViewLocationCacheResult(Enumerable.Empty<string>());
+
+        /// <summary>
+        /// The location the view was found.
+        /// </summary>
+        /// <remarks>This is available if <see cref="IsFoundResult"/> is <c>true</c>.</remarks>
+        public string ViewLocation { get; }
+
+        /// <summary>
+        /// The sequence of locations that were searched.
+        /// </summary>
+        /// <remarks>
+        /// When <see cref="IsFoundResult"/> is <c>true</c> this includes all paths that were search prior to finding
+        /// a view at <see cref="ViewLocation"/>. When <see cref="IsFoundResult"/> is <c>false</c>, this includes
+        /// all search paths.
+        /// </remarks>
+        public IEnumerable<string> SearchedLocations { get; }
+
+        /// <summary>
+        /// Gets a value that indicates whether the view was successfully found.
+        /// </summary>
+        public bool IsFoundResult { get; }
+
+        /// <inheritdoc />
+        public bool Equals(ViewLocationCacheResult other)
+        {
+            if (IsFoundResult != other.IsFoundResult)
+            {
+                return false;
+            }
+
+            if (IsFoundResult)
+            {
+                return string.Equals(ViewLocation, other.ViewLocation, StringComparison.Ordinal);
+            }
+            else
+            {
+                if (SearchedLocations == other.SearchedLocations)
+                {
+                    return true;
+                }
+
+                if (SearchedLocations == null || other.SearchedLocations == null)
+                {
+                    return false;
+                }
+
+                return Enumerable.SequenceEqual(SearchedLocations, other.SearchedLocations, StringComparer.Ordinal);
+            }
+        }
+
+        /// <inheritdoc />
+        public override int GetHashCode()
+        {
+            var hashCodeCombiner = HashCodeCombiner.Start()
+                .Add(IsFoundResult);
+
+
+            if (IsFoundResult)
+            {
+                hashCodeCombiner.Add(ViewLocation, StringComparer.Ordinal);
+            }
+            else if (SearchedLocations != null)
+            {
+                foreach (var location in SearchedLocations)
+                {
+                    hashCodeCombiner.Add(location, StringComparer.Ordinal);
+                }
+            }
+
+            return hashCodeCombiner;
+        }
+    }
+}

test/Microsoft.AspNet.Mvc.Razor.Test/DefaultViewLocationCacheTest.cs

@@ -53,7 +53,7 @@ namespace Microsoft.AspNet.Mvc.Razor
             var result = cache.Get(context);
 
             // Assert
-            Assert.Null(result);
+            Assert.Equal(result, ViewLocationCacheResult.None);
         }
 
         [Theory]
@@ -62,13 +62,25 @@ namespace Microsoft.AspNet.Mvc.Razor
         {
             // Arrange
             var cache = new DefaultViewLocationCache();
-            var value = Guid.NewGuid().ToString();
+            var value = new ViewLocationCacheResult(
+                Guid.NewGuid().ToString(),
+                new[]
+                {
+                    Guid.NewGuid().ToString(),
+                    Guid.NewGuid().ToString()
+                });
 
-            // Act
+            // Act - 1
             cache.Set(context, value);
             var result = cache.Get(context);
 
-            // Assert
+            // Assert - 1
+            Assert.Equal(value, result);
+
+            // Act - 2
+            result = cache.Get(context);
+
+            // Assert - 2
             Assert.Equal(value, result);
         }
 

test/Microsoft.AspNet.Mvc.Razor.Test/RazorViewEngineTest.cs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System.Collections.Generic;
+using System.Linq;
 using Microsoft.AspNet.Http.Internal;
 using Microsoft.AspNet.Mvc.Rendering;
 using Microsoft.AspNet.Mvc.Routing;
@@ -409,9 +410,13 @@ namespace Microsoft.AspNet.Mvc.Razor.Test
 
             var cache = GetViewLocationCache();
             var cacheMock = Mock.Get(cache);
-
+            cacheMock.Setup(c => c.Set(It.IsAny<ViewLocationExpanderContext>(), It.IsAny<ViewLocationCacheResult>()))
-            cacheMock.Setup(c => c.Set(It.IsAny<ViewLocationExpanderContext>(), "/Views/Shared/baz.cshtml"))
+                .Callback((ViewLocationExpanderContext _, ViewLocationCacheResult cacheResult) =>
-                     .Verifiable();
+                {
+                    Assert.Equal("/Views/Shared/baz.cshtml", cacheResult.ViewLocation);
+                    Assert.Equal(new[] { "/Views/bar/baz.cshtml" }, cacheResult.SearchedLocations);
+                })
+                .Verifiable();
 
             var viewEngine = CreateViewEngine(pageFactory.Object, viewFactory.Object, cache: cache);
             var context = GetActionContext(_controllerTestContext);
@@ -443,8 +448,8 @@ namespace Microsoft.AspNet.Mvc.Razor.Test
                     .Verifiable();
             var cacheMock = new Mock<IViewLocationCache>();
             cacheMock.Setup(c => c.Get(It.IsAny<ViewLocationExpanderContext>()))
-                     .Returns("some-view-location")
+                .Returns(new ViewLocationCacheResult("some-view-location", Enumerable.Empty<string>()))
-                     .Verifiable();
+                .Verifiable();
 
             var viewEngine = CreateViewEngine(pageFactory.Object,
                                               viewFactory.Object,
@@ -480,7 +485,7 @@ namespace Microsoft.AspNet.Mvc.Razor.Test
 
             var cacheMock = new Mock<IViewLocationCache>();
             cacheMock.Setup(c => c.Get(It.IsAny<ViewLocationExpanderContext>()))
-                     .Returns("expired-location");
+                .Returns(new ViewLocationCacheResult("expired-location", Enumerable.Empty<string>()));
 
             var expander = new Mock<IViewLocationExpander>();
             expander.Setup(v => v.PopulateValues(It.IsAny<ViewLocationExpanderContext>()))
@@ -507,6 +512,65 @@ namespace Microsoft.AspNet.Mvc.Razor.Test
             expander.Verify();
         }
 
+        // This test validates an important perf scenario of RazorViewEngine not constructing
+        // multiple strings for views that do not exist in the file system on a per-request basis.
+        [Fact]
+        public void FindView_DoesNotInvokeExpandViewLocations_IfCacheEntryMatchesButViewIsNotFound()
+        {
+            // Arrange
+            var pageFactory = Mock.Of<IRazorPageFactory>();
+            var viewFactory = Mock.Of<IRazorViewFactory>();
+            var cache = new DefaultViewLocationCache();
+            var expander = new Mock<IViewLocationExpander>();
+            var expandedLocations = new[]
+            {
+                "viewlocation1",
+                "viewlocation2",
+                "viewlocation3",
+            };
+            expander
+                .Setup(v => v.PopulateValues(It.IsAny<ViewLocationExpanderContext>()))
+                .Callback((ViewLocationExpanderContext expanderContext) =>
+                {
+                    expanderContext.Values["somekey"] = "somevalue";
+                })
+                .Verifiable();
+            expander
+                .Setup(v => v.ExpandViewLocations(
+                    It.IsAny<ViewLocationExpanderContext>(), It.IsAny<IEnumerable<string>>()))
+                .Returns((ViewLocationExpanderContext c, IEnumerable<string> viewLocations) => expandedLocations)
+                .Verifiable();
+
+
+            var viewEngine = CreateViewEngine(
+                pageFactory,
+                viewFactory,
+                expanders: new[] { expander.Object },
+                cache: cache);
+            var context = GetActionContext(_controllerTestContext);
+
+            // Act - 1
+            var result = viewEngine.FindView(context, "myview");
+
+            // Assert - 1
+            Assert.False(result.Success);
+            Assert.Equal(expandedLocations, result.SearchedLocations);
+            expander.Verify();
+
+            // Act - 2
+            result = viewEngine.FindView(context, "myview");
+
+            // Assert - 2
+            Assert.False(result.Success);
+            Assert.Equal(expandedLocations, result.SearchedLocations);
+            expander.Verify(
+                v => v.PopulateValues(It.IsAny<ViewLocationExpanderContext>()),
+                Times.Exactly(2));
+            expander.Verify(
+                v => v.ExpandViewLocations(It.IsAny<ViewLocationExpanderContext>(), It.IsAny<IEnumerable<string>>()),
+                Times.Once());
+        }
+
         [Theory]
         [InlineData(null)]
         [InlineData("")]
@@ -1045,7 +1109,7 @@ namespace Microsoft.AspNet.Mvc.Razor.Test
                     options.ViewLocationExpanders.Add(expander);
                 }
             }
-            
+
             var optionsAccessor = new Mock<IOptions<RazorViewEngineOptions>>();
             optionsAccessor.SetupGet(v => v.Options)
                 .Returns(options);