Skip to content

Commit c392aac

Browse files
jasmith-hsjasmith-hs
authored
Merge pull request #1205 from HubSpot/fix-macro-child-scope-reconstruction
Overhaul current path reconstruction in eager execution
2 parents 11926b5 + bc15907 commit c392aac

46 files changed

Lines changed: 509 additions & 336 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎src/main/java/com/hubspot/jinjava/el/ext/eager/EvalResultHolder.java‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy;
66
import com.hubspot.jinjava.interpret.DeferredValueException;
77
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
8+
import com.hubspot.jinjava.interpret.MetaContextVariables;
89
import com.hubspot.jinjava.interpret.PartiallyDeferredValue;
910
import com.hubspot.jinjava.util.EagerExpressionResolver;
1011
import de.odysseus.el.tree.Bindings;
@@ -125,11 +126,12 @@ static String reconstructNode(
125126
if (astNode instanceof AstIdentifier) {
126127
String name = ((AstIdentifier) astNode).getName();
127128
if (
128-
((JinjavaInterpreter) context
129-
.getELResolver()
130-
.getValue(context, null, ExtendedParser.INTERPRETER)).getContext()
131-
.getComputedMetaContextVariables()
132-
.contains(name)
129+
MetaContextVariables.isMetaContextVariable(
130+
name,
131+
((JinjavaInterpreter) context
132+
.getELResolver()
133+
.getValue(context, null, ExtendedParser.INTERPRETER)).getContext()
134+
)
133135
) {
134136
return name;
135137
}

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -338,14 +338,17 @@ public void addResolvedFunction(String function) {
338338
}
339339
}
340340

341+
/**
342+
* @deprecated Use {@link MetaContextVariables#isMetaContextVariable(String, Context)}
343+
*/
341344
@Deprecated
342345
@Beta
343346
public Set<String> getMetaContextVariables() {
344347
return metaContextVariables;
345348
}
346349

347350
@Beta
348-
public Set<String> getComputedMetaContextVariables() {
351+
Set<String> getComputedMetaContextVariables() {
349352
return Sets.difference(metaContextVariables, overriddenNonMetaContextVariables);
350353
}
351354

@@ -354,6 +357,10 @@ public void addMetaContextVariables(Collection<String> variables) {
354357
metaContextVariables.addAll(variables);
355358
}
356359

360+
Set<String> getNonMetaContextVariables() {
361+
return overriddenNonMetaContextVariables;
362+
}
363+
357364
@Beta
358365
public void addNonMetaContextVariables(Collection<String> variables) {
359366
overriddenNonMetaContextVariables.addAll(

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
package com.hubspot.jinjava.interpret;
2+
3+
import com.google.common.annotations.Beta;
4+
import java.util.Objects;
5+
6+
@Beta
7+
public class MetaContextVariables {
8+
9+
public static final String TEMPORARY_META_CONTEXT_PREFIX = "__temp_meta_";
10+
private static final String TEMPORARY_IMPORT_ALIAS_PREFIX =
11+
TEMPORARY_META_CONTEXT_PREFIX + "import_alias_";
12+
13+
private static final String TEMPORARY_IMPORT_ALIAS_FORMAT =
14+
TEMPORARY_IMPORT_ALIAS_PREFIX + "%d__";
15+
private static final String TEMP_CURRENT_PATH_PREFIX =
16+
TEMPORARY_META_CONTEXT_PREFIX + "current_path_";
17+
private static final String TEMP_CURRENT_PATH_FORMAT =
18+
TEMP_CURRENT_PATH_PREFIX + "%d__";
19+
20+
public static boolean isMetaContextVariable(String varName, Context context) {
21+
if (isTemporaryMetaContextVariable(varName)) {
22+
return true;
23+
}
24+
return (
25+
context.getMetaContextVariables().contains(varName) &&
26+
!context.getNonMetaContextVariables().contains(varName)
27+
);
28+
}
29+
30+
private static boolean isTemporaryMetaContextVariable(String varName) {
31+
return varName.startsWith(TEMPORARY_META_CONTEXT_PREFIX);
32+
}
33+
34+
public static boolean isTemporaryImportAlias(String varName) {
35+
// This is just faster than checking a regex
36+
return varName.startsWith(TEMPORARY_IMPORT_ALIAS_PREFIX);
37+
}
38+
39+
public static String getTemporaryImportAlias(String fullAlias) {
40+
return String.format(
41+
TEMPORARY_IMPORT_ALIAS_FORMAT,
42+
Math.abs(Objects.hashCode(fullAlias))
43+
);
44+
}
45+
46+
public static String getTemporaryCurrentPathVarName(String newPath) {
47+
return String.format(
48+
TEMP_CURRENT_PATH_FORMAT,
49+
Math.abs(Objects.hash(newPath, TEMPORARY_META_CONTEXT_PREFIX) >> 1)
50+
);
51+
}
52+
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,11 @@ public String getEvaluationResult(
127127
);
128128
} else {
129129
if (!alreadyDeferredInEarlierCall(scopeEntry.getKey(), interpreter)) {
130+
if (
131+
interpreter.getContext().get(scopeEntry.getKey()) == scopeEntry.getValue()
132+
) {
133+
continue; // don't override if it's the same object
134+
}
130135
interpreter.getContext().put(scopeEntry.getKey(), scopeEntry.getValue());
131136
}
132137
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,12 @@ public Object doEvaluate(
126126
);
127127
throw new DeferredInvocationResolutionException(tempVarName);
128128
}
129+
if (!eagerExecutionResult.getResult().isFullyResolved()) {
130+
return EagerReconstructionUtils.wrapInChildScope(
131+
eagerExecutionResult.getResult().toString(true),
132+
interpreter
133+
);
134+
}
129135
return eagerExecutionResult.getResult().toString(true);
130136
}
131137

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import com.hubspot.jinjava.interpret.DeferredValue;
1111
import com.hubspot.jinjava.interpret.DeferredValueShadow;
1212
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
13+
import com.hubspot.jinjava.interpret.MetaContextVariables;
1314
import com.hubspot.jinjava.tree.parse.Token;
1415
import com.hubspot.jinjava.tree.parse.TokenScannerSymbols;
1516
import com.hubspot.jinjava.util.EagerExpressionResolver;
@@ -376,7 +377,7 @@ private static Collection<String> markDeferredWordsAndFindSources(
376377
}
377378
return !(val instanceof DeferredValue);
378379
})
379-
.filter(prop -> !context.getComputedMetaContextVariables().contains(prop))
380+
.filter(prop -> !MetaContextVariables.isMetaContextVariable(prop, context))
380381
.filter(prop -> {
381382
DeferredValue deferredValue = convertToDeferredValue(context, prop);
382383
context.put(prop, deferredValue);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ protected Triple<String, String, String> getPrefixTokenAndSuffix(
170170
.build()
171171
)
172172
);
173-
String suffixToPreserveState = getSuffixToPreserveState(variables[0], interpreter);
173+
String suffixToPreserveState = getSuffixToPreserveState(variables, interpreter);
174174
return Triple.of(
175175
prefixToPreserveState.toString(),
176176
joiner.toString(),

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,6 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter
3838
importingData
3939
);
4040

41-
final String newPathSetter;
42-
4341
Optional<String> maybeTemplateFile;
4442
try {
4543
maybeTemplateFile =
@@ -53,7 +51,6 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter
5351
String templateFile = maybeTemplateFile.get();
5452
try {
5553
Node node = ImportTag.parseTemplateAsNode(interpreter, templateFile);
56-
newPathSetter = EagerImportingStrategyFactory.getSetTagForCurrentPath(interpreter);
5754

5855
JinjavaInterpreter child = interpreter
5956
.getConfig()
@@ -96,7 +93,11 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter
9693
return "";
9794
}
9895
return EagerReconstructionUtils.wrapInTag(
99-
eagerImportingStrategy.getFinalOutput(newPathSetter, output, child),
96+
EagerReconstructionUtils.wrapPathAroundText(
97+
eagerImportingStrategy.getFinalOutput(output, child),
98+
templateFile,
99+
interpreter
100+
),
100101
DoTag.TAG_NAME,
101102
interpreter,
102103
true

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

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@
33
import com.google.common.annotations.Beta;
44
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
55
import com.hubspot.jinjava.lib.tag.IncludeTag;
6-
import com.hubspot.jinjava.lib.tag.eager.importing.EagerImportingStrategyFactory;
7-
import com.hubspot.jinjava.loader.RelativePathResolver;
86
import com.hubspot.jinjava.tree.TagNode;
97
import com.hubspot.jinjava.util.EagerReconstructionUtils;
108
import com.hubspot.jinjava.util.HelperStringTokenizer;
@@ -30,14 +28,11 @@ public String innerInterpret(TagNode tagNode, JinjavaInterpreter interpreter) {
3028
tagNode.getStartPosition()
3129
);
3230
templateFile = interpreter.resolveResourceLocation(templateFile);
33-
final String initialPathSetter =
34-
EagerImportingStrategyFactory.getSetTagForCurrentPath(interpreter);
35-
final String newPathSetter = EagerReconstructionUtils.buildBlockOrInlineSetTag(
36-
RelativePathResolver.CURRENT_PATH_CONTEXT_KEY,
31+
return EagerReconstructionUtils.wrapPathAroundText(
32+
output,
3733
templateFile,
3834
interpreter
3935
);
40-
return newPathSetter + output + initialPathSetter;
4136
}
4237
return output;
4338
}

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,10 +101,7 @@ public Triple<String, String, String> getPrefixTokenAndSuffix(
101101
.build()
102102
)
103103
);
104-
String suffixToPreserveState = getSuffixToPreserveState(
105-
String.join(",", Arrays.asList(variables)),
106-
interpreter
107-
);
104+
String suffixToPreserveState = getSuffixToPreserveState(variables, interpreter);
108105
return Triple.of(
109106
prefixToPreserveState.toString(),
110107
joiner.toString(),

0 commit comments

Comments
 (0)