Skip to content

Commit

Permalink
JEXL-403 : cache template expression in literal in a dedicated class …
Browse files Browse the repository at this point in the history 
…member;

 - clean up interpreter;
 - update test, release notes, changes;
  • Loading branch information
Henri Biestro committed Aug 29, 2023
1 parent cd3db93 commit c64ee0d
Show file tree
Hide file tree
Showing 6 changed files with 68 additions and 45 deletions.
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ New Features in 3.3.1:

Bugs Fixed in 3.3.1:
===================
* JEXL-403: Exception while evaluating template literal used in array assignment in loop.
* JEXL-402: parse failed with empty return value.

========================================================================================================================
Expand Down
5 changes: 4 additions & 1 deletion src/changes/changes.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@
Allow 'trailing commas' or ellipsis while defining array, map and set literals
</action>
<!-- FIX -->
<action dev="henrib" type="fix" due-to="Xu Pengcheng">
<action dev="henrib" type="fix" issue="JEXL-403" due-to="Xu Pengcheng" >
Exception while evaluating template literal used in array assignment in loop.
</action>
<action dev="henrib" type="fix" issue="JEXL-402" due-to="Xu Pengcheng">
Parse failed with empty return value.
</action>
<action dev="ggregory" type="fix" due-to="step-security-bot, Gary Gregory">
Expand Down
45 changes: 24 additions & 21 deletions src/main/java/org/apache/commons/jexl3/internal/Interpreter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ private Object forIterator(final ASTForeachStatement node, final Object data) {
final ASTReference loopReference = (ASTReference) node.jjtGetChild(0);
final ASTIdentifier loopVariable = (ASTIdentifier) loopReference.jjtGetChild(0);
final int symbol = loopVariable.getSymbol();
final boolean lexical = loopVariable.isLexical() || options.isLexical() ;// && node.getSymbolCount() > 0;
final boolean lexical = loopVariable.isLexical() || options.isLexical();
final LexicalFrame locals = lexical? new LexicalFrame(frame, block) : null;
final boolean loopSymbol = symbol >= 0 && loopVariable instanceof ASTVar;
if (lexical) {
Expand Down Expand Up @@ -1065,9 +1065,9 @@ protected Object visit(final ASTJexlScript script, final Object data) {
// if the function is named, assign in the local frame
final JexlNode child0 = script.jjtGetChild(0);
if (child0 instanceof ASTVar) {
final ASTVar var = (ASTVar) child0;
this.visit(var, data);
final int symbol = var.getSymbol();
final ASTVar variable = (ASTVar) child0;
this.visit(variable, data);
final int symbol = variable.getSymbol();
frame.set(symbol, closure);
// make the closure accessible to itself, ie capture the currently set variable after frame creation
closure.setCaptured(symbol, closure);
Expand Down Expand Up @@ -1390,24 +1390,24 @@ protected Object executeAssign(final JexlNode node, final JexlOperator assignop,
cancelCheck(node);
// left contains the reference to assign to
final JexlNode left = node.jjtGetChild(0);
final ASTIdentifier var;
final ASTIdentifier variable;
Object object = null;
final int symbol;
// check var decl with assign is ok
if (left instanceof ASTIdentifier) {
var = (ASTIdentifier) left;
symbol = var.getSymbol();
if (symbol >= 0 && (var.isLexical() || options.isLexical())) {
if (var instanceof ASTVar) {
if (!defineVariable((ASTVar) var, block)) {
return redefinedVariable(var, var.getName());
variable = (ASTIdentifier) left;
symbol = variable.getSymbol();
if (symbol >= 0 && (variable.isLexical() || options.isLexical())) {
if (variable instanceof ASTVar) {
if (!defineVariable((ASTVar) variable, block)) {
return redefinedVariable(variable, variable.getName());
}
} else if (var.isShaded() && (var.isLexical() || options.isLexicalShade())) {
return undefinedVariable(var, var.getName());
} else if (variable.isShaded() && (variable.isLexical() || options.isLexicalShade())) {
return undefinedVariable(variable, variable.getName());
}
}
} else {
var = null;
variable = null;
symbol = -1;
}
boolean antish = options.isAntish();
Expand All @@ -1418,7 +1418,7 @@ protected Object executeAssign(final JexlNode node, final JexlOperator assignop,
// actual value to return, right in most cases
Object actual = right;
// a (var?) v = ... expression
if (var != null) {
if (variable != null) {
if (symbol >= 0) {
// check we are not assigning a symbol itself
if (last < 0) {
Expand All @@ -1430,18 +1430,18 @@ protected Object executeAssign(final JexlNode node, final JexlOperator assignop,
frame.set(symbol, right);
} else {
// go through potential overload
final Object self = getVariable(frame, block, var);
final Object self = getVariable(frame, block, variable);
final Consumer<Object> f = r -> frame.set(symbol, r);
actual = operators.tryAssignOverload(node, assignop, f, self, right);
}
return actual; // 1
}
object = getVariable(frame, block, var);
object = getVariable(frame, block, variable);
// top level is a symbol, can not be an antish var
antish = false;
} else {
// check we are not assigning direct global
final String name = var.getName();
final String name = variable.getName();
if (last < 0) {
if (assignop == null) {
setContextVariable(node, name, right);
Expand Down Expand Up @@ -1894,15 +1894,18 @@ protected Object visit(final ASTConstructorNode node, final Object data) {

@Override
protected Object visit(final ASTJxltLiteral node, final Object data) {
TemplateEngine.TemplateExpression tp = (TemplateEngine.TemplateExpression) node.jjtGetValue();
if (tp == null) {
Object cache = node.getExpression();
TemplateEngine.TemplateExpression tp;
if (cache instanceof TemplateEngine.TemplateExpression) {
tp = (TemplateEngine.TemplateExpression) cache;
} else {
final TemplateEngine jxlt = jexl.jxlt();
JexlInfo info = node.jexlInfo();
if (this.block != null) {
info = new JexlNode.Info(node, info);
}
tp = jxlt.parseExpression(info, node.getLiteral(), frame != null ? frame.getScope() : null);
node.jjtSetValue(tp);
node.setExpression(tp);
}
if (tp != null) {
return tp.evaluate(context, frame, options);
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/org/apache/commons/jexl3/parser/ASTIdentifierAccessJxlt.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
* x.`expr`.
*/
public class ASTIdentifierAccessJxlt extends ASTIdentifierAccess {
protected JxltEngine.Expression jxltExpr;
protected transient JxltEngine.Expression jxltExpression;

ASTIdentifierAccessJxlt(final int id) {
super(id);
Expand All @@ -39,11 +39,11 @@ public boolean isExpression() {
}

public void setExpression(final JxltEngine.Expression tp) {
jxltExpr = tp;
jxltExpression = tp;
}

public JxltEngine.Expression getExpression() {
return jxltExpr;
return jxltExpression;
}

}
18 changes: 14 additions & 4 deletions src/main/java/org/apache/commons/jexl3/parser/ASTJxltLiteral.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@
*/
package org.apache.commons.jexl3.parser;

import org.apache.commons.jexl3.JxltEngine;

public final class ASTJxltLiteral extends JexlNode {
/**
*
*/
/** serial uid.*/
private static final long serialVersionUID = 1L;
/** The actual literal value; the inherited 'value' member may host a cached template expression. */
/** The actual literal value. */
private String literal = null;
/** The expression (parsed). */
private transient JxltEngine.Expression jxltExpression = null;

ASTJxltLiteral(final int id) {
super(id);
Expand All @@ -36,6 +38,14 @@ void setLiteral(final String literal) {
this.literal = literal;
}

public void setExpression(JxltEngine.Expression e) {
this.jxltExpression = e;
}

public JxltEngine.Expression getExpression() {
return jxltExpression;
}

/**
* Gets the literal value.
* @return the string literal
Expand Down
38 changes: 22 additions & 16 deletions src/test/java/org/apache/commons/jexl3/Issues400Test.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,22 +64,28 @@ public void test402() {

@Test
public void test403() {
String src = "var a = {'a': 1};\n" +
"var list = [a, a];\n" +
"let map1 = {:};\n" +
"for (var item : list) {\n" +
" map1[`${item.a}`] = 1;\n" +
"}\n " +
"map1";
final JexlEngine jexl = new JexlBuilder().create();
JexlScript script = jexl.createScript(src);
for(int i = 0; i < 2; ++ i) {
Object result = script.execute(null);
Assert.assertTrue(result instanceof Map);
Map<?, ?> map = (Map<?, ?>) result;
Assert.assertEquals(1, map.size());
Assert.assertTrue(map.containsKey(1));
Assert.assertTrue(map.containsValue(1));
for(String setmap : new String[]{
" map1.`${item.a}` = 1;\n",
" map1[`${item.a}`] = 1;\n",
" map1[item.a] = 1;\n"
}) {
String src = "var a = {'a': 1};\n" +
"var list = [a, a];\n" +
"let map1 = {:};\n" +
"for (var item : list) {\n" +
setmap +
"}\n " +
"map1";
final JexlEngine jexl = new JexlBuilder().cache(64).create();
JexlScript script = jexl.createScript(src);
for (int i = 0; i < 2; ++i) {
Object result = script.execute(null);
Assert.assertTrue(result instanceof Map);
Map<?, ?> map = (Map<?, ?>) result;
Assert.assertEquals(1, map.size());
Assert.assertTrue(map.containsKey(1));
Assert.assertTrue(map.containsValue(1));
}
}
}
}

0 comments on commit c64ee0d

Please sign in to comment.