Copilot commented on code in PR #397:
URL: https://github.com/apache/commons-jexl/pull/397#discussion_r2763694373
##########
src/test/java/org/apache/commons/jexl3/Issues400Test.java:
##########
@@ -798,6 +799,38 @@ void testIssue441() {
Assertions.assertEquals(ctl, o);
}
+ @Test
+ void testIssue455a() {
+ final JexlEngine jexl = new JexlBuilder().create();
+ String code = "name -> `${name +\n\t\b\f\r name}`";
+ JexlScript script = jexl.createScript(code);
+ Object o = script.execute(null, "Hello");
+ String ctl = "HelloHello";
+ Assertions.assertEquals(ctl, o);
+ }
Review Comment:
This test validates that backspace (`\b`) is ignored within template
expressions. However, backspace is not a whitespace character according to
Java's Character.isWhitespace() standard. This test should be updated to only
test actual whitespace characters (newline, carriage return, tab, and form
feed) to match the corrected implementation. If backspace handling is still
desired, it should be explicitly documented why backspace is treated as
ignorable in this context.
##########
src/main/java/org/apache/commons/jexl3/internal/TemplateEngine.java:
##########
@@ -1184,6 +1186,10 @@ private String escapeString(final CharSequence str) {
return StringParser.escapeString(str, (char) 0);
}
+ private static boolean isIgnorable(char c) {
+ return c == '\n' || c == '\r' || c == '\t' || c == '\f' || c == '\b';
Review Comment:
The backspace character (`\b`) should not be included in the ignorable
characters list. Backspace (U+0008) is not considered whitespace by Java's
Character.isWhitespace() method - it's a control character, not whitespace. The
standard whitespace characters to ignore should be: newline (`\n`), carriage
return (`\r`), tab (`\t`), and form feed (`\f`). Including backspace could lead
to unexpected behavior where valid backspace characters in expressions are
silently removed.
```suggestion
return c == '\n' || c == '\r' || c == '\t' || c == '\f';
```
##########
src/main/java/org/apache/commons/jexl3/internal/TemplateEngine.java:
##########
@@ -1086,54 +1086,56 @@ TemplateExpression parseExpression(final JexlInfo info,
final String expr, final
// nested immediate in deferred; need to balance count of '{'
& '}'
// closing '}'
switch (c) {
- case '"':
- case '\'':
- strb.append(c);
- column = StringParser.readString(strb, expr, column + 1,
c);
- continue;
- case '{':
- if (expr.charAt(column - 1) == immediateChar) {
- inner1 += 1;
- strb.deleteCharAt(strb.length() - 1);
- nested = true;
- } else {
- deferred1 += 1;
- strb.append(c);
- }
- continue;
- case '}':
- // balance nested immediate
- if (deferred1 > 0) {
- deferred1 -= 1;
+ case '"':
+ case '\'':
strb.append(c);
- } else if (inner1 > 0) {
- inner1 -= 1;
- } else {
- // materialize the nested/deferred expr
- final String src = escapeString(strb);
- final JexlInfo srcInfo = info.at(lineno, column);
- TemplateExpression dexpr;
- if (nested) {
- dexpr = new NestedExpression(
- escapeString(expr.substring(inested,
column + 1)),
+ column = StringParser.readString(strb, expr, column +
1, c);
+ continue;
+ case '{':
+ if (expr.charAt(column - 1) == immediateChar) {
+ inner1 += 1;
+ strb.deleteCharAt(strb.length() - 1);
+ nested = true;
+ } else {
+ deferred1 += 1;
+ strb.append(c);
+ }
+ continue;
Review Comment:
The nested immediate expression detection logic has a bug when ignorable
characters are present between the immediate character and the opening brace.
The check `expr.charAt(column - 1) == immediateChar` looks at the previous
character in the original expression string, but if there are ignorable
whitespace characters between the immediate character (typically '$') and the
'{', this check will fail because column-1 will point to the whitespace
character, not the immediate character. This would cause expressions like
`#{...$\n{...}...}` to not be correctly recognized as nested immediate
expressions. The logic should check the last non-ignorable character instead,
or track whether the previous non-ignorable character was the immediate
character.
##########
src/test/java/org/apache/commons/jexl3/Issues400Test.java:
##########
@@ -798,6 +799,38 @@ void testIssue441() {
Assertions.assertEquals(ctl, o);
}
+ @Test
+ void testIssue455a() {
+ final JexlEngine jexl = new JexlBuilder().create();
+ String code = "name -> `${name +\n\t\b\f\r name}`";
+ JexlScript script = jexl.createScript(code);
+ Object o = script.execute(null, "Hello");
+ String ctl = "HelloHello";
+ Assertions.assertEquals(ctl, o);
+ }
+
+ @Test
+ void testIssue455b() {
+ final JexlEngine jexl = new JexlBuilder().create();
+ String code = "name -> `${name}\n${name}`;";
+ JexlScript script = jexl.createScript(code);
+ Object o = script.execute(null, "Hello");
+ String ctl = "Hello\nHello";
+ Assertions.assertEquals(ctl, o);
+ }
+
+ @Test
+ void testIssue455() {
+ final JexlEngine jexl = new JexlBuilder().create();
+ final JexlContext context = new MapContext();
+ context.set("name", "Hello");
+ final JxltEngine jxlt = jexl.createJxltEngine();
+ final JxltEngine.Template template =
jxlt.createTemplate("<b>\n\t${name\n\t+ name}\n</b>");
+ final StringWriter writer = new StringWriter();
+ template.evaluate(context, writer);
+ assertEquals("<b>\n\tHelloHello\n</b>", writer.toString());
+ }
Review Comment:
The test suite lacks coverage for nested expressions with ignorable
whitespace characters between the immediate character and the opening brace
(e.g., `#{ $\n{...} }`). This test case should be added to ensure that nested
immediate expressions are correctly detected even when ignorable whitespace is
present, or to document that such syntax is not supported.
--
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]
