[api][java] Substitute prompt placeholders in a single pass#908
Conversation
github-actions
Bot
added
doc-label-missing
Zhuoxi2000
force-pushed
the
fix-prompt-single-pass-substitution
branch
from
3f71645 to
751dc86
Compare
|
the only failing check is could a committer rerun the failed job when convenient? Happy to investigate if it reproduces. Thanks! |
| // Unknown placeholder: leave it as-is. | ||
| replacement = matcher.group(0); | ||
| } | ||
| matcher.appendReplacement(result, Matcher.quoteReplacement(replacement)); |
There was a problem hiding this comment.
One small optional thought: The switch from String.replace to appendReplacement quietly changes how a value's own characters are treated: String.replace inserted values literally, whereas appendReplacement reads $ as a group reference and \ as an escape — so quoteReplacement(...) here is doing real load-bearing work (a value like "$5.00" would otherwise throw). The new tests all use $-free values, so nothing currently locks that behavior in. Would it be worth one assertion with a value containing a $ (say {price} -> "$5.00 (was $9)") so a future refactor can't drop the quoteReplacement call unnoticed? Something like this, if useful:
@Test
@DisplayName("Values containing $ and \\ are inserted literally")
void testValueWithRegexReplacementChars() {
Prompt prompt = Prompt.fromText("Price: {price}");
Map<String, String> vars = new HashMap<>();
vars.put("price", "$5.00 (was $9) \\ end");
assertEquals("Price: $5.00 (was $9) \\ end", prompt.formatString(vars));
}Python's re.sub with a function replacement has no $/\ sensitivity, so a mirror Python test is optional — this one is really a Java-only guard.
There was a problem hiding this comment.
Thanks @weiqingy great catch you're right, I tried removing quoteReplacement,it indeed makes the test fail it is doing important work here. I've added the suggested Java test
I kept this Java-only, as you suggested, since Python's re.sub with a function replacement doesn't have the same $/\ behavior. Thanks for catching this again!
Same failing check unrelated to this change
There was a problem hiding this comment.
Thanks for the fix. Nothing further from me; I'll leave the final call to the maintainers.
wenjin272
added
doc-not-needed
LocalPrompt.format iterated over the variables map and applied
String.replace once per entry. Because each replacement scanned the
whole partially-substituted string, text introduced by one variable's
value could be re-interpreted as another placeholder, and the result
depended on the map's iteration order. A caller-supplied value
containing e.g. "{secret}" could be expanded into another variable's
value.
Scan the template once and replace each {key} from the original text
only, leaving unknown placeholders untouched. This removes the order
dependence and the value-injection path, and matches the Python
SafeFormatter, which already substitutes in a single pass.
Add regression tests in both languages covering re-expansion and the
value-injection case, through both formatString/format_string and
formatMessages/format_messages (which share the same substitution
path). A Java test also locks in that values containing regex
replacement characters ($, \\) are inserted literally, since the
single-pass implementation relies on Matcher.quoteReplacement.
Closes apache#907
Zhuoxi2000
force-pushed
the
fix-prompt-single-pass-substitution
branch
from
751dc86 to
7972731
Compare
wenjin272
left a comment
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for taking this on @Zhuoxi2000. LGTM.
(cherry picked from commit ad0a935)
3 participants
Linked issue: #907
Purpose of change
LocalPrompt.formatpreviously appliedString.replaceonce per variable. This meant that substituted values could be interpreted again as placeholders, making the result dependent on map iteration order and allowing a value such as{secret}to expand into another variable's value.This change scans the original template once and replaces only placeholders found in that template. Unknown placeholders are left untouched, and substituted values are inserted literally rather than processed again.
This also restores parity with Python's
SafeFormatter, which already performs substitution in a single pass.Tests
Added regression tests in both Java and Python covering:
The new Java tests fail on
mainand pass with this change.Validation:
mvn -pl api test: 276 run, 0 failures, 9 skippedapi/tests/test_prompt.py: 10 passedmvn -pl api spotless:check: cleanruff check: cleanruff format --check: cleanAPI
No public API signatures are changed. This only fixes placeholder substitution behavior for values that themselves contain
{...}tokens matching another provided variable. Normal template formatting is unaffected.Documentation
doc-neededdoc-not-neededdoc-included