From 43226fe54de140db30746336f3d46130be47c0e4 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Mon, 21 Dec 2015 16:22:21 -0800 Subject: [PATCH] Modify FileVersionProvider to cache missing file info. Fixes #3765 --- .../Internal/FileVersionProvider.cs | 53 ++-- .../ImageTagHelperTest.cs | 16 +- .../Internal/FileVersionProviderTest.cs | 256 +++++++++++++----- .../LinkTagHelperTest.cs | 17 +- .../ScriptTagHelperTest.cs | 19 +- .../TestFileChangeToken.cs | 9 +- 6 files changed, 232 insertions(+), 138 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.TagHelpers/Internal/FileVersionProvider.cs b/src/Microsoft.AspNet.Mvc.TagHelpers/Internal/FileVersionProvider.cs index 6d5a4a989d..dd8678f344 100644 --- a/src/Microsoft.AspNet.Mvc.TagHelpers/Internal/FileVersionProvider.cs +++ b/src/Microsoft.AspNet.Mvc.TagHelpers/Internal/FileVersionProvider.cs @@ -16,6 +16,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal public class FileVersionProvider { private const string VersionKey = "v"; + private static readonly char[] QueryStringAndFragmentTokens = new [] { '?', '#' }; private readonly IFileProvider _fileProvider; private readonly IMemoryCache _cache; private readonly PathString _requestPathBase; @@ -24,8 +25,8 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal /// Creates a new instance of . /// /// The file provider to get and watch files. - /// Name of the application. /// where versioned urls of files are cached. + /// The base path for the current HTTP request. public FileVersionProvider( IFileProvider fileProvider, IMemoryCache cache, @@ -52,7 +53,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal /// The path of the file to which version should be added. /// Path containing the version query string. /// - /// The version query string is appended as with the key "v". + /// The version query string is appended with the key "v". /// public string AddFileVersionToPath(string path) { @@ -63,7 +64,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal var resolvedPath = path; - var queryStringOrFragmentStartIndex = path.IndexOfAny(new char[] { '?', '#' }); + var queryStringOrFragmentStartIndex = path.IndexOfAny(QueryStringAndFragmentTokens); if (queryStringOrFragmentStartIndex != -1) { resolvedPath = path.Substring(0, queryStringOrFragmentStartIndex); @@ -76,35 +77,39 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal return path; } - var fileInfo = _fileProvider.GetFileInfo(resolvedPath); - if (!fileInfo.Exists) - { - if (_requestPathBase.HasValue && - resolvedPath.StartsWith(_requestPathBase.Value, StringComparison.OrdinalIgnoreCase)) - { - resolvedPath = resolvedPath.Substring(_requestPathBase.Value.Length); - fileInfo = _fileProvider.GetFileInfo(resolvedPath); - } - - if (!fileInfo.Exists) - { - // if the file is not in the current server. - return path; - } - } - string value; if (!_cache.TryGetValue(path, out value)) { - value = QueryHelpers.AddQueryString(path, VersionKey, GetHashForFile(fileInfo)); - var cacheEntryOptions = new MemoryCacheEntryOptions().AddExpirationToken(_fileProvider.Watch(resolvedPath)); - _cache.Set(path, value, cacheEntryOptions); + var cacheEntryOptions = new MemoryCacheEntryOptions(); + cacheEntryOptions.AddExpirationToken(_fileProvider.Watch(resolvedPath)); + var fileInfo = _fileProvider.GetFileInfo(resolvedPath); + + if (!fileInfo.Exists && + _requestPathBase.HasValue && + resolvedPath.StartsWith(_requestPathBase.Value, StringComparison.OrdinalIgnoreCase)) + { + var requestPathBaseRelativePath = resolvedPath.Substring(_requestPathBase.Value.Length); + cacheEntryOptions.AddExpirationToken(_fileProvider.Watch(requestPathBaseRelativePath)); + fileInfo = _fileProvider.GetFileInfo(requestPathBaseRelativePath); + } + + if (fileInfo.Exists) + { + value = QueryHelpers.AddQueryString(path, VersionKey, GetHashForFile(fileInfo)); + } + else + { + // if the file is not in the current server. + value = path; + } + + value = _cache.Set(path, value, cacheEntryOptions); } return value; } - private string GetHashForFile(IFileInfo fileInfo) + private static string GetHashForFile(IFileInfo fileInfo) { using (var sha256 = SHA256.Create()) { diff --git a/test/Microsoft.AspNet.Mvc.TagHelpers.Test/ImageTagHelperTest.cs b/test/Microsoft.AspNet.Mvc.TagHelpers.Test/ImageTagHelperTest.cs index b022fffe45..eaec7657f9 100644 --- a/test/Microsoft.AspNet.Mvc.TagHelpers.Test/ImageTagHelperTest.cs +++ b/test/Microsoft.AspNet.Mvc.TagHelpers.Test/ImageTagHelperTest.cs @@ -325,21 +325,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers return hostingEnvironment.Object; } - private static IMemoryCache MakeCache() - { - object result = null; - var cache = new Mock(); - cache.CallBase = true; - cache.Setup(c => c.TryGetValue(It.IsAny(), out result)) - .Returns(result != null); - cache.Setup( - c => c.Set( - /*key*/ It.IsAny(), - /*value*/ It.IsAny(), - /*options*/ It.IsAny())) - .Returns(new object()); - return cache.Object; - } + private static IMemoryCache MakeCache() => new MemoryCache(new MemoryCacheOptions()); private static IUrlHelperFactory MakeUrlHelperFactory() { diff --git a/test/Microsoft.AspNet.Mvc.TagHelpers.Test/Internal/FileVersionProviderTest.cs b/test/Microsoft.AspNet.Mvc.TagHelpers.Test/Internal/FileVersionProviderTest.cs index 998f55c258..429ef194c0 100644 --- a/test/Microsoft.AspNet.Mvc.TagHelpers.Test/Internal/FileVersionProviderTest.cs +++ b/test/Microsoft.AspNet.Mvc.TagHelpers.Test/Internal/FileVersionProviderTest.cs @@ -1,12 +1,11 @@ // 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.IO; using System.Text; using Microsoft.AspNet.FileProviders; -using Microsoft.AspNet.Hosting; using Microsoft.AspNet.Http; +using Microsoft.AspNet.Mvc.Razor; using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Primitives; using Moq; @@ -22,13 +21,13 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal [InlineData("/hello/world?q=foo&bar", "/hello/world?q=foo&bar&v=f4OxZX_x_FO5LcGBSKHWXfwtSx-j1ncoSt3SABJtkGk")] [InlineData("/hello/world?q=foo&bar#abc", "/hello/world?q=foo&bar&v=f4OxZX_x_FO5LcGBSKHWXfwtSx-j1ncoSt3SABJtkGk#abc")] [InlineData("/hello/world#somefragment", "/hello/world?v=f4OxZX_x_FO5LcGBSKHWXfwtSx-j1ncoSt3SABJtkGk#somefragment")] - public void AddsVersionToFiles_WhenCacheIsAbsent(string filePath, string expected) + public void AddFileVersionToPath_WhenCacheIsAbsent(string filePath, string expected) { // Arrange - var hostingEnvironment = GetMockHostingEnvironment(filePath); + var fileProvider = GetMockFileProvider(filePath); var fileVersionProvider = new FileVersionProvider( - hostingEnvironment.WebRootFileProvider, - GetMockCache(), + fileProvider, + new MemoryCache(new MemoryCacheOptions()), GetRequestPathBase()); // Act @@ -38,54 +37,188 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal Assert.Equal(expected, result); } - // Verifies if the stream is closed after reading. [Fact] - public void AddsVersionToFiles_DoesNotLockFileAfterReading() + public void AddFileVersionToPath_CachesNotFoundResults() { // Arrange - var stream = new MemoryStream(Encoding.UTF8.GetBytes("Hello World!")); + var path = "/wwwroot/file.txt"; + var fileProvider = GetMockFileProvider( + path, + pathStartsWithAppName: false, + fileDoesNotExist: true); + var fileVersionProvider = new FileVersionProvider( + fileProvider, + new MemoryCache(new MemoryCacheOptions()), + GetRequestPathBase()); + var mockFileProvider = Mock.Get(fileProvider); + + // Act 1 + var result = fileVersionProvider.AddFileVersionToPath(path); + + // Assert 1 + Assert.Equal(path, result); + mockFileProvider.Verify(f => f.GetFileInfo(It.IsAny()), Times.Once()); + mockFileProvider.Verify(f => f.Watch(It.IsAny()), Times.Once()); + + // Act 2 + result = fileVersionProvider.AddFileVersionToPath(path); + + // Assert 2 + Assert.Equal(path, result); + mockFileProvider.Verify(f => f.GetFileInfo(It.IsAny()), Times.Once()); + mockFileProvider.Verify(f => f.Watch(It.IsAny()), Times.Once()); + } + + [Theory] + [InlineData("file.txt", false)] + [InlineData("/wwwroot/file.txt", true)] + public void AddFileVersionToPath_CachesFoundResults(string path, bool pathStartsWithAppName) + { + // Arrange + var fileProvider = GetMockFileProvider( + "file.txt", + pathStartsWithAppName); + var fileVersionProvider = new FileVersionProvider( + fileProvider, + new MemoryCache(new MemoryCacheOptions()), + GetRequestPathBase()); + var mockFileProvider = Mock.Get(fileProvider); + + // Act 1 + var result = fileVersionProvider.AddFileVersionToPath(path); + + // Assert 1 + Assert.Equal($"{path}?v=f4OxZX_x_FO5LcGBSKHWXfwtSx-j1ncoSt3SABJtkGk", result); + mockFileProvider.Verify(f => f.GetFileInfo(It.IsAny()), Times.Once()); + mockFileProvider.Verify(f => f.Watch(It.IsAny()), Times.Once()); + + // Act 2 + result = fileVersionProvider.AddFileVersionToPath(path); + + // Assert 2 + Assert.Equal($"{path}?v=f4OxZX_x_FO5LcGBSKHWXfwtSx-j1ncoSt3SABJtkGk", result); + mockFileProvider.Verify(f => f.GetFileInfo(It.IsAny()), Times.Once()); + mockFileProvider.Verify(f => f.Watch(It.IsAny()), Times.Once()); + } + + [Fact] + public void AddFileVersionToPath_UpdatesEntryWhenCacheExpires_ForNonExistingFile() + { + // Arrange + var fileProvider = new TestFileProvider(); + var fileVersionProvider = new FileVersionProvider( + fileProvider, + new MemoryCache(new MemoryCacheOptions()), + GetRequestPathBase()); + + // Act 1 - File does not exist + var result = fileVersionProvider.AddFileVersionToPath("file.txt"); + + // Assert 1 + Assert.Equal("file.txt", result); + + // Act 2 - File gets added + fileProvider.AddFile("file.txt", "Hello World!"); + fileProvider.GetChangeToken("file.txt").HasChanged = true; + result = fileVersionProvider.AddFileVersionToPath("file.txt"); + + // Assert 2 + Assert.Equal("file.txt?v=f4OxZX_x_FO5LcGBSKHWXfwtSx-j1ncoSt3SABJtkGk", result); + } + + [Fact] + public void AddFileVersionToPath_UpdatesEntryWhenCacheExpires_ForExistingFile() + { + // Arrange + var fileProvider = new TestFileProvider(); + var fileVersionProvider = new FileVersionProvider( + fileProvider, + new MemoryCache(new MemoryCacheOptions()), + GetRequestPathBase()); + fileProvider.AddFile("file.txt", "Hello World!"); + + // Act 1 - File exists + var result = fileVersionProvider.AddFileVersionToPath("file.txt"); + + // Assert 1 + Assert.Equal("file.txt?v=f4OxZX_x_FO5LcGBSKHWXfwtSx-j1ncoSt3SABJtkGk", result); + + // Act 2 + fileProvider.DeleteFile("file.txt"); + fileProvider.GetChangeToken("file.txt").HasChanged = true; + result = fileVersionProvider.AddFileVersionToPath("file.txt"); + + // Assert 2 + Assert.Equal("file.txt", result); + } + + [Fact] + public void AddFileVersionToPath_UpdatesEntryWhenCacheExpires_ForExistingFile_WithRequestPathBase() + { + // Arrange + var fileProvider = new TestFileProvider(); + var fileVersionProvider = new FileVersionProvider( + fileProvider, + new MemoryCache(new MemoryCacheOptions()), + GetRequestPathBase("/wwwroot/")); + fileProvider.AddFile("file.txt", "Hello World!"); + + // Act 1 - File exists + var result = fileVersionProvider.AddFileVersionToPath("/wwwroot/file.txt"); + + // Assert 1 + Assert.Equal("/wwwroot/file.txt?v=f4OxZX_x_FO5LcGBSKHWXfwtSx-j1ncoSt3SABJtkGk", result); + + // Act 2 + fileProvider.DeleteFile("file.txt"); + fileProvider.GetChangeToken("file.txt").HasChanged = true; + result = fileVersionProvider.AddFileVersionToPath("/wwwroot/file.txt"); + + // Assert 2 + Assert.Equal("/wwwroot/file.txt", result); + } + + // Verifies if the stream is closed after reading. + [Fact] + public void AddFileVersionToPath_DoesNotLockFileAfterReading() + { + // Arrange + var stream = new TestableMemoryStream(Encoding.UTF8.GetBytes("Hello World!")); var mockFile = new Mock(); mockFile.SetupGet(f => f.Exists).Returns(true); mockFile .Setup(m => m.CreateReadStream()) .Returns(stream); - var mockFileProvider = new Mock(); - mockFileProvider.Setup(fp => fp.GetFileInfo(It.IsAny())) - .Returns(mockFile.Object); - mockFileProvider.Setup(fp => fp.Watch(It.IsAny())) - .Returns(new TestFileChangeToken()); - - var hostingEnvironment = new Mock(); - hostingEnvironment.Setup(h => h.WebRootFileProvider).Returns(mockFileProvider.Object); + var fileProvider = new TestFileProvider(); + fileProvider.AddFile("/hello/world", mockFile.Object); var fileVersionProvider = new FileVersionProvider( - hostingEnvironment.Object.WebRootFileProvider, - GetMockCache(), + fileProvider, + new MemoryCache(new MemoryCacheOptions()), GetRequestPathBase()); // Act var result = fileVersionProvider.AddFileVersionToPath("/hello/world"); // Assert - Assert.False(stream.CanRead); - Assert.Throws(() => fileVersionProvider.AddFileVersionToPath("/hello/world")); + Assert.True(stream.Disposed); } [Theory] [InlineData("/testApp/hello/world", true, "/testApp")] [InlineData("/testApp/foo/bar/hello/world", true, "/testApp/foo/bar")] [InlineData("/test/testApp/hello/world", false, "/testApp")] - public void AddsVersionToFiles_PathContainingAppName( + public void AddFileVersionToPath_PathContainingAppName( string filePath, bool pathStartsWithAppBase, string requestPathBase) { // Arrange - var hostingEnvironment = GetMockHostingEnvironment(filePath, pathStartsWithAppBase); + var fileProvider = GetMockFileProvider(filePath, pathStartsWithAppBase); var fileVersionProvider = new FileVersionProvider( - hostingEnvironment.WebRootFileProvider, - GetMockCache(), + fileProvider, + new MemoryCache(new MemoryCacheOptions()), GetRequestPathBase(requestPathBase)); // Act @@ -100,10 +233,10 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal { // Arrange var filePath = "http://contoso.com/hello/world"; - var hostingEnvironment = GetMockHostingEnvironment(filePath, false, true); + var fileProvider = GetMockFileProvider(filePath, false, true); var fileVersionProvider = new FileVersionProvider( - hostingEnvironment.WebRootFileProvider, - GetMockCache(), + fileProvider, + new MemoryCache(new MemoryCacheOptions()), GetRequestPathBase()); // Act @@ -118,10 +251,12 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal { // Arrange var filePath = "/hello/world"; - var hostingEnvironment = GetMockHostingEnvironment(filePath); + var fileProvider = GetMockFileProvider(filePath); + var memoryCache = new MemoryCache(new MemoryCacheOptions()); + memoryCache.Set(filePath, "FromCache"); var fileVersionProvider = new FileVersionProvider( - hostingEnvironment.WebRootFileProvider, - GetMockCache("FromCache"), + fileProvider, + memoryCache, GetRequestPathBase()); // Act @@ -138,8 +273,8 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal { // Arrange var changeToken = new Mock(); - var hostingEnvironment = GetMockHostingEnvironment(filePath, requestPathBase != null); - Mock.Get(hostingEnvironment.WebRootFileProvider) + var fileProvider = GetMockFileProvider(filePath, requestPathBase != null); + Mock.Get(fileProvider) .Setup(f => f.Watch(watchPath)).Returns(changeToken.Object); object cacheValue = null; @@ -151,10 +286,10 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal /*key*/ filePath, /*value*/ It.IsAny(), /*options*/ It.IsAny())) - .Returns(new object()) + .Returns((object key, object value, MemoryCacheEntryOptions options) => value) .Verifiable(); var fileVersionProvider = new FileVersionProvider( - hostingEnvironment.WebRootFileProvider, + fileProvider, cache.Object, GetRequestPathBase(requestPathBase)); @@ -166,7 +301,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal cache.VerifyAll(); } - private IHostingEnvironment GetMockHostingEnvironment( + private static IFileProvider GetMockFileProvider( string filePath, bool pathStartsWithAppName = false, bool fileDoesNotExist = false) @@ -177,54 +312,47 @@ namespace Microsoft.AspNet.Mvc.TagHelpers.Internal .Setup(m => m.CreateReadStream()) .Returns(() => new MemoryStream(Encoding.UTF8.GetBytes("Hello World!"))); - var nonExistingMockFile = new Mock(); - nonExistingMockFile.SetupGet(f => f.Exists).Returns(false); - nonExistingMockFile - .Setup(m => m.CreateReadStream()) - .Returns(() => new MemoryStream(Encoding.UTF8.GetBytes("Hello World!"))); + var doesNotExistMockFile = new Mock(); + doesNotExistMockFile.SetupGet(f => f.Exists).Returns(false); var mockFileProvider = new Mock(); if (pathStartsWithAppName) { - mockFileProvider.Setup(fp => fp.GetFileInfo(filePath)).Returns(nonExistingMockFile.Object); + mockFileProvider.Setup(fp => fp.GetFileInfo(filePath)).Returns(doesNotExistMockFile.Object); mockFileProvider.Setup(fp => fp.GetFileInfo(It.Is(str => str != filePath))) .Returns(existingMockFile.Object); } else { mockFileProvider.Setup(fp => fp.GetFileInfo(It.IsAny())) - .Returns(fileDoesNotExist? nonExistingMockFile.Object : existingMockFile.Object); + .Returns(fileDoesNotExist ? doesNotExistMockFile.Object : existingMockFile.Object); } mockFileProvider.Setup(fp => fp.Watch(It.IsAny())) .Returns(new TestFileChangeToken()); - var hostingEnvironment = new Mock(); - hostingEnvironment.Setup(h => h.WebRootFileProvider).Returns(mockFileProvider.Object); - - return hostingEnvironment.Object; - } - - private static IMemoryCache GetMockCache(object result = null) - { - var cache = new Mock(); - cache.CallBase = true; - cache.Setup(c => c.TryGetValue(It.IsAny(), out result)) - .Returns(result != null); - - cache - .Setup( - c => c.Set( - /*key*/ It.IsAny(), - /*value*/ It.IsAny(), - /*options*/ It.IsAny())) - .Returns(new object()); - return cache.Object; + return mockFileProvider.Object; } private static PathString GetRequestPathBase(string requestPathBase = null) { return new PathString(requestPathBase); } + + private class TestableMemoryStream : MemoryStream + { + public TestableMemoryStream(byte[] buffer) + : base(buffer) + { + } + + public bool Disposed { get; private set; } + + protected override void Dispose(bool disposing) + { + base.Dispose(disposing); + Disposed = true; + } + } } } diff --git a/test/Microsoft.AspNet.Mvc.TagHelpers.Test/LinkTagHelperTest.cs b/test/Microsoft.AspNet.Mvc.TagHelpers.Test/LinkTagHelperTest.cs index ebf2d04ace..a113f630ee 100644 --- a/test/Microsoft.AspNet.Mvc.TagHelpers.Test/LinkTagHelperTest.cs +++ b/test/Microsoft.AspNet.Mvc.TagHelpers.Test/LinkTagHelperTest.cs @@ -885,22 +885,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers return applicationEnvironment.Object; } - private static IMemoryCache MakeCache(object result = null) - { - var cache = new Mock(); - cache.CallBase = true; - cache.Setup(c => c.TryGetValue(It.IsAny(), out result)) - .Returns(result != null); - - cache - .Setup( - c => c.Set( - /*key*/ It.IsAny(), - /*value*/ It.IsAny(), - /*options*/ It.IsAny())) - .Returns(result); - return cache.Object; - } + private static IMemoryCache MakeCache() => new MemoryCache(new MemoryCacheOptions()); private static IUrlHelperFactory MakeUrlHelperFactory() { diff --git a/test/Microsoft.AspNet.Mvc.TagHelpers.Test/ScriptTagHelperTest.cs b/test/Microsoft.AspNet.Mvc.TagHelpers.Test/ScriptTagHelperTest.cs index d96d5abb37..ae0f4975f3 100644 --- a/test/Microsoft.AspNet.Mvc.TagHelpers.Test/ScriptTagHelperTest.cs +++ b/test/Microsoft.AspNet.Mvc.TagHelpers.Test/ScriptTagHelperTest.cs @@ -980,24 +980,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers return applicationEnvironment.Object; } - private static IMemoryCache MakeCache(object result = null) - { - var cache = new Mock(); - cache.CallBase = true; - cache.Setup(c => c.TryGetValue(It.IsAny(), out result)) - .Returns(result != null); - - var cacheEntryOptions = new MemoryCacheEntryOptions(); - cacheEntryOptions.AddExpirationToken(Mock.Of()); - cache - .Setup( - c => c.Set( - /*key*/ It.IsAny(), - /*value*/ It.IsAny(), - /*options*/ cacheEntryOptions)) - .Returns(result); - return cache.Object; - } + private static IMemoryCache MakeCache() => new MemoryCache(new MemoryCacheOptions()); private static IUrlHelperFactory MakeUrlHelperFactory() { diff --git a/test/Microsoft.AspNet.Mvc.TestCommon/TestFileChangeToken.cs b/test/Microsoft.AspNet.Mvc.TestCommon/TestFileChangeToken.cs index 718e089e2b..a06c378185 100644 --- a/test/Microsoft.AspNet.Mvc.TestCommon/TestFileChangeToken.cs +++ b/test/Microsoft.AspNet.Mvc.TestCommon/TestFileChangeToken.cs @@ -13,7 +13,14 @@ namespace Microsoft.Extensions.Primitives public IDisposable RegisterChangeCallback(Action callback, object state) { - throw new NotImplementedException(); + return new NullDisposable(); + } + + private class NullDisposable : IDisposable + { + public void Dispose() + { + } } } } \ No newline at end of file