Merge pull request #1205 from HubSpot/fix-macro-child-scope-reconstru…

…ction

Overhaul current path reconstruction in eager execution

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

@@ -5,6 +5,7 @@
 import com.hubspot.jinjava.el.ext.IdentifierPreservationStrategy;
 import com.hubspot.jinjava.interpret.DeferredValueException;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
+import com.hubspot.jinjava.interpret.MetaContextVariables;
 import com.hubspot.jinjava.interpret.PartiallyDeferredValue;
 import com.hubspot.jinjava.util.EagerExpressionResolver;
 import de.odysseus.el.tree.Bindings;
@@ -125,11 +126,12 @@ static String reconstructNode(
         if (astNode instanceof AstIdentifier) {
           String name = ((AstIdentifier) astNode).getName();
           if (
-            ((JinjavaInterpreter) context
-                .getELResolver()
-                .getValue(context, null, ExtendedParser.INTERPRETER)).getContext()
-              .getComputedMetaContextVariables()
-              .contains(name)
+            MetaContextVariables.isMetaContextVariable(
+              name,
+              ((JinjavaInterpreter) context
+                  .getELResolver()
+                  .getValue(context, null, ExtendedParser.INTERPRETER)).getContext()
+            )
           ) {
             return name;
           }

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

@@ -338,14 +338,17 @@ public void addResolvedFunction(String function) {
     }
   }
 
+  /**
+   * @deprecated Use {@link MetaContextVariables#isMetaContextVariable(String, Context)}
+   */
   @Deprecated
   @Beta
   public Set<String> getMetaContextVariables() {
     return metaContextVariables;
   }
 
   @Beta
-  public Set<String> getComputedMetaContextVariables() {
+  Set<String> getComputedMetaContextVariables() {
     return Sets.difference(metaContextVariables, overriddenNonMetaContextVariables);
   }
 
@@ -354,6 +357,10 @@ public void addMetaContextVariables(Collection<String> variables) {
     metaContextVariables.addAll(variables);
   }
 
+  Set<String> getNonMetaContextVariables() {
+    return overriddenNonMetaContextVariables;
+  }
+
   @Beta
   public void addNonMetaContextVariables(Collection<String> variables) {
     overriddenNonMetaContextVariables.addAll(

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

@@ -0,0 +1,52 @@
+package com.hubspot.jinjava.interpret;
+
+import com.google.common.annotations.Beta;
+import java.util.Objects;
+
+@Beta
+public class MetaContextVariables {
+
+  public static final String TEMPORARY_META_CONTEXT_PREFIX = "__temp_meta_";
+  private static final String TEMPORARY_IMPORT_ALIAS_PREFIX =
+    TEMPORARY_META_CONTEXT_PREFIX + "import_alias_";
+
+  private static final String TEMPORARY_IMPORT_ALIAS_FORMAT =
+    TEMPORARY_IMPORT_ALIAS_PREFIX + "%d__";
+  private static final String TEMP_CURRENT_PATH_PREFIX =
+    TEMPORARY_META_CONTEXT_PREFIX + "current_path_";
+  private static final String TEMP_CURRENT_PATH_FORMAT =
+    TEMP_CURRENT_PATH_PREFIX + "%d__";
+
+  public static boolean isMetaContextVariable(String varName, Context context) {
+    if (isTemporaryMetaContextVariable(varName)) {
+      return true;
+    }
+    return (
+      context.getMetaContextVariables().contains(varName) &&
+      !context.getNonMetaContextVariables().contains(varName)
+    );
+  }
+
+  private static boolean isTemporaryMetaContextVariable(String varName) {
+    return varName.startsWith(TEMPORARY_META_CONTEXT_PREFIX);
+  }
+
+  public static boolean isTemporaryImportAlias(String varName) {
+    // This is just faster than checking a regex
+    return varName.startsWith(TEMPORARY_IMPORT_ALIAS_PREFIX);
+  }
+
+  public static String getTemporaryImportAlias(String fullAlias) {
+    return String.format(
+      TEMPORARY_IMPORT_ALIAS_FORMAT,
+      Math.abs(Objects.hashCode(fullAlias))
+    );
+  }
+
+  public static String getTemporaryCurrentPathVarName(String newPath) {
+    return String.format(
+      TEMP_CURRENT_PATH_FORMAT,
+      Math.abs(Objects.hash(newPath, TEMPORARY_META_CONTEXT_PREFIX) >> 1)
+    );
+  }
+}

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

@@ -127,6 +127,11 @@ public String getEvaluationResult(
             );
         } else {
           if (!alreadyDeferredInEarlierCall(scopeEntry.getKey(), interpreter)) {
+            if (
+              interpreter.getContext().get(scopeEntry.getKey()) == scopeEntry.getValue()
+            ) {
+              continue; // don't override if it's the same object
+            }
             interpreter.getContext().put(scopeEntry.getKey(), scopeEntry.getValue());
           }
         }

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

@@ -126,6 +126,12 @@ public Object doEvaluate(
         );
       throw new DeferredInvocationResolutionException(tempVarName);
     }
+    if (!eagerExecutionResult.getResult().isFullyResolved()) {
+      return EagerReconstructionUtils.wrapInChildScope(
+        eagerExecutionResult.getResult().toString(true),
+        interpreter
+      );
+    }
     return eagerExecutionResult.getResult().toString(true);
   }
 

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

@@ -10,6 +10,7 @@
 import com.hubspot.jinjava.interpret.DeferredValue;
 import com.hubspot.jinjava.interpret.DeferredValueShadow;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
+import com.hubspot.jinjava.interpret.MetaContextVariables;
 import com.hubspot.jinjava.tree.parse.Token;
 import com.hubspot.jinjava.tree.parse.TokenScannerSymbols;
 import com.hubspot.jinjava.util.EagerExpressionResolver;
@@ -376,7 +377,7 @@ private static Collection<String> markDeferredWordsAndFindSources(
         }
         return !(val instanceof DeferredValue);
       })
