Skip to content

Commit 86be819

Browse files
jasmith-hsjasmith-hs
committed
Make use of AutoCloseableSupplier and deprecate unsafe CallStack
operations
1 parent 490d2fd commit 86be819

11 files changed

Lines changed: 414 additions & 272 deletions

File tree

‎src/main/java/com/hubspot/jinjava/el/ext/AstMacroFunction.java‎

Lines changed: 59 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package com.hubspot.jinjava.el.ext;
22

33
import com.google.common.collect.ImmutableMap;
4+
import com.hubspot.jinjava.interpret.AutoCloseableSupplier;
5+
import com.hubspot.jinjava.interpret.AutoCloseableSupplier.AutoCloseableImpl;
46
import com.hubspot.jinjava.interpret.CallStack;
57
import com.hubspot.jinjava.interpret.DeferredValueException;
68
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
@@ -37,30 +39,21 @@ public Object eval(Bindings bindings, ELContext context) {
3739
interpreter.getPosition()
3840
);
3941
}
40-
if (!macroFunction.isCaller()) {
41-
if (checkAndPushMacroStack(interpreter, getName())) {
42+
if (macroFunction.isCaller()) {
43+
return wrapInvoke(bindings, context, macroFunction);
44+
}
45+
try (
46+
AutoCloseableImpl<Boolean> macroStackPush = checkAndPushMacroStackWithWrapper(
47+
interpreter,
48+
getName()
49+
)
50+
.get()
51+
) {
52+
if (macroStackPush.value()) {
4253
return "";
4354
}
44-
}
4555

46-
try {
47-
return invoke(
48-
bindings,
49-
context,
50-
macroFunction,
51-
AbstractCallableMethod.EVAL_METHOD
52-
);
53-
} catch (IllegalAccessException e) {
54-
throw new ELException(LocalMessages.get("error.function.access", getName()), e);
55-
} catch (InvocationTargetException e) {
56-
throw new ELException(
57-
LocalMessages.get("error.function.invocation", getName()),
58-
e.getCause()
59-
);
60-
} finally {
61-
if (!macroFunction.isCaller()) {
62-
interpreter.getContext().getMacroStack().pop();
63-
}
56+
return wrapInvoke(bindings, context, macroFunction);
6457
}
6558
}
6659

@@ -69,35 +62,55 @@ public Object eval(Bindings bindings, ELContext context) {
6962
: super.eval(bindings, context);
7063
}
7164

72-
public static boolean checkAndPushMacroStack(
65+
private Object wrapInvoke(
66+
Bindings bindings,
67+
ELContext context,
68+
MacroFunction macroFunction
69+
) {
70+
try {
71+
return invoke(bindings, context, macroFunction, AbstractCallableMethod.EVAL_METHOD);
72+
} catch (IllegalAccessException e) {
73+
throw new ELException(LocalMessages.get("error.function.access", getName()), e);
74+
} catch (InvocationTargetException e) {
75+
throw new ELException(
76+
LocalMessages.get("error.function.invocation", getName()),
77+
e.getCause()
78+
);
79+
}
80+
}
81+
82+
public static AutoCloseableSupplier<Boolean> checkAndPushMacroStackWithWrapper(
7383
JinjavaInterpreter interpreter,
7484
String name
7585
) {
7686
CallStack macroStack = interpreter.getContext().getMacroStack();
7787
try {
7888
if (interpreter.getConfig().isEnableRecursiveMacroCalls()) {
7989
if (interpreter.getConfig().getMaxMacroRecursionDepth() != 0) {
80-
macroStack.pushWithMaxDepth(
81-
name,
82-
interpreter.getConfig().getMaxMacroRecursionDepth(),
83-
interpreter.getLineNumber(),
84-
interpreter.getPosition()
85-
);
90+
return macroStack
91+
.closeablePushWithMaxDepth(
92+
name,
93+
interpreter.getConfig().getMaxMacroRecursionDepth(),
94+
interpreter.getLineNumber(),
95+
interpreter.getPosition()
96+
)
97+
.map(macro -> false);
8698
} else {
87-
macroStack.pushWithoutCycleCheck(
88-
name,
89-
interpreter.getLineNumber(),
90-
interpreter.getPosition()
91-
);
99+
return macroStack
100+
.closeablePushWithoutCycleCheck(
101+
name,
102+
interpreter.getLineNumber(),
103+
interpreter.getPosition()
104+
)
105+
.map(macro -> false);
92106
}
93-
} else {
94-
macroStack.push(name, -1, -1);
95107
}
108+
return macroStack.closeablePush(name, -1, -1).map(macro -> false);
96109
} catch (MacroTagCycleException e) {
97110
int maxDepth = interpreter.getConfig().getMaxMacroRecursionDepth();
98111
if (maxDepth != 0 && interpreter.getConfig().isValidationMode()) {
99112
// validation mode is only concerned with syntax
100-
return true;
113+
return AutoCloseableSupplier.of(true);
101114
}
102115

103116
String message = maxDepth == 0
@@ -123,8 +136,16 @@ public static boolean checkAndPushMacroStack(
123136
)
124137
);
125138

126-
return true;
139+
return AutoCloseableSupplier.of(true);
127140
}
128-
return false;
141+
}
142+
143+
@Deprecated
144+
public static boolean checkAndPushMacroStack(
145+
JinjavaInterpreter interpreter,
146+
String name
147+
) {
148+
return checkAndPushMacroStackWithWrapper(interpreter, name)
149+
.dangerouslyGetWithoutClosing();
129150
}
130151
}

