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();