From 5f8747b4d3770ea2585a86f080a582f1826c3160 Mon Sep 17 00:00:00 2001 From: Stephan Date: Wed, 4 May 2016 12:45:20 +0200 Subject: [PATCH] U4-1780 - detect colliding urls (#1243) * U4-1780 - detect colliding urls * U4-1780 - fix error message * U4-1780 - fix XmlHelper issue with sortOrder * U4-1780 - bugfix --- src/Umbraco.Core/XmlHelper.cs | 10 +++- src/Umbraco.Web.UI/umbraco/config/lang/en.xml | 1 + .../umbraco/config/lang/en_us.xml | 3 +- src/Umbraco.Web/Models/ContentExtensions.cs | 2 + .../PublishedContentCache.cs | 57 +++++++++++++++---- src/Umbraco.Web/Routing/DefaultUrlProvider.cs | 12 ++++ .../Routing/UrlProviderExtensions.cs | 24 ++++++++ 7 files changed, 93 insertions(+), 16 deletions(-) diff --git a/src/Umbraco.Core/XmlHelper.cs b/src/Umbraco.Core/XmlHelper.cs index 1978c329ab..d45f0cac3b 100644 --- a/src/Umbraco.Core/XmlHelper.cs +++ b/src/Umbraco.Core/XmlHelper.cs @@ -210,6 +210,9 @@ namespace Umbraco.Core var childNodesAndOrder = parentNode.SelectNodes(childNodesXPath).Cast() .Select(x => Tuple.Create(x, orderBy(x))).ToArray(); + // only one node = node is in the right place already, obviously + if (childNodesAndOrder.Length == 1) return false; + // find the first node with a sortOrder > node.sortOrder var i = 0; while (i < childNodesAndOrder.Length && childNodesAndOrder[i].Item2 <= nodeSortOrder) @@ -220,17 +223,18 @@ namespace Umbraco.Core { // and node is just before, we're done already // else we need to move it right before the node that was found - if (i > 0 && childNodesAndOrder[i - 1].Item1 != node) + if (i == 0 || childNodesAndOrder[i - 1].Item1 != node) { parentNode.InsertBefore(node, childNodesAndOrder[i].Item1); return true; } } - else + else // i == childNodesAndOrder.Length && childNodesAndOrder.Length > 1 { // and node is the last one, we're done already // else we need to append it as the last one - if (i > 0 && childNodesAndOrder[i - 1].Item1 != node) + // (and i > 1, see above) + if (childNodesAndOrder[i - 1].Item1 != node) { parentNode.AppendChild(node); return true; diff --git a/src/Umbraco.Web.UI/umbraco/config/lang/en.xml b/src/Umbraco.Web.UI/umbraco/config/lang/en.xml index e62cbd7ed4..94963cddb1 100644 --- a/src/Umbraco.Web.UI/umbraco/config/lang/en.xml +++ b/src/Umbraco.Web.UI/umbraco/config/lang/en.xml @@ -147,6 +147,7 @@ Properties This document is published but is not visible because the parent '%0%' is unpublished Oops: this document is published but is not in the cache (internal error) + Oops: this document is published but its url would collide with content %0% Publish Publication Status Publish at diff --git a/src/Umbraco.Web.UI/umbraco/config/lang/en_us.xml b/src/Umbraco.Web.UI/umbraco/config/lang/en_us.xml index c313d7ad6a..9743d3166e 100644 --- a/src/Umbraco.Web.UI/umbraco/config/lang/en_us.xml +++ b/src/Umbraco.Web.UI/umbraco/config/lang/en_us.xml @@ -148,6 +148,7 @@ Properties This document is published but is not visible because the parent '%0%' is unpublished Oops: this document is published but is not in the cache (internal error) + Oops: this document is published but its url would collide with content %0% Publish Publication Status Publish at @@ -676,7 +677,7 @@ To manage your website, simply open the Umbraco back office and start adding con Umbraco: Reset Password Your username to login to the Umbraco back-office is: %0%

Click here to reset your password or copy/paste this URL into your browser:

%1%

]]> - + Dashboard diff --git a/src/Umbraco.Web/Models/ContentExtensions.cs b/src/Umbraco.Web/Models/ContentExtensions.cs index 0d280c4c49..cfee14a2a8 100644 --- a/src/Umbraco.Web/Models/ContentExtensions.cs +++ b/src/Umbraco.Web/Models/ContentExtensions.cs @@ -48,6 +48,8 @@ namespace Umbraco.Web.Models ? null // for tests only : umbracoContext.ContentCache.GetRouteById(contentId); // cached + if (route != null && route.StartsWith("err/")) route = null; + var domainHelper = new DomainHelper(domainService); IDomain domain; diff --git a/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs b/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs index d16133f170..8e9b529b7f 100644 --- a/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs +++ b/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs @@ -55,19 +55,28 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache // cache if we have a content and not previewing if (content != null && preview == false) - { - var domainRootNodeId = route.StartsWith("/") ? -1 : int.Parse(route.Substring(0, route.IndexOf('/'))); - var iscanon = - UnitTesting == false - && DomainHelper.ExistsDomainInPath(umbracoContext.Application.Services.DomainService.GetAll(false), content.Path, domainRootNodeId) == false; - // and only if this is the canonical url (the one GetUrl would return) - if (iscanon) - _routesCache.Store(content.Id, route); - } + AddToCacheIfDeepestRoute(umbracoContext, content, route); return content; } + private void AddToCacheIfDeepestRoute(UmbracoContext umbracoContext, IPublishedContent content, string route) + { + var domainRootNodeId = route.StartsWith("/") ? -1 : int.Parse(route.Substring(0, route.IndexOf('/'))); + + // so we have a route that maps to a content... say "1234/path/to/content" - however, there could be a + // domain set on "to" and route "4567/content" would also map to the same content - and due to how + // urls computing work (by walking the tree up to the first domain we find) it is that second route + // that would be returned - the "deepest" route - and that is the route we want to cache, *not* the + // longer one - so make sure we don't cache the wrong route + + var deepest = UnitTesting == false + && DomainHelper.ExistsDomainInPath(umbracoContext.Application.Services.DomainService.GetAll(false), content.Path, domainRootNodeId) == false; + + if (deepest) + _routesCache.Store(content.Id, route); + } + public virtual string GetRouteById(UmbracoContext umbracoContext, bool preview, int contentId) { // try to get from cache if not previewing @@ -80,11 +89,35 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache // else actually determine the route route = DetermineRouteById(umbracoContext, preview, contentId); - // cache if we have a route and not previewing - if (route != null && preview == false) + // node not found + if (route == null) + return null; + + // find the content back, detect routes collisions: we should find ourselves back, + // else it means that another content with "higher priority" is sharing the same route. + // perf impact: + // - non-colliding, adds one complete "by route" lookup, only on the first time a url is computed (then it's cached anyways) + // - colliding, adds one "by route" lookup, the first time the url is computed, then one dictionary looked each time it is computed again + // assuming no collisions, the impact is one complete "by route" lookup the first time each url is computed + var loopId = preview ? 0 : _routesCache.GetNodeId(route); // might be cached already in case of collision + if (loopId == 0) + { + var content = DetermineIdByRoute(umbracoContext, preview, route, GlobalSettings.HideTopLevelNodeFromPath); + + // add the other route to cache so next time we have it already + if (content != null && preview == false) + AddToCacheIfDeepestRoute(umbracoContext, content, route); + + loopId = content == null ? 0 : content.Id; // though... 0 here would be quite weird? + } + + // cache if we have a route and not previewing and it's not a colliding route + // (the result of DetermineRouteById is always the deepest route) + if (/*route != null &&*/ preview == false && loopId == contentId) _routesCache.Store(contentId, route); - return route; + // return route if no collision, else report collision + return loopId == contentId ? route : ("err/" + loopId); } IPublishedContent DetermineIdByRoute(UmbracoContext umbracoContext, bool preview, string route, bool hideTopLevelNode) diff --git a/src/Umbraco.Web/Routing/DefaultUrlProvider.cs b/src/Umbraco.Web/Routing/DefaultUrlProvider.cs index f15c485560..915636889d 100644 --- a/src/Umbraco.Web/Routing/DefaultUrlProvider.cs +++ b/src/Umbraco.Web/Routing/DefaultUrlProvider.cs @@ -52,6 +52,7 @@ namespace Umbraco.Web.Routing // will not use cache if previewing var route = umbracoContext.ContentCache.GetRouteById(id); + if (string.IsNullOrWhiteSpace(route)) { LogHelper.Debug( @@ -60,6 +61,14 @@ namespace Umbraco.Web.Routing return null; } + if (route.StartsWith("err/")) + { + LogHelper.Debug( + "Page with nodeId={0} has a colliding url with page with nodeId={1}.", + () => id, () => route.Substring(4)); + return "#err-" + route.Substring(4); + } + var domainHelper = new DomainHelper(umbracoContext.Application.Services.DomainService); // extract domainUri and path @@ -102,6 +111,9 @@ namespace Umbraco.Web.Routing return null; } + if (route.StartsWith("err/")) + return null; + var domainHelper = new DomainHelper(umbracoContext.Application.Services.DomainService); // extract domainUri and path diff --git a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs index 91860e03e6..0a20f51da7 100644 --- a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs +++ b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs @@ -48,6 +48,30 @@ namespace Umbraco.Web.Routing else urls.Add(ui.Text("content", "parentNotPublished", parent.Name, umbracoContext.Security.CurrentUser)); } + else if (url.StartsWith("#err-")) + { + // route error, report + var id = int.Parse(url.Substring(5)); + var o = umbracoContext.ContentCache.GetById(id); + string s; + if (o == null) + { + s = "(unknown)"; + } + else + { + var l = new List(); + while (o != null) + { + l.Add(o.Name); + o = o.Parent; + } + l.Reverse(); + s = "/" + string.Join("/", l) + " (id=" + id + ")"; + + } + urls.Add(ui.Text("content", "routeError", s, umbracoContext.Security.CurrentUser)); + } else { urls.Add(url);