Merge pull request #1199 from HubSpot/macro-current-path-fix

Wrap the current path around reconstructed macro functions
HubSpot · Sep 9, 2024 · 027cdcc · 027cdcc
2 parents 789cfe2 + 6497990
commit 027cdcc
Show file tree

Hide file tree

Showing 16 changed files with 146 additions and 12 deletions.
diff --git a/src/main/java/com/hubspot/jinjava/Jinjava.java b/src/main/java/com/hubspot/jinjava/Jinjava.java
@@ -35,6 +35,7 @@
 import com.hubspot.jinjava.lib.fn.ELFunctionDefinition;
 import com.hubspot.jinjava.lib.tag.Tag;
 import com.hubspot.jinjava.loader.ClasspathResourceLocator;
+import com.hubspot.jinjava.loader.RelativePathResolver;
 import com.hubspot.jinjava.loader.ResourceLocator;
 import de.odysseus.el.ExpressionFactoryImpl;
 import de.odysseus.el.misc.TypeConverter;
@@ -244,6 +245,10 @@ public RenderResult renderForResult(
     } else {
       context = new Context(copyGlobalContext(), bindings, renderConfig.getDisabled());
     }
+    Object currentPath = context.get(RelativePathResolver.CURRENT_PATH_CONTEXT_KEY);
+    if (currentPath != null) {
+      context.getCurrentPathStack().pushWithoutCycleCheck(currentPath.toString(), 0, 0);
+    }
 
     JinjavaInterpreter interpreter = globalConfig
       .getInterpreterFactory()
@@ -290,6 +295,9 @@ public RenderResult renderForResult(
       );
     } finally {
       globalContext.reset();
+      if (currentPath != null) {
+        context.getCurrentPathStack().pop();
+      }
       JinjavaInterpreter.popCurrent();
     }
   }

diff --git a/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java b/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java
@@ -1,5 +1,7 @@
 package com.hubspot.jinjava.lib.fn.eager;
 
+import static com.hubspot.jinjava.loader.RelativePathResolver.CURRENT_PATH_CONTEXT_KEY;
+
 import com.google.common.annotations.Beta;
 import com.hubspot.jinjava.el.ext.AstMacroFunction;
 import com.hubspot.jinjava.el.ext.DeferredInvocationResolutionException;
@@ -13,6 +15,7 @@
 import com.hubspot.jinjava.lib.fn.MacroFunction;
 import com.hubspot.jinjava.lib.tag.MacroTag;
 import com.hubspot.jinjava.lib.tag.eager.EagerExecutionResult;
+import com.hubspot.jinjava.lib.tag.eager.importing.EagerImportingStrategyFactory;
 import com.hubspot.jinjava.objects.serialization.PyishObjectMapper;
 import com.hubspot.jinjava.tree.Node;
 import com.hubspot.jinjava.util.EagerContextWatcher;
@@ -80,6 +83,11 @@ public Object doEvaluate(
               () -> getEvaluationResultDirectly(argMap, kwargMap, varArgs, interpreter),
               interpreter
             );
