Fix to ensure that when set tags are reconstructed, do-update's are reconstructed if that set variable should get imported into an alias

Author: jasmith-hs

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

@@ -150,23 +150,30 @@ protected PrefixToPreserveState getPrefixToPreserveState(
     return prefixToPreserveState;
   }
 
-  protected String getSuffixToPreserveState(
+  public static String getSuffixToPreserveState(
     String variables,
     JinjavaInterpreter interpreter
   ) {
+    if (variables.isEmpty()) {
+      return "";
+    }
     StringBuilder suffixToPreserveState = new StringBuilder();
     Optional<String> maybeTemporaryImportAlias = AliasedEagerImportingStrategy.getTemporaryImportAlias(
       interpreter.getContext()
     );
-    if (maybeTemporaryImportAlias.isPresent()) {
+    if (
+      maybeTemporaryImportAlias.isPresent() &&
+      !AliasedEagerImportingStrategy.isTemporaryImportAlias(variables) &&
+      !interpreter.getContext().getMetaContextVariables().contains(variables)
+    ) {
       String updateString = getUpdateString(variables);
+
+      // Don't need to render because the temporary import alias's value is always deferred, and rendering will do nothing
       suffixToPreserveState.append(
-        interpreter.render(
-          EagerReconstructionUtils.buildDoUpdateTag(
-            maybeTemporaryImportAlias.get(),
-            updateString,
-            interpreter
-          )
+        EagerReconstructionUtils.buildDoUpdateTag(
+          maybeTemporaryImportAlias.get(),
+          updateString,
+          interpreter
         )
       );
     }

src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/AliasedEagerImportingStrategy.java

@@ -22,14 +22,21 @@
 import java.util.stream.Stream;
 
 public class AliasedEagerImportingStrategy implements EagerImportingStrategy {
-  private static final String TEMPORARY_IMPORT_ALIAS_FORMAT = "__temp_import_alias_%d__";
+  private static final String TEMPORARY_IMPORT_ALIAS_PREFIX = "__temp_import_alias_";
+  private static final String TEMPORARY_IMPORT_ALIAS_FORMAT =
+    TEMPORARY_IMPORT_ALIAS_PREFIX + "%d__";
 
   public static Optional<String> getTemporaryImportAlias(Context context) {
     return context
       .getImportResourceAlias()
       .map(AliasedEagerImportingStrategy::getTemporaryImportAlias);
   }
 
+  public static boolean isTemporaryImportAlias(String varName) {
+    // This is just faster than checking a regex
+    return varName.startsWith(TEMPORARY_IMPORT_ALIAS_PREFIX);
+  }
+
   private static String getTemporaryImportAlias(String fullAlias) {
     return String.format(
       TEMPORARY_IMPORT_ALIAS_FORMAT,
@@ -83,10 +90,7 @@ public void setup(JinjavaInterpreter child) {
     child.getContext().getScope().put(Context.IMPORT_RESOURCE_ALIAS_KEY, fullImportAlias);
     child.getContext().put(Context.IMPORT_RESOURCE_ALIAS_KEY, fullImportAlias);
     constructFullAliasPathMap(currentImportAlias, child);
-    Map<String, Object> currentContextAliasMap = getMapForCurrentContextAlias(
-      currentImportAlias,
-      child
-    );
+    getMapForCurrentContextAlias(currentImportAlias, child);
     importingData
       .getOriginalInterpreter()
       .getContext()
@@ -133,7 +137,7 @@ public String getFinalOutput(
       ) +
       wrapInChildScope(
         EagerImportingStrategy.getSetTagForDeferredChildBindings(
-          importingData.getOriginalInterpreter(),
+          child,
           currentImportAlias,
           child.getContext()
         ) +

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

@@ -21,6 +21,8 @@
 import com.hubspot.jinjava.lib.tag.SetTag;
 import com.hubspot.jinjava.lib.tag.eager.DeferredToken;
 import com.hubspot.jinjava.lib.tag.eager.EagerExecutionResult;
+import com.hubspot.jinjava.lib.tag.eager.EagerSetTagStrategy;
+import com.hubspot.jinjava.lib.tag.eager.importing.AliasedEagerImportingStrategy;
 import com.hubspot.jinjava.mode.EagerExecutionMode;
 import com.hubspot.jinjava.objects.serialization.PyishBlockSetSerializable;
 import com.hubspot.jinjava.objects.serialization.PyishObjectMapper;
@@ -474,11 +476,15 @@ public static String buildSetTag(
 
     StringJoiner vars = new StringJoiner(",");
     StringJoiner values = new StringJoiner(",");
+    StringJoiner varsRequiringSuffix = new StringJoiner(",");
     deferredValuesToSet.forEach(
       (key, value) -> {
         // This ensures they are properly aligned to each other.
         vars.add(key);
         values.add(value);
+        if (!AliasedEagerImportingStrategy.isTemporaryImportAlias(value)) {
+          varsRequiringSuffix.add(key);
+        }
       }
     );
     LengthLimitingStringJoiner result = new LengthLimitingStringJoiner(
@@ -493,6 +499,10 @@ public static String buildSetTag(
       .add(values.toString())
       .add(interpreter.getConfig().getTokenScannerSymbols().getExpressionEndWithTag());
     String image = result.toString();
+    String suffix = EagerSetTagStrategy.getSuffixToPreserveState(
+      varsRequiringSuffix.toString(),
+      interpreter
+    );
     // Don't defer if we're sticking with the new value
     if (registerDeferredToken) {
       return (
@@ -505,10 +515,11 @@ public static String buildSetTag(
               .build()
           )
         ) +
-        image
+        image +
+        suffix
       );
     }
-    return image;
+    return (image + suffix);
   }
 
   /**
@@ -552,6 +563,7 @@ public static String buildBlockSetTag(
       .add("end" + SetTag.TAG_NAME)
       .add(interpreter.getConfig().getTokenScannerSymbols().getExpressionEndWithTag());
     String image = blockSetTokenBuilder + value + endTokenBuilder;
+    String suffix = EagerSetTagStrategy.getSuffixToPreserveState(name, interpreter);
     if (registerDeferredToken) {
       return (
         new PrefixToPreserveState(
@@ -567,10 +579,11 @@ public static String buildBlockSetTag(
               .build()
           )
         ) +
-        image
+        image +
+        suffix
       );
     }
-    return image;
+    return image + suffix;
   }
 
   public static String buildDoUpdateTag(

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

@@ -1008,13 +1008,17 @@ public void itHandlesDoubleImportModificationSecondPass() {
 
   @Test
   public void itHandlesSameNameImportVar() {
-    String template = expectedTemplateInterpreter.getFixtureTemplate(
+    expectedTemplateInterpreter.assertExpectedOutputNonIdempotent(
       "handles-same-name-import-var"
     );
-    JinjavaInterpreter.getCurrent().render(template);
-    // No longer allows importing a file that uses the same alias as a variable declared in the import file
-    assertThat(JinjavaInterpreter.getCurrent().getContext().getDeferredNodes())
-      .isNotEmpty();
+  }
+
+  @Test
+  public void itHandlesSameNameImportVarSecondPass() {
+    interpreter.getContext().put("deferred", "resolved");
+    expectedTemplateInterpreter.assertExpectedNonEagerOutput(
+      "handles-same-name-import-var.expected"
+    );
   }
 
   @Test

src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java

@@ -397,7 +397,7 @@ public void itHandlesQuadLayerInDeferredIf() {
     );
     assertThat(result)
       .isEqualTo(
-        "{% if deferred %}{% do %}{% set current_path = 'import-tree-b.jinja' %}{% set __temp_import_alias_98__ = {}  %}{% for __ignored__ in [0] %}{% set a = {'foo_a': 'a', 'import_resource_path': 'import-tree-a.jinja', 'something': 'somn'}  %}{% do %}{% set current_path = 'import-tree-a.jinja' %}{% set __temp_import_alias_95701__ = {}  %}{% for __ignored__ in [0] %}{% set something = 'somn' %}{% do __temp_import_alias_95701__.update({'something': something}) %}\n" +
+        "{% if deferred %}{% do %}{% set current_path = 'import-tree-b.jinja' %}{% set __temp_import_alias_98__ = {}  %}{% for __ignored__ in [0] %}{% do %}{% set current_path = 'import-tree-a.jinja' %}{% set __temp_import_alias_95701__ = {}  %}{% for __ignored__ in [0] %}{% set something = 'somn' %}{% do __temp_import_alias_95701__.update({'something': something}) %}\n" +
         "{% set foo_a = 'a' %}{% do __temp_import_alias_95701__.update({'foo_a': foo_a}) %}\n" +
         "{% do __temp_import_alias_95701__.update({'foo_a': 'a','import_resource_path': 'import-tree-a.jinja','something': 'somn'}) %}{% endfor %}{% set a = __temp_import_alias_95701__ %}{% set current_path = 'import-tree-b.jinja' %}{% enddo %}\n" +
         "{% set foo_b = 'b' + a.foo_a %}{% do __temp_import_alias_98__.update({'foo_b': foo_b}) %}\n" +

src/test/resources/eager/allows-variable-sharing-alias-name.expected.jinja

@@ -1,7 +1,7 @@
 {% do %}{% set current_path = 'filters.jinja' %}{% set __temp_import_alias_854547461__ = {}  %}{% for __ignored__ in [0] %}
 {% set bar = deferred %}{% do __temp_import_alias_854547461__.update({'bar': bar}) %}
 
-{% set filters = {}  %}{% do filters.update(deferred) %}
+{% 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 %}
 
 {{ filters }}

src/test/resources/eager/handles-same-name-import-var.expected.expected.jinja

@@ -0,0 +1 @@
+[fn:map_entry('import_resource_path', '../settag/set-var-and-deferred.jinja'), fn:map_entry('my_var', {'my_var': {'foo': 'bar'} , 'value': 'resolved', 'import_resource_path': '../settag/set-var-and-deferred.jinja'} ), fn:map_entry('path', ''), fn:map_entry('value', 'resolved')]

src/test/resources/eager/handles-same-name-import-var.expected.jinja

@@ -0,0 +1,10 @@
+{% if deferred %}
+{% do %}{% set current_path = '../settag/set-var-and-deferred.jinja' %}{% set __temp_import_alias_1059697132__ = {}  %}{% for __ignored__ in [0] %}{% if deferred %}
+{% do %}{% set path = '' %}{% do __temp_import_alias_1059697132__.update({'path': path}) %}{% set my_var = {'my_var': {'foo': 'bar'} }  %}{% do __temp_import_alias_1059697132__.update({'my_var': my_var}) %}{% set path = '../settag/set-var-and-deferred.jinja' %}{% do __temp_import_alias_1059697132__.update({'path': path}) %}{% set value = null %}{% do __temp_import_alias_1059697132__.update({'value': value}) %}{% set my_var = {}  %}{% do __temp_import_alias_1059697132__.update({'my_var': my_var}) %}{% set my_var = {'foo': 'bar'}  %}{% do __temp_import_alias_1059697132__.update({'my_var': my_var}) %}{% set my_var = {'my_var': {'foo': 'bar'} }  %}{% do __temp_import_alias_1059697132__.update({'my_var': my_var}) %}
+{% set value = deferred %}{% do __temp_import_alias_1059697132__.update({'value': value}) %}{% set my_var = {'my_var': {'foo': 'bar'} }  %}{% do __temp_import_alias_1059697132__.update({'my_var': my_var}) %}{% do my_var.update({'value': value}) %}
+{% do my_var.update({'import_resource_path': '../settag/set-var-and-deferred.jinja', 'value': value}) %}{% set path = '' %}{% do __temp_import_alias_1059697132__.update({'path': path}) %}{% enddo %}
+{{ my_var }}
+{% endif %}
+{% do __temp_import_alias_1059697132__.update({'path': path,'import_resource_path': '../settag/set-var-and-deferred.jinja','value': value}) %}{% endfor %}{% set my_var = __temp_import_alias_1059697132__ %}{% set current_path = '' %}{% enddo %}
+{{ filter:dictsort.filter(my_var, ____int3rpr3t3r____, false, 'key') }}
+{% endif %}

src/test/resources/eager/handles-same-name-import-var.jinja

@@ -1,4 +1,4 @@
 {% if deferred %}
 {% import '../settag/set-var-and-deferred.jinja' as my_var %}
-{{ my_var }}
+{{ my_var|dictsort(false, 'key') }}
 {% endif %}