-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[LANG-1753] StringUtils.replaceEachRepeatedly regression in 3.11+ #1297
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.
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
Conversation
| */ | ||
| 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); |
There was a problem hiding this comment.
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.
| assertEquals("a", StringUtils.replaceEachRepeatedly("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", | ||
| new String[]{"aa"}, new String[]{"a"})); |
There was a problem hiding this comment.
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.
|
Thanks for the fix! Seems perfect for us. |
### What changes were proposed in this pull request? This PR aims to upgrade `commons-lang3` to 3.18.0. Although SPARK-52469 removes ` org.apache.commons.lang3.JavaVersion` usages, we still use many methods of `commons-lang3`. - #51174 ### Why are the changes needed? To bring the latest bug fixes. - https://commons.apache.org/proper/commons-lang/changes.html#a3.18.0 - apache/commons-lang/pull/1327 - apache/commons-lang/pull/1297 - apache/commons-lang/pull/1273 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #51460 from dongjoon-hyun/SPARK-52778. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: yangjie01 <yangjie01@baidu.com>
### What changes were proposed in this pull request? This PR aims to upgrade `commons-lang3` to 3.18.0. Although SPARK-52469 removes ` org.apache.commons.lang3.JavaVersion` usages, we still use many methods of `commons-lang3`. - #51174 ### Why are the changes needed? To bring the latest bug fixes. - https://commons.apache.org/proper/commons-lang/changes.html#a3.18.0 - apache/commons-lang/pull/1327 - apache/commons-lang/pull/1297 - apache/commons-lang/pull/1273 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #51460 from dongjoon-hyun/SPARK-52778. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: yangjie01 <yangjie01@baidu.com>
https://issues.apache.org/jira/browse/LANG-1753
The method
replaceEachRepeatedlycallsreplaceEachrepeatedly until no more replacements are possible ortimeToLive < 0. ThetimeToLivestarts with the value ofsearchList.length. The current implementation can't distinguish between infinite loops and deeply nested string patterns. In both cases, it continues replacements until it hits the time-to-live limit and throwsIllegalStateExceptionif replacements are still possible.The problem with LANG-1528 fix is that in some cases, it allows partially replaced strings after TTL is exceeded. This fix reverts
replaceEachlogic back to version 3.10 (see PR #505), but allows deeper dives due to an increased initial TTL value. The initial TTL value is now set to the greater ofsearchList.lengthor up to 5 levels deep. This default value was chosen based on rational considerations.