‎src/main/java/com/hubspot/jinjava/interpret/CallStack.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,34 @@ public boolean isEmpty() {
111111
return stack.empty() && (parent == null || parent.isEmpty());
112112
}
113113

114+
public AutoCloseableSupplier<String> closeablePush(
115+
String path,
116+
int lineNumber,
117+
int startPosition
118+
) {
119+
push(path, lineNumber, startPosition);
120+
return AutoCloseableSupplier.of(path, ignored -> pop());
121+
}
122+
123+
public AutoCloseableSupplier<String> closeablePushWithoutCycleCheck(
124+
String path,
125+
int lineNumber,
126+
int startPosition
127+
) {
128+
pushWithoutCycleCheck(path, lineNumber, startPosition);
129+
return AutoCloseableSupplier.of(path, ignored -> pop());
130+
}
131+
132+
public AutoCloseableSupplier<String> closeablePushWithMaxDepth(
133+
String path,
134+
int maxDepth,
135+
int lineNumber,
136+
int startPosition
137+
) {
138+
pushWithMaxDepth(path, maxDepth, lineNumber, startPosition);
139+
return AutoCloseableSupplier.of(path, ignored -> pop());
140+
}
141+
114142
private void pushToStack(String path, int lineNumber, int startPosition) {
115143
if (isEmpty()) {
116144
topLineNumber = lineNumber;

‎src/main/java/com/hubspot/jinjava/interpret/Context.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.google.common.collect.ImmutableSet;
2222
import com.google.common.collect.SetMultimap;
2323
import com.google.common.collect.Sets;
24+
import com.hubspot.jinjava.interpret.AutoCloseableSupplier.AutoCloseableImpl;
2425
import com.hubspot.jinjava.interpret.ContextConfigurationIF.ErrorHandlingStrategyIF.TemplateErrorTypeHandlingStrategy;
2526
import com.hubspot.jinjava.lib.Importable;
2627
import com.hubspot.jinjava.lib.expression.ExpressionStrategy;
@@ -677,6 +678,10 @@ public CallStack getImportPathStack() {
677678
return importPathStack;
678679
}
679680

681+
public CallStack getFromPathStack() {
682+
return fromStack;
683+
}
684+
680685
public CallStack getIncludePathStack() {
681686
return includePathStack;
682687
}
@@ -693,10 +698,12 @@ public CallStack getCurrentPathStack() {
693698
return currentPathStack;
694699
}
695700

701+
@Deprecated
696702
public void pushFromStack(String path, int lineNumber, int startPosition) {
697703
fromStack.push(path, lineNumber, startPosition);
698704
}
699705

706+
@Deprecated
700707
public void popFromStack() {
701708
fromStack.pop();
702709
}
@@ -878,7 +885,7 @@ public TemporaryValueClosable<Boolean> withUnwrapRawOverride() {
878885
return temporaryValueClosable;
879886
}
880887

881-
public static class TemporaryValueClosable<T> extends AutoCloseableWrapper<T> {
888+
public static class TemporaryValueClosable<T> extends AutoCloseableImpl<T> {
882889

883890
private TemporaryValueClosable(T previousValue, Consumer<T> resetValueConsumer) {
884891
super(previousValue, resetValueConsumer);

‎src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java‎

Lines changed: 39 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import com.hubspot.jinjava.el.ExpressionResolver;
3030
import com.hubspot.jinjava.el.ext.DeferredParsingException;
3131
import com.hubspot.jinjava.el.ext.ExtendedParser;
32+
import com.hubspot.jinjava.interpret.AutoCloseableSupplier.AutoCloseableImpl;
3233
import com.hubspot.jinjava.interpret.Context.TemporaryValueClosable;
3334
import com.hubspot.jinjava.interpret.ContextConfigurationIF.ErrorHandlingStrategyIF;
3435
import com.hubspot.jinjava.interpret.ContextConfigurationIF.ErrorHandlingStrategyIF.TemplateErrorTypeHandlingStrategy;
@@ -547,37 +548,29 @@ private void resolveBlockStubs(OutputList output, Stack<String> blockNames) {
547548
OutputList blockValueBuilder = new OutputList(config.getMaxOutputSize());
548549
DynamicRenderedOutputNode prefix = new DynamicRenderedOutputNode();
549550
blockValueBuilder.addNode(prefix);
550-
boolean pushedParentPathOntoStack = false;
551551
int numDeferredTokensBefore = context.getDeferredTokens().size();
552-
if (
553-
block.getParentPath().isPresent() &&
554-
!getContext().getCurrentPathStack().contains(block.getParentPath().get())
552+
553+
try (
554+
AutoCloseableImpl<Boolean> parentPathPush = conditionallyPushParentPath(block)
555+
.get()
555556
) {
556-
getContext()
557-
.getCurrentPathStack()
558-
.push(
559-
block.getParentPath().get(),
560-
block.getParentLineNo(),
561-
block.getParentPosition()
562-
);
563-
pushedParentPathOntoStack = true;
564-
lineNumber--; // The line number is off by one when rendering the block from the parent template
565-
}
566-
for (Node child : block.getNodes()) {
567-
lineNumber = child.getLineNumber();
568-
position = child.getStartPosition();
557+
if (parentPathPush.value()) {
558+
lineNumber--; // The line number is off by one when rendering the block from the parent template
559+
}
569560

570-
blockValueBuilder.addNode(child.render(this));
571-
}
572-
if (context.getDeferredTokens().size() > numDeferredTokensBefore) {
573-
EagerReconstructionUtils.reconstructPathAroundBlock(
574-
prefix,
575-
blockValueBuilder,
576-
this
577-
);
578-
}
579-
if (pushedParentPathOntoStack) {
580-
getContext().getCurrentPathStack().pop();
561+
for (Node child : block.getNodes()) {
562+
lineNumber = child.getLineNumber();
563+
position = child.getStartPosition();
564+
565+
blockValueBuilder.addNode(child.render(this));
566+
}
567+
if (context.getDeferredTokens().size() > numDeferredTokensBefore) {
568+
EagerReconstructionUtils.reconstructPathAroundBlock(
569+
prefix,
570+
blockValueBuilder,
571+
this
572+
);
573+
}
581574
}
582575
blockNames.push(blockPlaceholder.getBlockName());
583576
resolveBlockStubs(blockValueBuilder, blockNames);
@@ -596,6 +589,24 @@ private void resolveBlockStubs(OutputList output, Stack<String> blockNames) {
596589
}
597590
}
598591

592+
private AutoCloseableSupplier<Boolean> conditionallyPushParentPath(BlockInfo block) {
593+
if (
594+
block.getParentPath().isPresent() &&
595+
!getContext().getCurrentPathStack().contains(block.getParentPath().get())
596+
) {
597+
return getContext()
598+
.getCurrentPathStack()
599+
.closeablePush(
600+
block.getParentPath().get(),
601+
block.getParentLineNo(),
602+
block.getParentPosition()
603+
)
604+
.map(path -> true);
605+
} else {
606+
return AutoCloseableSupplier.of(false);
607+
}
608+
}
609+
599610
/**
600611
* Resolve a variable from the interpreter context, returning null if not found. This method updates the template error accumulators when a variable is not found.
601612
*

‎src/main/java/com/hubspot/jinjava/lib/fn/MacroFunction.java‎

Lines changed: 27 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package com.hubspot.jinjava.lib.fn;
22

33
import com.hubspot.jinjava.el.ext.AbstractCallableMethod;
4+
import com.hubspot.jinjava.interpret.AutoCloseableSupplier;
5+
import com.hubspot.jinjava.interpret.AutoCloseableSupplier.AutoCloseableImpl;
46
import com.hubspot.jinjava.interpret.Context;
57
import com.hubspot.jinjava.interpret.Context.TemporaryValueClosable;
68
import com.hubspot.jinjava.interpret.DeferredValue;
@@ -72,31 +74,39 @@ public Object doEvaluate(
7274
List<Object> varArgs
7375
) {
7476
JinjavaInterpreter interpreter = JinjavaInterpreter.getCurrent();
75-
Optional<String> importFile = getImportFile(interpreter);
76-
try (InterpreterScopeClosable c = interpreter.enterScope()) {
77+
try (
78+
InterpreterScopeClosable c = interpreter.enterScope();
79+
AutoCloseableImpl<Optional<String>> importFile = getImportFileWithWrapper(
80+
interpreter
81+
)
82+
.get()
83+
) {
7784
return getEvaluationResult(argMap, kwargMap, varArgs, interpreter);
78-
} finally {
79-
importFile.ifPresent(path -> interpreter.getContext().getCurrentPathStack().pop());
8085
}
8186
}
8287

8388
public Optional<String> getImportFile(JinjavaInterpreter interpreter) {
89+
return getImportFileWithWrapper(interpreter).dangerouslyGetWithoutClosing();
90+
}
91+
92+
public AutoCloseableSupplier<Optional<String>> getImportFileWithWrapper(
93+
JinjavaInterpreter interpreter
94+
) {
8495
Optional<String> importFile = Optional.ofNullable(
8596
(String) localContextScope.get(Context.IMPORT_RESOURCE_PATH_KEY)
8697
);
87-
88-
// pushWithoutCycleCheck() is used to here so that macros calling macros from the same file will not throw a TagCycleException
89-
importFile.ifPresent(path ->
90-
interpreter
91-
.getContext()
92-
.getCurrentPathStack()
93-
.pushWithoutCycleCheck(
94-
path,
95-
interpreter.getLineNumber(),
96-
interpreter.getPosition()
97-
)
98-
);
99-
return importFile;
98+
if (importFile.isEmpty()) {
99+
return AutoCloseableSupplier.of(Optional.empty());
100+
}
101+
return interpreter
102+
.getContext()
103+
.getCurrentPathStack()
104+
.closeablePushWithoutCycleCheck(
105+
importFile.get(),
106+
interpreter.getLineNumber(),
107+
interpreter.getPosition()
108+
)
109+
.map(Optional::of);
100110
}
101111

102112
public String getEvaluationResult(

0 commit comments

Comments
 (0)