From 8be0f683906873f18ce888e866d5a92742c6111e Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 5 Jan 2017 10:29:03 +1100 Subject: [PATCH] Adds logic to validate an entity's path when it is used to GetDescendants, adds logic to fix an entity's path when it's state is going to be persisted when GetDescendants is used (i.e. MoveToRecycleBin), added this logic to Media too, adds unit tests to support --- .../Models/UmbracoEntityExtensions.cs | 72 ++++++++++ src/Umbraco.Core/Services/ContentService.cs | 40 +++++- src/Umbraco.Core/Services/MediaService.cs | 116 ++++++++++------ .../Models/UmbracoEntityTests.cs | 129 ++++++++++++++++++ .../Services/ContentServiceTests.cs | 16 ++- 5 files changed, 318 insertions(+), 55 deletions(-) diff --git a/src/Umbraco.Core/Models/UmbracoEntityExtensions.cs b/src/Umbraco.Core/Models/UmbracoEntityExtensions.cs index 987be2a276..1b4cc20d73 100644 --- a/src/Umbraco.Core/Models/UmbracoEntityExtensions.cs +++ b/src/Umbraco.Core/Models/UmbracoEntityExtensions.cs @@ -4,12 +4,84 @@ using System.Linq; using System.Text; using System.Threading.Tasks; using System.Web.UI.WebControls; +using Umbraco.Core.Logging; using Umbraco.Core.Models.EntityBase; namespace Umbraco.Core.Models { internal static class UmbracoEntityExtensions { + /// + /// Does a quick check on the entity's set path to ensure that it's valid and consistent + /// + /// + /// + public static bool ValidatePath(this IUmbracoEntity entity) + { + //don't validate if it's empty and it has no id + if (entity.HasIdentity == false && entity.Path.IsNullOrWhiteSpace()) + return true; + + if (entity.Path.IsNullOrWhiteSpace()) + return false; + + var pathParts = entity.Path.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries); + if (pathParts.Length < 2) + { + //a path cannot be less than 2 parts, at a minimum it must be root (-1) and it's own id + return false; + } + + if (entity.ParentId != default(int) && pathParts[pathParts.Length - 2] != entity.ParentId.ToInvariantString()) + { + //the 2nd last id in the path must be it's parent id + return false; + } + + return true; + } + + /// + /// This will validate the entity's path and if it's invalid it will fix it, if fixing is required it will recursively + /// check and fix all ancestors if required. + /// + /// + /// + /// A callback specified to retrieve the parent entity of the entity + /// A callback specified to update a fixed entity + public static void EnsureValidPath(this T entity, + ILogger logger, + Func getParent, + Action update) + where T: IUmbracoEntity + { + if (entity.HasIdentity == false) + throw new InvalidOperationException("Could not ensure the entity path, the entity has not been assigned an identity"); + + if (entity.ValidatePath() == false) + { + logger.Warn(typeof(UmbracoEntityExtensions), string.Format("The content item {0} has an invalid path: {1} with parentID: {2}", entity.Id, entity.Path, entity.ParentId)); + if (entity.ParentId == -1) + { + entity.Path = string.Concat("-1,", entity.Id); + //path changed, update it + update(entity); + } + else + { + var parent = getParent(entity); + if (parent == null) + throw new NullReferenceException("Could not ensure path for entity " + entity.Id + " could not resolve it's parent " + entity.ParentId); + + //the parent must also be valid! + parent.EnsureValidPath(logger, getParent, update); + + entity.Path = string.Concat(parent.Path, ",", entity.Id); + //path changed, update it + update(entity); + } + } + } public static bool HasChildren(this IUmbracoEntity entity) { diff --git a/src/Umbraco.Core/Services/ContentService.cs b/src/Umbraco.Core/Services/ContentService.cs index a4c6d354c1..e0edaa38c4 100644 --- a/src/Umbraco.Core/Services/ContentService.cs +++ b/src/Umbraco.Core/Services/ContentService.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; using System.ComponentModel; using System.Globalization; +using System.IO; using System.Linq; using System.Threading; using System.Xml; @@ -11,6 +12,7 @@ using Umbraco.Core.Configuration; using Umbraco.Core.Events; using Umbraco.Core.Logging; using Umbraco.Core.Models; +using Umbraco.Core.Models.EntityBase; using Umbraco.Core.Models.Membership; using Umbraco.Core.Models.Rdbms; using Umbraco.Core.Persistence; @@ -714,7 +716,12 @@ namespace Umbraco.Core.Services /// An Enumerable list of objects public IEnumerable GetDescendants(IContent content) { - using (var repository = RepositoryFactory.CreateContentRepository(UowProvider.GetUnitOfWork())) + //This is a check to ensure that the path is correct for this entity to avoid problems like: http://issues.umbraco.org/issue/U4-9336 due to data corruption + if (content.ValidatePath() == false) + throw new InvalidDataException(string.Format("The content item {0} has an invalid path: {1} with parentID: {2}", content.Id, content.Path, content.ParentId)); + + var uow = UowProvider.GetUnitOfWork(); + using (var repository = RepositoryFactory.CreateContentRepository(uow)) { var pathMatch = content.Path + ","; var query = Query.Builder.Where(x => x.Path.StartsWith(pathMatch) && x.Id != content.Id); @@ -996,6 +1003,10 @@ namespace Umbraco.Core.Services using (new WriteLock(Locker)) { + //Hack: this ensures that the entity's path is valid and if not it fixes/persists it + //see: http://issues.umbraco.org/issue/U4-9336 + content.EnsureValidPath(Logger, entity => GetById(entity.ParentId), QuickUpdate); + var originalPath = content.Path; if (Trashing.IsRaisedEventCancelled( @@ -1683,16 +1694,16 @@ namespace Umbraco.Core.Services /// True if sorting succeeded, otherwise False public bool Sort(IEnumerable items, int userId = 0, bool raiseEvents = true) { + var asArray = items.ToArray(); if (raiseEvents) { - if (Saving.IsRaisedEventCancelled(new SaveEventArgs(items), this)) + if (Saving.IsRaisedEventCancelled(new SaveEventArgs(asArray), this)) return false; } var shouldBePublished = new List(); var shouldBeSaved = new List(); - - var asArray = items.ToArray(); + using (new WriteLock(Locker)) { var uow = UowProvider.GetUnitOfWork(); @@ -1812,6 +1823,23 @@ namespace Umbraco.Core.Services #region Private Methods + /// + /// Hack: This is used to fix some data if an entity's properties are invalid/corrupt + /// + /// + private void QuickUpdate(IContent content) + { + if (content == null) throw new ArgumentNullException("content"); + if (content.HasIdentity == false) throw new InvalidOperationException("Cannot update an entity without an Identity"); + + var uow = UowProvider.GetUnitOfWork(); + using (var repository = RepositoryFactory.CreateContentRepository(uow)) + { + repository.AddOrUpdate(content); + uow.Commit(); + } + } + private void Audit(AuditType type, string message, int userId, int objectId) { var uow = UowProvider.GetUnitOfWork(); @@ -1919,6 +1947,10 @@ namespace Umbraco.Core.Services using (new WriteLock(Locker)) { + //Hack: this ensures that the entity's path is valid and if not it fixes/persists it + //see: http://issues.umbraco.org/issue/U4-9336 + content.EnsureValidPath(Logger, entity => GetById(entity.ParentId), QuickUpdate); + var result = new List>(); //Check if parent is published (although not if its a root node) - if parent isn't published this Content cannot be published diff --git a/src/Umbraco.Core/Services/MediaService.cs b/src/Umbraco.Core/Services/MediaService.cs index a1182ce80a..bd373f1ec0 100644 --- a/src/Umbraco.Core/Services/MediaService.cs +++ b/src/Umbraco.Core/Services/MediaService.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; using System.ComponentModel; using System.Globalization; +using System.IO; using System.Linq; using System.Text.RegularExpressions; using System.Threading; @@ -527,6 +528,10 @@ namespace Umbraco.Core.Services /// An Enumerable flat list of objects public IEnumerable GetDescendants(IMedia media) { + //This is a check to ensure that the path is correct for this entity to avoid problems like: http://issues.umbraco.org/issue/U4-9336 due to data corruption + if (media.ValidatePath() == false) + throw new InvalidDataException(string.Format("The content item {0} has an invalid path: {1} with parentID: {2}", media.Id, media.Path, media.ParentId)); + var uow = UowProvider.GetUnitOfWork(); using (var repository = RepositoryFactory.CreateMediaRepository(uow)) { @@ -619,7 +624,7 @@ namespace Umbraco.Core.Services public IMedia GetMediaByPath(string mediaPath) { var umbracoFileValue = mediaPath; - + const string Pattern = ".*[_][0-9]+[x][0-9]+[.].*"; var isResized = Regex.IsMatch(mediaPath, Pattern); @@ -998,56 +1003,63 @@ namespace Umbraco.Core.Services { if (media == null) throw new ArgumentNullException("media"); - var originalPath = media.Path; - var evtMsgs = EventMessagesFactory.Get(); - if (Trashing.IsRaisedEventCancelled( - new MoveEventArgs(new MoveEventInfo(media, originalPath, Constants.System.RecycleBinMedia)), this)) + using (new WriteLock(Locker)) { - return OperationStatus.Cancelled(evtMsgs); - } + //Hack: this ensures that the entity's path is valid and if not it fixes/persists it + //see: http://issues.umbraco.org/issue/U4-9336 + media.EnsureValidPath(Logger, entity => GetById(entity.ParentId), QuickUpdate); - var moveInfo = new List> - { - new MoveEventInfo(media, originalPath, Constants.System.RecycleBinMedia) - }; + var originalPath = media.Path; - //Find Descendants, which will be moved to the recycle bin along with the parent/grandparent. - var descendants = GetDescendants(media).OrderBy(x => x.Level).ToList(); - - var uow = UowProvider.GetUnitOfWork(); - using (var repository = RepositoryFactory.CreateMediaRepository(uow)) - { - //TODO: This should be part of the repo! - - //Remove 'published' xml from the cmsContentXml table for the unpublished media - uow.Database.Delete("WHERE nodeId = @Id", new { Id = media.Id }); - - media.ChangeTrashedState(true, Constants.System.RecycleBinMedia); - repository.AddOrUpdate(media); - - //Loop through descendants to update their trash state, but ensuring structure by keeping the ParentId - foreach (var descendant in descendants) + if (Trashing.IsRaisedEventCancelled( + new MoveEventArgs(new MoveEventInfo(media, originalPath, Constants.System.RecycleBinMedia)), this)) { - //Remove 'published' xml from the cmsContentXml table for the unpublished media - uow.Database.Delete("WHERE nodeId = @Id", new { Id = descendant.Id }); - - descendant.ChangeTrashedState(true, descendant.ParentId); - repository.AddOrUpdate(descendant); - - moveInfo.Add(new MoveEventInfo(descendant, descendant.Path, descendant.ParentId)); + return OperationStatus.Cancelled(evtMsgs); } - uow.Commit(); + var moveInfo = new List> + { + new MoveEventInfo(media, originalPath, Constants.System.RecycleBinMedia) + }; + + //Find Descendants, which will be moved to the recycle bin along with the parent/grandparent. + var descendants = GetDescendants(media).OrderBy(x => x.Level).ToList(); + + var uow = UowProvider.GetUnitOfWork(); + using (var repository = RepositoryFactory.CreateMediaRepository(uow)) + { + //TODO: This should be part of the repo! + + //Remove 'published' xml from the cmsContentXml table for the unpublished media + uow.Database.Delete("WHERE nodeId = @Id", new { Id = media.Id }); + + media.ChangeTrashedState(true, Constants.System.RecycleBinMedia); + repository.AddOrUpdate(media); + + //Loop through descendants to update their trash state, but ensuring structure by keeping the ParentId + foreach (var descendant in descendants) + { + //Remove 'published' xml from the cmsContentXml table for the unpublished media + uow.Database.Delete("WHERE nodeId = @Id", new { Id = descendant.Id }); + + descendant.ChangeTrashedState(true, descendant.ParentId); + repository.AddOrUpdate(descendant); + + moveInfo.Add(new MoveEventInfo(descendant, descendant.Path, descendant.ParentId)); + } + + uow.Commit(); + } + + Trashed.RaiseEvent( + new MoveEventArgs(false, evtMsgs, moveInfo.ToArray()), this); + + Audit(AuditType.Move, "Move Media to Recycle Bin performed by user", userId, media.Id); + + return OperationStatus.Success(evtMsgs); } - - Trashed.RaiseEvent( - new MoveEventArgs(false, evtMsgs, moveInfo.ToArray()), this); - - Audit(AuditType.Move, "Move Media to Recycle Bin performed by user", userId, media.Id); - - return OperationStatus.Success(evtMsgs); } /// @@ -1064,8 +1076,6 @@ namespace Umbraco.Core.Services ((IMediaServiceOperations)this).Delete(media, userId); } - - /// /// Permanently deletes versions from an object prior to a specific date. /// This method will never delete the latest version of a content item. @@ -1154,7 +1164,6 @@ namespace Umbraco.Core.Services public bool Sort(IEnumerable items, int userId = 0, bool raiseEvents = true) { var asArray = items.ToArray(); - if (raiseEvents) { if (Saving.IsRaisedEventCancelled(new SaveEventArgs(asArray), this)) @@ -1316,6 +1325,23 @@ namespace Umbraco.Core.Services } } + /// + /// Hack: This is used to fix some data if an entity's properties are invalid/corrupt + /// + /// + private void QuickUpdate(IMedia media) + { + if (media == null) throw new ArgumentNullException("media"); + if (media.HasIdentity == false) throw new InvalidOperationException("Cannot update an entity without an Identity"); + + var uow = UowProvider.GetUnitOfWork(); + using (var repository = RepositoryFactory.CreateMediaRepository(uow)) + { + repository.AddOrUpdate(media); + uow.Commit(); + } + } + #region Event Handlers /// diff --git a/src/Umbraco.Tests/Models/UmbracoEntityTests.cs b/src/Umbraco.Tests/Models/UmbracoEntityTests.cs index 651e955f33..7186474999 100644 --- a/src/Umbraco.Tests/Models/UmbracoEntityTests.cs +++ b/src/Umbraco.Tests/Models/UmbracoEntityTests.cs @@ -1,7 +1,10 @@ using System; using System.Diagnostics; +using Moq; using NUnit.Framework; +using Umbraco.Core.Logging; using Umbraco.Core.Models; +using Umbraco.Core.Models.EntityBase; using Umbraco.Core.Serialization; namespace Umbraco.Tests.Models @@ -9,6 +12,132 @@ namespace Umbraco.Tests.Models [TestFixture] public class UmbracoEntityTests { + [Test] + public void Validate_Path() + { + var entity = new UmbracoEntity(); + + //it's empty with no id so we need to allow it + Assert.IsTrue(entity.ValidatePath()); + + entity.Id = 1234; + + //it has an id but no path, so we can't allow it + Assert.IsFalse(entity.ValidatePath()); + + entity.Path = "-1"; + + //invalid path + Assert.IsFalse(entity.ValidatePath()); + + entity.Path = string.Concat("-1,", entity.Id); + + //valid path + Assert.IsTrue(entity.ValidatePath()); + } + + [Test] + public void Ensure_Path_Throws_Without_Id() + { + var entity = new UmbracoEntity(); + + //no id assigned + Assert.Throws(() => entity.EnsureValidPath(Mock.Of(), umbracoEntity => new UmbracoEntity(), umbracoEntity => { })); + } + + [Test] + public void Ensure_Path_Throws_Without_Parent() + { + var entity = new UmbracoEntity {Id = 1234}; + + //no parent found + Assert.Throws(() => entity.EnsureValidPath(Mock.Of(), umbracoEntity => null, umbracoEntity => { })); + } + + [Test] + public void Ensure_Path_Entity_At_Root() + { + var entity = new UmbracoEntity + { + Id = 1234, + ParentId = -1 + }; + + + entity.EnsureValidPath(Mock.Of(), umbracoEntity => null, umbracoEntity => { }); + + //works because it's under the root + Assert.AreEqual("-1,1234", entity.Path); + } + + [Test] + public void Ensure_Path_Entity_Valid_Parent() + { + var entity = new UmbracoEntity + { + Id = 1234, + ParentId = 888 + }; + + entity.EnsureValidPath(Mock.Of(), umbracoEntity => umbracoEntity.ParentId == 888 ? new UmbracoEntity{Id = 888, Path = "-1,888"} : null, umbracoEntity => { }); + + //works because the parent was found + Assert.AreEqual("-1,888,1234", entity.Path); + } + + [Test] + public void Ensure_Path_Entity_Valid_Recursive_Parent() + { + var parentA = new UmbracoEntity + { + Id = 999, + ParentId = -1 + }; + + var parentB = new UmbracoEntity + { + Id = 888, + ParentId = 999 + }; + + var parentC = new UmbracoEntity + { + Id = 777, + ParentId = 888 + }; + + var entity = new UmbracoEntity + { + Id = 1234, + ParentId = 777 + }; + + Func getParent = umbracoEntity => + { + switch (umbracoEntity.ParentId) + { + case 999: + return parentA; + case 888: + return parentB; + case 777: + return parentC; + case 1234: + return entity; + default: + return null; + } + }; + + //this will recursively fix all paths + entity.EnsureValidPath(Mock.Of(), getParent, umbracoEntity => { }); + + Assert.AreEqual("-1,999", parentA.Path); + Assert.AreEqual("-1,999,888", parentB.Path); + Assert.AreEqual("-1,999,888,777", parentC.Path); + Assert.AreEqual("-1,999,888,777,1234", entity.Path); + } + [Test] public void UmbracoEntity_Can_Be_Initialized_From_Dynamic() { diff --git a/src/Umbraco.Tests/Services/ContentServiceTests.cs b/src/Umbraco.Tests/Services/ContentServiceTests.cs index afffc8ad1e..72a58861e2 100644 --- a/src/Umbraco.Tests/Services/ContentServiceTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceTests.cs @@ -55,7 +55,7 @@ namespace Umbraco.Tests.Services /// Regression test: http://issues.umbraco.org/issue/U4-9336 /// [Test] - public void Deleting_Node_With_Invalid_Path() + public void Moving_Node_To_Recycle_Bin_With_Invalid_Path() { var contentService = ServiceContext.ContentService; var root = ServiceContext.ContentService.GetById(NodeDto.NodeIdSeed + 1); @@ -72,15 +72,19 @@ namespace Umbraco.Tests.Services //now make the data corrupted :/ DatabaseContext.Database.Execute("UPDATE umbracoNode SET path = '-1' WHERE id = @id", new {id = content.Id}); - // need to clear the caches otherwise the ContentService will just serve is a cached version with the non-updated path from db. - ApplicationContext.Current.ApplicationCache.RuntimeCache.ClearAllCache(); - //re-get with the corrupt path content = contentService.GetById(content.Id); - // Note to Shan: put a breakpoint at ContentService.cs line: 1021 // here we get all descendants by the path of the node being moved to bin, and unpublish all of them. - ServiceContext.ContentService.MoveToRecycleBin(content); + // since the path is invalid, there's logic in here to fix that if it's possible and re-persist the entity. + var moveResult = ServiceContext.ContentService.WithResult().MoveToRecycleBin(content); + + Assert.IsTrue(moveResult.Success); + + //re-get with the fixed/moved path + content = contentService.GetById(content.Id); + + Assert.AreEqual("-1,-20," + content.Id, content.Path); //re-get hierarchy = contentService.GetByIds(hierarchy.Select(x => x.Id).ToArray()).OrderBy(x => x.Level).ToArray();