From 3fac40b4b5dbadd0e099badc7f6745478b0393e1 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 10 Feb 2020 20:15:12 +1100 Subject: [PATCH 01/20] 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/20] 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/20] 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/20] 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 50e9f79ae6f878e837fc0d4748a803378c8d0216 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 6 Apr 2020 23:10:52 +1000 Subject: [PATCH 05/20] 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 06/20] 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 07/20] 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 08/20] 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 09/20] 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 10/20] 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 11/20] 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 12/20] 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 13/20] 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 14/20] 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 15/20] 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 188d868f553e7938bf5d5f691e0c877e57ec9091 Mon Sep 17 00:00:00 2001 From: Rachel Breeze Date: Sat, 11 Apr 2020 14:45:14 +0100 Subject: [PATCH 16/20] 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 17/20] 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 18/20] 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 6cd7e5d05558bb30a33434c685d43a1982a9a7a5 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 16 Apr 2020 22:04:27 +1000 Subject: [PATCH 19/20] 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 d5e2c12685e46e55b360c385b97eb424855b54ed Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Apr 2020 13:20:57 +1000 Subject: [PATCH 20/20] 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