From 3fac40b4b5dbadd0e099badc7f6745478b0393e1 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 10 Feb 2020 20:15:12 +1100 Subject: [PATCH 01/55] loads preview via sql paging instead of one single query --- .../Repositories/ContentRepository.cs | 67 ++++++++++++------- 1 file changed, 44 insertions(+), 23 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs index a0b211b6b2..24c9e84873 100644 --- a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs @@ -837,6 +837,8 @@ order by umbracoNode.{2}, umbracoNode.parentID, umbracoNode.sortOrder", public XmlDocument BuildPreviewXmlCache() { + + var xmlDoc = new XmlDocument(); var doctype = xmlDoc.CreateDocumentType("root", null, null, ApplicationContext.Current.Services.ContentTypeService.GetContentTypesDtd()); @@ -851,42 +853,61 @@ order by umbracoNode.{2}, umbracoNode.parentID, umbracoNode.sortOrder", var sql = string.Format(@"select umbracoNode.id, umbracoNode.parentID, umbracoNode.sortOrder, cmsPreviewXml.{0}, umbracoNode.{1} from umbracoNode inner join cmsPreviewXml on cmsPreviewXml.nodeId = umbracoNode.id and umbracoNode.nodeObjectType = @type inner join cmsDocument on cmsPreviewXml.versionId = cmsDocument.versionId and cmsDocument.newest=1 -where umbracoNode.trashed = 0 -order by umbracoNode.{2}, umbracoNode.parentID, umbracoNode.sortOrder", +where (umbracoNode.trashed = 0) +order by (umbracoNode.{2}), (umbracoNode.parentID), (umbracoNode.sortOrder)", SqlSyntax.GetQuotedColumnName("xml"), SqlSyntax.GetQuotedColumnName("level"), SqlSyntax.GetQuotedColumnName("level")); + var args = new object[] { new { type = NodeObjectTypeId } }; XmlElement last = null; - //NOTE: Query creates a reader - does not load all into memory - foreach (var row in Database.Query(sql, new { type = NodeObjectTypeId })) + long pageSize = 500; + int? itemCount = null; + long currPage = 0; + do { - string parentId = ((int)row.parentID).ToInvariantString(); - string xml = row.xml; - int sortOrder = row.sortOrder; - //if the parentid is changing - if (last != null && last.GetAttribute("parentID") != parentId) + // Get the paged queries + Database.BuildPageQueries(currPage, pageSize, sql, ref args, out var sqlCount, out var sqlPage); + + // get the item count once + if (itemCount == null) { - parent = xmlDoc.GetElementById(parentId); - if (parent == null) + itemCount = Database.ExecuteScalar(sqlCount, args); + } + currPage++; + + // iterate over rows without allocating all items to memory (Query vs Fetch) + foreach (var row in Database.Query(sqlPage, args)) + { + string parentId = ((int)row.parentID).ToInvariantString(); + string xml = row.xml; + int sortOrder = row.sortOrder; + + //if the parentid is changing + if (last != null && last.GetAttribute("parentID") != parentId) { - //Need to short circuit here, if the parent is not there it means that the parent is unpublished - // and therefore the child is not published either so cannot be included in the xml cache - continue; + parent = xmlDoc.GetElementById(parentId); + if (parent == null) + { + //Need to short circuit here, if the parent is not there it means that the parent is unpublished + // and therefore the child is not published either so cannot be included in the xml cache + continue; + } } + + var xmlDocFragment = xmlDoc.CreateDocumentFragment(); + xmlDocFragment.InnerXml = xml; + + last = (XmlElement)parent.AppendChild(xmlDocFragment); + + // fix sortOrder - see notes in UpdateSortOrder + last.Attributes["sortOrder"].Value = sortOrder.ToInvariantString(); } - var xmlDocFragment = xmlDoc.CreateDocumentFragment(); - xmlDocFragment.InnerXml = xml; - - last = (XmlElement)parent.AppendChild(xmlDocFragment); - - // fix sortOrder - see notes in UpdateSortOrder - last.Attributes["sortOrder"].Value = sortOrder.ToInvariantString(); - } - + } while (itemCount == pageSize); + return xmlDoc; } From e748d842a48282e74ba47edfdb25efad6a9bdc9a Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 26 Mar 2020 22:50:47 +1100 Subject: [PATCH 02/55] Fixes Memory Leak in GetCacheItem cache dependency #7773 --- .../Cache/HttpRuntimeCacheProvider.cs | 59 +++++++------------ src/umbraco.businesslogic/CacheHelper.cs | 31 ---------- .../umbraco.businesslogic.csproj | 1 - 3 files changed, 22 insertions(+), 69 deletions(-) delete mode 100644 src/umbraco.businesslogic/CacheHelper.cs diff --git a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs index 0e98cd5318..30ce3c9926 100644 --- a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs @@ -72,21 +72,16 @@ namespace Umbraco.Core.Cache /// public override object GetCacheItem(string cacheKey, Func getCacheItem) { - return GetCacheItem(cacheKey, getCacheItem, null, dependentFiles: null); + return GetCacheItemInternal(cacheKey, getCacheItem, null); } - /// - /// This overload is here for legacy purposes - /// - /// - /// - /// - /// - /// - /// - /// - /// + [Obsolete("This is here for legacy reasons only, do not use this method, it has memory leaks")] internal object GetCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, CacheDependency dependency = null) + { + return GetCacheItemInternal(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, () => dependency); + } + + private object GetCacheItemInternal(string cacheKey, Func getCacheItem, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, Func dependency = null) { cacheKey = GetCacheKey(cacheKey); @@ -141,7 +136,7 @@ namespace Umbraco.Core.Cache lck.UpgradeToWriteLock(); //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! - _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); + _cache.Insert(cacheKey, result, dependency?.Invoke(), absolute, sliding, priority, removedCallback); } } @@ -160,29 +155,22 @@ namespace Umbraco.Core.Cache public object GetCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - CacheDependency dependency = null; - if (dependentFiles != null && dependentFiles.Any()) - { - dependency = new CacheDependency(dependentFiles); - } - return GetCacheItem(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, dependency); + return GetCacheItemInternal(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, + // Don't create a CacheDependency object unless we need it, see https://github.com/umbraco/Umbraco-CMS/issues/7773 + () => dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null); } #endregion #region Insert - /// - /// This overload is here for legacy purposes - /// - /// - /// - /// - /// - /// - /// - /// + [Obsolete("This is here for legacy reasons only, do not use this method, it has memory leaks")] internal void InsertCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, CacheDependency dependency = null) + { + InsertCacheItemInternal(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, () => dependency); + } + + private void InsertCacheItemInternal(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, Func dependency = null) { // NOTE - here also we must insert a Lazy but we can evaluate it right now // and make sure we don't store a null value. @@ -199,20 +187,17 @@ namespace Umbraco.Core.Cache using (new WriteLock(_locker)) { //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! - _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); + _cache.Insert(cacheKey, result, dependency?.Invoke(), absolute, sliding, priority, removedCallback); } } public void InsertCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - CacheDependency dependency = null; - if (dependentFiles != null && dependentFiles.Any()) - { - dependency = new CacheDependency(dependentFiles); - } - InsertCacheItem(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, dependency); + InsertCacheItemInternal(cacheKey, getCacheItem, timeout, isSliding, priority, removedCallback, + // Don't create a CacheDependency object unless we need it, see https://github.com/umbraco/Umbraco-CMS/issues/7773 + () => dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null); } #endregion } -} \ No newline at end of file +} diff --git a/src/umbraco.businesslogic/CacheHelper.cs b/src/umbraco.businesslogic/CacheHelper.cs deleted file mode 100644 index 243da8eadd..0000000000 --- a/src/umbraco.businesslogic/CacheHelper.cs +++ /dev/null @@ -1,31 +0,0 @@ -//using System; -//using System.Web.Caching; - -//namespace umbraco.BusinessLogic -//{ -// internal class CacheHelper -// { -// public delegate TT GetCacheItemDelegate(); -// public static TT GetCacheItem(string cacheKey, object syncLock, -// CacheItemPriority priority, CacheItemRemovedCallback refreshAction, -// CacheDependency cacheDependency, TimeSpan timeout, GetCacheItemDelegate getCacheItem) -// { -// object result = System.Web.HttpRuntime.Cache.Get(cacheKey); -// if (result == null) -// { -// lock (syncLock) -// { -// result = System.Web.HttpRuntime.Cache.Get(cacheKey); -// if (result == null) -// { -// result = getCacheItem(); -// System.Web.HttpRuntime.Cache.Add(cacheKey, result, cacheDependency, -// DateTime.Now.Add(timeout), TimeSpan.Zero, priority, refreshAction); -// } -// } -// } -// return (TT)result; -// } - -// } -//} diff --git a/src/umbraco.businesslogic/umbraco.businesslogic.csproj b/src/umbraco.businesslogic/umbraco.businesslogic.csproj index f3daf20bb4..aef9f365ad 100644 --- a/src/umbraco.businesslogic/umbraco.businesslogic.csproj +++ b/src/umbraco.businesslogic/umbraco.businesslogic.csproj @@ -203,7 +203,6 @@ ASPXCodeBehind - From f1b398fab63cdf7399c6058f6284638ec4ee00c4 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 1 Apr 2020 11:06:21 +1100 Subject: [PATCH 03/55] Adds some null checks, need to see if tests pass --- src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs | 5 +++++ src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs | 6 ++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index f92d8adebb..e5b66e259c 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -1028,8 +1028,13 @@ namespace Umbraco.Web.PublishedCache.NuCache { parentLink = parentLink ?? GetRequiredParentLink(content, null); + // TODO: This can result in a null value? see https://github.com/umbraco/Umbraco-CMS/issues/7868 + // It seems to be related to having corrupt Paths in the umbracoNode table. var parent = parentLink.Value; + if (parent == null) + throw new PanicException($"A null Value was returned on the {nameof(parentLink)} LinkedNode with id={content.ParentContentId}, potentially your database paths are corrupted, please see the HealthCheck dashboard and fixup data inconsistencies."); + // if parent has no children, clone parent + add as first child if (parent.FirstChildContentId < 0) { diff --git a/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs b/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs index d187996df8..8794978852 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs @@ -1,4 +1,6 @@ -namespace Umbraco.Web.PublishedCache.NuCache.Snap +using System; + +namespace Umbraco.Web.PublishedCache.NuCache.Snap { //NOTE: This cannot be struct because it references itself @@ -11,7 +13,7 @@ { public LinkedNode(TValue value, long gen, LinkedNode next = null) { - Value = value; + Value = value ?? throw new ArgumentNullException(nameof(value)); Gen = gen; Next = next; } From c9c869356001e55dcd30a3c3f7076ba0d01bb3b4 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 1 Apr 2020 13:13:15 +1100 Subject: [PATCH 04/55] removes one null check, adds notes --- src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs | 8 ++++++++ src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs | 6 ++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index e5b66e259c..9298201ecf 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -502,6 +502,14 @@ namespace Umbraco.Web.PublishedCache.NuCache } } + /// + /// Validate the and try to create a parent + /// + /// + /// + /// + /// Returns false if the parent was not found or if the kit validation failed + /// private bool BuildKit(ContentNodeKit kit, out LinkedNode parent) { // make sure parent exists diff --git a/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs b/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs index 8794978852..94f83ac4e5 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/Snap/LinkedNode.cs @@ -1,6 +1,4 @@ -using System; - -namespace Umbraco.Web.PublishedCache.NuCache.Snap +namespace Umbraco.Web.PublishedCache.NuCache.Snap { //NOTE: This cannot be struct because it references itself @@ -13,7 +11,7 @@ namespace Umbraco.Web.PublishedCache.NuCache.Snap { public LinkedNode(TValue value, long gen, LinkedNode next = null) { - Value = value ?? throw new ArgumentNullException(nameof(value)); + Value = value; // This is allowed to be null, we actually explicitly set this to null in ClearLocked Gen = gen; Next = next; } From df19ca45a071bf2711c17bd4daea8e583de7ef57 Mon Sep 17 00:00:00 2001 From: Warren Buckley Date: Thu, 2 Apr 2020 15:35:21 +0100 Subject: [PATCH 05/55] Make this a sucessful login even though they have no start nodes - so our SPA app can decide in the UI what to do/show --- src/Umbraco.Web/Security/BackOfficeSignInManager.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Web/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web/Security/BackOfficeSignInManager.cs index 8e5e532731..5f1c1012f3 100644 --- a/src/Umbraco.Web/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web/Security/BackOfficeSignInManager.cs @@ -121,7 +121,9 @@ namespace Umbraco.Web.Security { _logger.WriteCore(TraceEventType.Information, 0, $"Login attempt failed for username {userName} from IP address {_request.RemoteIpAddress}, no content and/or media start nodes could be found for any of the user's groups", null, null); - return SignInStatus.Failure; + + // We will say its a sucessful login which it is, but they have no node access + return SignInStatus.Success; } } From cc62525378fec06658d3aa6e11409ce929460df1 Mon Sep 17 00:00:00 2001 From: Warren Buckley Date: Thu, 2 Apr 2020 15:36:16 +0100 Subject: [PATCH 06/55] Debug with emitted events - ensure to remove this once PR is done --- .../src/common/services/events.service.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/services/events.service.js b/src/Umbraco.Web.UI.Client/src/common/services/events.service.js index 51f63e6787..f90936f371 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/events.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/events.service.js @@ -1,7 +1,7 @@ /** Used to broadcast and listen for global events and allow the ability to add async listeners to the callbacks */ /* - Core app events: + Core app events: app.ready app.authenticated @@ -12,12 +12,14 @@ */ function eventsService($q, $rootScope) { - + return { - + /** raise an event with a given name */ emit: function (name, args) { + console.log(`Emitting event: ${name}`, args); + //there are no listeners if (!$rootScope.$$listeners[name]) { return; From ce3ea2328d6a761a5fbcfacec170368611a9aa92 Mon Sep 17 00:00:00 2001 From: Warren Buckley Date: Thu, 2 Apr 2020 15:38:19 +0100 Subject: [PATCH 07/55] Use the events to notify us that the user does not have start nodes - potetnially could emit a new event as the login screen not showing in this POC --- src/Umbraco.Web.UI.Client/src/init.js | 7 +++++++ src/Umbraco.Web.UI.Client/src/main.controller.js | 4 ++++ .../src/views/common/login.controller.js | 7 +++++++ 3 files changed, 18 insertions(+) diff --git a/src/Umbraco.Web.UI.Client/src/init.js b/src/Umbraco.Web.UI.Client/src/init.js index d5c5166d21..f46b24a546 100644 --- a/src/Umbraco.Web.UI.Client/src/init.js +++ b/src/Umbraco.Web.UI.Client/src/init.js @@ -18,6 +18,13 @@ app.run(['$rootScope', '$route', '$location', 'urlHelper', 'navigationService', /** Listens for authentication and checks if our required assets are loaded, if/once they are we'll broadcast a ready event */ eventsService.on("app.authenticated", function (evt, data) { + // Lets check if the auth'd user has a start node set (befor trying to make the app ready) + const user = data.user; + if(user.startContentIds.length === 0 && user.startMediaIds.length === 0){ + const args = { isTimedOut: true, noAccess: true }; + eventsService.emit("app.notAuthenticated", args); + } + assetsService._loadInitAssets().then(function () { appReady(data); diff --git a/src/Umbraco.Web.UI.Client/src/main.controller.js b/src/Umbraco.Web.UI.Client/src/main.controller.js index 198ac132c1..4fac328f70 100644 --- a/src/Umbraco.Web.UI.Client/src/main.controller.js +++ b/src/Umbraco.Web.UI.Client/src/main.controller.js @@ -57,6 +57,7 @@ function MainController($scope, $location, appState, treeService, notificationsS }; $scope.showLoginScreen = function(isTimedOut) { + console.log('SHOW ME THE LOGIN SCREEN'); $scope.login.isTimedOut = isTimedOut; $scope.login.show = true; }; @@ -72,6 +73,9 @@ function MainController($scope, $location, appState, treeService, notificationsS $scope.authenticated = null; $scope.user = null; const isTimedOut = data && data.isTimedOut ? true : false; + + console.log('Log me out & show login screen?', isTimedOut); + $scope.showLoginScreen(isTimedOut); // Remove the localstorage items for tours shown diff --git a/src/Umbraco.Web.UI.Client/src/views/common/login.controller.js b/src/Umbraco.Web.UI.Client/src/views/common/login.controller.js index 86132fe8f3..713af9661b 100644 --- a/src/Umbraco.Web.UI.Client/src/views/common/login.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/common/login.controller.js @@ -17,6 +17,13 @@ angular.module('umbraco').controller("Umbraco.LoginController", function (events $location.url(path); }); + eventsService.on("app.notAuthenticated", function(evt, data){ + console.log('not authenticated event back', data); + if(data.noAccess){ + alert('NO NO NO YOU HAVE NO START NODES'); + } + }); + $scope.$on('$destroy', function () { eventsService.unsubscribe(evtOn); }); From 50e9f79ae6f878e837fc0d4748a803378c8d0216 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 6 Apr 2020 23:10:52 +1000 Subject: [PATCH 08/55] Adds a null check if things are out of order when building up the tree/linked list --- .../PublishedCache/NuCache/ContentStore.cs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 9298201ecf..474c390027 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -520,6 +520,15 @@ namespace Umbraco.Web.PublishedCache.NuCache return false; } + // We cannot continue if there's no value. This shouldn't happen but it can happen if the database umbracoNode.path + // data is invalid/corrupt. If that is the case, the parentId might be ok but not the Path which can result in null + // because the data sort operation is by path. + if (parent.Value == null) + { + _logger.Warn($"Skip item id={kit.Node.Id}, no Data assigned for linked node with path {kit.Node.Path} and parent id {kit.Node.ParentContentId}. This can indicate data corruption for the Path value for node {kit.Node.Id}."); + return false; + } + // make sure the kit is valid if (kit.DraftData == null && kit.PublishedData == null) { @@ -1036,12 +1045,13 @@ namespace Umbraco.Web.PublishedCache.NuCache { parentLink = parentLink ?? GetRequiredParentLink(content, null); - // TODO: This can result in a null value? see https://github.com/umbraco/Umbraco-CMS/issues/7868 - // It seems to be related to having corrupt Paths in the umbracoNode table. var parent = parentLink.Value; + // We are doing a null check here but this should no longer be possible because we have a null check in BuildKit + // for the parent.Value property and we'll output a warning. However I'll leave this additional null check in place. + // see https://github.com/umbraco/Umbraco-CMS/issues/7868 if (parent == null) - throw new PanicException($"A null Value was returned on the {nameof(parentLink)} LinkedNode with id={content.ParentContentId}, potentially your database paths are corrupted, please see the HealthCheck dashboard and fixup data inconsistencies."); + throw new PanicException($"A null Value was returned on the {nameof(parentLink)} LinkedNode with id={content.ParentContentId}, potentially your database paths are corrupted."); // if parent has no children, clone parent + add as first child if (parent.FirstChildContentId < 0) From 4b467bf4702b2662cccf478cdb61b60eea17d70b Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 01:02:08 +1000 Subject: [PATCH 09/55] Creates data integrity health checks --- .../Repositories/IContentRepository.cs | 7 + .../Implement/ContentRepositoryBase.cs | 165 +++++++++++++++++- src/Umbraco.Core/Services/IContentService.cs | 2 +- .../Services/IContentServiceBase.cs | 13 +- .../Services/Implement/ContentService.cs | 20 +++ .../Services/Implement/MediaService.cs | 24 +++ .../Checks/Data/DatabaseIntegrityCheck.cs | 97 ++++++++++ src/Umbraco.Web/Umbraco.Web.csproj | 1 + 8 files changed, 326 insertions(+), 3 deletions(-) create mode 100644 src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs diff --git a/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs index 217719e144..aff7a58652 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs @@ -77,5 +77,12 @@ namespace Umbraco.Core.Persistence.Repositories /// Here, can be null but cannot. IEnumerable GetPage(IQuery query, long pageIndex, int pageSize, out long totalRecords, IQuery filter, Ordering ordering); + + /// + /// Checks the data integrity of the node paths stored in the database + /// + bool VerifyNodePaths(out int[] invalidIds); + + void FixNodePaths(); } } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index 13b687eb4e..cd79923323 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -403,7 +403,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement } // content type alias is invariant - if(ordering.OrderBy.InvariantEquals("contentTypeAlias")) + if (ordering.OrderBy.InvariantEquals("contentTypeAlias")) { var joins = Sql() .InnerJoin("ctype").On((content, contentType) => content.ContentTypeId == contentType.NodeId, aliasRight: "ctype"); @@ -477,6 +477,169 @@ namespace Umbraco.Core.Persistence.Repositories.Implement IQuery filter, Ordering ordering); + public bool VerifyNodePaths(out int[] invalidIds) + { + var invalid = new List(); + + var sql = SqlContext.Sql() + .Select() + .From() + .Where(x => x.NodeObjectType == NodeObjectTypeId) + .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); + + // TODO: Could verify sort orders here too + + var currentParentIds = new HashSet { -1 }; + var prevParentIds = currentParentIds; + var lastLevel = -1; + + // use a forward cursor (query) + foreach (var node in Database.Query(sql)) + { + if (node.Level != lastLevel) + { + // changing levels + prevParentIds = currentParentIds; + currentParentIds = null; + lastLevel = node.Level; + } + + if (currentParentIds == null) + { + // we're reset + currentParentIds = new HashSet(); + } + + currentParentIds.Add(node.NodeId); + + var pathParts = node.Path.Split(','); + + if (!prevParentIds.Contains(node.ParentId)) + { + // invalid, this will be because the level is wrong + invalid.Add(node.NodeId); + } + else if (pathParts.Length < 2) + { + // invalid path + invalid.Add(node.NodeId); + } + else if (pathParts.Length - 1 != node.Level) + { + // invalid, either path or level is wrong + invalid.Add(node.NodeId); + } + else if (pathParts[pathParts.Length - 1] != node.NodeId.ToString()) + { + // invalid path + invalid.Add(node.NodeId); + } + else if (pathParts[pathParts.Length - 2] != node.ParentId.ToString()) + { + // invalid path + invalid.Add(node.NodeId); + } + } + + invalidIds = invalid.ToArray(); + return invalid.Count == 0; + } + + public void FixNodePaths() + { + // TODO: We can probably combine this logic with the above + + var invalid = new List<(int child, int parent)>(); + + var sql = SqlContext.Sql() + .Select() + .From() + .Where(x => x.NodeObjectType == NodeObjectTypeId) + .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); + + // TODO: Could verify sort orders here too + + var updated = new List(); + var missingParentIds = new Dictionary>(); + var currentParentIds = new HashSet { -1 }; + var prevParentIds = currentParentIds; + var lastLevel = -1; + + // use a forward cursor (query) + foreach (var node in Database.Query(sql)) + { + if (node.Level != lastLevel) + { + // changing levels + prevParentIds = currentParentIds; + currentParentIds = null; + lastLevel = node.Level; + } + + if (currentParentIds == null) + { + // we're reset + currentParentIds = new HashSet(); + } + + currentParentIds.Add(node.NodeId); + + var pathParts = node.Path.Split(','); + + if (!prevParentIds.Contains(node.ParentId)) + { + // invalid, this will be because the level is wrong (which prob means path is wrong too) + invalid.Add((node.NodeId, node.ParentId)); + if (missingParentIds.TryGetValue(node.ParentId, out var childIds)) + childIds.Add(node); + else + missingParentIds[node.ParentId] = new List {node}; + } + else if (pathParts.Length < 2) + { + // invalid path + invalid.Add((node.NodeId, node.ParentId)); + } + else if (pathParts.Length - 1 != node.Level) + { + // invalid, either path or level is wrong + invalid.Add((node.NodeId, node.ParentId)); + } + else if (pathParts[pathParts.Length - 1] != node.NodeId.ToString()) + { + // invalid path + invalid.Add((node.NodeId, node.ParentId)); + } + else if (pathParts[pathParts.Length - 2] != node.ParentId.ToString()) + { + // invalid path + invalid.Add((node.NodeId, node.ParentId)); + } + else + { + // it's valid + + if (missingParentIds.TryGetValue(node.NodeId, out var invalidNodes)) + { + // this parent has been flagged as missing which means one or more of it's children was ordered + // wrong and was checked first. So now we can try to rebuild the invalid paths. + + foreach (var invalidNode in invalidNodes) + { + invalidNode.Level = (short) (node.Level + 1); + invalidNode.Path = node.Path + "," + invalidNode.NodeId; + updated.Add(invalidNode); + } + } + } + } + + foreach (var node in updated) + { + Database.Update(node); + } + } + // here, filter can be null and ordering cannot protected IEnumerable GetPage(IQuery query, long pageIndex, int pageSize, out long totalRecords, diff --git a/src/Umbraco.Core/Services/IContentService.cs b/src/Umbraco.Core/Services/IContentService.cs index 6f9ca58821..58279fb4da 100644 --- a/src/Umbraco.Core/Services/IContentService.cs +++ b/src/Umbraco.Core/Services/IContentService.cs @@ -526,6 +526,6 @@ namespace Umbraco.Core.Services OperationResult Rollback(int id, int versionId, string culture = "*", int userId = Constants.Security.SuperUserId); #endregion - + } } diff --git a/src/Umbraco.Core/Services/IContentServiceBase.cs b/src/Umbraco.Core/Services/IContentServiceBase.cs index 439c55d0d0..1c04e0b4a3 100644 --- a/src/Umbraco.Core/Services/IContentServiceBase.cs +++ b/src/Umbraco.Core/Services/IContentServiceBase.cs @@ -5,5 +5,16 @@ /// TODO: Start sharing the logic! /// public interface IContentServiceBase : IService - { } + { + + /// + /// Checks the data integrity of the node paths/levels stored in the database + /// + bool VerifyNodePaths(out int[] invalidIds); + + /// + /// Fixes the data integrity of node paths/levels stored in the database + /// + void FixNodePaths(); + } } diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index 1558b0170b..5d010d321f 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -2375,6 +2375,26 @@ namespace Umbraco.Core.Services.Implement return OperationResult.Succeed(evtMsgs); } + public bool VerifyNodePaths(out int[] invalidIds) + { + using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + { + scope.ReadLock(Constants.Locks.ContentTree); + return _documentRepository.VerifyNodePaths(out invalidIds); + } + } + + public void FixNodePaths() + { + using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + { + scope.WriteLock(Constants.Locks.ContentTree); + _documentRepository.FixNodePaths(); + + // TODO: We're going to have to clear all caches + } + } + #endregion #region Internal Methods diff --git a/src/Umbraco.Core/Services/Implement/MediaService.cs b/src/Umbraco.Core/Services/Implement/MediaService.cs index 528d0a0bf9..94f57bd859 100644 --- a/src/Umbraco.Core/Services/Implement/MediaService.cs +++ b/src/Umbraco.Core/Services/Implement/MediaService.cs @@ -1139,6 +1139,28 @@ namespace Umbraco.Core.Services.Implement } return true; + + } + + + public bool VerifyNodePaths(out int[] invalidIds) + { + using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + { + scope.ReadLock(Constants.Locks.MediaTree); + return _mediaRepository.VerifyNodePaths(out invalidIds); + } + } + + public void FixNodePaths() + { + using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + { + scope.WriteLock(Constants.Locks.MediaTree); + _mediaRepository.FixNodePaths(); + + // TODO: We're going to have to clear all caches + } } #endregion @@ -1358,5 +1380,7 @@ namespace Umbraco.Core.Services.Implement } #endregion + + } } diff --git a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs new file mode 100644 index 0000000000..d7bf62067f --- /dev/null +++ b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs @@ -0,0 +1,97 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; +using Umbraco.Core.Services; + +namespace Umbraco.Web.HealthCheck.Checks.Data +{ + [HealthCheck( + "73DD0C1C-E0CA-4C31-9564-1DCA509788AF", + "Database integrity check", + Description = "Checks for various data integrity issues in the Umbraco database.", + Group = "Data Integrity")] + public class DatabaseIntegrityCheck : HealthCheck + { + private readonly IContentService _contentService; + private readonly IMediaService _mediaService; + private const string _fixMediaPaths = "fixMediaPaths"; + private const string _fixContentPaths = "fixContentPaths"; + + public DatabaseIntegrityCheck(IContentService contentService, IMediaService mediaService) + { + _contentService = contentService; + _mediaService = mediaService; + } + + /// + /// Get the status for this health check + /// + /// + public override IEnumerable GetStatus() + { + //return the statuses + return new[] + { + CheckContent(), + CheckMedia() + }; + } + + private HealthCheckStatus CheckMedia() + { + return CheckPaths(_fixMediaPaths, "Fix media paths", "media", () => + { + var mediaPaths = _mediaService.VerifyNodePaths(out var invalidMediaPaths); + return (mediaPaths, invalidMediaPaths); + }); + } + + private HealthCheckStatus CheckContent() + { + return CheckPaths(_fixContentPaths, "Fix content paths", "content", () => + { + var contentPaths = _contentService.VerifyNodePaths(out var invalidContentPaths); + return (contentPaths, invalidContentPaths); + }); + } + + private HealthCheckStatus CheckPaths(string actionAlias, string actionName, string entityType, Func<(bool success, int[] invalidPaths)> doCheck) + { + var result = doCheck(); + + var actions = new List(); + if (!result.success) + { + actions.Add(new HealthCheckAction(actionAlias, Id) + { + Name = actionName + }); + } + + return new HealthCheckStatus(result.success + ? $"All {entityType} paths are valid" + : $"There are {result.invalidPaths.Length} invalid {entityType} paths") + { + ResultType = result.success ? StatusResultType.Success : StatusResultType.Error, + Actions = actions + }; + } + + public override HealthCheckStatus ExecuteAction(HealthCheckAction action) + { + switch (action.Alias) + { + case _fixContentPaths: + _contentService.FixNodePaths(); + return CheckContent(); + case _fixMediaPaths: + _mediaService.FixNodePaths(); + return CheckMedia(); + default: + throw new InvalidOperationException("Action not supported"); + } + } + } +} diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index c3024f63ae..e39687bed8 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -156,6 +156,7 @@ + From 727578e51c57b202cad93d04858b64bf98b3893c Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 13:29:00 +1000 Subject: [PATCH 10/55] Fixes media moving and massively improves moving performance --- src/Umbraco.Core/Constants-SqlTemplates.cs | 17 +++++ .../Implement/ContentRepositoryBase.cs | 14 ++-- .../Repositories/Implement/MediaRepository.cs | 72 +++++++++++-------- .../Services/Implement/MediaService.cs | 43 ++++++----- src/Umbraco.Core/Umbraco.Core.csproj | 1 + 5 files changed, 92 insertions(+), 55 deletions(-) create mode 100644 src/Umbraco.Core/Constants-SqlTemplates.cs diff --git a/src/Umbraco.Core/Constants-SqlTemplates.cs b/src/Umbraco.Core/Constants-SqlTemplates.cs new file mode 100644 index 0000000000..33c94f0f93 --- /dev/null +++ b/src/Umbraco.Core/Constants-SqlTemplates.cs @@ -0,0 +1,17 @@ +namespace Umbraco.Core +{ + public static partial class Constants + { + public static class SqlTemplates + { + public const string VersionableRepositoryGetVersionIds = "Umbraco.Core.VersionableRepository.GetVersionIds"; + public const string VersionableRepositoryGetVersion = "Umbraco.Core.VersionableRepository.GetVersion"; + public const string VersionableRepositoryGetVersions = "Umbraco.Core.VersionableRepository.GetVersions"; + public const string VersionableRepositoryEnsureUniqueNodeName = "Umbraco.Core.VersionableRepository.EnsureUniqueNodeName"; + public const string VersionableRepositoryGetSortOrder = "Umbraco.Core.VersionableRepository.GetSortOrder"; + public const string VersionableRepositoryGetParentNode = "Umbraco.Core.VersionableRepository.GetParentNode"; + public const string VersionableRepositoryGetReservedId = "Umbraco.Core.VersionableRepository.GetReservedId"; + + } + } +} diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index cd79923323..cf67244fbe 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -83,7 +83,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // gets all version ids, current first public virtual IEnumerable GetVersionIds(int nodeId, int maxRows) { - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetVersionIds", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersionIds, tsql => tsql.Select(x => x.Id) .From() .Where(x => x.NodeId == SqlTemplate.Arg("nodeId")) @@ -99,7 +99,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // TODO: test object node type? // get the version we want to delete - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetVersion", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersion, tsql => tsql.Select().From().Where(x => x.Id == SqlTemplate.Arg("versionId")) ); var versionDto = Database.Fetch(template.Sql(new { versionId })).FirstOrDefault(); @@ -121,7 +121,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // TODO: test object node type? // get the versions we want to delete, excluding the current one - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetVersions", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersions, tsql => tsql.Select().From().Where(x => x.NodeId == SqlTemplate.Arg("nodeId") && !x.Current && x.VersionDate < SqlTemplate.Arg("versionDate")) ); var versionDtos = Database.Fetch(template.Sql(new { nodeId, versionDate })); @@ -933,7 +933,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual string EnsureUniqueNodeName(int parentId, string nodeName, int id = 0) { - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.EnsureUniqueNodeName", tsql => tsql + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryEnsureUniqueNodeName, tsql => tsql .Select(x => Alias(x.NodeId, "id"), x => Alias(x.Text, "name")) .From() .Where(x => x.NodeObjectType == SqlTemplate.Arg("nodeObjectType") && x.ParentId == SqlTemplate.Arg("parentId"))); @@ -946,7 +946,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual int GetNewChildSortOrder(int parentId, int first) { - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetSortOrder", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetSortOrder, tsql => tsql.Select($"COALESCE(MAX(sortOrder),{first - 1})").From().Where(x => x.ParentId == SqlTemplate.Arg("parentId") && x.NodeObjectType == NodeObjectTypeId) ); @@ -955,7 +955,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual NodeDto GetParentNodeDto(int parentId) { - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetParentNode", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetParentNode, tsql => tsql.Select().From().Where(x => x.NodeId == SqlTemplate.Arg("parentId")) ); @@ -964,7 +964,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual int GetReservedId(Guid uniqueId) { - var template = SqlContext.Templates.Get("Umbraco.Core.VersionableRepository.GetReservedId", tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetReservedId, tsql => tsql.Select(x => x.NodeId).From().Where(x => x.UniqueId == SqlTemplate.Arg("uniqueId") && x.NodeObjectType == Constants.ObjectTypes.IdReservation) ); var id = Database.ExecuteScalar(template.Sql(new { uniqueId = uniqueId })); diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs index 081efcfdf6..8518da8d8c 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs @@ -219,7 +219,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected override void PersistNewItem(IMedia entity) { - var media = (Models.Media) entity; + var media = entity; entity.AddingEntity(); // ensure unique name on the same level @@ -298,26 +298,35 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected override void PersistUpdatedItem(IMedia entity) { - var media = (Models.Media) entity; + var media = entity; // update media.UpdatingEntity(); - // ensure unique name on the same level - entity.Name = EnsureUniqueNodeName(entity.ParentId, entity.Name, entity.Id); + // Check if this entity is being moved as a descendant as part of a bulk moving operations. + // When this occurs, only Path + Level + UpdateDate are being changed. In this case we can bypass a lot of the below + // operations which will make this whole operation go much faster. When moving we don't need to create + // new versions, etc... because we cannot roll this operation back anyways. + var isMoving = entity.GetDirtyProperties().All(x => x == nameof(entity.Path) || x == nameof(entity.Level) || x == nameof(entity.UpdateDate)); - // ensure that strings don't contain characters that are invalid in xml - // TODO: do we really want to keep doing this here? - entity.SanitizeEntityPropertiesForXmlStorage(); - - // if parent has changed, get path, level and sort order - if (entity.IsPropertyDirty("ParentId")) + if (!isMoving) { - var parent = GetParentNodeDto(entity.ParentId); + // ensure unique name on the same level + entity.Name = EnsureUniqueNodeName(entity.ParentId, entity.Name, entity.Id); - entity.Path = string.Concat(parent.Path, ",", entity.Id); - entity.Level = parent.Level + 1; - entity.SortOrder = GetNewChildSortOrder(entity.ParentId, 0); + // ensure that strings don't contain characters that are invalid in xml + // TODO: do we really want to keep doing this here? + entity.SanitizeEntityPropertiesForXmlStorage(); + + // if parent has changed, get path, level and sort order + if (entity.IsPropertyDirty(nameof(entity.ParentId))) + { + var parent = GetParentNodeDto(entity.ParentId); + + entity.Path = string.Concat(parent.Path, ",", entity.Id); + entity.Level = parent.Level + 1; + entity.SortOrder = GetNewChildSortOrder(entity.ParentId, 0); + } } // create the dto @@ -328,26 +337,29 @@ namespace Umbraco.Core.Persistence.Repositories.Implement nodeDto.ValidatePathWithException(); Database.Update(nodeDto); - // update the content dto - Database.Update(dto.ContentDto); + if (!isMoving) + { + // update the content dto + Database.Update(dto.ContentDto); - // update the content & media version dtos - var contentVersionDto = dto.MediaVersionDto.ContentVersionDto; - var mediaVersionDto = dto.MediaVersionDto; - contentVersionDto.Current = true; - Database.Update(contentVersionDto); - Database.Update(mediaVersionDto); + // update the content & media version dtos + var contentVersionDto = dto.MediaVersionDto.ContentVersionDto; + var mediaVersionDto = dto.MediaVersionDto; + contentVersionDto.Current = true; + Database.Update(contentVersionDto); + Database.Update(mediaVersionDto); - // replace the property data - var deletePropertyDataSql = SqlContext.Sql().Delete().Where(x => x.VersionId == media.VersionId); - Database.Execute(deletePropertyDataSql); - var propertyDataDtos = PropertyFactory.BuildDtos(media.ContentType.Variations, media.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); - foreach (var propertyDataDto in propertyDataDtos) - Database.Insert(propertyDataDto); + // replace the property data + var deletePropertyDataSql = SqlContext.Sql().Delete().Where(x => x.VersionId == media.VersionId); + Database.Execute(deletePropertyDataSql); + var propertyDataDtos = PropertyFactory.BuildDtos(media.ContentType.Variations, media.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); + foreach (var propertyDataDto in propertyDataDtos) + Database.Insert(propertyDataDto); - SetEntityTags(entity, _tagRepository); + SetEntityTags(entity, _tagRepository); - PersistRelations(entity); + PersistRelations(entity); + } OnUowRefreshedEntity(new ScopedEntityEventArgs(AmbientScope, entity)); diff --git a/src/Umbraco.Core/Services/Implement/MediaService.cs b/src/Umbraco.Core/Services/Implement/MediaService.cs index 94f57bd859..aa3e07fa81 100644 --- a/src/Umbraco.Core/Services/Implement/MediaService.cs +++ b/src/Umbraco.Core/Services/Implement/MediaService.cs @@ -530,23 +530,27 @@ namespace Umbraco.Core.Services.Implement totalChildren = 0; return Enumerable.Empty(); } - return GetPagedDescendantsLocked(mediaPath[0].Path, pageIndex, pageSize, out totalChildren, filter, ordering); + return GetPagedLocked(GetPagedDescendantQuery(mediaPath[0].Path), pageIndex, pageSize, out totalChildren, filter, ordering); } - return GetPagedDescendantsLocked(null, pageIndex, pageSize, out totalChildren, filter, ordering); + return GetPagedLocked(GetPagedDescendantQuery(null), pageIndex, pageSize, out totalChildren, filter, ordering); } } - private IEnumerable GetPagedDescendantsLocked(string mediaPath, long pageIndex, int pageSize, out long totalChildren, + private IQuery GetPagedDescendantQuery(string mediaPath) + { + var query = Query(); + if (!mediaPath.IsNullOrWhiteSpace()) + query.Where(x => x.Path.SqlStartsWith(mediaPath + ",", TextColumnType.NVarchar)); + return query; + } + + private IEnumerable GetPagedLocked(IQuery query, long pageIndex, int pageSize, out long totalChildren, IQuery filter, Ordering ordering) { if (pageIndex < 0) throw new ArgumentOutOfRangeException(nameof(pageIndex)); if (pageSize <= 0) throw new ArgumentOutOfRangeException(nameof(pageSize)); if (ordering == null) throw new ArgumentNullException(nameof(ordering)); - var query = Query(); - if (!mediaPath.IsNullOrWhiteSpace()) - query.Where(x => x.Path.SqlStartsWith(mediaPath + ",", TextColumnType.NVarchar)); - return _mediaRepository.GetPage(query, pageIndex, pageSize, out totalChildren, filter, ordering); } @@ -888,7 +892,7 @@ namespace Umbraco.Core.Services.Implement public Attempt MoveToRecycleBin(IMedia media, int userId = Constants.Security.SuperUserId) { var evtMsgs = EventMessagesFactory.Get(); - var moves = new List>(); + var moves = new List<(IMedia, string)>(); using (var scope = ScopeProvider.CreateScope()) { @@ -940,7 +944,7 @@ namespace Umbraco.Core.Services.Implement return OperationResult.Attempt.Succeed(evtMsgs); } - var moves = new List>(); + var moves = new List<(IMedia, string)>(); using (var scope = ScopeProvider.CreateScope()) { @@ -979,7 +983,7 @@ namespace Umbraco.Core.Services.Implement // MUST be called from within WriteLock // trash indicates whether we are trashing, un-trashing, or not changing anything - private void PerformMoveLocked(IMedia media, int parentId, IMedia parent, int userId, ICollection> moves, bool? trash) + private void PerformMoveLocked(IMedia media, int parentId, IMedia parent, int userId, ICollection<(IMedia, string)> moves, bool? trash) { media.ParentId = parentId; @@ -989,7 +993,7 @@ namespace Umbraco.Core.Services.Implement var paths = new Dictionary(); - moves.Add(Tuple.Create(media, media.Path)); // capture original path + moves.Add((media, media.Path)); // capture original path //need to store the original path to lookup descendants based on it below var originalPath = media.Path; @@ -1006,21 +1010,24 @@ namespace Umbraco.Core.Services.Implement paths[media.Id] = (parent == null ? (parentId == Constants.System.RecycleBinMedia ? "-1,-21" : Constants.System.RootString) : parent.Path) + "," + media.Id; const int pageSize = 500; - var page = 0; - var total = long.MaxValue; - while (page * pageSize < total) + var query = GetPagedDescendantQuery(originalPath); + long total; + do { - var descendants = GetPagedDescendantsLocked(originalPath, page++, pageSize, out total, null, Ordering.By("Path", Direction.Ascending)); + var descendants = GetPagedLocked(query, 0, pageSize, out total, null, Ordering.By("Path", Direction.Ascending)); + foreach (var descendant in descendants) { - moves.Add(Tuple.Create(descendant, descendant.Path)); // capture original path + moves.Add((descendant, descendant.Path)); // capture original path // update path and level since we do not update parentId descendant.Path = paths[descendant.Id] = paths[descendant.ParentId] + "," + descendant.Id; descendant.Level += levelDelta; PerformMoveMediaLocked(descendant, userId, trash); } - } + + } while (total > pageSize); + } private void PerformMoveMediaLocked(IMedia media, int userId, bool? trash) @@ -1299,7 +1306,7 @@ namespace Umbraco.Core.Services.Implement // which we need for many things like keeping caches in sync, but we can surely do this MUCH better. var changes = new List>(); - var moves = new List>(); + var moves = new List<(IMedia, string)>(); var mediaTypeIdsA = mediaTypeIds.ToArray(); using (var scope = ScopeProvider.CreateScope()) diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 24954e316c..efb76f2756 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -128,6 +128,7 @@ --> + From ad698f9c19c6442dfbc66c80e3bbe299c235cf4c Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 15:02:08 +1000 Subject: [PATCH 11/55] Fixes content service/repository for moving things correctly along with the perf improvements that were done to media. --- src/Umbraco.Core/ContentExtensions.cs | 10 + .../Implement/DocumentRepository.cs | 301 +++++++++--------- .../Repositories/Implement/MediaRepository.cs | 7 +- .../Services/Implement/ContentService.cs | 42 ++- .../Services/Implement/MediaService.cs | 1 + 5 files changed, 196 insertions(+), 165 deletions(-) diff --git a/src/Umbraco.Core/ContentExtensions.cs b/src/Umbraco.Core/ContentExtensions.cs index 3edad0c963..3ee2a75b09 100644 --- a/src/Umbraco.Core/ContentExtensions.cs +++ b/src/Umbraco.Core/ContentExtensions.cs @@ -60,6 +60,16 @@ namespace Umbraco.Core #endregion + internal static bool IsMoving(this IContentBase entity) + { + // Check if this entity is being moved as a descendant as part of a bulk moving operations. + // When this occurs, only Path + Level + UpdateDate are being changed. In this case we can bypass a lot of the below + // operations which will make this whole operation go much faster. When moving we don't need to create + // new versions, etc... because we cannot roll this operation back anyways. + var isMoving = entity.GetDirtyProperties().All(x => x == nameof(entity.Path) || x == nameof(entity.Level) || x == nameof(entity.UpdateDate)); + return isMoving; + } + /// /// Removes characters that are not valid XML characters from all entity properties /// of type string. See: http://stackoverflow.com/a/961504/5018 diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs index ccfa8209fb..a75165a7b9 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs @@ -321,7 +321,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement .InnerJoin() .On((c, d) => c.Id == d.Id) .Where(x => x.NodeId == SqlTemplate.Arg("nodeId") && !x.Current && x.VersionDate < SqlTemplate.Arg("versionDate")) - .Where( x => !x.Published) + .Where(x => !x.Published) ); var versionDtos = Database.Fetch(template.Sql(new { nodeId, versionDate })); foreach (var versionDto in versionDtos) @@ -519,8 +519,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected override void PersistUpdatedItem(IContent entity) { - var entityBase = entity as EntityBase; - var isEntityDirty = entityBase != null && entityBase.IsDirty(); + var isEntityDirty = entity.IsDirty(); // check if we need to make any database changes at all if ((entity.PublishedState == PublishedState.Published || entity.PublishedState == PublishedState.Unpublished) @@ -535,29 +534,37 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // update entity.UpdatingEntity(); + // Check if this entity is being moved as a descendant as part of a bulk moving operations. + // In this case we can bypass a lot of the below operations which will make this whole operation go much faster. + // When moving we don't need to create new versions, etc... because we cannot roll this operation back anyways. + var isMoving = entity.IsMoving(); + var publishing = entity.PublishedState == PublishedState.Publishing; - // check if we need to create a new version - if (publishing && entity.PublishedVersionId > 0) + if (!isMoving) { - // published version is not published anymore - Database.Execute(Sql().Update(u => u.Set(x => x.Published, false)).Where(x => x.Id == entity.PublishedVersionId)); - } + // check if we need to create a new version + if (publishing && entity.PublishedVersionId > 0) + { + // published version is not published anymore + Database.Execute(Sql().Update(u => u.Set(x => x.Published, false)).Where(x => x.Id == entity.PublishedVersionId)); + } - // sanitize names - SanitizeNames(entity, publishing); + // sanitize names + SanitizeNames(entity, publishing); - // ensure that strings don't contain characters that are invalid in xml - // TODO: do we really want to keep doing this here? - entity.SanitizeEntityPropertiesForXmlStorage(); + // ensure that strings don't contain characters that are invalid in xml + // TODO: do we really want to keep doing this here? + entity.SanitizeEntityPropertiesForXmlStorage(); - // if parent has changed, get path, level and sort order - if (entity.IsPropertyDirty("ParentId")) - { - var parent = GetParentNodeDto(entity.ParentId); - entity.Path = string.Concat(parent.Path, ",", entity.Id); - entity.Level = parent.Level + 1; - entity.SortOrder = GetNewChildSortOrder(entity.ParentId, 0); + // if parent has changed, get path, level and sort order + if (entity.IsPropertyDirty("ParentId")) + { + var parent = GetParentNodeDto(entity.ParentId); + entity.Path = string.Concat(parent.Path, ",", entity.Id); + entity.Level = parent.Level + 1; + entity.SortOrder = GetNewChildSortOrder(entity.ParentId, 0); + } } // create the dto @@ -568,146 +575,152 @@ namespace Umbraco.Core.Persistence.Repositories.Implement nodeDto.ValidatePathWithException(); Database.Update(nodeDto); - // update the content dto - Database.Update(dto.ContentDto); - - // update the content & document version dtos - var contentVersionDto = dto.DocumentVersionDto.ContentVersionDto; - var documentVersionDto = dto.DocumentVersionDto; - if (publishing) + if (!isMoving) { - documentVersionDto.Published = true; // now published - contentVersionDto.Current = false; // no more current - } - Database.Update(contentVersionDto); - Database.Update(documentVersionDto); + // update the content dto + Database.Update(dto.ContentDto); - // and, if publishing, insert new content & document version dtos - if (publishing) - { - entity.PublishedVersionId = entity.VersionId; - - contentVersionDto.Id = 0; // want a new id - contentVersionDto.Current = true; // current version - contentVersionDto.Text = entity.Name; - Database.Insert(contentVersionDto); - entity.VersionId = documentVersionDto.Id = contentVersionDto.Id; // get the new id - - documentVersionDto.Published = false; // non-published version - Database.Insert(documentVersionDto); - } - - // replace the property data (rather than updating) - // only need to delete for the version that existed, the new version (if any) has no property data yet - var versionToDelete = publishing ? entity.PublishedVersionId : entity.VersionId; - var deletePropertyDataSql = Sql().Delete().Where(x => x.VersionId == versionToDelete); - Database.Execute(deletePropertyDataSql); - - // insert property data - var propertyDataDtos = PropertyFactory.BuildDtos(entity.ContentType.Variations, entity.VersionId, publishing ? entity.PublishedVersionId : 0, - entity.Properties, LanguageRepository, out var edited, out var editedCultures); - foreach (var propertyDataDto in propertyDataDtos) - Database.Insert(propertyDataDto); - - // if !publishing, we may have a new name != current publish name, - // also impacts 'edited' - if (!publishing && entity.PublishName != entity.Name) - edited = true; - - if (entity.ContentType.VariesByCulture()) - { - // bump dates to align cultures to version + // update the content & document version dtos + var contentVersionDto = dto.DocumentVersionDto.ContentVersionDto; + var documentVersionDto = dto.DocumentVersionDto; if (publishing) - entity.AdjustDates(contentVersionDto.VersionDate); + { + documentVersionDto.Published = true; // now published + contentVersionDto.Current = false; // no more current + } + Database.Update(contentVersionDto); + Database.Update(documentVersionDto); - // names also impact 'edited' - // ReSharper disable once UseDeconstruction - foreach (var cultureInfo in entity.CultureInfos) - if (cultureInfo.Name != entity.GetPublishName(cultureInfo.Culture)) - { - edited = true; - (editedCultures ?? (editedCultures = new HashSet(StringComparer.OrdinalIgnoreCase))).Add(cultureInfo.Culture); + // and, if publishing, insert new content & document version dtos + if (publishing) + { + entity.PublishedVersionId = entity.VersionId; - // TODO: change tracking - // at the moment, we don't do any dirty tracking on property values, so we don't know whether the - // culture has just been edited or not, so we don't update its update date - that date only changes - // when the name is set, and it all works because the controller does it - but, if someone uses a - // service to change a property value and save (without setting name), the update date does not change. - } + contentVersionDto.Id = 0; // want a new id + contentVersionDto.Current = true; // current version + contentVersionDto.Text = entity.Name; + Database.Insert(contentVersionDto); + entity.VersionId = documentVersionDto.Id = contentVersionDto.Id; // get the new id - // replace the content version variations (rather than updating) + documentVersionDto.Published = false; // non-published version + Database.Insert(documentVersionDto); + } + + // replace the property data (rather than updating) // only need to delete for the version that existed, the new version (if any) has no property data yet - var deleteContentVariations = Sql().Delete().Where(x => x.VersionId == versionToDelete); - Database.Execute(deleteContentVariations); + var versionToDelete = publishing ? entity.PublishedVersionId : entity.VersionId; + var deletePropertyDataSql = Sql().Delete().Where(x => x.VersionId == versionToDelete); + Database.Execute(deletePropertyDataSql); - // replace the document version variations (rather than updating) - var deleteDocumentVariations = Sql().Delete().Where(x => x.NodeId == entity.Id); - Database.Execute(deleteDocumentVariations); + // insert property data + var propertyDataDtos = PropertyFactory.BuildDtos(entity.ContentType.Variations, entity.VersionId, publishing ? entity.PublishedVersionId : 0, + entity.Properties, LanguageRepository, out var edited, out var editedCultures); + foreach (var propertyDataDto in propertyDataDtos) + Database.Insert(propertyDataDto); - // TODO: NPoco InsertBulk issue? - // we should use the native NPoco InsertBulk here but it causes problems (not sure exactly all scenarios) - // but by using SQL Server and updating a variants name will cause: Unable to cast object of type - // 'Umbraco.Core.Persistence.FaultHandling.RetryDbConnection' to type 'System.Data.SqlClient.SqlConnection'. - // (same in PersistNewItem above) + // if !publishing, we may have a new name != current publish name, + // also impacts 'edited' + if (!publishing && entity.PublishName != entity.Name) + edited = true; - // insert content variations - Database.BulkInsertRecords(GetContentVariationDtos(entity, publishing)); + if (entity.ContentType.VariesByCulture()) + { + // bump dates to align cultures to version + if (publishing) + entity.AdjustDates(contentVersionDto.VersionDate); - // insert document variations - Database.BulkInsertRecords(GetDocumentVariationDtos(entity, editedCultures)); + // names also impact 'edited' + // ReSharper disable once UseDeconstruction + foreach (var cultureInfo in entity.CultureInfos) + if (cultureInfo.Name != entity.GetPublishName(cultureInfo.Culture)) + { + edited = true; + (editedCultures ?? (editedCultures = new HashSet(StringComparer.OrdinalIgnoreCase))).Add(cultureInfo.Culture); + + // TODO: change tracking + // at the moment, we don't do any dirty tracking on property values, so we don't know whether the + // culture has just been edited or not, so we don't update its update date - that date only changes + // when the name is set, and it all works because the controller does it - but, if someone uses a + // service to change a property value and save (without setting name), the update date does not change. + } + + // replace the content version variations (rather than updating) + // only need to delete for the version that existed, the new version (if any) has no property data yet + var deleteContentVariations = Sql().Delete().Where(x => x.VersionId == versionToDelete); + Database.Execute(deleteContentVariations); + + // replace the document version variations (rather than updating) + var deleteDocumentVariations = Sql().Delete().Where(x => x.NodeId == entity.Id); + Database.Execute(deleteDocumentVariations); + + // TODO: NPoco InsertBulk issue? + // we should use the native NPoco InsertBulk here but it causes problems (not sure exactly all scenarios) + // but by using SQL Server and updating a variants name will cause: Unable to cast object of type + // 'Umbraco.Core.Persistence.FaultHandling.RetryDbConnection' to type 'System.Data.SqlClient.SqlConnection'. + // (same in PersistNewItem above) + + // insert content variations + Database.BulkInsertRecords(GetContentVariationDtos(entity, publishing)); + + // insert document variations + Database.BulkInsertRecords(GetDocumentVariationDtos(entity, editedCultures)); + } + + // refresh content + entity.SetCultureEdited(editedCultures); + + // update the document dto + // at that point, when un/publishing, the entity still has its old Published value + // so we need to explicitly update the dto to persist the correct value + if (entity.PublishedState == PublishedState.Publishing) + dto.Published = true; + else if (entity.PublishedState == PublishedState.Unpublishing) + dto.Published = false; + entity.Edited = dto.Edited = !dto.Published || edited; // if not published, always edited + Database.Update(dto); + + //update the schedule + if (entity.IsPropertyDirty("ContentSchedule")) + PersistContentSchedule(entity, true); + + // if entity is publishing, update tags, else leave tags there + // means that implicitly unpublished, or trashed, entities *still* have tags in db + if (entity.PublishedState == PublishedState.Publishing) + SetEntityTags(entity, _tagRepository); } - // refresh content - entity.SetCultureEdited(editedCultures); - - // update the document dto - // at that point, when un/publishing, the entity still has its old Published value - // so we need to explicitly update the dto to persist the correct value - if (entity.PublishedState == PublishedState.Publishing) - dto.Published = true; - else if (entity.PublishedState == PublishedState.Unpublishing) - dto.Published = false; - entity.Edited = dto.Edited = !dto.Published || edited; // if not published, always edited - Database.Update(dto); - - //update the schedule - if (entity.IsPropertyDirty("ContentSchedule")) - PersistContentSchedule(entity, true); - - // if entity is publishing, update tags, else leave tags there - // means that implicitly unpublished, or trashed, entities *still* have tags in db - if (entity.PublishedState == PublishedState.Publishing) - SetEntityTags(entity, _tagRepository); - // trigger here, before we reset Published etc OnUowRefreshedEntity(new ScopedEntityEventArgs(AmbientScope, entity)); - // flip the entity's published property - // this also flips its published state - if (entity.PublishedState == PublishedState.Publishing) + if (!isMoving) { - entity.Published = true; - entity.PublishTemplateId = entity.TemplateId; - entity.PublisherId = entity.WriterId; - entity.PublishName = entity.Name; - entity.PublishDate = entity.UpdateDate; + // flip the entity's published property + // this also flips its published state + if (entity.PublishedState == PublishedState.Publishing) + { + entity.Published = true; + entity.PublishTemplateId = entity.TemplateId; + entity.PublisherId = entity.WriterId; + entity.PublishName = entity.Name; + entity.PublishDate = entity.UpdateDate; - SetEntityTags(entity, _tagRepository); + SetEntityTags(entity, _tagRepository); + } + else if (entity.PublishedState == PublishedState.Unpublishing) + { + entity.Published = false; + entity.PublishTemplateId = null; + entity.PublisherId = null; + entity.PublishName = null; + entity.PublishDate = null; + + ClearEntityTags(entity, _tagRepository); + } + + PersistRelations(entity); + + // TODO: note re. tags: explicitly unpublished entities have cleared tags, but masked or trashed entities *still* have tags in the db - so what? } - else if (entity.PublishedState == PublishedState.Unpublishing) - { - entity.Published = false; - entity.PublishTemplateId = null; - entity.PublisherId = null; - entity.PublishName = null; - entity.PublishDate = null; - - ClearEntityTags(entity, _tagRepository); - } - - PersistRelations(entity); - - // TODO: note re. tags: explicitly unpublished entities have cleared tags, but masked or trashed entities *still* have tags in the db - so what? entity.ResetDirtyProperties(); @@ -1183,7 +1196,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (temp.Template2Id.HasValue && templates.ContainsKey(temp.Template2Id.Value)) temp.Content.PublishTemplateId = temp.Template2Id; } - + // set properties if (loadProperties) @@ -1216,7 +1229,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement SetVariations(temp.Content, contentVariations, documentVariations); } } - + foreach (var c in content) @@ -1430,7 +1443,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement yield return dto; } - + } private class ContentVariation diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs index 8518da8d8c..b9ef2d4086 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs @@ -304,10 +304,9 @@ namespace Umbraco.Core.Persistence.Repositories.Implement media.UpdatingEntity(); // Check if this entity is being moved as a descendant as part of a bulk moving operations. - // When this occurs, only Path + Level + UpdateDate are being changed. In this case we can bypass a lot of the below - // operations which will make this whole operation go much faster. When moving we don't need to create - // new versions, etc... because we cannot roll this operation back anyways. - var isMoving = entity.GetDirtyProperties().All(x => x == nameof(entity.Path) || x == nameof(entity.Level) || x == nameof(entity.UpdateDate)); + // In this case we can bypass a lot of the below operations which will make this whole operation go much faster. + // When moving we don't need to create new versions, etc... because we cannot roll this operation back anyways. + var isMoving = entity.IsMoving(); if (!isMoving) { diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index 5d010d321f..f5f3d03abb 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -601,23 +601,27 @@ namespace Umbraco.Core.Services.Implement totalChildren = 0; return Enumerable.Empty(); } - return GetPagedDescendantsLocked(contentPath[0].Path, pageIndex, pageSize, out totalChildren, filter, ordering); + return GetPagedLocked(GetPagedDescendantQuery(contentPath[0].Path), pageIndex, pageSize, out totalChildren, filter, ordering); } - return GetPagedDescendantsLocked(null, pageIndex, pageSize, out totalChildren, filter, ordering); + return GetPagedLocked(null, pageIndex, pageSize, out totalChildren, filter, ordering); } } - private IEnumerable GetPagedDescendantsLocked(string contentPath, long pageIndex, int pageSize, out long totalChildren, + private IQuery GetPagedDescendantQuery(string contentPath) + { + var query = Query(); + if (!contentPath.IsNullOrWhiteSpace()) + query.Where(x => x.Path.SqlStartsWith($"{contentPath},", TextColumnType.NVarchar)); + return query; + } + + private IEnumerable GetPagedLocked(IQuery query, long pageIndex, int pageSize, out long totalChildren, IQuery filter, Ordering ordering) { if (pageIndex < 0) throw new ArgumentOutOfRangeException(nameof(pageIndex)); if (pageSize <= 0) throw new ArgumentOutOfRangeException(nameof(pageSize)); if (ordering == null) throw new ArgumentNullException(nameof(ordering)); - var query = Query(); - if (!contentPath.IsNullOrWhiteSpace()) - query.Where(x => x.Path.SqlStartsWith($"{contentPath},", TextColumnType.NVarchar)); - return _documentRepository.GetPage(query, pageIndex, pageSize, out totalChildren, filter, ordering); } @@ -1866,7 +1870,7 @@ namespace Umbraco.Core.Services.Implement public OperationResult MoveToRecycleBin(IContent content, int userId) { var evtMsgs = EventMessagesFactory.Get(); - var moves = new List>(); + var moves = new List<(IContent, string)>(); using (var scope = ScopeProvider.CreateScope()) { @@ -1925,7 +1929,7 @@ namespace Umbraco.Core.Services.Implement return; } - var moves = new List>(); + var moves = new List<(IContent, string)>(); using (var scope = ScopeProvider.CreateScope()) { @@ -1978,7 +1982,7 @@ namespace Umbraco.Core.Services.Implement // MUST be called from within WriteLock // trash indicates whether we are trashing, un-trashing, or not changing anything private void PerformMoveLocked(IContent content, int parentId, IContent parent, int userId, - ICollection> moves, + ICollection<(IContent, string)> moves, bool? trash) { content.WriterId = userId; @@ -1990,7 +1994,7 @@ namespace Umbraco.Core.Services.Implement var paths = new Dictionary(); - moves.Add(Tuple.Create(content, content.Path)); // capture original path + moves.Add((content, content.Path)); // capture original path //need to store the original path to lookup descendants based on it below var originalPath = content.Path; @@ -2007,20 +2011,24 @@ namespace Umbraco.Core.Services.Implement paths[content.Id] = (parent == null ? (parentId == Constants.System.RecycleBinContent ? "-1,-20" : Constants.System.RootString) : parent.Path) + "," + content.Id; const int pageSize = 500; - var total = long.MaxValue; - while (total > 0) + var query = GetPagedDescendantQuery(originalPath); + long total; + do { - var descendants = GetPagedDescendantsLocked(originalPath, 0, pageSize, out total, null, Ordering.By("Path", Direction.Ascending)); + // We always page a page 0 because for each page, we are moving the result so the resulting total will be reduced + var descendants = GetPagedLocked(query, 0, pageSize, out total, null, Ordering.By("Path", Direction.Ascending)); + foreach (var descendant in descendants) { - moves.Add(Tuple.Create(descendant, descendant.Path)); // capture original path + moves.Add((descendant, descendant.Path)); // capture original path // update path and level since we do not update parentId descendant.Path = paths[descendant.Id] = paths[descendant.ParentId] + "," + descendant.Id; descendant.Level += levelDelta; PerformMoveContentLocked(descendant, userId, trash); } - } + + } while (total > pageSize); } @@ -2832,7 +2840,7 @@ namespace Umbraco.Core.Services.Implement // which we need for many things like keeping caches in sync, but we can surely do this MUCH better. var changes = new List>(); - var moves = new List>(); + var moves = new List<(IContent, string)>(); var contentTypeIdsA = contentTypeIds.ToArray(); // using an immediate uow here because we keep making changes with diff --git a/src/Umbraco.Core/Services/Implement/MediaService.cs b/src/Umbraco.Core/Services/Implement/MediaService.cs index aa3e07fa81..219beb7088 100644 --- a/src/Umbraco.Core/Services/Implement/MediaService.cs +++ b/src/Umbraco.Core/Services/Implement/MediaService.cs @@ -1014,6 +1014,7 @@ namespace Umbraco.Core.Services.Implement long total; do { + // We always page a page 0 because for each page, we are moving the result so the resulting total will be reduced var descendants = GetPagedLocked(query, 0, pageSize, out total, null, Ordering.By("Path", Direction.Ascending)); foreach (var descendant in descendants) From 18d9ad3c7367550fb2d6b9d49995c86330d8b9d4 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 16:42:21 +1000 Subject: [PATCH 12/55] udpates health check and service methods to be flexible for data integrity checks, fixes tests --- src/Umbraco.Core/ContentExtensions.cs | 5 +- .../Models/ContentDataIntegrityReport.cs | 44 +++++ .../Models/ContentDataIntegrityReportEntry.cs | 13 ++ .../ContentDataIntegrityReportOptions.cs | 13 ++ .../Repositories/IContentRepository.cs | 8 +- .../Implement/ContentRepositoryBase.cs | 151 ++++++------------ .../Services/IContentServiceBase.cs | 14 +- .../Services/Implement/ContentService.cs | 14 +- .../Services/Implement/MediaService.cs | 15 +- src/Umbraco.Core/Umbraco.Core.csproj | 3 + .../Checks/Data/DatabaseIntegrityCheck.cs | 53 +++--- .../PublishedCache/NuCache/ContentStore.cs | 2 +- 12 files changed, 163 insertions(+), 172 deletions(-) create mode 100644 src/Umbraco.Core/Models/ContentDataIntegrityReport.cs create mode 100644 src/Umbraco.Core/Models/ContentDataIntegrityReportEntry.cs create mode 100644 src/Umbraco.Core/Models/ContentDataIntegrityReportOptions.cs diff --git a/src/Umbraco.Core/ContentExtensions.cs b/src/Umbraco.Core/ContentExtensions.cs index 3ee2a75b09..7bce23e98e 100644 --- a/src/Umbraco.Core/ContentExtensions.cs +++ b/src/Umbraco.Core/ContentExtensions.cs @@ -66,7 +66,10 @@ namespace Umbraco.Core // When this occurs, only Path + Level + UpdateDate are being changed. In this case we can bypass a lot of the below // operations which will make this whole operation go much faster. When moving we don't need to create // new versions, etc... because we cannot roll this operation back anyways. - var isMoving = entity.GetDirtyProperties().All(x => x == nameof(entity.Path) || x == nameof(entity.Level) || x == nameof(entity.UpdateDate)); + var isMoving = entity.IsPropertyDirty(nameof(entity.Path)) + && entity.IsPropertyDirty(nameof(entity.Level)) + && entity.IsPropertyDirty(nameof(entity.UpdateDate)); + return isMoving; } diff --git a/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs new file mode 100644 index 0000000000..60683ee72f --- /dev/null +++ b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs @@ -0,0 +1,44 @@ +using System.Collections.Generic; + +namespace Umbraco.Core.Models +{ + public class ContentDataIntegrityReport + { + public ContentDataIntegrityReport(IReadOnlyDictionary detectedIssues) + { + DetectedIssues = detectedIssues; + } + + public bool Ok => DetectedIssues.Count == 0; + + public IReadOnlyDictionary DetectedIssues { get; } + + public enum IssueType + { + /// + /// The item's level and path are inconsistent with it's parent's path and level + /// + InvalidPathAndLevelByParentId, + + /// + /// The item's path doesn't contain all required parts + /// + InvalidPathEmpty, + + /// + /// The item's path parts are inconsistent with it's level value + /// + InvalidPathLevelMismatch, + + /// + /// The item's path does not end with it's own ID + /// + InvalidPathById, + + /// + /// The item's path does not have it's parent Id as the 2nd last entry + /// + InvalidPathByParentId, + } + } +} diff --git a/src/Umbraco.Core/Models/ContentDataIntegrityReportEntry.cs b/src/Umbraco.Core/Models/ContentDataIntegrityReportEntry.cs new file mode 100644 index 0000000000..517b9e80dc --- /dev/null +++ b/src/Umbraco.Core/Models/ContentDataIntegrityReportEntry.cs @@ -0,0 +1,13 @@ +namespace Umbraco.Core.Models +{ + public class ContentDataIntegrityReportEntry + { + public ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType issueType) + { + IssueType = issueType; + } + + public ContentDataIntegrityReport.IssueType IssueType { get; } + public bool Fixed { get; set; } + } +} diff --git a/src/Umbraco.Core/Models/ContentDataIntegrityReportOptions.cs b/src/Umbraco.Core/Models/ContentDataIntegrityReportOptions.cs new file mode 100644 index 0000000000..c4689467c1 --- /dev/null +++ b/src/Umbraco.Core/Models/ContentDataIntegrityReportOptions.cs @@ -0,0 +1,13 @@ +namespace Umbraco.Core.Models +{ + public class ContentDataIntegrityReportOptions + { + /// + /// Set to true to try to automatically resolve data integrity issues + /// + public bool FixIssues { get; set; } + + // TODO: We could define all sorts of options for the data integrity check like what to check for, what to fix, etc... + // things like Tag data consistency, etc... + } +} diff --git a/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs index aff7a58652..ad9e2d27c1 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IContentRepository.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using Umbraco.Core.Models; using Umbraco.Core.Models.Entities; using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Umbraco.Core.Persistence.Querying; @@ -78,11 +79,6 @@ namespace Umbraco.Core.Persistence.Repositories IEnumerable GetPage(IQuery query, long pageIndex, int pageSize, out long totalRecords, IQuery filter, Ordering ordering); - /// - /// Checks the data integrity of the node paths stored in the database - /// - bool VerifyNodePaths(out int[] invalidIds); - - void FixNodePaths(); + ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options); } } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index cf67244fbe..42d7e67bc0 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -477,9 +477,9 @@ namespace Umbraco.Core.Persistence.Repositories.Implement IQuery filter, Ordering ordering); - public bool VerifyNodePaths(out int[] invalidIds) + public ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options) { - var invalid = new List(); + var report = new Dictionary(); var sql = SqlContext.Sql() .Select() @@ -487,80 +487,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement .Where(x => x.NodeObjectType == NodeObjectTypeId) .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); - // TODO: Could verify sort orders here too - - var currentParentIds = new HashSet { -1 }; - var prevParentIds = currentParentIds; - var lastLevel = -1; - - // use a forward cursor (query) - foreach (var node in Database.Query(sql)) - { - if (node.Level != lastLevel) - { - // changing levels - prevParentIds = currentParentIds; - currentParentIds = null; - lastLevel = node.Level; - } - - if (currentParentIds == null) - { - // we're reset - currentParentIds = new HashSet(); - } - - currentParentIds.Add(node.NodeId); - - var pathParts = node.Path.Split(','); - - if (!prevParentIds.Contains(node.ParentId)) - { - // invalid, this will be because the level is wrong - invalid.Add(node.NodeId); - } - else if (pathParts.Length < 2) - { - // invalid path - invalid.Add(node.NodeId); - } - else if (pathParts.Length - 1 != node.Level) - { - // invalid, either path or level is wrong - invalid.Add(node.NodeId); - } - else if (pathParts[pathParts.Length - 1] != node.NodeId.ToString()) - { - // invalid path - invalid.Add(node.NodeId); - } - else if (pathParts[pathParts.Length - 2] != node.ParentId.ToString()) - { - // invalid path - invalid.Add(node.NodeId); - } - } - - invalidIds = invalid.ToArray(); - return invalid.Count == 0; - } - - public void FixNodePaths() - { - // TODO: We can probably combine this logic with the above - - var invalid = new List<(int child, int parent)>(); - - var sql = SqlContext.Sql() - .Select() - .From() - .Where(x => x.NodeObjectType == NodeObjectTypeId) - .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); - - // TODO: Could verify sort orders here too - - var updated = new List(); - var missingParentIds = new Dictionary>(); + var nodesToRebuild = new Dictionary>(); + var validNodes = new Dictionary(); var currentParentIds = new HashSet { -1 }; var prevParentIds = currentParentIds; var lastLevel = -1; @@ -589,54 +517,77 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (!prevParentIds.Contains(node.ParentId)) { // invalid, this will be because the level is wrong (which prob means path is wrong too) - invalid.Add((node.NodeId, node.ParentId)); - if (missingParentIds.TryGetValue(node.ParentId, out var childIds)) - childIds.Add(node); - else - missingParentIds[node.ParentId] = new List {node}; + report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathAndLevelByParentId)); + AppendNodeToFix(nodesToRebuild, node); } else if (pathParts.Length < 2) { // invalid path - invalid.Add((node.NodeId, node.ParentId)); + report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathEmpty)); + AppendNodeToFix(nodesToRebuild, node); } else if (pathParts.Length - 1 != node.Level) { // invalid, either path or level is wrong - invalid.Add((node.NodeId, node.ParentId)); + report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathLevelMismatch)); + AppendNodeToFix(nodesToRebuild, node); } else if (pathParts[pathParts.Length - 1] != node.NodeId.ToString()) { // invalid path - invalid.Add((node.NodeId, node.ParentId)); + report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathById)); + AppendNodeToFix(nodesToRebuild, node); } else if (pathParts[pathParts.Length - 2] != node.ParentId.ToString()) { // invalid path - invalid.Add((node.NodeId, node.ParentId)); + report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathByParentId)); + AppendNodeToFix(nodesToRebuild, node); } else { - // it's valid + // it's valid! - if (missingParentIds.TryGetValue(node.NodeId, out var invalidNodes)) - { - // this parent has been flagged as missing which means one or more of it's children was ordered - // wrong and was checked first. So now we can try to rebuild the invalid paths. - - foreach (var invalidNode in invalidNodes) - { - invalidNode.Level = (short) (node.Level + 1); - invalidNode.Path = node.Path + "," + invalidNode.NodeId; - updated.Add(invalidNode); - } - } + // don't track unless we are configured to fix + if (options.FixIssues) + validNodes.Add(node.NodeId, node); } } - foreach (var node in updated) + var updated = new List(); + + if (options.FixIssues) { - Database.Update(node); + // iterate all valid nodes to see if these are parents for invalid nodes + foreach (var (nodeId, node) in validNodes) + { + if (!nodesToRebuild.TryGetValue(nodeId, out var invalidNodes)) continue; + + // now we can try to rebuild the invalid paths. + + foreach (var invalidNode in invalidNodes) + { + invalidNode.Level = (short)(node.Level + 1); + invalidNode.Path = node.Path + "," + invalidNode.NodeId; + updated.Add(invalidNode); + } + } + + foreach (var node in updated) + { + Database.Update(node); + } + } + + return new ContentDataIntegrityReport(report); + + // inline method used to append nodes to rebuild + static void AppendNodeToFix(IDictionary> nodesToRebuild, NodeDto node) + { + if (nodesToRebuild.TryGetValue(node.ParentId, out var childIds)) + childIds.Add(node); + else + nodesToRebuild[node.ParentId] = new List { node }; } } diff --git a/src/Umbraco.Core/Services/IContentServiceBase.cs b/src/Umbraco.Core/Services/IContentServiceBase.cs index 1c04e0b4a3..c40f49347f 100644 --- a/src/Umbraco.Core/Services/IContentServiceBase.cs +++ b/src/Umbraco.Core/Services/IContentServiceBase.cs @@ -1,4 +1,6 @@ -namespace Umbraco.Core.Services +using Umbraco.Core.Models; + +namespace Umbraco.Core.Services { /// /// Placeholder for sharing logic between the content, media (and member) services @@ -6,15 +8,9 @@ /// public interface IContentServiceBase : IService { - /// - /// Checks the data integrity of the node paths/levels stored in the database + /// Checks/fixes the data integrity of node paths/levels stored in the database /// - bool VerifyNodePaths(out int[] invalidIds); - - /// - /// Fixes the data integrity of node paths/levels stored in the database - /// - void FixNodePaths(); + ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options); } } diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index f5f3d03abb..1f384d694f 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -2383,23 +2383,13 @@ namespace Umbraco.Core.Services.Implement return OperationResult.Succeed(evtMsgs); } - public bool VerifyNodePaths(out int[] invalidIds) - { - using (var scope = ScopeProvider.CreateScope(autoComplete: true)) - { - scope.ReadLock(Constants.Locks.ContentTree); - return _documentRepository.VerifyNodePaths(out invalidIds); - } - } - - public void FixNodePaths() + public ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options) { using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { scope.WriteLock(Constants.Locks.ContentTree); - _documentRepository.FixNodePaths(); - // TODO: We're going to have to clear all caches + return _documentRepository.CheckDataIntegrity(options); } } diff --git a/src/Umbraco.Core/Services/Implement/MediaService.cs b/src/Umbraco.Core/Services/Implement/MediaService.cs index 219beb7088..4a2b37ca31 100644 --- a/src/Umbraco.Core/Services/Implement/MediaService.cs +++ b/src/Umbraco.Core/Services/Implement/MediaService.cs @@ -1150,24 +1150,13 @@ namespace Umbraco.Core.Services.Implement } - - public bool VerifyNodePaths(out int[] invalidIds) - { - using (var scope = ScopeProvider.CreateScope(autoComplete: true)) - { - scope.ReadLock(Constants.Locks.MediaTree); - return _mediaRepository.VerifyNodePaths(out invalidIds); - } - } - - public void FixNodePaths() + public ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options) { using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { scope.WriteLock(Constants.Locks.MediaTree); - _mediaRepository.FixNodePaths(); - // TODO: We're going to have to clear all caches + return _mediaRepository.CheckDataIntegrity(options); } } diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index efb76f2756..f15b098473 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -132,6 +132,9 @@ + + + diff --git a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs index d7bf62067f..e40be33fff 100644 --- a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs +++ b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs @@ -3,13 +3,15 @@ using System.Collections.Generic; using System.Linq; using System.Text; using System.Threading.Tasks; +using Serilog.Core; +using Umbraco.Core.Models; using Umbraco.Core.Services; namespace Umbraco.Web.HealthCheck.Checks.Data { [HealthCheck( "73DD0C1C-E0CA-4C31-9564-1DCA509788AF", - "Database integrity check", + "Database data integrity check", Description = "Checks for various data integrity issues in the Umbraco database.", Group = "Data Integrity")] public class DatabaseIntegrityCheck : HealthCheck @@ -18,6 +20,8 @@ namespace Umbraco.Web.HealthCheck.Checks.Data private readonly IMediaService _mediaService; private const string _fixMediaPaths = "fixMediaPaths"; private const string _fixContentPaths = "fixContentPaths"; + private const string _fixMediaPathsTitle = "Fix media paths"; + private const string _fixContentPathsTitle = "Fix content paths"; public DatabaseIntegrityCheck(IContentService contentService, IMediaService mediaService) { @@ -34,35 +38,29 @@ namespace Umbraco.Web.HealthCheck.Checks.Data //return the statuses return new[] { - CheckContent(), - CheckMedia() + CheckDocuments(false), + CheckMedia(false) }; } - private HealthCheckStatus CheckMedia() + private HealthCheckStatus CheckMedia(bool fix) { - return CheckPaths(_fixMediaPaths, "Fix media paths", "media", () => - { - var mediaPaths = _mediaService.VerifyNodePaths(out var invalidMediaPaths); - return (mediaPaths, invalidMediaPaths); - }); + return CheckPaths(_fixMediaPaths, _fixMediaPathsTitle, Core.Constants.UdiEntityType.Media, + () => _mediaService.CheckDataIntegrity(new ContentDataIntegrityReportOptions {FixIssues = fix})); } - private HealthCheckStatus CheckContent() + private HealthCheckStatus CheckDocuments(bool fix) { - return CheckPaths(_fixContentPaths, "Fix content paths", "content", () => - { - var contentPaths = _contentService.VerifyNodePaths(out var invalidContentPaths); - return (contentPaths, invalidContentPaths); - }); + return CheckPaths(_fixContentPaths, _fixContentPathsTitle, Core.Constants.UdiEntityType.Document, + () => _contentService.CheckDataIntegrity(new ContentDataIntegrityReportOptions {FixIssues = fix})); } - private HealthCheckStatus CheckPaths(string actionAlias, string actionName, string entityType, Func<(bool success, int[] invalidPaths)> doCheck) + private HealthCheckStatus CheckPaths(string actionAlias, string actionName, string entityType, Func doCheck) { var result = doCheck(); var actions = new List(); - if (!result.success) + if (!result.Ok) { actions.Add(new HealthCheckAction(actionAlias, Id) { @@ -70,28 +68,23 @@ namespace Umbraco.Web.HealthCheck.Checks.Data }); } - return new HealthCheckStatus(result.success + return new HealthCheckStatus(result.Ok ? $"All {entityType} paths are valid" - : $"There are {result.invalidPaths.Length} invalid {entityType} paths") + : $"There are {result.DetectedIssues.Count} invalid {entityType} paths") { - ResultType = result.success ? StatusResultType.Success : StatusResultType.Error, + ResultType = result.Ok ? StatusResultType.Success : StatusResultType.Error, Actions = actions }; } public override HealthCheckStatus ExecuteAction(HealthCheckAction action) { - switch (action.Alias) + return action.Alias switch { - case _fixContentPaths: - _contentService.FixNodePaths(); - return CheckContent(); - case _fixMediaPaths: - _mediaService.FixNodePaths(); - return CheckMedia(); - default: - throw new InvalidOperationException("Action not supported"); - } + _fixContentPaths => CheckDocuments(true), + _fixMediaPaths => CheckMedia(true), + _ => throw new InvalidOperationException("Action not supported") + }; } } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 474c390027..e40e7130d0 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -525,7 +525,7 @@ namespace Umbraco.Web.PublishedCache.NuCache // because the data sort operation is by path. if (parent.Value == null) { - _logger.Warn($"Skip item id={kit.Node.Id}, no Data assigned for linked node with path {kit.Node.Path} and parent id {kit.Node.ParentContentId}. This can indicate data corruption for the Path value for node {kit.Node.Id}."); + _logger.Warn($"Skip item id={kit.Node.Id}, no Data assigned for linked node with path {kit.Node.Path} and parent id {kit.Node.ParentContentId}. This can indicate data corruption for the Path value for node {kit.Node.Id}. See the Health Check dashboard in Settings to resolve data integrity issues."); return false; } From 3ef188376517a8afc024d2e4b39170fe351013fd Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 16:53:54 +1000 Subject: [PATCH 13/55] fix build (non latest c#) --- .../Implement/ContentRepositoryBase.cs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index 42d7e67bc0..93c4078f46 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -580,15 +580,14 @@ namespace Umbraco.Core.Persistence.Repositories.Implement } return new ContentDataIntegrityReport(report); + } - // inline method used to append nodes to rebuild - static void AppendNodeToFix(IDictionary> nodesToRebuild, NodeDto node) - { - if (nodesToRebuild.TryGetValue(node.ParentId, out var childIds)) - childIds.Add(node); - else - nodesToRebuild[node.ParentId] = new List { node }; - } + private static void AppendNodeToFix(IDictionary> nodesToRebuild, NodeDto node) + { + if (nodesToRebuild.TryGetValue(node.ParentId, out var childIds)) + childIds.Add(node); + else + nodesToRebuild[node.ParentId] = new List { node }; } // here, filter can be null and ordering cannot From 19f0793cba726ce03acea4da15ed1f4ff59bc09c Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 7 Apr 2020 17:38:40 +1000 Subject: [PATCH 14/55] updates health check --- .../Implement/ContentRepositoryBase.cs | 12 +++--- .../Checks/Data/DatabaseIntegrityCheck.cs | 38 +++++++++++++++---- 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index 93c4078f46..9999e9f2d6 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -489,7 +489,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement var nodesToRebuild = new Dictionary>(); var validNodes = new Dictionary(); - var currentParentIds = new HashSet { -1 }; + var rootIds = new[] {Constants.System.Root, Constants.System.RecycleBinContent, Constants.System.RecycleBinMedia}; + var currentParentIds = new HashSet(rootIds); var prevParentIds = currentParentIds; var lastLevel = -1; @@ -512,7 +513,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement currentParentIds.Add(node.NodeId); - var pathParts = node.Path.Split(','); + // paths parts without the roots + var pathParts = node.Path.Split(',').Where(x => !rootIds.Contains(int.Parse(x))).ToArray(); if (!prevParentIds.Contains(node.ParentId)) { @@ -520,13 +522,13 @@ namespace Umbraco.Core.Persistence.Repositories.Implement report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathAndLevelByParentId)); AppendNodeToFix(nodesToRebuild, node); } - else if (pathParts.Length < 2) + else if (pathParts.Length == 0) { // invalid path report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathEmpty)); AppendNodeToFix(nodesToRebuild, node); } - else if (pathParts.Length - 1 != node.Level) + else if (pathParts.Length != node.Level) { // invalid, either path or level is wrong report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathLevelMismatch)); @@ -538,7 +540,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathById)); AppendNodeToFix(nodesToRebuild, node); } - else if (pathParts[pathParts.Length - 2] != node.ParentId.ToString()) + else if (!rootIds.Contains(node.ParentId) && pathParts[pathParts.Length - 2] != node.ParentId.ToString()) { // invalid path report.Add(node.NodeId, new ContentDataIntegrityReportEntry(ContentDataIntegrityReport.IssueType.InvalidPathByParentId)); diff --git a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs index e40be33fff..876ad9f1d7 100644 --- a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs +++ b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs @@ -45,22 +45,22 @@ namespace Umbraco.Web.HealthCheck.Checks.Data private HealthCheckStatus CheckMedia(bool fix) { - return CheckPaths(_fixMediaPaths, _fixMediaPathsTitle, Core.Constants.UdiEntityType.Media, + return CheckPaths(_fixMediaPaths, _fixMediaPathsTitle, Core.Constants.UdiEntityType.Media, fix, () => _mediaService.CheckDataIntegrity(new ContentDataIntegrityReportOptions {FixIssues = fix})); } private HealthCheckStatus CheckDocuments(bool fix) { - return CheckPaths(_fixContentPaths, _fixContentPathsTitle, Core.Constants.UdiEntityType.Document, + return CheckPaths(_fixContentPaths, _fixContentPathsTitle, Core.Constants.UdiEntityType.Document, fix, () => _contentService.CheckDataIntegrity(new ContentDataIntegrityReportOptions {FixIssues = fix})); } - private HealthCheckStatus CheckPaths(string actionAlias, string actionName, string entityType, Func doCheck) + private HealthCheckStatus CheckPaths(string actionAlias, string actionName, string entityType, bool detailedReport, Func doCheck) { - var result = doCheck(); + var report = doCheck(); var actions = new List(); - if (!result.Ok) + if (!report.Ok) { actions.Add(new HealthCheckAction(actionAlias, Id) { @@ -68,15 +68,37 @@ namespace Umbraco.Web.HealthCheck.Checks.Data }); } - return new HealthCheckStatus(result.Ok + return new HealthCheckStatus(report.Ok ? $"All {entityType} paths are valid" - : $"There are {result.DetectedIssues.Count} invalid {entityType} paths") + : GetInvalidReport(report, entityType, detailedReport)) { - ResultType = result.Ok ? StatusResultType.Success : StatusResultType.Error, + ResultType = report.Ok ? StatusResultType.Success : StatusResultType.Error, Actions = actions }; } + private static string GetInvalidReport(ContentDataIntegrityReport report, string entityType, bool detailed) + { + var sb = new StringBuilder(); + sb.AppendLine($"There are {report.DetectedIssues.Count} invalid {entityType} paths"); + + if (true && report.DetectedIssues.Count > 0) + { + sb.AppendLine("
    "); + foreach (var issueGroup in report.DetectedIssues.GroupBy(x => x.Value.IssueType)) + { + var countByGroup = issueGroup.Count(); + var fixedByGroup = issueGroup.Count(x => x.Value.Fixed); + sb.AppendLine("
  • "); + sb.AppendLine($"{countByGroup} issues of type {issueGroup.Key} ... {fixedByGroup} fixed"); + sb.AppendLine("
    • "); + } + sb.AppendLine("
"); + } + + return sb.ToString(); + } + public override HealthCheckStatus ExecuteAction(HealthCheckAction action) { return action.Alias switch From 49c6fd2c76f76f09f7f14868ad827a67a1af6c3d Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 8 Apr 2020 10:38:02 +1000 Subject: [PATCH 15/55] data integrity report working --- .../Models/ContentDataIntegrityReport.cs | 3 ++- .../Implement/ContentRepositoryBase.cs | 2 ++ .../Checks/Data/DatabaseIntegrityCheck.cs | 21 +++++++++++++------ 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs index 60683ee72f..d3660679a7 100644 --- a/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs +++ b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Linq; namespace Umbraco.Core.Models { @@ -9,7 +10,7 @@ namespace Umbraco.Core.Models DetectedIssues = detectedIssues; } - public bool Ok => DetectedIssues.Count == 0; + public bool Ok => DetectedIssues.Count == 0 || DetectedIssues.Count == DetectedIssues.Values.Count(x => x.Fixed); public IReadOnlyDictionary DetectedIssues { get; } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index 9999e9f2d6..4aa9655249 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -578,6 +578,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement foreach (var node in updated) { Database.Update(node); + if (report.TryGetValue(node.NodeId, out var entry)) + entry.Fixed = true; } } diff --git a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs index 876ad9f1d7..b7d9aea3b9 100644 --- a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs +++ b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs @@ -68,21 +68,30 @@ namespace Umbraco.Web.HealthCheck.Checks.Data }); } - return new HealthCheckStatus(report.Ok - ? $"All {entityType} paths are valid" - : GetInvalidReport(report, entityType, detailedReport)) + return new HealthCheckStatus(GetReport(report, entityType, detailedReport)) { ResultType = report.Ok ? StatusResultType.Success : StatusResultType.Error, Actions = actions }; } - private static string GetInvalidReport(ContentDataIntegrityReport report, string entityType, bool detailed) + private static string GetReport(ContentDataIntegrityReport report, string entityType, bool detailed) { var sb = new StringBuilder(); - sb.AppendLine($"There are {report.DetectedIssues.Count} invalid {entityType} paths"); - if (true && report.DetectedIssues.Count > 0) + if (report.Ok) + { + sb.AppendLine($"

All {entityType} paths are valid

"); + + if (!detailed) + return sb.ToString(); + } + else + { + sb.AppendLine($"

{report.DetectedIssues.Count} invalid {entityType} paths detected.

"); + } + + if (detailed && report.DetectedIssues.Count > 0) { sb.AppendLine("
    "); foreach (var issueGroup in report.DetectedIssues.GroupBy(x => x.Value.IssueType)) From d54eed5cb9396d23094bf712458e23588703f341 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 8 Apr 2020 10:54:48 +1000 Subject: [PATCH 16/55] argh! can't use switch expressions in v8 --- .../Checks/Data/DatabaseIntegrityCheck.cs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs index b7d9aea3b9..0c3e2f3d91 100644 --- a/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs +++ b/src/Umbraco.Web/HealthCheck/Checks/Data/DatabaseIntegrityCheck.cs @@ -110,12 +110,15 @@ namespace Umbraco.Web.HealthCheck.Checks.Data public override HealthCheckStatus ExecuteAction(HealthCheckAction action) { - return action.Alias switch + switch (action.Alias) { - _fixContentPaths => CheckDocuments(true), - _fixMediaPaths => CheckMedia(true), - _ => throw new InvalidOperationException("Action not supported") - }; + case _fixContentPaths: + return CheckDocuments(true); + case _fixMediaPaths: + return CheckMedia(true); + default: + throw new InvalidOperationException("Action not supported"); + } } } } From 36c9cd47a106a14b1944f708ae96ea933aa6181a Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 8 Apr 2020 11:22:15 +1000 Subject: [PATCH 17/55] Adds notes --- .../Repositories/Implement/DocumentRepository.cs | 4 ++++ src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs | 9 +++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs index a75165a7b9..db1e2b350d 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs @@ -538,6 +538,10 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // In this case we can bypass a lot of the below operations which will make this whole operation go much faster. // When moving we don't need to create new versions, etc... because we cannot roll this operation back anyways. var isMoving = entity.IsMoving(); + // TODO: I'm sure we can also detect a "Copy" (of a descendant) operation and probably perform similar checks below. + // There is probably more stuff that would be required for copying but I'm sure not all of this logic would be, we could more than likely boost + // copy performance by 95% just like we did for Move + var publishing = entity.PublishedState == PublishedState.Publishing; diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index e40e7130d0..b5fc1abd7f 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -817,7 +817,7 @@ namespace Umbraco.Web.PublishedCache.NuCache { //this zero's out the branch (recursively), if we're in a new gen this will add a NULL placeholder for the gen ClearBranchLocked(existing); - //TODO: This removes the current GEN from the tree - do we really want to do that? + //TODO: This removes the current GEN from the tree - do we really want to do that? (not sure if this is still an issue....) RemoveTreeNodeLocked(existing); } @@ -882,6 +882,10 @@ namespace Umbraco.Web.PublishedCache.NuCache private void ClearBranchLocked(ContentNode content) { + // This should never be null, all code that calls this method is null checking but we've seen + // issues of null ref exceptions in issue reports so we'll double check here + if (content == null) throw new ArgumentNullException(nameof(content)); + SetValueLocked(_contentNodes, content.Id, null); if (_localDb != null) RegisterChange(content.Id, ContentNodeKit.Null); @@ -890,8 +894,9 @@ namespace Umbraco.Web.PublishedCache.NuCache var id = content.FirstChildContentId; while (id > 0) { + // get the required link node, this ensures that both `link` and `link.Value` are not null var link = GetRequiredLinkedNode(id, "child", null); - ClearBranchLocked(link.Value); + ClearBranchLocked(link.Value); // recurse id = link.Value.NextSiblingContentId; } } From 64e7a29795cd62bf5253ab997250a510a84f07bc Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 8 Apr 2020 15:03:21 +1000 Subject: [PATCH 18/55] Ensures caches are refreshed after data errors are fixed --- .../Models/ContentDataIntegrityReport.cs | 3 +++ .../Services/Implement/ContentService.cs | 13 +++++++++++-- src/Umbraco.Core/Services/Implement/MediaService.cs | 13 +++++++++++-- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs index d3660679a7..9f0f147083 100644 --- a/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs +++ b/src/Umbraco.Core/Models/ContentDataIntegrityReport.cs @@ -14,6 +14,9 @@ namespace Umbraco.Core.Models public IReadOnlyDictionary DetectedIssues { get; } + public IReadOnlyDictionary FixedIssues + => DetectedIssues.Where(x => x.Value.Fixed).ToDictionary(x => x.Key, x => x.Value); + public enum IssueType { /// diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index 1f384d694f..068864a558 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -2388,8 +2388,17 @@ namespace Umbraco.Core.Services.Implement using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { scope.WriteLock(Constants.Locks.ContentTree); - // TODO: We're going to have to clear all caches - return _documentRepository.CheckDataIntegrity(options); + + var report = _documentRepository.CheckDataIntegrity(options); + + if (report.FixedIssues.Count > 0) + { + //The event args needs a content item so we'll make a fake one with enough properties to not cause a null ref + var root = new Content("root", -1, new ContentType(-1)) {Id = -1, Key = Guid.Empty}; + scope.Events.Dispatch(TreeChanged, this, new TreeChange.EventArgs(new TreeChange(root, TreeChangeTypes.RefreshAll))); + } + + return report; } } diff --git a/src/Umbraco.Core/Services/Implement/MediaService.cs b/src/Umbraco.Core/Services/Implement/MediaService.cs index 4a2b37ca31..ecd4cccc8d 100644 --- a/src/Umbraco.Core/Services/Implement/MediaService.cs +++ b/src/Umbraco.Core/Services/Implement/MediaService.cs @@ -1155,8 +1155,17 @@ namespace Umbraco.Core.Services.Implement using (var scope = ScopeProvider.CreateScope(autoComplete: true)) { scope.WriteLock(Constants.Locks.MediaTree); - // TODO: We're going to have to clear all caches - return _mediaRepository.CheckDataIntegrity(options); + + var report = _mediaRepository.CheckDataIntegrity(options); + + if (report.FixedIssues.Count > 0) + { + //The event args needs a content item so we'll make a fake one with enough properties to not cause a null ref + var root = new Models.Media("root", -1, new MediaType(-1)) { Id = -1, Key = Guid.Empty }; + scope.Events.Dispatch(TreeChanged, this, new TreeChange.EventArgs(new TreeChange(root, TreeChangeTypes.RefreshAll))); + } + + return report; } } From 5f5773b0c02e2190ac7d94b3ce84008c9a85a63c Mon Sep 17 00:00:00 2001 From: Benjamin Carleski Date: Wed, 1 Apr 2020 16:22:20 -0700 Subject: [PATCH 19/55] 7879: Handle for multiple picked media relations (cherry picked from commit 9f7c44c64ebfb23acd1bb06aa22c5bc39ad5079a) --- .../PropertyEditors/MediaPickerPropertyEditor.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Web/PropertyEditors/MediaPickerPropertyEditor.cs b/src/Umbraco.Web/PropertyEditors/MediaPickerPropertyEditor.cs index ece210b9d1..fa8060bd15 100644 --- a/src/Umbraco.Web/PropertyEditors/MediaPickerPropertyEditor.cs +++ b/src/Umbraco.Web/PropertyEditors/MediaPickerPropertyEditor.cs @@ -45,8 +45,11 @@ namespace Umbraco.Web.PropertyEditors if (string.IsNullOrEmpty(asString)) yield break; - if (Udi.TryParse(asString, out var udi)) - yield return new UmbracoEntityReference(udi); + foreach (var udiStr in asString.Split(',')) + { + if (Udi.TryParse(udiStr, out var udi)) + yield return new UmbracoEntityReference(udi); + } } } } From d0eee32f1d184da7a8eb8207482ba90c3130bd9c Mon Sep 17 00:00:00 2001 From: mattchanner Date: Wed, 1 Apr 2020 13:12:05 +0100 Subject: [PATCH 20/55] Added null check on raw JSON to prevent null reference error deserializing the grid (cherry picked from commit bcea0f645a1f39b13bedbac0f6b4b6f6b6d6a9e0) --- src/Umbraco.Web/PropertyEditors/GridPropertyEditor.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Umbraco.Web/PropertyEditors/GridPropertyEditor.cs b/src/Umbraco.Web/PropertyEditors/GridPropertyEditor.cs index 7134fe8703..6606e5d100 100644 --- a/src/Umbraco.Web/PropertyEditors/GridPropertyEditor.cs +++ b/src/Umbraco.Web/PropertyEditors/GridPropertyEditor.cs @@ -165,6 +165,10 @@ namespace Umbraco.Web.PropertyEditors public IEnumerable GetReferences(object value) { var rawJson = value == null ? string.Empty : value is string str ? str : value.ToString(); + + if (rawJson.IsNullOrWhiteSpace()) + yield break; + DeserializeGridValue(rawJson, out var richTextEditorValues, out var mediaValues); foreach (var umbracoEntityReference in richTextEditorValues.SelectMany(x => From 188d868f553e7938bf5d5f691e0c877e57ec9091 Mon Sep 17 00:00:00 2001 From: Rachel Breeze Date: Sat, 11 Apr 2020 14:45:14 +0100 Subject: [PATCH 21/55] Updated tinyMCE to 4.9.9 --- src/Umbraco.Web.UI.Client/bower.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Web.UI.Client/bower.json b/src/Umbraco.Web.UI.Client/bower.json index b173909c63..ab438e1d8d 100644 --- a/src/Umbraco.Web.UI.Client/bower.json +++ b/src/Umbraco.Web.UI.Client/bower.json @@ -25,7 +25,7 @@ "jquery-migrate": "1.4.0", "angular-dynamic-locale": "0.1.28", "ng-file-upload": "~7.3.8", - "tinymce": "~4.9.4", + "tinymce": "~4.9.9", "codemirror": "~5.3.0", "angular-local-storage": "~0.2.3", "moment": "~2.10.3", From 3b03d812f87a332b7ea61be9c1289665fc04ff6b Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 15 Apr 2020 11:55:12 +1000 Subject: [PATCH 22/55] fixes paging --- .../Persistence/Repositories/ContentRepository.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs index 24c9e84873..11b56c5bf0 100644 --- a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs @@ -862,21 +862,21 @@ order by (umbracoNode.{2}), (umbracoNode.parentID), (umbracoNode.sortOrder)", XmlElement last = null; - long pageSize = 500; + const long pageSize = 500; int? itemCount = null; - long currPage = 0; + long pageIndex = 0; do { // Get the paged queries - Database.BuildPageQueries(currPage, pageSize, sql, ref args, out var sqlCount, out var sqlPage); + Database.BuildPageQueries(pageIndex * pageSize, pageSize, sql, ref args, out var sqlCount, out var sqlPage); // get the item count once if (itemCount == null) { itemCount = Database.ExecuteScalar(sqlCount, args); } - currPage++; + pageIndex++; // iterate over rows without allocating all items to memory (Query vs Fetch) foreach (var row in Database.Query(sqlPage, args)) @@ -906,7 +906,7 @@ order by (umbracoNode.{2}), (umbracoNode.parentID), (umbracoNode.sortOrder)", last.Attributes["sortOrder"].Value = sortOrder.ToInvariantString(); } - } while (itemCount == pageSize); + } while ((pageIndex * pageSize) < itemCount); return xmlDoc; From 61c39eea64c9fe7a1de4a9cb38fb89834dc754df Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 15 Apr 2020 12:49:38 +1000 Subject: [PATCH 23/55] Nicer sql template constants --- src/Umbraco.Core/Constants-SqlTemplates.cs | 17 ++++++++++------- .../Implement/ContentRepositoryBase.cs | 14 +++++++------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/Umbraco.Core/Constants-SqlTemplates.cs b/src/Umbraco.Core/Constants-SqlTemplates.cs index 33c94f0f93..984bc495b0 100644 --- a/src/Umbraco.Core/Constants-SqlTemplates.cs +++ b/src/Umbraco.Core/Constants-SqlTemplates.cs @@ -4,13 +4,16 @@ { public static class SqlTemplates { - public const string VersionableRepositoryGetVersionIds = "Umbraco.Core.VersionableRepository.GetVersionIds"; - public const string VersionableRepositoryGetVersion = "Umbraco.Core.VersionableRepository.GetVersion"; - public const string VersionableRepositoryGetVersions = "Umbraco.Core.VersionableRepository.GetVersions"; - public const string VersionableRepositoryEnsureUniqueNodeName = "Umbraco.Core.VersionableRepository.EnsureUniqueNodeName"; - public const string VersionableRepositoryGetSortOrder = "Umbraco.Core.VersionableRepository.GetSortOrder"; - public const string VersionableRepositoryGetParentNode = "Umbraco.Core.VersionableRepository.GetParentNode"; - public const string VersionableRepositoryGetReservedId = "Umbraco.Core.VersionableRepository.GetReservedId"; + public static class VersionableRepository + { + public const string GetVersionIds = "Umbraco.Core.VersionableRepository.GetVersionIds"; + public const string GetVersion = "Umbraco.Core.VersionableRepository.GetVersion"; + public const string GetVersions = "Umbraco.Core.VersionableRepository.GetVersions"; + public const string EnsureUniqueNodeName = "Umbraco.Core.VersionableRepository.EnsureUniqueNodeName"; + public const string GetSortOrder = "Umbraco.Core.VersionableRepository.GetSortOrder"; + public const string GetParentNode = "Umbraco.Core.VersionableRepository.GetParentNode"; + public const string GetReservedId = "Umbraco.Core.VersionableRepository.GetReservedId"; + } } } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs index 4aa9655249..845006891d 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -83,7 +83,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // gets all version ids, current first public virtual IEnumerable GetVersionIds(int nodeId, int maxRows) { - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersionIds, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetVersionIds, tsql => tsql.Select(x => x.Id) .From() .Where(x => x.NodeId == SqlTemplate.Arg("nodeId")) @@ -99,7 +99,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // TODO: test object node type? // get the version we want to delete - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersion, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetVersion, tsql => tsql.Select().From().Where(x => x.Id == SqlTemplate.Arg("versionId")) ); var versionDto = Database.Fetch(template.Sql(new { versionId })).FirstOrDefault(); @@ -121,7 +121,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // TODO: test object node type? // get the versions we want to delete, excluding the current one - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetVersions, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetVersions, tsql => tsql.Select().From().Where(x => x.NodeId == SqlTemplate.Arg("nodeId") && !x.Current && x.VersionDate < SqlTemplate.Arg("versionDate")) ); var versionDtos = Database.Fetch(template.Sql(new { nodeId, versionDate })); @@ -887,7 +887,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual string EnsureUniqueNodeName(int parentId, string nodeName, int id = 0) { - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryEnsureUniqueNodeName, tsql => tsql + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.EnsureUniqueNodeName, tsql => tsql .Select(x => Alias(x.NodeId, "id"), x => Alias(x.Text, "name")) .From() .Where(x => x.NodeObjectType == SqlTemplate.Arg("nodeObjectType") && x.ParentId == SqlTemplate.Arg("parentId"))); @@ -900,7 +900,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual int GetNewChildSortOrder(int parentId, int first) { - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetSortOrder, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetSortOrder, tsql => tsql.Select($"COALESCE(MAX(sortOrder),{first - 1})").From().Where(x => x.ParentId == SqlTemplate.Arg("parentId") && x.NodeObjectType == NodeObjectTypeId) ); @@ -909,7 +909,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual NodeDto GetParentNodeDto(int parentId) { - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetParentNode, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetParentNode, tsql => tsql.Select().From().Where(x => x.NodeId == SqlTemplate.Arg("parentId")) ); @@ -918,7 +918,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected virtual int GetReservedId(Guid uniqueId) { - var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepositoryGetReservedId, tsql => + var template = SqlContext.Templates.Get(Constants.SqlTemplates.VersionableRepository.GetReservedId, tsql => tsql.Select(x => x.NodeId).From().Where(x => x.UniqueId == SqlTemplate.Arg("uniqueId") && x.NodeObjectType == Constants.ObjectTypes.IdReservation) ); var id = Database.ExecuteScalar(template.Sql(new { uniqueId = uniqueId })); From 055f18d6398761ecdb69a8faee8c91d61e58c05a Mon Sep 17 00:00:00 2001 From: Sebastiaan Janssen Date: Wed, 15 Apr 2020 09:52:07 +0200 Subject: [PATCH 24/55] Add back ctor to revert a breaking change --- src/Umbraco.Core/Models/RelationType.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Core/Models/RelationType.cs b/src/Umbraco.Core/Models/RelationType.cs index 1085ecdcdd..05caf79a99 100644 --- a/src/Umbraco.Core/Models/RelationType.cs +++ b/src/Umbraco.Core/Models/RelationType.cs @@ -1,5 +1,6 @@ using System; using System.Runtime.Serialization; +using Umbraco.Core.Exceptions; using Umbraco.Core.Models.Entities; namespace Umbraco.Core.Models @@ -17,8 +18,6 @@ namespace Umbraco.Core.Models private Guid? _parentObjectType; private Guid? _childObjectType; - //TODO: Should we put back the broken ctors with obsolete attributes? - public RelationType(string alias, string name) : this(name, alias, false, null, null) { @@ -33,6 +32,15 @@ namespace Umbraco.Core.Models _childObjectType = childObjectType; } + [Obsolete("This constructor is incomplete, use one of the other constructors instead")] + public RelationType(Guid childObjectType, Guid parentObjectType, string alias) + { + if (string.IsNullOrWhiteSpace(alias)) throw new ArgumentNullOrEmptyException(nameof(alias)); + _childObjectType = childObjectType; + _parentObjectType = parentObjectType; + _alias = alias; + Name = _alias; + } /// From aab629808d522760323d2f562760a2f20ff0dbc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Wed, 15 Apr 2020 15:08:10 +0200 Subject: [PATCH 25/55] slightly more shadow on content-grid items --- .../src/less/components/umb-content-grid.less | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Web.UI.Client/src/less/components/umb-content-grid.less b/src/Umbraco.Web.UI.Client/src/less/components/umb-content-grid.less index 622dcb8b0a..47fc8a10b9 100644 --- a/src/Umbraco.Web.UI.Client/src/less/components/umb-content-grid.less +++ b/src/Umbraco.Web.UI.Client/src/less/components/umb-content-grid.less @@ -11,7 +11,7 @@ cursor: pointer; position: relative; user-select: none; - box-shadow: 0 1px 1px 0 rgba(0,0,0,0.16); + box-shadow: 0 1px 2px 0 rgba(0,0,0,0.16); border-radius: 3px; } From 627243c84cc3381b3062f56e367926400da137d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Wed, 15 Apr 2020 16:43:59 +0200 Subject: [PATCH 26/55] show message when user has no start nodes --- .../src/common/services/events.service.js | 2 -- src/Umbraco.Web.UI.Client/src/init.js | 1 + src/Umbraco.Web.UI.Client/src/main.controller.js | 11 ++++++----- .../src/views/common/login.controller.js | 7 ------- .../src/views/components/application/umb-login.html | 3 ++- 5 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/services/events.service.js b/src/Umbraco.Web.UI.Client/src/common/services/events.service.js index f90936f371..965ac3d635 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/events.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/events.service.js @@ -18,8 +18,6 @@ function eventsService($q, $rootScope) { /** raise an event with a given name */ emit: function (name, args) { - console.log(`Emitting event: ${name}`, args); - //there are no listeners if (!$rootScope.$$listeners[name]) { return; diff --git a/src/Umbraco.Web.UI.Client/src/init.js b/src/Umbraco.Web.UI.Client/src/init.js index f46b24a546..c5a20cd890 100644 --- a/src/Umbraco.Web.UI.Client/src/init.js +++ b/src/Umbraco.Web.UI.Client/src/init.js @@ -23,6 +23,7 @@ app.run(['$rootScope', '$route', '$location', 'urlHelper', 'navigationService', if(user.startContentIds.length === 0 && user.startMediaIds.length === 0){ const args = { isTimedOut: true, noAccess: true }; eventsService.emit("app.notAuthenticated", args); + return; } assetsService._loadInitAssets().then(function () { diff --git a/src/Umbraco.Web.UI.Client/src/main.controller.js b/src/Umbraco.Web.UI.Client/src/main.controller.js index 4fac328f70..54d897ca74 100644 --- a/src/Umbraco.Web.UI.Client/src/main.controller.js +++ b/src/Umbraco.Web.UI.Client/src/main.controller.js @@ -56,9 +56,11 @@ function MainController($scope, $location, appState, treeService, notificationsS appState.setSearchState("show", false); }; - $scope.showLoginScreen = function(isTimedOut) { - console.log('SHOW ME THE LOGIN SCREEN'); + $scope.showLoginScreen = function(isTimedOut, noAccess) { + console.log('SHOW ME THE LOGIN SCREEN', isTimedOut, noAccess); + $scope.login.isTimedOut = isTimedOut; + $scope.login.noAccess = noAccess; $scope.login.show = true; }; @@ -73,10 +75,9 @@ function MainController($scope, $location, appState, treeService, notificationsS $scope.authenticated = null; $scope.user = null; const isTimedOut = data && data.isTimedOut ? true : false; + const noAccess = data && data.noAccess ? true : false; - console.log('Log me out & show login screen?', isTimedOut); - - $scope.showLoginScreen(isTimedOut); + $scope.showLoginScreen(isTimedOut, noAccess); // Remove the localstorage items for tours shown // Means that when next logged in they can be re-shown if not already dismissed etc diff --git a/src/Umbraco.Web.UI.Client/src/views/common/login.controller.js b/src/Umbraco.Web.UI.Client/src/views/common/login.controller.js index 713af9661b..86132fe8f3 100644 --- a/src/Umbraco.Web.UI.Client/src/views/common/login.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/common/login.controller.js @@ -17,13 +17,6 @@ angular.module('umbraco').controller("Umbraco.LoginController", function (events $location.url(path); }); - eventsService.on("app.notAuthenticated", function(evt, data){ - console.log('not authenticated event back', data); - if(data.noAccess){ - alert('NO NO NO YOU HAVE NO START NODES'); - } - }); - $scope.$on('$destroy', function () { eventsService.unsubscribe(evtOn); }); diff --git a/src/Umbraco.Web.UI.Client/src/views/components/application/umb-login.html b/src/Umbraco.Web.UI.Client/src/views/components/application/umb-login.html index 2e81395643..c26e3daa8a 100644 --- a/src/Umbraco.Web.UI.Client/src/views/components/application/umb-login.html +++ b/src/Umbraco.Web.UI.Client/src/views/components/application/umb-login.html @@ -120,7 +120,8 @@

    - Log in below. + Session timed out. + User has no start-nodes.

    From b870ca7fcf0d8b10c35e9b3e8f7115136c762acf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Wed, 15 Apr 2020 17:46:12 +0200 Subject: [PATCH 27/55] revert --- src/Umbraco.Web.UI.Client/src/init.js | 8 -------- .../src/views/components/application/umb-login.html | 3 +-- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/init.js b/src/Umbraco.Web.UI.Client/src/init.js index c5a20cd890..d5c5166d21 100644 --- a/src/Umbraco.Web.UI.Client/src/init.js +++ b/src/Umbraco.Web.UI.Client/src/init.js @@ -18,14 +18,6 @@ app.run(['$rootScope', '$route', '$location', 'urlHelper', 'navigationService', /** Listens for authentication and checks if our required assets are loaded, if/once they are we'll broadcast a ready event */ eventsService.on("app.authenticated", function (evt, data) { - // Lets check if the auth'd user has a start node set (befor trying to make the app ready) - const user = data.user; - if(user.startContentIds.length === 0 && user.startMediaIds.length === 0){ - const args = { isTimedOut: true, noAccess: true }; - eventsService.emit("app.notAuthenticated", args); - return; - } - assetsService._loadInitAssets().then(function () { appReady(data); diff --git a/src/Umbraco.Web.UI.Client/src/views/components/application/umb-login.html b/src/Umbraco.Web.UI.Client/src/views/components/application/umb-login.html index c26e3daa8a..2e81395643 100644 --- a/src/Umbraco.Web.UI.Client/src/views/components/application/umb-login.html +++ b/src/Umbraco.Web.UI.Client/src/views/components/application/umb-login.html @@ -120,8 +120,7 @@

    - Session timed out. - User has no start-nodes. + Log in below.

    From 61cb9208085e00b4360e0590f8239c6abd54ef42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Wed, 15 Apr 2020 17:46:28 +0200 Subject: [PATCH 28/55] show message if user has no start-nodes --- .../src/common/services/user.service.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/services/user.service.js b/src/Umbraco.Web.UI.Client/src/common/services/user.service.js index afd7b606e7..45a819317c 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/user.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/user.service.js @@ -185,7 +185,19 @@ angular.module('umbraco.services') authenticate: function (login, password) { return authResource.performLogin(login, password) - .then(this.setAuthenticationSuccessful); + .then(function(data) { + + // Check if user has a start node set. + if(data.startContentIds.length === 0 && data.startMediaIds.length === 0){ + var errorMsg = "User has no start-nodes"; + var result = { errorMsg: errorMsg, user: data, authenticated: false, lastUserId: lastUserId, loginType: "credentials" };; + eventsService.emit("app.notAuthenticated", result); + throw result; + } + + return this.setAuthenticationSuccessful(data); + + }); }, setAuthenticationSuccessful: function (data) { From 0b4b28851e6ab3b82285c04ef07701f55dbfd9c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Wed, 15 Apr 2020 17:47:32 +0200 Subject: [PATCH 29/55] revert --- src/Umbraco.Web.UI.Client/src/main.controller.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/main.controller.js b/src/Umbraco.Web.UI.Client/src/main.controller.js index 54d897ca74..81eadf150f 100644 --- a/src/Umbraco.Web.UI.Client/src/main.controller.js +++ b/src/Umbraco.Web.UI.Client/src/main.controller.js @@ -56,11 +56,8 @@ function MainController($scope, $location, appState, treeService, notificationsS appState.setSearchState("show", false); }; - $scope.showLoginScreen = function(isTimedOut, noAccess) { - console.log('SHOW ME THE LOGIN SCREEN', isTimedOut, noAccess); - + $scope.showLoginScreen = function(isTimedOut) { $scope.login.isTimedOut = isTimedOut; - $scope.login.noAccess = noAccess; $scope.login.show = true; }; @@ -75,9 +72,8 @@ function MainController($scope, $location, appState, treeService, notificationsS $scope.authenticated = null; $scope.user = null; const isTimedOut = data && data.isTimedOut ? true : false; - const noAccess = data && data.noAccess ? true : false; - $scope.showLoginScreen(isTimedOut, noAccess); + $scope.showLoginScreen(isTimedOut); // Remove the localstorage items for tours shown // Means that when next logged in they can be re-shown if not already dismissed etc From 3f22d1c452db197250336be47f1a32eee850ec9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Wed, 15 Apr 2020 17:57:38 +0200 Subject: [PATCH 30/55] correcting promise --- .../src/common/services/user.service.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/services/user.service.js b/src/Umbraco.Web.UI.Client/src/common/services/user.service.js index 45a819317c..5b4e516289 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/user.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/user.service.js @@ -190,14 +190,14 @@ angular.module('umbraco.services') // Check if user has a start node set. if(data.startContentIds.length === 0 && data.startMediaIds.length === 0){ var errorMsg = "User has no start-nodes"; - var result = { errorMsg: errorMsg, user: data, authenticated: false, lastUserId: lastUserId, loginType: "credentials" };; + var result = { errorMsg: errorMsg, user: data, authenticated: false, lastUserId: lastUserId, loginType: "credentials" }; eventsService.emit("app.notAuthenticated", result); throw result; } - return this.setAuthenticationSuccessful(data); + return data; - }); + }).then(this.setAuthenticationSuccessful); }, setAuthenticationSuccessful: function (data) { From 4a071f6e059c77dc14d3f0554e19d0ee7a5516da Mon Sep 17 00:00:00 2001 From: Shannon Deminick Date: Thu, 16 Apr 2020 04:51:13 +1000 Subject: [PATCH 31/55] Nucache NullReferenceException when copying (#7961) (cherry picked from commit b30db05cc7a1b69c7e9d9990ea2677f18473bae3) # Conflicts: # src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs --- .../PublishedContent/NuCacheChildrenTests.cs | 113 ++++++++++++++---- .../PublishedCache/NuCache/ContentStore.cs | 6 +- 2 files changed, 91 insertions(+), 28 deletions(-) diff --git a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs index cc9ffdba3c..6f79b12d4c 100644 --- a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs +++ b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs @@ -47,7 +47,7 @@ namespace Umbraco.Tests.PublishedContent _snapshotService?.Dispose(); } - private void Init(IEnumerable kits) + private void Init(Func> kits) { Current.Reset(); @@ -136,7 +136,7 @@ namespace Umbraco.Tests.PublishedContent _snapshotAccessor = new TestPublishedSnapshotAccessor(); // create a data source for NuCache - _source = new TestDataSource(kits); + _source = new TestDataSource(kits()); // at last, create the complete NuCache snapshot service! var options = new PublishedSnapshotServiceOptions { IgnoreLocalDb = true }; @@ -374,7 +374,7 @@ namespace Umbraco.Tests.PublishedContent [Test] public void EmptyTest() { - Init(Enumerable.Empty()); + Init(() => Enumerable.Empty()); var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); _snapshotAccessor.PublishedSnapshot = snapshot; @@ -386,7 +386,7 @@ namespace Umbraco.Tests.PublishedContent [Test] public void ChildrenTest() { - Init(GetInvariantKits()); + Init(GetInvariantKits); var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); _snapshotAccessor.PublishedSnapshot = snapshot; @@ -413,7 +413,7 @@ namespace Umbraco.Tests.PublishedContent [Test] public void ParentTest() { - Init(GetInvariantKits()); + Init(GetInvariantKits); var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); _snapshotAccessor.PublishedSnapshot = snapshot; @@ -439,7 +439,7 @@ namespace Umbraco.Tests.PublishedContent [Test] public void MoveToRootTest() { - Init(GetInvariantKits()); + Init(GetInvariantKits); // get snapshot var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); @@ -481,7 +481,7 @@ namespace Umbraco.Tests.PublishedContent [Test] public void MoveFromRootTest() { - Init(GetInvariantKits()); + Init(GetInvariantKits); // get snapshot var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); @@ -523,7 +523,7 @@ namespace Umbraco.Tests.PublishedContent [Test] public void ReOrderTest() { - Init(GetInvariantKits()); + Init(GetInvariantKits); // get snapshot var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); @@ -598,7 +598,7 @@ namespace Umbraco.Tests.PublishedContent [Test] public void MoveTest() { - Init(GetInvariantKits()); + Init(GetInvariantKits); // get snapshot var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); @@ -699,11 +699,61 @@ namespace Umbraco.Tests.PublishedContent Assert.AreEqual(1, snapshot.Content.GetById(7).Parent?.Id); } + [Test] + public void Clear_Branch_Locked() + { + // This test replicates an issue we saw here https://github.com/umbraco/Umbraco-CMS/pull/7907#issuecomment-610259393 + // The data was sent to me and this replicates it's structure + + var paths = new Dictionary { { -1, "-1" } }; + + Init(() => new List + { + CreateInvariantKit(1, -1, 1, paths), // first level + CreateInvariantKit(2, 1, 1, paths), // second level + CreateInvariantKit(3, 2, 1, paths), // third level + + CreateInvariantKit(4, 3, 1, paths), // fourth level (we'll copy this one to the same level) + + CreateInvariantKit(5, 4, 1, paths), // 6th level + + CreateInvariantKit(6, 5, 2, paths), // 7th level + CreateInvariantKit(7, 5, 3, paths), + CreateInvariantKit(8, 5, 4, paths), + CreateInvariantKit(9, 5, 5, paths), + CreateInvariantKit(10, 5, 6, paths) + }); + + // get snapshot + var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); + _snapshotAccessor.PublishedSnapshot = snapshot; + + var snapshotService = (PublishedSnapshotService)_snapshotService; + var contentStore = snapshotService.GetContentStore(); + //This will set a flag to force creating a new Gen next time the store is locked (i.e. In Notify) + contentStore.CreateSnapshot(); + + // notify - which ensures there are 2 generations in the cache meaning each LinkedNode has a Next value. + _snapshotService.Notify(new[] + { + new ContentCacheRefresher.JsonPayload(4, Guid.Empty, TreeChangeTypes.RefreshBranch) + }, out _, out _); + + // refresh the branch again, this used to show the issue where a null ref exception would occur + // because in the ClearBranchLocked logic, when SetValueLocked was called within a recursive call + // to a child, we null out the .Value of the LinkedNode within the while loop because we didn't capture + // this value before recursing. + Assert.DoesNotThrow(() => + _snapshotService.Notify(new[] + { + new ContentCacheRefresher.JsonPayload(4, Guid.Empty, TreeChangeTypes.RefreshBranch) + }, out _, out _)); + } + [Test] public void NestedVariationChildrenTest() { - var mixedKits = GetNestedVariantKits(); - Init(mixedKits); + Init(GetNestedVariantKits); var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); _snapshotAccessor.PublishedSnapshot = snapshot; @@ -792,7 +842,7 @@ namespace Umbraco.Tests.PublishedContent [Test] public void VariantChildrenTest() { - Init(GetVariantKits()); + Init(GetVariantKits); var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); _snapshotAccessor.PublishedSnapshot = snapshot; @@ -864,7 +914,7 @@ namespace Umbraco.Tests.PublishedContent [Test] public void RemoveTest() { - Init(GetInvariantKits()); + Init(GetInvariantKits); var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); _snapshotAccessor.PublishedSnapshot = snapshot; @@ -913,7 +963,7 @@ namespace Umbraco.Tests.PublishedContent [Test] public void UpdateTest() { - Init(GetInvariantKits()); + Init(GetInvariantKits); var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); _snapshotAccessor.PublishedSnapshot = snapshot; @@ -960,13 +1010,13 @@ namespace Umbraco.Tests.PublishedContent documents = snapshot.Content.GetById(2).Children().ToArray(); AssertDocuments(documents, "N9", "N8", "N7"); - + } [Test] public void AtRootTest() { - Init(GetVariantWithDraftKits()); + Init(GetVariantWithDraftKits); var snapshot = _snapshotService.CreatePublishedSnapshot(previewToken: null); _snapshotAccessor.PublishedSnapshot = snapshot; @@ -995,7 +1045,7 @@ namespace Umbraco.Tests.PublishedContent yield return CreateInvariantKit(2, 1, 1, paths); } - Init(GetKits()); + Init(GetKits); var snapshotService = (PublishedSnapshotService)_snapshotService; var contentStore = snapshotService.GetContentStore(); @@ -1034,7 +1084,7 @@ namespace Umbraco.Tests.PublishedContent yield return CreateInvariantKit(4, 1, 3, paths); } - Init(GetKits()); + Init(GetKits); var snapshotService = (PublishedSnapshotService)_snapshotService; var contentStore = snapshotService.GetContentStore(); @@ -1114,7 +1164,7 @@ namespace Umbraco.Tests.PublishedContent yield return CreateInvariantKit(40, 1, 3, paths); } - Init(GetKits()); + Init(GetKits); var snapshotService = (PublishedSnapshotService)_snapshotService; var contentStore = snapshotService.GetContentStore(); @@ -1133,7 +1183,7 @@ namespace Umbraco.Tests.PublishedContent _snapshotService.Notify(new[] { - new ContentCacheRefresher.JsonPayload(1, Guid.Empty, TreeChangeTypes.RefreshNode) + new ContentCacheRefresher.JsonPayload(1, Guid.Empty, TreeChangeTypes.RefreshNode) }, out _, out _); Assert.AreEqual(2, contentStore.Test.LiveGen); @@ -1181,12 +1231,12 @@ namespace Umbraco.Tests.PublishedContent //children of 1 yield return CreateInvariantKit(20, 1, 1, paths); - yield return CreateInvariantKit(30, 1, 2, paths); + yield return CreateInvariantKit(30, 1, 2, paths); yield return CreateInvariantKit(40, 1, 3, paths); } //init with all published - Init(GetKits()); + Init(GetKits); var snapshotService = (PublishedSnapshotService)_snapshotService; var contentStore = snapshotService.GetContentStore(); @@ -1203,7 +1253,7 @@ namespace Umbraco.Tests.PublishedContent //Change the root publish flag var kit = rootKit.Clone(); kit.DraftData = published ? null : kit.PublishedData; - kit.PublishedData = published? kit.PublishedData : null; + kit.PublishedData = published ? kit.PublishedData : null; _source.Kits[1] = kit; _snapshotService.Notify(new[] @@ -1218,12 +1268,12 @@ namespace Umbraco.Tests.PublishedContent var (gen, contentNode) = contentStore.Test.GetValues(1)[0]; Assert.AreEqual(assertGen, gen); //even when unpublishing/re-publishing/etc... the linked list is always maintained - AssertLinkedNode(contentNode, 100, 2, 3, 20, 40); + AssertLinkedNode(contentNode, 100, 2, 3, 20, 40); } //unpublish the root ChangePublishFlagOfRoot(false, 2, TreeChangeTypes.RefreshBranch); - + //publish the root (since it's not published, it will cause a RefreshBranch) ChangePublishFlagOfRoot(true, 3, TreeChangeTypes.RefreshBranch); @@ -1256,7 +1306,7 @@ namespace Umbraco.Tests.PublishedContent yield return CreateInvariantKit(4, 1, 3, paths); } - Init(GetKits()); + Init(GetKits); var snapshotService = (PublishedSnapshotService)_snapshotService; var contentStore = snapshotService.GetContentStore(); @@ -1312,6 +1362,17 @@ namespace Umbraco.Tests.PublishedContent AssertLinkedNode(child3.contentNode, 1, 3, -1, -1, -1); } + [Test] + public void MultipleCacheIteration() + { + //see https://github.com/umbraco/Umbraco-CMS/issues/7798 + Init(GetInvariantKits); + var snapshot = this._snapshotService.CreatePublishedSnapshot(previewToken: null); + _snapshotAccessor.PublishedSnapshot = snapshot; + + var items = snapshot.Content.GetByXPath("/root/itype"); + Assert.AreEqual(items.Count(), items.Count()); + } private void AssertLinkedNode(ContentNode node, int parent, int prevSibling, int nextSibling, int firstChild, int lastChild) { Assert.AreEqual(parent, node.ParentContentId); diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index f92d8adebb..34d21497a2 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -873,9 +873,11 @@ namespace Umbraco.Web.PublishedCache.NuCache var id = content.FirstChildContentId; while (id > 0) { + // get the required link node, this ensures that both `link` and `link.Value` are not null var link = GetRequiredLinkedNode(id, "child", null); - ClearBranchLocked(link.Value); - id = link.Value.NextSiblingContentId; + var linkValue = link.Value; // capture local since clearing in recurse can clear it + ClearBranchLocked(linkValue); // recurse + id = linkValue.NextSiblingContentId; } } From 0d7b0b96746a473a9e79f3246c32e05ad0e759da Mon Sep 17 00:00:00 2001 From: Sebastiaan Janssen Date: Wed, 15 Apr 2020 21:11:14 +0200 Subject: [PATCH 32/55] Add back ctor to revert a breaking change --- src/Umbraco.Core/Models/RelationType.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Umbraco.Core/Models/RelationType.cs b/src/Umbraco.Core/Models/RelationType.cs index 05caf79a99..7975c39a2e 100644 --- a/src/Umbraco.Core/Models/RelationType.cs +++ b/src/Umbraco.Core/Models/RelationType.cs @@ -42,6 +42,13 @@ namespace Umbraco.Core.Models Name = _alias; } + [Obsolete("This constructor is incomplete, use one of the other constructors instead")] + public RelationType(Guid childObjectType, Guid parentObjectType, string alias, string name) + : this(childObjectType, parentObjectType, alias) + { + if (string.IsNullOrWhiteSpace(name)) throw new ArgumentNullOrEmptyException(nameof(name)); + Name = name; + } /// /// Gets or sets the Name of the RelationType From b72abd50a094abe5f7f7ca3fb16be3cf890c5729 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 17:47:43 +1000 Subject: [PATCH 33/55] fixes relation type ctor --- src/Umbraco.Core/Models/RelationType.cs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Core/Models/RelationType.cs b/src/Umbraco.Core/Models/RelationType.cs index 7975c39a2e..fafe37dfa8 100644 --- a/src/Umbraco.Core/Models/RelationType.cs +++ b/src/Umbraco.Core/Models/RelationType.cs @@ -18,6 +18,7 @@ namespace Umbraco.Core.Models private Guid? _parentObjectType; private Guid? _childObjectType; + [Obsolete("This constructor is no longer used and will be removed in future versions, use one of the other constructors instead")] public RelationType(string alias, string name) : this(name, alias, false, null, null) { @@ -32,21 +33,25 @@ namespace Umbraco.Core.Models _childObjectType = childObjectType; } - [Obsolete("This constructor is incomplete, use one of the other constructors instead")] + [Obsolete("This constructor is no longer used and will be removed in future versions, use one of the other constructors instead")] public RelationType(Guid childObjectType, Guid parentObjectType, string alias) { - if (string.IsNullOrWhiteSpace(alias)) throw new ArgumentNullOrEmptyException(nameof(alias)); + if (alias == null) throw new ArgumentNullException(nameof(alias)); + if (string.IsNullOrWhiteSpace(alias)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(alias)); + _childObjectType = childObjectType; _parentObjectType = parentObjectType; _alias = alias; Name = _alias; } - [Obsolete("This constructor is incomplete, use one of the other constructors instead")] + [Obsolete("This constructor is no longer used and will be removed in future versions, use one of the other constructors instead")] public RelationType(Guid childObjectType, Guid parentObjectType, string alias, string name) : this(childObjectType, parentObjectType, alias) { - if (string.IsNullOrWhiteSpace(name)) throw new ArgumentNullOrEmptyException(nameof(name)); + if (name == null) throw new ArgumentNullException(nameof(name)); + if (string.IsNullOrWhiteSpace(name)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(name)); + Name = name; } From feeff07d5c8d2bdc2471a6f83c3ef63e49be26a0 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 18:39:02 +1000 Subject: [PATCH 34/55] Cherry pick - Possible fix for issue #7870 --- src/Umbraco.Core/PropertyEditors/DataEditor.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Umbraco.Core/PropertyEditors/DataEditor.cs b/src/Umbraco.Core/PropertyEditors/DataEditor.cs index 7dc260e4c7..8a4cec0f48 100644 --- a/src/Umbraco.Core/PropertyEditors/DataEditor.cs +++ b/src/Umbraco.Core/PropertyEditors/DataEditor.cs @@ -19,7 +19,6 @@ namespace Umbraco.Core.PropertyEditors public class DataEditor : IDataEditor { private IDictionary _defaultConfiguration; - private IDataValueEditor _dataValueEditor; /// /// Initializes a new instance of the class. @@ -91,7 +90,7 @@ namespace Umbraco.Core.PropertyEditors /// simple enough for now. /// // TODO: point of that one? shouldn't we always configure? - public IDataValueEditor GetValueEditor() => ExplicitValueEditor ?? (_dataValueEditor ?? (_dataValueEditor = CreateValueEditor())); + public IDataValueEditor GetValueEditor() => ExplicitValueEditor ?? CreateValueEditor(); /// /// From 53824cac25aafe6bc99d6dbc5533fca14badfa29 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 18:46:00 +1000 Subject: [PATCH 35/55] fix isBidirectional spelling --- src/Umbraco.Core/Models/RelationType.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Core/Models/RelationType.cs b/src/Umbraco.Core/Models/RelationType.cs index fafe37dfa8..62091d090d 100644 --- a/src/Umbraco.Core/Models/RelationType.cs +++ b/src/Umbraco.Core/Models/RelationType.cs @@ -14,7 +14,7 @@ namespace Umbraco.Core.Models { private string _name; private string _alias; - private bool _isBidrectional; + private bool _isBidirectional; private Guid? _parentObjectType; private Guid? _childObjectType; @@ -24,11 +24,11 @@ namespace Umbraco.Core.Models { } - public RelationType(string name, string alias, bool isBidrectional, Guid? parentObjectType, Guid? childObjectType) + public RelationType(string name, string alias, bool isBidirectional, Guid? parentObjectType, Guid? childObjectType) { _name = name; _alias = alias; - _isBidrectional = isBidrectional; + _isBidirectional = isBidirectional; _parentObjectType = parentObjectType; _childObjectType = childObjectType; } @@ -81,8 +81,8 @@ namespace Umbraco.Core.Models [DataMember] public bool IsBidirectional { - get => _isBidrectional; - set => SetPropertyValueAndDetectChanges(value, ref _isBidrectional, nameof(IsBidirectional)); + get => _isBidirectional; + set => SetPropertyValueAndDetectChanges(value, ref _isBidirectional, nameof(IsBidirectional)); } /// From d9bf7dfa9689c7d3fea4645d60bb8184ea6d96d6 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 18:53:45 +1000 Subject: [PATCH 36/55] changes back isBidrectional spelling in case someone is using it with a named parameter binding :/ --- src/Umbraco.Core/Models/RelationType.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Core/Models/RelationType.cs b/src/Umbraco.Core/Models/RelationType.cs index 62091d090d..5def14a0c5 100644 --- a/src/Umbraco.Core/Models/RelationType.cs +++ b/src/Umbraco.Core/Models/RelationType.cs @@ -24,11 +24,11 @@ namespace Umbraco.Core.Models { } - public RelationType(string name, string alias, bool isBidirectional, Guid? parentObjectType, Guid? childObjectType) + public RelationType(string name, string alias, bool isBidrectional, Guid? parentObjectType, Guid? childObjectType) { _name = name; _alias = alias; - _isBidirectional = isBidirectional; + _isBidirectional = isBidrectional; _parentObjectType = parentObjectType; _childObjectType = childObjectType; } From b09d736c1bfcb0ce285a118d8860a3d69700441d Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Thu, 16 Apr 2020 13:47:23 +0200 Subject: [PATCH 37/55] Bump verison to 8.6.1 --- src/SolutionInfo.cs | 4 ++-- src/Umbraco.Web.UI/Umbraco.Web.UI.csproj | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/SolutionInfo.cs b/src/SolutionInfo.cs index 1bb53004a0..760c49e436 100644 --- a/src/SolutionInfo.cs +++ b/src/SolutionInfo.cs @@ -18,5 +18,5 @@ using System.Resources; [assembly: AssemblyVersion("8.0.0")] // these are FYI and changed automatically -[assembly: AssemblyFileVersion("8.6.0")] -[assembly: AssemblyInformationalVersion("8.6.0")] +[assembly: AssemblyFileVersion("8.6.1")] +[assembly: AssemblyInformationalVersion("8.6.1")] diff --git a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj index cc6200cea7..d4a82331f5 100644 --- a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj +++ b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj @@ -345,9 +345,9 @@ False True - 8600 + 8610 / - http://localhost:8600 + http://localhost:8610 False False @@ -429,4 +429,4 @@ - \ No newline at end of file + From 6cd7e5d05558bb30a33434c685d43a1982a9a7a5 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 22:04:27 +1000 Subject: [PATCH 38/55] removes the extra unneeded variable --- .../Repositories/Implement/MediaRepository.cs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs index b9ef2d4086..242f21c749 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/MediaRepository.cs @@ -219,7 +219,6 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected override void PersistNewItem(IMedia entity) { - var media = entity; entity.AddingEntity(); // ensure unique name on the same level @@ -274,15 +273,15 @@ namespace Umbraco.Core.Persistence.Repositories.Implement contentVersionDto.NodeId = nodeDto.NodeId; contentVersionDto.Current = true; Database.Insert(contentVersionDto); - media.VersionId = contentVersionDto.Id; + entity.VersionId = contentVersionDto.Id; // persist the media version dto var mediaVersionDto = dto.MediaVersionDto; - mediaVersionDto.Id = media.VersionId; + mediaVersionDto.Id = entity.VersionId; Database.Insert(mediaVersionDto); // persist the property data - var propertyDataDtos = PropertyFactory.BuildDtos(media.ContentType.Variations, media.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); + var propertyDataDtos = PropertyFactory.BuildDtos(entity.ContentType.Variations, entity.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); foreach (var propertyDataDto in propertyDataDtos) Database.Insert(propertyDataDto); @@ -298,10 +297,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected override void PersistUpdatedItem(IMedia entity) { - var media = entity; - // update - media.UpdatingEntity(); + entity.UpdatingEntity(); // Check if this entity is being moved as a descendant as part of a bulk moving operations. // In this case we can bypass a lot of the below operations which will make this whole operation go much faster. @@ -349,9 +346,9 @@ namespace Umbraco.Core.Persistence.Repositories.Implement Database.Update(mediaVersionDto); // replace the property data - var deletePropertyDataSql = SqlContext.Sql().Delete().Where(x => x.VersionId == media.VersionId); + var deletePropertyDataSql = SqlContext.Sql().Delete().Where(x => x.VersionId == entity.VersionId); Database.Execute(deletePropertyDataSql); - var propertyDataDtos = PropertyFactory.BuildDtos(media.ContentType.Variations, media.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); + var propertyDataDtos = PropertyFactory.BuildDtos(entity.ContentType.Variations, entity.VersionId, 0, entity.Properties, LanguageRepository, out _, out _); foreach (var propertyDataDto in propertyDataDtos) Database.Insert(propertyDataDto); From d2c05bf681724aef2779d2ace62ff98fd5ca865a Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 22:27:05 +1000 Subject: [PATCH 39/55] Fixes issue with RelationTypeFactory swapping the parent/child --- .../Persistence/Factories/RelationTypeFactory.cs | 2 +- .../Repositories/RelationTypeRepositoryTest.cs | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Factories/RelationTypeFactory.cs b/src/Umbraco.Core/Persistence/Factories/RelationTypeFactory.cs index edd87fec68..177a0494a2 100644 --- a/src/Umbraco.Core/Persistence/Factories/RelationTypeFactory.cs +++ b/src/Umbraco.Core/Persistence/Factories/RelationTypeFactory.cs @@ -9,7 +9,7 @@ namespace Umbraco.Core.Persistence.Factories public static IRelationType BuildEntity(RelationTypeDto dto) { - var entity = new RelationType(dto.Name, dto.Alias, dto.Dual, dto.ChildObjectType, dto.ParentObjectType); + var entity = new RelationType(dto.Name, dto.Alias, dto.Dual, dto.ParentObjectType, dto.ChildObjectType); try { diff --git a/src/Umbraco.Tests/Persistence/Repositories/RelationTypeRepositoryTest.cs b/src/Umbraco.Tests/Persistence/Repositories/RelationTypeRepositoryTest.cs index 962737e1dc..ab06434e34 100644 --- a/src/Umbraco.Tests/Persistence/Repositories/RelationTypeRepositoryTest.cs +++ b/src/Umbraco.Tests/Persistence/Repositories/RelationTypeRepositoryTest.cs @@ -107,13 +107,16 @@ namespace Umbraco.Tests.Persistence.Repositories var repository = CreateRepository(provider); // Act - var relationType = repository.Get(RelationTypeDto.NodeIdSeed); + var relationType = repository.Get(RelationTypeDto.NodeIdSeed + 2); // Assert Assert.That(relationType, Is.Not.Null); Assert.That(relationType.HasIdentity, Is.True); - Assert.That(relationType.Alias, Is.EqualTo("relateContentOnCopy")); - Assert.That(relationType.Name, Is.EqualTo("Relate Content on Copy")); + Assert.That(relationType.IsBidirectional, Is.True); + Assert.That(relationType.Alias, Is.EqualTo("relateContentToMedia")); + Assert.That(relationType.Name, Is.EqualTo("Relate Content to Media")); + Assert.That(relationType.ChildObjectType, Is.EqualTo(Constants.ObjectTypes.Media)); + Assert.That(relationType.ParentObjectType, Is.EqualTo(Constants.ObjectTypes.Document)); } } @@ -224,8 +227,9 @@ namespace Umbraco.Tests.Persistence.Repositories public void CreateTestData() { - var relateContent = new RelationType("Relate Content on Copy", "relateContentOnCopy", true, Constants.ObjectTypes.Document, new Guid("C66BA18E-EAF3-4CFF-8A22-41B16D66A972")); - var relateContentType = new RelationType("Relate ContentType on Copy", "relateContentTypeOnCopy", true, Constants.ObjectTypes.DocumentType, new Guid("A2CB7800-F571-4787-9638-BC48539A0EFB")); + var relateContent = new RelationType("Relate Content on Copy", "relateContentOnCopy", true, Constants.ObjectTypes.Document, Constants.ObjectTypes.Document); + var relateContentType = new RelationType("Relate ContentType on Copy", "relateContentTypeOnCopy", true, Constants.ObjectTypes.DocumentType, Constants.ObjectTypes.DocumentType); + var relateContentMedia = new RelationType("Relate Content to Media", "relateContentToMedia", true, Constants.ObjectTypes.Document, Constants.ObjectTypes.Media); var provider = TestObjects.GetScopeProvider(Logger); using (var scope = ScopeProvider.CreateScope()) @@ -234,6 +238,7 @@ namespace Umbraco.Tests.Persistence.Repositories repository.Save(relateContent);//Id 2 repository.Save(relateContentType);//Id 3 + repository.Save(relateContentMedia);//Id 4 scope.Complete(); } } From 6a5366469e6cd054632f104a21e0a7b5e704b172 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 22:34:15 +1000 Subject: [PATCH 40/55] fixes null checks in ctor --- src/Umbraco.Core/Models/RelationType.cs | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/Umbraco.Core/Models/RelationType.cs b/src/Umbraco.Core/Models/RelationType.cs index 5def14a0c5..16e689f719 100644 --- a/src/Umbraco.Core/Models/RelationType.cs +++ b/src/Umbraco.Core/Models/RelationType.cs @@ -20,12 +20,17 @@ namespace Umbraco.Core.Models [Obsolete("This constructor is no longer used and will be removed in future versions, use one of the other constructors instead")] public RelationType(string alias, string name) - : this(name, alias, false, null, null) + : this(name: name, alias: alias, false, null, null) { } public RelationType(string name, string alias, bool isBidrectional, Guid? parentObjectType, Guid? childObjectType) { + if (name == null) throw new ArgumentNullException(nameof(alias)); + if (string.IsNullOrWhiteSpace(name)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(name)); + if (alias == null) throw new ArgumentNullException(nameof(alias)); + if (string.IsNullOrWhiteSpace(alias)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(alias)); + _name = name; _alias = alias; _isBidirectional = isBidrectional; @@ -35,24 +40,14 @@ namespace Umbraco.Core.Models [Obsolete("This constructor is no longer used and will be removed in future versions, use one of the other constructors instead")] public RelationType(Guid childObjectType, Guid parentObjectType, string alias) - { - if (alias == null) throw new ArgumentNullException(nameof(alias)); - if (string.IsNullOrWhiteSpace(alias)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(alias)); - - _childObjectType = childObjectType; - _parentObjectType = parentObjectType; - _alias = alias; - Name = _alias; + : this(name: alias, alias: alias, false, parentObjectType: parentObjectType, childObjectType: childObjectType) + { } [Obsolete("This constructor is no longer used and will be removed in future versions, use one of the other constructors instead")] public RelationType(Guid childObjectType, Guid parentObjectType, string alias, string name) - : this(childObjectType, parentObjectType, alias) + : this(name: name, alias: alias, false, parentObjectType: parentObjectType, childObjectType: childObjectType) { - if (name == null) throw new ArgumentNullException(nameof(name)); - if (string.IsNullOrWhiteSpace(name)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(name)); - - Name = name; } /// From 153daa9fc5f3f6d8f2bde7c5a03213ee16cefb68 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 22:35:53 +1000 Subject: [PATCH 41/55] fixes nameof --- src/Umbraco.Core/Models/RelationType.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Models/RelationType.cs b/src/Umbraco.Core/Models/RelationType.cs index 16e689f719..8cee2dccf2 100644 --- a/src/Umbraco.Core/Models/RelationType.cs +++ b/src/Umbraco.Core/Models/RelationType.cs @@ -26,7 +26,7 @@ namespace Umbraco.Core.Models public RelationType(string name, string alias, bool isBidrectional, Guid? parentObjectType, Guid? childObjectType) { - if (name == null) throw new ArgumentNullException(nameof(alias)); + if (name == null) throw new ArgumentNullException(nameof(name)); if (string.IsNullOrWhiteSpace(name)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(name)); if (alias == null) throw new ArgumentNullException(nameof(alias)); if (string.IsNullOrWhiteSpace(alias)) throw new ArgumentException("Value can't be empty or consist only of white-space characters.", nameof(alias)); From eabc9583f05b33c38d1383f654506284d61a7690 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 22:39:32 +1000 Subject: [PATCH 42/55] adds more named params --- src/Umbraco.Core/Models/RelationType.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Core/Models/RelationType.cs b/src/Umbraco.Core/Models/RelationType.cs index 8cee2dccf2..25156cdb8a 100644 --- a/src/Umbraco.Core/Models/RelationType.cs +++ b/src/Umbraco.Core/Models/RelationType.cs @@ -40,13 +40,13 @@ namespace Umbraco.Core.Models [Obsolete("This constructor is no longer used and will be removed in future versions, use one of the other constructors instead")] public RelationType(Guid childObjectType, Guid parentObjectType, string alias) - : this(name: alias, alias: alias, false, parentObjectType: parentObjectType, childObjectType: childObjectType) + : this(name: alias, alias: alias, isBidrectional: false, parentObjectType: parentObjectType, childObjectType: childObjectType) { } [Obsolete("This constructor is no longer used and will be removed in future versions, use one of the other constructors instead")] public RelationType(Guid childObjectType, Guid parentObjectType, string alias, string name) - : this(name: name, alias: alias, false, parentObjectType: parentObjectType, childObjectType: childObjectType) + : this(name: name, alias: alias, isBidrectional: false, parentObjectType: parentObjectType, childObjectType: childObjectType) { } From 41f98a41fbc928113d703d2fb09d0b4cea26695f Mon Sep 17 00:00:00 2001 From: Sebastiaan Janssen Date: Thu, 16 Apr 2020 14:58:36 +0200 Subject: [PATCH 43/55] Added fix from #7832 since the test was somehow cherry picked into 8.6.1 already --- src/Umbraco.Web/PublishedCache/NuCache/ContentCache.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentCache.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentCache.cs index 84edb9113c..24c6a7018b 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentCache.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentCache.cs @@ -355,6 +355,7 @@ namespace Umbraco.Web.PublishedCache.NuCache private static IEnumerable GetByXPath(XPathNodeIterator iterator) { + iterator = iterator.Clone(); while (iterator.MoveNext()) { var xnav = iterator.Current as NavigableNavigator; From 271669581a200670916c470793816222c4c121d3 Mon Sep 17 00:00:00 2001 From: Sebastiaan Janssen Date: Thu, 16 Apr 2020 15:21:54 +0200 Subject: [PATCH 44/55] Merge pull request #7847 from umbraco/v8/bugfix/7773-memory-leak Fixes Memory Leak in GetCacheItem cache dependency #7773 (cherry picked from commit b8abc0441783bc161151360e24949fa09590a0be) --- src/Umbraco.Core/Cache/WebCachingAppCache.cs | 26 +++++++++----------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/Umbraco.Core/Cache/WebCachingAppCache.cs b/src/Umbraco.Core/Cache/WebCachingAppCache.cs index c6e104221a..f9be8c13a8 100644 --- a/src/Umbraco.Core/Cache/WebCachingAppCache.cs +++ b/src/Umbraco.Core/Cache/WebCachingAppCache.cs @@ -37,23 +37,13 @@ namespace Umbraco.Core.Cache /// public object Get(string key, Func factory, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - CacheDependency dependency = null; - if (dependentFiles != null && dependentFiles.Any()) - { - dependency = new CacheDependency(dependentFiles); - } - return Get(key, factory, timeout, isSliding, priority, removedCallback, dependency); + return GetInternal(key, factory, timeout, isSliding, priority, removedCallback, dependentFiles); } /// public void Insert(string key, Func factory, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - CacheDependency dependency = null; - if (dependentFiles != null && dependentFiles.Any()) - { - dependency = new CacheDependency(dependentFiles); - } - Insert(key, factory, timeout, isSliding, priority, removedCallback, dependency); + InsertInternal(key, factory, timeout, isSliding, priority, removedCallback, dependentFiles); } #region Dictionary @@ -103,7 +93,7 @@ namespace Umbraco.Core.Cache #endregion - private object Get(string key, Func factory, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, CacheDependency dependency = null) + private object GetInternal(string key, Func factory, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { key = GetCacheKey(key); @@ -163,6 +153,10 @@ namespace Umbraco.Core.Cache var sliding = isSliding == false ? System.Web.Caching.Cache.NoSlidingExpiration : (timeout ?? System.Web.Caching.Cache.NoSlidingExpiration); lck.UpgradeToWriteLock(); + + // create a cache dependency if one is needed. + var dependency = dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null; + //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! _cache.Insert(key, result, dependency, absolute, sliding, priority, removedCallback); } @@ -180,7 +174,7 @@ namespace Umbraco.Core.Cache return value; } - private void Insert(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, CacheDependency dependency = null) + private void InsertInternal(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { // NOTE - here also we must insert a Lazy but we can evaluate it right now // and make sure we don't store a null value. @@ -197,6 +191,10 @@ namespace Umbraco.Core.Cache try { _locker.EnterWriteLock(); + + // create a cache dependency if one is needed. + var dependency = dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null; + //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); } From 3310ef9d8d392be2a57f227e33c53c002df9966d Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 23:53:52 +1000 Subject: [PATCH 45/55] fix tests --- .../Persistence/Repositories/RelationTypeRepositoryTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Tests/Persistence/Repositories/RelationTypeRepositoryTest.cs b/src/Umbraco.Tests/Persistence/Repositories/RelationTypeRepositoryTest.cs index ab06434e34..edde8e8f81 100644 --- a/src/Umbraco.Tests/Persistence/Repositories/RelationTypeRepositoryTest.cs +++ b/src/Umbraco.Tests/Persistence/Repositories/RelationTypeRepositoryTest.cs @@ -136,7 +136,7 @@ namespace Umbraco.Tests.Persistence.Repositories Assert.That(relationTypes, Is.Not.Null); Assert.That(relationTypes.Any(), Is.True); Assert.That(relationTypes.Any(x => x == null), Is.False); - Assert.That(relationTypes.Count(), Is.EqualTo(7)); + Assert.That(relationTypes.Count(), Is.EqualTo(8)); } } @@ -193,7 +193,7 @@ namespace Umbraco.Tests.Persistence.Repositories int count = repository.Count(query); // Assert - Assert.That(count, Is.EqualTo(5)); + Assert.That(count, Is.EqualTo(6)); } } From 52ecd59c11742cb6015773f5d68b9f0f795a4f54 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Apr 2020 00:01:26 +1000 Subject: [PATCH 46/55] fixes test --- src/Umbraco.Tests/Services/RelationServiceTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Tests/Services/RelationServiceTests.cs b/src/Umbraco.Tests/Services/RelationServiceTests.cs index 2ec10811b7..06de405cec 100644 --- a/src/Umbraco.Tests/Services/RelationServiceTests.cs +++ b/src/Umbraco.Tests/Services/RelationServiceTests.cs @@ -110,8 +110,8 @@ namespace Umbraco.Tests.Services Assert.AreEqual("Test", rt.Name); Assert.AreEqual("repeatedEventOccurence", rt.Alias); Assert.AreEqual(false, rt.IsBidirectional); - Assert.AreEqual(Constants.ObjectTypes.Document, rt.ChildObjectType.Value); - Assert.AreEqual(Constants.ObjectTypes.Media, rt.ParentObjectType.Value); + Assert.AreEqual(Constants.ObjectTypes.Document, rt.ParentObjectType.Value); + Assert.AreEqual(Constants.ObjectTypes.Media, rt.ChildObjectType.Value); } [Test] From ee6d3e0478ff958663a733be3003b0330d119fd7 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Apr 2020 00:14:05 +1000 Subject: [PATCH 47/55] Removes the alpha builds of examine from nuget.config --- NuGet.Config | 1 - 1 file changed, 1 deletion(-) diff --git a/NuGet.Config b/NuGet.Config index 7d786702f4..31fd84e456 100644 --- a/NuGet.Config +++ b/NuGet.Config @@ -7,6 +7,5 @@ --> - From 5d8bfcea979d1582af1dc3f7ed973107b16368e5 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Apr 2020 00:47:26 +1000 Subject: [PATCH 48/55] Ensure when we are loading in ALL data that we page over the data as to not cause an SQL timeout --- .../Persistence/NPocoDatabaseExtensions.cs | 42 +++++++++++++ .../NuCache/DataSource/DatabaseDataSource.cs | 63 ++++++++++++------- 2 files changed, 82 insertions(+), 23 deletions(-) diff --git a/src/Umbraco.Core/Persistence/NPocoDatabaseExtensions.cs b/src/Umbraco.Core/Persistence/NPocoDatabaseExtensions.cs index acfa51f895..152dcbe6d3 100644 --- a/src/Umbraco.Core/Persistence/NPocoDatabaseExtensions.cs +++ b/src/Umbraco.Core/Persistence/NPocoDatabaseExtensions.cs @@ -18,6 +18,48 @@ namespace Umbraco.Core.Persistence /// public static partial class NPocoDatabaseExtensions { + /// + /// Iterates over the result of a paged data set with a db reader + /// + /// + /// + /// + /// The number of rows to load per page + /// + /// + /// + /// + /// NPoco's normal Page returns a List{T} but sometimes we don't want all that in memory and instead want to + /// iterate over each row with a reader using Query vs Fetch. + /// + internal static IEnumerable QueryPaged(this IDatabase database, long pageSize, Sql sql) + { + var sqlString = sql.SQL; + var sqlArgs = sql.Arguments; + + int? itemCount = null; + long pageIndex = 0; + do + { + // Get the paged queries + database.BuildPageQueries(pageIndex * pageSize, pageSize, sqlString, ref sqlArgs, out var sqlCount, out var sqlPage); + + // get the item count once + if (itemCount == null) + { + itemCount = database.ExecuteScalar(sqlCount, sqlArgs); + } + pageIndex++; + + // iterate over rows without allocating all items to memory (Query vs Fetch) + foreach (var row in database.Query(sqlPage, sqlArgs)) + { + yield return row; + } + + } while ((pageIndex * pageSize) < itemCount); + } + // NOTE // // proper way to do it with TSQL and SQLCE diff --git a/src/Umbraco.Web/PublishedCache/NuCache/DataSource/DatabaseDataSource.cs b/src/Umbraco.Web/PublishedCache/NuCache/DataSource/DatabaseDataSource.cs index 19aab7ea65..694dac04df 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/DataSource/DatabaseDataSource.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/DataSource/DatabaseDataSource.cs @@ -20,6 +20,8 @@ namespace Umbraco.Web.PublishedCache.NuCache.DataSource // provides efficient database access for NuCache internal class DatabaseDataSource : IDataSource { + private const int PageSize = 500; + // we want arrays, we want them all loaded, not an enumerable private Sql ContentSourcesSelect(IScope scope, Func, Sql> joins = null) @@ -79,33 +81,43 @@ namespace Umbraco.Web.PublishedCache.NuCache.DataSource .Where(x => x.NodeObjectType == Constants.ObjectTypes.Document && !x.Trashed) .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); - return scope.Database.Query(sql).Select(CreateContentNodeKit); + // We need to page here. We don't want to iterate over every single row in one connection cuz this can cause an SQL Timeout. + // We also want to read with a db reader and not load everything into memory, QueryPaged lets us do that. + + foreach (var row in scope.Database.QueryPaged(PageSize, sql)) + yield return CreateContentNodeKit(row); } public IEnumerable GetBranchContentSources(IScope scope, int id) { var syntax = scope.SqlContext.SqlSyntax; - var sql = ContentSourcesSelect(scope, s => s + var sql = ContentSourcesSelect(scope, + s => s.InnerJoin("x").On((left, right) => left.NodeId == right.NodeId || SqlText(left.Path, right.Path, (lp, rp) => $"({lp} LIKE {syntax.GetConcat(rp, "',%'")})"), aliasRight: "x")) + .Where(x => x.NodeObjectType == Constants.ObjectTypes.Document && !x.Trashed) + .Where(x => x.NodeId == id, "x") + .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); - .InnerJoin("x").On((left, right) => left.NodeId == right.NodeId || SqlText(left.Path, right.Path, (lp, rp) => $"({lp} LIKE {syntax.GetConcat(rp, "',%'")})"), aliasRight: "x")) + // We need to page here. We don't want to iterate over every single row in one connection cuz this can cause an SQL Timeout. + // We also want to read with a db reader and not load everything into memory, QueryPaged lets us do that. - .Where(x => x.NodeObjectType == Constants.ObjectTypes.Document && !x.Trashed) - .Where(x => x.NodeId == id, "x") - .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); - - return scope.Database.Query(sql).Select(CreateContentNodeKit); + foreach (var row in scope.Database.QueryPaged(PageSize, sql)) + yield return CreateContentNodeKit(row); } public IEnumerable GetTypeContentSources(IScope scope, IEnumerable ids) { - if (!ids.Any()) return Enumerable.Empty(); + if (!ids.Any()) yield break; var sql = ContentSourcesSelect(scope) .Where(x => x.NodeObjectType == Constants.ObjectTypes.Document && !x.Trashed) .WhereIn(x => x.ContentTypeId, ids) .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); - return scope.Database.Query(sql).Select(CreateContentNodeKit); + // We need to page here. We don't want to iterate over every single row in one connection cuz this can cause an SQL Timeout. + // We also want to read with a db reader and not load everything into memory, QueryPaged lets us do that. + + foreach (var row in scope.Database.QueryPaged(PageSize, sql)) + yield return CreateContentNodeKit(row); } private Sql MediaSourcesSelect(IScope scope, Func, Sql> joins = null) @@ -116,11 +128,8 @@ namespace Umbraco.Web.PublishedCache.NuCache.DataSource x => Alias(x.Level, "Level"), x => Alias(x.Path, "Path"), x => Alias(x.SortOrder, "SortOrder"), x => Alias(x.ParentId, "ParentId"), x => Alias(x.CreateDate, "CreateDate"), x => Alias(x.UserId, "CreatorId")) .AndSelect(x => Alias(x.ContentTypeId, "ContentTypeId")) - .AndSelect(x => Alias(x.Id, "VersionId"), x => Alias(x.Text, "EditName"), x => Alias(x.VersionDate, "EditVersionDate"), x => Alias(x.UserId, "EditWriterId")) - .AndSelect("nuEdit", x => Alias(x.Data, "EditData")) - .From(); if (joins != null) @@ -128,9 +137,7 @@ namespace Umbraco.Web.PublishedCache.NuCache.DataSource sql = sql .InnerJoin().On((left, right) => left.NodeId == right.NodeId) - .InnerJoin().On((left, right) => left.NodeId == right.NodeId && right.Current) - .LeftJoin("nuEdit").On((left, right) => left.NodeId == right.NodeId && !right.Published, aliasRight: "nuEdit"); return sql; @@ -152,33 +159,43 @@ namespace Umbraco.Web.PublishedCache.NuCache.DataSource .Where(x => x.NodeObjectType == Constants.ObjectTypes.Media && !x.Trashed) .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); - return scope.Database.Query(sql).Select(CreateMediaNodeKit); + // We need to page here. We don't want to iterate over every single row in one connection cuz this can cause an SQL Timeout. + // We also want to read with a db reader and not load everything into memory, QueryPaged lets us do that. + + foreach (var row in scope.Database.QueryPaged(PageSize, sql)) + yield return CreateMediaNodeKit(row); } public IEnumerable GetBranchMediaSources(IScope scope, int id) { var syntax = scope.SqlContext.SqlSyntax; - var sql = MediaSourcesSelect(scope, s => s - - .InnerJoin("x").On((left, right) => left.NodeId == right.NodeId || SqlText(left.Path, right.Path, (lp, rp) => $"({lp} LIKE {syntax.GetConcat(rp, "',%'")})"), aliasRight: "x")) - + var sql = MediaSourcesSelect(scope, + s => s.InnerJoin("x").On((left, right) => left.NodeId == right.NodeId || SqlText(left.Path, right.Path, (lp, rp) => $"({lp} LIKE {syntax.GetConcat(rp, "',%'")})"), aliasRight: "x")) .Where(x => x.NodeObjectType == Constants.ObjectTypes.Media && !x.Trashed) .Where(x => x.NodeId == id, "x") .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); - return scope.Database.Query(sql).Select(CreateMediaNodeKit); + // We need to page here. We don't want to iterate over every single row in one connection cuz this can cause an SQL Timeout. + // We also want to read with a db reader and not load everything into memory, QueryPaged lets us do that. + + foreach (var row in scope.Database.QueryPaged(PageSize, sql)) + yield return CreateMediaNodeKit(row); } public IEnumerable GetTypeMediaSources(IScope scope, IEnumerable ids) { - if (!ids.Any()) return Enumerable.Empty(); + if (!ids.Any()) yield break; var sql = MediaSourcesSelect(scope) .Where(x => x.NodeObjectType == Constants.ObjectTypes.Media && !x.Trashed) .WhereIn(x => x.ContentTypeId, ids) .OrderBy(x => x.Level, x => x.ParentId, x => x.SortOrder); - return scope.Database.Query(sql).Select(CreateMediaNodeKit); + // We need to page here. We don't want to iterate over every single row in one connection cuz this can cause an SQL Timeout. + // We also want to read with a db reader and not load everything into memory, QueryPaged lets us do that. + + foreach (var row in scope.Database.QueryPaged(PageSize, sql)) + yield return CreateMediaNodeKit(row); } private static ContentNodeKit CreateContentNodeKit(ContentSourceDto dto) From d5e2c12685e46e55b360c385b97eb424855b54ed Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Apr 2020 13:20:57 +1000 Subject: [PATCH 49/55] fixes merge issue --- src/Umbraco.Web.UI/Umbraco.Web.UI.csproj | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj index 57a71a2cf0..1d77505fc6 100644 --- a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj +++ b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj @@ -347,10 +347,7 @@ True 8700 / - http://localhost:8700 - 8610 - / - http://localhost:8610 + http://localhost:8700 False False From 2509dc3749dac1ce44071f59a0ea2a8a21f27024 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Apr 2020 14:46:45 +1000 Subject: [PATCH 50/55] runs in parallel the call to rebuild the in-memory cache from the db sources when in pure live mode and content types change --- .../NuCache/PublishedSnapshotService.cs | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index a33d9ee427..4e630cad3d 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -5,6 +5,7 @@ using System.Globalization; using System.IO; using System.Linq; using System.Threading; +using System.Threading.Tasks; using CSharpTest.Net.Collections; using Newtonsoft.Json; using Umbraco.Core; @@ -866,12 +867,24 @@ namespace Umbraco.Web.PublishedCache.NuCache //into a new DLL for the application which includes both content types and media types. //Since the models in the cache are based on these actual classes, all of the objects in the cache need to be updated //to use the newest version of the class. - using (_contentStore.GetScopedWriteLock(_scopeProvider)) - using (_mediaStore.GetScopedWriteLock(_scopeProvider)) - { - NotifyLocked(new[] { new ContentCacheRefresher.JsonPayload(0, null, TreeChangeTypes.RefreshAll) }, out var draftChanged, out var publishedChanged); - NotifyLocked(new[] { new MediaCacheRefresher.JsonPayload(0, null, TreeChangeTypes.RefreshAll) }, out var anythingChanged); - } + + // These can be run side by side in parallel + + Parallel.Invoke( + () => + { + using (_contentStore.GetScopedWriteLock(_scopeProvider)) + { + NotifyLocked(new[] { new ContentCacheRefresher.JsonPayload(0, null, TreeChangeTypes.RefreshAll) }, out _, out _); + } + }, + () => + { + using (_mediaStore.GetScopedWriteLock(_scopeProvider)) + { + NotifyLocked(new[] { new MediaCacheRefresher.JsonPayload(0, null, TreeChangeTypes.RefreshAll) }, out _); + } + }); } ((PublishedSnapshot)CurrentPublishedSnapshot)?.Resync(); From 49da58b23c064b6cdeb31e4d5c858e09da351095 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Apr 2020 14:56:49 +1000 Subject: [PATCH 51/55] adds notes --- .../NuCache/PublishedSnapshotService.cs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index 4e630cad3d..6866878484 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -868,7 +868,20 @@ namespace Umbraco.Web.PublishedCache.NuCache //Since the models in the cache are based on these actual classes, all of the objects in the cache need to be updated //to use the newest version of the class. - // These can be run side by side in parallel + // NOTE: Ideally this can be run on background threads here which would prevent blocking the UI + // as is the case when saving a content type. Intially one would think that it won't be any different + // between running this here or in another background thread immediately after with regards to how the + // UI will respond because we already know between calling `WithSafeLiveFactoryReset` to reset the PureLive models + // and this code here, that many front-end requests could be attempted to be processed. If that is the case, those pages are going to get a + // model binding error and our ModelBindingExceptionFilter is going to to its magic to reload those pages so the end user is none the wiser. + // So whether or not this executes 'here' or on a background thread immediately wouldn't seem to make any difference except that we can return + // execution to the UI sooner. + // BUT!... there is a difference IIRC. There is still execution logic that continues after this call on this thread with the cache refreshers + // and those cache refreshers need to have the up-to-date data since other user cache refreshers will be expecting the data to be 'live'. If + // we ran this on a background thread then those cache refreshers are going to not get 'live' data when they query the content cache which + // they require. + + // These can be run side by side in parallel. Parallel.Invoke( () => From 008df6018c5483886e6707a97ab0204c84f19f76 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 21 Apr 2020 00:02:59 +1000 Subject: [PATCH 52/55] Fixes: SqlMainDomLock when configured via appSettings cannot be used unless umbraco is installed #7967 --- src/Umbraco.Core/Runtime/SqlMainDomLock.cs | 76 +++++++++++++--------- 1 file changed, 46 insertions(+), 30 deletions(-) diff --git a/src/Umbraco.Core/Runtime/SqlMainDomLock.cs b/src/Umbraco.Core/Runtime/SqlMainDomLock.cs index 4433a8e307..f3bfe4eefc 100644 --- a/src/Umbraco.Core/Runtime/SqlMainDomLock.cs +++ b/src/Umbraco.Core/Runtime/SqlMainDomLock.cs @@ -32,6 +32,7 @@ namespace Umbraco.Core.Runtime // unique id for our appdomain, this is more unique than the appdomain id which is just an INT counter to its safer _lockId = Guid.NewGuid().ToString(); _logger = logger; + _dbFactory = new UmbracoDatabaseFactory( Constants.System.UmbracoConnectionName, _logger, @@ -40,6 +41,12 @@ namespace Umbraco.Core.Runtime public async Task AcquireLockAsync(int millisecondsTimeout) { + if (!_dbFactory.Configured) + { + // if we aren't configured, then we're in an install state, in which case we have no choice but to assume we can acquire + return true; + } + if (!(_dbFactory.SqlContext.SqlSyntax is SqlServerSyntaxProvider sqlServerSyntaxProvider)) throw new NotSupportedException("SqlMainDomLock is only supported for Sql Server"); @@ -126,6 +133,12 @@ namespace Umbraco.Core.Runtime // poll every 1 second Thread.Sleep(1000); + if (!_dbFactory.Configured) + { + // if we aren't configured, we just keep looping since we can't query the db + continue; + } + lock (_locker) { // If cancellation has been requested we will just exit. Depending on timing of the shutdown, @@ -358,41 +371,44 @@ namespace Umbraco.Core.Runtime _cancellationTokenSource.Cancel(); _cancellationTokenSource.Dispose(); - var db = GetDatabase(); - try + if (_dbFactory.Configured) { - db.BeginTransaction(IsolationLevel.ReadCommitted); - - // get a write lock - _sqlServerSyntax.WriteLock(db, Constants.Locks.MainDom); - - // When we are disposed, it means we have released the MainDom lock - // and called all MainDom release callbacks, in this case - // if another maindom is actually coming online we need - // to signal to the MainDom coming online that we have shutdown. - // To do that, we update the existing main dom DB record with a suffixed "_updated" string. - // Otherwise, if we are just shutting down, we want to just delete the row. - if (_mainDomChanging) + var db = GetDatabase(); + try { - _logger.Debug("Releasing MainDom, updating row, new application is booting."); - db.Execute($"UPDATE umbracoKeyValue SET [value] = [value] + '{UpdatedSuffix}' WHERE [key] = @key", new { key = MainDomKey }); + db.BeginTransaction(IsolationLevel.ReadCommitted); + + // get a write lock + _sqlServerSyntax.WriteLock(db, Constants.Locks.MainDom); + + // When we are disposed, it means we have released the MainDom lock + // and called all MainDom release callbacks, in this case + // if another maindom is actually coming online we need + // to signal to the MainDom coming online that we have shutdown. + // To do that, we update the existing main dom DB record with a suffixed "_updated" string. + // Otherwise, if we are just shutting down, we want to just delete the row. + if (_mainDomChanging) + { + _logger.Debug("Releasing MainDom, updating row, new application is booting."); + db.Execute($"UPDATE umbracoKeyValue SET [value] = [value] + '{UpdatedSuffix}' WHERE [key] = @key", new { key = MainDomKey }); + } + else + { + _logger.Debug("Releasing MainDom, deleting row, application is shutting down."); + db.Execute("DELETE FROM umbracoKeyValue WHERE [key] = @key", new { key = MainDomKey }); + } } - else + catch (Exception ex) { - _logger.Debug("Releasing MainDom, deleting row, application is shutting down."); - db.Execute("DELETE FROM umbracoKeyValue WHERE [key] = @key", new { key = MainDomKey }); + ResetDatabase(); + _logger.Error(ex, "Unexpected error during dipsose."); + _hasError = true; + } + finally + { + db?.CompleteTransaction(); + ResetDatabase(); } - } - catch (Exception ex) - { - ResetDatabase(); - _logger.Error(ex, "Unexpected error during dipsose."); - _hasError = true; - } - finally - { - db?.CompleteTransaction(); - ResetDatabase(); } } } From 2a89b36871cbf36d6e30895d7310cd8ff5a412df Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 22 Apr 2020 10:37:23 +1000 Subject: [PATCH 53/55] Fixing Nasty Exception with Scope/Provider #5151 --- src/Umbraco.Examine/ContentValueSetBuilder.cs | 34 +++++++++++++++++-- .../UmbracoExamine/IndexInitializer.cs | 16 ++++++--- src/Umbraco.Tests/UmbracoExamine/IndexTest.cs | 10 +++--- .../UmbracoExamine/SearchTests.cs | 2 +- src/Umbraco.Web/Search/ExamineComposer.cs | 3 ++ 5 files changed, 52 insertions(+), 13 deletions(-) diff --git a/src/Umbraco.Examine/ContentValueSetBuilder.cs b/src/Umbraco.Examine/ContentValueSetBuilder.cs index 9cbc311639..788ddfac4a 100644 --- a/src/Umbraco.Examine/ContentValueSetBuilder.cs +++ b/src/Umbraco.Examine/ContentValueSetBuilder.cs @@ -1,9 +1,13 @@ using Examine; +using System; using System.Collections.Generic; using System.Linq; using Umbraco.Core; +using Umbraco.Core.Composing; using Umbraco.Core.Models; +using Umbraco.Core.Models.Membership; using Umbraco.Core.PropertyEditors; +using Umbraco.Core.Scoping; using Umbraco.Core.Services; using Umbraco.Core.Strings; @@ -16,20 +20,46 @@ namespace Umbraco.Examine { private readonly UrlSegmentProviderCollection _urlSegmentProviders; private readonly IUserService _userService; + private readonly IScopeProvider _scopeProvider; + + [Obsolete("Use the other ctor instead")] + public ContentValueSetBuilder(PropertyEditorCollection propertyEditors, + UrlSegmentProviderCollection urlSegmentProviders, + IUserService userService, + bool publishedValuesOnly) + : this(propertyEditors, urlSegmentProviders, userService, Current.ScopeProvider, publishedValuesOnly) + { + } public ContentValueSetBuilder(PropertyEditorCollection propertyEditors, UrlSegmentProviderCollection urlSegmentProviders, IUserService userService, + IScopeProvider scopeProvider, bool publishedValuesOnly) : base(propertyEditors, publishedValuesOnly) { _urlSegmentProviders = urlSegmentProviders; _userService = userService; + _scopeProvider = scopeProvider; } /// public override IEnumerable GetValueSets(params IContent[] content) { + Dictionary creatorIds; + Dictionary writerIds; + + // We can lookup all of the creator/writer names at once which can save some + // processing below instead of one by one. + using (var scope = _scopeProvider.CreateScope()) + { + creatorIds = content.Select(x => x.CreatorId).Distinct().Select(x => _userService.GetProfileById(x)) + .ToDictionary(x => x.Id, x => x); + writerIds = content.Select(x => x.WriterId).Distinct().Select(x => _userService.GetProfileById(x)) + .ToDictionary(x => x.Id, x => x); + scope.Complete(); + } + // TODO: There is a lot of boxing going on here and ultimately all values will be boxed by Lucene anyways // but I wonder if there's a way to reduce the boxing that we have to do or if it will matter in the end since // Lucene will do it no matter what? One idea was to create a `FieldValue` struct which would contain `object`, `object[]`, `ValueType` and `ValueType[]` @@ -58,8 +88,8 @@ namespace Umbraco.Examine {"urlName", urlValue?.Yield() ?? Enumerable.Empty()}, //Always add invariant urlName {"path", c.Path?.Yield() ?? Enumerable.Empty()}, {"nodeType", c.ContentType.Id.ToString().Yield() ?? Enumerable.Empty()}, - {"creatorName", (c.GetCreatorProfile(_userService)?.Name ?? "??").Yield() }, - {"writerName",(c.GetWriterProfile(_userService)?.Name ?? "??").Yield() }, + {"creatorName", (creatorIds.TryGetValue(c.CreatorId, out var creatorProfile) ? creatorProfile.Name : "??").Yield() }, + {"writerName", (writerIds.TryGetValue(c.CreatorId, out var writerProfile) ? writerProfile.Name : "??").Yield() }, {"writerID", new object[] {c.WriterId}}, {"templateID", new object[] {c.TemplateId ?? 0}}, {UmbracoContentIndex.VariesByCultureFieldName, new object[] {"n"}}, diff --git a/src/Umbraco.Tests/UmbracoExamine/IndexInitializer.cs b/src/Umbraco.Tests/UmbracoExamine/IndexInitializer.cs index 1653de827d..e9f18d8947 100644 --- a/src/Umbraco.Tests/UmbracoExamine/IndexInitializer.cs +++ b/src/Umbraco.Tests/UmbracoExamine/IndexInitializer.cs @@ -30,16 +30,22 @@ namespace Umbraco.Tests.UmbracoExamine /// internal static class IndexInitializer { - public static ContentValueSetBuilder GetContentValueSetBuilder(PropertyEditorCollection propertyEditors, bool publishedValuesOnly) + public static ContentValueSetBuilder GetContentValueSetBuilder(PropertyEditorCollection propertyEditors, IScopeProvider scopeProvider, bool publishedValuesOnly) { - var contentValueSetBuilder = new ContentValueSetBuilder(propertyEditors, new UrlSegmentProviderCollection(new[] { new DefaultUrlSegmentProvider() }), GetMockUserService(), publishedValuesOnly); + var contentValueSetBuilder = new ContentValueSetBuilder( + propertyEditors, + new UrlSegmentProviderCollection(new[] { new DefaultUrlSegmentProvider() }), + GetMockUserService(), + scopeProvider, + publishedValuesOnly); + return contentValueSetBuilder; } - public static ContentIndexPopulator GetContentIndexRebuilder(PropertyEditorCollection propertyEditors, IContentService contentService, ISqlContext sqlContext, bool publishedValuesOnly) + public static ContentIndexPopulator GetContentIndexRebuilder(PropertyEditorCollection propertyEditors, IContentService contentService, IScopeProvider scopeProvider, bool publishedValuesOnly) { - var contentValueSetBuilder = GetContentValueSetBuilder(propertyEditors, publishedValuesOnly); - var contentIndexDataSource = new ContentIndexPopulator(true, null, contentService, sqlContext, contentValueSetBuilder); + var contentValueSetBuilder = GetContentValueSetBuilder(propertyEditors, scopeProvider, publishedValuesOnly); + var contentIndexDataSource = new ContentIndexPopulator(true, null, contentService, scopeProvider.SqlContext, contentValueSetBuilder); return contentIndexDataSource; } diff --git a/src/Umbraco.Tests/UmbracoExamine/IndexTest.cs b/src/Umbraco.Tests/UmbracoExamine/IndexTest.cs index 9e59422310..acb26fb8f6 100644 --- a/src/Umbraco.Tests/UmbracoExamine/IndexTest.cs +++ b/src/Umbraco.Tests/UmbracoExamine/IndexTest.cs @@ -29,7 +29,7 @@ namespace Umbraco.Tests.UmbracoExamine [Test] public void Index_Property_Data_With_Value_Indexer() { - var contentValueSetBuilder = IndexInitializer.GetContentValueSetBuilder(Factory.GetInstance(), false); + var contentValueSetBuilder = IndexInitializer.GetContentValueSetBuilder(Factory.GetInstance(), ScopeProvider, false); using (var luceneDir = new RandomIdRamDirectory()) using (var indexer = IndexInitializer.GetUmbracoIndexer(ProfilingLogger, luceneDir, @@ -121,7 +121,7 @@ namespace Umbraco.Tests.UmbracoExamine [Test] public void Rebuild_Index() { - var contentRebuilder = IndexInitializer.GetContentIndexRebuilder(Factory.GetInstance(), IndexInitializer.GetMockContentService(), ScopeProvider.SqlContext, false); + var contentRebuilder = IndexInitializer.GetContentIndexRebuilder(Factory.GetInstance(), IndexInitializer.GetMockContentService(), ScopeProvider, false); var mediaRebuilder = IndexInitializer.GetMediaIndexRebuilder(Factory.GetInstance(), IndexInitializer.GetMockMediaService()); using (var luceneDir = new RandomIdRamDirectory()) @@ -149,7 +149,7 @@ namespace Umbraco.Tests.UmbracoExamine [Test] public void Index_Protected_Content_Not_Indexed() { - var rebuilder = IndexInitializer.GetContentIndexRebuilder(Factory.GetInstance(), IndexInitializer.GetMockContentService(), ScopeProvider.SqlContext, false); + var rebuilder = IndexInitializer.GetContentIndexRebuilder(Factory.GetInstance(), IndexInitializer.GetMockContentService(), ScopeProvider, false); using (var luceneDir = new RandomIdRamDirectory()) @@ -274,7 +274,7 @@ namespace Umbraco.Tests.UmbracoExamine [Test] public void Index_Reindex_Content() { - var rebuilder = IndexInitializer.GetContentIndexRebuilder(Factory.GetInstance(), IndexInitializer.GetMockContentService(), ScopeProvider.SqlContext, false); + var rebuilder = IndexInitializer.GetContentIndexRebuilder(Factory.GetInstance(), IndexInitializer.GetMockContentService(), ScopeProvider, false); using (var luceneDir = new RandomIdRamDirectory()) using (var indexer = IndexInitializer.GetUmbracoIndexer(ProfilingLogger, luceneDir, validator: new ContentValueSetValidator(false))) @@ -315,7 +315,7 @@ namespace Umbraco.Tests.UmbracoExamine public void Index_Delete_Index_Item_Ensure_Heirarchy_Removed() { - var rebuilder = IndexInitializer.GetContentIndexRebuilder(Factory.GetInstance(), IndexInitializer.GetMockContentService(), ScopeProvider.SqlContext, false); + var rebuilder = IndexInitializer.GetContentIndexRebuilder(Factory.GetInstance(), IndexInitializer.GetMockContentService(), ScopeProvider, false); using (var luceneDir = new RandomIdRamDirectory()) using (var indexer = IndexInitializer.GetUmbracoIndexer(ProfilingLogger, luceneDir)) diff --git a/src/Umbraco.Tests/UmbracoExamine/SearchTests.cs b/src/Umbraco.Tests/UmbracoExamine/SearchTests.cs index a45a33ec00..96e8892cd1 100644 --- a/src/Umbraco.Tests/UmbracoExamine/SearchTests.cs +++ b/src/Umbraco.Tests/UmbracoExamine/SearchTests.cs @@ -55,7 +55,7 @@ namespace Umbraco.Tests.UmbracoExamine allRecs); var propertyEditors = Factory.GetInstance(); - var rebuilder = IndexInitializer.GetContentIndexRebuilder(propertyEditors, contentService, ScopeProvider.SqlContext, true); + var rebuilder = IndexInitializer.GetContentIndexRebuilder(propertyEditors, contentService, ScopeProvider, true); using (var luceneDir = new RandomIdRamDirectory()) using (var indexer = IndexInitializer.GetUmbracoIndexer(ProfilingLogger, luceneDir)) diff --git a/src/Umbraco.Web/Search/ExamineComposer.cs b/src/Umbraco.Web/Search/ExamineComposer.cs index b30f0cbe03..64eeb6978a 100644 --- a/src/Umbraco.Web/Search/ExamineComposer.cs +++ b/src/Umbraco.Web/Search/ExamineComposer.cs @@ -4,6 +4,7 @@ using Umbraco.Core; using Umbraco.Core.Composing; using Umbraco.Core.Models; using Umbraco.Core.PropertyEditors; +using Umbraco.Core.Scoping; using Umbraco.Core.Services; using Umbraco.Core.Strings; using Umbraco.Examine; @@ -36,12 +37,14 @@ namespace Umbraco.Web.Search factory.GetInstance(), factory.GetInstance(), factory.GetInstance(), + factory.GetInstance(), true)); composition.RegisterUnique(factory => new ContentValueSetBuilder( factory.GetInstance(), factory.GetInstance(), factory.GetInstance(), + factory.GetInstance(), false)); composition.RegisterUnique, MediaValueSetBuilder>(); composition.RegisterUnique, MemberValueSetBuilder>(); From c58831925e516c247d6c74642e56ad26cb3cabff Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Wed, 22 Apr 2020 16:20:49 +0200 Subject: [PATCH 54/55] 5151 - Fix using writerId instead of creatorId --- src/Umbraco.Examine/ContentValueSetBuilder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Examine/ContentValueSetBuilder.cs b/src/Umbraco.Examine/ContentValueSetBuilder.cs index 788ddfac4a..8ff556a470 100644 --- a/src/Umbraco.Examine/ContentValueSetBuilder.cs +++ b/src/Umbraco.Examine/ContentValueSetBuilder.cs @@ -89,7 +89,7 @@ namespace Umbraco.Examine {"path", c.Path?.Yield() ?? Enumerable.Empty()}, {"nodeType", c.ContentType.Id.ToString().Yield() ?? Enumerable.Empty()}, {"creatorName", (creatorIds.TryGetValue(c.CreatorId, out var creatorProfile) ? creatorProfile.Name : "??").Yield() }, - {"writerName", (writerIds.TryGetValue(c.CreatorId, out var writerProfile) ? writerProfile.Name : "??").Yield() }, + {"writerName", (writerIds.TryGetValue(c.WriterId, out var writerProfile) ? writerProfile.Name : "??").Yield() }, {"writerID", new object[] {c.WriterId}}, {"templateID", new object[] {c.TemplateId ?? 0}}, {UmbracoContentIndex.VariesByCultureFieldName, new object[] {"n"}}, From 27f7d5efae744e9e4d227e3d526ec83113ded80f Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Thu, 23 Apr 2020 08:38:29 +0200 Subject: [PATCH 55/55] 5151 - Added GetProfilesById extension --- .../Services/UserServiceExtensions.cs | 13 +++++++++++++ src/Umbraco.Examine/ContentValueSetBuilder.cs | 4 ++-- src/Umbraco.Tests/Services/UserServiceTests.cs | 18 ++++++++++++++++++ 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Core/Services/UserServiceExtensions.cs b/src/Umbraco.Core/Services/UserServiceExtensions.cs index 82cab07b25..c365f1ccc2 100644 --- a/src/Umbraco.Core/Services/UserServiceExtensions.cs +++ b/src/Umbraco.Core/Services/UserServiceExtensions.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Linq; using System.Web.Security; using Umbraco.Core.Models.Membership; @@ -116,5 +117,17 @@ namespace Umbraco.Core.Services var permissionCollection = userService.GetPermissions(user, nodeId); return permissionCollection.SelectMany(c => c.AssignedPermissions).Distinct().ToArray(); } + + internal static IEnumerable GetProfilesById(this IUserService userService, params int[] ids) + { + var fullUsers = userService.GetUsersById(ids); + + return fullUsers.Select(user => + { + var asProfile = user as IProfile; + return asProfile ?? new UserProfile(user.Id, user.Name); + }); + + } } } diff --git a/src/Umbraco.Examine/ContentValueSetBuilder.cs b/src/Umbraco.Examine/ContentValueSetBuilder.cs index 8ff556a470..b8477a9047 100644 --- a/src/Umbraco.Examine/ContentValueSetBuilder.cs +++ b/src/Umbraco.Examine/ContentValueSetBuilder.cs @@ -53,9 +53,9 @@ namespace Umbraco.Examine // processing below instead of one by one. using (var scope = _scopeProvider.CreateScope()) { - creatorIds = content.Select(x => x.CreatorId).Distinct().Select(x => _userService.GetProfileById(x)) + creatorIds = _userService.GetProfilesById(content.Select(x => x.CreatorId).ToArray()) .ToDictionary(x => x.Id, x => x); - writerIds = content.Select(x => x.WriterId).Distinct().Select(x => _userService.GetProfileById(x)) + writerIds = _userService.GetProfilesById(content.Select(x => x.WriterId).ToArray()) .ToDictionary(x => x.Id, x => x); scope.Complete(); } diff --git a/src/Umbraco.Tests/Services/UserServiceTests.cs b/src/Umbraco.Tests/Services/UserServiceTests.cs index a96385a923..016085c352 100644 --- a/src/Umbraco.Tests/Services/UserServiceTests.cs +++ b/src/Umbraco.Tests/Services/UserServiceTests.cs @@ -924,6 +924,24 @@ namespace Umbraco.Tests.Services Assert.AreEqual(user.Id, profile.Id); } + [Test] + public void Get_By_Profile_Id_Must_return_null_if_user_not_exists() + { + var profile = ServiceContext.UserService.GetProfileById(42); + + // Assert + Assert.IsNull(profile); + } + + [Test] + public void GetProfilesById_Must_empty_if_users_not_exists() + { + var profiles = ServiceContext.UserService.GetProfilesById(42); + + // Assert + CollectionAssert.IsEmpty(profiles); + } + [Test] public void Get_User_By_Username() {