simpleqt opened a new pull request, #788:
URL: https://github.com/apache/commons-text/pull/788
## Summary
`StringSubstitutor` ignores the escape character when the variable's escape
character is the first character of the range passed to the offset/length
overloads, so an escaped variable is substituted anyway.
## Root cause
`StringSubstitutor.substitute(TextStringBuilder, int offset, int length,
List)` decides whether a variable prefix is escaped with:
```java
if (pos > offset && builder.charAt(pos - 1) == escapeCh) {
```
`offset` is the start of the range being processed, not the start of the
builder. When a caller substitutes only part of a builder and the escape
character is immediately before that range, `pos == offset` for the prefix, so
the test fails and the escape is not recognised:
- `replaceIn(builder, 0, length)` on `~${user}` → `${user}` (correct)
- `replaceIn(builder, 1, length - 1)` on the same text → `~fred` (escape
ignored, variable substituted)
The whole-string overloads hide the bug because they pass `offset = 0`.
## Reproduction
With `new StringSubstitutor(map, "${", "}", '~')` and `map.put("user",
"fred")`:
```java
TextStringBuilder b = new TextStringBuilder("~${user}");
sub.replaceIn(b, 1, b.length() - 1);
// before: "~fred" expected: "${user}"
```
`~` here is the escape character; the caller's intent is that `${user}` is
left literal, exactly as it is when the whole builder is processed.
## Fix
The escape character is now honoured when it lies in the range being
substituted **or immediately before it**, because the character directly before
the range is the one that prefixes the range's first position:
```java
final int escStart = offset > 0 ? offset - 1 : offset;
...
if (pos > escStart && builder.charAt(pos - 1) == escapeCh) {
```
This keeps the existing behaviour for `offset == 0` (and for ranges shorter
than one character), and makes substitution of a sub-range agree with
substitution of the whole builder. An escape character that is not the
variable's own escape is still left untouched, and an escape character inside
the range behaves as before.
## Tests
Added `StringSubstitutorTest.testSubstituteEscapeAtStartOfRange`, which
covers: the whole builder, the escape character immediately before the
substituted range, the same followed by an unescaped variable, an escape
character inside the range, and an unrelated escape character before the range
that must be left alone.
Evidence on the new test:
- Without the production change: `expected: <${user}> but was: <~fred>` — 80
tests run, 1 failure.
- With the change: 80 tests run, 0 failures.
Full verification with the default `mvn` goal: 2045 tests, 0 failures;
checkstyle, PMD and SpotBugs all pass.
Note: 6 tests in `PathFenceResolverTest` and `FileStringLookupTest` error on
this Windows machine with `IllegalArgumentException: 'other' has different
root`, because their `write(Path, String)` helper does
`CURRENT_PATH.toAbsolutePath().relativize(path.toAbsolutePath())` and the
`@TempDir` is on `C:\` while the working directory is on `G:\`. Those 6 errors
are present on unmodified `master` and are unrelated to this change.
## Jira
`CONTRIBUTING.md` asks for a Jira ticket, and `RELEASE-NOTES.txt` is
generated from Jira. I do not have an Apache Jira account, so there is no
ticket key to reference here; the commit message has no Jira key for that
reason. Happy to update the commit message and add a `RELEASE-NOTES.txt` entry
if a ticket is created and a key is provided.
## AI disclosure
An AI agent assisted with investigating, implementing and testing this
change, and with drafting this description. The change was reviewed and
verified by the submitter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
