From 6be60f4ea7f2b0c96b83b837e7e489dcaad86468 Mon Sep 17 00:00:00 2001 From: Libo Song Date: Mon, 12 Jan 2026 10:31:23 -0500 Subject: [PATCH 1/8] Chained filters optimization. --- .../jinjava/el/ext/AstFilterChain.java | 212 +++++++++++++++++ .../jinjava/el/ext/ExtendedParser.java | 25 +- .../hubspot/jinjava/el/ext/FilterSpec.java | 48 ++++ .../el/ext/eager/EagerAstFilterChain.java | 225 ++++++++++++++++++ .../el/ext/eager/EagerExtendedParser.java | 10 + .../java/com/hubspot/jinjava/EagerTest.java | 11 +- .../el/ext/eager/EagerAstMethodTest.java | 3 +- .../lib/tag/eager/EagerImportTagTest.java | 8 +- .../lib/tag/eager/EagerSetTagTest.java | 17 +- .../util/EagerExpressionResolverTest.java | 6 +- .../test.expected.jinja | 4 +- .../defers-macro-in-for/test.expected.jinja | 2 +- .../defers-macro-in-if/test.expected.jinja | 2 +- .../test.expected.jinja | 14 +- .../test.expected.jinja | 4 +- .../test.expected.jinja | 6 +- .../test.expected.jinja | 10 +- .../test.expected.jinja | 4 +- .../test.expected.jinja | 2 +- .../test.expected.jinja | 2 +- .../test.expected.jinja | 2 +- .../test.expected.jinja | 4 +- 22 files changed, 553 insertions(+), 68 deletions(-) create mode 100644 src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java create mode 100644 src/main/java/com/hubspot/jinjava/el/ext/FilterSpec.java create mode 100644 src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstFilterChain.java diff --git a/src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java b/src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java new file mode 100644 index 000000000..618b87905 --- /dev/null +++ b/src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java @@ -0,0 +1,212 @@ +package com.hubspot.jinjava.el.ext; + +import com.hubspot.jinjava.interpret.DisabledException; +import com.hubspot.jinjava.interpret.JinjavaInterpreter; +import com.hubspot.jinjava.interpret.TemplateError; +import com.hubspot.jinjava.interpret.TemplateError.ErrorItem; +import com.hubspot.jinjava.interpret.TemplateError.ErrorReason; +import com.hubspot.jinjava.interpret.TemplateError.ErrorType; +import com.hubspot.jinjava.lib.filter.Filter; +import com.hubspot.jinjava.objects.SafeString; +import de.odysseus.el.tree.Bindings; +import de.odysseus.el.tree.impl.ast.AstNode; +import de.odysseus.el.tree.impl.ast.AstParameters; +import de.odysseus.el.tree.impl.ast.AstRightValue; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import javax.el.ELContext; +import javax.el.ELException; + +/** + * AST node for a chain of filters applied to an input expression. + * Instead of creating nested AstMethod calls for each filter in a chain like: + * filter:length.filter(filter:lower.filter(filter:trim.filter(input))) + * + * This node represents the entire chain as a single evaluation unit: + * input|trim|lower|length + * + * This optimization reduces: + * - Filter lookups (done once per filter instead of per AST node traversal) + * - Method invocation overhead + * - Object wrapping/unwrapping between filters + * - Context operations + */ +public class AstFilterChain extends AstRightValue { + + protected final AstNode input; + protected final List filterSpecs; + + public AstFilterChain(AstNode input, List filterSpecs) { + this.input = Objects.requireNonNull(input, "Input node cannot be null"); + this.filterSpecs = Objects.requireNonNull(filterSpecs, "Filter specs cannot be null"); + if (filterSpecs.isEmpty()) { + throw new IllegalArgumentException("Filter chain must have at least one filter"); + } + } + + public AstNode getInput() { + return input; + } + + public List getFilterSpecs() { + return filterSpecs; + } + + @Override + public Object eval(Bindings bindings, ELContext context) { + JinjavaInterpreter interpreter = getInterpreter(context); + + if (interpreter.getContext().isValidationMode()) { + return ""; + } + + Object value = input.eval(bindings, context); + + for (FilterSpec spec : filterSpecs) { + String filterKey = ExtendedParser.FILTER_PREFIX + spec.getName(); + interpreter.getContext().addResolvedValue(filterKey); + + Filter filter; + try { + filter = interpreter.getContext().getFilter(spec.getName()); + } catch (DisabledException e) { + interpreter.addError( + new TemplateError( + ErrorType.FATAL, + ErrorReason.DISABLED, + ErrorItem.FILTER, + e.getMessage(), + spec.getName(), + interpreter.getLineNumber(), + -1, + e + ) + ); + return null; + } + if (filter == null) { + continue; + } + + Object[] args = evaluateFilterArgs(spec, bindings, context); + Map kwargs = extractNamedParams(args); + Object[] positionalArgs = extractPositionalArgs(args); + + boolean wasSafeString = value instanceof SafeString; + if (wasSafeString) { + value = value.toString(); + } + + try { + value = filter.filter(value, interpreter, positionalArgs, kwargs); + } catch (ELException e) { + throw e; + } catch (RuntimeException e) { + throw new ELException( + String.format("Error in filter '%s': %s", spec.getName(), e.getMessage()), + e + ); + } + + if (wasSafeString && filter.preserveSafeString() && value instanceof String) { + value = new SafeString((String) value); + } + } + + return value; + } + + protected JinjavaInterpreter getInterpreter(ELContext context) { + return (JinjavaInterpreter) context + .getELResolver() + .getValue(context, null, ExtendedParser.INTERPRETER); + } + + protected Object[] evaluateFilterArgs( + FilterSpec spec, + Bindings bindings, + ELContext context + ) { + AstParameters params = spec.getParams(); + if (params == null || params.getCardinality() == 0) { + return new Object[0]; + } + + Object[] args = new Object[params.getCardinality()]; + for (int i = 0; i < params.getCardinality(); i++) { + args[i] = params.getChild(i).eval(bindings, context); + } + return args; + } + + private Map extractNamedParams(Object[] args) { + Map kwargs = new LinkedHashMap<>(); + for (Object arg : args) { + if (arg instanceof NamedParameter) { + NamedParameter namedParam = (NamedParameter) arg; + kwargs.put(namedParam.getName(), namedParam.getValue()); + } + } + return kwargs; + } + + private Object[] extractPositionalArgs(Object[] args) { + List positional = new ArrayList<>(); + for (Object arg : args) { + if (!(arg instanceof NamedParameter)) { + positional.add(arg); + } + } + return positional.toArray(); + } + + @Override + public void appendStructure(StringBuilder builder, Bindings bindings) { + input.appendStructure(builder, bindings); + for (FilterSpec spec : filterSpecs) { + builder.append('|').append(spec.getName()); + AstParameters params = spec.getParams(); + if (params != null && params.getCardinality() > 0) { + builder.append('('); + for (int i = 0; i < params.getCardinality(); i++) { + if (i > 0) { + builder.append(", "); + } + params.getChild(i).appendStructure(builder, bindings); + } + builder.append(')'); + } + } + } + + @Override + public String toString() { + StringBuilder sb = new StringBuilder(); + sb.append(input.toString()); + for (FilterSpec spec : filterSpecs) { + sb.append('|').append(spec.toString()); + } + return sb.toString(); + } + + @Override + public int getCardinality() { + return 1 + filterSpecs.size(); + } + + @Override + public AstNode getChild(int i) { + if (i == 0) { + return input; + } + int filterIndex = i - 1; + if (filterIndex < filterSpecs.size()) { + FilterSpec spec = filterSpecs.get(filterIndex); + return spec.getParams(); + } + return null; + } +} diff --git a/src/main/java/com/hubspot/jinjava/el/ext/ExtendedParser.java b/src/main/java/com/hubspot/jinjava/el/ext/ExtendedParser.java index 4f7f741a0..f206fa40a 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/ExtendedParser.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/ExtendedParser.java @@ -531,30 +531,22 @@ protected AstNode value() throws ScanException, ParseException { private AstNode parseOperators(AstNode left) throws ScanException, ParseException { if ("|".equals(getToken().getImage()) && lookahead(0).getSymbol() == IDENTIFIER) { - AstNode v = left; + List filterSpecs = new ArrayList<>(); do { consumeToken(); // '|' String filterName = consumeToken().getImage(); - List filterParams = Lists.newArrayList(v, interpreter()); + AstParameters filterParams = null; // optional filter args if (getToken().getSymbol() == Symbol.LPAREN) { - AstParameters astParameters = params(); - for (int i = 0; i < astParameters.getCardinality(); i++) { - filterParams.add(astParameters.getChild(i)); - } + filterParams = params(); } - AstProperty filterProperty = createAstDot( - identifier(FILTER_PREFIX + filterName), - "filter", - true - ); - v = createAstMethod(filterProperty, createAstParameters(filterParams)); // function("filter:" + filterName, new AstParameters(filterParams)); + filterSpecs.add(new FilterSpec(filterName, filterParams)); } while ("|".equals(getToken().getImage())); - return v; + return createAstFilterChain(left, filterSpecs); } else if ( "is".equals(getToken().getImage()) && "not".equals(lookahead(0).getImage()) && @@ -577,6 +569,13 @@ protected AstParameters createAstParameters(List nodes) { return new AstParameters(nodes); } + protected AstFilterChain createAstFilterChain( + AstNode input, + List filterSpecs + ) { + return new AstFilterChain(input, filterSpecs); + } + private boolean isPossibleExpTest(Symbol symbol) { return VALID_SYMBOLS_FOR_EXP_TEST.contains(symbol); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/FilterSpec.java b/src/main/java/com/hubspot/jinjava/el/ext/FilterSpec.java new file mode 100644 index 000000000..175016913 --- /dev/null +++ b/src/main/java/com/hubspot/jinjava/el/ext/FilterSpec.java @@ -0,0 +1,48 @@ +package com.hubspot.jinjava.el.ext; + +import de.odysseus.el.tree.impl.ast.AstParameters; +import java.util.Objects; + +/** + * Specification for a filter in a filter chain. + * Holds the filter name and optional parameters. + */ +public class FilterSpec { + + private final String name; + private final AstParameters params; + + public FilterSpec(String name, AstParameters params) { + this.name = Objects.requireNonNull(name, "Filter name cannot be null"); + this.params = params; + } + + public String getName() { + return name; + } + + public AstParameters getParams() { + return params; + } + + public boolean hasParams() { + return params != null && params.getCardinality() > 0; + } + + @Override + public String toString() { + if (hasParams()) { + StringBuilder sb = new StringBuilder(name); + sb.append('('); + for (int i = 0; i < params.getCardinality(); i++) { + if (i > 0) { + sb.append(", "); + } + sb.append(params.getChild(i)); + } + sb.append(')'); + return sb.toString(); + } + return name; + } +} diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstFilterChain.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstFilterChain.java new file mode 100644 index 000000000..3954a798a --- /dev/null +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstFilterChain.java @@ -0,0 +1,225 @@ +package com.hubspot.jinjava.el.ext.eager; + +import com.hubspot.jinjava.el.ext.AstFilterChain; +import com.hubspot.jinjava.el.ext.DeferredParsingException; +import com.hubspot.jinjava.el.ext.FilterSpec; +import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; +import com.hubspot.jinjava.interpret.DeferredValueException; +import com.hubspot.jinjava.interpret.JinjavaInterpreter; +import com.hubspot.jinjava.lib.filter.Filter; +import com.hubspot.jinjava.objects.SafeString; +import com.hubspot.jinjava.util.EagerExpressionResolver; +import de.odysseus.el.tree.Bindings; +import de.odysseus.el.tree.impl.ast.AstNode; +import de.odysseus.el.tree.impl.ast.AstParameters; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import javax.el.ELContext; +import javax.el.ELException; + +/** + * Eager variant of AstFilterChain that supports deferred value resolution. + * When a deferred value is encountered during filter chain evaluation, + * this class reconstructs the partially resolved expression. + */ +public class EagerAstFilterChain extends AstFilterChain implements EvalResultHolder { + + protected Object evalResult; + protected boolean hasEvalResult; + protected final EvalResultHolder inputHolder; + protected final List paramHolders; + + public EagerAstFilterChain(AstNode input, List filterSpecs) { + super( + (AstNode) EagerAstNodeDecorator.getAsEvalResultHolder(input), + wrapFilterSpecs(filterSpecs) + ); + this.inputHolder = (EvalResultHolder) this.input; + this.paramHolders = extractParamHolders(this.filterSpecs); + } + + private static List wrapFilterSpecs(List specs) { + List wrapped = new ArrayList<>(specs.size()); + for (FilterSpec spec : specs) { + AstParameters params = spec.getParams(); + if (params != null) { + params = (AstParameters) EagerAstNodeDecorator.getAsEvalResultHolder(params); + } + wrapped.add(new FilterSpec(spec.getName(), params)); + } + return wrapped; + } + + private static List extractParamHolders(List specs) { + List holders = new ArrayList<>(); + for (FilterSpec spec : specs) { + if (spec.getParams() instanceof EvalResultHolder) { + holders.add((EvalResultHolder) spec.getParams()); + } + } + return holders; + } + + @Override + public Object eval(Bindings bindings, ELContext context) { + try { + setEvalResult(evalFilterChain(bindings, context)); + return checkEvalResultSize(context); + } catch (DeferredValueException | ELException originalException) { + DeferredParsingException e = EvalResultHolder.convertToDeferredParsingException( + originalException + ); + throw new DeferredParsingException( + this, + getPartiallyResolved( + bindings, + context, + e, + IdentifierPreservationStrategy.PRESERVING + ), + IdentifierPreservationStrategy.PRESERVING + ); + } + } + + /** + * Evaluates the filter chain, keeping track of which filters have been applied + * to support partial resolution when deferred values are encountered. + */ + protected Object evalFilterChain(Bindings bindings, ELContext context) { + JinjavaInterpreter interpreter = getInterpreter(context); + + if (interpreter.getContext().isValidationMode()) { + return ""; + } + + Object value = input.eval(bindings, context); + + for (FilterSpec spec : filterSpecs) { + Filter filter = interpreter.getContext().getFilter(spec.getName()); + if (filter == null) { + continue; + } + + Object[] args = evaluateFilterArgs(spec, bindings, context); + Map kwargs = extractNamedParams(args); + Object[] positionalArgs = extractPositionalArgs(args); + + boolean wasSafeString = value instanceof SafeString; + if (wasSafeString) { + value = value.toString(); + } + + try { + value = filter.filter(value, interpreter, positionalArgs, kwargs); + } catch (ELException e) { + throw e; + } catch (DeferredValueException e) { + throw e; + } catch (RuntimeException e) { + throw new ELException( + String.format("Error in filter '%s': %s", spec.getName(), e.getMessage()), + e + ); + } + + if (wasSafeString && filter.preserveSafeString() && value instanceof String) { + value = new SafeString((String) value); + } + } + + return value; + } + + private Map extractNamedParams(Object[] args) { + java.util.Map kwargs = new java.util.LinkedHashMap<>(); + for (Object arg : args) { + if (arg instanceof com.hubspot.jinjava.el.ext.NamedParameter) { + com.hubspot.jinjava.el.ext.NamedParameter namedParam = + (com.hubspot.jinjava.el.ext.NamedParameter) arg; + kwargs.put(namedParam.getName(), namedParam.getValue()); + } + } + return kwargs; + } + + private Object[] extractPositionalArgs(Object[] args) { + java.util.List positional = new java.util.ArrayList<>(); + for (Object arg : args) { + if (!(arg instanceof com.hubspot.jinjava.el.ext.NamedParameter)) { + positional.add(arg); + } + } + return positional.toArray(); + } + + @Override + public Object getEvalResult() { + return evalResult; + } + + @Override + public void setEvalResult(Object evalResult) { + this.evalResult = evalResult; + hasEvalResult = true; + } + + @Override + public boolean hasEvalResult() { + return hasEvalResult; + } + + @Override + public String getPartiallyResolved( + Bindings bindings, + ELContext context, + DeferredParsingException deferredParsingException, + IdentifierPreservationStrategy identifierPreservationStrategy + ) { + StringBuilder sb = new StringBuilder(); + + String inputResult = EvalResultHolder.reconstructNode( + bindings, + context, + inputHolder, + deferredParsingException, + identifierPreservationStrategy + ); + sb.append(inputResult); + + for (FilterSpec spec : filterSpecs) { + sb.append('|').append(spec.getName()); + AstParameters params = spec.getParams(); + if (params != null && params.getCardinality() > 0) { + sb.append('('); + for (int i = 0; i < params.getCardinality(); i++) { + if (i > 0) { + sb.append(", "); + } + AstNode paramNode = params.getChild(i); + if (paramNode instanceof EvalResultHolder) { + String paramResult = EvalResultHolder.reconstructNode( + bindings, + context, + (EvalResultHolder) paramNode, + deferredParsingException, + identifierPreservationStrategy + ); + sb.append(paramResult); + } else { + try { + Object paramValue = paramNode.eval(bindings, context); + sb.append(EagerExpressionResolver.getValueAsJinjavaStringSafe(paramValue)); + } catch (DeferredValueException e) { + sb.append(paramNode.toString()); + } + } + } + sb.append(')'); + } + } + + return sb.toString(); + } +} diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerExtendedParser.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerExtendedParser.java index 5ca383afd..bb171f1cd 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerExtendedParser.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerExtendedParser.java @@ -6,12 +6,14 @@ import com.hubspot.jinjava.el.ext.AbsOperator; import com.hubspot.jinjava.el.ext.AstDict; +import com.hubspot.jinjava.el.ext.AstFilterChain; import com.hubspot.jinjava.el.ext.AstList; import com.hubspot.jinjava.el.ext.AstRangeBracket; import com.hubspot.jinjava.el.ext.AstTuple; import com.hubspot.jinjava.el.ext.CollectionMembershipOperator; import com.hubspot.jinjava.el.ext.CollectionNonMembershipOperator; import com.hubspot.jinjava.el.ext.ExtendedParser; +import com.hubspot.jinjava.el.ext.FilterSpec; import com.hubspot.jinjava.el.ext.NamedParameterOperator; import com.hubspot.jinjava.el.ext.PowerOfOperator; import com.hubspot.jinjava.el.ext.StringConcatOperator; @@ -198,4 +200,12 @@ protected AstList createAstList(AstParameters parameters) protected AstParameters createAstParameters(List nodes) { return new EagerAstParameters(nodes); } + + @Override + protected AstFilterChain createAstFilterChain( + AstNode input, + List filterSpecs + ) { + return new EagerAstFilterChain(input, filterSpecs); + } } diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java index e720a02bc..ba5ef053c 100644 --- a/src/test/java/com/hubspot/jinjava/EagerTest.java +++ b/src/test/java/com/hubspot/jinjava/EagerTest.java @@ -307,8 +307,7 @@ public void itPreservesForTag() { @Test public void itPreservesFilters() { String output = interpreter.render("{{ deferred|capitalize }}"); - assertThat(output) - .isEqualTo("{{ filter:capitalize.filter(deferred, ____int3rpr3t3r____) }}"); + assertThat(output).isEqualTo("{{ deferred|capitalize }}"); assertThat(interpreter.getErrors()).isEmpty(); localContext.put("deferred", "foo"); assertThat(interpreter.render(output)).isEqualTo("Foo"); @@ -317,18 +316,14 @@ public void itPreservesFilters() { @Test public void itPreservesFunctions() { String output = interpreter.render("{{ deferred|datetimeformat('%B %e, %Y') }}"); - assertThat(output) - .isEqualTo( - "{{ filter:datetimeformat.filter(deferred, ____int3rpr3t3r____, '%B %e, %Y') }}" - ); + assertThat(output).isEqualTo("{{ deferred|datetimeformat('%B %e, %Y') }}"); assertThat(interpreter.getErrors()).isEmpty(); } @Test public void itPreservesRandomness() { String output = interpreter.render("{{ [1, 2, 3]|shuffle }}"); - assertThat(output) - .isEqualTo("{{ filter:shuffle.filter([1, 2, 3], ____int3rpr3t3r____) }}"); + assertThat(output).isEqualTo("{{ [1, 2, 3]|shuffle }}"); assertThat(interpreter.getErrors()).isEmpty(); } 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 7a24f9e5b..0f869bb24 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 @@ -231,8 +231,7 @@ public void itPreservesUnresolvable() { interpreter.resolveELExpression("foo_object.deferred|upper", -1); fail("Should throw DeferredParsingException"); } catch (DeferredParsingException e) { - assertThat(e.getDeferredEvalResult()) - .isEqualTo("filter:upper.filter(foo_object.deferred, ____int3rpr3t3r____)"); + assertThat(e.getDeferredEvalResult()).isEqualTo("foo_object.deferred|upper"); } } } diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java index 031f7183d..b00ae916e 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java @@ -423,7 +423,7 @@ public void itCorrectlySetsAliasedPathForSecondPass() { assertThat(firstPassResult) .isEqualTo( "{% set deferred_import_resource_path = 'import-macro.jinja' %}{% macro m.print_path_macro(var) %}\n" + - "{{ filter:print_path.filter(var, ____int3rpr3t3r____) }}\n" + + "{{ var|print_path }}\n" + "{{ var }}\n" + "{% endmacro %}{% set deferred_import_resource_path = null %}{{ m.print_path_macro(foo) }}" ); @@ -442,7 +442,7 @@ public void itCorrectlySetsPathForSecondPass() { assertThat(firstPassResult) .isEqualTo( "{% set deferred_import_resource_path = 'import-macro.jinja' %}{% macro print_path_macro(var) %}\n" + - "{{ filter:print_path.filter(var, ____int3rpr3t3r____) }}\n" + + "{{ var|print_path }}\n" + "{{ var }}\n" + "{% endmacro %}{% set deferred_import_resource_path = null %}{{ print_path_macro(foo) }}" ); @@ -460,9 +460,9 @@ public void itCorrectlySetsNestedPathsForSecondPass() { ); assertThat(firstPassResult) .isEqualTo( - "{% set deferred_import_resource_path = 'double-import-macro.jinja' %}{% macro print_path_macro2(var) %}{{ filter:print_path.filter(var, ____int3rpr3t3r____) }}\n" + + "{% set deferred_import_resource_path = 'double-import-macro.jinja' %}{% macro print_path_macro2(var) %}{{ var|print_path }}\n" + "{% set deferred_import_resource_path = 'import-macro.jinja' %}{% macro print_path_macro(var) %}\n" + - "{{ filter:print_path.filter(var, ____int3rpr3t3r____) }}\n" + + "{{ var|print_path }}\n" + "{{ var }}\n" + "{% endmacro %}{% set deferred_import_resource_path = 'double-import-macro.jinja' %}{{ print_path_macro(var) }}{% endmacro %}{% set deferred_import_resource_path = null %}{{ print_path_macro2(foo) }}" ); diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagTest.java index d86055ff2..3acf7b665 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagTest.java @@ -141,10 +141,7 @@ public void itDefersBlockWithFilter() { "{% set foo | int |add(deferred) %}{{ 1 + 1 }}{% endset %}{{ foo }}"; final String result = interpreter.render(template); - assertThat(result) - .isEqualTo( - "{% set foo = filter:add.filter(2, ____int3rpr3t3r____, deferred) %}{{ foo }}" - ); + assertThat(result).isEqualTo("{% set foo = '2'|int|add(deferred) %}{{ foo }}"); assertThat( context .getDeferredTokens() @@ -160,7 +157,7 @@ public void itDefersBlockWithFilter() { .flatMap(deferredToken -> deferredToken.getUsedDeferredWords().stream()) .collect(Collectors.toSet()) ) - .containsExactlyInAnyOrder("deferred", "foo", "add.filter"); + .containsExactlyInAnyOrder("deferred", "foo", "add", "int"); assertThat( context .getDeferredTokens() @@ -168,7 +165,7 @@ public void itDefersBlockWithFilter() { .flatMap(deferredToken -> deferredToken.getUsedDeferredBases().stream()) .collect(Collectors.toSet()) ) - .containsExactlyInAnyOrder("deferred", "foo", "add"); + .containsExactlyInAnyOrder("deferred", "foo", "add", "int"); } @Test @@ -179,7 +176,7 @@ public void itDefersDeferredBlockWithDeferredFilter() { assertThat(result) .isEqualTo( - "{% set foo %}{{ 1 + deferred }}{% endset %}{% set foo = filter:add.filter(foo, ____int3rpr3t3r____, filter:int.filter(deferred, ____int3rpr3t3r____)) %}{{ foo }}" + "{% set foo %}{{ 1 + deferred }}{% endset %}{% set foo = foo|add(deferred|int) %}{{ foo }}" ); context.remove("foo"); context.put("deferred", 2); @@ -203,7 +200,7 @@ public void itDefersInDeferredExecutionModeWithFilter() { assertThat(result) .isEqualTo( - "{% set foo %}1{% endset %}{% set foo = filter:add.filter(1, ____int3rpr3t3r____, deferred) %}{{ foo }}" + "{% set foo %}1{% endset %}{% set foo = '1'|int|add(deferred) %}{{ foo }}" ); assertThat( context @@ -220,7 +217,7 @@ public void itDefersInDeferredExecutionModeWithFilter() { .flatMap(deferredToken -> deferredToken.getUsedDeferredWords().stream()) .collect(Collectors.toSet()) ) - .containsExactlyInAnyOrder("deferred", "foo", "add.filter"); + .containsExactlyInAnyOrder("deferred", "foo", "add", "int"); assertThat( context .getDeferredTokens() @@ -228,7 +225,7 @@ public void itDefersInDeferredExecutionModeWithFilter() { .flatMap(deferredToken -> deferredToken.getUsedDeferredBases().stream()) .collect(Collectors.toSet()) ) - .containsExactlyInAnyOrder("deferred", "foo", "add"); + .containsExactlyInAnyOrder("deferred", "foo", "add", "int"); context.remove("foo"); context.put("deferred", 2); context.setDeferredExecutionMode(false); diff --git a/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java b/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java index 2e1d962cf..65855fdf9 100644 --- a/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java +++ b/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java @@ -363,7 +363,7 @@ public void itHandlesEscapedSlashBeforeQuoteProperly() { "deferred|replace('\\\\', '.')" ); assertThat(eagerExpressionResult.getDeferredWords()) - .containsExactlyInAnyOrder("deferred", "replace.filter"); + .containsExactlyInAnyOrder("deferred", "replace"); } @Test @@ -750,7 +750,7 @@ public void itHandlesDeferredMethod() { assertThat(eagerResolveExpression("deferred.append(foo)").toString()) .isEqualTo("deferred.append('foo')"); assertThat(eagerResolveExpression("deferred[1 + 1] | length").toString()) - .isEqualTo("filter:length.filter(deferred[2], ____int3rpr3t3r____)"); + .isEqualTo("deferred[2]|length"); } @Test @@ -817,7 +817,7 @@ public void itDoesNotSplitJsonInArrayResolvedExpression() { @Test public void itHandlesRandom() { assertThat(eagerResolveExpression("range(1)|random").toString()) - .isEqualTo("filter:random.filter(range(1), ____int3rpr3t3r____)"); + .isEqualTo("range(1)|random"); } @Test diff --git a/src/test/resources/eager/defers-macro-in-expression/test.expected.jinja b/src/test/resources/eager/defers-macro-in-expression/test.expected.jinja index 7443a94eb..e6a659701 100644 --- a/src/test/resources/eager/defers-macro-in-expression/test.expected.jinja +++ b/src/test/resources/eager/defers-macro-in-expression/test.expected.jinja @@ -1,10 +1,10 @@ 2 {% macro plus(foo, add) %}\ -{{ foo + (filter:int.filter(add, ____int3rpr3t3r____)) }}\ +{{ foo + (add|int) }}\ {% endmacro %}\ {{ plus(deferred, 1.1) }}\ {% set deferred = deferred + 2 %} {% macro plus(foo, add) %}\ -{{ foo + (filter:int.filter(add, ____int3rpr3t3r____)) }}\ +{{ foo + (add|int) }}\ {% endmacro %}\ {{ plus(deferred, 3.1) }} \ No newline at end of file diff --git a/src/test/resources/eager/defers-macro-in-for/test.expected.jinja b/src/test/resources/eager/defers-macro-in-for/test.expected.jinja index 3311b8714..6b183ee39 100644 --- a/src/test/resources/eager/defers-macro-in-for/test.expected.jinja +++ b/src/test/resources/eager/defers-macro-in-for/test.expected.jinja @@ -3,6 +3,6 @@ {% do my_list.append(num) %}\ {{ my_list }}\ {% endmacro %}\ -{% for item in filter:split.filter(macro_append(deferred), ____int3rpr3t3r____, ',', 2) %} +{% for item in macro_append(deferred)|split(',', 2) %} {{ item }} {% endfor %} diff --git a/src/test/resources/eager/defers-macro-in-if/test.expected.jinja b/src/test/resources/eager/defers-macro-in-if/test.expected.jinja index 67f28d9e4..d73fe7c9f 100644 --- a/src/test/resources/eager/defers-macro-in-if/test.expected.jinja +++ b/src/test/resources/eager/defers-macro-in-if/test.expected.jinja @@ -3,6 +3,6 @@ {% do my_list.append(num) %}\ {{ my_list }}\ {% endmacro %}\ -{% if [] == filter:split.filter(macro_append(deferred), ____int3rpr3t3r____, ',', 2) %} +{% if [] == macro_append(deferred)|split(',', 2) %} {{ my_list }} {% endif %} diff --git a/src/test/resources/eager/does-not-override-import-modification-in-for/test.expected.jinja b/src/test/resources/eager/does-not-override-import-modification-in-for/test.expected.jinja index 0e1e4a8a2..de5c88d02 100644 --- a/src/test/resources/eager/does-not-override-import-modification-in-for/test.expected.jinja +++ b/src/test/resources/eager/does-not-override-import-modification-in-for/test.expected.jinja @@ -11,7 +11,7 @@ {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ +{% set foo = [foo, 'b']|join('') %}\ {% do __temp_meta_import_alias_3016318__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016318__.update({'foo': foo,'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ @@ -25,12 +25,12 @@ {% for __ignored__ in [0] %}\ {% if deferred %} -{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}\ +{% set foo = [foo, 'a']|join('') %}\ {% do __temp_meta_import_alias_3016319__.update({'foo': foo}) %} {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ +{% set foo = [foo, 'b']|join('') %}\ {% do __temp_meta_import_alias_3016319__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016319__.update({'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ @@ -45,12 +45,12 @@ {% for __ignored__ in [0] %}\ {% if deferred %} -{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}\ +{% set foo = [foo, 'a']|join('') %}\ {% do __temp_meta_import_alias_3016318__.update({'foo': foo}) %} {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ +{% set foo = [foo, 'b']|join('') %}\ {% do __temp_meta_import_alias_3016318__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016318__.update({'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ @@ -64,12 +64,12 @@ {% for __ignored__ in [0] %}\ {% if deferred %} -{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}\ +{% set foo = [foo, 'a']|join('') %}\ {% do __temp_meta_import_alias_3016319__.update({'foo': foo}) %} {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ +{% set foo = [foo, 'b']|join('') %}\ {% do __temp_meta_import_alias_3016319__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016319__.update({'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ diff --git a/src/test/resources/eager/fully-defers-filtered-macro/test.expected.jinja b/src/test/resources/eager/fully-defers-filtered-macro/test.expected.jinja index 4938403bd..e2268dbb1 100644 --- a/src/test/resources/eager/fully-defers-filtered-macro/test.expected.jinja +++ b/src/test/resources/eager/fully-defers-filtered-macro/test.expected.jinja @@ -1,5 +1,5 @@ {% macro flashy(foo) %}\ -{{ filter:upper.filter(foo, ____int3rpr3t3r____) }} +{{ foo|upper }} A flashy {{ deferred }}\ .{% endmacro %}\ {% set __macro_flashy_1625622909_temp_variable_0__ %}\ @@ -12,4 +12,4 @@ BAR {% 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 +{{ __macro_silly_2092874071_temp_variable_0__|upper }} \ No newline at end of file diff --git a/src/test/resources/eager/handles-deferred-cycle-as/test.expected.jinja b/src/test/resources/eager/handles-deferred-cycle-as/test.expected.jinja index 5893e2ee2..5b52e0fd9 100644 --- a/src/test/resources/eager/handles-deferred-cycle-as/test.expected.jinja +++ b/src/test/resources/eager/handles-deferred-cycle-as/test.expected.jinja @@ -1,19 +1,19 @@ {% for __ignored__ in [0] %} {% set c = [1, deferred] %} {% if exptest:iterable.evaluate(c, ____int3rpr3t3r____) %}\ -{{ c[0 % filter:length.filter(c, ____int3rpr3t3r____)] }}\ +{{ c[0 % c|length] }}\ {% else %}\ {{ c }}\ {% endif %} {% set c = [2, deferred] %} {% if exptest:iterable.evaluate(c, ____int3rpr3t3r____) %}\ -{{ c[1 % filter:length.filter(c, ____int3rpr3t3r____)] }}\ +{{ c[1 % c|length] }}\ {% else %}\ {{ c }}\ {% endif %} {% set c = [3, deferred] %} {% if exptest:iterable.evaluate(c, ____int3rpr3t3r____) %}\ -{{ c[2 % filter:length.filter(c, ____int3rpr3t3r____)] }}\ +{{ c[2 % c|length] }}\ {% else %}\ {{ c }}\ {% endif %}\ diff --git a/src/test/resources/eager/handles-deferred-for-loop-var-from-macro/test.expected.jinja b/src/test/resources/eager/handles-deferred-for-loop-var-from-macro/test.expected.jinja index 54d9fc69a..210f8f1b2 100644 --- a/src/test/resources/eager/handles-deferred-for-loop-var-from-macro/test.expected.jinja +++ b/src/test/resources/eager/handles-deferred-for-loop-var-from-macro/test.expected.jinja @@ -3,14 +3,14 @@ {% for __ignored__ in [0] %} {% set __macro_doIt_1327224118_temp_variable_0__ %} -{{ deferred ~ '{\"a\":\"a\"}' }} +{{ deferred ~ '{"a":"a"}' }} {% endset %}\ -{{ filter:upper.filter(__macro_doIt_1327224118_temp_variable_0__, ____int3rpr3t3r____) }} +{{ __macro_doIt_1327224118_temp_variable_0__|upper }} {% set __macro_doIt_1327224118_temp_variable_1__ %} -{{ deferred ~ '{\"b\":\"b\"}' }} +{{ deferred ~ '{"b":"b"}' }} {% endset %}\ -{{ filter:upper.filter(__macro_doIt_1327224118_temp_variable_1__, ____int3rpr3t3r____) }} +{{ __macro_doIt_1327224118_temp_variable_1__|upper }} {% endfor %} {% endset %}\ -{{ filter:upper.filter(__macro_getData_357124436_temp_variable_0__, ____int3rpr3t3r____) }} +{{ __macro_getData_357124436_temp_variable_0__|upper }} diff --git a/src/test/resources/eager/handles-double-import-modification/test.expected.jinja b/src/test/resources/eager/handles-double-import-modification/test.expected.jinja index 66acdde36..cd174801f 100644 --- a/src/test/resources/eager/handles-double-import-modification/test.expected.jinja +++ b/src/test/resources/eager/handles-double-import-modification/test.expected.jinja @@ -9,7 +9,7 @@ {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ +{% set foo = [foo, 'b']|join('') %}\ {% do __temp_meta_import_alias_3016318__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016318__.update({'foo': foo,'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ @@ -28,7 +28,7 @@ {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ +{% set foo = [foo, 'b']|join('') %}\ {% do __temp_meta_import_alias_3016319__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016319__.update({'foo': foo,'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ diff --git a/src/test/resources/eager/handles-same-name-import-var/test.expected.jinja b/src/test/resources/eager/handles-same-name-import-var/test.expected.jinja index 66cfa99f0..4cec80792 100644 --- a/src/test/resources/eager/handles-same-name-import-var/test.expected.jinja +++ b/src/test/resources/eager/handles-same-name-import-var/test.expected.jinja @@ -35,5 +35,5 @@ {% set my_var = __temp_meta_import_alias_1059697132__ %}\ {% set current_path,__temp_meta_current_path_944750549__ = __temp_meta_current_path_944750549__,null %}\ {% enddo %} -{{ filter:dictsort.filter(my_var, ____int3rpr3t3r____, false, 'key') }} +{{ my_var|dictsort(false, 'key') }} {% endif %} diff --git a/src/test/resources/eager/reconstructs-block-set-variables-in-for-loop/test.expected.jinja b/src/test/resources/eager/reconstructs-block-set-variables-in-for-loop/test.expected.jinja index d0152a7f9..40fb3b930 100644 --- a/src/test/resources/eager/reconstructs-block-set-variables-in-for-loop/test.expected.jinja +++ b/src/test/resources/eager/reconstructs-block-set-variables-in-for-loop/test.expected.jinja @@ -2,5 +2,5 @@ {% set __macro_foo_97643642_temp_variable_0__ %} {{ deferred }} {% endset %}\ -{{ filter:int.filter(__macro_foo_97643642_temp_variable_0__, ____int3rpr3t3r____) + 3 }} +{{ __macro_foo_97643642_temp_variable_0__|int + 3 }} {% endfor %} diff --git a/src/test/resources/eager/reconstructs-fromed-macro/test.expected.jinja b/src/test/resources/eager/reconstructs-fromed-macro/test.expected.jinja index c7213b008..3aa5fc5c2 100644 --- a/src/test/resources/eager/reconstructs-fromed-macro/test.expected.jinja +++ b/src/test/resources/eager/reconstructs-fromed-macro/test.expected.jinja @@ -1,6 +1,6 @@ {% set deferred_import_resource_path = 'eager/reconstructs-fromed-macro/has-macro.jinja' %}\ {% macro to_upper(param) %} - {{ filter:upper.filter(param, ____int3rpr3t3r____) }} + {{ param|upper }} {% endmacro %}\ {% set deferred_import_resource_path = null %}\ {{ to_upper(deferred) }} \ No newline at end of file diff --git a/src/test/resources/eager/uses-unique-macro-names/test.expected.jinja b/src/test/resources/eager/uses-unique-macro-names/test.expected.jinja index 54c40b809..a1089d9bc 100644 --- a/src/test/resources/eager/uses-unique-macro-names/test.expected.jinja +++ b/src/test/resources/eager/uses-unique-macro-names/test.expected.jinja @@ -3,13 +3,13 @@ {% set __macro_foo_97643642_temp_variable_0__ %} Goodbye {{ myname }} {% endset %}\ -{% set a = filter:upper.filter(__macro_foo_97643642_temp_variable_0__, ____int3rpr3t3r____) %} +{% set a = __macro_foo_97643642_temp_variable_0__|upper %} {% do %}\ {% set __temp_meta_current_path_203114534__,current_path = current_path,'eager/uses-unique-macro-names/macro-with-filter.jinja' %} {% set __macro_foo_1717337666_temp_variable_0__ %}\ Hello {{ myname }}\ {% endset %}\ -{% set b = filter:upper.filter(__macro_foo_1717337666_temp_variable_0__, ____int3rpr3t3r____) %} +{% set b = __macro_foo_1717337666_temp_variable_0__|upper %} {% set current_path,__temp_meta_current_path_203114534__ = __temp_meta_current_path_203114534__,null %}\ {% enddo %} {{ a }} From 9b5e8b0e2ce3f3a5dab2008035a90d4518f2d3c0 Mon Sep 17 00:00:00 2001 From: Libo Song Date: Mon, 12 Jan 2026 11:11:38 -0500 Subject: [PATCH 2/8] Make filter chain optimization configurable and disable for eager mode - Add enableFilterChainOptimization config option to JinjavaConfig (defaults to false) - Modify ExtendedParser to conditionally use AstFilterChain based on config - Override shouldUseFilterChainOptimization() in EagerExtendedParser to always return false - Remove EagerAstFilterChain.java as eager mode now uses old nested method approach - Revert test files to expect old format for eager execution --- keep-undefined.md | 88 +++++++ .../com/hubspot/jinjava/JinjavaConfig.java | 14 ++ .../jinjava/el/ext/ExtendedParser.java | 76 ++++-- .../el/ext/eager/EagerAstFilterChain.java | 225 ------------------ .../el/ext/eager/EagerExtendedParser.java | 9 +- .../java/com/hubspot/jinjava/EagerTest.java | 11 +- .../el/ext/eager/EagerAstMethodTest.java | 3 +- .../lib/tag/eager/EagerImportTagTest.java | 8 +- .../lib/tag/eager/EagerSetTagTest.java | 17 +- .../util/EagerExpressionResolverTest.java | 6 +- .../test.expected.jinja | 4 +- .../defers-macro-in-for/test.expected.jinja | 2 +- .../defers-macro-in-if/test.expected.jinja | 2 +- .../test.expected.jinja | 14 +- .../test.expected.jinja | 4 +- .../test.expected.jinja | 6 +- .../test.expected.jinja | 10 +- .../test.expected.jinja | 4 +- .../test.expected.jinja | 2 +- .../test.expected.jinja | 2 +- .../test.expected.jinja | 2 +- .../test.expected.jinja | 4 +- 22 files changed, 219 insertions(+), 294 deletions(-) create mode 100644 keep-undefined.md delete mode 100644 src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstFilterChain.java diff --git a/keep-undefined.md b/keep-undefined.md new file mode 100644 index 000000000..044d97572 --- /dev/null +++ b/keep-undefined.md @@ -0,0 +1,88 @@ +# PreserveUnknownExecutionMode + +A Jinjava execution mode that preserves unknown/undefined variables as their original template syntax instead of rendering them as empty strings. This enables multi-pass rendering scenarios where templates are processed in stages with different variable contexts available at each stage. + +## Usage + +```java +JinjavaConfig config = JinjavaConfig + .newBuilder() + .withExecutionMode(PreserveUnknownExecutionMode.instance()) + .build(); +Jinjava jinjava = new Jinjava(config); + +String result = jinjava.render("Hello {{ name }}!", new HashMap<>()); +// Result: "Hello {{ name }}!" +``` + +## Behavior + +### Expressions + +| Template | Context | Output | +|----------|---------|--------| +| `{{ unknown }}` | `{}` | `{{ unknown }}` | +| `{{ name }}` | `{name: "World"}` | `World` | +| `{{ name \| upper }}` | `{}` | `{{ name \| upper }}` | +| `{{ obj.property }}` | `{}` | `{{ obj.property }}` | + +### Control Structures + +| Template | Context | Output | +|----------|---------|--------| +| `{% if item %}yes{% endif %}` | `{}` | `{% if item %}yes{% endif %}` | +| `{% if item %}yes{% endif %}` | `{item: true}` | `yes` | +| `{% if item %}yes{% else %}no{% endif %}` | `{}` | `{% if item %}yes{% else %}no{% endif %}` | +| `{% for x in items %}{{ x }}{% endfor %}` | `{}` | `{% for x in items %}{{ x }}{% endfor %}` | +| `{% for x in items %}{{ x }}{% endfor %}` | `{items: ["a","b"]}` | `ab` | + +### Set Tags + +Set tags are preserved in output while their right-hand side expressions are evaluated when possible: + +| Template | Output | +|----------|--------| +| `{% set var = unknown %}{{ var }}` | `{% set var = unknown %}{{ var }}` | +| `{% set a = 1 %}{{ a }}` | `{% set a = 1 %}1` | +| `{% set a = 1 %}{% set b = a %}{{ b }}` | `{% set a = 1 %}{% set b = 1 %}1` | + +### Include Tags + +| Scenario | Template | Output | +|----------|----------|--------| +| Unknown path variable | `{% include unknown_path %}` | `{% include unknown_path %}` | +| Known literal path | `{% include 'test.html' %}` | *(renders included content)* | +| Known path with unknown vars | `{% include 'test.html' %}` where test.html contains `{{ unknown }}` | `{{ unknown }}` | + +## Implementation Details + +`PreserveUnknownExecutionMode` configures the Jinjava context with: + +1. **Eager Tag Decorators**: Registers eager versions of all tags to handle deferred values +2. **Dynamic Variable Resolver**: Returns `DeferredValue.instance()` for any unknown variable +3. **Deferred Execution Mode**: Enables preservation of set tags and other state-changing operations + +```java +@Override +public void prepareContext(Context context) { + // Register eager tag decorators for all tags + context.getAllTags().stream() + .filter(tag -> !(tag instanceof EagerTagDecorator)) + .map(EagerTagFactory::getEagerTagDecorator) + .filter(Optional::isPresent) + .forEach(maybeEagerTag -> context.registerTag(maybeEagerTag.get())); + + // Return DeferredValue for unknown variables + context.setDynamicVariableResolver(varName -> DeferredValue.instance()); + + // Preserve set tags in output + context.setDeferredExecutionMode(true); +} +``` + +## Use Cases + +- **Multi-pass rendering**: First pass resolves some variables, second pass resolves others +- **Template composition**: Combine templates where some placeholders are filled later +- **Partial evaluation**: Evaluate what's known now, defer what isn't +- **Template debugging**: See which variables are missing without errors diff --git a/src/main/java/com/hubspot/jinjava/JinjavaConfig.java b/src/main/java/com/hubspot/jinjava/JinjavaConfig.java index 3cff4787c..343df63a8 100644 --- a/src/main/java/com/hubspot/jinjava/JinjavaConfig.java +++ b/src/main/java/com/hubspot/jinjava/JinjavaConfig.java @@ -88,6 +88,7 @@ public class JinjavaConfig { private final ExecutionMode executionMode; private final LegacyOverrides legacyOverrides; private final boolean enablePreciseDivideFilter; + private final boolean enableFilterChainOptimization; private final ObjectMapper objectMapper; private final Features features; @@ -151,6 +152,7 @@ private JinjavaConfig(Builder builder) { legacyOverrides = builder.legacyOverrides; dateTimeProvider = builder.dateTimeProvider; enablePreciseDivideFilter = builder.enablePreciseDivideFilter; + enableFilterChainOptimization = builder.enableFilterChainOptimization; objectMapper = setupObjectMapper(builder.objectMapper); objectUnwrapper = builder.objectUnwrapper; processors = builder.processors; @@ -307,6 +309,10 @@ public boolean getEnablePreciseDivideFilter() { return enablePreciseDivideFilter; } + public boolean isEnableFilterChainOptimization() { + return enableFilterChainOptimization; + } + public DateTimeProvider getDateTimeProvider() { return dateTimeProvider; } @@ -349,6 +355,7 @@ public static class Builder { private ExecutionMode executionMode = DefaultExecutionMode.instance(); private LegacyOverrides legacyOverrides = LegacyOverrides.NONE; private boolean enablePreciseDivideFilter = false; + private boolean enableFilterChainOptimization = false; private ObjectMapper objectMapper = null; private ObjectUnwrapper objectUnwrapper = new JinjavaObjectUnwrapper(); @@ -520,6 +527,13 @@ public Builder withEnablePreciseDivideFilter(boolean enablePreciseDivideFilter) return this; } + public Builder withEnableFilterChainOptimization( + boolean enableFilterChainOptimization + ) { + this.enableFilterChainOptimization = enableFilterChainOptimization; + return this; + } + public Builder withObjectMapper(ObjectMapper objectMapper) { this.objectMapper = objectMapper; return this; diff --git a/src/main/java/com/hubspot/jinjava/el/ext/ExtendedParser.java b/src/main/java/com/hubspot/jinjava/el/ext/ExtendedParser.java index f206fa40a..543726a7b 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/ExtendedParser.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/ExtendedParser.java @@ -531,22 +531,11 @@ protected AstNode value() throws ScanException, ParseException { private AstNode parseOperators(AstNode left) throws ScanException, ParseException { if ("|".equals(getToken().getImage()) && lookahead(0).getSymbol() == IDENTIFIER) { - List filterSpecs = new ArrayList<>(); - - do { - consumeToken(); // '|' - String filterName = consumeToken().getImage(); - AstParameters filterParams = null; - - // optional filter args - if (getToken().getSymbol() == Symbol.LPAREN) { - filterParams = params(); - } - - filterSpecs.add(new FilterSpec(filterName, filterParams)); - } while ("|".equals(getToken().getImage())); - - return createAstFilterChain(left, filterSpecs); + if (shouldUseFilterChainOptimization()) { + return parseFiltersAsChain(left); + } else { + return parseFiltersAsNestedMethods(left); + } } else if ( "is".equals(getToken().getImage()) && "not".equals(lookahead(0).getImage()) && @@ -576,6 +565,61 @@ protected AstFilterChain createAstFilterChain( return new AstFilterChain(input, filterSpecs); } + private AstNode parseFiltersAsChain(AstNode left) throws ScanException, ParseException { + List filterSpecs = new ArrayList<>(); + + do { + consumeToken(); // '|' + String filterName = consumeToken().getImage(); + AstParameters filterParams = null; + + // optional filter args + if (getToken().getSymbol() == Symbol.LPAREN) { + filterParams = params(); + } + + filterSpecs.add(new FilterSpec(filterName, filterParams)); + } while ("|".equals(getToken().getImage())); + + return createAstFilterChain(left, filterSpecs); + } + + protected AstNode parseFiltersAsNestedMethods(AstNode left) + throws ScanException, ParseException { + AstNode v = left; + + do { + consumeToken(); // '|' + String filterName = consumeToken().getImage(); + List filterParams = Lists.newArrayList(v, interpreter()); + + // optional filter args + if (getToken().getSymbol() == Symbol.LPAREN) { + AstParameters astParameters = params(); + for (int i = 0; i < astParameters.getCardinality(); i++) { + filterParams.add(astParameters.getChild(i)); + } + } + + AstProperty filterProperty = createAstDot( + identifier(FILTER_PREFIX + filterName), + "filter", + true + ); + v = createAstMethod(filterProperty, createAstParameters(filterParams)); + } while ("|".equals(getToken().getImage())); + + return v; + } + + protected boolean shouldUseFilterChainOptimization() { + return JinjavaInterpreter + .getCurrentMaybe() + .map(JinjavaInterpreter::getConfig) + .map(JinjavaConfig::isEnableFilterChainOptimization) + .orElse(false); + } + private boolean isPossibleExpTest(Symbol symbol) { return VALID_SYMBOLS_FOR_EXP_TEST.contains(symbol); } diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstFilterChain.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstFilterChain.java deleted file mode 100644 index 3954a798a..000000000 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerAstFilterChain.java +++ /dev/null @@ -1,225 +0,0 @@ -package com.hubspot.jinjava.el.ext.eager; - -import com.hubspot.jinjava.el.ext.AstFilterChain; -import com.hubspot.jinjava.el.ext.DeferredParsingException; -import com.hubspot.jinjava.el.ext.FilterSpec; -import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy; -import com.hubspot.jinjava.interpret.DeferredValueException; -import com.hubspot.jinjava.interpret.JinjavaInterpreter; -import com.hubspot.jinjava.lib.filter.Filter; -import com.hubspot.jinjava.objects.SafeString; -import com.hubspot.jinjava.util.EagerExpressionResolver; -import de.odysseus.el.tree.Bindings; -import de.odysseus.el.tree.impl.ast.AstNode; -import de.odysseus.el.tree.impl.ast.AstParameters; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import javax.el.ELContext; -import javax.el.ELException; - -/** - * Eager variant of AstFilterChain that supports deferred value resolution. - * When a deferred value is encountered during filter chain evaluation, - * this class reconstructs the partially resolved expression. - */ -public class EagerAstFilterChain extends AstFilterChain implements EvalResultHolder { - - protected Object evalResult; - protected boolean hasEvalResult; - protected final EvalResultHolder inputHolder; - protected final List paramHolders; - - public EagerAstFilterChain(AstNode input, List filterSpecs) { - super( - (AstNode) EagerAstNodeDecorator.getAsEvalResultHolder(input), - wrapFilterSpecs(filterSpecs) - ); - this.inputHolder = (EvalResultHolder) this.input; - this.paramHolders = extractParamHolders(this.filterSpecs); - } - - private static List wrapFilterSpecs(List specs) { - List wrapped = new ArrayList<>(specs.size()); - for (FilterSpec spec : specs) { - AstParameters params = spec.getParams(); - if (params != null) { - params = (AstParameters) EagerAstNodeDecorator.getAsEvalResultHolder(params); - } - wrapped.add(new FilterSpec(spec.getName(), params)); - } - return wrapped; - } - - private static List extractParamHolders(List specs) { - List holders = new ArrayList<>(); - for (FilterSpec spec : specs) { - if (spec.getParams() instanceof EvalResultHolder) { - holders.add((EvalResultHolder) spec.getParams()); - } - } - return holders; - } - - @Override - public Object eval(Bindings bindings, ELContext context) { - try { - setEvalResult(evalFilterChain(bindings, context)); - return checkEvalResultSize(context); - } catch (DeferredValueException | ELException originalException) { - DeferredParsingException e = EvalResultHolder.convertToDeferredParsingException( - originalException - ); - throw new DeferredParsingException( - this, - getPartiallyResolved( - bindings, - context, - e, - IdentifierPreservationStrategy.PRESERVING - ), - IdentifierPreservationStrategy.PRESERVING - ); - } - } - - /** - * Evaluates the filter chain, keeping track of which filters have been applied - * to support partial resolution when deferred values are encountered. - */ - protected Object evalFilterChain(Bindings bindings, ELContext context) { - JinjavaInterpreter interpreter = getInterpreter(context); - - if (interpreter.getContext().isValidationMode()) { - return ""; - } - - Object value = input.eval(bindings, context); - - for (FilterSpec spec : filterSpecs) { - Filter filter = interpreter.getContext().getFilter(spec.getName()); - if (filter == null) { - continue; - } - - Object[] args = evaluateFilterArgs(spec, bindings, context); - Map kwargs = extractNamedParams(args); - Object[] positionalArgs = extractPositionalArgs(args); - - boolean wasSafeString = value instanceof SafeString; - if (wasSafeString) { - value = value.toString(); - } - - try { - value = filter.filter(value, interpreter, positionalArgs, kwargs); - } catch (ELException e) { - throw e; - } catch (DeferredValueException e) { - throw e; - } catch (RuntimeException e) { - throw new ELException( - String.format("Error in filter '%s': %s", spec.getName(), e.getMessage()), - e - ); - } - - if (wasSafeString && filter.preserveSafeString() && value instanceof String) { - value = new SafeString((String) value); - } - } - - return value; - } - - private Map extractNamedParams(Object[] args) { - java.util.Map kwargs = new java.util.LinkedHashMap<>(); - for (Object arg : args) { - if (arg instanceof com.hubspot.jinjava.el.ext.NamedParameter) { - com.hubspot.jinjava.el.ext.NamedParameter namedParam = - (com.hubspot.jinjava.el.ext.NamedParameter) arg; - kwargs.put(namedParam.getName(), namedParam.getValue()); - } - } - return kwargs; - } - - private Object[] extractPositionalArgs(Object[] args) { - java.util.List positional = new java.util.ArrayList<>(); - for (Object arg : args) { - if (!(arg instanceof com.hubspot.jinjava.el.ext.NamedParameter)) { - positional.add(arg); - } - } - return positional.toArray(); - } - - @Override - public Object getEvalResult() { - return evalResult; - } - - @Override - public void setEvalResult(Object evalResult) { - this.evalResult = evalResult; - hasEvalResult = true; - } - - @Override - public boolean hasEvalResult() { - return hasEvalResult; - } - - @Override - public String getPartiallyResolved( - Bindings bindings, - ELContext context, - DeferredParsingException deferredParsingException, - IdentifierPreservationStrategy identifierPreservationStrategy - ) { - StringBuilder sb = new StringBuilder(); - - String inputResult = EvalResultHolder.reconstructNode( - bindings, - context, - inputHolder, - deferredParsingException, - identifierPreservationStrategy - ); - sb.append(inputResult); - - for (FilterSpec spec : filterSpecs) { - sb.append('|').append(spec.getName()); - AstParameters params = spec.getParams(); - if (params != null && params.getCardinality() > 0) { - sb.append('('); - for (int i = 0; i < params.getCardinality(); i++) { - if (i > 0) { - sb.append(", "); - } - AstNode paramNode = params.getChild(i); - if (paramNode instanceof EvalResultHolder) { - String paramResult = EvalResultHolder.reconstructNode( - bindings, - context, - (EvalResultHolder) paramNode, - deferredParsingException, - identifierPreservationStrategy - ); - sb.append(paramResult); - } else { - try { - Object paramValue = paramNode.eval(bindings, context); - sb.append(EagerExpressionResolver.getValueAsJinjavaStringSafe(paramValue)); - } catch (DeferredValueException e) { - sb.append(paramNode.toString()); - } - } - } - sb.append(')'); - } - } - - return sb.toString(); - } -} diff --git a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerExtendedParser.java b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerExtendedParser.java index bb171f1cd..29f53e2b7 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerExtendedParser.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/eager/EagerExtendedParser.java @@ -6,14 +6,12 @@ import com.hubspot.jinjava.el.ext.AbsOperator; import com.hubspot.jinjava.el.ext.AstDict; -import com.hubspot.jinjava.el.ext.AstFilterChain; import com.hubspot.jinjava.el.ext.AstList; import com.hubspot.jinjava.el.ext.AstRangeBracket; import com.hubspot.jinjava.el.ext.AstTuple; import com.hubspot.jinjava.el.ext.CollectionMembershipOperator; import com.hubspot.jinjava.el.ext.CollectionNonMembershipOperator; import com.hubspot.jinjava.el.ext.ExtendedParser; -import com.hubspot.jinjava.el.ext.FilterSpec; import com.hubspot.jinjava.el.ext.NamedParameterOperator; import com.hubspot.jinjava.el.ext.PowerOfOperator; import com.hubspot.jinjava.el.ext.StringConcatOperator; @@ -202,10 +200,7 @@ protected AstParameters createAstParameters(List nodes) { } @Override - protected AstFilterChain createAstFilterChain( - AstNode input, - List filterSpecs - ) { - return new EagerAstFilterChain(input, filterSpecs); + protected boolean shouldUseFilterChainOptimization() { + return false; } } diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java index ba5ef053c..e720a02bc 100644 --- a/src/test/java/com/hubspot/jinjava/EagerTest.java +++ b/src/test/java/com/hubspot/jinjava/EagerTest.java @@ -307,7 +307,8 @@ public void itPreservesForTag() { @Test public void itPreservesFilters() { String output = interpreter.render("{{ deferred|capitalize }}"); - assertThat(output).isEqualTo("{{ deferred|capitalize }}"); + assertThat(output) + .isEqualTo("{{ filter:capitalize.filter(deferred, ____int3rpr3t3r____) }}"); assertThat(interpreter.getErrors()).isEmpty(); localContext.put("deferred", "foo"); assertThat(interpreter.render(output)).isEqualTo("Foo"); @@ -316,14 +317,18 @@ public void itPreservesFilters() { @Test public void itPreservesFunctions() { String output = interpreter.render("{{ deferred|datetimeformat('%B %e, %Y') }}"); - assertThat(output).isEqualTo("{{ deferred|datetimeformat('%B %e, %Y') }}"); + assertThat(output) + .isEqualTo( + "{{ filter:datetimeformat.filter(deferred, ____int3rpr3t3r____, '%B %e, %Y') }}" + ); assertThat(interpreter.getErrors()).isEmpty(); } @Test public void itPreservesRandomness() { String output = interpreter.render("{{ [1, 2, 3]|shuffle }}"); - assertThat(output).isEqualTo("{{ [1, 2, 3]|shuffle }}"); + assertThat(output) + .isEqualTo("{{ filter:shuffle.filter([1, 2, 3], ____int3rpr3t3r____) }}"); assertThat(interpreter.getErrors()).isEmpty(); } 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 0f869bb24..7a24f9e5b 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 @@ -231,7 +231,8 @@ public void itPreservesUnresolvable() { interpreter.resolveELExpression("foo_object.deferred|upper", -1); fail("Should throw DeferredParsingException"); } catch (DeferredParsingException e) { - assertThat(e.getDeferredEvalResult()).isEqualTo("foo_object.deferred|upper"); + assertThat(e.getDeferredEvalResult()) + .isEqualTo("filter:upper.filter(foo_object.deferred, ____int3rpr3t3r____)"); } } } diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java index b00ae916e..031f7183d 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java @@ -423,7 +423,7 @@ public void itCorrectlySetsAliasedPathForSecondPass() { assertThat(firstPassResult) .isEqualTo( "{% set deferred_import_resource_path = 'import-macro.jinja' %}{% macro m.print_path_macro(var) %}\n" + - "{{ var|print_path }}\n" + + "{{ filter:print_path.filter(var, ____int3rpr3t3r____) }}\n" + "{{ var }}\n" + "{% endmacro %}{% set deferred_import_resource_path = null %}{{ m.print_path_macro(foo) }}" ); @@ -442,7 +442,7 @@ public void itCorrectlySetsPathForSecondPass() { assertThat(firstPassResult) .isEqualTo( "{% set deferred_import_resource_path = 'import-macro.jinja' %}{% macro print_path_macro(var) %}\n" + - "{{ var|print_path }}\n" + + "{{ filter:print_path.filter(var, ____int3rpr3t3r____) }}\n" + "{{ var }}\n" + "{% endmacro %}{% set deferred_import_resource_path = null %}{{ print_path_macro(foo) }}" ); @@ -460,9 +460,9 @@ public void itCorrectlySetsNestedPathsForSecondPass() { ); assertThat(firstPassResult) .isEqualTo( - "{% set deferred_import_resource_path = 'double-import-macro.jinja' %}{% macro print_path_macro2(var) %}{{ var|print_path }}\n" + + "{% set deferred_import_resource_path = 'double-import-macro.jinja' %}{% macro print_path_macro2(var) %}{{ filter:print_path.filter(var, ____int3rpr3t3r____) }}\n" + "{% set deferred_import_resource_path = 'import-macro.jinja' %}{% macro print_path_macro(var) %}\n" + - "{{ var|print_path }}\n" + + "{{ filter:print_path.filter(var, ____int3rpr3t3r____) }}\n" + "{{ var }}\n" + "{% endmacro %}{% set deferred_import_resource_path = 'double-import-macro.jinja' %}{{ print_path_macro(var) }}{% endmacro %}{% set deferred_import_resource_path = null %}{{ print_path_macro2(foo) }}" ); diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagTest.java index 3acf7b665..d86055ff2 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagTest.java @@ -141,7 +141,10 @@ public void itDefersBlockWithFilter() { "{% set foo | int |add(deferred) %}{{ 1 + 1 }}{% endset %}{{ foo }}"; final String result = interpreter.render(template); - assertThat(result).isEqualTo("{% set foo = '2'|int|add(deferred) %}{{ foo }}"); + assertThat(result) + .isEqualTo( + "{% set foo = filter:add.filter(2, ____int3rpr3t3r____, deferred) %}{{ foo }}" + ); assertThat( context .getDeferredTokens() @@ -157,7 +160,7 @@ public void itDefersBlockWithFilter() { .flatMap(deferredToken -> deferredToken.getUsedDeferredWords().stream()) .collect(Collectors.toSet()) ) - .containsExactlyInAnyOrder("deferred", "foo", "add", "int"); + .containsExactlyInAnyOrder("deferred", "foo", "add.filter"); assertThat( context .getDeferredTokens() @@ -165,7 +168,7 @@ public void itDefersBlockWithFilter() { .flatMap(deferredToken -> deferredToken.getUsedDeferredBases().stream()) .collect(Collectors.toSet()) ) - .containsExactlyInAnyOrder("deferred", "foo", "add", "int"); + .containsExactlyInAnyOrder("deferred", "foo", "add"); } @Test @@ -176,7 +179,7 @@ public void itDefersDeferredBlockWithDeferredFilter() { assertThat(result) .isEqualTo( - "{% set foo %}{{ 1 + deferred }}{% endset %}{% set foo = foo|add(deferred|int) %}{{ foo }}" + "{% set foo %}{{ 1 + deferred }}{% endset %}{% set foo = filter:add.filter(foo, ____int3rpr3t3r____, filter:int.filter(deferred, ____int3rpr3t3r____)) %}{{ foo }}" ); context.remove("foo"); context.put("deferred", 2); @@ -200,7 +203,7 @@ public void itDefersInDeferredExecutionModeWithFilter() { assertThat(result) .isEqualTo( - "{% set foo %}1{% endset %}{% set foo = '1'|int|add(deferred) %}{{ foo }}" + "{% set foo %}1{% endset %}{% set foo = filter:add.filter(1, ____int3rpr3t3r____, deferred) %}{{ foo }}" ); assertThat( context @@ -217,7 +220,7 @@ public void itDefersInDeferredExecutionModeWithFilter() { .flatMap(deferredToken -> deferredToken.getUsedDeferredWords().stream()) .collect(Collectors.toSet()) ) - .containsExactlyInAnyOrder("deferred", "foo", "add", "int"); + .containsExactlyInAnyOrder("deferred", "foo", "add.filter"); assertThat( context .getDeferredTokens() @@ -225,7 +228,7 @@ public void itDefersInDeferredExecutionModeWithFilter() { .flatMap(deferredToken -> deferredToken.getUsedDeferredBases().stream()) .collect(Collectors.toSet()) ) - .containsExactlyInAnyOrder("deferred", "foo", "add", "int"); + .containsExactlyInAnyOrder("deferred", "foo", "add"); context.remove("foo"); context.put("deferred", 2); context.setDeferredExecutionMode(false); diff --git a/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java b/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java index 65855fdf9..2e1d962cf 100644 --- a/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java +++ b/src/test/java/com/hubspot/jinjava/util/EagerExpressionResolverTest.java @@ -363,7 +363,7 @@ public void itHandlesEscapedSlashBeforeQuoteProperly() { "deferred|replace('\\\\', '.')" ); assertThat(eagerExpressionResult.getDeferredWords()) - .containsExactlyInAnyOrder("deferred", "replace"); + .containsExactlyInAnyOrder("deferred", "replace.filter"); } @Test @@ -750,7 +750,7 @@ public void itHandlesDeferredMethod() { assertThat(eagerResolveExpression("deferred.append(foo)").toString()) .isEqualTo("deferred.append('foo')"); assertThat(eagerResolveExpression("deferred[1 + 1] | length").toString()) - .isEqualTo("deferred[2]|length"); + .isEqualTo("filter:length.filter(deferred[2], ____int3rpr3t3r____)"); } @Test @@ -817,7 +817,7 @@ public void itDoesNotSplitJsonInArrayResolvedExpression() { @Test public void itHandlesRandom() { assertThat(eagerResolveExpression("range(1)|random").toString()) - .isEqualTo("range(1)|random"); + .isEqualTo("filter:random.filter(range(1), ____int3rpr3t3r____)"); } @Test diff --git a/src/test/resources/eager/defers-macro-in-expression/test.expected.jinja b/src/test/resources/eager/defers-macro-in-expression/test.expected.jinja index e6a659701..7443a94eb 100644 --- a/src/test/resources/eager/defers-macro-in-expression/test.expected.jinja +++ b/src/test/resources/eager/defers-macro-in-expression/test.expected.jinja @@ -1,10 +1,10 @@ 2 {% macro plus(foo, add) %}\ -{{ foo + (add|int) }}\ +{{ foo + (filter:int.filter(add, ____int3rpr3t3r____)) }}\ {% endmacro %}\ {{ plus(deferred, 1.1) }}\ {% set deferred = deferred + 2 %} {% macro plus(foo, add) %}\ -{{ foo + (add|int) }}\ +{{ foo + (filter:int.filter(add, ____int3rpr3t3r____)) }}\ {% endmacro %}\ {{ plus(deferred, 3.1) }} \ No newline at end of file diff --git a/src/test/resources/eager/defers-macro-in-for/test.expected.jinja b/src/test/resources/eager/defers-macro-in-for/test.expected.jinja index 6b183ee39..3311b8714 100644 --- a/src/test/resources/eager/defers-macro-in-for/test.expected.jinja +++ b/src/test/resources/eager/defers-macro-in-for/test.expected.jinja @@ -3,6 +3,6 @@ {% do my_list.append(num) %}\ {{ my_list }}\ {% endmacro %}\ -{% for item in macro_append(deferred)|split(',', 2) %} +{% for item in filter:split.filter(macro_append(deferred), ____int3rpr3t3r____, ',', 2) %} {{ item }} {% endfor %} diff --git a/src/test/resources/eager/defers-macro-in-if/test.expected.jinja b/src/test/resources/eager/defers-macro-in-if/test.expected.jinja index d73fe7c9f..67f28d9e4 100644 --- a/src/test/resources/eager/defers-macro-in-if/test.expected.jinja +++ b/src/test/resources/eager/defers-macro-in-if/test.expected.jinja @@ -3,6 +3,6 @@ {% do my_list.append(num) %}\ {{ my_list }}\ {% endmacro %}\ -{% if [] == macro_append(deferred)|split(',', 2) %} +{% if [] == filter:split.filter(macro_append(deferred), ____int3rpr3t3r____, ',', 2) %} {{ my_list }} {% endif %} diff --git a/src/test/resources/eager/does-not-override-import-modification-in-for/test.expected.jinja b/src/test/resources/eager/does-not-override-import-modification-in-for/test.expected.jinja index de5c88d02..0e1e4a8a2 100644 --- a/src/test/resources/eager/does-not-override-import-modification-in-for/test.expected.jinja +++ b/src/test/resources/eager/does-not-override-import-modification-in-for/test.expected.jinja @@ -11,7 +11,7 @@ {% endif %} -{% set foo = [foo, 'b']|join('') %}\ +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ {% do __temp_meta_import_alias_3016318__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016318__.update({'foo': foo,'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ @@ -25,12 +25,12 @@ {% for __ignored__ in [0] %}\ {% if deferred %} -{% set foo = [foo, 'a']|join('') %}\ +{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}\ {% do __temp_meta_import_alias_3016319__.update({'foo': foo}) %} {% endif %} -{% set foo = [foo, 'b']|join('') %}\ +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ {% do __temp_meta_import_alias_3016319__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016319__.update({'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ @@ -45,12 +45,12 @@ {% for __ignored__ in [0] %}\ {% if deferred %} -{% set foo = [foo, 'a']|join('') %}\ +{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}\ {% do __temp_meta_import_alias_3016318__.update({'foo': foo}) %} {% endif %} -{% set foo = [foo, 'b']|join('') %}\ +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ {% do __temp_meta_import_alias_3016318__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016318__.update({'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ @@ -64,12 +64,12 @@ {% for __ignored__ in [0] %}\ {% if deferred %} -{% set foo = [foo, 'a']|join('') %}\ +{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}\ {% do __temp_meta_import_alias_3016319__.update({'foo': foo}) %} {% endif %} -{% set foo = [foo, 'b']|join('') %}\ +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ {% do __temp_meta_import_alias_3016319__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016319__.update({'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ diff --git a/src/test/resources/eager/fully-defers-filtered-macro/test.expected.jinja b/src/test/resources/eager/fully-defers-filtered-macro/test.expected.jinja index e2268dbb1..4938403bd 100644 --- a/src/test/resources/eager/fully-defers-filtered-macro/test.expected.jinja +++ b/src/test/resources/eager/fully-defers-filtered-macro/test.expected.jinja @@ -1,5 +1,5 @@ {% macro flashy(foo) %}\ -{{ foo|upper }} +{{ filter:upper.filter(foo, ____int3rpr3t3r____) }} A flashy {{ deferred }}\ .{% endmacro %}\ {% set __macro_flashy_1625622909_temp_variable_0__ %}\ @@ -12,4 +12,4 @@ BAR {% set __macro_silly_2092874071_temp_variable_0__ %}\ A silly {{ deferred }}\ .{% endset %}\ -{{ __macro_silly_2092874071_temp_variable_0__|upper }} \ No newline at end of file +{{ filter:upper.filter(__macro_silly_2092874071_temp_variable_0__, ____int3rpr3t3r____) }} \ No newline at end of file diff --git a/src/test/resources/eager/handles-deferred-cycle-as/test.expected.jinja b/src/test/resources/eager/handles-deferred-cycle-as/test.expected.jinja index 5b52e0fd9..5893e2ee2 100644 --- a/src/test/resources/eager/handles-deferred-cycle-as/test.expected.jinja +++ b/src/test/resources/eager/handles-deferred-cycle-as/test.expected.jinja @@ -1,19 +1,19 @@ {% for __ignored__ in [0] %} {% set c = [1, deferred] %} {% if exptest:iterable.evaluate(c, ____int3rpr3t3r____) %}\ -{{ c[0 % c|length] }}\ +{{ c[0 % filter:length.filter(c, ____int3rpr3t3r____)] }}\ {% else %}\ {{ c }}\ {% endif %} {% set c = [2, deferred] %} {% if exptest:iterable.evaluate(c, ____int3rpr3t3r____) %}\ -{{ c[1 % c|length] }}\ +{{ c[1 % filter:length.filter(c, ____int3rpr3t3r____)] }}\ {% else %}\ {{ c }}\ {% endif %} {% set c = [3, deferred] %} {% if exptest:iterable.evaluate(c, ____int3rpr3t3r____) %}\ -{{ c[2 % c|length] }}\ +{{ c[2 % filter:length.filter(c, ____int3rpr3t3r____)] }}\ {% else %}\ {{ c }}\ {% endif %}\ diff --git a/src/test/resources/eager/handles-deferred-for-loop-var-from-macro/test.expected.jinja b/src/test/resources/eager/handles-deferred-for-loop-var-from-macro/test.expected.jinja index 210f8f1b2..54d9fc69a 100644 --- a/src/test/resources/eager/handles-deferred-for-loop-var-from-macro/test.expected.jinja +++ b/src/test/resources/eager/handles-deferred-for-loop-var-from-macro/test.expected.jinja @@ -3,14 +3,14 @@ {% for __ignored__ in [0] %} {% set __macro_doIt_1327224118_temp_variable_0__ %} -{{ deferred ~ '{"a":"a"}' }} +{{ deferred ~ '{\"a\":\"a\"}' }} {% endset %}\ -{{ __macro_doIt_1327224118_temp_variable_0__|upper }} +{{ filter:upper.filter(__macro_doIt_1327224118_temp_variable_0__, ____int3rpr3t3r____) }} {% set __macro_doIt_1327224118_temp_variable_1__ %} -{{ deferred ~ '{"b":"b"}' }} +{{ deferred ~ '{\"b\":\"b\"}' }} {% endset %}\ -{{ __macro_doIt_1327224118_temp_variable_1__|upper }} +{{ filter:upper.filter(__macro_doIt_1327224118_temp_variable_1__, ____int3rpr3t3r____) }} {% endfor %} {% endset %}\ -{{ __macro_getData_357124436_temp_variable_0__|upper }} +{{ filter:upper.filter(__macro_getData_357124436_temp_variable_0__, ____int3rpr3t3r____) }} diff --git a/src/test/resources/eager/handles-double-import-modification/test.expected.jinja b/src/test/resources/eager/handles-double-import-modification/test.expected.jinja index cd174801f..66acdde36 100644 --- a/src/test/resources/eager/handles-double-import-modification/test.expected.jinja +++ b/src/test/resources/eager/handles-double-import-modification/test.expected.jinja @@ -9,7 +9,7 @@ {% endif %} -{% set foo = [foo, 'b']|join('') %}\ +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ {% do __temp_meta_import_alias_3016318__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016318__.update({'foo': foo,'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ @@ -28,7 +28,7 @@ {% endif %} -{% set foo = [foo, 'b']|join('') %}\ +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}\ {% do __temp_meta_import_alias_3016319__.update({'foo': foo}) %} {% do __temp_meta_import_alias_3016319__.update({'foo': foo,'import_resource_path': 'eager/supplements/deferred-modification.jinja'}) %}\ {% endfor %}\ diff --git a/src/test/resources/eager/handles-same-name-import-var/test.expected.jinja b/src/test/resources/eager/handles-same-name-import-var/test.expected.jinja index 4cec80792..66cfa99f0 100644 --- a/src/test/resources/eager/handles-same-name-import-var/test.expected.jinja +++ b/src/test/resources/eager/handles-same-name-import-var/test.expected.jinja @@ -35,5 +35,5 @@ {% set my_var = __temp_meta_import_alias_1059697132__ %}\ {% set current_path,__temp_meta_current_path_944750549__ = __temp_meta_current_path_944750549__,null %}\ {% enddo %} -{{ my_var|dictsort(false, 'key') }} +{{ filter:dictsort.filter(my_var, ____int3rpr3t3r____, false, 'key') }} {% endif %} diff --git a/src/test/resources/eager/reconstructs-block-set-variables-in-for-loop/test.expected.jinja b/src/test/resources/eager/reconstructs-block-set-variables-in-for-loop/test.expected.jinja index 40fb3b930..d0152a7f9 100644 --- a/src/test/resources/eager/reconstructs-block-set-variables-in-for-loop/test.expected.jinja +++ b/src/test/resources/eager/reconstructs-block-set-variables-in-for-loop/test.expected.jinja @@ -2,5 +2,5 @@ {% set __macro_foo_97643642_temp_variable_0__ %} {{ deferred }} {% endset %}\ -{{ __macro_foo_97643642_temp_variable_0__|int + 3 }} +{{ filter:int.filter(__macro_foo_97643642_temp_variable_0__, ____int3rpr3t3r____) + 3 }} {% endfor %} diff --git a/src/test/resources/eager/reconstructs-fromed-macro/test.expected.jinja b/src/test/resources/eager/reconstructs-fromed-macro/test.expected.jinja index 3aa5fc5c2..c7213b008 100644 --- a/src/test/resources/eager/reconstructs-fromed-macro/test.expected.jinja +++ b/src/test/resources/eager/reconstructs-fromed-macro/test.expected.jinja @@ -1,6 +1,6 @@ {% set deferred_import_resource_path = 'eager/reconstructs-fromed-macro/has-macro.jinja' %}\ {% macro to_upper(param) %} - {{ param|upper }} + {{ filter:upper.filter(param, ____int3rpr3t3r____) }} {% endmacro %}\ {% set deferred_import_resource_path = null %}\ {{ to_upper(deferred) }} \ No newline at end of file diff --git a/src/test/resources/eager/uses-unique-macro-names/test.expected.jinja b/src/test/resources/eager/uses-unique-macro-names/test.expected.jinja index a1089d9bc..54c40b809 100644 --- a/src/test/resources/eager/uses-unique-macro-names/test.expected.jinja +++ b/src/test/resources/eager/uses-unique-macro-names/test.expected.jinja @@ -3,13 +3,13 @@ {% set __macro_foo_97643642_temp_variable_0__ %} Goodbye {{ myname }} {% endset %}\ -{% set a = __macro_foo_97643642_temp_variable_0__|upper %} +{% set a = filter:upper.filter(__macro_foo_97643642_temp_variable_0__, ____int3rpr3t3r____) %} {% do %}\ {% set __temp_meta_current_path_203114534__,current_path = current_path,'eager/uses-unique-macro-names/macro-with-filter.jinja' %} {% set __macro_foo_1717337666_temp_variable_0__ %}\ Hello {{ myname }}\ {% endset %}\ -{% set b = __macro_foo_1717337666_temp_variable_0__|upper %} +{% set b = filter:upper.filter(__macro_foo_1717337666_temp_variable_0__, ____int3rpr3t3r____) %} {% set current_path,__temp_meta_current_path_203114534__ = __temp_meta_current_path_203114534__,null %}\ {% enddo %} {{ a }} From 63ff83c54d13b52b83207985489c2f8364e99361 Mon Sep 17 00:00:00 2001 From: Libo Song Date: Mon, 12 Jan 2026 11:14:41 -0500 Subject: [PATCH 3/8] Add performance test for filter chain optimization - Add AstFilterChainPerformanceTest to verify optimization performance - Tests verify both approaches produce identical results - Includes manual benchmark test (ignored by default) for detailed comparison - Automated test verifies optimized version is faster than unoptimized (at least 5% improvement) - Uses nested property access (content.text) to better simulate real-world usage --- .../el/ext/AstFilterChainPerformanceTest.java | 244 ++++++++++++++++++ 1 file changed, 244 insertions(+) create mode 100644 src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainPerformanceTest.java diff --git a/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainPerformanceTest.java b/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainPerformanceTest.java new file mode 100644 index 000000000..5ed90f2fa --- /dev/null +++ b/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainPerformanceTest.java @@ -0,0 +1,244 @@ +package com.hubspot.jinjava.el.ext; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.hubspot.jinjava.Jinjava; +import com.hubspot.jinjava.JinjavaConfig; +import java.util.HashMap; +import java.util.Map; +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Test; + +/** + * Performance test to verify that the optimized filter chain performs better than + * the nested method approach. + * + * Run with: mvn test -Dtest=AstFilterChainPerformanceTest + * Or run the main() method directly for more detailed output. + */ +public class AstFilterChainPerformanceTest { + + private Jinjava jinjavaOptimized; + private Jinjava jinjavaUnoptimized; + private Map context; + + @Before + public void setup() { + jinjavaOptimized = + new Jinjava( + JinjavaConfig.newBuilder().withEnableFilterChainOptimization(true).build() + ); + + jinjavaUnoptimized = + new Jinjava( + JinjavaConfig.newBuilder().withEnableFilterChainOptimization(false).build() + ); + + context = new HashMap<>(); + context.put("name", " Hello World "); + context.put("text", "the quick brown fox jumps over the lazy dog"); + context.put("number", 12345); + context.put("items", new String[] { "apple", "banana", "cherry" }); + context.put("content", Map.of("text", "the quick brown fox jumps over the lazy dog")); + } + + public static void main(String[] args) { + AstFilterChainPerformanceTest test = new AstFilterChainPerformanceTest(); + test.setup(); + test.runPerformanceComparison(); + } + + /** + * Run this test manually to see detailed performance comparison. + * Use main() method or run with -Dtest=AstFilterChainPerformanceTest#runPerformanceComparison + */ + @Test + @Ignore("Manual performance test - run explicitly when needed") + public void runPerformanceComparison() { + int warmupIterations = 10000; + int testIterations = 100000; + + System.out.println("=== Filter Chain Performance Test ===\n"); + System.out.println("Warming up..."); + + // Warmup + runFilterTests(jinjavaOptimized, warmupIterations, false); + runFilterTests(jinjavaUnoptimized, warmupIterations, false); + + System.out.println( + "Running performance tests with " + testIterations + " iterations each\n" + ); + + // Single filter + comparePerformance("Single filter: {{ name|trim }}", testIterations); + + // Two chained filters + comparePerformance("Two filters: {{ name|trim|lower }}", testIterations); + + // Three chained filters + comparePerformance("Three filters: {{ name|trim|lower|capitalize }}", testIterations); + + // Five chained filters + comparePerformance( + "Five filters: {{ text|upper|replace('THE', 'a')|trim|lower|title }}", + testIterations + ); + + // Filter with arguments + comparePerformance( + "Filters with args: {{ text|truncate(20)|upper }}", + testIterations + ); + + // Multiple filter chains in same template + comparePerformance( + "Multiple chains: {{ name|trim|lower }} and {{ text|upper|truncate(10) }}", + testIterations + ); + } + + private void comparePerformance(String description, int iterations) { + String template = description.substring(description.indexOf("{{")); + if (description.contains(":")) { + template = description.substring(description.indexOf(":") + 2); + } + + System.out.println(description); + + // Run optimized + long optimizedTime = timeExecution(jinjavaOptimized, template, iterations); + + // Run unoptimized + long unoptimizedTime = timeExecution(jinjavaUnoptimized, template, iterations); + + double speedup = (double) unoptimizedTime / optimizedTime; + System.out.printf( + " Optimized: %d ms, Unoptimized: %d ms, Speedup: %.2fx%n%n", + optimizedTime, + unoptimizedTime, + speedup + ); + } + + private long timeExecution(Jinjava jinjava, String template, int iterations) { + long startTime = System.currentTimeMillis(); + for (int i = 0; i < iterations; i++) { + jinjava.render(template, context); + } + return System.currentTimeMillis() - startTime; + } + + private void runFilterTests(Jinjava jinjava, int iterations, boolean print) { + String[] templates = { + "{{ name|trim }}", + "{{ name|trim|lower }}", + "{{ name|trim|lower|capitalize }}", + "{{ text|upper|replace('THE', 'a')|trim|lower|title }}", + "{{ text|truncate(20)|upper }}", + }; + + for (String template : templates) { + for (int i = 0; i < iterations; i++) { + jinjava.render(template, context); + } + } + } + + @Test + public void itProducesSameResultsWithAndWithoutOptimization() { + String[] templates = { + "{{ name|trim }}", + "{{ name|trim|lower }}", + "{{ name|trim|lower|capitalize }}", + "{{ text|upper|replace('THE', 'a')|trim|lower|title }}", + "{{ text|truncate(20)|upper }}", + "{{ name|trim|lower }} and {{ text|upper|truncate(10) }}", + "{{ items|join(', ')|upper }}", + "{{ number|string|length }}", + }; + + for (String template : templates) { + String optimizedResult = jinjavaOptimized.render(template, context); + String unoptimizedResult = jinjavaUnoptimized.render(template, context); + assertThat(optimizedResult) + .as("Template: " + template) + .isEqualTo(unoptimizedResult); + } + } + + @Test + public void itHandlesSingleFilterWithOptimization() { + String result = jinjavaOptimized.render("{{ name|trim }}", context); + assertThat(result).isEqualTo("Hello World"); + } + + @Test + public void itHandlesChainedFiltersWithOptimization() { + String result = jinjavaOptimized.render("{{ name|trim|lower }}", context); + assertThat(result).isEqualTo("hello world"); + } + + @Test + public void itHandlesFiltersWithArgumentsWithOptimization() { + String result = jinjavaOptimized.render("{{ text|truncate(20)|upper }}", context); + assertThat(result).isNotEmpty(); + assertThat(result).isUpperCase(); + } + + @Test + public void itHandlesComplexFilterChainWithOptimization() { + String result = jinjavaOptimized.render( + "{{ text|upper|replace('THE', 'a')|trim|lower|capitalize }}", + context + ); + assertThat(result).isNotEmpty(); + } + + /** + * This test verifies that the optimized version is faster than the unoptimized version. + * The optimization should provide a measurable speedup for chained filters. + */ + @Test + public void optimizedVersionShouldBeFaster() { + int warmupIterations = 100; + int testIterations = 1000; + String template = "{{ content.text|upper|replace('THE', 'a')|trim|lower|title }}"; + + // Warmup both to ensure JIT compilation + for (int i = 0; i < warmupIterations; i++) { + jinjavaOptimized.render(template, context); + jinjavaUnoptimized.render(template, context); + } + + // Run multiple rounds to get more stable results + long totalOptimizedTime = 0; + long totalUnoptimizedTime = 0; + int rounds = 3; + + for (int round = 0; round < rounds; round++) { + totalUnoptimizedTime += timeExecution(jinjavaUnoptimized, template, testIterations); + totalOptimizedTime += timeExecution(jinjavaOptimized, template, testIterations); + } + + long avgUnoptimizedTime = totalUnoptimizedTime / rounds; + long avgOptimizedTime = totalOptimizedTime / rounds; + + System.out.printf( + "Performance test: Optimized=%d ms, Unoptimized=%d ms, Speedup=%.2fx%n", + avgOptimizedTime, + avgUnoptimizedTime, + (double) avgUnoptimizedTime / avgOptimizedTime + ); + + // The optimized version should be faster (allow 10% margin for system variance) + // If optimized takes more than 90% of unoptimized time, fail the test + assertThat(avgOptimizedTime) + .as( + "Optimized (%d ms) should be faster than unoptimized (%d ms)", + avgOptimizedTime, + avgUnoptimizedTime + ) + .isLessThan((long) (avgUnoptimizedTime * 0.95)); + } +} From 95094de7f74e773c373ca1a67d139225ede69f2e Mon Sep 17 00:00:00 2001 From: Libo Song Date: Mon, 12 Jan 2026 11:30:53 -0500 Subject: [PATCH 4/8] Add documentation for filter chain optimization --- filter-chain-optimization.md | 102 +++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 filter-chain-optimization.md diff --git a/filter-chain-optimization.md b/filter-chain-optimization.md new file mode 100644 index 000000000..0de350dff --- /dev/null +++ b/filter-chain-optimization.md @@ -0,0 +1,102 @@ +# Filter Chain Optimization + +## Overview + +This branch introduces a performance optimization for chained filter expressions in Jinjava's expression language. The optimization is **configurable** and **disabled by default** for backward compatibility. + +## Problem + +Previously, chained filters like `input|trim|lower|length` were parsed as deeply nested AST method calls: + +``` +filter:length.filter(filter:lower.filter(filter:trim.filter(input, interpreter), interpreter), interpreter) +``` + +This resulted in: +- Multiple redundant filter lookups per AST node traversal +- Increased method invocation overhead +- Extra object wrapping/unwrapping between filters +- Unnecessary context operations + +## Solution + +The optimization introduces a new `AstFilterChain` AST node that represents the entire filter chain as a **single evaluation unit**: + +``` +input|trim|lower|length +``` + +Instead of nested method calls, the filter chain is evaluated in a single pass: +1. Look up each filter once +2. Directly invoke `filter.filter(...)` sequentially +3. Handle `SafeString` preservation inline +4. Handle disabled filters and errors + +## New Files + +| File | Description | +|------|-------------| +| `AstFilterChain.java` | AST node for optimized filter chain evaluation | +| `FilterSpec.java` | Data class holding filter name and parameters | +| `AstFilterChainPerformanceTest.java` | Performance tests verifying the optimization | + +## Modified Files + +| File | Changes | +|------|---------| +| `JinjavaConfig.java` | Added `enableFilterChainOptimization` config option | +| `ExtendedParser.java` | Added conditional logic to use optimization based on config | +| `EagerExtendedParser.java` | Override to always use old behavior (no optimization for eager mode) | + +## Configuration + +To enable the filter chain optimization: + +```java +JinjavaConfig config = JinjavaConfig.newBuilder() + .withEnableFilterChainOptimization(true) + .build(); + +Jinjava jinjava = new Jinjava(config); +``` + +### Behavior by Execution Mode + +| Mode | Config Enabled | Behavior | +|------|---------------|----------| +| Normal (non-eager) | `true` | Uses optimized `AstFilterChain` | +| Normal (non-eager) | `false` | Uses old nested method approach | +| Eager | `true` or `false` | Always uses old nested method approach | + +**Note:** The optimization is disabled for eager execution mode regardless of configuration, as eager mode requires the old behavior for proper deferred value handling. + +## Performance + +The optimization provides measurable performance improvements for chained filter expressions: + +- **Single filter**: Minimal improvement +- **2+ chained filters**: Noticeable improvement +- **5+ chained filters**: Significant improvement + +Run the performance test to see actual numbers: + +```bash +# Run automated performance test +mvn test -Dtest=AstFilterChainPerformanceTest + +# Run detailed benchmark (main method) +mvn exec:java -Dexec.mainClass="com.hubspot.jinjava.el.ext.AstFilterChainPerformanceTest" +``` + +## Commits + +1. **Chained filters optimization** - Initial implementation with `AstFilterChain` and `FilterSpec` +2. **Make filter chain optimization configurable and disable for eager mode** - Added config option and disabled for eager execution +3. **Add performance test for filter chain optimization** - Added tests to verify correctness and performance + +## Backward Compatibility + +- The optimization is **disabled by default** (`enableFilterChainOptimization = false`) +- Existing code works unchanged without any configuration +- Eager execution mode always uses the old behavior +- All existing tests pass without modification From 5877a0539396f78bb6da24c7170dd4dc5df20a91 Mon Sep 17 00:00:00 2001 From: Libo Song Date: Mon, 12 Jan 2026 15:18:25 -0500 Subject: [PATCH 5/8] Remove documentation files from PR Co-Authored-By: Claude Opus 4.5 --- filter-chain-optimization.md | 102 ----------------------------------- keep-undefined.md | 88 ------------------------------ 2 files changed, 190 deletions(-) delete mode 100644 filter-chain-optimization.md delete mode 100644 keep-undefined.md diff --git a/filter-chain-optimization.md b/filter-chain-optimization.md deleted file mode 100644 index 0de350dff..000000000 --- a/filter-chain-optimization.md +++ /dev/null @@ -1,102 +0,0 @@ -# Filter Chain Optimization - -## Overview - -This branch introduces a performance optimization for chained filter expressions in Jinjava's expression language. The optimization is **configurable** and **disabled by default** for backward compatibility. - -## Problem - -Previously, chained filters like `input|trim|lower|length` were parsed as deeply nested AST method calls: - -``` -filter:length.filter(filter:lower.filter(filter:trim.filter(input, interpreter), interpreter), interpreter) -``` - -This resulted in: -- Multiple redundant filter lookups per AST node traversal -- Increased method invocation overhead -- Extra object wrapping/unwrapping between filters -- Unnecessary context operations - -## Solution - -The optimization introduces a new `AstFilterChain` AST node that represents the entire filter chain as a **single evaluation unit**: - -``` -input|trim|lower|length -``` - -Instead of nested method calls, the filter chain is evaluated in a single pass: -1. Look up each filter once -2. Directly invoke `filter.filter(...)` sequentially -3. Handle `SafeString` preservation inline -4. Handle disabled filters and errors - -## New Files - -| File | Description | -|------|-------------| -| `AstFilterChain.java` | AST node for optimized filter chain evaluation | -| `FilterSpec.java` | Data class holding filter name and parameters | -| `AstFilterChainPerformanceTest.java` | Performance tests verifying the optimization | - -## Modified Files - -| File | Changes | -|------|---------| -| `JinjavaConfig.java` | Added `enableFilterChainOptimization` config option | -| `ExtendedParser.java` | Added conditional logic to use optimization based on config | -| `EagerExtendedParser.java` | Override to always use old behavior (no optimization for eager mode) | - -## Configuration - -To enable the filter chain optimization: - -```java -JinjavaConfig config = JinjavaConfig.newBuilder() - .withEnableFilterChainOptimization(true) - .build(); - -Jinjava jinjava = new Jinjava(config); -``` - -### Behavior by Execution Mode - -| Mode | Config Enabled | Behavior | -|------|---------------|----------| -| Normal (non-eager) | `true` | Uses optimized `AstFilterChain` | -| Normal (non-eager) | `false` | Uses old nested method approach | -| Eager | `true` or `false` | Always uses old nested method approach | - -**Note:** The optimization is disabled for eager execution mode regardless of configuration, as eager mode requires the old behavior for proper deferred value handling. - -## Performance - -The optimization provides measurable performance improvements for chained filter expressions: - -- **Single filter**: Minimal improvement -- **2+ chained filters**: Noticeable improvement -- **5+ chained filters**: Significant improvement - -Run the performance test to see actual numbers: - -```bash -# Run automated performance test -mvn test -Dtest=AstFilterChainPerformanceTest - -# Run detailed benchmark (main method) -mvn exec:java -Dexec.mainClass="com.hubspot.jinjava.el.ext.AstFilterChainPerformanceTest" -``` - -## Commits - -1. **Chained filters optimization** - Initial implementation with `AstFilterChain` and `FilterSpec` -2. **Make filter chain optimization configurable and disable for eager mode** - Added config option and disabled for eager execution -3. **Add performance test for filter chain optimization** - Added tests to verify correctness and performance - -## Backward Compatibility - -- The optimization is **disabled by default** (`enableFilterChainOptimization = false`) -- Existing code works unchanged without any configuration -- Eager execution mode always uses the old behavior -- All existing tests pass without modification diff --git a/keep-undefined.md b/keep-undefined.md deleted file mode 100644 index 044d97572..000000000 --- a/keep-undefined.md +++ /dev/null @@ -1,88 +0,0 @@ -# PreserveUnknownExecutionMode - -A Jinjava execution mode that preserves unknown/undefined variables as their original template syntax instead of rendering them as empty strings. This enables multi-pass rendering scenarios where templates are processed in stages with different variable contexts available at each stage. - -## Usage - -```java -JinjavaConfig config = JinjavaConfig - .newBuilder() - .withExecutionMode(PreserveUnknownExecutionMode.instance()) - .build(); -Jinjava jinjava = new Jinjava(config); - -String result = jinjava.render("Hello {{ name }}!", new HashMap<>()); -// Result: "Hello {{ name }}!" -``` - -## Behavior - -### Expressions - -| Template | Context | Output | -|----------|---------|--------| -| `{{ unknown }}` | `{}` | `{{ unknown }}` | -| `{{ name }}` | `{name: "World"}` | `World` | -| `{{ name \| upper }}` | `{}` | `{{ name \| upper }}` | -| `{{ obj.property }}` | `{}` | `{{ obj.property }}` | - -### Control Structures - -| Template | Context | Output | -|----------|---------|--------| -| `{% if item %}yes{% endif %}` | `{}` | `{% if item %}yes{% endif %}` | -| `{% if item %}yes{% endif %}` | `{item: true}` | `yes` | -| `{% if item %}yes{% else %}no{% endif %}` | `{}` | `{% if item %}yes{% else %}no{% endif %}` | -| `{% for x in items %}{{ x }}{% endfor %}` | `{}` | `{% for x in items %}{{ x }}{% endfor %}` | -| `{% for x in items %}{{ x }}{% endfor %}` | `{items: ["a","b"]}` | `ab` | - -### Set Tags - -Set tags are preserved in output while their right-hand side expressions are evaluated when possible: - -| Template | Output | -|----------|--------| -| `{% set var = unknown %}{{ var }}` | `{% set var = unknown %}{{ var }}` | -| `{% set a = 1 %}{{ a }}` | `{% set a = 1 %}1` | -| `{% set a = 1 %}{% set b = a %}{{ b }}` | `{% set a = 1 %}{% set b = 1 %}1` | - -### Include Tags - -| Scenario | Template | Output | -|----------|----------|--------| -| Unknown path variable | `{% include unknown_path %}` | `{% include unknown_path %}` | -| Known literal path | `{% include 'test.html' %}` | *(renders included content)* | -| Known path with unknown vars | `{% include 'test.html' %}` where test.html contains `{{ unknown }}` | `{{ unknown }}` | - -## Implementation Details - -`PreserveUnknownExecutionMode` configures the Jinjava context with: - -1. **Eager Tag Decorators**: Registers eager versions of all tags to handle deferred values -2. **Dynamic Variable Resolver**: Returns `DeferredValue.instance()` for any unknown variable -3. **Deferred Execution Mode**: Enables preservation of set tags and other state-changing operations - -```java -@Override -public void prepareContext(Context context) { - // Register eager tag decorators for all tags - context.getAllTags().stream() - .filter(tag -> !(tag instanceof EagerTagDecorator)) - .map(EagerTagFactory::getEagerTagDecorator) - .filter(Optional::isPresent) - .forEach(maybeEagerTag -> context.registerTag(maybeEagerTag.get())); - - // Return DeferredValue for unknown variables - context.setDynamicVariableResolver(varName -> DeferredValue.instance()); - - // Preserve set tags in output - context.setDeferredExecutionMode(true); -} -``` - -## Use Cases - -- **Multi-pass rendering**: First pass resolves some variables, second pass resolves others -- **Template composition**: Combine templates where some placeholders are filled later -- **Partial evaluation**: Evaluate what's known now, defer what isn't -- **Template debugging**: See which variables are missing without errors From a3577655d4468d35b107d7b064ca27237720209b Mon Sep 17 00:00:00 2001 From: Libo Song Date: Tue, 13 Jan 2026 11:18:49 -0500 Subject: [PATCH 6/8] Add parity test for filter chain optimization and fix unknown filter handling The parity test ensures the optimized filter chain produces identical results to the unoptimized nested method approach across many scenarios including chained filters, named parameters, SafeString handling, and edge cases. Fixed a behavioral difference where unknown filters would pass through the original value in the optimized path but return empty in the unoptimized path. Now both paths return null for unknown filters. Co-Authored-By: Claude Opus 4.5 --- .../jinjava/el/ext/AstFilterChain.java | 2 +- .../el/ext/AstFilterChainParityTest.java | 531 ++++++++++++++++++ 2 files changed, 532 insertions(+), 1 deletion(-) create mode 100644 src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainParityTest.java diff --git a/src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java b/src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java index 618b87905..d68ae8e8f 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java @@ -88,7 +88,7 @@ public Object eval(Bindings bindings, ELContext context) { return null; } if (filter == null) { - continue; + return null; } Object[] args = evaluateFilterArgs(spec, bindings, context); diff --git a/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainParityTest.java b/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainParityTest.java new file mode 100644 index 000000000..e95e9ab74 --- /dev/null +++ b/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainParityTest.java @@ -0,0 +1,531 @@ +package com.hubspot.jinjava.el.ext; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.hubspot.jinjava.Jinjava; +import com.hubspot.jinjava.JinjavaConfig; +import com.hubspot.jinjava.LegacyOverrides; +import com.hubspot.jinjava.interpret.JinjavaInterpreter; +import com.hubspot.jinjava.interpret.RenderResult; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.junit.Before; +import org.junit.Test; + +public class AstFilterChainParityTest { + + private Jinjava jinjavaOptimized; + private Jinjava jinjavaUnoptimized; + private Map context; + + @Before + public void setup() { + LegacyOverrides legacyOverrides = LegacyOverrides + .newBuilder() + .withUsePyishObjectMapper(true) + .withKeepNullableLoopValues(true) + .build(); + + jinjavaOptimized = + new Jinjava( + JinjavaConfig + .newBuilder() + .withEnableFilterChainOptimization(true) + .withLegacyOverrides(legacyOverrides) + .build() + ); + + jinjavaUnoptimized = + new Jinjava( + JinjavaConfig + .newBuilder() + .withEnableFilterChainOptimization(false) + .withLegacyOverrides(legacyOverrides) + .build() + ); + + context = new HashMap<>(); + context.put("name", " Hello World "); + context.put("text", "the quick brown fox jumps over the lazy dog"); + context.put("number", 12345); + context.put("float_num", 3.14159); + context.put("negative", -42); + context.put("items", Arrays.asList("apple", "banana", "cherry")); + context.put("empty_list", ImmutableList.of()); + context.put("numbers", Arrays.asList(3, 1, 4, 1, 5, 9, 2, 6)); + context.put("html", ""); + context.put("null_value", null); + context.put( + "nested", + ImmutableMap.of("key", "value", "num", 100, "list", Arrays.asList(1, 2, 3)) + ); + context.put( + "objects", + Arrays.asList( + ImmutableMap.of("name", "Alice", "age", 30), + ImmutableMap.of("name", "Bob", "age", 25), + ImmutableMap.of("name", "Charlie", "age", 35) + ) + ); + context.put("mixed_case", "HeLLo WoRLd"); + context.put("whitespace", " lots of spaces "); + context.put("unicode", "héllo wörld 你好"); + context.put("special_chars", "a&bd\"e'f"); + context.put("json_string", "{\"key\": \"value\", \"num\": 42}"); + context.put("long_text", "word ".repeat(100)); + context.put("arg_value", 10); + } + + @Test + public void itProducesSameResultsForSingleFilters() { + List templates = ImmutableList.of( + "{{ name|trim }}", + "{{ name|lower }}", + "{{ name|upper }}", + "{{ name|length }}", + "{{ number|string }}", + "{{ number|abs }}", + "{{ float_num|round }}", + "{{ float_num|int }}", + "{{ items|first }}", + "{{ items|last }}", + "{{ items|length }}", + "{{ items|reverse }}", + "{{ items|sort }}", + "{{ html|escape }}", + "{{ html|e }}", + "{{ text|capitalize }}", + "{{ text|title }}", + "{{ text|wordcount }}", + "{{ negative|abs }}", + "{{ mixed_case|lower }}", + "{{ mixed_case|upper }}", + "{{ whitespace|trim }}", + "{{ unicode|upper }}", + "{{ unicode|lower }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForChainedFilters() { + List templates = ImmutableList.of( + "{{ name|trim|lower }}", + "{{ name|trim|upper }}", + "{{ name|trim|lower|capitalize }}", + "{{ name|trim|lower|upper }}", + "{{ text|upper|lower|capitalize }}", + "{{ text|capitalize|lower|upper }}", + "{{ number|string|length }}", + "{{ number|string|upper }}", + "{{ items|first|upper }}", + "{{ items|last|lower }}", + "{{ items|reverse|first }}", + "{{ items|sort|last }}", + "{{ items|sort|reverse|first }}", + "{{ html|escape|upper }}", + "{{ float_num|round|string|length }}", + "{{ whitespace|trim|lower|capitalize }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForFiltersWithPositionalArgs() { + List templates = ImmutableList.of( + "{{ text|truncate(20) }}", + "{{ text|truncate(20, True) }}", + "{{ text|truncate(20, True, '...') }}", + "{{ text|truncate(10, False) }}", + "{{ items|join(', ') }}", + "{{ items|join(' - ') }}", + "{{ items|join('') }}", + "{{ text|replace('the', 'a') }}", + "{{ text|replace('o', '0') }}", + "{{ text|split(' ') }}", + "{{ text|split(' ', 3) }}", + "{{ number|default(0) }}", + "{{ null_value|default('fallback') }}", + "{{ null_value|default(42) }}", + "{{ float_num|round(2) }}", + "{{ float_num|round(0) }}", + "{{ text|center(50) }}", + "{{ text|center(50, '-') }}", + "{{ numbers|batch(3) }}", + "{{ numbers|slice(3) }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForFiltersWithNamedParams() { + List templates = ImmutableList.of( + "{{ text|truncate(length=20) }}", + "{{ text|truncate(length=20, killwords=True) }}", + "{{ text|truncate(length=20, end='!!!') }}", + "{{ text|truncate(length=15, killwords=False, end='...') }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForMixedPositionalAndNamedParams() { + List templates = ImmutableList.of( + "{{ text|truncate(20, killwords=True) }}", + "{{ text|truncate(20, end='!') }}", + "{{ items|join(', ')|truncate(length=15) }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForChainedFiltersWithArgs() { + List templates = ImmutableList.of( + "{{ text|truncate(20)|upper }}", + "{{ text|upper|truncate(20) }}", + "{{ text|replace('the', 'a')|upper }}", + "{{ text|upper|replace('THE', 'a') }}", + "{{ text|truncate(30)|replace('...', '!')|upper }}", + "{{ items|join(', ')|upper }}", + "{{ items|join(', ')|truncate(10) }}", + "{{ items|sort|join(' - ')|upper }}", + "{{ items|reverse|join(', ')|lower }}", + "{{ numbers|sort|join('-') }}", + "{{ numbers|reverse|join(', ')|length }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForFilterArgsWithExpressions() { + List templates = ImmutableList.of( + "{{ text|truncate(arg_value) }}", + "{{ text|truncate(arg_value + 5) }}", + "{{ text|truncate(arg_value * 2) }}", + "{{ items|join(name|trim) }}", + "{{ text|replace(items|first, items|last) }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForNullAndUndefinedHandling() { + List templates = ImmutableList.of( + "{{ null_value|default('fallback') }}", + "{{ null_value|default('fallback')|upper }}", + "{{ undefined_var|default('missing') }}", + "{{ undefined_var|default('missing')|lower }}", + "{{ null_value|string }}", + "{{ null_value|e }}", + "{{ nested.missing|default('not found') }}", + "{{ nested.missing|default('')|length }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForSafeStringHandling() { + context.put("safe_html", "Bold"); + + List templates = ImmutableList.of( + "{{ safe_html|safe }}", + "{{ safe_html|safe|upper }}", + "{{ safe_html|upper|safe }}", + "{{ safe_html|safe|length }}", + "{{ safe_html|safe|trim }}", + "{{ safe_html|safe|lower|capitalize }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForCollectionFilters() { + List templates = ImmutableList.of( + "{{ items|list }}", + "{{ items|unique }}", + "{{ numbers|sum }}", + "{{ numbers|sort }}", + "{{ numbers|sort|reverse }}", + "{{ objects|map(attribute='name') }}", + "{{ objects|map(attribute='name')|join(', ') }}", + "{{ objects|selectattr('age', '>', 28) }}", + "{{ objects|rejectattr('age', '<', 30) }}", + "{{ numbers|select('>', 3) }}", + "{{ numbers|reject('==', 1) }}", + "{{ items|batch(2)|list }}", + "{{ numbers|slice(3)|list }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForStringManipulationFilters() { + List templates = ImmutableList.of( + "{{ text|format }}", + "{{ text|striptags }}", + "{{ html|striptags }}", + "{{ text|urlize }}", + "{{ special_chars|escape }}", + "{{ special_chars|urlencode }}", + "{{ text|regex_replace('\\\\s+', '_') }}", + "{{ text|replace(' ', '_') }}", + "{{ name|trim|replace(' ', '-')|lower }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForNumericFilters() { + List templates = ImmutableList.of( + "{{ number|filesizeformat }}", + "{{ float_num|round }}", + "{{ float_num|round(2) }}", + "{{ float_num|round(2, 'floor') }}", + "{{ float_num|round(2, 'ceil') }}", + "{{ negative|abs }}", + "{{ number|float }}", + "{{ float_num|int }}", + "{{ number|divide(100) }}", + "{{ number|multiply(2) }}", + "{{ float_num|log }}", + "{{ number|root }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForDateTimeFilters() { + context.put("timestamp", 1609459200000L); + context.put("date_string", "2021-01-01"); + + List templates = ImmutableList.of( + "{{ timestamp|datetimeformat }}", + "{{ timestamp|unixtimestamp }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForJsonFilters() { + List templates = ImmutableList.of( + "{{ nested|tojson }}", + "{{ items|tojson }}", + "{{ json_string|fromjson }}", + "{{ json_string|fromjson|tojson }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForMultipleFilterChainsInTemplate() { + List templates = ImmutableList.of( + "{{ name|trim|lower }} and {{ text|upper|truncate(10) }}", + "Hello {{ name|trim }}, you have {{ items|length }} items", + "{{ items|first|upper }} - {{ items|last|lower }}", + "{{ number|string }} is {{ number|string|length }} digits", + "Name: {{ name|trim|lower|capitalize }}, Count: {{ items|length }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForNestedPropertyAccess() { + List templates = ImmutableList.of( + "{{ nested.key|upper }}", + "{{ nested.num|string }}", + "{{ nested.list|first }}", + "{{ nested.list|join('-') }}", + "{{ nested.key|upper|lower|capitalize }}", + "{{ objects[0].name|upper }}", + "{{ objects[0].name|upper|truncate(3) }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForFilterChainInConditions() { + List templates = ImmutableList.of( + "{% if name|trim|length > 5 %}long{% else %}short{% endif %}", + "{% if items|length > 2 %}many{% else %}few{% endif %}", + "{% if name|trim|lower == 'hello world' %}match{% else %}no match{% endif %}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForFilterChainInLoops() { + List templates = ImmutableList.of( + "{% for item in items|sort %}{{ item|upper }}{% endfor %}", + "{% for item in items|reverse %}{{ item|capitalize }}{% endfor %}", + "{% for n in numbers|sort|unique %}{{ n }}{% endfor %}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForLongFilterChains() { + List templates = ImmutableList.of( + "{{ text|upper|lower|capitalize|trim }}", + "{{ text|trim|lower|upper|lower|capitalize }}", + "{{ name|trim|lower|upper|lower|upper|lower }}", + "{{ text|replace('the', 'a')|upper|lower|capitalize|trim }}", + "{{ items|sort|reverse|join(', ')|upper|truncate(20) }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itTracksResolvedValuesConsistently() { + String template = "{{ name|trim|lower|upper }}"; + + RenderResult optimizedResult = jinjavaOptimized.renderForResult(template, context); + RenderResult unoptimizedResult = jinjavaUnoptimized.renderForResult( + template, + context + ); + + assertThat(optimizedResult.getOutput()) + .as("Output should match") + .isEqualTo(unoptimizedResult.getOutput()); + + Set optimizedResolved = optimizedResult.getContext().getResolvedValues(); + Set unoptimizedResolved = unoptimizedResult.getContext().getResolvedValues(); + + assertThat(optimizedResolved).as("Resolved filter:trim").contains("filter:trim"); + assertThat(optimizedResolved).as("Resolved filter:lower").contains("filter:lower"); + assertThat(optimizedResolved).as("Resolved filter:upper").contains("filter:upper"); + + assertThat(unoptimizedResolved) + .as("Unoptimized resolved filter:trim") + .contains("filter:trim"); + assertThat(unoptimizedResolved) + .as("Unoptimized resolved filter:lower") + .contains("filter:lower"); + assertThat(unoptimizedResolved) + .as("Unoptimized resolved filter:upper") + .contains("filter:upper"); + } + + @Test + public void itHandlesUnknownFiltersConsistently() { + String template = "{{ name|unknownfilter }}"; + + RenderResult optimizedResult = jinjavaOptimized.renderForResult(template, context); + RenderResult unoptimizedResult = jinjavaUnoptimized.renderForResult( + template, + context + ); + + assertThat(optimizedResult.getOutput()) + .as("Both paths should return empty for unknown filter") + .isEqualTo(unoptimizedResult.getOutput()); + } + + @Test + public void itProducesSameResultsForEmptyInputs() { + context.put("empty_string", ""); + + List templates = ImmutableList.of( + "{{ empty_string|upper }}", + "{{ empty_string|trim }}", + "{{ empty_string|default('fallback') }}", + "{{ empty_string|length }}", + "{{ empty_list|join(', ') }}", + "{{ empty_list|first }}", + "{{ empty_list|last }}", + "{{ empty_list|length }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForSpecialCharacters() { + List templates = ImmutableList.of( + "{{ special_chars|escape }}", + "{{ special_chars|escape|upper }}", + "{{ special_chars|urlencode }}", + "{{ special_chars|replace('&', 'and') }}", + "{{ unicode|upper }}", + "{{ unicode|lower }}", + "{{ unicode|length }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForBase64Filters() { + context.put("plain_text", "Hello, World!"); + context.put("base64_text", "SGVsbG8sIFdvcmxkIQ=="); + + List templates = ImmutableList.of( + "{{ plain_text|b64encode }}", + "{{ base64_text|b64decode }}", + "{{ plain_text|b64encode|b64decode }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForSelectAndRejectFilters() { + List templates = ImmutableList.of( + "{{ numbers|select('even')|list }}", + "{{ numbers|select('odd')|list }}", + "{{ numbers|reject('even')|list }}", + "{{ numbers|select('>', 3)|list }}", + "{{ numbers|select('>=', 4)|list }}", + "{{ numbers|reject('>', 5)|list }}" + ); + + assertParityForTemplates(templates); + } + + @Test + public void itProducesSameResultsForAttrFilters() { + List templates = ImmutableList.of( + "{{ objects|map(attribute='name')|list }}", + "{{ objects|map(attribute='age')|list }}", + "{{ objects|selectattr('age', '>', 28)|map(attribute='name')|list }}", + "{{ objects|rejectattr('age', '<', 30)|map(attribute='name')|list }}", + "{{ objects|groupby('age') }}" + ); + + assertParityForTemplates(templates); + } + + private void assertParityForTemplates(List templates) { + for (String template : templates) { + String optimizedResult = jinjavaOptimized.render(template, context); + String unoptimizedResult = jinjavaUnoptimized.render(template, context); + assertThat(optimizedResult) + .as("Template: %s", template) + .isEqualTo(unoptimizedResult); + } + } +} From 372c0a85f7c1bde13cad7303590937399ca74559 Mon Sep 17 00:00:00 2001 From: Libo Song Date: Tue, 13 Jan 2026 11:23:59 -0500 Subject: [PATCH 7/8] Separate unit tests from performance tests for filter chain optimization Moved unit tests to AstFilterChainTest.java, keeping only performance-related tests in AstFilterChainPerformanceTest.java for clearer test organization. Co-Authored-By: Claude Opus 4.5 --- .../el/ext/AstFilterChainPerformanceTest.java | 166 +++++------------- .../jinjava/el/ext/AstFilterChainTest.java | 70 ++++++++ 2 files changed, 116 insertions(+), 120 deletions(-) create mode 100644 src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainTest.java diff --git a/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainPerformanceTest.java b/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainPerformanceTest.java index 5ed90f2fa..b5cc4b8b7 100644 --- a/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainPerformanceTest.java +++ b/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainPerformanceTest.java @@ -11,11 +11,10 @@ import org.junit.Test; /** - * Performance test to verify that the optimized filter chain performs better than - * the nested method approach. + * Performance tests for the filter chain optimization. * - * Run with: mvn test -Dtest=AstFilterChainPerformanceTest - * Or run the main() method directly for more detailed output. + * Run manually with: mvn test -Dtest=AstFilterChainPerformanceTest + * Or run the main() method directly for detailed output. */ public class AstFilterChainPerformanceTest { @@ -62,42 +61,69 @@ public void runPerformanceComparison() { System.out.println("=== Filter Chain Performance Test ===\n"); System.out.println("Warming up..."); - // Warmup - runFilterTests(jinjavaOptimized, warmupIterations, false); - runFilterTests(jinjavaUnoptimized, warmupIterations, false); + runFilterTests(jinjavaOptimized, warmupIterations); + runFilterTests(jinjavaUnoptimized, warmupIterations); System.out.println( "Running performance tests with " + testIterations + " iterations each\n" ); - // Single filter comparePerformance("Single filter: {{ name|trim }}", testIterations); - - // Two chained filters comparePerformance("Two filters: {{ name|trim|lower }}", testIterations); - - // Three chained filters comparePerformance("Three filters: {{ name|trim|lower|capitalize }}", testIterations); - - // Five chained filters comparePerformance( "Five filters: {{ text|upper|replace('THE', 'a')|trim|lower|title }}", testIterations ); - - // Filter with arguments comparePerformance( "Filters with args: {{ text|truncate(20)|upper }}", testIterations ); - - // Multiple filter chains in same template comparePerformance( "Multiple chains: {{ name|trim|lower }} and {{ text|upper|truncate(10) }}", testIterations ); } + @Test + public void optimizedVersionShouldBeFaster() { + int warmupIterations = 100; + int testIterations = 1000; + String template = "{{ content.text|upper|replace('THE', 'a')|trim|lower|title }}"; + + for (int i = 0; i < warmupIterations; i++) { + jinjavaOptimized.render(template, context); + jinjavaUnoptimized.render(template, context); + } + + long totalOptimizedTime = 0; + long totalUnoptimizedTime = 0; + int rounds = 3; + + for (int round = 0; round < rounds; round++) { + totalUnoptimizedTime += timeExecution(jinjavaUnoptimized, template, testIterations); + totalOptimizedTime += timeExecution(jinjavaOptimized, template, testIterations); + } + + long avgUnoptimizedTime = totalUnoptimizedTime / rounds; + long avgOptimizedTime = totalOptimizedTime / rounds; + + System.out.printf( + "Performance test: Optimized=%d ms, Unoptimized=%d ms, Speedup=%.2fx%n", + avgOptimizedTime, + avgUnoptimizedTime, + (1.0 * avgUnoptimizedTime) / avgOptimizedTime + ); + + assertThat(avgOptimizedTime) + .as( + "Optimized (%d ms) should be faster than unoptimized (%d ms)", + avgOptimizedTime, + avgUnoptimizedTime + ) + .isLessThan((avgUnoptimizedTime * 95) / 100); + } + private void comparePerformance(String description, int iterations) { String template = description.substring(description.indexOf("{{")); if (description.contains(":")) { @@ -106,13 +132,10 @@ private void comparePerformance(String description, int iterations) { System.out.println(description); - // Run optimized long optimizedTime = timeExecution(jinjavaOptimized, template, iterations); - - // Run unoptimized long unoptimizedTime = timeExecution(jinjavaUnoptimized, template, iterations); - double speedup = (double) unoptimizedTime / optimizedTime; + double speedup = (1.0 * unoptimizedTime) / optimizedTime; System.out.printf( " Optimized: %d ms, Unoptimized: %d ms, Speedup: %.2fx%n%n", optimizedTime, @@ -129,7 +152,7 @@ private long timeExecution(Jinjava jinjava, String template, int iterations) { return System.currentTimeMillis() - startTime; } - private void runFilterTests(Jinjava jinjava, int iterations, boolean print) { + private void runFilterTests(Jinjava jinjava, int iterations) { String[] templates = { "{{ name|trim }}", "{{ name|trim|lower }}", @@ -144,101 +167,4 @@ private void runFilterTests(Jinjava jinjava, int iterations, boolean print) { } } } - - @Test - public void itProducesSameResultsWithAndWithoutOptimization() { - String[] templates = { - "{{ name|trim }}", - "{{ name|trim|lower }}", - "{{ name|trim|lower|capitalize }}", - "{{ text|upper|replace('THE', 'a')|trim|lower|title }}", - "{{ text|truncate(20)|upper }}", - "{{ name|trim|lower }} and {{ text|upper|truncate(10) }}", - "{{ items|join(', ')|upper }}", - "{{ number|string|length }}", - }; - - for (String template : templates) { - String optimizedResult = jinjavaOptimized.render(template, context); - String unoptimizedResult = jinjavaUnoptimized.render(template, context); - assertThat(optimizedResult) - .as("Template: " + template) - .isEqualTo(unoptimizedResult); - } - } - - @Test - public void itHandlesSingleFilterWithOptimization() { - String result = jinjavaOptimized.render("{{ name|trim }}", context); - assertThat(result).isEqualTo("Hello World"); - } - - @Test - public void itHandlesChainedFiltersWithOptimization() { - String result = jinjavaOptimized.render("{{ name|trim|lower }}", context); - assertThat(result).isEqualTo("hello world"); - } - - @Test - public void itHandlesFiltersWithArgumentsWithOptimization() { - String result = jinjavaOptimized.render("{{ text|truncate(20)|upper }}", context); - assertThat(result).isNotEmpty(); - assertThat(result).isUpperCase(); - } - - @Test - public void itHandlesComplexFilterChainWithOptimization() { - String result = jinjavaOptimized.render( - "{{ text|upper|replace('THE', 'a')|trim|lower|capitalize }}", - context - ); - assertThat(result).isNotEmpty(); - } - - /** - * This test verifies that the optimized version is faster than the unoptimized version. - * The optimization should provide a measurable speedup for chained filters. - */ - @Test - public void optimizedVersionShouldBeFaster() { - int warmupIterations = 100; - int testIterations = 1000; - String template = "{{ content.text|upper|replace('THE', 'a')|trim|lower|title }}"; - - // Warmup both to ensure JIT compilation - for (int i = 0; i < warmupIterations; i++) { - jinjavaOptimized.render(template, context); - jinjavaUnoptimized.render(template, context); - } - - // Run multiple rounds to get more stable results - long totalOptimizedTime = 0; - long totalUnoptimizedTime = 0; - int rounds = 3; - - for (int round = 0; round < rounds; round++) { - totalUnoptimizedTime += timeExecution(jinjavaUnoptimized, template, testIterations); - totalOptimizedTime += timeExecution(jinjavaOptimized, template, testIterations); - } - - long avgUnoptimizedTime = totalUnoptimizedTime / rounds; - long avgOptimizedTime = totalOptimizedTime / rounds; - - System.out.printf( - "Performance test: Optimized=%d ms, Unoptimized=%d ms, Speedup=%.2fx%n", - avgOptimizedTime, - avgUnoptimizedTime, - (double) avgUnoptimizedTime / avgOptimizedTime - ); - - // The optimized version should be faster (allow 10% margin for system variance) - // If optimized takes more than 90% of unoptimized time, fail the test - assertThat(avgOptimizedTime) - .as( - "Optimized (%d ms) should be faster than unoptimized (%d ms)", - avgOptimizedTime, - avgUnoptimizedTime - ) - .isLessThan((long) (avgUnoptimizedTime * 0.95)); - } } diff --git a/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainTest.java b/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainTest.java new file mode 100644 index 000000000..f0a0931a3 --- /dev/null +++ b/src/test/java/com/hubspot/jinjava/el/ext/AstFilterChainTest.java @@ -0,0 +1,70 @@ +package com.hubspot.jinjava.el.ext; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.hubspot.jinjava.Jinjava; +import com.hubspot.jinjava.JinjavaConfig; +import java.util.HashMap; +import java.util.Map; +import org.junit.Before; +import org.junit.Test; + +public class AstFilterChainTest { + + private Jinjava jinjava; + private Map context; + + @Before + public void setup() { + jinjava = + new Jinjava( + JinjavaConfig.newBuilder().withEnableFilterChainOptimization(true).build() + ); + + context = new HashMap<>(); + context.put("name", " Hello World "); + context.put("text", "the quick brown fox jumps over the lazy dog"); + context.put("number", 12345); + context.put("items", new String[] { "apple", "banana", "cherry" }); + } + + @Test + public void itHandlesSingleFilter() { + String result = jinjava.render("{{ name|trim }}", context); + assertThat(result).isEqualTo("Hello World"); + } + + @Test + public void itHandlesChainedFilters() { + String result = jinjava.render("{{ name|trim|lower }}", context); + assertThat(result).isEqualTo("hello world"); + } + + @Test + public void itHandlesFiltersWithArguments() { + String result = jinjava.render("{{ text|truncate(20)|upper }}", context); + assertThat(result).isNotEmpty(); + assertThat(result).isUpperCase(); + } + + @Test + public void itHandlesComplexFilterChain() { + String result = jinjava.render( + "{{ text|upper|replace('THE', 'a')|trim|lower|capitalize }}", + context + ); + assertThat(result).isNotEmpty(); + } + + @Test + public void itHandlesFilterWithJoin() { + String result = jinjava.render("{{ items|join(', ')|upper }}", context); + assertThat(result).isEqualTo("APPLE, BANANA, CHERRY"); + } + + @Test + public void itHandlesFilterWithStringConversion() { + String result = jinjava.render("{{ number|string|length }}", context); + assertThat(result).isEqualTo("5"); + } +} From eb83265f8a0832328d19ffc4881fc1c28a1c445e Mon Sep 17 00:00:00 2001 From: Libo Song Date: Tue, 13 Jan 2026 11:29:06 -0500 Subject: [PATCH 8/8] Simplify appendStructure by delegating to AstParameters Use params.appendStructure() instead of manually iterating through children, reducing code duplication. Co-Authored-By: Claude Opus 4.5 --- .../java/com/hubspot/jinjava/el/ext/AstFilterChain.java | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java b/src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java index d68ae8e8f..30dfc4435 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java @@ -170,14 +170,7 @@ public void appendStructure(StringBuilder builder, Bindings bindings) { builder.append('|').append(spec.getName()); AstParameters params = spec.getParams(); if (params != null && params.getCardinality() > 0) { - builder.append('('); - for (int i = 0; i < params.getCardinality(); i++) { - if (i > 0) { - builder.append(", "); - } - params.getChild(i).appendStructure(builder, bindings); - } - builder.append(')'); + params.appendStructure(builder, bindings); } } }