Fix length change omission for StringSubstitutor cyclic detection

[nickbabcock]

StringSubstitutor#substitute would throw StringIndexOutOfBoundsException when removing a variable start marker if StringSubstitutor wasn't using the internal representation of TextStringBuilder#buffer. The local variable, priorVariables, contains unbalanced squirrely parentheses (replacing $${${TEST}} would cause priorVariables to add ${${TEST}}} -- amusingly didn't result in unexpected behavior). I discovered this when swapping all instances of buf.buffer to buf.toCharArray(). This PR ensures that both now have equivalent behavior. I was unable to tease out a test for this, as the length is only decremented to that point (but this change in length goes unused, hence the exception for toCharArray), so no overflows. I did port a test from StrSubstitutorTest.java, but the test passes before and after this change.

I believe this to be a trivial change (hence no JIRA ticket). Let me know if otherwise

[coveralls]

Coverage increased (+0.1%) to 98.031% when pulling e291fb5 on nickbabcock:sub-cycle-fix into 8b07e17 on apache:master.

[kinow]

Sorry, I tried to understand the change here, or how to reproduce it, but failed at both.

I did port a test from StrSubstitutorTest.java, but the test passes before and after this change

This is definitely not ideal. Is there any other way to reproduce the error before the fix? I tried a few different things on master, comparing with the output of your branch, but still couldn't reproduce the error you mentioned.

I believe this to be a trivial change (hence no JIRA ticket). Let me know if otherwise

I think this would still require a change, as it appears to prevent an error you had (and which users may have as well). Trivial changes are more javadocs, style changes, rename a local variable, fix unused imports, etc. Anything that doesn't affect users regarding backward compatibility (binary or behaviour wise), and also don't add any new feature.

[nickbabcock]

My apologies!

1. Checkout master branch
2. Apply diff

diff --git a/src/main/java/org/apache/commons/text/StringSubstitutor.java b/src/main/java/org/apache/commons/text/StringSubstitutor.java
index f4b53ee..4680edc 100644
--- a/src/main/java/org/apache/commons/text/StringSubstitutor.java
+++ b/src/main/java/org/apache/commons/text/StringSubstitutor.java
@@ -1330,7 +1330,7 @@ public class StringSubstitutor {
         final boolean top = priorVariables == null;
         boolean altered = false;
         int lengthChange = 0;
-        char[] chars = buf.buffer;
+        char[] chars = buf.toCharArray();
         int bufEnd = offset + length;
         int pos = offset;
         while (pos < bufEnd) {
@@ -1346,7 +1346,7 @@ public class StringSubstitutor {
                         continue;
                     }
                     buf.deleteCharAt(pos - 1);
-                    chars = buf.buffer; // in case buffer was altered
+                    chars = buf.toCharArray(); // in case buffer was altered
                     lengthChange--;
                     altered = true;
                     bufEnd--;
@@ -1432,7 +1432,7 @@ public class StringSubstitutor {
                                     pos += change;
                                     bufEnd += change;
                                     lengthChange += change;
-                                    chars = buf.buffer; // in case buffer was altered
+                                    chars = buf.toCharArray(); // in case buffer was altered
                                 } else if (undefinedVariableException) {
                                     throw new IllegalArgumentException(String.format(
                                             "Cannot resolve variable '%s' (enableSubstitutionInVariables=%s).", varName,

3. Run tests
4. Notice StringIndexOutOfBoundsException is thrown at

if (priorVariables == null) {
    priorVariables = new ArrayList<>();
    priorVariables.add(new String(chars, offset, length));
}

5. Understand "length" points passed the character array but since chars is implicitly relying on TextStringBuilder allocating behavior so the current implementation does not throw exception.
6. The real length is length + lengthChange

[nickbabcock]

I think this would still require a change, as it appears to prevent an error you had (and which users may have as well). Trivial changes are more javadocs, style changes, rename a local variable, fix unused imports, etc. Anything that doesn't affect users regarding backward compatibility (binary or behaviour wise), and also don't add any new feature.

It's a logic bug that can't manifest in commons-text is due to the reliance on internal TextStringBuilder#buffer and how it's implemented. So the number of users effected by this bug is 0. I only came across the bug when a I needed to apply some changes and created the diff posted above.

Let me know if you still want me to create a JIRA ticket 😄

[garydgregory]

I would like feedback from the community on whether or not this should be merged. A failing test would make this a no-brainer of course but I understand this might not be possible here.