diff --git a/src/Umbraco.Web/Models/ContentExtensions.cs b/src/Umbraco.Web/Models/ContentExtensions.cs index cfee14a2a8..d1f153125e 100644 --- a/src/Umbraco.Web/Models/ContentExtensions.cs +++ b/src/Umbraco.Web/Models/ContentExtensions.cs @@ -46,9 +46,7 @@ namespace Umbraco.Web.Models { var route = umbracoContext == null ? null // for tests only - : umbracoContext.ContentCache.GetRouteById(contentId); // cached - - if (route != null && route.StartsWith("err/")) route = null; + : umbracoContext.ContentCache.GetRouteById(contentId); // may be cached var domainHelper = new DomainHelper(domainService); IDomain domain; @@ -73,7 +71,7 @@ namespace Umbraco.Web.Models } else { - // if content is published then we have a (cached) route + // if content is published then we have a route // from which we can figure out the domain var pos = route.IndexOf('/'); diff --git a/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs b/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs index 5033948730..0b027deb92 100644 --- a/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs +++ b/src/Umbraco.Web/PublishedCache/XmlPublishedCache/PublishedContentCache.cs @@ -98,42 +98,10 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache // else actually determine the route route = DetermineRouteById(umbracoContext, preview, contentId); - // 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 - // - // U4-9121 - this lookup is too expensive when computing a large amount of urls on a front-end (eg menu) - // ... thinking about moving the lookup out of the path into its own async task, so we are not reporting errors - // in the back-office anymore, but at least we are not polluting the cache - // instead, refactored DeterminedIdByRoute to stop using XPath, with a 16x improvement according to benchmarks - // will it be enough? - - 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 if no collision, else report collision - return loopId == contentId ? route : ("err/" + loopId); + // 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 + return route; } IPublishedContent DetermineIdByRoute(UmbracoContext umbracoContext, bool preview, string route, bool hideTopLevelNode) @@ -149,18 +117,25 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache //check if we can find the node in our xml cache var id = NavigateRoute(umbracoContext, preview, startNodeId, path, hideTopLevelNode); - if (id > 0) return GetById(umbracoContext, preview, id); + return id > 0 ? GetById(umbracoContext, preview, id) : null; + } - // if hideTopLevelNodePath is true then for url /foo we looked for /*/foo - // but maybe that was the url of a non-default top-level node, so we also - // have to look for /foo (see note in ApplyHideTopLevelNodeFromPath). - if (hideTopLevelNode && path.Length > 1 && path.IndexOf('/', 1) < 0) + private static XmlElement GetXmlElementChildWithLowestSortOrder(XmlNode element) + { + XmlElement elt = null; + var min = int.MaxValue; + foreach (var n in element.ChildNodes) { - var id2 = NavigateRoute(umbracoContext, preview, startNodeId, path, false); - if (id2 > 0) return GetById(umbracoContext, preview, id2); - } + var e = n as XmlElement; + if (e == null) continue; - return null; + var sortOrder = int.Parse(e.GetAttribute("sortOrder")); + if (sortOrder >= min) continue; + + min = sortOrder; + elt = e; + } + return elt; } private int NavigateRoute(UmbracoContext umbracoContext, bool preview, int startNodeId, string path, bool hideTopLevelNode) @@ -177,20 +152,7 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache return elt == null ? -1 : startNodeId; } - elt = null; - var min = int.MaxValue; - //Don't use OfType or Cast, this is critical code, all ChildNodes are XmlElement so explicitly cast - // https://gist.github.com/Shazwazza/04e2e5642a316f4a87e52dada2901198 - foreach (var n in xml.DocumentElement.ChildNodes) - { - var e = (XmlElement) n; - var sortOrder = int.Parse(e.GetAttribute("sortOrder")); - if (sortOrder < min) - { - min = sortOrder; - elt = e; - } - } + elt = GetXmlElementChildWithLowestSortOrder(xml.DocumentElement); return elt == null ? -1 : int.Parse(elt.GetAttribute("id")); } @@ -208,10 +170,16 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache // https://gist.github.com/Shazwazza/04e2e5642a316f4a87e52dada2901198 foreach (var n in elt.ChildNodes) { - var e = (XmlElement)n; + var e = n as XmlElement; + if (e == null) continue; + var id = NavigateElementRoute(e, urlParts); if (id > 0) return id; } + + if (urlParts.Length == 1) + return NavigateElementRoute(elt, urlParts); + return -1; } @@ -232,9 +200,12 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache found = false; //Don't use OfType or Cast, this is critical code, all ChildNodes are XmlElement so explicitly cast // https://gist.github.com/Shazwazza/04e2e5642a316f4a87e52dada2901198 + var sortOrder = -1; foreach (var o in elt.ChildNodes) { - var child = (XmlElement) o; + var child = o as XmlElement; + if (child == null) continue; + var noNode = UseLegacySchema ? child.Name != "node" : child.GetAttributeNode("isDoc") == null; @@ -242,8 +213,12 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache if (child.GetAttribute("urlName") != urlParts[i]) continue; found = true; + + var so = int.Parse(child.GetAttribute("sortOrder")); + if (sortOrder >= 0 && so >= sortOrder) continue; + + sortOrder = so; elt = child; - break; } i++; } @@ -252,7 +227,8 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache string DetermineRouteById(UmbracoContext umbracoContext, bool preview, int contentId) { - var elt = GetXml(umbracoContext, preview).GetElementById(contentId.ToString(CultureInfo.InvariantCulture)); + var xml = GetXml(umbracoContext, preview); + var elt = xml.GetElementById(contentId.ToString(CultureInfo.InvariantCulture)); if (elt == null) return null; var domainHelper = GetDomainHelper(umbracoContext.Application.Services.DomainService); @@ -279,7 +255,18 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache // no domain, respect HideTopLevelNodeFromPath for legacy purposes if (hasDomains == false && GlobalSettings.HideTopLevelNodeFromPath) - ApplyHideTopLevelNodeFromPath(umbracoContext, eltId, eltParentId, pathParts); + { + if (eltParentId == -1) + { + var rootNode = GetXmlElementChildWithLowestSortOrder(xml.DocumentElement); + if (rootNode != null && rootNode == elt) + pathParts.RemoveAt(pathParts.Count - 1); + } + else + { + pathParts.RemoveAt(pathParts.Count - 1); + } + } // assemble the route pathParts.Reverse(); @@ -289,30 +276,6 @@ namespace Umbraco.Web.PublishedCache.XmlPublishedCache return route; } - static void ApplyHideTopLevelNodeFromPath(UmbracoContext umbracoContext, int nodeId, int parentId, IList pathParts) - { - // in theory if hideTopLevelNodeFromPath is true, then there should be only once - // top-level node, or else domains should be assigned. but for backward compatibility - // we add this check - we look for the document matching "/" and if it's not us, then - // we do not hide the top level path - // it has to be taken care of in GetByRoute too so if - // "/foo" fails (looking for "/*/foo") we try also "/foo". - // this does not make much sense anyway esp. if both "/foo/" and "/bar/foo" exist, but - // that's the way it works pre-4.10 and we try to be backward compat for the time being - if (parentId == -1) - { - var rootNode = umbracoContext.ContentCache.GetByRoute("/", true); - if (rootNode == null) - throw new Exception("Failed to get node at /."); - if (rootNode.Id == nodeId) // remove only if we're the default node - pathParts.RemoveAt(pathParts.Count - 1); - } - else - { - pathParts.RemoveAt(pathParts.Count - 1); - } - } - #endregion #region XPath Strings diff --git a/src/Umbraco.Web/Routing/DefaultUrlProvider.cs b/src/Umbraco.Web/Routing/DefaultUrlProvider.cs index d55095d151..6b3c0fe69b 100644 --- a/src/Umbraco.Web/Routing/DefaultUrlProvider.cs +++ b/src/Umbraco.Web/Routing/DefaultUrlProvider.cs @@ -65,14 +65,6 @@ 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 @@ -115,9 +107,6 @@ 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/PublishedContentRequestEngine.cs b/src/Umbraco.Web/Routing/PublishedContentRequestEngine.cs index 03a50a4b99..05b6b0e468 100644 --- a/src/Umbraco.Web/Routing/PublishedContentRequestEngine.cs +++ b/src/Umbraco.Web/Routing/PublishedContentRequestEngine.cs @@ -67,6 +67,32 @@ namespace Umbraco.Web.Routing #region Public + /// + /// Tries to route the request. + /// + internal bool TryRouteRequest() + { + // disabled - is it going to change the routing? + //_pcr.OnPreparing(); + + FindDomain(); + if (_pcr.IsRedirect) return false; + if (_pcr.HasPublishedContent) return true; + FindPublishedContent(); + if (_pcr.IsRedirect) return false; + + // don't handle anything - we just want to ensure that we find the content + //HandlePublishedContent(); + //FindTemplate(); + //FollowExternalRedirect(); + //HandleWildcardDomains(); + + // disabled - we just want to ensure that we find the content + //_pcr.OnPrepared(); + + return _pcr.HasPublishedContent; + } + /// /// Prepares the request. /// diff --git a/src/Umbraco.Web/Routing/RedirectTrackingEventHandler.cs b/src/Umbraco.Web/Routing/RedirectTrackingEventHandler.cs index b13cb7b296..80e4192f72 100644 --- a/src/Umbraco.Web/Routing/RedirectTrackingEventHandler.cs +++ b/src/Umbraco.Web/Routing/RedirectTrackingEventHandler.cs @@ -295,8 +295,7 @@ namespace Umbraco.Web.Routing private static bool IsNotRoute(string route) { // null if content not found - // err/- if collision or anomaly or ... - return route == null || route.StartsWith("err/"); + return route == null; } // gets a value indicating whether server is 'slave' diff --git a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs index c6e08c16c5..54ef8a42b5 100644 --- a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs +++ b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs @@ -1,7 +1,10 @@ using System; using System.Collections.Generic; +using System.Web.Security; using Umbraco.Core.Models; using umbraco; +using Umbraco.Core; +using Umbraco.Core.Configuration; using Umbraco.Core.Logging; namespace Umbraco.Web.Routing @@ -62,34 +65,45 @@ namespace Umbraco.Web.Routing { urls.Add(ui.Text("content", "getUrlException", umbracoContext.Security.CurrentUser)); } - else if (url.StartsWith("#err-")) + else { - // route error, report - var id = int.Parse(url.Substring(5)); - var o = umbracoContext.ContentCache.GetById(id); - string s; - if (o == null) + // test for collisions + var uri = new Uri(url.TrimEnd('/'), UriKind.RelativeOrAbsolute); + if (uri.IsAbsoluteUri == false) uri = uri.MakeAbsolute(UmbracoContext.Current.CleanedUmbracoUrl); + var pcr = new PublishedContentRequest(uri, UmbracoContext.Current.RoutingContext, UmbracoConfig.For.UmbracoSettings().WebRouting, s => Roles.Provider.GetRolesForUser(s)); + pcr.Engine.TryRouteRequest(); + + if (pcr.HasPublishedContent == false) { - s = "(unknown)"; + urls.Add(ui.Text("content", "routeError", "(error)", umbracoContext.Security.CurrentUser)); + } + else if (pcr.PublishedContent.Id != content.Id) + { + var o = pcr.PublishedContent; + 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=" + pcr.PublishedContent.Id + ")"; + + } + urls.Add(ui.Text("content", "routeError", s, umbracoContext.Security.CurrentUser)); } 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(url); + urls.AddRange(urlProvider.GetOtherUrls(content.Id)); } - urls.Add(ui.Text("content", "routeError", s, umbracoContext.Security.CurrentUser)); - } - else - { - urls.Add(url); - urls.AddRange(urlProvider.GetOtherUrls(content.Id)); } return urls; }