Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LANG-1753] StringUtils.replaceEachRepeatedly regression in 3.11+ #1297

Merged
merged 1 commit into from
Oct 15, 2024
Merged

[LANG-1753] StringUtils.replaceEachRepeatedly regression in 3.11+ #1297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 12 additions & 14 deletions src/main/java/org/apache/commons/lang3/StringUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,10 @@
import java.text.Normalizer;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Locale;
import java.util.Objects;
import java.util.Set;
import java.util.function.Supplier;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -181,6 +179,11 @@ public class StringUtils {
*/
private static final int PAD_LIMIT = 8192;

/**
* The default maximum depth at which recursive replacement will continue until no further search replacements are possible.
*/
private static final int DEFAULT_TTL = 5;

/**
* Pattern used in {@link #stripAccents(String)}.
*/
Expand Down Expand Up @@ -6450,20 +6453,14 @@ private static String replaceEach(

// mchyzer Performance note: This creates very few new objects (one major goal)
// let me know if there are performance requests, we can create a harness to measure
if (isEmpty(text) || ArrayUtils.isEmpty(searchList) || ArrayUtils.isEmpty(replacementList)) {
return text;
}

// if recursing, this shouldn't be less than 0
if (timeToLive < 0) {
final Set<String> searchSet = new HashSet<>(Arrays.asList(searchList));
final Set<String> replacementSet = new HashSet<>(Arrays.asList(replacementList));
searchSet.retainAll(replacementSet);
if (!searchSet.isEmpty()) {
throw new IllegalStateException("Aborting to protect against StackOverflowError - " +
"output of one loop is the input of another");
}
}

if (isEmpty(text) || ArrayUtils.isEmpty(searchList) || ArrayUtils.isEmpty(replacementList) || ArrayUtils.isNotEmpty(searchList) && timeToLive == -1) {
return text;
throw new IllegalStateException("Aborting to protect against StackOverflowError - " +
"output of one loop is the input of another");
}

final int searchLength = searchList.length;
Expand Down Expand Up @@ -6611,7 +6608,8 @@ private static String replaceEach(
* @since 2.4
*/
public static String replaceEachRepeatedly(final String text, final String[] searchList, final String[] replacementList) {
return replaceEach(text, searchList, replacementList, true, ArrayUtils.getLength(searchList));
int timeToLive = Math.max(ArrayUtils.getLength(searchList), DEFAULT_TTL);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Javadoc only mentions endless loops ("if the search is repeating and there is an endless loop"). It would be clearer if it also included a mention of the TTL to avoid confusion.

return replaceEach(text, searchList, replacementList, true, timeToLive);
}

/**
Expand Down
48 changes: 47 additions & 1 deletion src/test/java/org/apache/commons/lang3/StringUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1990,7 +1990,53 @@ public void testReplace_StringStringArrayStringArrayBoolean() {
assertEquals("aba", StringUtils.replaceEachRepeatedly("aba", new String[]{null}, new String[]{"a"}));
assertEquals("wcte", StringUtils.replaceEachRepeatedly("abcde", new String[]{"ab", "d"}, new String[]{"w", "t"}));
assertEquals("tcte", StringUtils.replaceEachRepeatedly("abcde", new String[]{"ab", "d"}, new String[]{"d", "t"}));
assertEquals("blaan", StringUtils.replaceEachRepeatedly("blllaan", new String[]{"llaan"}, new String[]{"laan"}) );

// Test recursive replacement - LANG-1528 & LANG-1753
assertEquals("blaan", StringUtils.replaceEachRepeatedly("blllaan", new String[]{"llaan"}, new String[]{"laan"}));
assertEquals("blaan", StringUtils.replaceEachRepeatedly("bllllaan", new String[]{"llaan"}, new String[]{"laan"}));

// Test default TTL for smaller search lists. 32 characters reduced to 16, then 8, 4, 2, 1.
assertEquals("a", StringUtils.replaceEachRepeatedly("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
new String[]{"aa"}, new String[]{"a"}));
Comment on lines +1999 to +2000
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest adding an example of IllegalStateException with a shorter input, like:

assertThrows(
	IllegalStateException.class,
	() -> StringUtils.replaceEachRepeatedly("aaaaaab", new String[] {"ab"}, new String[] {"b"}),
	"Should be a circular reference");

It might surprise newcomers that even far fewer than 32 repetitions can trigger an IllegalStateException.


// Test default TTL exceeded. 33 characters reduced to 17, then 9, 5, 3, 2 (still found).
assertThrows(
IllegalStateException.class,
() -> StringUtils.replaceEachRepeatedly("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
new String[]{"aa"}, new String[]{"a"}),
"Cannot be resolved within the default time-to-live limit");

// Test larger TTL for larger search lists. Replace repeatedly until there are no more possible replacements.
assertEquals("000000000", StringUtils.replaceEachRepeatedly("aA0aA0aA0",
new String[]{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n",
"o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z", "A", "B", "C", "D",
"E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T",
"U", "V", "W", "X", "Y", "Z", "1", "2", "3", "4", "5", "6", "7", "8", "9"},
new String[]{"b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o",
"p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z", "A", "B", "C", "D", "E",
"F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U",
"V", "W", "X", "Y", "Z", "1", "2", "3", "4", "5", "6", "7", "8", "9", "0"}));

// Test long infinite cycle: a -> b -> ... -> 9 -> 0 -> a -> b -> ...
assertThrows(
IllegalStateException.class,
() -> StringUtils.replaceEachRepeatedly("a",
new String[]{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n",
"o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z", "A", "B", "C", "D",
"E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T",
"U", "V", "W", "X", "Y", "Z", "1", "2", "3", "4", "5", "6", "7", "8", "9", "0"},
new String[]{"b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o",
"p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z", "A", "B", "C", "D", "E",
"F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U",
"V", "W", "X", "Y", "Z", "1", "2", "3", "4", "5", "6", "7", "8", "9", "0", "a"}),
"Should be a circular reference");

assertThrows(
IllegalStateException.class,
() -> StringUtils.replaceEachRepeatedly("%{key1}",
new String[] {"%{key1}", "%{key2}", "%{key3}"},
new String[] {"Key1 %{key2}", "Key2 %{key3}", "Key3 %{key1}"}),
"Should be a circular reference");

assertThrows(
IllegalStateException.class,
Expand Down
Loading