Skip to content

Commit ee1bcdf

Browse files
jasmith-hsjasmith-hs
committed
Fix up minor bugs in logic and update tests
1 parent 9bd559d commit ee1bcdf

21 files changed

+156
-232
lines changed

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

Lines changed: 5 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
*
2424
*/
2525
public class MacroFunction extends AbstractCallableMethod {
26+
public static final String KWARGS_KEY = "kwargs";
27+
public static final String VARARGS_KEY = "varargs";
2628
protected final List<Node> content;
2729

2830
protected final boolean caller;
@@ -71,33 +73,10 @@ public Object doEvaluate(
7173
Map<String, Object> kwargMap,
7274
List<Object> varArgs
7375
) {
74-
int currentCallCount = callCount.getAndIncrement();
7576
JinjavaInterpreter interpreter = JinjavaInterpreter.getCurrent();
7677
Optional<String> importFile = getImportFile(interpreter);
7778
try (InterpreterScopeClosable c = interpreter.enterScope()) {
78-
String result = getEvaluationResult(argMap, kwargMap, varArgs, interpreter);
79-
// if (
80-
// !interpreter.getContext().getDeferredNodes().isEmpty() ||
81-
// !interpreter.getContext().getDeferredTokens().isEmpty()
82-
// ) {
83-
// if (!interpreter.getContext().isPartialMacroEvaluation()) {
84-
// String tempVarName = MacroFunctionTempVariable.getVarName(
85-
// getName(),
86-
// hashCode(),
87-
// currentCallCount
88-
// );
89-
// interpreter
90-
// .getContext()
91-
// .getParent()
92-
// .put(tempVarName, new MacroFunctionTempVariable(result));
93-
// throw new DeferredParsingException(this, tempVarName);
94-
// }
95-
// if (interpreter.getContext().isDeferredExecutionMode()) {
96-
// return EagerReconstructionUtils.wrapInChildScope(result, interpreter);
97-
// }
98-
// }
99-
100-
return result;
79+
return getEvaluationResult(argMap, kwargMap, varArgs, interpreter);
10180
} finally {
10281
importFile.ifPresent(path -> interpreter.getContext().getCurrentPathStack().pop());
10382
}
@@ -162,9 +141,9 @@ public String getEvaluationResult(
162141
interpreter.getContext().put(argEntry.getKey(), argEntry.getValue());
163142
}
164143
// parameter map
165-
interpreter.getContext().put("kwargs", kwargMap);
144+
interpreter.getContext().put(KWARGS_KEY, kwargMap);
166145
// varargs list
167-
interpreter.getContext().put("varargs", varArgs);
146+
interpreter.getContext().put(VARARGS_KEY, varArgs);
168147

169148
LengthLimitingStringBuilder result = new LengthLimitingStringBuilder(
170149
interpreter.getConfig().getMaxOutputSize()

‎src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java‎

Lines changed: 68 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -12,21 +12,20 @@
1212
import com.hubspot.jinjava.interpret.JinjavaInterpreter.InterpreterScopeClosable;
1313
import com.hubspot.jinjava.lib.fn.MacroFunction;
1414
import com.hubspot.jinjava.lib.tag.MacroTag;
15-
import com.hubspot.jinjava.lib.tag.eager.DeferredToken;
1615
import com.hubspot.jinjava.lib.tag.eager.EagerExecutionResult;
1716
import com.hubspot.jinjava.objects.serialization.PyishObjectMapper;
1817
import com.hubspot.jinjava.tree.Node;
1918
import com.hubspot.jinjava.util.EagerContextWatcher;
19+
import com.hubspot.jinjava.util.EagerContextWatcher.EagerChildContextConfig;
2020
import com.hubspot.jinjava.util.EagerExpressionResolver.EagerExpressionResult;
2121
import com.hubspot.jinjava.util.EagerReconstructionUtils;
22-
import java.util.HashSet;
2322
import java.util.LinkedHashMap;
2423
import java.util.List;
2524
import java.util.Map;
2625
import java.util.Optional;
27-
import java.util.Set;
2826
import java.util.StringJoiner;
2927
import java.util.concurrent.atomic.AtomicInteger;
28+
import java.util.function.Supplier;
3029

3130
public class EagerMacroFunction extends MacroFunction {
3231
private AtomicInteger callCount = new AtomicInteger();
@@ -60,9 +59,19 @@ public Object doEvaluate(
6059
JinjavaInterpreter interpreter = JinjavaInterpreter.getCurrent();
6160
if (reconstructing) {
6261
Optional<String> importFile = getImportFile(interpreter);
63-
try (InterpreterScopeClosable c = interpreter.enterNonStackingScope()) {
64-
interpreter.getContext().setDeferredExecutionMode(true);
65-
return getEvaluationResult(argMap, kwargMap, varArgs, interpreter);
62+
try (InterpreterScopeClosable c = interpreter.enterScope()) {
63+
EagerExecutionResult result = eagerEvaluateInDeferredExecutionMode(
64+
() -> getEvaluationResultDirectly(argMap, kwargMap, varArgs, interpreter),
65+
interpreter
66+
);
67+
if (!result.getResult().isFullyResolved()) {
68+
result =
69+
eagerEvaluateInDeferredExecutionMode(
70+
() -> getEvaluationResultDirectly(argMap, kwargMap, varArgs, interpreter),
71+
interpreter
72+
);
73+
}
74+
return result.asTemplateString();
6675
} finally {
6776
importFile.ifPresent(
6877
path -> interpreter.getContext().getCurrentPathStack().pop()
@@ -71,21 +80,9 @@ public Object doEvaluate(
7180
}
7281

7382
int currentCallCount = callCount.getAndIncrement();
74-
Set<DeferredToken> addedTokens = new HashSet<>();
75-
EagerExecutionResult result = EagerContextWatcher.executeInChildContext(
76-
eagerInterpreter -> {
77-
EagerExpressionResult expressionResult = EagerExpressionResult.fromSupplier(
78-
() -> super.doEvaluate(argMap, kwargMap, varArgs).toString(),
79-
eagerInterpreter
80-
);
81-
addedTokens.addAll(eagerInterpreter.getContext().getDeferredTokens());
82-
return expressionResult;
83-
},
84-
interpreter,
85-
EagerContextWatcher
86-
.EagerChildContextConfig.newBuilder()
87-
.withCheckForContextChanges(!interpreter.getContext().isDeferredExecutionMode())
88-
.build()
83+
EagerExecutionResult result = eagerEvaluate(
84+
() -> super.doEvaluate(argMap, kwargMap, varArgs).toString(),
85+
interpreter
8986
);
9087
if (
9188
!result.getResult().isFullyResolved() &&
@@ -95,14 +92,11 @@ public Object doEvaluate(
9592
interpreter.getContext().isDeferredExecutionMode()
9693
)
9794
) {
98-
// EagerReconstructionUtils.resetSpeculativeBindings(interpreter, result);
99-
// interpreter.getContext().removeDeferredTokens(addedTokens);
100-
EagerExecutionResult firstRunResult = runLoopOnce(
101-
argMap,
102-
kwargMap,
103-
varArgs,
104-
interpreter
105-
);
95+
result =
96+
eagerEvaluateInDeferredExecutionMode(
97+
() -> super.doEvaluate(argMap, kwargMap, varArgs).toString(),
98+
interpreter
99+
);
106100
String tempVarName = MacroFunctionTempVariable.getVarName(
107101
getName(),
108102
hashCode(),
@@ -111,87 +105,64 @@ public Object doEvaluate(
111105
interpreter
112106
.getContext()
113107
.getParent()
114-
.put(
115-
tempVarName,
116-
new MacroFunctionTempVariable(firstRunResult.asTemplateString())
117-
);
108+
.put(tempVarName, new MacroFunctionTempVariable(result.asTemplateString()));
118109
throw new DeferredParsingException(this, tempVarName);
119-
// }
120-
//
121-
// if (
122-
// interpreter.getContext().isDeferredExecutionMode() ||
123-
// !result.getSpeculativeBindings().isEmpty()
124-
// ) {
125-
// return reconstructImage();
126-
// EagerReconstructionUtils.resetSpeculativeBindings(interpreter, result);
127-
// interpreter.getContext().removeDeferredTokens(addedTokens);
128-
// String prefix = "";
129-
//
130-
// EagerExecutionResult firstRunResult = runLoopOnce(
131-
// argMap,
132-
// kwargMap,
133-
// varArgs,
134-
// interpreter
135-
// );
136-
// if (!firstRunResult.getSpeculativeBindings().isEmpty()) {
137-
// // Set<String> toRemove = firstRunResult
138-
// // .getSpeculativeBindings()
139-
// // .keySet()
140-
// // .stream()
141-
// // .filter(key -> argMap.containsKey(key) || kwargMap.containsKey(key))
142-
// // .collect(Collectors.toSet());
143-
// // toRemove.forEach(key -> firstRunResult.getSpeculativeBindings().remove(key));
144-
// // Defer any variables that we tried to modify during the loop
145-
// prefix = firstRunResult.getPrefixToPreserveState(true);
146-
// }
147-
// // Run for loop again now that the necessary values have been deferred
148-
// EagerExecutionResult secondRunResult = runLoopOnce(
149-
// argMap,
150-
// kwargMap,
151-
// varArgs,
152-
// interpreter
153-
// );
154-
// if (
155-
// secondRunResult
156-
// .getSpeculativeBindings()
157-
// .keySet()
158-
// .stream()
159-
// .anyMatch(key -> !firstRunResult.getSpeculativeBindings().containsKey(key))
160-
// ) {
161-
// throw new DeferredValueException(
162-
// "Modified values in deferred for loop: " +
163-
// String.join(", ", secondRunResult.getSpeculativeBindings().keySet())
164-
// );
165-
// }
166-
// return (
167-
// prefix +
168-
// EagerReconstructionUtils.wrapInChildScope(
169-
// secondRunResult.getResult().toString(true),
170-
// interpreter
171-
// )
172-
// );
173-
// }
174110
}
175111
return result.getResult().toString(true);
176112
}
177113

178-
private EagerExecutionResult runLoopOnce(
114+
private String getEvaluationResultDirectly(
179115
Map<String, Object> argMap,
180116
Map<String, Object> kwargMap,
181117
List<Object> varArgs,
182118
JinjavaInterpreter interpreter
119+
) {
120+
String evaluationResult = getEvaluationResult(argMap, kwargMap, varArgs, interpreter);
121+
interpreter.getContext().getScope().remove(KWARGS_KEY);
122+
interpreter.getContext().getScope().remove(VARARGS_KEY);
123+
return evaluationResult;
124+
}
125+
126+
private EagerExecutionResult eagerEvaluate(
127+
Supplier<String> stringSupplier,
128+
JinjavaInterpreter interpreter
129+
) {
130+
return eagerEvaluate(
131+
stringSupplier,
132+
EagerChildContextConfig
133+
.newBuilder()
134+
.withCheckForContextChanges(!interpreter.getContext().isDeferredExecutionMode())
135+
.build(),
136+
interpreter
137+
);
138+
}
139+
140+
private EagerExecutionResult eagerEvaluateInDeferredExecutionMode(
141+
Supplier<String> stringSupplier,
142+
JinjavaInterpreter interpreter
143+
) {
144+
return eagerEvaluate(
145+
stringSupplier,
146+
EagerChildContextConfig
147+
.newBuilder()
148+
.withForceDeferredExecutionMode(true)
149+
.withTakeNewValue(true)
150+
.withCheckForContextChanges(true)
151+
.build(),
152+
interpreter
153+
);
154+
}
155+
156+
private EagerExecutionResult eagerEvaluate(
157+
Supplier<String> stringSupplier,
158+
EagerChildContextConfig eagerChildContextConfig,
159+
JinjavaInterpreter interpreter
183160
) {
184161
return EagerContextWatcher.executeInChildContext(
185162
eagerInterpreter ->
186-
EagerExpressionResult.fromSupplier(
187-
() -> super.doEvaluate(argMap, kwargMap, varArgs).toString(),
188-
eagerInterpreter
189-
),
163+
EagerExpressionResult.fromSupplier(stringSupplier, eagerInterpreter),
190164
interpreter,
191-
EagerContextWatcher
192-
.EagerChildContextConfig.newBuilder()
193-
.withForceDeferredExecutionMode(true)
194-
.build()
165+
eagerChildContextConfig
195166
);
196167
}
197168

@@ -292,16 +263,6 @@ public String reconstructImage(String fullName) {
292263
String evaluation = (String) evaluate(
293264
getArguments().stream().map(arg -> DeferredMacroValueImpl.instance()).toArray()
294265
);
295-
296-
if (!interpreter.getContext().getDeferredTokens().isEmpty()) {
297-
evaluation =
298-
(String) evaluate(
299-
getArguments()
300-
.stream()
301-
.map(arg -> DeferredMacroValueImpl.instance())
302-
.toArray()
303-
);
304-
}
305266
result =
306267
(getStartTag(fullName, interpreter) + evaluation + getEndTag(interpreter));
307268
} catch (DeferredValueException e) {

‎src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerBlockSetTagStrategy.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ protected EagerExecutionResult getEagerExecutionResult(
4242
EagerContextWatcher
4343
.EagerChildContextConfig.newBuilder()
4444
.withTakeNewValue(true)
45+
.withDiscardSessionBindings(!SetTag.IGNORED_VARIABLE_NAME.equals(variables[0]))
4546
.build()
4647
);
4748
if (result.getResult().getResolutionState() == ResolutionState.NONE) {

‎src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerDoTag.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import com.hubspot.jinjava.util.EagerContextWatcher;
1010
import com.hubspot.jinjava.util.EagerExpressionResolver.EagerExpressionResult;
1111
import com.hubspot.jinjava.util.EagerReconstructionUtils;
12+
import com.hubspot.jinjava.util.PrefixToPreserveState;
1213

1314
public class EagerDoTag extends EagerStateChangingTag<DoTag> implements FlexibleTag {
1415

@@ -40,9 +41,9 @@ public String eagerInterpret(
4041
.withCheckForContextChanges(!interpreter.getContext().isDeferredExecutionMode())
4142
.build()
4243
);
43-
StringBuilder prefixToPreserveState = new StringBuilder();
44+
PrefixToPreserveState prefixToPreserveState = new PrefixToPreserveState();
4445
if (interpreter.getContext().isDeferredExecutionMode()) {
45-
prefixToPreserveState.append(eagerExecutionResult.getPrefixToPreserveState());
46+
prefixToPreserveState.withAll(eagerExecutionResult.getPrefixToPreserveState());
4647
} else {
4748
interpreter.getContext().putAll(eagerExecutionResult.getSpeculativeBindings());
4849
}

‎src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerExecutionResult.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,6 @@ public PrefixToPreserveState getPrefixToPreserveState(boolean registerDeferredTo
124124
}
125125

126126
public String asTemplateString() {
127-
return getPrefixToPreserveState().toString() + result;
127+
return getPrefixToPreserveState().toString() + result.toString(true);
128128
}
129129
}

‎src/test/java/com/hubspot/jinjava/EagerTest.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1290,12 +1290,17 @@ public void itKeepsMacroModificationsInScopeSecondPass() {
12901290
}
12911291

12921292
@Test
1293-
public void itE() {
1294-
expectedTemplateInterpreter.assertExpectedOutput("e");
1293+
public void itDoesNotReconstructVariableInWrongScope() {
1294+
expectedTemplateInterpreter.assertExpectedOutput(
1295+
"does-not-reconstruct-variable-in-wrong-scope"
1296+
);
12951297
}
12961298

12971299
@Test
1298-
public void itE2() {
1299-
expectedTemplateInterpreter.assertExpectedOutput("e2");
1300+
public void itDoesNotReconstructVariableInWrongScopeSecondPass() {
1301+
interpreter.getContext().put("deferred", true);
1302+
expectedTemplateInterpreter.assertExpectedNonEagerOutput(
1303+
"does-not-reconstruct-variable-in-wrong-scope.expected"
1304+
);
13001305
}
13011306
}

0 commit comments

Comments
 (0)