From 5ca32ba1e9376002d14c735d9825a687acab3720 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 16 Oct 2019 17:29:45 +1100 Subject: [PATCH 1/6] starts writing tests --- .../PublishedContent/NuCacheChildrenTests.cs | 108 ++++++++++++++++-- 1 file changed, 96 insertions(+), 12 deletions(-) diff --git a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs index ac3ac7d455..d67d69d46e 100644 --- a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs +++ b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs @@ -213,22 +213,24 @@ namespace Umbraco.Tests.PublishedContent var level = path.Count(x => x == ','); var now = DateTime.Now; + var contentData = new ContentData + { + Name = "N" + id, + Published = true, + TemplateId = 0, + VersionId = 1, + VersionDate = now, + WriterId = 0, + Properties = new Dictionary(), + CultureInfos = new Dictionary() + }; + return new ContentNodeKit { ContentTypeId = _contentTypeInvariant.Id, Node = new ContentNode(id, Guid.NewGuid(), level, path, sortOrder, parentId, DateTime.Now, 0), DraftData = null, - PublishedData = new ContentData - { - Name = "N" + id, - Published = true, - TemplateId = 0, - VersionId = 1, - VersionDate = now, - WriterId = 0, - Properties = new Dictionary(), - CultureInfos = new Dictionary() - } + PublishedData = contentData }; } @@ -333,7 +335,7 @@ namespace Umbraco.Tests.PublishedContent CultureInfos = GetCultureInfos(id, now) }; - var withDraft = id%2==0; + var withDraft = id % 2 == 0; var withPublished = !withDraft; return new ContentNodeKit @@ -1063,6 +1065,88 @@ namespace Umbraco.Tests.PublishedContent AssertLinkedNode(child3.contentNode, 1, 2, -1, -1, -1); } + /// + /// This addresses issue: https://github.com/umbraco/Umbraco-CMS/issues/6698 + /// + /// + /// This test mimics if someone were to: + /// 1) Unpublish a "middle child" + /// 2) Save and publish it + /// 3) Publish it with descendants + /// 4) Repeat steps 2 and 3 + /// + /// Which has caused an exception. To replicate this test: + /// 1) RefreshBranch with kits for a branch where the top most node is unpublished + /// 2) RefreshBranch with kits for the branch where the top most node is published + /// 3) RefreshBranch with kits for the branch where the top most node is published + /// 4) RefreshBranch with kits for the branch where the top most node is published + /// 5) RefreshBranch with kits for the branch where the top most node is published + /// + [Test] + public void Refresh_Branch_With_Alternating_Publish_Flags() + { + // NOTE: these tests are not using real scopes, in which case a Scope does not control + // how the snapshots generations work. We are forcing new snapshot generations manually. + + IEnumerable GetKits() + { + var paths = new Dictionary { { -1, "-1" } }; + + //root + yield return CreateInvariantKit(1, -1, 1, paths); + + //children + yield return CreateInvariantKit(2, 1, 1, paths); + yield return CreateInvariantKit(3, 1, 2, paths); //middle child + yield return CreateInvariantKit(4, 1, 3, paths); + } + + //init with all published + Init(GetKits()); + + var snapshotService = (PublishedSnapshotService)_snapshotService; + var contentStore = snapshotService.GetContentStore(); + + var rootKit = _source.Kits[1].Clone(); + + void ChangePublishFlagOfRoot(bool published, int assertGen) + { + //This will set a flag to force creating a new Gen next time the store is locked (i.e. In Notify) + contentStore.CreateSnapshot(); + + Assert.IsFalse(contentStore.Test.NextGen); + + //Change the root publish flag + var kit = rootKit.Clone(); + kit.DraftData = published ? null : kit.PublishedData; + kit.PublishedData = published? kit.PublishedData : null; + _source.Kits[1] = kit; + + _snapshotService.Notify(new[] + { + new ContentCacheRefresher.JsonPayload(1, Guid.Empty, TreeChangeTypes.RefreshBranch) + }, out _, out _); + + Assert.AreEqual(assertGen, contentStore.Test.LiveGen); + Assert.IsTrue(contentStore.Test.NextGen); + } + + //unpublish the root + ChangePublishFlagOfRoot(false, 2); + + //publish the root + ChangePublishFlagOfRoot(true, 3); + + //publish root + descendants + ChangePublishFlagOfRoot(true, 4); + + //publish the root + ChangePublishFlagOfRoot(true, 5); + + //publish root + descendants + ChangePublishFlagOfRoot(true, 6); //TODO: This should fail, need to figure out what the diff is between this and a website + } + [Test] public void Refresh_Branch_Ensures_Linked_List() { From f384b31e981b634cf8b2c79f2c714ad36c8b3e3a Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 17 Oct 2019 15:47:15 +1100 Subject: [PATCH 2/6] Gets a test to confirm the issue, now to fix --- .../PublishedContent/NuCacheChildrenTests.cs | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs index d67d69d46e..7904eed0ec 100644 --- a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs +++ b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs @@ -1079,7 +1079,7 @@ namespace Umbraco.Tests.PublishedContent /// 1) RefreshBranch with kits for a branch where the top most node is unpublished /// 2) RefreshBranch with kits for the branch where the top most node is published /// 3) RefreshBranch with kits for the branch where the top most node is published - /// 4) RefreshBranch with kits for the branch where the top most node is published + /// 4) RefreshNode /// 5) RefreshBranch with kits for the branch where the top most node is published /// [Test] @@ -1093,12 +1093,17 @@ namespace Umbraco.Tests.PublishedContent var paths = new Dictionary { { -1, "-1" } }; //root - yield return CreateInvariantKit(1, -1, 1, paths); + yield return CreateInvariantKit(100, -1, 1, paths); - //children - yield return CreateInvariantKit(2, 1, 1, paths); - yield return CreateInvariantKit(3, 1, 2, paths); //middle child - yield return CreateInvariantKit(4, 1, 3, paths); + //site + yield return CreateInvariantKit(2, 100, 1, paths); + yield return CreateInvariantKit(1, 100, 1, paths); //middle child + yield return CreateInvariantKit(3, 100, 1, paths); + + //children of 1 + yield return CreateInvariantKit(20, 1, 1, paths); + yield return CreateInvariantKit(30, 1, 2, paths); + yield return CreateInvariantKit(40, 1, 3, paths); } //init with all published @@ -1109,7 +1114,7 @@ namespace Umbraco.Tests.PublishedContent var rootKit = _source.Kits[1].Clone(); - void ChangePublishFlagOfRoot(bool published, int assertGen) + void ChangePublishFlagOfRoot(bool published, int assertGen, TreeChangeTypes changeType) { //This will set a flag to force creating a new Gen next time the store is locked (i.e. In Notify) contentStore.CreateSnapshot(); @@ -1124,7 +1129,7 @@ namespace Umbraco.Tests.PublishedContent _snapshotService.Notify(new[] { - new ContentCacheRefresher.JsonPayload(1, Guid.Empty, TreeChangeTypes.RefreshBranch) + new ContentCacheRefresher.JsonPayload(1, Guid.Empty, changeType) }, out _, out _); Assert.AreEqual(assertGen, contentStore.Test.LiveGen); @@ -1132,19 +1137,19 @@ namespace Umbraco.Tests.PublishedContent } //unpublish the root - ChangePublishFlagOfRoot(false, 2); + ChangePublishFlagOfRoot(false, 2, TreeChangeTypes.RefreshBranch); - //publish the root - ChangePublishFlagOfRoot(true, 3); + //publish the root (since it's not published, it will cause a RefreshBranch) + ChangePublishFlagOfRoot(true, 3, TreeChangeTypes.RefreshBranch); //publish root + descendants - ChangePublishFlagOfRoot(true, 4); + ChangePublishFlagOfRoot(true, 4, TreeChangeTypes.RefreshBranch); - //publish the root - ChangePublishFlagOfRoot(true, 5); + //save/publish the root (since it's already published, it will just cause a RefreshNode + ChangePublishFlagOfRoot(true, 5, TreeChangeTypes.RefreshNode); //publish root + descendants - ChangePublishFlagOfRoot(true, 6); //TODO: This should fail, need to figure out what the diff is between this and a website + ChangePublishFlagOfRoot(true, 6, TreeChangeTypes.RefreshBranch); //TODO: This should fail, need to figure out what the diff is between this and a website } [Test] From 439bacff0708bed566d397fec26deddf290b53ab Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 17 Oct 2019 18:41:05 +1100 Subject: [PATCH 3/6] Fixes issue - when calling Set, we were not setting all node graph elements. --- .../PublishedContent/NuCacheChildrenTests.cs | 14 +++++++++---- .../PublishedCache/NuCache/ContentNode.cs | 2 ++ .../PublishedCache/NuCache/ContentStore.cs | 21 ++++++++++++++++--- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs index 7904eed0ec..d0dcd66193 100644 --- a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs +++ b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs @@ -1097,8 +1097,8 @@ namespace Umbraco.Tests.PublishedContent //site yield return CreateInvariantKit(2, 100, 1, paths); - yield return CreateInvariantKit(1, 100, 1, paths); //middle child - yield return CreateInvariantKit(3, 100, 1, paths); + yield return CreateInvariantKit(1, 100, 2, paths); //middle child + yield return CreateInvariantKit(3, 100, 3, paths); //children of 1 yield return CreateInvariantKit(20, 1, 1, paths); @@ -1134,11 +1134,17 @@ namespace Umbraco.Tests.PublishedContent Assert.AreEqual(assertGen, contentStore.Test.LiveGen); Assert.IsTrue(contentStore.Test.NextGen); + + //get the latest gen for content Id 1 + var (gen, contentNode) = contentStore.Test.GetValues(1)[0]; + Assert.AreEqual(assertGen, gen); + //even when unpublishing/re-publishing/etc... the linked list is always maintained + AssertLinkedNode(contentNode, 100, 2, 3, 20, 40); } //unpublish the root ChangePublishFlagOfRoot(false, 2, TreeChangeTypes.RefreshBranch); - + //publish the root (since it's not published, it will cause a RefreshBranch) ChangePublishFlagOfRoot(true, 3, TreeChangeTypes.RefreshBranch); @@ -1149,7 +1155,7 @@ namespace Umbraco.Tests.PublishedContent ChangePublishFlagOfRoot(true, 5, TreeChangeTypes.RefreshNode); //publish root + descendants - ChangePublishFlagOfRoot(true, 6, TreeChangeTypes.RefreshBranch); //TODO: This should fail, need to figure out what the diff is between this and a website + ChangePublishFlagOfRoot(true, 6, TreeChangeTypes.RefreshBranch); } [Test] diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentNode.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentNode.cs index 5f8e81fd31..3b4859432d 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentNode.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentNode.cs @@ -1,4 +1,5 @@ using System; +using System.Diagnostics; using Umbraco.Core.Models.PublishedContent; using Umbraco.Web.PublishedCache.NuCache.DataSource; @@ -6,6 +7,7 @@ namespace Umbraco.Web.PublishedCache.NuCache { // represents a content "node" ie a pair of draft + published versions // internal, never exposed, to be accessed from ContentStore (only!) + [DebuggerDisplay("Id: {Id}, Path: {Path}")] internal class ContentNode { // special ctor for root pseudo node diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index f0d695f090..e90e67c050 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -549,7 +549,10 @@ namespace Umbraco.Web.PublishedCache.NuCache // manage children if (existing != null) + { kit.Node.FirstChildContentId = existing.FirstChildContentId; + kit.Node.LastChildContentId = existing.LastChildContentId; + } // set SetValueLocked(_contentNodes, kit.Node.Id, kit.Node); @@ -571,6 +574,8 @@ namespace Umbraco.Web.PublishedCache.NuCache { // replacing existing, handle siblings kit.Node.NextSiblingContentId = existing.NextSiblingContentId; + //TODO: What about previous sibling?? + kit.Node.PreviousSiblingContentId = existing.PreviousSiblingContentId; } _xmap[kit.Node.Uid] = kit.Node.Id; @@ -729,7 +734,9 @@ namespace Umbraco.Web.PublishedCache.NuCache // clear if (existing != null) { + //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? RemoveTreeNodeLocked(existing); } @@ -1002,11 +1009,19 @@ namespace Umbraco.Web.PublishedCache.NuCache } // else it's going somewhere in the middle, - // and this is bad, perfs-wise - we only do it when moving - // inserting in linked list is slow, optimizing would require trees - // but... that should not happen very often - and not on large amount of data + // TODO: There was a note about performance when this occurs and that this only happens when moving and not very often, but that is not true, + // this also happens anytime a middle node is unpublished or republished (which causes a branch update), i'm unsure if this has perf impacts, + // i think this used to but it doesn't seem bad anymore that I can see... while (child.NextSiblingContentId > 0) { + if (child.NextSiblingContentId == content.Id) + { + content.PreviousSiblingContentId = child.Id; + child = content; + continue; + } + + // get next child var nextChildLink = GetRequiredLinkedNode(child.NextSiblingContentId, "next child", null); var nextChild = nextChildLink.Value; From ea642b3c51b5158b13e6b96df2d6f4b9b98c9460 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 17 Oct 2019 18:43:09 +1100 Subject: [PATCH 4/6] removes TODO --- src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index e90e67c050..8ac7eec2b6 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -574,7 +574,6 @@ namespace Umbraco.Web.PublishedCache.NuCache { // replacing existing, handle siblings kit.Node.NextSiblingContentId = existing.NextSiblingContentId; - //TODO: What about previous sibling?? kit.Node.PreviousSiblingContentId = existing.PreviousSiblingContentId; } From c91680e4c71cdef4f550f7879760b4aad7f42106 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 17 Oct 2019 18:43:54 +1100 Subject: [PATCH 5/6] removes test code --- src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 8ac7eec2b6..f71abd6aa7 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -1013,14 +1013,6 @@ namespace Umbraco.Web.PublishedCache.NuCache // i think this used to but it doesn't seem bad anymore that I can see... while (child.NextSiblingContentId > 0) { - if (child.NextSiblingContentId == content.Id) - { - content.PreviousSiblingContentId = child.Id; - child = content; - continue; - } - - // get next child var nextChildLink = GetRequiredLinkedNode(child.NextSiblingContentId, "next child", null); var nextChild = nextChildLink.Value; From b95404f5854df8732e0a278e66651b6c37ba28b9 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 17 Oct 2019 18:51:15 +1100 Subject: [PATCH 6/6] Adds simpler test to validate RefreshNode --- .../PublishedContent/NuCacheChildrenTests.cs | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs index d0dcd66193..e615933c44 100644 --- a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs +++ b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs @@ -1065,6 +1065,60 @@ namespace Umbraco.Tests.PublishedContent AssertLinkedNode(child3.contentNode, 1, 2, -1, -1, -1); } + [Test] + public void Refresh_Node_Ensures_Linked_list() + { + // NOTE: these tests are not using real scopes, in which case a Scope does not control + // how the snapshots generations work. We are forcing new snapshot generations manually. + + IEnumerable GetKits() + { + var paths = new Dictionary { { -1, "-1" } }; + + //root + yield return CreateInvariantKit(100, -1, 1, paths); + + //site + yield return CreateInvariantKit(2, 100, 1, paths); + yield return CreateInvariantKit(1, 100, 2, paths); //middle child + yield return CreateInvariantKit(3, 100, 3, paths); + + //children of 1 + yield return CreateInvariantKit(20, 1, 1, paths); + yield return CreateInvariantKit(30, 1, 2, paths); + yield return CreateInvariantKit(40, 1, 3, paths); + } + + Init(GetKits()); + + var snapshotService = (PublishedSnapshotService)_snapshotService; + var contentStore = snapshotService.GetContentStore(); + + Assert.AreEqual(1, contentStore.Test.LiveGen); + Assert.IsTrue(contentStore.Test.NextGen); + + var middleNode = contentStore.Test.GetValues(1)[0]; + Assert.AreEqual(1, middleNode.gen); + AssertLinkedNode(middleNode.contentNode, 100, 2, 3, 20, 40); + + //This will set a flag to force creating a new Gen next time the store is locked (i.e. In Notify) + contentStore.CreateSnapshot(); + + Assert.IsFalse(contentStore.Test.NextGen); + + _snapshotService.Notify(new[] + { + new ContentCacheRefresher.JsonPayload(1, Guid.Empty, TreeChangeTypes.RefreshNode) + }, out _, out _); + + Assert.AreEqual(2, contentStore.Test.LiveGen); + Assert.IsTrue(contentStore.Test.NextGen); + + middleNode = contentStore.Test.GetValues(1)[0]; + Assert.AreEqual(2, middleNode.gen); + AssertLinkedNode(middleNode.contentNode, 100, 2, 3, 20, 40); + } + /// /// This addresses issue: https://github.com/umbraco/Umbraco-CMS/issues/6698 ///