weiqingy commented on code in PR #908:
URL: https://github.com/apache/flink-agents/pull/908#discussion_r3584904521
##########
api/src/main/java/org/apache/flink/agents/api/prompt/Prompt.java:
##########
@@ -205,19 +207,40 @@ public String toString() {
return "LocalPrompt{" + "template=" + template + '}';
}
- /** Format template string with keyword arguments */
+ /** Matches a {placeholder}; the key may not itself contain braces. */
+ private static final Pattern PLACEHOLDER =
Pattern.compile("\\{([^{}]+)\\}");
+
+ /**
+ * Format a template string with keyword arguments.
+ *
+ * <p>The template is scanned once and each {@code {key}} placeholder
is replaced with its
+ * value (or left untouched if the key is absent). Substitution is
single-pass: text
+ * introduced by a substituted value is never itself re-interpreted as
a placeholder. This
+ * keeps the result independent of the {@code kwargs} iteration order
and prevents a
+ * caller-supplied value from expanding into another variable's value.
It mirrors the Python
+ * {@code SafeFormatter}.
+ */
private static String format(String template, Map<String, String>
kwargs) {
if (template == null) {
return "";
}
- String result = template;
- for (Map.Entry<String, String> entry : kwargs.entrySet()) {
- String placeholder = "{" + entry.getKey() + "}";
- String value = entry.getValue() != null ? entry.getValue() :
"";
- result = result.replace(placeholder, value);
+ Matcher matcher = PLACEHOLDER.matcher(template);
+ StringBuilder result = new StringBuilder();
+ while (matcher.find()) {
+ String key = matcher.group(1);
+ String replacement;
+ if (kwargs.containsKey(key)) {
+ String value = kwargs.get(key);
+ replacement = value != null ? value : "";
+ } else {
+ // Unknown placeholder: leave it as-is.
+ replacement = matcher.group(0);
+ }
+ matcher.appendReplacement(result,
Matcher.quoteReplacement(replacement));
Review 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:
```java
@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.
--
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]
