Skip to content

Commit c1dbba8

Browse files
jasmith-hsjasmith-hs
authored
Merge pull request #1081 from HubSpot/fix-new-values-in-do-tag-not-takes
Fix EagerDoTag not committing results when partially evaluated
2 parents a573690 + fa073c9 commit c1dbba8

14 files changed

Lines changed: 125 additions & 45 deletions

‎src/main/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategy.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,10 @@ private String eagerResolveExpression(
5656
) {
5757
prefixToPreserveState.putAll(eagerExecutionResult.getPrefixToPreserveState());
5858
} else {
59-
interpreter.getContext().putAll(eagerExecutionResult.getSpeculativeBindings());
59+
EagerReconstructionUtils.commitSpeculativeBindings(
60+
interpreter,
61+
eagerExecutionResult
62+
);
6063
}
6164
if (eagerExecutionResult.getResult().isFullyResolved()) {
6265
String result = eagerExecutionResult.getResult().toString(true);

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,10 @@ public String eagerInterpret(
7474
) {
7575
prefixToPreserveState.putAll(eagerExecutionResult.getPrefixToPreserveState());
7676
} else {
77-
interpreter.getContext().putAll(eagerExecutionResult.getSpeculativeBindings());
77+
EagerReconstructionUtils.commitSpeculativeBindings(
78+
interpreter,
79+
eagerExecutionResult
80+
);
7881
}
7982
if (eagerExecutionResult.getResult().isFullyResolved()) {
8083
// Possible macro/set tag in front of this one.

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,10 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter
6464
) {
6565
prefixToPreserveState.putAll(eagerExecutionResult.getPrefixToPreserveState());
6666
} else {
67-
interpreter.getContext().putAll(eagerExecutionResult.getSpeculativeBindings());
67+
EagerReconstructionUtils.commitSpeculativeBindings(
68+
interpreter,
69+
eagerExecutionResult
70+
);
6871
}
6972
String resolvedExpression;
7073
List<String> resolvedValues; // can only be retrieved if the EagerExpressionResult are fully resolved.

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,13 @@ public String eagerInterpret(
4343
.build()
4444
);
4545
PrefixToPreserveState prefixToPreserveState = new PrefixToPreserveState();
46-
if (
47-
!eagerExecutionResult.getResult().isFullyResolved() ||
48-
interpreter.getContext().isDeferredExecutionMode()
49-
) {
46+
if (interpreter.getContext().isDeferredExecutionMode()) {
5047
prefixToPreserveState.withAll(eagerExecutionResult.getPrefixToPreserveState());
5148
} else {
52-
interpreter.getContext().putAll(eagerExecutionResult.getSpeculativeBindings());
49+
EagerReconstructionUtils.commitSpeculativeBindings(
50+
interpreter,
51+
eagerExecutionResult
52+
);
5353
}
5454
if (eagerExecutionResult.getResult().isFullyResolved()) {
5555
return (prefixToPreserveState.toString());

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,10 @@ public static String interpretExpression(
7171
) {
7272
prefixToPreserveState.putAll(eagerExecutionResult.getPrefixToPreserveState());
7373
} else {
74-
interpreter.getContext().putAll(eagerExecutionResult.getSpeculativeBindings());
74+
EagerReconstructionUtils.commitSpeculativeBindings(
75+
interpreter,
76+
eagerExecutionResult
77+
);
7578
}
7679
if (eagerExecutionResult.getResult().isFullyResolved()) {
7780
// Possible macro/set tag in front of this one.

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,10 @@ public String run(TagNode tagNode, JinjavaInterpreter interpreter) {
5454
eagerExecutionResult.getResult().isFullyResolved() &&
5555
!interpreter.getContext().isDeferredExecutionMode()
5656
) {
57-
interpreter.getContext().putAll(eagerExecutionResult.getSpeculativeBindings());
57+
EagerReconstructionUtils.commitSpeculativeBindings(
58+
interpreter,
59+
eagerExecutionResult
60+
);
5861
Optional<String> maybeResolved = resolveSet(
5962
tagNode,
6063
variables,
@@ -128,7 +131,10 @@ protected PrefixToPreserveState getPrefixToPreserveState(
128131
) {
129132
prefixToPreserveState.putAll(eagerExecutionResult.getPrefixToPreserveState());
130133
} else {
131-
interpreter.getContext().putAll(eagerExecutionResult.getSpeculativeBindings());
134+
EagerReconstructionUtils.commitSpeculativeBindings(
135+
interpreter,
136+
eagerExecutionResult
137+
);
132138
}
133139
prefixToPreserveState.putAll(
134140
EagerReconstructionUtils.reconstructFromContextBeforeDeferringAsMap(

‎src/main/java/com/hubspot/jinjava/util/EagerContextWatcher.java‎

Lines changed: 21 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -209,12 +209,7 @@ private static Map<String, Object> getBasicSpeculativeBindings(
209209
.peek(
210210
entry -> ((OneTimeReconstructible) entry.getValue()).setReconstructed(true)
211211
)
212-
.collect(
213-
Collectors.toMap(
214-
Entry::getKey,
215-
entry -> ((OneTimeReconstructible) entry.getValue()).getOriginalValue()
216-
)
217-
)
212+
.collect(Collectors.toMap(Entry::getKey, Entry::getValue))
218213
);
219214
}
220215
return eagerExecutionResult
@@ -226,15 +221,12 @@ private static Map<String, Object> getBasicSpeculativeBindings(
226221
.map(
227222
entry -> {
228223
if (
229-
(
230-
eagerExecutionResult.getResult().isFullyResolved() ||
231-
eagerChildContextConfig.takeNewValue
232-
) &&
233-
!(entry.getValue() instanceof DeferredValue) &&
234-
entry.getValue() != null
224+
eagerExecutionResult.getResult().isFullyResolved() ||
225+
eagerChildContextConfig.takeNewValue
235226
) {
236227
return entry;
237228
}
229+
238230
Object contextValue = interpreter.getContext().get(entry.getKey());
239231
if (
240232
contextValue instanceof DeferredValue &&
@@ -248,24 +240,15 @@ private static Map<String, Object> getBasicSpeculativeBindings(
248240
) {
249241
throw new CannotReconstructValueException(entry.getKey());
250242
}
251-
return new AbstractMap.SimpleImmutableEntry<>(
252-
entry.getKey(),
253-
((DeferredValue) contextValue).getOriginalValue()
254-
);
243+
return new AbstractMap.SimpleImmutableEntry<>(entry.getKey(), contextValue);
255244
}
256245
return null;
257246
}
258247
)
259248
.filter(Objects::nonNull)
260-
.collect(
261-
Collectors.toMap(
262-
Entry::getKey,
263-
entry ->
264-
entry.getValue() instanceof DeferredValue
265-
? ((DeferredValue) entry.getValue()).getOriginalValue()
266-
: entry.getValue()
267-
)
268-
);
249+
.filter(entry -> entry.getValue() != null)
250+
.filter(entry -> !isDeferredWithOriginalValueNull(entry.getValue()))
251+
.collect(Collectors.toMap(Entry::getKey, Entry::getValue));
269252
}
270253

271254
private static Map<String, Object> getAllSpeculativeBindings(
@@ -325,14 +308,22 @@ private static Map<String, Object> getAllSpeculativeBindings(
325308
.entrySet()
326309
.stream()
327310
.filter(entry -> !metaContextVariables.contains(entry.getKey()))
328-
.filter(
329-
entry ->
330-
!(entry.getValue() instanceof DeferredValue) && entry.getValue() != null
331-
) // these are already set recursively
311+
.filter(entry -> entry.getValue() != null)
312+
.filter(entry -> !isDeferredWithOriginalValueNull(entry.getValue()))
332313
.collect(Collectors.toMap(Entry::getKey, Entry::getValue));
333314
return speculativeBindings;
334315
}
335316

317+
/**
318+
* This is an optimization used to filter so that we don't reconstruct unnecessary tags like {@code {% set num = null %}}
319+
* because {@code num} is already null when it hasn't been set to anything.
320+
*/
321+
private static boolean isDeferredWithOriginalValueNull(Object value) {
322+
return (
323+
value instanceof DeferredValue && ((DeferredValue) value).getOriginalValue() == null
324+
);
325+
}
326+
336327
private static void cacheRevertibleObject(
337328
JinjavaInterpreter interpreter,
338329
Map<String, Object> initiallyResolvedHashes,
@@ -373,9 +364,6 @@ private static Object getOriginalValue(
373364
boolean isFullyResolved
374365
) {
375366
if (eagerChildContextConfig.takeNewValue || isFullyResolved) {
376-
if (e.getValue() instanceof DeferredValue) {
377-
return ((DeferredValue) e.getValue()).getOriginalValue();
378-
}
379367
return e.getValue();
380368
}
381369

@@ -385,7 +373,7 @@ private static Object getOriginalValue(
385373
.get(e.getKey())
386374
.equals(getObjectOrHashCode(((DeferredValue) e.getValue()).getOriginalValue()))
387375
) {
388-
return ((DeferredValue) e.getValue()).getOriginalValue();
376+
return e.getValue();
389377
}
390378

391379
// This is necessary if a state-changing function, such as .update()

‎src/main/java/com/hubspot/jinjava/util/EagerReconstructionUtils.java‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,9 @@ public static String buildBlockOrInlineSetTag(
270270
JinjavaInterpreter interpreter,
271271
boolean registerDeferredToken
272272
) {
273+
if (value instanceof DeferredValue) {
274+
value = ((DeferredValue) value).getOriginalValue();
275+
}
273276
if (value instanceof PyishBlockSetSerializable) {
274277
return buildBlockSetTag(
275278
name,
@@ -747,7 +750,14 @@ public static Set<String> resetSpeculativeBindings(
747750
) {
748751
result
749752
.getSpeculativeBindings()
750-
.forEach((k, v) -> replace(interpreter.getContext(), k, v));
753+
.forEach(
754+
(k, v) -> {
755+
if (v instanceof DeferredValue) {
756+
v = ((DeferredValue) v).getOriginalValue();
757+
}
758+
replace(interpreter.getContext(), k, v);
759+
}
760+
);
751761
return result.getSpeculativeBindings().keySet();
752762
}
753763

@@ -763,4 +773,18 @@ private static void replace(Context context, String k, Object v) {
763773
replace(context.getParent(), k, v);
764774
}
765775
}
776+
777+
public static void commitSpeculativeBindings(
778+
JinjavaInterpreter interpreter,
779+
EagerExecutionResult result
780+
) {
781+
result
782+
.getSpeculativeBindings()
783+
.entrySet()
784+
.stream()
785+
// Filter DeferredValueShadow because these are just used to mark that a value became deferred within this scope
786+
// The original key will be a DeferredValueImpl already on its original scope
787+
.filter(entry -> !(entry.getValue() instanceof DeferredValueShadow))
788+
.forEach(entry -> interpreter.getContext().put(entry.getKey(), entry.getValue()));
789+
}
766790
}

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1365,4 +1365,29 @@ public void itAllowsDeferredLazyReferenceToGetOverriddenSecondPass() {
13651365
"allows-deferred-lazy-reference-to-get-overridden.expected"
13661366
);
13671367
}
1368+
1369+
@Test
1370+
public void itCommitsVariablesFromDoTagWhenPartiallyResolved() {
1371+
expectedTemplateInterpreter.assertExpectedOutputNonIdempotent(
1372+
"commits-variables-from-do-tag-when-partially-resolved"
1373+
);
1374+
}
1375+
1376+
@Test
1377+
public void itCommitsVariablesFromDoTagWhenPartiallyResolvedSecondPass() {
1378+
interpreter.getContext().put("deferred", "resolved");
1379+
expectedTemplateInterpreter.assertExpectedNonEagerOutput(
1380+
"commits-variables-from-do-tag-when-partially-resolved.expected"
1381+
);
1382+
expectedTemplateInterpreter.assertExpectedOutput(
1383+
"commits-variables-from-do-tag-when-partially-resolved.expected"
1384+
);
1385+
}
1386+
1387+
@Test
1388+
public void itFindsDeferredWordsInsideReconstructedString() {
1389+
expectedTemplateInterpreter.assertExpectedOutput(
1390+
"finds-deferred-words-inside-reconstructed-string"
1391+
);
1392+
}
13681393
}

‎src/test/resources/eager/commits-variables-from-do-tag-when-partially-resolved.expected.expected.jinja‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
L0: ['a']
2+
L1: ['b']
3+
L2: ['c', 'resolved']

0 commit comments

Comments
 (0)