From c1781c2f4b38a7f090e748fc58dd06e040f011f2 Mon Sep 17 00:00:00 2001 From: John Date: Tue, 29 Apr 2014 12:53:51 +0200 Subject: [PATCH 1/4] U4-2635 add failing test Only first occurrence of oldString is replaced. --- src/Umbraco.Tests/CoreStrings/StringExtensionsTests.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/Umbraco.Tests/CoreStrings/StringExtensionsTests.cs b/src/Umbraco.Tests/CoreStrings/StringExtensionsTests.cs index 1474bd98f9..d1c7efa489 100644 --- a/src/Umbraco.Tests/CoreStrings/StringExtensionsTests.cs +++ b/src/Umbraco.Tests/CoreStrings/StringExtensionsTests.cs @@ -80,6 +80,15 @@ namespace Umbraco.Tests.CoreStrings Assert.AreEqual(shouldBe, trimmed); } + [TestCase("Hello this is my string", "hello", "replaced", "replaced this is my string", StringComparison.CurrentCultureIgnoreCase)] + [TestCase("Hello this is hello my string", "hello", "replaced", "replaced this is replaced my string", StringComparison.CurrentCultureIgnoreCase)] + [TestCase("Hello this is my string", "nonexistent", "replaced", "Hello this is my string", StringComparison.CurrentCultureIgnoreCase)] + public void ReplaceWithStringComparison(string input, string oldString, string newString, string shouldBe, StringComparison stringComparison) + { + var replaced = input.Replace(oldString, newString, stringComparison); + Assert.AreEqual(shouldBe, replaced); + } + [TestCase(null, null)] [TestCase("", "")] [TestCase("x", "X")] From 86f7d79e7a326a90c299c5777dbaff8dc9a95f99 Mon Sep 17 00:00:00 2001 From: John Date: Tue, 29 Apr 2014 13:03:52 +0200 Subject: [PATCH 2/4] Fixes: U4-2635 Umbraco.Core.Strings string Rep... ...lace(this string source, string oldString, string newString, StringComparison stringComparison) method only replaces first ocurrence of oldString --- src/Umbraco.Core/StringExtensions.cs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Core/StringExtensions.cs b/src/Umbraco.Core/StringExtensions.cs index 6a2784cc98..0da99b42f0 100644 --- a/src/Umbraco.Core/StringExtensions.cs +++ b/src/Umbraco.Core/StringExtensions.cs @@ -1191,17 +1191,15 @@ namespace Umbraco.Core /// Updated string public static string Replace(this string source, string oldString, string newString, StringComparison stringComparison) { - var index = source.IndexOf(oldString, stringComparison); + int index; - // Determine if we found a match - var matchFound = index >= 0; - - if (matchFound) + // Determine if there are any matches left in source. + while((index = source.IndexOf(oldString, stringComparison)) >= 0) { - // Remove the old text + // Remove the old text. source = source.Remove(index, oldString.Length); - // Add the replacemenet text + // Add the replacemenet text. source = source.Insert(index, newString); } From 74dd1f9181422c40753c614212d97dcca3298c18 Mon Sep 17 00:00:00 2001 From: John Date: Tue, 29 Apr 2014 13:33:48 +0200 Subject: [PATCH 3/4] U4-2635 Skip already-checked part of source input --- src/Umbraco.Core/StringExtensions.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Core/StringExtensions.cs b/src/Umbraco.Core/StringExtensions.cs index 0da99b42f0..c7ea0c43dd 100644 --- a/src/Umbraco.Core/StringExtensions.cs +++ b/src/Umbraco.Core/StringExtensions.cs @@ -1191,10 +1191,11 @@ namespace Umbraco.Core /// Updated string public static string Replace(this string source, string oldString, string newString, StringComparison stringComparison) { - int index; + // Initialised to zero so it can be used as startIndex on first iteration. + int index = 0; - // Determine if there are any matches left in source. - while((index = source.IndexOf(oldString, stringComparison)) >= 0) + // Determine if there are any matches left in source, starting from last found match. + while((index = source.IndexOf(oldString, index, stringComparison)) >= 0) { // Remove the old text. source = source.Remove(index, oldString.Length); From b4127294e51a3ffc3971bda30ec0d5cec8a1049c Mon Sep 17 00:00:00 2001 From: John Date: Tue, 29 Apr 2014 13:54:26 +0200 Subject: [PATCH 4/4] U4-2635 Skip past replaced word when checking Avoids infinite loop if search and replacement are equal --- src/Umbraco.Core/StringExtensions.cs | 10 ++++++---- src/Umbraco.Tests/CoreStrings/StringExtensionsTests.cs | 3 +++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Core/StringExtensions.cs b/src/Umbraco.Core/StringExtensions.cs index c7ea0c43dd..6e1b367669 100644 --- a/src/Umbraco.Core/StringExtensions.cs +++ b/src/Umbraco.Core/StringExtensions.cs @@ -1191,11 +1191,13 @@ namespace Umbraco.Core /// Updated string public static string Replace(this string source, string oldString, string newString, StringComparison stringComparison) { - // Initialised to zero so it can be used as startIndex on first iteration. - int index = 0; + // This initialisation ensures the first check starts at index zero of the source. On successive checks for + // a match, the source is skipped to immediately after the last replaced occurrence for efficiency + // and to avoid infinite loops when oldString and newString compare equal. + int index = -1 * newString.Length; - // Determine if there are any matches left in source, starting from last found match. - while((index = source.IndexOf(oldString, index, stringComparison)) >= 0) + // Determine if there are any matches left in source, starting from just after the result of replacing the last match. + while((index = source.IndexOf(oldString, index + newString.Length, stringComparison)) >= 0) { // Remove the old text. source = source.Remove(index, oldString.Length); diff --git a/src/Umbraco.Tests/CoreStrings/StringExtensionsTests.cs b/src/Umbraco.Tests/CoreStrings/StringExtensionsTests.cs index d1c7efa489..89d03baa2d 100644 --- a/src/Umbraco.Tests/CoreStrings/StringExtensionsTests.cs +++ b/src/Umbraco.Tests/CoreStrings/StringExtensionsTests.cs @@ -83,6 +83,9 @@ namespace Umbraco.Tests.CoreStrings [TestCase("Hello this is my string", "hello", "replaced", "replaced this is my string", StringComparison.CurrentCultureIgnoreCase)] [TestCase("Hello this is hello my string", "hello", "replaced", "replaced this is replaced my string", StringComparison.CurrentCultureIgnoreCase)] [TestCase("Hello this is my string", "nonexistent", "replaced", "Hello this is my string", StringComparison.CurrentCultureIgnoreCase)] + [TestCase("Hellohello this is my string", "hello", "replaced", "replacedreplaced this is my string", StringComparison.CurrentCultureIgnoreCase)] + // Ensure replacing with the same string doesn't cause infinite loop. + [TestCase("Hello this is my string", "hello", "hello", "hello this is my string", StringComparison.CurrentCultureIgnoreCase)] public void ReplaceWithStringComparison(string input, string oldString, string newString, string shouldBe, StringComparison stringComparison) { var replaced = input.Replace(oldString, newString, stringComparison);