Merge pull request #1120 from HubSpot/aliased-macro-modification

jasmith-hs · web-flow · commit 23695ab534b5 · 2023-10-20T15:25:36.000-04:00
Explicitly defer node when attempting deferring an aliased modification in an imported macro function
diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagStrategy.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagStrategy.java
@@ -1,6 +1,7 @@
 package com.hubspot.jinjava.lib.tag.eager;
 
 import com.google.common.annotations.Beta;
+import com.hubspot.jinjava.interpret.DeferredValueException;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
 import com.hubspot.jinjava.lib.tag.SetTag;
 import com.hubspot.jinjava.lib.tag.eager.importing.AliasedEagerImportingStrategy;
@@ -166,6 +167,11 @@ public static String getSuffixToPreserveState(
       !AliasedEagerImportingStrategy.isTemporaryImportAlias(variables) &&
       !interpreter.getContext().getMetaContextVariables().contains(variables)
     ) {
+      if (!interpreter.getContext().containsKey(maybeTemporaryImportAlias.get())) {
+        throw new DeferredValueException(
+          "Cannot modify temporary import alias outside of import tag"
+        );
+      }
       String updateString = getUpdateString(variables);
 
       // Don't need to render because the temporary import alias's value is always deferred, and rendering will do nothing
diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/AliasedEagerImportingStrategy.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/AliasedEagerImportingStrategy.java
@@ -115,9 +115,17 @@ public void integrateChild(JinjavaInterpreter child) {
     childBindings.remove(temporaryImportAlias);
     importingData.getOriginalInterpreter().getContext().remove(temporaryImportAlias);
     // Remove meta keys
-    childBindings.remove(Context.GLOBAL_MACROS_SCOPE_KEY);
-    childBindings.remove(Context.IMPORT_RESOURCE_ALIAS_KEY);
-    mapForCurrentContextAlias.putAll(childBindings);
+    childBindings
+      .entrySet()
+      .stream()
+      .filter(
+        entry ->
+          !(
+            entry.getKey().equals(Context.GLOBAL_MACROS_SCOPE_KEY) ||
+            entry.getKey().equals(Context.IMPORT_RESOURCE_ALIAS_KEY)
+          )
+      )
+      .forEach(entry -> mapForCurrentContextAlias.put(entry.getKey(), entry.getValue()));
   }
 
   @Override
diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java
@@ -1499,4 +1499,14 @@ public void itAllowsVariableSharingAliasName() {
       "allows-variable-sharing-alias-name"
     );
   }
+
+  @Test
+  public void itFailsOnModificationInAliasedMacro() {
+    String input = expectedTemplateInterpreter.getFixtureTemplate(
+      "fails-on-modification-in-aliased-macro"
+    );
+    interpreter.render(input);
+    // We don't support this
+    assertThat(interpreter.getContext().getDeferredNodes()).isNotEmpty();
+  }
 }
diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java
@@ -344,7 +344,7 @@ public void itDefersTripleLayer() {
     context.put("b_val", "b");
     context.put("c_val", "c");
     String result = interpreter.render(
-      "{% import 'import-tree-c.jinja' as c %}{{ c|dictsort(false, 'key') }}"
+      "{% import 'import-tree-c.jinja' as c %}{{ c.b|dictsort(false, 'key') }}{{ c.foo_b }}{{ c.import_resource_path }}"
     );
     assertThat(interpreter.render("{{ c.b.a.foo_a }}")).isEqualTo("{{ c.b.a.foo_a }}");
     assertThat(interpreter.render("{{ c.b.foo_b }}")).isEqualTo("{{ c.b.foo_b }}");
@@ -355,7 +355,7 @@ public void itDefersTripleLayer() {
     assertThat(interpreter.render(result).trim())
       .isEqualTo(
         interpreter.render(
-          "{% import 'import-tree-c.jinja' as c %}{{ c|dictsort(false, 'key') }}"
+          "{% import 'import-tree-c.jinja' as c %}{{ c.b|dictsort(false, 'key') }}{{ c.foo_b }}{{ c.import_resource_path }}"
         )
       );
   }
diff --git a/src/test/resources/eager/allows-variable-sharing-alias-name.expected.jinja b/src/test/resources/eager/allows-variable-sharing-alias-name.expected.jinja
@@ -2,6 +2,6 @@
 {% set bar = deferred %}{% do __temp_import_alias_854547461__.update({'bar': bar}) %}
 
 {% set filters = {}  %}{% do __temp_import_alias_854547461__.update({'filters': filters}) %}{% do filters.update(deferred) %}
-{% do __temp_import_alias_854547461__.update({'bar': bar,'import_resource_path': 'filters.jinja','filters': filters,'foo': 123}) %}{% endfor %}{% set filters = __temp_import_alias_854547461__ %}{% set current_path = '' %}{% enddo %}
+{% do __temp_import_alias_854547461__.update({'bar': bar,'foo': 123,'import_resource_path': 'filters.jinja','filters': filters}) %}{% endfor %}{% set filters = __temp_import_alias_854547461__ %}{% set current_path = '' %}{% enddo %}
 
 {{ filters }}
diff --git a/src/test/resources/eager/fails-on-modification-in-aliased-macro.jinja b/src/test/resources/eager/fails-on-modification-in-aliased-macro.jinja
@@ -0,0 +1,6 @@
+{% import 'eager/settings.jinja' as shared %}
+
+{% if deferred %}
+{{ shared.load_settings() }}
+{% endif %}
+{{ shared.settings }}
diff --git a/src/test/resources/tags/macrotag/eager/settings.jinja b/src/test/resources/tags/macrotag/eager/settings.jinja
@@ -0,0 +1,5 @@
+{% set settings = {} %}
+
+{% macro load_settings() %}
+{% do settings.put('foo', 'bar') %}
+{% endmacro %}