-      .filter(prop -> !context.getComputedMetaContextVariables().contains(prop))
+      .filter(prop -> !MetaContextVariables.isMetaContextVariable(prop, context))
       .filter(prop -> {
         DeferredValue deferredValue = convertToDeferredValue(context, prop);
         context.put(prop, deferredValue);

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

@@ -170,7 +170,7 @@ protected Triple<String, String, String> getPrefixTokenAndSuffix(
             .build()
         )
       );
-    String suffixToPreserveState = getSuffixToPreserveState(variables[0], interpreter);
+    String suffixToPreserveState = getSuffixToPreserveState(variables, interpreter);
     return Triple.of(
       prefixToPreserveState.toString(),
       joiner.toString(),

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

@@ -38,8 +38,6 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter
       importingData
     );
 
-    final String newPathSetter;
-
     Optional<String> maybeTemplateFile;
     try {
       maybeTemplateFile =
@@ -53,7 +51,6 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter
     String templateFile = maybeTemplateFile.get();
     try {
       Node node = ImportTag.parseTemplateAsNode(interpreter, templateFile);
-      newPathSetter = EagerImportingStrategyFactory.getSetTagForCurrentPath(interpreter);
 
       JinjavaInterpreter child = interpreter
         .getConfig()
@@ -96,7 +93,11 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter
         return "";
       }
       return EagerReconstructionUtils.wrapInTag(
-        eagerImportingStrategy.getFinalOutput(newPathSetter, output, child),
+        EagerReconstructionUtils.wrapPathAroundText(
+          eagerImportingStrategy.getFinalOutput(output, child),
+          templateFile,
+          interpreter
+        ),
         DoTag.TAG_NAME,
         interpreter,
         true

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

@@ -3,8 +3,6 @@
 import com.google.common.annotations.Beta;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
 import com.hubspot.jinjava.lib.tag.IncludeTag;
-import com.hubspot.jinjava.lib.tag.eager.importing.EagerImportingStrategyFactory;
-import com.hubspot.jinjava.loader.RelativePathResolver;
 import com.hubspot.jinjava.tree.TagNode;
 import com.hubspot.jinjava.util.EagerReconstructionUtils;
 import com.hubspot.jinjava.util.HelperStringTokenizer;
@@ -30,14 +28,11 @@ public String innerInterpret(TagNode tagNode, JinjavaInterpreter interpreter) {
         tagNode.getStartPosition()
       );
       templateFile = interpreter.resolveResourceLocation(templateFile);
-      final String initialPathSetter =
-        EagerImportingStrategyFactory.getSetTagForCurrentPath(interpreter);
-      final String newPathSetter = EagerReconstructionUtils.buildBlockOrInlineSetTag(
-        RelativePathResolver.CURRENT_PATH_CONTEXT_KEY,
+      return EagerReconstructionUtils.wrapPathAroundText(
+        output,
         templateFile,
         interpreter
       );
-      return newPathSetter + output + initialPathSetter;
     }
     return output;
   }

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

@@ -101,10 +101,7 @@ public Triple<String, String, String> getPrefixTokenAndSuffix(
           .build()
       )
     );
-    String suffixToPreserveState = getSuffixToPreserveState(
-      String.join(",", Arrays.asList(variables)),
-      interpreter
-    );
+    String suffixToPreserveState = getSuffixToPreserveState(variables, interpreter);
     return Triple.of(
       prefixToPreserveState.toString(),
       joiner.toString(),

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

@@ -3,8 +3,9 @@
 import com.google.common.annotations.Beta;
 import com.hubspot.jinjava.interpret.DeferredValueException;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
+import com.hubspot.jinjava.interpret.MetaContextVariables;
 import com.hubspot.jinjava.lib.tag.SetTag;
-import com.hubspot.jinjava.lib.tag.eager.importing.AliasedEagerImportingStrategy;
+import com.hubspot.jinjava.loader.RelativePathResolver;
 import com.hubspot.jinjava.tree.TagNode;
 import com.hubspot.jinjava.util.EagerReconstructionUtils;
 import com.hubspot.jinjava.util.PrefixToPreserveState;
@@ -13,6 +14,7 @@
 import java.util.Optional;
 import java.util.StringJoiner;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import org.apache.commons.lang3.tuple.Triple;
 
 @Beta
@@ -52,9 +54,14 @@ public String run(TagNode tagNode, JinjavaInterpreter interpreter) {
       expression,
       interpreter
     );
+    boolean triedResolve = false;
     if (
       eagerExecutionResult.getResult().isFullyResolved() &&
-      !interpreter.getContext().isDeferredExecutionMode()
+      !interpreter.getContext().isDeferredExecutionMode() &&
+      (Arrays
+          .stream(variables)
+          .noneMatch(RelativePathResolver.CURRENT_PATH_CONTEXT_KEY::equals) ||
+        interpreter.getContext().getPenultimateParent().getDeferredTokens().isEmpty()) // Prevents set tags from disappearing in nested interpretation
     ) {
       EagerReconstructionUtils.commitSpeculativeBindings(
         interpreter,
@@ -66,6 +73,7 @@ public String run(TagNode tagNode, JinjavaInterpreter interpreter) {
         eagerExecutionResult,
         interpreter
       );
+      triedResolve = true;
       if (maybeResolved.isPresent()) {
         return maybeResolved.get();
       }
@@ -76,10 +84,7 @@ public String run(TagNode tagNode, JinjavaInterpreter interpreter) {
       eagerExecutionResult,
       interpreter
     );
-    if (
-      eagerExecutionResult.getResult().isFullyResolved() &&
-      interpreter.getContext().isDeferredExecutionMode()
-    ) {
+    if (eagerExecutionResult.getResult().isFullyResolved() && !triedResolve) {
       attemptResolve(tagNode, variables, eagerExecutionResult, interpreter);
     }
     return buildImage(tagNode, variables, eagerExecutionResult, triple, interpreter);
@@ -155,39 +160,66 @@ protected PrefixToPreserveState getPrefixToPreserveState(
   }
 
   public static String getSuffixToPreserveState(
-    String variables,
+    List<String> varList,
+    JinjavaInterpreter interpreter
+  ) {
+    if (varList.isEmpty()) {
+      return "";
+    }
+    return getSuffixToPreserveState(varList.stream(), interpreter);
+  }
+
+  public static String getSuffixToPreserveState(
+    String[] varList,
     JinjavaInterpreter interpreter
   ) {
-    if (variables.isEmpty()) {
+    if (varList.length == 0) {
       return "";
     }
+    return getSuffixToPreserveState(Arrays.stream(varList), interpreter);
+  }
+
+  private static String getSuffixToPreserveState(
+    Stream<String> varStream,
+    JinjavaInterpreter interpreter
+  ) {
     StringBuilder suffixToPreserveState = new StringBuilder();
-    Optional<String> maybeTemporaryImportAlias =
-      AliasedEagerImportingStrategy.getTemporaryImportAlias(interpreter.getContext());
-    if (
-      maybeTemporaryImportAlias.isPresent() &&
-      !AliasedEagerImportingStrategy.isTemporaryImportAlias(variables) &&
-      !interpreter.getContext().getComputedMetaContextVariables().contains(variables)
-    ) {
-      if (!interpreter.getContext().containsKey(maybeTemporaryImportAlias.get())) {
-        if (
-          interpreter.retraceVariable(
-            String.format(
-              "%s.%s",
-              interpreter.getContext().getImportResourceAlias().get(),
-              variables
-            ),
-            -1
-          ) !=
-          null
-        ) {
-          throw new DeferredValueException(
-            "Cannot modify temporary import alias outside of import tag"
-          );
-        }
+    Optional<String> maybeTemporaryImportAlias = interpreter
+      .getContext()
+      .getImportResourceAlias()
+      .map(MetaContextVariables::getTemporaryImportAlias);
+    if (maybeTemporaryImportAlias.isPresent()) {
+      boolean stillInsideImportTag = interpreter
+        .getContext()
+        .containsKey(maybeTemporaryImportAlias.get());
+      List<String> filteredVars = varStream
+        .filter(var ->
+          !MetaContextVariables.isMetaContextVariable(var, interpreter.getContext())
+        )
+        .peek(var -> {
+          if (!stillInsideImportTag) {
+            if (
+              interpreter.retraceVariable(
+                String.format(
+                  "%s.%s",
+                  interpreter.getContext().getImportResourceAlias().get(),
+                  var
+                ),
+                -1
+              ) !=
+              null
+            ) {
+              throw new DeferredValueException(
+                "Cannot modify temporary import alias outside of import tag"
+              );
+            }
+          }
+        })
+        .collect(Collectors.toList());
+      if (filteredVars.isEmpty()) {
+        return "";
       }
-      String updateString = getUpdateString(variables);
-
+      String updateString = getUpdateString(filteredVars);
       // Don't need to render because the temporary import alias's value is always deferred, and rendering will do nothing
       suffixToPreserveState.append(
         EagerReconstructionUtils.buildDoUpdateTag(
@@ -197,15 +229,10 @@ public static String getSuffixToPreserveState(
         )
       );
     }
-
     return suffixToPreserveState.toString();
   }
 
-  private static String getUpdateString(String variables) {
-    List<String> varList = Arrays
-      .stream(variables.split(","))
-      .map(String::trim)
-      .collect(Collectors.toList());
+  private static String getUpdateString(List<String> varList) {
     StringJoiner updateString = new StringJoiner(",");
     // Update the alias map to the value of the set variable.
     varList.forEach(var -> updateString.add(String.format("'%s': %s", var, var)));