+          return wrapCurrentPathSetting(
+            interpreter,
+            importFile,
+            result.asTemplateString()
+          );
         }
         return result.asTemplateString();
       } finally {
@@ -121,12 +129,45 @@ public Object doEvaluate(
         .put(
           tempVarName,
           new MacroFunctionTempVariable(
-            prefixToPreserveState + eagerExecutionResult.asTemplateString()
+            wrapCurrentPathSetting(
+              interpreter,
+              Optional
+                .ofNullable(localContextScope.get(Context.IMPORT_RESOURCE_PATH_KEY))
+                .map(Object::toString),
+              prefixToPreserveState + eagerExecutionResult.asTemplateString()
+            )
           )
         );
       throw new DeferredInvocationResolutionException(tempVarName);
     }
-    return eagerExecutionResult.getResult().toString(true);
+    return wrapCurrentPathSetting(
+      interpreter,
+      Optional
+        .ofNullable(localContextScope.get(Context.IMPORT_RESOURCE_PATH_KEY))
+        .map(Object::toString),
+      eagerExecutionResult.getResult().toString(true)
+    );
+  }
+
+  private static String wrapCurrentPathSetting(
+    JinjavaInterpreter interpreter,
+    Optional<String> importFile,
+    String resultToWrap
+  ) {
+    if (!importFile.isPresent()) {
+      return resultToWrap;
+    }
+    final String initialPathSetter =
+      EagerImportingStrategyFactory.getSetTagForCurrentPath(interpreter);
+    final String newPathSetter = EagerReconstructionUtils.buildBlockOrInlineSetTag(
+      CURRENT_PATH_CONTEXT_KEY,
+      importFile.get(),
+      interpreter
+    );
+    if (initialPathSetter.equals(newPathSetter)) {
+      return resultToWrap;
+    }
+    return newPathSetter + resultToWrap + initialPathSetter;
   }
 
   private String getEvaluationResultDirectly(
@@ -312,6 +353,7 @@ private boolean differentMacroWithSameNameExists(JinjavaInterpreter interpreter)
       return false;
     }
     MacroFunction mostRecent = context.getGlobalMacro(getName());
+    //noinspection ErrorProne
     if (this != mostRecent) {
       return true;
     }

diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategyFactory.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategyFactory.java
@@ -34,6 +34,12 @@ public static String getSetTagForCurrentPath(JinjavaInterpreter interpreter) {
         .getContext()
         .getCurrentPathStack()
         .peek()
+        .map(c -> {
+          interpreter
+            .getContext()
+            .replace(RelativePathResolver.CURRENT_PATH_CONTEXT_KEY, c);
+          return c;
+        })
         .orElseGet(() ->
           (String) interpreter
             .getContext()

diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java
@@ -625,7 +625,7 @@ public void itDefersMacroInIf() {
 
   @Test
   public void itPutsDeferredImportedMacroInOutput() {
-    expectedTemplateInterpreter.assertExpectedOutput(
+    expectedTemplateInterpreter.assertExpectedOutputNonIdempotent(
       "puts-deferred-imported-macro-in-output"
     );
   }
@@ -643,7 +643,7 @@ public void itPutsDeferredImportedMacroInOutputSecondPass() {
 
   @Test
   public void itPutsDeferredFromedMacroInOutput() {
-    expectedTemplateInterpreter.assertExpectedOutput(
+    expectedTemplateInterpreter.assertExpectedOutputNonIdempotent(
       "puts-deferred-fromed-macro-in-output"
     );
   }
@@ -1597,4 +1597,11 @@ public void prepareContext(Context context) {
       "keeps-meta-context-variables-through-import/test"
     );
   }
+
+  @Test
+  public void itWrapsCurrentPathAroundMacro() {
+    expectedTemplateInterpreter.assertExpectedOutputNonIdempotent(
+      "wraps-current-path-around-macro/test"
+    );
+  }
 }
diff --git a/src/test/java/com/hubspot/jinjava/ExpectedTemplateInterpreter.java b/src/test/java/com/hubspot/jinjava/ExpectedTemplateInterpreter.java
@@ -15,6 +15,7 @@ public class ExpectedTemplateInterpreter {
   private Jinjava jinjava;
   private JinjavaInterpreter interpreter;
   private String path;
+  private boolean sensibleCurrentPath = false;
 
   public ExpectedTemplateInterpreter(
     Jinjava jinjava,
@@ -26,6 +27,26 @@ public ExpectedTemplateInterpreter(
     this.path = path;
   }
 
+  public static ExpectedTemplateInterpreter withSensibleCurrentPath(
+    Jinjava jinjava,
+    JinjavaInterpreter interpreter,
+    String path
+  ) {
+    return new ExpectedTemplateInterpreter(jinjava, interpreter, path, true);
+  }
+
+  private ExpectedTemplateInterpreter(
+    Jinjava jinjava,
+    JinjavaInterpreter interpreter,
+    String path,
+    boolean sensibleCurrentPath
+  ) {
+    this.jinjava = jinjava;
+    this.interpreter = interpreter;
+    this.path = path;
+    this.sensibleCurrentPath = sensibleCurrentPath;
+  }
+
   public String assertExpectedOutput(String name) {
     String template = getFixtureTemplate(name);
     String output = JinjavaInterpreter.getCurrent().render(template);
@@ -116,6 +137,13 @@ public String assertExpectedNonEagerOutput(String name) {
 
   public String getFixtureTemplate(String name) {
     try {
+      if (sensibleCurrentPath) {
+        JinjavaInterpreter
+          .getCurrent()
+          .getContext()
+          .getCurrentPathStack()
+          .push(String.format("%s/%s.jinja", path, name), 0, 0);
+      }
       return Resources.toString(
         Resources.getResource(String.format("%s/%s.jinja", path, name)),
         StandardCharsets.UTF_8
@@ -127,10 +155,12 @@ public String getFixtureTemplate(String name) {
 
   private String expected(String name) {
     try {
-      return Resources.toString(
-        Resources.getResource(String.format("%s/%s.expected.jinja", path, name)),
-        StandardCharsets.UTF_8
-      );
+      return Resources
+        .toString(
+          Resources.getResource(String.format("%s/%s.expected.jinja", path, name)),
+          StandardCharsets.UTF_8
+        )
+        .replaceAll("\\\\\n\\s*", "");
     } catch (IOException e) {
       throw new RuntimeException(e);
     }

diff --git a/src/test/java/com/hubspot/jinjava/FullSnippetsTest.java b/src/test/java/com/hubspot/jinjava/FullSnippetsTest.java
@@ -39,7 +39,7 @@ public String getString(
           JinjavaInterpreter interpreter
         ) throws IOException {
           return Resources.toString(
-            Resources.getResource(String.format("tags/macrotag/%s", fullName)),
+            Resources.getResource(relativePathResolver.resolve(fullName, interpreter)),
             StandardCharsets.UTF_8
           );
         }
@@ -66,7 +66,11 @@ public Optional<LocationResolver> getLocationResolver() {
     );
     interpreter = new JinjavaInterpreter(parentInterpreter);
     expectedTemplateInterpreter =
-      new ExpectedTemplateInterpreter(jinjava, interpreter, "snippets");
+      ExpectedTemplateInterpreter.withSensibleCurrentPath(
+        jinjava,
+        interpreter,
+        "snippets"
+      );
     localContext = interpreter.getContext();
 
     JinjavaInterpreter.pushCurrent(interpreter);
@@ -101,4 +105,11 @@ public void itUsesLowerScopeValueInMacroEvaluation() {
       "uses-lower-scope-value-in-macro-evaluation"
     );
   }
+
+  @Test
+  public void itFromTagDoesntStealExtends() {
+    expectedTemplateInterpreter.assertExpectedOutput(
+      "from-tag-doesnt-steal-extends/test"
+    );
+  }
 }
diff --git a/src/test/resources/eager/puts-deferred-fromed-macro-in-output.expected.jinja b/src/test/resources/eager/puts-deferred-fromed-macro-in-output.expected.jinja
@@ -1 +1 @@
-{% set myname = deferred + 3 %}{% set __macro_getPath_331491059_temp_variable_1__ %}Hello {{ myname }}{% endset %}{% print __macro_getPath_331491059_temp_variable_1__ %}
+{% set myname = deferred + 3 %}{% set __macro_getPath_331491059_temp_variable_1__ %}{% set current_path = 'simple-with-call.jinja' %}Hello {{ myname }}{% set current_path = '' %}{% endset %}{% print __macro_getPath_331491059_temp_variable_1__ %}
diff --git a/src/test/resources/eager/puts-deferred-imported-macro-in-output.expected.jinja b/src/test/resources/eager/puts-deferred-imported-macro-in-output.expected.jinja
@@ -1 +1 @@
-{% set myname = deferred + 3 %}{% set __macro_getPath_331491059_temp_variable_1__ %}Hello {{ myname }}{% endset %}{% print __macro_getPath_331491059_temp_variable_1__ %}
+{% set myname = deferred + 3 %}{% set __macro_getPath_331491059_temp_variable_1__ %}{% set current_path = 'simple-with-call.jinja' %}Hello {{ myname }}{% set current_path = '' %}{% endset %}{% print __macro_getPath_331491059_temp_variable_1__ %}
diff --git a/src/test/resources/eager/wraps-current-path-around-macro/dir1/macro.jinja b/src/test/resources/eager/wraps-current-path-around-macro/dir1/macro.jinja
@@ -0,0 +1,4 @@
+{% macro foo_importer() -%}
+  {%- import "../../../eager/wraps-current-path-around-macro/dir2/imported.jinja" -%}
+  {%- print foo -%}
+{%- endmacro %}
diff --git a/src/test/resources/eager/wraps-current-path-around-macro/dir2/imported.jinja b/src/test/resources/eager/wraps-current-path-around-macro/dir2/imported.jinja
@@ -0,0 +1 @@
+{% set foo = deferred %}
diff --git a/src/test/resources/eager/wraps-current-path-around-macro/test.expected.jinja b/src/test/resources/eager/wraps-current-path-around-macro/test.expected.jinja
@@ -0,0 +1,11 @@
+{% set __macro_foo_importer_1564912668_temp_variable_0__ %}\
+  {% set current_path = '../../eager/wraps-current-path-around-macro/dir1/macro.jinja' %}\
+  {% do %}\
+    {% set current_path = '../../eager/wraps-current-path-around-macro/dir2/imported.jinja' %}\
+    {% set foo = deferred %}\
+    {% set current_path = '../../eager/wraps-current-path-around-macro/dir1/macro.jinja' %}\
+  {% enddo %}\
+  {% print foo %}\
+  {% set current_path = '' %}\
+{% endset %}\
+{% print __macro_foo_importer_1564912668_temp_variable_0__ %}
diff --git a/src/test/resources/eager/wraps-current-path-around-macro/test.jinja b/src/test/resources/eager/wraps-current-path-around-macro/test.jinja
@@ -0,0 +1,2 @@
+{% from "../../eager/wraps-current-path-around-macro/dir1/macro.jinja" import foo_importer -%}
+{% print foo_importer() %}
diff --git a/src/test/resources/snippets/from-tag-doesnt-steal-extends/base.jinja b/src/test/resources/snippets/from-tag-doesnt-steal-extends/base.jinja
@@ -0,0 +1,3 @@
+{% block content %}
+yo
+{% endblock %}
diff --git a/src/test/resources/snippets/from-tag-doesnt-steal-extends/something-to-import.jinja b/src/test/resources/snippets/from-tag-doesnt-steal-extends/something-to-import.jinja
@@ -0,0 +1 @@
+{% set foo = 'hi' %}
diff --git a/src/test/resources/snippets/from-tag-doesnt-steal-extends/test.expected.jinja b/src/test/resources/snippets/from-tag-doesnt-steal-extends/test.expected.jinja
@@ -0,0 +1 @@
+<p>Test</p>
diff --git a/src/test/resources/snippets/from-tag-doesnt-steal-extends/test.jinja b/src/test/resources/snippets/from-tag-doesnt-steal-extends/test.jinja
@@ -0,0 +1,7 @@
+{% extends './base.jinja' %}
+{% from './something-to-import.jinja' import foo %}
+{{ foo }}
+Don't show me!
+{% block content %}
+  <p>Test</p>
+{% endblock %}