Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/main/java/com/hubspot/jinjava/JinjavaConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -307,6 +309,10 @@ public boolean getEnablePreciseDivideFilter() {
return enablePreciseDivideFilter;
}

public boolean isEnableFilterChainOptimization() {
return enableFilterChainOptimization;
}

public DateTimeProvider getDateTimeProvider() {
return dateTimeProvider;
}
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all the tests pass if this is set to true?

private ObjectMapper objectMapper = null;

private ObjectUnwrapper objectUnwrapper = new JinjavaObjectUnwrapper();
Expand Down Expand Up @@ -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;
Expand Down
212 changes: 212 additions & 0 deletions src/main/java/com/hubspot/jinjava/el/ext/AstFilterChain.java
Original file line number Diff line number Diff line change
@@ -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<FilterSpec> filterSpecs;

public AstFilterChain(AstNode input, List<FilterSpec> 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<FilterSpec> 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);
Comment on lines +69 to +70
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be tested for parity with regular filter parsing and evaluation


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<String, Object> 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<String, Object> extractNamedParams(Object[] args) {
Map<String, Object> 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<Object> 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(')');
Comment on lines +173 to +180
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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);

}
}
}

@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;
}
}
91 changes: 67 additions & 24 deletions src/main/java/com/hubspot/jinjava/el/ext/ExtendedParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -531,30 +531,11 @@ 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;

do {
consumeToken(); // '|'
String filterName = consumeToken().getImage();
List<AstNode> 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)); // function("filter:" + filterName, new AstParameters(filterParams));
} while ("|".equals(getToken().getImage()));

return v;
if (shouldUseFilterChainOptimization()) {
return parseFiltersAsChain(left);
} else {
return parseFiltersAsNestedMethods(left);
}
} else if (
"is".equals(getToken().getImage()) &&
"not".equals(lookahead(0).getImage()) &&
Expand All @@ -577,6 +558,68 @@ protected AstParameters createAstParameters(List<AstNode> nodes) {
return new AstParameters(nodes);
}

protected AstFilterChain createAstFilterChain(
AstNode input,
List<FilterSpec> filterSpecs
) {
return new AstFilterChain(input, filterSpecs);
}

private AstNode parseFiltersAsChain(AstNode left) throws ScanException, ParseException {
List<FilterSpec> 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<AstNode> 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);
}
Expand Down
Loading