From a0785f2e0d8bd7e5486d1e155b8f69d76d42c57f Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 23 Aug 2017 16:22:01 +1000 Subject: [PATCH] backports U4-10349 Optimize EnsureUniqueNodeName --- .../Repositories/ContentRepository.cs | 27 +---- .../DataTypeDefinitionRepository.cs | 29 +---- .../Repositories/MediaRepository.cs | 27 +---- .../Repositories/SimilarNodeName.cs | 112 ++++++++++++++++++ .../Repositories/SimilarNodeNameComparer.cs | 45 ------- src/Umbraco.Core/Umbraco.Core.csproj | 2 +- .../Repositories/SimilarNodeNameTests.cs | 98 +++++++++++++++ src/Umbraco.Tests/Umbraco.Tests.csproj | 1 + 8 files changed, 221 insertions(+), 120 deletions(-) create mode 100644 src/Umbraco.Core/Persistence/Repositories/SimilarNodeName.cs delete mode 100644 src/Umbraco.Core/Persistence/Repositories/SimilarNodeNameComparer.cs create mode 100644 src/Umbraco.Tests/Persistence/Repositories/SimilarNodeNameTests.cs diff --git a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs index f3d199e175..eac9d197be 100644 --- a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs @@ -1122,31 +1122,10 @@ ORDER BY cmsContentVersion.id DESC if (EnsureUniqueNaming == false) return nodeName; - var sql = new Sql(); - sql.Select("*") - .From() - .Where(x => x.NodeObjectType == NodeObjectTypeId && x.ParentId == parentId && x.Text.StartsWith(nodeName)); + var names = Database.Fetch("SELECT id, text AS name FROM umbracoNode WHERE nodeObjectType=@objectType AND parentId=@parentId", + new { objectType = NodeObjectTypeId, parentId }); - int uniqueNumber = 1; - var currentName = nodeName; - - var dtos = Database.Fetch(sql); - if (dtos.Any()) - { - var results = dtos.OrderBy(x => x.Text, new SimilarNodeNameComparer()); - foreach (var dto in results) - { - if (id != 0 && id == dto.NodeId) continue; - - if (dto.Text.ToLowerInvariant().Equals(currentName.ToLowerInvariant())) - { - currentName = nodeName + string.Format(" ({0})", uniqueNumber); - uniqueNumber++; - } - } - } - - return currentName; + return SimilarNodeName.GetUniqueName(names, id, nodeName); } /// diff --git a/src/Umbraco.Core/Persistence/Repositories/DataTypeDefinitionRepository.cs b/src/Umbraco.Core/Persistence/Repositories/DataTypeDefinitionRepository.cs index 1690b36148..eb64ea190b 100644 --- a/src/Umbraco.Core/Persistence/Repositories/DataTypeDefinitionRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/DataTypeDefinitionRepository.cs @@ -450,33 +450,10 @@ AND umbracoNode.id <> @id", private string EnsureUniqueNodeName(string nodeName, int id = 0) { + var names = Database.Fetch("SELECT id, text AS name FROM umbracoNode WHERE nodeObjectType=@objectType", + new { objectType = NodeObjectTypeId }); - - var sql = new Sql(); - sql.Select("*") - .From(SqlSyntax) - .Where(x => x.NodeObjectType == NodeObjectTypeId && x.Text.StartsWith(nodeName)); - - int uniqueNumber = 1; - var currentName = nodeName; - - var dtos = Database.Fetch(sql); - if (dtos.Any()) - { - var results = dtos.OrderBy(x => x.Text, new SimilarNodeNameComparer()); - foreach (var dto in results) - { - if (id != 0 && id == dto.NodeId) continue; - - if (dto.Text.ToLowerInvariant().Equals(currentName.ToLowerInvariant())) - { - currentName = nodeName + string.Format(" ({0})", uniqueNumber); - uniqueNumber++; - } - } - } - - return currentName; + return SimilarNodeName.GetUniqueName(names, id, nodeName); } /// diff --git a/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs b/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs index c9f06c6c75..598b16d78d 100644 --- a/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/MediaRepository.cs @@ -584,31 +584,10 @@ namespace Umbraco.Core.Persistence.Repositories if (EnsureUniqueNaming == false) return nodeName; - var sql = new Sql(); - sql.Select("*") - .From() - .Where(x => x.NodeObjectType == NodeObjectTypeId && x.ParentId == parentId && x.Text.StartsWith(nodeName)); + var names = Database.Fetch("SELECT id, text AS name FROM umbracoNode WHERE nodeObjectType=@objectType AND parentId=@parentId", + new { objectType = NodeObjectTypeId, parentId }); - int uniqueNumber = 1; - var currentName = nodeName; - - var dtos = Database.Fetch(sql); - if (dtos.Any()) - { - var results = dtos.OrderBy(x => x.Text, new SimilarNodeNameComparer()); - foreach (var dto in results) - { - if (id != 0 && id == dto.NodeId) continue; - - if (dto.Text.ToLowerInvariant().Equals(currentName.ToLowerInvariant())) - { - currentName = nodeName + string.Format(" ({0})", uniqueNumber); - uniqueNumber++; - } - } - } - - return currentName; + return SimilarNodeName.GetUniqueName(names, id, nodeName); } /// diff --git a/src/Umbraco.Core/Persistence/Repositories/SimilarNodeName.cs b/src/Umbraco.Core/Persistence/Repositories/SimilarNodeName.cs new file mode 100644 index 0000000000..371f73b27f --- /dev/null +++ b/src/Umbraco.Core/Persistence/Repositories/SimilarNodeName.cs @@ -0,0 +1,112 @@ +using System; +using System.Collections.Generic; +using System.Linq; + +namespace Umbraco.Core.Persistence.Repositories +{ + internal class SimilarNodeName + { + private int _numPos = -2; + + public int Id { get; set; } + public string Name { get; set; } + + // cached - reused + public int NumPos + { + get + { + if (_numPos != -2) return _numPos; + + var name = Name; + + if (name[name.Length - 1] != ')') + return _numPos = -1; + + var pos = name.LastIndexOf('('); + if (pos < 2 || pos == name.Length - 2) // < 2 and not < 0, because we want at least "x (" + return _numPos = -1; + + return _numPos = pos; + } + } + + // not cached - used only once + public int NumVal + { + get + { + if (NumPos < 0) + throw new InvalidOperationException(); + int num; + if (int.TryParse(Name.Substring(NumPos + 1, Name.Length - 2 - NumPos), out num)) + return num; + return 0; + } + } + + // compare without allocating, nor parsing integers + internal class Comparer : IComparer + { + public int Compare(SimilarNodeName x, SimilarNodeName y) + { + if (x == null) throw new ArgumentNullException("x"); + if (y == null) throw new ArgumentNullException("y"); + + var xpos = x.NumPos; + var ypos = y.NumPos; + + var xname = x.Name; + var yname = y.Name; + + if (xpos < 0 || ypos < 0 || xpos != ypos) + return string.Compare(xname, yname, StringComparison.Ordinal); + + // compare the part before (number) + var n = string.Compare(xname, 0, yname, 0, xpos, StringComparison.Ordinal); + if (n != 0) + return n; + + // compare (number) lengths + var diff = xname.Length - yname.Length; + if (diff != 0) return diff < 0 ? -1 : +1; + + // actually compare (number) + var i = xpos; + while (i < xname.Length - 1) + { + if (xname[i] != yname[i]) + return xname[i] < yname[i] ? -1 : +1; + i++; + } + return 0; + } + } + + // gets a unique name + public static string GetUniqueName(IEnumerable names, int nodeId, string nodeName) + { + var uniqueNumber = 1; + var uniqueing = false; + foreach (var name in names.OrderBy(x => x, new Comparer())) + { + // ignore self + if (nodeId != 0 && name.Id == nodeId) continue; + + if (uniqueing) + { + if (name.NumPos > 0 && name.Name.StartsWith(nodeName) && name.NumVal == uniqueNumber) + uniqueNumber++; + else + break; + } + else if (name.Name.InvariantEquals(nodeName)) + { + uniqueing = true; + } + } + + return uniqueing ? string.Concat(nodeName, " (", uniqueNumber.ToString(), ")") : nodeName; + } + } +} \ No newline at end of file diff --git a/src/Umbraco.Core/Persistence/Repositories/SimilarNodeNameComparer.cs b/src/Umbraco.Core/Persistence/Repositories/SimilarNodeNameComparer.cs deleted file mode 100644 index ce25486422..0000000000 --- a/src/Umbraco.Core/Persistence/Repositories/SimilarNodeNameComparer.cs +++ /dev/null @@ -1,45 +0,0 @@ -using System; -using System.Collections.Generic; - -namespace Umbraco.Core.Persistence.Repositories -{ - /// - /// Comparer that takes into account the duplicate index of a node name - /// This is needed as a normal alphabetic sort would go Page (1), Page (10), Page (2) etc. - /// - internal class SimilarNodeNameComparer : IComparer - { - public int Compare(string x, string y) - { - if (x.LastIndexOf('(') != -1 && x.LastIndexOf(')') == x.Length - 1 && y.LastIndexOf(')') == y.Length - 1) - { - if (x.ToLower().Substring(0, x.LastIndexOf('(')) == y.ToLower().Substring(0, y.LastIndexOf('('))) - { - int xDuplicateIndex = ExtractDuplicateIndex(x); - int yDuplicateIndex = ExtractDuplicateIndex(y); - - if (xDuplicateIndex != 0 && yDuplicateIndex != 0) - { - return xDuplicateIndex.CompareTo(yDuplicateIndex); - } - } - } - return String.Compare(x.ToLower(), y.ToLower(), StringComparison.Ordinal); - } - - private int ExtractDuplicateIndex(string text) - { - int index = 0; - - if (text.LastIndexOf('(') != -1 && text.LastIndexOf('(') < text.Length - 2) - { - int startPos = text.LastIndexOf('(') + 1; - int length = text.Length - 1 - startPos; - - int.TryParse(text.Substring(startPos, length), out index); - } - - return index; - } - } -} \ No newline at end of file diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 24fdf32150..228905226f 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -586,6 +586,7 @@ + @@ -1188,7 +1189,6 @@ - diff --git a/src/Umbraco.Tests/Persistence/Repositories/SimilarNodeNameTests.cs b/src/Umbraco.Tests/Persistence/Repositories/SimilarNodeNameTests.cs new file mode 100644 index 0000000000..72f3e39874 --- /dev/null +++ b/src/Umbraco.Tests/Persistence/Repositories/SimilarNodeNameTests.cs @@ -0,0 +1,98 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; +using NUnit.Framework; +using Umbraco.Core.Persistence.Repositories; + +namespace Umbraco.Tests.Persistence.Repositories +{ + [TestFixture] + public class SimilarNodeNameTests + { + [TestCase("Alpha", "Alpha", 0)] + [TestCase("Alpha", "ALPHA", +1)] // case is important + [TestCase("Alpha", "Bravo", -1)] + [TestCase("Bravo", "Alpha", +1)] + [TestCase("Alpha (1)", "Alpha (1)", 0)] + [TestCase("Alpha", "Alpha (1)", -1)] + [TestCase("Alpha (1)", "Alpha", +1)] + [TestCase("Alpha (1)", "Alpha (2)", -1)] + [TestCase("Alpha (2)", "Alpha (1)", +1)] + [TestCase("Alpha (2)", "Alpha (10)", -1)] // this is the real stuff + [TestCase("Alpha (10)", "Alpha (2)", +1)] // this is the real stuff + [TestCase("Kilo", "Golf (2)", +1)] + [TestCase("Kilo (1)", "Golf (2)", +1)] + public void ComparerTest(string name1, string name2, int expected) + { + var comparer = new SimilarNodeName.Comparer(); + + var result = comparer.Compare(new SimilarNodeName { Name = name1 }, new SimilarNodeName { Name = name2 }); + if (expected == 0) + Assert.AreEqual(0, result); + else if (expected < 0) + Assert.IsTrue(result < 0, "Expected <0 but was " + result); + else if (expected > 0) + Assert.IsTrue(result > 0, "Expected >0 but was " + result); + } + + [Test] + public void OrderByTest() + { + var names = new[] + { + new SimilarNodeName { Id = 1, Name = "Alpha (2)" }, + new SimilarNodeName { Id = 2, Name = "Alpha" }, + new SimilarNodeName { Id = 3, Name = "Golf" }, + new SimilarNodeName { Id = 4, Name = "Zulu" }, + new SimilarNodeName { Id = 5, Name = "Mike" }, + new SimilarNodeName { Id = 6, Name = "Kilo (1)" }, + new SimilarNodeName { Id = 7, Name = "Yankee" }, + new SimilarNodeName { Id = 8, Name = "Kilo" }, + new SimilarNodeName { Id = 9, Name = "Golf (2)" }, + new SimilarNodeName { Id = 10, Name = "Alpha (1)" }, + }; + + var ordered = names.OrderBy(x => x, new SimilarNodeName.Comparer()).ToArray(); + + var i = 0; + Assert.AreEqual(2, ordered[i++].Id); + Assert.AreEqual(10, ordered[i++].Id); + Assert.AreEqual(1, ordered[i++].Id); + Assert.AreEqual(3, ordered[i++].Id); + Assert.AreEqual(9, ordered[i++].Id); + Assert.AreEqual(8, ordered[i++].Id); + Assert.AreEqual(6, ordered[i++].Id); + Assert.AreEqual(5, ordered[i++].Id); + Assert.AreEqual(7, ordered[i++].Id); + Assert.AreEqual(4, ordered[i++].Id); + } + + [TestCase(0, "Charlie", "Charlie")] + [TestCase(0, "Zulu", "Zulu (1)")] + [TestCase(0, "Golf", "Golf (1)")] + [TestCase(0, "Kilo", "Kilo (2)")] + [TestCase(0, "Alpha", "Alpha (3)")] + [TestCase(0, "Kilo (1)", "Kilo (1) (1)")] // though... we might consider "Kilo (2)" + [TestCase(6, "Kilo (1)", "Kilo (1)")] // because of the id + public void Test(int nodeId, string nodeName, string expected) + { + var names = new[] + { + new SimilarNodeName { Id = 1, Name = "Alpha (2)" }, + new SimilarNodeName { Id = 2, Name = "Alpha" }, + new SimilarNodeName { Id = 3, Name = "Golf" }, + new SimilarNodeName { Id = 4, Name = "Zulu" }, + new SimilarNodeName { Id = 5, Name = "Mike" }, + new SimilarNodeName { Id = 6, Name = "Kilo (1)" }, + new SimilarNodeName { Id = 7, Name = "Yankee" }, + new SimilarNodeName { Id = 8, Name = "Kilo" }, + new SimilarNodeName { Id = 9, Name = "Golf (2)" }, + new SimilarNodeName { Id = 10, Name = "Alpha (1)" }, + }; + + Assert.AreEqual(expected, SimilarNodeName.GetUniqueName(names, nodeId, nodeName)); + } + } +} diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index 067109d8bf..6859cb899f 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -163,6 +163,7 @@ +