From a84a2c919a2fe587cfc0ded85144f270dbbba423 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Thu, 29 Jun 2023 07:36:04 -0400 Subject: [PATCH 1/3] Preserve identifier in EagerAstMethod like is done in EagerAstMacroFunction. Also migrate both to directly use the EagerAstParameters.getPartiallyResolved to reconstruct the parameters. There's no reason that it wasn't already done this way --- .../el/ext/eager/EagerAstMacroFunction.java | 34 ++++++++----------- .../jinjava/el/ext/eager/EagerAstMethod.java | 19 ++++------- .../el/ext/eager/EagerAstParameters.java | 2 +- .../el/ext/eager/EagerAstMethodTest.java | 10 ++++++ 4 files changed, 33 insertions(+), 32 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java index 8b5fc23dd..ba0d26dab 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java @@ -12,7 +12,6 @@ import java.lang.reflect.Array; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; -import java.util.StringJoiner; import javax.el.ELContext; import javax.el.ELException; @@ -154,26 +153,23 @@ public String getPartiallyResolved( ) { return deferredParsingException.getDeferredEvalResult(); } - StringBuilder sb = new StringBuilder(); - sb.append(getName()); - try { - StringJoiner paramString = new StringJoiner(", "); - for (int i = 0; i < ((AstParameters) params).getCardinality(); i++) { - paramString.add( - EvalResultHolder.reconstructNode( - bindings, - context, - (EvalResultHolder) ((AstParameters) params).getChild(i), - deferredParsingException, - preserveIdentifier - ) + String paramString; + if ( + deferredParsingException != null && + deferredParsingException.getSourceNode() == params + ) { + paramString = deferredParsingException.getDeferredEvalResult(); + } else { + paramString = + params.getPartiallyResolved( + bindings, + context, + deferredParsingException, + preserveIdentifier ); - } - sb.append(String.format("(%s)", paramString)); - } catch (DeferredParsingException dpe) { - sb.append(String.format("(%s)", dpe.getDeferredEvalResult())); } - return sb.toString(); + + return (getName() + String.format("(%s)", paramString)); } @Override diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java index 366195214..30b416a1c 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java @@ -2,10 +2,8 @@ import com.hubspot.jinjava.el.ext.DeferredParsingException; import com.hubspot.jinjava.interpret.DeferredValueException; -import com.hubspot.jinjava.util.EagerExpressionResolver; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstMethod; -import de.odysseus.el.tree.impl.ast.AstNode; import de.odysseus.el.tree.impl.ast.AstParameters; import de.odysseus.el.tree.impl.ast.AstProperty; import javax.el.ELContext; @@ -90,16 +88,13 @@ public String getPartiallyResolved( ) { paramString = deferredParsingException.getDeferredEvalResult(); } else { - try { - paramString = - EagerExpressionResolver.getValueAsJinjavaStringSafe( - ((AstNode) params).eval(bindings, context) - ); - // remove brackets so they can get replaced with parentheses - paramString = paramString.substring(1, paramString.length() - 1); - } catch (DeferredParsingException e) { - paramString = e.getDeferredEvalResult(); - } + paramString = + params.getPartiallyResolved( + bindings, + context, + deferredParsingException, + preserveIdentifier + ); } return (propertyResult + String.format("(%s)", paramString)); diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java index 920bafff0..6040eaf3b 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java @@ -70,7 +70,7 @@ public String getPartiallyResolved( context, node, deferredParsingException, - false + preserveIdentifier ) ) ); diff --git a/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java b/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java index 98c31584f..2a079a9cc 100644 --- a/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java +++ b/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java @@ -69,6 +69,16 @@ public void itPreservesIdentifier() { } } + @Test + public void itPreservesNonDeferredIdentifier() { + try { + interpreter.resolveELExpression("deferred.append(foo_map)", -1); + fail("Should throw DeferredParsingException"); + } catch (DeferredParsingException e) { + assertThat(e.getDeferredEvalResult()).isEqualTo("deferred.append(foo_map)"); + } + } + @Test public void itPreservesAstDot() { try { From 884d8c5bfe7315126ab7fa11fa45ee31f04a1103 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Thu, 29 Jun 2023 08:37:41 -0400 Subject: [PATCH 2/3] Support preserving non-deferred identifier when later params are deferred. To do this, we need to be aware of if the DeferredParsingException was created when forcing the identifiers to be preserved. So I introduced a new enum to be more readable than the boolean that represented that before --- .../el/ext/DeferredParsingException.java | 24 +++++++++ .../ext/IdentifierPreservationStrategy.java | 20 +++++++ .../jinjava/el/ext/eager/EagerAstBinary.java | 7 +-- .../jinjava/el/ext/eager/EagerAstBracket.java | 7 +-- .../jinjava/el/ext/eager/EagerAstChoice.java | 16 ++++-- .../jinjava/el/ext/eager/EagerAstDict.java | 9 ++-- .../jinjava/el/ext/eager/EagerAstDot.java | 11 +++- .../el/ext/eager/EagerAstIdentifier.java | 3 +- .../jinjava/el/ext/eager/EagerAstList.java | 5 +- .../el/ext/eager/EagerAstMacroFunction.java | 18 ++++--- .../jinjava/el/ext/eager/EagerAstMethod.java | 20 ++++--- .../el/ext/eager/EagerAstNamedParameter.java | 5 +- .../jinjava/el/ext/eager/EagerAstNested.java | 5 +- .../el/ext/eager/EagerAstNodeDecorator.java | 3 +- .../el/ext/eager/EagerAstParameters.java | 30 ++++++++--- .../el/ext/eager/EagerAstRangeBracket.java | 9 ++-- .../jinjava/el/ext/eager/EagerAstTuple.java | 5 +- .../jinjava/el/ext/eager/EagerAstUnary.java | 5 +- .../el/ext/eager/EvalResultHolder.java | 53 +++++++++++++++---- .../java/com/hubspot/jinjava/EagerTest.java | 5 +- .../el/ext/eager/EagerAstMethodTest.java | 11 ++++ .../lib/fn/eager/EagerMacroFunctionTest.java | 2 +- .../lib/tag/eager/EagerForTagTest.java | 2 +- .../util/EagerExpressionResolverTest.java | 2 +- ...fully-defers-filtered-macro.expected.jinja | 5 +- 25 files changed, 211 insertions(+), 71 deletions(-) create mode 100644 src/main/java/com/hubspot/jinjava/el/ext/IdentifierPreservationStrategy.java diff --git a/src/main/java/com/hubspot/jinjava/el/ext/DeferredParsingException.java b/src/main/java/com/hubspot/jinjava/el/ext/DeferredParsingException.java index c8842308e..a58df5984 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/DeferredParsingException.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/DeferredParsingException.java @@ -5,11 +5,13 @@ public class DeferredParsingException extends DeferredValueException { private final String deferredEvalResult; private final Object sourceNode; + private final IdentifierPreservationStrategy identifierPreservationStrategy; public DeferredParsingException(String message) { super(message); this.deferredEvalResult = message; this.sourceNode = null; + this.identifierPreservationStrategy = IdentifierPreservationStrategy.RESOLVING; } public DeferredParsingException(Object sourceNode, String deferredEvalResult) { @@ -22,6 +24,24 @@ public DeferredParsingException(Object sourceNode, String deferredEvalResult) { ); this.deferredEvalResult = deferredEvalResult; this.sourceNode = sourceNode; + this.identifierPreservationStrategy = IdentifierPreservationStrategy.RESOLVING; + } + + public DeferredParsingException( + Object sourceNode, + String deferredEvalResult, + IdentifierPreservationStrategy identifierPreservationStrategy + ) { + super( + String.format( + "%s could not be parsed more than: %s", + sourceNode.getClass(), + deferredEvalResult + ) + ); + this.deferredEvalResult = deferredEvalResult; + this.sourceNode = sourceNode; + this.identifierPreservationStrategy = identifierPreservationStrategy; } public String getDeferredEvalResult() { @@ -31,4 +51,8 @@ public String getDeferredEvalResult() { public Object getSourceNode() { return sourceNode; } + + public IdentifierPreservationStrategy getIdentifierPreservationStrategy() { + return identifierPreservationStrategy; + } } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/IdentifierPreservationStrategy.java b/src/main/java/com/hubspot/jinjava/el/ext/IdentifierPreservationStrategy.java new file mode 100644 index 000000000..d3e6536dd --- /dev/null +++ b/src/main/java/com/hubspot/jinjava/el/ext/IdentifierPreservationStrategy.java @@ -0,0 +1,20 @@ +package com.hubspot.jinjava.el.ext; + +public enum IdentifierPreservationStrategy { + PRESERVING(true), + RESOLVING(false); + + public static IdentifierPreservationStrategy preserving(boolean preserveIdentifier) { + return preserveIdentifier ? PRESERVING : RESOLVING; + } + + private final boolean preserving; + + IdentifierPreservationStrategy(boolean preserving) { + this.preserving = preserving; + } + + public boolean isPreserving() { + return preserving; + } +} diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBinary.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBinary.java index 95d92e917..63611056a 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBinary.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBinary.java @@ -2,6 +2,7 @@ import com.hubspot.jinjava.el.NoInvokeELContext; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import com.hubspot.jinjava.el.ext.OrOperator; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstBinary; @@ -48,7 +49,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { return ( EvalResultHolder.reconstructNode( @@ -56,7 +57,7 @@ public String getPartiallyResolved( context, left, deferredParsingException, - false + IdentifierPreservationStrategy.RESOLVING ) + String.format(" %s ", operator.toString()) + EvalResultHolder.reconstructNode( @@ -66,7 +67,7 @@ public String getPartiallyResolved( : context, right, deferredParsingException, - false + IdentifierPreservationStrategy.RESOLVING ) ); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBracket.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBracket.java index 85bd00346..fd72a726d 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBracket.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstBracket.java @@ -1,6 +1,7 @@ package com.hubspot.jinjava.el.ext.eager; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstBracket; import de.odysseus.el.tree.impl.ast.AstNode; @@ -63,7 +64,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { return String.format( "%s[%s]", @@ -72,14 +73,14 @@ public String getPartiallyResolved( context, (EvalResultHolder) prefix, deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ), EvalResultHolder.reconstructNode( bindings, context, (EvalResultHolder) property, deferredParsingException, - false + IdentifierPreservationStrategy.RESOLVING ) ); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstChoice.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstChoice.java index dd5d0472f..17c709bb3 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstChoice.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstChoice.java @@ -2,6 +2,7 @@ import com.hubspot.jinjava.el.NoInvokeELContext; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstChoice; import de.odysseus.el.tree.impl.ast.AstNode; @@ -46,7 +47,12 @@ public Object eval(Bindings bindings, ELContext context) throws ELException { } throw new DeferredParsingException( this, - getPartiallyResolved(bindings, context, e, false) + getPartiallyResolved( + bindings, + context, + e, + IdentifierPreservationStrategy.RESOLVING + ) ); } } @@ -72,7 +78,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { return ( EvalResultHolder.reconstructNode( @@ -80,7 +86,7 @@ public String getPartiallyResolved( context, question, deferredParsingException, - false + IdentifierPreservationStrategy.RESOLVING ) + " ? " + EvalResultHolder.reconstructNode( @@ -88,7 +94,7 @@ public String getPartiallyResolved( new NoInvokeELContext(context), yes, deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ) + " : " + EvalResultHolder.reconstructNode( @@ -96,7 +102,7 @@ public String getPartiallyResolved( new NoInvokeELContext(context), no, deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ) ); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDict.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDict.java index 48302a4a1..ea4ebd339 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDict.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDict.java @@ -3,6 +3,7 @@ import com.hubspot.jinjava.el.ext.AstDict; import com.hubspot.jinjava.el.ext.DeferredParsingException; import com.hubspot.jinjava.el.ext.ExtendedParser; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import com.hubspot.jinjava.interpret.JinjavaInterpreter; import com.hubspot.jinjava.util.EagerExpressionResolver; import de.odysseus.el.tree.Bindings; @@ -34,7 +35,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { JinjavaInterpreter interpreter = (JinjavaInterpreter) context .getELResolver() @@ -52,7 +53,9 @@ public String getPartiallyResolved( context, (EvalResultHolder) key, deferredParsingException, - !interpreter.getConfig().getLegacyOverrides().isEvaluateMapKeys() + IdentifierPreservationStrategy.preserving( + !interpreter.getConfig().getLegacyOverrides().isEvaluateMapKeys() + ) ) ); } else { @@ -69,7 +72,7 @@ public String getPartiallyResolved( context, (EvalResultHolder) value, deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ) ); } else { diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDot.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDot.java index 862d9620d..73e56ab3a 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDot.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstDot.java @@ -1,6 +1,7 @@ package com.hubspot.jinjava.el.ext.eager; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstDot; import de.odysseus.el.tree.impl.ast.AstNode; @@ -52,11 +53,17 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException e, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { return String.format( "%s.%s", - EvalResultHolder.reconstructNode(bindings, context, base, e, preserveIdentifier), + EvalResultHolder.reconstructNode( + bindings, + context, + base, + e, + identifierPreservationStrategy + ), property ); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstIdentifier.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstIdentifier.java index a7b631e4a..ee72b3f7e 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstIdentifier.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstIdentifier.java @@ -1,6 +1,7 @@ package com.hubspot.jinjava.el.ext.eager; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstIdentifier; import javax.el.ELContext; @@ -43,7 +44,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { return getName(); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstList.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstList.java index 6ca90e4a1..d0b80658a 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstList.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstList.java @@ -2,6 +2,7 @@ import com.hubspot.jinjava.el.ext.AstList; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstParameters; import java.util.StringJoiner; @@ -45,7 +46,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { StringJoiner joiner = new StringJoiner(", "); for (int i = 0; i < elements.getCardinality(); i++) { @@ -55,7 +56,7 @@ public String getPartiallyResolved( context, (EvalResultHolder) elements.getChild(i), deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ) ); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java index ba0d26dab..80f809dda 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMacroFunction.java @@ -3,6 +3,7 @@ import com.hubspot.jinjava.el.ext.AstMacroFunction; import com.hubspot.jinjava.el.ext.DeferredParsingException; import com.hubspot.jinjava.el.ext.ExtendedParser; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import com.hubspot.jinjava.interpret.Context.TemporaryValueClosable; import com.hubspot.jinjava.interpret.DeferredValueException; import com.hubspot.jinjava.interpret.JinjavaInterpreter; @@ -53,7 +54,13 @@ public Object eval(Bindings bindings, ELContext context) { ); throw new DeferredParsingException( this, - getPartiallyResolved(bindings, context, e, true) // Need this to always be true because the macro function may modify the identifier + getPartiallyResolved( + bindings, + context, + e, + IdentifierPreservationStrategy.PRESERVING + ), // Need this to always be true because the macro function may modify the identifier + IdentifierPreservationStrategy.PRESERVING ); } } @@ -145,7 +152,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { if ( deferredParsingException != null && @@ -154,10 +161,7 @@ public String getPartiallyResolved( return deferredParsingException.getDeferredEvalResult(); } String paramString; - if ( - deferredParsingException != null && - deferredParsingException.getSourceNode() == params - ) { + if (EvalResultHolder.exceptionMatchesNode(deferredParsingException, params)) { paramString = deferredParsingException.getDeferredEvalResult(); } else { paramString = @@ -165,7 +169,7 @@ public String getPartiallyResolved( bindings, context, deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java index 30b416a1c..413536cd7 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethod.java @@ -1,6 +1,7 @@ package com.hubspot.jinjava.el.ext.eager; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import com.hubspot.jinjava.interpret.DeferredValueException; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstMethod; @@ -41,7 +42,13 @@ public Object eval(Bindings bindings, ELContext context) { ); throw new DeferredParsingException( this, - getPartiallyResolved(bindings, context, e, true) // Need this to always be true because the method may modify the identifier + getPartiallyResolved( + bindings, + context, + e, + IdentifierPreservationStrategy.PRESERVING + ), // Need this to always be true because the method may modify the identifier + IdentifierPreservationStrategy.PRESERVING ); } } @@ -71,7 +78,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { String propertyResult; propertyResult = @@ -79,13 +86,10 @@ public String getPartiallyResolved( bindings, context, deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ); String paramString; - if ( - deferredParsingException != null && - deferredParsingException.getSourceNode() == params - ) { + if (EvalResultHolder.exceptionMatchesNode(deferredParsingException, params)) { paramString = deferredParsingException.getDeferredEvalResult(); } else { paramString = @@ -93,7 +97,7 @@ public String getPartiallyResolved( bindings, context, deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNamedParameter.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNamedParameter.java index 8d1a23c61..5f70202cd 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNamedParameter.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNamedParameter.java @@ -2,6 +2,7 @@ import com.hubspot.jinjava.el.ext.AstNamedParameter; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstIdentifier; import de.odysseus.el.tree.impl.ast.AstNode; @@ -39,7 +40,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { return String.format( "%s=%s", @@ -49,7 +50,7 @@ public String getPartiallyResolved( context, value, deferredParsingException, - false + IdentifierPreservationStrategy.RESOLVING ) ); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNested.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNested.java index 317167c11..749006117 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNested.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNested.java @@ -1,6 +1,7 @@ package com.hubspot.jinjava.el.ext.eager; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.Node; import de.odysseus.el.tree.impl.ast.AstNode; @@ -71,7 +72,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { return String.format( "(%s)", @@ -80,7 +81,7 @@ public String getPartiallyResolved( context, (EvalResultHolder) child, deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ) ); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNodeDecorator.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNodeDecorator.java index 2b34b3071..7ea532357 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNodeDecorator.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstNodeDecorator.java @@ -1,6 +1,7 @@ package com.hubspot.jinjava.el.ext.eager; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.Node; import de.odysseus.el.tree.impl.ast.AstNode; @@ -64,7 +65,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { return astNode.toString(); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java index 6040eaf3b..f6deaf6cd 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstParameters.java @@ -2,7 +2,9 @@ import com.hubspot.jinjava.el.ext.DeferredParsingException; import com.hubspot.jinjava.el.ext.ExtendedParser; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import com.hubspot.jinjava.interpret.Context.TemporaryValueClosable; +import com.hubspot.jinjava.interpret.DeferredValueException; import com.hubspot.jinjava.interpret.JinjavaInterpreter; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstNode; @@ -11,6 +13,7 @@ import java.util.StringJoiner; import java.util.stream.Collectors; import javax.el.ELContext; +import javax.el.ELException; public class EagerAstParameters extends AstParameters implements EvalResultHolder { protected Object evalResult; @@ -43,11 +46,24 @@ public Object[] eval(Bindings bindings, ELContext context) { ).getContext() .withPartialMacroEvaluation(false) ) { - return (Object[]) EvalResultHolder.super.eval( - () -> super.eval(bindings, context), - bindings, - context - ); + try { + setEvalResult(super.eval(bindings, context)); + return (Object[]) checkEvalResultSize(context); + } catch (DeferredValueException | ELException originalException) { + DeferredParsingException e = EvalResultHolder.convertToDeferredParsingException( + originalException + ); + throw new DeferredParsingException( + this, + getPartiallyResolved( + bindings, + context, + e, + IdentifierPreservationStrategy.PRESERVING + ), // Need this to always be true because a function may modify the identifier + IdentifierPreservationStrategy.PRESERVING + ); + } } } @@ -56,7 +72,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { StringJoiner joiner = new StringJoiner(", "); nodes @@ -70,7 +86,7 @@ public String getPartiallyResolved( context, node, deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ) ) ); diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRangeBracket.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRangeBracket.java index 0f4f8d4f6..c2fe057e3 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRangeBracket.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstRangeBracket.java @@ -2,6 +2,7 @@ import com.hubspot.jinjava.el.ext.AstRangeBracket; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstNode; import javax.el.ELContext; @@ -42,7 +43,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { return ( EvalResultHolder.reconstructNode( @@ -50,7 +51,7 @@ public String getPartiallyResolved( context, (EvalResultHolder) prefix, deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ) + "[" + EvalResultHolder.reconstructNode( @@ -58,7 +59,7 @@ public String getPartiallyResolved( context, (EvalResultHolder) property, deferredParsingException, - false + IdentifierPreservationStrategy.RESOLVING ) + ":" + EvalResultHolder.reconstructNode( @@ -66,7 +67,7 @@ public String getPartiallyResolved( context, (EvalResultHolder) rangeMax, deferredParsingException, - false + IdentifierPreservationStrategy.RESOLVING ) + "]" ); diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstTuple.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstTuple.java index e0c4b86b4..ab731cbee 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstTuple.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstTuple.java @@ -2,6 +2,7 @@ import com.hubspot.jinjava.el.ext.AstTuple; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstParameters; import java.util.StringJoiner; @@ -29,7 +30,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { StringJoiner joiner = new StringJoiner(", "); for (int i = 0; i < elements.getCardinality(); i++) { @@ -39,7 +40,7 @@ public String getPartiallyResolved( context, (EvalResultHolder) elements.getChild(i), deferredParsingException, - preserveIdentifier + identifierPreservationStrategy ) ); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstUnary.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstUnary.java index ffdbdfc36..344f92701 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstUnary.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstUnary.java @@ -1,6 +1,7 @@ package com.hubspot.jinjava.el.ext.eager; import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import de.odysseus.el.tree.Bindings; import de.odysseus.el.tree.impl.ast.AstNode; import de.odysseus.el.tree.impl.ast.AstUnary; @@ -36,7 +37,7 @@ public String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ) { return ( operator.toString() + @@ -45,7 +46,7 @@ public String getPartiallyResolved( context, child, deferredParsingException, - false + IdentifierPreservationStrategy.RESOLVING ) ); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EvalResultHolder.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EvalResultHolder.java index 30de67e77..921289f7a 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EvalResultHolder.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EvalResultHolder.java @@ -2,6 +2,7 @@ import com.hubspot.jinjava.el.ext.DeferredParsingException; import com.hubspot.jinjava.el.ext.ExtendedParser; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; import com.hubspot.jinjava.interpret.DeferredValueException; import com.hubspot.jinjava.interpret.JinjavaInterpreter; import com.hubspot.jinjava.util.EagerExpressionResolver; @@ -34,7 +35,12 @@ default Object eval( ); throw new DeferredParsingException( this, - getPartiallyResolved(bindings, context, e, false) + getPartiallyResolved( + bindings, + context, + e, + IdentifierPreservationStrategy.RESOLVING + ) ); } } @@ -60,7 +66,7 @@ String getPartiallyResolved( Bindings bindings, ELContext context, DeferredParsingException deferredParsingException, - boolean preserveIdentifier + IdentifierPreservationStrategy identifierPreservationStrategy ); static String reconstructNode( @@ -68,19 +74,23 @@ static String reconstructNode( ELContext context, EvalResultHolder astNode, DeferredParsingException exception, - boolean preserveIdentifier + IdentifierPreservationStrategy preserveIdentifier ) { if (astNode == null) { return ""; } - preserveIdentifier |= - astNode instanceof AstIdentifier && - ExtendedParser.INTERPRETER.equals(((AstIdentifier) astNode).getName()); + preserveIdentifier = + IdentifierPreservationStrategy.preserving( + preserveIdentifier.isPreserving() || + astNode instanceof AstIdentifier && + ExtendedParser.INTERPRETER.equals(((AstIdentifier) astNode).getName()) + ); if ( - preserveIdentifier && + preserveIdentifier.isPreserving() && !astNode.hasEvalResult() && - !(exception != null && exception.getSourceNode() == astNode) + !(exceptionMatchesNode(exception, astNode)) ) { + // Evaluate to determine if the result is primitive. If so, we don't need to preserve the identifier try { EagerExpressionResolver.getValueAsJinjavaStringSafe( ((AstNode) astNode).eval(bindings, context) @@ -89,10 +99,16 @@ static String reconstructNode( } Object evalResult = astNode.getEvalResult(); if ( - !preserveIdentifier || + exceptionMatchesNode(exception, astNode) && + exception.getIdentifierPreservationStrategy().isPreserving() + ) { + return exception.getDeferredEvalResult(); + } + if ( + !preserveIdentifier.isPreserving() || (astNode.hasEvalResult() && EagerExpressionResolver.isPrimitive(evalResult)) ) { - if (exception != null && exception.getSourceNode() == astNode) { + if (exceptionMatchesNode(exception, astNode)) { return exception.getDeferredEvalResult(); } if (!astNode.hasEvalResult()) { @@ -106,7 +122,12 @@ static String reconstructNode( return EagerExpressionResolver.getValueAsJinjavaStringSafe(evalResult); } catch (DeferredValueException ignored) {} } - return astNode.getPartiallyResolved(bindings, context, exception, true); + return astNode.getPartiallyResolved( + bindings, + context, + exception, + IdentifierPreservationStrategy.PRESERVING + ); } static DeferredParsingException convertToDeferredParsingException( @@ -127,4 +148,14 @@ static DeferredParsingException convertToDeferredParsingException( } return null; } + + static boolean exceptionMatchesNode( + DeferredParsingException deferredParsingException, + Object astNode + ) { + return ( + deferredParsingException != null && + deferredParsingException.getSourceNode() == astNode + ); + } } diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java index 65b12639d..a39842ec5 100644 --- a/src/test/java/com/hubspot/jinjava/EagerTest.java +++ b/src/test/java/com/hubspot/jinjava/EagerTest.java @@ -1053,7 +1053,10 @@ public void itReconstructsWithMultipleLoops() { @Test public void itFullyDefersFilteredMacro() { - expectedTemplateInterpreter.assertExpectedOutput("fully-defers-filtered-macro"); + // Macro and set tag reconstruction are flipped so not exactly idempotent, but functionally identical + expectedTemplateInterpreter.assertExpectedOutputNonIdempotent( + "fully-defers-filtered-macro" + ); } @Test diff --git a/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java b/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java index 2a079a9cc..7518c119c 100644 --- a/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java +++ b/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java @@ -79,6 +79,17 @@ public void itPreservesNonDeferredIdentifier() { } } + @Test + public void itPreservesNonDeferredIdentifierWhenSecondParamIsDeferred() { + try { + interpreter.resolveELExpression("foo_list.append(foo_map, deferred)", -1); + fail("Should throw DeferredParsingException"); + } catch (DeferredParsingException e) { + assertThat(e.getDeferredEvalResult()) + .isEqualTo("foo_list.append(foo_map, deferred)"); + } + } + @Test public void itPreservesAstDot() { try { diff --git a/src/test/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunctionTest.java b/src/test/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunctionTest.java index 95d4cf0ee..b14b56a33 100644 --- a/src/test/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunctionTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunctionTest.java @@ -81,7 +81,7 @@ public void itReconstructsForAliasedName() { public void itResolvesFromSet() { String template = "{% macro foo(foobar, other) %}" + - " {% do foobar.update({'a': 'b'} ) %} " + + " {% do foobar.update({'a': 'b'}) %} " + " {{ foobar }} and {{ other }}" + "{% endmacro %}" + "{% set bar = {} %}" + diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerForTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerForTagTest.java index bbfb3c523..f42a0b73f 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerForTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerForTagTest.java @@ -173,7 +173,7 @@ public void itDoesntAllowChangesInDeferredForWithSameHashCode() { "{% set foo = {'a': 'a'} %}{% for i in range(0, deferred) %}\n" + "bar{{ foo }}\n" + "{% do foo.clear() %}\n" + - "{% do foo.update({'b': 'b'} ) %}\n" + + "{% do foo.update({'b': 'b'}) %}\n" + "{% endfor %}\n" + "{{ foo }}" ); diff --git a/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java b/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java index 6f8dac047..2a2b22605 100644 --- a/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java +++ b/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java @@ -782,7 +782,7 @@ public void itHandlesToday() { @Test public void itHandlesRandom() { assertThat(eagerResolveExpression("range(1)|random").toString()) - .isEqualTo("filter:random.filter([0], ____int3rpr3t3r____)"); + .isEqualTo("filter:random.filter(range(1), ____int3rpr3t3r____)"); } @Test diff --git a/src/test/resources/eager/fully-defers-filtered-macro.expected.jinja b/src/test/resources/eager/fully-defers-filtered-macro.expected.jinja index 3fd6290f0..6eff41e08 100644 --- a/src/test/resources/eager/fully-defers-filtered-macro.expected.jinja +++ b/src/test/resources/eager/fully-defers-filtered-macro.expected.jinja @@ -1,5 +1,6 @@ {% macro flashy(foo) %}{{ filter:upper.filter(foo, ____int3rpr3t3r____) }} - A flashy {{ deferred }}.{% endmacro %}{{ flashy(flashy('bar')) }} + A flashy {{ deferred }}.{% endmacro %}{% set __macro_flashy_1625622909_temp_variable_0__ %}BAR + A flashy {{ deferred }}.{% endset %}{{ flashy(__macro_flashy_1625622909_temp_variable_0__) }} --- -{% set __macro_silly_2092874071_temp_variable_0__ %}A silly {{ deferred }}.{% endset %}{{ filter:upper.filter(__macro_silly_2092874071_temp_variable_0__, ____int3rpr3t3r____) }} +{% set __macro_silly_2092874071_temp_variable_0__ %}A silly {{ deferred }}.{% endset %}{{ filter:upper.filter(__macro_silly_2092874071_temp_variable_0__, ____int3rpr3t3r____) }} \ No newline at end of file From 8c20c269c0213d006145ce51cc228b0498543236 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Thu, 29 Jun 2023 08:54:28 -0400 Subject: [PATCH 3/3] Update tests to more clearly demonstrate intent --- .../hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java b/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java index 7518c119c..2f65c6e49 100644 --- a/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java +++ b/src/test/java/com/hubspot/jinjava/el/ext/eager/EagerAstMethodTest.java @@ -72,21 +72,21 @@ public void itPreservesIdentifier() { @Test public void itPreservesNonDeferredIdentifier() { try { - interpreter.resolveELExpression("deferred.append(foo_map)", -1); + interpreter.resolveELExpression("deferred.modify(foo_map)", -1); fail("Should throw DeferredParsingException"); } catch (DeferredParsingException e) { - assertThat(e.getDeferredEvalResult()).isEqualTo("deferred.append(foo_map)"); + assertThat(e.getDeferredEvalResult()).isEqualTo("deferred.modify(foo_map)"); } } @Test public void itPreservesNonDeferredIdentifierWhenSecondParamIsDeferred() { try { - interpreter.resolveELExpression("foo_list.append(foo_map, deferred)", -1); + interpreter.resolveELExpression("foo_list.modify(foo_map, deferred)", -1); fail("Should throw DeferredParsingException"); } catch (DeferredParsingException e) { assertThat(e.getDeferredEvalResult()) - .isEqualTo("foo_list.append(foo_map, deferred)"); + .isEqualTo("foo_list.modify(foo_map, deferred)"); } }