From d889d3e3857632fa5bb6ac9badd3f117eff6beca Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 4 Jan 2017 20:00:30 +1100 Subject: [PATCH 1/4] adds test --- .../Services/ContentServiceTests.cs | 196 +++++++++++------- 1 file changed, 116 insertions(+), 80 deletions(-) diff --git a/src/Umbraco.Tests/Services/ContentServiceTests.cs b/src/Umbraco.Tests/Services/ContentServiceTests.cs index ed4a59bab4..7a386dbbbc 100644 --- a/src/Umbraco.Tests/Services/ContentServiceTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceTests.cs @@ -35,18 +35,54 @@ namespace Umbraco.Tests.Services [SetUp] public override void Initialize() { - base.Initialize(); + base.Initialize(); + } + + [TearDown] + public override void TearDown() + { + base.TearDown(); } - - [TearDown] - public override void TearDown() - { - base.TearDown(); - } //TODO Add test to verify there is only ONE newest document/content in cmsDocument table after updating. //TODO Add test to delete specific version (with and without deleting prior versions) and versions by date. + + /// + /// Ensures that we don't unpublish all nodes when a node is deleted that has an invalid path of -1 + /// Regression test: http://issues.umbraco.org/issue/U4-9336 + /// + [Test] + public void Deleting_Node_With_Invalid_Path() + { + var contentService = ServiceContext.ContentService; + var root = ServiceContext.ContentService.GetById(NodeDto.NodeIdSeed + 1); + Assert.IsTrue(contentService.PublishWithStatus(root).Success); + var content = contentService.CreateContentWithIdentity("Test", -1, "umbTextpage", 0); + Assert.IsTrue(contentService.PublishWithStatus(content).Success); + var hierarchy = CreateContentHierarchy().OrderBy(x => x.Level).ToArray(); + contentService.Save(hierarchy, 0); + foreach (var c in hierarchy) + { + Assert.IsTrue(contentService.PublishWithStatus(c).Success); + } + + //now make the data corrupted :/ + + DatabaseContext.Database.Execute("UPDATE umbracoNode SET path = '-1' WHERE id = @id", new {id = content.Id}); + + //re-get + content = contentService.GetById(content.Id); + + ServiceContext.ContentService.Delete(content); + + //re-get + hierarchy = contentService.GetByIds(hierarchy.Select(x => x.Id).ToArray()).OrderBy(x => x.Level).ToArray(); + + Assert.That(hierarchy.All(c => c.Trashed == false), Is.True); + Assert.That(hierarchy.All(c => c.Path.StartsWith("-1,-20") == false), Is.True); + } + [Test] public void Remove_Scheduled_Publishing_Date() { @@ -104,7 +140,7 @@ namespace Umbraco.Tests.Services results.Add(contentService.CreateContentWithIdentity("Test", -1, "umbTextpage", 0)); } - var sortedGet = contentService.GetByIds(new[] {results[10].Id, results[5].Id, results[12].Id}).ToArray(); + var sortedGet = contentService.GetByIds(new[] { results[10].Id, results[5].Id, results[12].Id }).ToArray(); // Assert Assert.AreEqual(sortedGet[0].Id, results[10].Id); @@ -121,7 +157,7 @@ namespace Umbraco.Tests.Services // Act for (int i = 0; i < 20; i++) { - contentService.CreateContentWithIdentity("Test", -1, "umbTextpage", 0); + contentService.CreateContentWithIdentity("Test", -1, "umbTextpage", 0); } // Assert @@ -230,7 +266,7 @@ namespace Umbraco.Tests.Services // Assert //there should be no tags for this entity - var tags = tagService.GetTagsForEntity(content1.Id); + var tags = tagService.GetTagsForEntity(content1.Id); Assert.AreEqual(0, tags.Count()); //these tags should still be returned since they still have actively published content assigned @@ -265,7 +301,7 @@ namespace Umbraco.Tests.Services contentService.MoveToRecycleBin(content2); // Assert - + //there should be no exposed content tags now that nothing is published. var allTags = tagService.GetAllContentTags(); Assert.AreEqual(0, allTags.Count()); @@ -395,7 +431,7 @@ namespace Umbraco.Tests.Services new PropertyType("test", DataTypeDatabaseType.Ntext, "tags") { DataTypeDefinitionId = 1041 - }); + }); contentTypeService.Save(contentType); contentType.AllowedContentTypes = new[] { new ContentTypeSort(new Lazy(() => contentType.Id), 0, contentType.Alias) }; @@ -410,7 +446,7 @@ namespace Umbraco.Tests.Services var child2 = MockedContent.CreateSimpleContent(contentType, "child 2 content", content.Id); child2.SetTags("tags", new[] { "hello2", "world2" }, true); contentService.Save(child2); - + // Act contentService.PublishWithChildrenWithStatus(content, includeUnpublished: true); @@ -461,34 +497,34 @@ namespace Umbraco.Tests.Services new { nodeId = content.Id, propTypeId = propertyTypeId })); } - [Test] - public void Can_Replace_Tag_Data_To_Published_Content() - { + [Test] + public void Can_Replace_Tag_Data_To_Published_Content() + { //Arrange var contentService = ServiceContext.ContentService; var contentTypeService = ServiceContext.ContentTypeService; var contentType = MockedContentTypes.CreateSimpleContentType("umbMandatory", "Mandatory Doc Type", true); contentType.PropertyGroups.First().PropertyTypes.Add( new PropertyType("test", DataTypeDatabaseType.Ntext, "tags") - { - DataTypeDefinitionId = 1041 - }); + { + DataTypeDefinitionId = 1041 + }); contentTypeService.Save(contentType); var content = MockedContent.CreateSimpleContent(contentType, "Tagged content", -1); - - + + // Act content.SetTags("tags", new[] { "hello", "world", "some", "tags" }, true); contentService.Publish(content); // Assert Assert.AreEqual(4, content.Properties["tags"].Value.ToString().Split(',').Distinct().Count()); - var propertyTypeId = contentType.PropertyTypes.Single(x => x.Alias == "tags").Id; - Assert.AreEqual(4, DatabaseContext.Database.ExecuteScalar( - "SELECT COUNT(*) FROM cmsTagRelationship WHERE nodeId=@nodeId AND propertyTypeId=@propTypeId", - new {nodeId = content.Id, propTypeId = propertyTypeId})); - } + var propertyTypeId = contentType.PropertyTypes.Single(x => x.Alias == "tags").Id; + Assert.AreEqual(4, DatabaseContext.Database.ExecuteScalar( + "SELECT COUNT(*) FROM cmsTagRelationship WHERE nodeId=@nodeId AND propertyTypeId=@propTypeId", + new { nodeId = content.Id, propTypeId = propertyTypeId })); + } [Test] public void Can_Append_Tag_Data_To_Published_Content() @@ -506,7 +542,7 @@ namespace Umbraco.Tests.Services var content = MockedContent.CreateSimpleContent(contentType, "Tagged content", -1); content.SetTags("tags", new[] { "hello", "world", "some", "tags" }, true); contentService.PublishWithStatus(content); - + // Act content.SetTags("tags", new[] { "another", "world" }, false); contentService.PublishWithStatus(content); @@ -548,9 +584,9 @@ namespace Umbraco.Tests.Services new { nodeId = content.Id, propTypeId = propertyTypeId })); } - [Test] - public void Can_Remove_Property_Type() - { + [Test] + public void Can_Remove_Property_Type() + { // Arrange var contentService = ServiceContext.ContentService; @@ -560,9 +596,9 @@ namespace Umbraco.Tests.Services // Assert Assert.That(content, Is.Not.Null); Assert.That(content.HasIdentity, Is.False); - } + } - [Test] + [Test] public void Can_Create_Content() { // Arrange @@ -575,7 +611,7 @@ namespace Umbraco.Tests.Services Assert.That(content, Is.Not.Null); Assert.That(content.HasIdentity, Is.False); } - + [Test] public void Can_Create_Content_Without_Explicitly_Set_User() { @@ -595,12 +631,12 @@ namespace Umbraco.Tests.Services public void Can_Save_New_Content_With_Explicit_User() { var user = new User(ServiceContext.UserService.GetUserTypeByAlias("admin")) - { - Name = "Test", - Email = "test@test.com", - Username = "test", + { + Name = "Test", + Email = "test@test.com", + Username = "test", RawPasswordValue = "test" - }; + }; ServiceContext.UserService.Save(user); var content = new Content("Test", -1, ServiceContext.ContentTypeService.GetContentType("umbTextpage")); @@ -857,14 +893,14 @@ namespace Umbraco.Tests.Services var provider = new PetaPocoUnitOfWorkProvider(Logger); using (var uow = provider.GetUnitOfWork()) { - uow.Database.TruncateTable("cmsContentXml"); + uow.Database.TruncateTable("cmsContentXml"); } - + //for this test we are also going to save a revision for a content item that is not published, this is to ensure //that it's published version still makes it into the cmsContentXml table! contentService.Save(allContent.Last()); - + // Act var published = contentService.RePublishAll(0); @@ -872,7 +908,7 @@ namespace Umbraco.Tests.Services Assert.IsTrue(published); using (var uow = provider.GetUnitOfWork()) { - Assert.AreEqual(allContent.Count(), uow.Database.ExecuteScalar("select count(*) from cmsContentXml")); + Assert.AreEqual(allContent.Count(), uow.Database.ExecuteScalar("select count(*) from cmsContentXml")); } } @@ -889,7 +925,7 @@ namespace Umbraco.Tests.Services var allContent = rootContent.Concat(rootContent.SelectMany(x => x.Descendants())).ToList(); //for testing we need to clear out the contentXml table so we can see if it worked var provider = new PetaPocoUnitOfWorkProvider(Logger); - + using (var uow = provider.GetUnitOfWork()) { uow.Database.TruncateTable("cmsContentXml"); @@ -899,7 +935,7 @@ namespace Umbraco.Tests.Services contentService.Save(allContent.Last()); // Act - contentService.RePublishAll(new int[]{allContent.Last().ContentTypeId}); + contentService.RePublishAll(new int[] { allContent.Last().ContentTypeId }); // Assert using (var uow = provider.GetUnitOfWork()) @@ -1085,7 +1121,7 @@ namespace Umbraco.Tests.Services { // Arrange var contentService = ServiceContext.ContentService; - var content = contentService.CreateContent("Home US", - 1, "umbTextpage", 0); + var content = contentService.CreateContent("Home US", -1, "umbTextpage", 0); content.SetValue("author", "Barack Obama"); // Act @@ -1116,7 +1152,7 @@ namespace Umbraco.Tests.Services var savedVersion = content.Version; // Act - var publishedDescendants = ((ContentService) contentService).GetPublishedDescendants(root).ToList(); + var publishedDescendants = ((ContentService)contentService).GetPublishedDescendants(root).ToList(); // Assert Assert.That(rootPublished, Is.True); @@ -1146,7 +1182,7 @@ namespace Umbraco.Tests.Services { // Arrange var contentService = ServiceContext.ContentService; - var content = contentService.CreateContent("Home US", - 1, "umbTextpage", 0); + var content = contentService.CreateContent("Home US", -1, "umbTextpage", 0); content.SetValue("author", "Barack Obama"); // Act @@ -1166,7 +1202,7 @@ namespace Umbraco.Tests.Services var contentType = contentTypeService.GetContentType("umbTextpage"); Content subpage = MockedContent.CreateSimpleContent(contentType, "Text Subpage 1", NodeDto.NodeIdSeed + 2); Content subpage2 = MockedContent.CreateSimpleContent(contentType, "Text Subpage 2", NodeDto.NodeIdSeed + 2); - var list = new List {subpage, subpage2}; + var list = new List { subpage, subpage2 }; // Act contentService.Save(list, 0); @@ -1186,9 +1222,9 @@ namespace Umbraco.Tests.Services contentService.Save(hierarchy, 0); Assert.That(hierarchy.Any(), Is.True); - Assert.That(hierarchy.Any(x => x.HasIdentity == false), Is.False); - //all parent id's should be ok, they are lazy and if they equal zero an exception will be thrown - Assert.DoesNotThrow(() => hierarchy.Any(x => x.ParentId != 0)); + Assert.That(hierarchy.Any(x => x.HasIdentity == false), Is.False); + //all parent id's should be ok, they are lazy and if they equal zero an exception will be thrown + Assert.DoesNotThrow(() => hierarchy.Any(x => x.ParentId != 0)); } @@ -1375,7 +1411,7 @@ namespace Umbraco.Tests.Services var contentType = ServiceContext.ContentTypeService.GetContentType("umbTextpage"); var temp = MockedContent.CreateSimpleContent(contentType, "Simple Text Page", -1); var prop = temp.Properties.First(); - temp.SetTags(prop.Alias, new[] {"hello", "world"}, true); + temp.SetTags(prop.Alias, new[] { "hello", "world" }, true); var status = contentService.PublishWithStatus(temp); // Act @@ -1418,14 +1454,14 @@ namespace Umbraco.Tests.Services [Test] public void Can_Save_Lazy_Content() - { - var unitOfWork = PetaPocoUnitOfWorkProvider.CreateUnitOfWork(Mock.Of()); + { + var unitOfWork = PetaPocoUnitOfWorkProvider.CreateUnitOfWork(Mock.Of()); var contentType = ServiceContext.ContentTypeService.GetContentType("umbTextpage"); var root = ServiceContext.ContentService.GetById(NodeDto.NodeIdSeed + 1); var c = new Lazy(() => MockedContent.CreateSimpleContent(contentType, "Hierarchy Simple Text Page", root.Id)); var c2 = new Lazy(() => MockedContent.CreateSimpleContent(contentType, "Hierarchy Simple Text Subpage", c.Value.Id)); - var list = new List> {c, c2}; + var list = new List> { c, c2 }; ContentTypeRepository contentTypeRepository; using (var repository = CreateRepository(unitOfWork, out contentTypeRepository)) @@ -1443,9 +1479,9 @@ namespace Umbraco.Tests.Services Assert.That(c2.Value.Id > 0, Is.True); Assert.That(c.Value.ParentId > 0, Is.True); - Assert.That(c2.Value.ParentId > 0, Is.True); + Assert.That(c2.Value.ParentId > 0, Is.True); } - + } [Test] @@ -1468,20 +1504,20 @@ namespace Umbraco.Tests.Services Assert.That(hasPublishedVersion, Is.True); } - [Test] - public void Can_Verify_Property_Types_On_Content() - { + [Test] + public void Can_Verify_Property_Types_On_Content() + { // Arrange - var contentTypeService = ServiceContext.ContentTypeService; + var contentTypeService = ServiceContext.ContentTypeService; var contentType = MockedContentTypes.CreateAllTypesContentType("allDataTypes", "All DataTypes"); contentTypeService.Save(contentType); - var contentService = ServiceContext.ContentService; - var content = MockedContent.CreateAllTypesContent(contentType, "Random Content", -1); + var contentService = ServiceContext.ContentService; + var content = MockedContent.CreateAllTypesContent(contentType, "Random Content", -1); contentService.Save(content); - var id = content.Id; + var id = content.Id; // Act - var sut = contentService.GetById(id); + var sut = contentService.GetById(id); // Arrange Assert.That(sut.GetValue("isTrue"), Is.True); @@ -1496,7 +1532,7 @@ namespace Umbraco.Tests.Services Assert.That(sut.GetValue("dateTime").ToString("G"), Is.EqualTo(content.GetValue("dateTime").ToString("G"))); Assert.That(sut.GetValue("colorPicker"), Is.EqualTo("black")); //that one is gone in 7.4 - //Assert.That(sut.GetValue("folderBrowser"), Is.Null); + //Assert.That(sut.GetValue("folderBrowser"), Is.Null); Assert.That(sut.GetValue("ddlMultiple"), Is.EqualTo("1234,1235")); Assert.That(sut.GetValue("rbList"), Is.EqualTo("random")); Assert.That(sut.GetValue("date").ToString("G"), Is.EqualTo(content.GetValue("date").ToString("G"))); @@ -1507,23 +1543,23 @@ namespace Umbraco.Tests.Services Assert.That(sut.GetValue("memberPicker"), Is.EqualTo(1092)); Assert.That(sut.GetValue("relatedLinks"), Is.EqualTo("")); Assert.That(sut.GetValue("tags"), Is.EqualTo("this,is,tags")); - } + } - [Test] - public void Can_Delete_Previous_Versions_Not_Latest() - { + [Test] + public void Can_Delete_Previous_Versions_Not_Latest() + { // Arrange var contentService = ServiceContext.ContentService; var content = contentService.GetById(NodeDto.NodeIdSeed + 4); - var version = content.Version; + var version = content.Version; - // Act + // Act contentService.DeleteVersion(NodeDto.NodeIdSeed + 4, version, true, 0); var sut = contentService.GetById(NodeDto.NodeIdSeed + 4); // Assert Assert.That(sut.Version, Is.EqualTo(version)); - } + } [Test] public void Ensure_Content_Xml_Created() @@ -1542,7 +1578,7 @@ namespace Umbraco.Tests.Services } contentService.Publish(content); - + using (var uow = provider.GetUnitOfWork()) { Assert.IsTrue(uow.Database.Exists(content.Id)); @@ -1559,10 +1595,10 @@ namespace Umbraco.Tests.Services contentService.Save(content); var provider = new PetaPocoUnitOfWorkProvider(Logger); - + using (var uow = provider.GetUnitOfWork()) { - Assert.IsTrue(uow.Database.SingleOrDefault("WHERE nodeId=@nodeId AND versionId = @versionId", new{nodeId = content.Id, versionId = content.Version}) != null); + Assert.IsTrue(uow.Database.SingleOrDefault("WHERE nodeId=@nodeId AND versionId = @versionId", new { nodeId = content.Id, versionId = content.Version }) != null); } } @@ -1665,11 +1701,11 @@ namespace Umbraco.Tests.Services var contentType = ServiceContext.ContentTypeService.GetContentType("umbTextpage"); var root = ServiceContext.ContentService.GetById(NodeDto.NodeIdSeed + 1); - var list = new List(); + var list = new List(); for (int i = 0; i < 10; i++) { - var content = MockedContent.CreateSimpleContent(contentType, "Hierarchy Simple Text Page " + i, root); + var content = MockedContent.CreateSimpleContent(contentType, "Hierarchy Simple Text Page " + i, root); list.Add(content); list.AddRange(CreateChildrenOf(contentType, content, 4)); @@ -1680,12 +1716,12 @@ namespace Umbraco.Tests.Services return list; } - private IEnumerable CreateChildrenOf(IContentType contentType, IContent content, int depth) + private IEnumerable CreateChildrenOf(IContentType contentType, IContent content, int depth) { var list = new List(); for (int i = 0; i < depth; i++) { - var c = MockedContent.CreateSimpleContent(contentType, "Hierarchy Simple Text Subpage " + i, content); + var c = MockedContent.CreateSimpleContent(contentType, "Hierarchy Simple Text Subpage " + i, content); list.Add(c); Debug.Print("Created: 'Hierarchy Simple Text Subpage {0}' - Depth: {1}", i, depth); From f200ab423deee091dbf40dab20b088f074fe463b Mon Sep 17 00:00:00 2001 From: Claus Date: Wed, 4 Jan 2017 11:59:07 +0100 Subject: [PATCH 2/4] updated test to fail and updated notes. --- src/Umbraco.Tests/Services/ContentServiceTests.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Tests/Services/ContentServiceTests.cs b/src/Umbraco.Tests/Services/ContentServiceTests.cs index 7a386dbbbc..afffc8ad1e 100644 --- a/src/Umbraco.Tests/Services/ContentServiceTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceTests.cs @@ -50,6 +50,8 @@ namespace Umbraco.Tests.Services /// /// Ensures that we don't unpublish all nodes when a node is deleted that has an invalid path of -1 + /// Note: it is actually the MoveToRecycleBin happening on the initial deletion of a node through the UI + /// that causes the issue. /// Regression test: http://issues.umbraco.org/issue/U4-9336 /// [Test] @@ -68,13 +70,17 @@ namespace Umbraco.Tests.Services } //now make the data corrupted :/ - DatabaseContext.Database.Execute("UPDATE umbracoNode SET path = '-1' WHERE id = @id", new {id = content.Id}); - //re-get + // 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); - ServiceContext.ContentService.Delete(content); + // 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); //re-get hierarchy = contentService.GetByIds(hierarchy.Select(x => x.Id).ToArray()).OrderBy(x => x.Level).ToArray(); From 8be0f683906873f18ce888e866d5a92742c6111e Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 5 Jan 2017 10:29:03 +1100 Subject: [PATCH 3/4] 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(); From 530a80d7eaa480ff23cca0d1e0e519896b1d9901 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 5 Jan 2017 10:43:33 +1100 Subject: [PATCH 4/4] Adds a check to validate the NodeDto when persisting the item to validate that it's path is correct this should prevent any data corruption in the future - if the data corruption actually came from Core --- .../Models/UmbracoEntityExtensions.cs | 30 +++++++++++++++++++ .../Repositories/ContentRepository.cs | 2 ++ .../Repositories/MediaRepository.cs | 2 ++ 3 files changed, 34 insertions(+) diff --git a/src/Umbraco.Core/Models/UmbracoEntityExtensions.cs b/src/Umbraco.Core/Models/UmbracoEntityExtensions.cs index 1b4cc20d73..42e1a0d415 100644 --- a/src/Umbraco.Core/Models/UmbracoEntityExtensions.cs +++ b/src/Umbraco.Core/Models/UmbracoEntityExtensions.cs @@ -1,16 +1,46 @@ using System; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Text; using System.Threading.Tasks; using System.Web.UI.WebControls; using Umbraco.Core.Logging; using Umbraco.Core.Models.EntityBase; +using Umbraco.Core.Models.Rdbms; 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 void ValidatePathWithException(this NodeDto entity) + { + //don't validate if it's empty and it has no id + if (entity.NodeId == default(int) && entity.Path.IsNullOrWhiteSpace()) + return; + + if (entity.Path.IsNullOrWhiteSpace()) + throw new InvalidDataException(string.Format("The content item {0} has an empty path: {1} with parentID: {2}", entity.NodeId, entity.Path, entity.ParentId)); + + 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 + throw new InvalidDataException(string.Format("The content item {0} has an invalid path: {1} with parentID: {2}", entity.NodeId, entity.Path, entity.ParentId)); + } + + 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 + throw new InvalidDataException(string.Format("The content item {0} has an invalid path: {1} with parentID: {2}", entity.NodeId, entity.Path, entity.ParentId)); + } + } + /// /// Does a quick check on the entity's set path to ensure that it's valid and consistent /// diff --git a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs index 90f005f68e..14e6c0e9a6 100644 --- a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs @@ -342,6 +342,7 @@ namespace Umbraco.Core.Persistence.Repositories //Update with new correct path nodeDto.Path = string.Concat(parent.Path, ",", nodeDto.NodeId); + nodeDto.ValidatePathWithException(); Database.Update(nodeDto); //Update entity with correct values @@ -479,6 +480,7 @@ namespace Umbraco.Core.Persistence.Repositories //Updates the (base) node data - umbracoNode var nodeDto = dto.ContentVersionDto.ContentDto.NodeDto; + nodeDto.ValidatePathWithException(); var o = Database.Update(nodeDto); //Only update this DTO if the contentType has actually changed diff --git a/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs b/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs index 092b7df025..08c2f7e0be 100644 --- a/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs @@ -324,6 +324,7 @@ namespace Umbraco.Core.Persistence.Repositories //Update with new correct path nodeDto.Path = string.Concat(parent.Path, ",", nodeDto.NodeId); + nodeDto.ValidatePathWithException(); Database.Update(nodeDto); //Update entity with correct values @@ -397,6 +398,7 @@ namespace Umbraco.Core.Persistence.Repositories //Updates the (base) node data - umbracoNode var nodeDto = dto.ContentDto.NodeDto; + nodeDto.ValidatePathWithException(); var o = Database.Update(nodeDto); //Only update this DTO if the contentType has actually changed