From aadd0c91297d648ba9876adcb9d6c4e7d6359185 Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 6 Jan 2017 09:12:25 +0100 Subject: [PATCH] U4-9337 - asymmetric route caching --- .../Routing/NiceUrlProviderTests.cs | 28 ++++--- .../Routing/NiceUrlRoutesTests.cs | 81 +++++++++++++++++-- .../NiceUrlsProviderWithDomainsTests.cs | 23 ++++-- .../Routing/UrlsWithNestedDomains.cs | 16 ++-- .../PublishedContentCache.cs | 18 +++-- .../XmlPublishedCache/RoutesCache.cs | 21 +++-- 6 files changed, 138 insertions(+), 49 deletions(-) diff --git a/src/Umbraco.Tests/Routing/NiceUrlProviderTests.cs b/src/Umbraco.Tests/Routing/NiceUrlProviderTests.cs index c6970cbeb3..a8e5b1e64d 100644 --- a/src/Umbraco.Tests/Routing/NiceUrlProviderTests.cs +++ b/src/Umbraco.Tests/Routing/NiceUrlProviderTests.cs @@ -32,7 +32,7 @@ namespace Umbraco.Tests.Routing } /// - /// This checks that when we retrieve a NiceUrl for multiple items that there are no issues with cache overlap + /// This checks that when we retrieve a NiceUrl for multiple items that there are no issues with cache overlap /// and that they are all cached correctly. /// [Test] @@ -71,11 +71,19 @@ namespace Umbraco.Tests.Routing var cache = routingContext.UmbracoContext.ContentCache.InnerCache as PublishedContentCache; if (cache == null) throw new Exception("Unsupported IPublishedContentCache, only the Xml one is supported."); - var cachedRoutes = cache.RoutesCache.GetCachedRoutes(); - // GetUrl does not write to cache - Assert.AreEqual(0, cachedRoutes.Count); - } + var cachedRoutes = cache.RoutesCache.GetCachedRoutes(); + Assert.AreEqual(8, cachedRoutes.Count); + + foreach (var sample in samples) + { + Assert.IsTrue(cachedRoutes.ContainsKey(sample.Key)); + Assert.AreEqual(sample.Value, cachedRoutes[sample.Key]); + } + + var cachedIds = cache.RoutesCache.GetCachedIds(); + Assert.AreEqual(0, cachedIds.Count); + } // test hideTopLevelNodeFromPath false [TestCase(1046, "/home/")] @@ -133,10 +141,10 @@ namespace Umbraco.Tests.Routing var routingContext = GetRoutingContext("http://example.com/test", 1111, umbracoSettings: _umbracoSettings); - + Assert.AreEqual("/home/sub1/custom-sub-1/", routingContext.UrlProvider.GetUrl(1177)); - + requestMock.Setup(x => x.UseDomainPrefixes).Returns(true); Assert.AreEqual("http://example.com/home/sub1/custom-sub-1/", routingContext.UrlProvider.GetUrl(1177)); @@ -159,14 +167,14 @@ namespace Umbraco.Tests.Routing Assert.AreEqual("#", routingContext.UrlProvider.GetUrl(999999)); - requestMock.Setup(x => x.UseDomainPrefixes).Returns(true); - + requestMock.Setup(x => x.UseDomainPrefixes).Returns(true); + Assert.AreEqual("#", routingContext.UrlProvider.GetUrl(999999)); requestMock.Setup(x => x.UseDomainPrefixes).Returns(false); routingContext.UrlProvider.Mode = UrlProviderMode.Absolute; - + Assert.AreEqual("#", routingContext.UrlProvider.GetUrl(999999)); } } diff --git a/src/Umbraco.Tests/Routing/NiceUrlRoutesTests.cs b/src/Umbraco.Tests/Routing/NiceUrlRoutesTests.cs index 79f2b2b64d..131107e8e6 100644 --- a/src/Umbraco.Tests/Routing/NiceUrlRoutesTests.cs +++ b/src/Umbraco.Tests/Routing/NiceUrlRoutesTests.cs @@ -7,6 +7,10 @@ using Umbraco.Web.Routing; namespace Umbraco.Tests.Routing { + // purpose: test the values returned by PublishedContentCache.GetRouteById + // and .GetByRoute (no caching at all, just routing nice urls) including all + // the quirks due to hideTopLevelFromPath and backward compatibility. + [DatabaseTestBehavior(DatabaseBehavior.NewDbFileAndSchemaPerFixture)] [TestFixture] public class NiceUrlRoutesTests : BaseRoutingTest @@ -85,7 +89,7 @@ GetByRoute(route, hide = null): # there are not other reason not to cache it if content and no domain between root and content: - cache route + cache route (as trusted) return content @@ -136,7 +140,11 @@ GetRouteById(id): # never cache the route, it may be colliding - return DetermineRouteById(id) + route = DetermineRouteById(id) + if route: + cache route (as not trusted) + + return route @@ -180,8 +188,6 @@ DetermineRouteById(id): * C 2005 * E 2006 * - * And the tests should verify all the quirks that are due to - * hideTopLevelFromPath */ [TestCase(1000, false, "/a")] @@ -214,10 +220,40 @@ DetermineRouteById(id): SettingsForTests.HideTopLevelNodeFromPath = hide; - var route = cache.GetRouteById(umbracoContext, false, id); + const bool preview = true; // make sure we don't cache + var route = cache.GetRouteById(umbracoContext, preview, id); Assert.AreEqual(expected, route); } + [Test] + public void GetRouteByIdCache() + { + var umbracoContext = GetUmbracoContext("/test", 0); + var cache = umbracoContext.ContentCache.InnerCache as PublishedContentCache; + if (cache == null) throw new Exception("Unsupported IPublishedContentCache, only the Xml one is supported."); + + SettingsForTests.HideTopLevelNodeFromPath = false; + + // make sure we cache + PublishedContentCache.UnitTesting = false; + const bool preview = false; + + var route = cache.GetRouteById(umbracoContext, preview, 1000); + Assert.AreEqual("/a", route); + + // GetRouteById registers a non-trusted route, which is cached for + // id -> route queries (fast GetUrl) but *not* for route -> id + // queries (safe inbound routing) + + var cachedRoutes = cache.RoutesCache.GetCachedRoutes(); + Assert.AreEqual(1, cachedRoutes.Count); + Assert.IsTrue(cachedRoutes.ContainsKey(1000)); + Assert.AreEqual("/a", cachedRoutes[1000]); + + var cachedIds = cache.RoutesCache.GetCachedIds(); + Assert.AreEqual(0, cachedIds.Count); + } + [TestCase("/", false, 1000)] [TestCase("/a", false, 1000)] // yes! [TestCase("/a/b", false, 1001)] @@ -241,7 +277,8 @@ DetermineRouteById(id): SettingsForTests.HideTopLevelNodeFromPath = hide; - var content = cache.GetByRoute(umbracoContext, false, route); + const bool preview = true; // make sure we don't cache + var content = cache.GetByRoute(umbracoContext, preview, route); if (expected < 0) { Assert.IsNull(content); @@ -252,5 +289,37 @@ DetermineRouteById(id): Assert.AreEqual(expected, content.Id); } } + + [Test] + public void GetByRouteCache() + { + var umbracoContext = GetUmbracoContext("/test", 0); + var cache = umbracoContext.ContentCache.InnerCache as PublishedContentCache; + if (cache == null) throw new Exception("Unsupported IPublishedContentCache, only the Xml one is supported."); + + SettingsForTests.HideTopLevelNodeFromPath = false; + + // make sure we cache + PublishedContentCache.UnitTesting = false; + const bool preview = false; + + var content = cache.GetByRoute(umbracoContext, preview, "/a/b/c"); + Assert.IsNotNull(content); + Assert.AreEqual(1002, content.Id); + + // GetByRoute registers a trusted route, which is cached both for + // id -> route queries (fast GetUrl) and for route -> id queries + // (fast inbound routing) + + var cachedRoutes = cache.RoutesCache.GetCachedRoutes(); + Assert.AreEqual(1, cachedRoutes.Count); + Assert.IsTrue(cachedRoutes.ContainsKey(1002)); + Assert.AreEqual("/a/b/c", cachedRoutes[1002]); + + var cachedIds = cache.RoutesCache.GetCachedIds(); + Assert.AreEqual(1, cachedIds.Count); + Assert.IsTrue(cachedIds.ContainsKey("/a/b/c")); + Assert.AreEqual(1002, cachedIds["/a/b/c"]); + } } } diff --git a/src/Umbraco.Tests/Routing/NiceUrlsProviderWithDomainsTests.cs b/src/Umbraco.Tests/Routing/NiceUrlsProviderWithDomainsTests.cs index 6e71efcbf3..ab25e0b43c 100644 --- a/src/Umbraco.Tests/Routing/NiceUrlsProviderWithDomainsTests.cs +++ b/src/Umbraco.Tests/Routing/NiceUrlsProviderWithDomainsTests.cs @@ -79,7 +79,7 @@ namespace Umbraco.Tests.Routing protected override string GetXmlContent(int templateId) { return @" - ]> @@ -302,10 +302,20 @@ namespace Umbraco.Tests.Routing var cache = routingContext.UmbracoContext.ContentCache.InnerCache as PublishedContentCache; if (cache == null) throw new Exception("Unsupported IPublishedContentCache, only the Xml one is supported."); - var cachedRoutes = cache.RoutesCache.GetCachedRoutes(); - // GetUrl does not write to cache - Assert.AreEqual(0, cachedRoutes.Count); + var cachedRoutes = cache.RoutesCache.GetCachedRoutes(); + Assert.AreEqual(7, cachedRoutes.Count); + + var cachedIds = cache.RoutesCache.GetCachedIds(); + Assert.AreEqual(0, cachedIds.Count); + + CheckRoute(cachedRoutes, cachedIds, 1001, "1001/"); + CheckRoute(cachedRoutes, cachedIds, 10011, "10011/"); + CheckRoute(cachedRoutes, cachedIds, 100111, "10011/1001-1-1"); + CheckRoute(cachedRoutes, cachedIds, 10012, "10012/"); + CheckRoute(cachedRoutes, cachedIds, 100121, "10012/1001-2-1"); + CheckRoute(cachedRoutes, cachedIds, 10013, "1001/1001-3"); + CheckRoute(cachedRoutes, cachedIds, 1002, "/1002"); // use the cache Assert.AreEqual("/", routingContext.UrlProvider.GetUrl(1001, new Uri("http://domain1.com"), false)); @@ -319,12 +329,11 @@ namespace Umbraco.Tests.Routing Assert.AreEqual("http://domain1.com/fr/1001-2-1/", routingContext.UrlProvider.GetUrl(100121, new Uri("http://domain2.com"), false)); } - void CheckRoute(IDictionary routes, IDictionary ids, int id, string route) + private static void CheckRoute(IDictionary routes, IDictionary ids, int id, string route) { Assert.IsTrue(routes.ContainsKey(id)); Assert.AreEqual(route, routes[id]); - Assert.IsTrue(ids.ContainsKey(route)); - Assert.AreEqual(id, ids[route]); + Assert.IsFalse(ids.ContainsKey(route)); } [Test] diff --git a/src/Umbraco.Tests/Routing/UrlsWithNestedDomains.cs b/src/Umbraco.Tests/Routing/UrlsWithNestedDomains.cs index 29a60232fa..19420e3120 100644 --- a/src/Umbraco.Tests/Routing/UrlsWithNestedDomains.cs +++ b/src/Umbraco.Tests/Routing/UrlsWithNestedDomains.cs @@ -26,7 +26,7 @@ namespace Umbraco.Tests.Routing public void DoNotPolluteCache() { SettingsForTests.UseDirectoryUrls = true; - SettingsForTests.HideTopLevelNodeFromPath = false; // ignored w/domains + SettingsForTests.HideTopLevelNodeFromPath = false; // ignored w/domains var settings = SettingsForTests.GenerateMockSettings(); var request = Mock.Get(settings.RequestHandler); @@ -36,7 +36,7 @@ namespace Umbraco.Tests.Routing RoutingContext routingContext; string url = "http://domain1.com/1001-1/1001-1-1"; - + // get the nice url for 100111 routingContext = GetRoutingContext(url, 9999, umbracoSettings: settings); Assert.AreEqual("http://domain2.com/1001-1-1/", routingContext.UrlProvider.GetUrl(100111, true)); @@ -44,8 +44,8 @@ namespace Umbraco.Tests.Routing // check that the proper route has been cached var cache = routingContext.UmbracoContext.ContentCache.InnerCache as PublishedContentCache; if (cache == null) throw new Exception("Unsupported IPublishedContentCache, only the Xml one is supported."); - //var cachedRoutes = cache.RoutesCache.GetCachedRoutes(); - //Assert.AreEqual("10011/1001-1-1", cachedRoutes[100111]); + var cachedRoutes = cache.RoutesCache.GetCachedRoutes(); + Assert.AreEqual("10011/1001-1-1", cachedRoutes[100111]); // route a rogue url url = "http://domain1.com/1001-1/1001-1-1"; @@ -61,9 +61,9 @@ namespace Umbraco.Tests.Routing Assert.AreEqual(100111, pcr.PublishedContent.Id); // has the cache been polluted? - //cachedRoutes = cache.RoutesCache.GetCachedRoutes(); - //Assert.AreEqual("10011/1001-1-1", cachedRoutes[100111]); // no - ////Assert.AreEqual("1001/1001-1/1001-1-1", cachedRoutes[100111]); // yes + cachedRoutes = cache.RoutesCache.GetCachedRoutes(); + Assert.AreEqual("10011/1001-1-1", cachedRoutes[100111]); // no + //Assert.AreEqual("1001/1001-1/1001-1-1", cachedRoutes[100111]); // yes // what's the nice url now? Assert.AreEqual("http://domain2.com/1001-1-1/", routingContext.UrlProvider.GetUrl(100111)); // good @@ -89,7 +89,7 @@ namespace Umbraco.Tests.Routing protected override string GetXmlContent(int templateId) { return @" - ]> diff --git a/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs b/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs index d9b0363408..c8ddd77afd 100644 --- a/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs +++ b/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs @@ -2,11 +2,9 @@ using System; using System.Collections.Generic; using System.Globalization; using System.Runtime.CompilerServices; -using System.Text; using System.Xml; using System.Xml.XPath; using Umbraco.Core.Configuration; -using Umbraco.Core.Logging; using Umbraco.Core; using Umbraco.Core.Models; using Umbraco.Core.Models.PublishedContent; @@ -18,7 +16,6 @@ using umbraco.BusinessLogic; using umbraco.presentation.preview; using Umbraco.Core.Services; using GlobalSettings = umbraco.GlobalSettings; -using Task = System.Threading.Tasks.Task; namespace Umbraco.Web.PublishedCache.XmlPublishedCache { @@ -83,7 +80,7 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache && DomainHelper.ExistsDomainInPath(umbracoContext.Application.Services.DomainService.GetAll(false), content.Path, domainRootNodeId) == false; if (deepest) - _routesCache.Store(content.Id, route); + _routesCache.Store(content.Id, route, true); // trusted route } public virtual string GetRouteById(UmbracoContext umbracoContext, bool preview, int contentId) @@ -98,9 +95,16 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache // else actually determine the route route = DetermineRouteById(umbracoContext, preview, contentId); - // may be null if node not found - // do NOT cache the route: it may be colliding - and since checking for a collision implies - // doing one DetermineIdByRoute anyways we are not causing any perf penalty by not caching + // node not found + if (route == null) + return null; + + // cache the route BUT do NOT trust it as it can be a colliding route + // meaning if we GetRouteById again, we'll get it from cache, but it + // won't be used for inbound routing + if (preview == false) + _routesCache.Store(contentId, route, false); + return route; } diff --git a/src/Umbraco.Web/PublishedCache/XmlPublishedCache/RoutesCache.cs b/src/Umbraco.Web/PublishedCache/XmlPublishedCache/RoutesCache.cs index 9c8eed322b..4e775b3253 100644 --- a/src/Umbraco.Web/PublishedCache/XmlPublishedCache/RoutesCache.cs +++ b/src/Umbraco.Web/PublishedCache/XmlPublishedCache/RoutesCache.cs @@ -83,10 +83,12 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache /// /// The node identified. /// The route. - public void Store(int nodeId, string route) + /// A value indicating whether the value can be trusted for inbound routing. + public void Store(int nodeId, string route, bool trust) { _routes.AddOrUpdate(nodeId, i => route, (i, s) => route); - _nodeIds.AddOrUpdate(route, i => nodeId, (i, s) => nodeId); + if (trust) + _nodeIds.AddOrUpdate(route, i => nodeId, (i, s) => nodeId); } /// @@ -119,15 +121,12 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache /// The node identifier. public void ClearNode(int nodeId) { - if (!_routes.ContainsKey(nodeId)) return; - - string key; - if (!_routes.TryGetValue(nodeId, out key)) return; - - int val; - _nodeIds.TryRemove(key, out val); - string val2; - _routes.TryRemove(nodeId, out val2); + string route; + if (_routes.TryRemove(nodeId, out route)) + { + int id; + _nodeIds.TryRemove(route, out id); + } } ///