From 06bb8b26ffb309af2aba28a52f28855f2684c8b2 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Tue, 3 Oct 2023 15:24:37 -0400 Subject: [PATCH 1/8] Add test that demonstrates shared alias name not working --- src/test/java/com/hubspot/jinjava/EagerTest.java | 7 +++++++ .../allows-variable-sharing-alias-name.expected.jinja | 4 ++++ .../eager/allows-variable-sharing-alias-name.jinja | 3 +++ src/test/resources/tags/macrotag/filters.jinja | 4 ++++ 4 files changed, 18 insertions(+) create mode 100644 src/test/resources/eager/allows-variable-sharing-alias-name.expected.jinja create mode 100644 src/test/resources/eager/allows-variable-sharing-alias-name.jinja create mode 100644 src/test/resources/tags/macrotag/filters.jinja diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java index c6c7ece6e..60c84916b 100644 --- a/src/test/java/com/hubspot/jinjava/EagerTest.java +++ b/src/test/java/com/hubspot/jinjava/EagerTest.java @@ -1425,4 +1425,11 @@ public void itDefersLoopSettingMetaContextVarSecondPass() { "defers-loop-setting-meta-context-var.expected" ); } + + @Test + public void itAllowsVariableSharingAliasName() { + expectedTemplateInterpreter.assertExpectedOutput( + "allows-variable-sharing-alias-name" + ); + } } 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 new file mode 100644 index 000000000..7d8c1dcab --- /dev/null +++ b/src/test/resources/eager/allows-variable-sharing-alias-name.expected.jinja @@ -0,0 +1,4 @@ +{% import 'filters.jinja' as filters %} + +{{ filters }} + diff --git a/src/test/resources/eager/allows-variable-sharing-alias-name.jinja b/src/test/resources/eager/allows-variable-sharing-alias-name.jinja new file mode 100644 index 000000000..3b08b9bfb --- /dev/null +++ b/src/test/resources/eager/allows-variable-sharing-alias-name.jinja @@ -0,0 +1,3 @@ +{% import 'filters.jinja' as filters %} + +{{ filters }} diff --git a/src/test/resources/tags/macrotag/filters.jinja b/src/test/resources/tags/macrotag/filters.jinja new file mode 100644 index 000000000..85d767628 --- /dev/null +++ b/src/test/resources/tags/macrotag/filters.jinja @@ -0,0 +1,4 @@ +{% set foo = 123 %} +{% set bar = deferred %} +{% set filters = {} %} +{% do filters.update(deferred) %} From 504e96bc1c2893e1a04f923fb5c5beeb62163047 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Wed, 4 Oct 2023 10:34:24 -0400 Subject: [PATCH 2/8] Split aliased and flat importing strategies out of EagerImportTag --- .../jinjava/lib/tag/eager/EagerImportTag.java | 424 +----------------- .../lib/tag/eager/EagerIncludeTag.java | 3 +- .../AliasedEagerImportingStrategy.java | 279 ++++++++++++ .../importing/EagerImportingStrategy.java | 46 ++ .../EagerImportingStrategyFactory.java | 46 ++ .../importing/FlatEagerImportingStrategy.java | 103 +++++ .../tag/eager/importing/ImportingData.java | 40 ++ .../lib/tag/eager/EagerImportTagTest.java | 172 +++---- 8 files changed, 605 insertions(+), 508 deletions(-) create mode 100644 src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/AliasedEagerImportingStrategy.java create mode 100644 src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategy.java create mode 100644 src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategyFactory.java create mode 100644 src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/FlatEagerImportingStrategy.java create mode 100644 src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/ImportingData.java diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTag.java index 69f928e67..1683b0ce5 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTag.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTag.java @@ -1,36 +1,21 @@ package com.hubspot.jinjava.lib.tag.eager; import com.google.common.annotations.Beta; -import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Strings; import com.hubspot.jinjava.interpret.Context; -import com.hubspot.jinjava.interpret.DeferredValue; import com.hubspot.jinjava.interpret.DeferredValueException; import com.hubspot.jinjava.interpret.InterpretException; import com.hubspot.jinjava.interpret.JinjavaInterpreter; -import com.hubspot.jinjava.lib.fn.MacroFunction; import com.hubspot.jinjava.lib.tag.DoTag; import com.hubspot.jinjava.lib.tag.ImportTag; -import com.hubspot.jinjava.loader.RelativePathResolver; -import com.hubspot.jinjava.objects.collections.PyMap; -import com.hubspot.jinjava.objects.serialization.PyishObjectMapper; +import com.hubspot.jinjava.lib.tag.eager.importing.EagerImportingStrategy; +import com.hubspot.jinjava.lib.tag.eager.importing.EagerImportingStrategyFactory; +import com.hubspot.jinjava.lib.tag.eager.importing.ImportingData; import com.hubspot.jinjava.tree.Node; import com.hubspot.jinjava.tree.parse.TagToken; import com.hubspot.jinjava.util.EagerReconstructionUtils; -import com.hubspot.jinjava.util.PrefixToPreserveState; import java.io.IOException; -import java.util.Arrays; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; import java.util.Map; -import java.util.Map.Entry; import java.util.Optional; -import java.util.Set; -import java.util.StringJoiner; -import java.util.stream.Collectors; -import java.util.stream.Stream; -import org.apache.commons.lang3.StringUtils; @Beta public class EagerImportTag extends EagerStateChangingTag { @@ -45,34 +30,22 @@ public EagerImportTag(ImportTag importTag) { @Override public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter) { - List helper = ImportTag.getHelpers(tagToken); - - String currentImportAlias = ImportTag.getContextVar(helper); + ImportingData importingData = EagerImportingStrategyFactory.getImportingData( + tagToken, + interpreter + ); + EagerImportingStrategy eagerImportingStrategy = EagerImportingStrategyFactory.create( + importingData + ); - final String initialPathSetter = getSetTagForCurrentPath(interpreter); final String newPathSetter; Optional maybeTemplateFile; try { - maybeTemplateFile = ImportTag.getTemplateFile(helper, tagToken, interpreter); + maybeTemplateFile = + ImportTag.getTemplateFile(importingData.getHelpers(), tagToken, interpreter); } catch (DeferredValueException e) { - if (currentImportAlias.isEmpty()) { - throw e; - } - return ( - initialPathSetter + - new PrefixToPreserveState( - EagerReconstructionUtils.handleDeferredTokenAndReconstructReferences( - interpreter, - DeferredToken - .builderFromToken(tagToken) - .addUsedDeferredWords(Stream.of(helper.get(0))) - .addSetDeferredWords(Stream.of(currentImportAlias)) - .build() - ) - ) + - tagToken.getImage() - ); + return eagerImportingStrategy.handleDeferredTemplateFile(e); } if (!maybeTemplateFile.isPresent()) { return ""; @@ -80,7 +53,7 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter String templateFile = maybeTemplateFile.get(); try { Node node = ImportTag.parseTemplateAsNode(interpreter, templateFile); - newPathSetter = getSetTagForCurrentPath(interpreter); + newPathSetter = EagerImportingStrategyFactory.getSetTagForCurrentPath(interpreter); JinjavaInterpreter child = interpreter .getConfig() @@ -90,7 +63,7 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter JinjavaInterpreter.pushCurrent(child); String output; try { - setupImportAlias(currentImportAlias, child, interpreter); + eagerImportingStrategy.setup(child); output = child.render(node); } finally { JinjavaInterpreter.popCurrent(); @@ -109,7 +82,7 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter ) { ImportTag.handleDeferredNodesDuringImport( node, - currentImportAlias, + ImportTag.getContextVar(importingData.getHelpers()), childBindings, child, interpreter @@ -120,34 +93,12 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter tagToken.getStartPosition() ); } - integrateChild(currentImportAlias, childBindings, child, interpreter); - String finalOutput; + eagerImportingStrategy.integrateChild(child); if (child.getContext().getDeferredTokens().isEmpty() || output == null) { return ""; - } else if (!Strings.isNullOrEmpty(currentImportAlias)) { - // Since some values got deferred, output a DoTag that will load the currentImportAlias on the context. - finalOutput = - getFinalOutputWithAlias( - interpreter, - currentImportAlias, - initialPathSetter, - newPathSetter, - output, - childBindings - ); - } else { - finalOutput = - getFinalOutputWithoutAlias( - interpreter, - currentImportAlias, - initialPathSetter, - newPathSetter, - output, - childBindings - ); } return EagerReconstructionUtils.wrapInTag( - finalOutput, + eagerImportingStrategy.getFinalOutput(newPathSetter, output, childBindings), DoTag.TAG_NAME, interpreter, true @@ -164,343 +115,4 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter interpreter.getContext().getImportPathStack().pop(); } } - - private String getFinalOutputWithoutAlias( - JinjavaInterpreter interpreter, - String currentImportAlias, - String initialPathSetter, - String newPathSetter, - String output, - Map childBindings - ) { - return ( - newPathSetter + - getSetTagForDeferredChildBindings(interpreter, currentImportAlias, childBindings) + - output + - initialPathSetter - ); - } - - private String getFinalOutputWithAlias( - JinjavaInterpreter interpreter, - String currentImportAlias, - String initialPathSetter, - String newPathSetter, - String output, - Map childBindings - ) { - return ( - newPathSetter + - EagerReconstructionUtils.buildBlockOrInlineSetTagAndRegisterDeferredToken( - currentImportAlias, - Collections.emptyMap(), - interpreter - ) + - wrapInChildScope( - interpreter, - getSetTagForDeferredChildBindings( - interpreter, - currentImportAlias, - childBindings - ) + - output, - currentImportAlias - ) + - initialPathSetter - ); - } - - private static String wrapInChildScope( - JinjavaInterpreter interpreter, - String output, - String currentImportAlias - ) { - String combined = output + getDoTagToPreserve(interpreter, currentImportAlias); - // So that any set variables other than the alias won't exist outside the child's scope - return EagerReconstructionUtils.wrapInChildScope(combined, interpreter); - } - - private String getSetTagForDeferredChildBindings( - JinjavaInterpreter interpreter, - String currentImportAlias, - Map childBindings - ) { - if ( - Strings.isNullOrEmpty(currentImportAlias) && - interpreter.getContext().isDeferredExecutionMode() - ) { - Set metaContextVariables = interpreter - .getContext() - .getMetaContextVariables(); - // defer imported variables - EagerReconstructionUtils.buildSetTag( - childBindings - .entrySet() - .stream() - .filter( - entry -> - !(entry.getValue() instanceof DeferredValue) && entry.getValue() != null - ) - .filter(entry -> !metaContextVariables.contains(entry.getKey())) - .collect(Collectors.toMap(Entry::getKey, entry -> "")), - interpreter, - true - ); - } - return childBindings - .entrySet() - .stream() - .filter( - entry -> - entry.getValue() instanceof DeferredValue && - ((DeferredValue) entry.getValue()).getOriginalValue() != null - ) - .filter(entry -> !interpreter.getContext().containsKey(entry.getKey())) - .filter(entry -> !entry.getKey().equals(currentImportAlias)) - .map( - entry -> - EagerReconstructionUtils.buildBlockOrInlineSetTag( // don't register deferred token so that we don't defer them on higher context scopes; they only exist in the child scope - entry.getKey(), - ((DeferredValue) entry.getValue()).getOriginalValue(), - interpreter - ) - ) - .collect(Collectors.joining()); - } - - public static String getSetTagForCurrentPath(JinjavaInterpreter interpreter) { - return EagerReconstructionUtils.buildBlockOrInlineSetTag( - RelativePathResolver.CURRENT_PATH_CONTEXT_KEY, - interpreter - .getContext() - .getCurrentPathStack() - .peek() - .orElseGet( - () -> - (String) interpreter - .getContext() - .getOrDefault(RelativePathResolver.CURRENT_PATH_CONTEXT_KEY, "") - ), - interpreter - ); - } - - @SuppressWarnings("unchecked") - private static String getDoTagToPreserve( - JinjavaInterpreter interpreter, - String currentImportAlias - ) { - StringJoiner keyValueJoiner = new StringJoiner(","); - Object currentAliasMap = interpreter - .getContext() - .getSessionBindings() - .get(currentImportAlias); - for (Map.Entry entry : ( - (Map) ((DeferredValue) currentAliasMap).getOriginalValue() - ).entrySet()) { - if (entry.getKey().equals(currentImportAlias)) { - continue; - } - if (entry.getValue() instanceof DeferredValue) { - keyValueJoiner.add(String.format("'%s': %s", entry.getKey(), entry.getKey())); - } else if (!(entry.getValue() instanceof MacroFunction)) { - keyValueJoiner.add( - String.format( - "'%s': %s", - entry.getKey(), - PyishObjectMapper.getAsPyishString(entry.getValue()) - ) - ); - } - } - if (keyValueJoiner.length() > 0) { - return EagerReconstructionUtils.buildDoUpdateTag( - currentImportAlias, - "{" + keyValueJoiner.toString() + "}", - interpreter - ); - } - return ""; - } - - @VisibleForTesting - public static void setupImportAlias( - String currentImportAlias, - JinjavaInterpreter child, - JinjavaInterpreter parent - ) { - if (!Strings.isNullOrEmpty(currentImportAlias)) { - Optional maybeParentImportAlias = parent - .getContext() - .getImportResourceAlias(); - if (maybeParentImportAlias.isPresent()) { - child - .getContext() - .getScope() - .put( - Context.IMPORT_RESOURCE_ALIAS_KEY, - String.format("%s.%s", maybeParentImportAlias.get(), currentImportAlias) - ); - } else { - child - .getContext() - .getScope() - .put(Context.IMPORT_RESOURCE_ALIAS_KEY, currentImportAlias); - } - constructFullAliasPathMap(currentImportAlias, child); - getMapForCurrentContextAlias(currentImportAlias, child); - } - } - - @SuppressWarnings("unchecked") - private static void constructFullAliasPathMap( - String currentImportAlias, - JinjavaInterpreter child - ) { - String fullImportAlias = child - .getContext() - .getImportResourceAlias() - .orElse(currentImportAlias); - String[] allAliases = fullImportAlias.split("\\."); - Map currentMap = child.getContext().getParent(); - for (int i = 0; i < allAliases.length - 1; i++) { - Object maybeNextMap = currentMap.get(allAliases[i]); - if (maybeNextMap instanceof Map) { - currentMap = (Map) maybeNextMap; - } else if ( - maybeNextMap instanceof DeferredValue && - ((DeferredValue) maybeNextMap).getOriginalValue() instanceof Map - ) { - currentMap = - (Map) ((DeferredValue) maybeNextMap).getOriginalValue(); - } else { - throw new InterpretException("Encountered a problem with import alias maps"); - } - } - currentMap.put( - allAliases[allAliases.length - 1], - child.getContext().isDeferredExecutionMode() - ? DeferredValue.instance(new PyMap(new HashMap<>())) - : new PyMap(new HashMap<>()) - ); - } - - @SuppressWarnings("unchecked") - private static Map getMapForCurrentContextAlias( - String currentImportAlias, - JinjavaInterpreter child - ) { - Object parentValueForChild = child - .getContext() - .getParent() - .getSessionBindings() - .get(currentImportAlias); - if (parentValueForChild instanceof Map) { - return (Map) parentValueForChild; - } else if (parentValueForChild instanceof DeferredValue) { - if (((DeferredValue) parentValueForChild).getOriginalValue() instanceof Map) { - return (Map) ( - (DeferredValue) parentValueForChild - ).getOriginalValue(); - } - Map newMap = new PyMap(new HashMap<>()); - child - .getContext() - .getParent() - .put(currentImportAlias, DeferredValue.instance(newMap)); - return newMap; - } else { - Map newMap = new PyMap(new HashMap<>()); - child - .getContext() - .getParent() - .put( - currentImportAlias, - child.getContext().isDeferredExecutionMode() - ? DeferredValue.instance(newMap) - : newMap - ); - return newMap; - } - } - - @VisibleForTesting - public static void integrateChild( - String currentImportAlias, - Map childBindings, - JinjavaInterpreter child, - JinjavaInterpreter parent - ) { - for (MacroFunction macro : child.getContext().getGlobalMacros().values()) { - if (parent.getContext().isDeferredExecutionMode()) { - macro.setDeferred(true); - } - } - if (StringUtils.isBlank(currentImportAlias)) { - for (MacroFunction macro : child.getContext().getGlobalMacros().values()) { - parent.getContext().addGlobalMacro(macro); - } - childBindings.remove(Context.GLOBAL_MACROS_SCOPE_KEY); - childBindings.remove(Context.IMPORT_RESOURCE_ALIAS_KEY); - Map childBindingsWithoutImportResourcePath = ImportTag.getChildBindingsWithoutImportResourcePath( - childBindings - ); - if (parent.getContext().isDeferredExecutionMode()) { - childBindingsWithoutImportResourcePath - .keySet() - .forEach( - key -> - parent - .getContext() - .put(key, DeferredValue.instance(parent.getContext().get(key))) - ); - } else { - parent.getContext().putAll(childBindingsWithoutImportResourcePath); - } - } else { - if ( - child.getContext().isDeferredExecutionMode() && - child - .getContext() - .getDeferredTokens() - .stream() - .flatMap(deferredToken -> deferredToken.getSetDeferredWords().stream()) - .collect(Collectors.toSet()) - .contains(currentImportAlias) - ) { - // since a child scope will be used, the import alias would not be properly reconstructed - throw new DeferredValueException( - "Same-named variable as import alias: " + currentImportAlias - ); - } - childBindings.putAll(child.getContext().getGlobalMacros()); - Map mapForCurrentContextAlias = getMapForCurrentContextAlias( - currentImportAlias, - child - ); - // Remove layers from self down to original import alias to prevent reference loops - Arrays - .stream( - child - .getContext() - .getImportResourceAlias() - .orElse(currentImportAlias) - .split("\\.") - ) - .filter( - key -> - mapForCurrentContextAlias == - ( - childBindings.get(key) instanceof DeferredValue - ? ((DeferredValue) childBindings.get(key)).getOriginalValue() - : childBindings.get(key) - ) - ) - .forEach(childBindings::remove); - // Remove meta keys - childBindings.remove(Context.GLOBAL_MACROS_SCOPE_KEY); - childBindings.remove(Context.IMPORT_RESOURCE_ALIAS_KEY); - mapForCurrentContextAlias.putAll(childBindings); - } - } } diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerIncludeTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerIncludeTag.java index 9fbc04f3f..b0054c47e 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerIncludeTag.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerIncludeTag.java @@ -3,6 +3,7 @@ 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; @@ -29,7 +30,7 @@ public String innerInterpret(TagNode tagNode, JinjavaInterpreter interpreter) { tagNode.getStartPosition() ); templateFile = interpreter.resolveResourceLocation(templateFile); - final String initialPathSetter = EagerImportTag.getSetTagForCurrentPath( + final String initialPathSetter = EagerImportingStrategyFactory.getSetTagForCurrentPath( interpreter ); final String newPathSetter = EagerReconstructionUtils.buildBlockOrInlineSetTag( 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 new file mode 100644 index 000000000..588fc2d69 --- /dev/null +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/AliasedEagerImportingStrategy.java @@ -0,0 +1,279 @@ +package com.hubspot.jinjava.lib.tag.eager.importing; + +import com.google.common.annotations.VisibleForTesting; +import com.hubspot.jinjava.interpret.Context; +import com.hubspot.jinjava.interpret.DeferredValue; +import com.hubspot.jinjava.interpret.DeferredValueException; +import com.hubspot.jinjava.interpret.InterpretException; +import com.hubspot.jinjava.interpret.JinjavaInterpreter; +import com.hubspot.jinjava.lib.fn.MacroFunction; +import com.hubspot.jinjava.lib.tag.eager.DeferredToken; +import com.hubspot.jinjava.objects.collections.PyMap; +import com.hubspot.jinjava.objects.serialization.PyishObjectMapper; +import com.hubspot.jinjava.util.EagerReconstructionUtils; +import com.hubspot.jinjava.util.PrefixToPreserveState; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.StringJoiner; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +public class AliasedEagerImportingStrategy implements EagerImportingStrategy { + private final ImportingData importingData; + private final String currentImportAlias; + + @VisibleForTesting + public AliasedEagerImportingStrategy( + ImportingData importingData, + String currentImportAlias + ) { + this.importingData = importingData; + this.currentImportAlias = currentImportAlias; + } + + @Override + public String handleDeferredTemplateFile(DeferredValueException e) { + return ( + importingData.getInitialPathSetter() + + new PrefixToPreserveState( + EagerReconstructionUtils.handleDeferredTokenAndReconstructReferences( + importingData.getOriginalInterpreter(), + DeferredToken + .builderFromToken(importingData.getTagToken()) + .addUsedDeferredWords(Stream.of(importingData.getHelpers().get(0))) + .addSetDeferredWords(Stream.of(currentImportAlias)) + .build() + ) + ) + + importingData.getTagToken().getImage() + ); + } + + @Override + public void setup(JinjavaInterpreter child) { + Optional maybeParentImportAlias = importingData + .getOriginalInterpreter() + .getContext() + .getImportResourceAlias(); + if (maybeParentImportAlias.isPresent()) { + child + .getContext() + .getScope() + .put( + Context.IMPORT_RESOURCE_ALIAS_KEY, + String.format("%s.%s", maybeParentImportAlias.get(), currentImportAlias) + ); + } else { + child + .getContext() + .getScope() + .put(Context.IMPORT_RESOURCE_ALIAS_KEY, currentImportAlias); + } + constructFullAliasPathMap(currentImportAlias, child); + getMapForCurrentContextAlias(currentImportAlias, child); + } + + @Override + public void integrateChild(JinjavaInterpreter child) { + JinjavaInterpreter parent = importingData.getOriginalInterpreter(); + for (MacroFunction macro : child.getContext().getGlobalMacros().values()) { + if (parent.getContext().isDeferredExecutionMode()) { + macro.setDeferred(true); + } + } + if ( + child.getContext().isDeferredExecutionMode() && + child + .getContext() + .getDeferredTokens() + .stream() + .flatMap(deferredToken -> deferredToken.getSetDeferredWords().stream()) + .collect(Collectors.toSet()) + .contains(currentImportAlias) + ) { + // since a child scope will be used, the import alias would not be properly reconstructed + throw new DeferredValueException( + "Same-named variable as import alias: " + currentImportAlias + ); + } + Map childBindings = child.getContext().getSessionBindings(); + childBindings.putAll(child.getContext().getGlobalMacros()); + Map mapForCurrentContextAlias = getMapForCurrentContextAlias( + currentImportAlias, + child + ); + // Remove layers from self down to original import alias to prevent reference loops + Arrays + .stream( + child + .getContext() + .getImportResourceAlias() + .orElse(currentImportAlias) + .split("\\.") + ) + .filter( + key -> + mapForCurrentContextAlias == + ( + childBindings.get(key) instanceof DeferredValue + ? ((DeferredValue) childBindings.get(key)).getOriginalValue() + : childBindings.get(key) + ) + ) + .forEach(childBindings::remove); + // Remove meta keys + childBindings.remove(Context.GLOBAL_MACROS_SCOPE_KEY); + childBindings.remove(Context.IMPORT_RESOURCE_ALIAS_KEY); + mapForCurrentContextAlias.putAll(childBindings); + } + + @Override + public String getFinalOutput( + String newPathSetter, + String output, + Map childBindings + ) { + return ( + newPathSetter + + EagerReconstructionUtils.buildBlockOrInlineSetTagAndRegisterDeferredToken( + currentImportAlias, + Collections.emptyMap(), + importingData.getOriginalInterpreter() + ) + + wrapInChildScope( + importingData.getOriginalInterpreter(), + EagerImportingStrategy.getSetTagForDeferredChildBindings( + importingData.getOriginalInterpreter(), + currentImportAlias, + childBindings + ) + + output, + currentImportAlias + ) + + importingData.getInitialPathSetter() + ); + } + + @SuppressWarnings("unchecked") + private static void constructFullAliasPathMap( + String currentImportAlias, + JinjavaInterpreter child + ) { + String fullImportAlias = child + .getContext() + .getImportResourceAlias() + .orElse(currentImportAlias); + String[] allAliases = fullImportAlias.split("\\."); + Map currentMap = child.getContext().getParent(); + for (int i = 0; i < allAliases.length - 1; i++) { + Object maybeNextMap = currentMap.get(allAliases[i]); + if (maybeNextMap instanceof Map) { + currentMap = (Map) maybeNextMap; + } else if ( + maybeNextMap instanceof DeferredValue && + ((DeferredValue) maybeNextMap).getOriginalValue() instanceof Map + ) { + currentMap = + (Map) ((DeferredValue) maybeNextMap).getOriginalValue(); + } else { + throw new InterpretException("Encountered a problem with import alias maps"); + } + } + currentMap.put( + allAliases[allAliases.length - 1], + child.getContext().isDeferredExecutionMode() + ? DeferredValue.instance(new PyMap(new HashMap<>())) + : new PyMap(new HashMap<>()) + ); + } + + @SuppressWarnings("unchecked") + private static Map getMapForCurrentContextAlias( + String currentImportAlias, + JinjavaInterpreter child + ) { + Object parentValueForChild = child + .getContext() + .getParent() + .getSessionBindings() + .get(currentImportAlias); + if (parentValueForChild instanceof Map) { + return (Map) parentValueForChild; + } else if (parentValueForChild instanceof DeferredValue) { + if (((DeferredValue) parentValueForChild).getOriginalValue() instanceof Map) { + return (Map) ( + (DeferredValue) parentValueForChild + ).getOriginalValue(); + } + Map newMap = new PyMap(new HashMap<>()); + child + .getContext() + .getParent() + .put(currentImportAlias, DeferredValue.instance(newMap)); + return newMap; + } else { + Map newMap = new PyMap(new HashMap<>()); + child + .getContext() + .getParent() + .put( + currentImportAlias, + child.getContext().isDeferredExecutionMode() + ? DeferredValue.instance(newMap) + : newMap + ); + return newMap; + } + } + + private static String wrapInChildScope( + JinjavaInterpreter interpreter, + String output, + String currentImportAlias + ) { + String combined = output + getDoTagToPreserve(interpreter, currentImportAlias); + // So that any set variables other than the alias won't exist outside the child's scope + return EagerReconstructionUtils.wrapInChildScope(combined, interpreter); + } + + @SuppressWarnings("unchecked") + private static String getDoTagToPreserve( + JinjavaInterpreter interpreter, + String currentImportAlias + ) { + StringJoiner keyValueJoiner = new StringJoiner(","); + Object currentAliasMap = interpreter + .getContext() + .getSessionBindings() + .get(currentImportAlias); + for (Map.Entry entry : ( + (Map) ((DeferredValue) currentAliasMap).getOriginalValue() + ).entrySet()) { + if (entry.getKey().equals(currentImportAlias)) { + continue; + } + if (entry.getValue() instanceof DeferredValue) { + keyValueJoiner.add(String.format("'%s': %s", entry.getKey(), entry.getKey())); + } else if (!(entry.getValue() instanceof MacroFunction)) { + keyValueJoiner.add( + String.format( + "'%s': %s", + entry.getKey(), + PyishObjectMapper.getAsPyishString(entry.getValue()) + ) + ); + } + } + if (keyValueJoiner.length() > 0) { + return EagerReconstructionUtils.buildDoUpdateTag( + currentImportAlias, + "{" + keyValueJoiner.toString() + "}", + interpreter + ); + } + return ""; + } +} diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategy.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategy.java new file mode 100644 index 000000000..cede53363 --- /dev/null +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategy.java @@ -0,0 +1,46 @@ +package com.hubspot.jinjava.lib.tag.eager.importing; + +import com.hubspot.jinjava.interpret.DeferredValue; +import com.hubspot.jinjava.interpret.DeferredValueException; +import com.hubspot.jinjava.interpret.JinjavaInterpreter; +import com.hubspot.jinjava.util.EagerReconstructionUtils; +import java.util.Map; +import java.util.stream.Collectors; + +public interface EagerImportingStrategy { + String handleDeferredTemplateFile(DeferredValueException e); + void setup(JinjavaInterpreter child); + + void integrateChild(JinjavaInterpreter child); + String getFinalOutput( + String newPathSetter, + String output, + Map childBindings + ); + + static String getSetTagForDeferredChildBindings( + JinjavaInterpreter interpreter, + String currentImportAlias, + Map childBindings + ) { + return childBindings + .entrySet() + .stream() + .filter( + entry -> + entry.getValue() instanceof DeferredValue && + ((DeferredValue) entry.getValue()).getOriginalValue() != null + ) + .filter(entry -> !interpreter.getContext().containsKey(entry.getKey())) + .filter(entry -> !entry.getKey().equals(currentImportAlias)) + .map( + entry -> + EagerReconstructionUtils.buildBlockOrInlineSetTag( // don't register deferred token so that we don't defer them on higher context scopes; they only exist in the child scope + entry.getKey(), + ((DeferredValue) entry.getValue()).getOriginalValue(), + interpreter + ) + ) + .collect(Collectors.joining()); + } +} 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 new file mode 100644 index 000000000..cb3a227ba --- /dev/null +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategyFactory.java @@ -0,0 +1,46 @@ +package com.hubspot.jinjava.lib.tag.eager.importing; + +import com.google.common.base.Strings; +import com.hubspot.jinjava.interpret.JinjavaInterpreter; +import com.hubspot.jinjava.lib.tag.ImportTag; +import com.hubspot.jinjava.loader.RelativePathResolver; +import com.hubspot.jinjava.tree.parse.TagToken; +import com.hubspot.jinjava.util.EagerReconstructionUtils; +import java.util.List; + +public class EagerImportingStrategyFactory { + + public static ImportingData getImportingData( + TagToken tagToken, + JinjavaInterpreter interpreter + ) { + List helpers = ImportTag.getHelpers(tagToken); + String initialPathSetter = getSetTagForCurrentPath(interpreter); + return new ImportingData(interpreter, tagToken, helpers, initialPathSetter); + } + + public static EagerImportingStrategy create(ImportingData importingData) { + String currentImportAlias = ImportTag.getContextVar(importingData.getHelpers()); + if (Strings.isNullOrEmpty(currentImportAlias)) { + return new FlatEagerImportingStrategy(importingData); + } + return new AliasedEagerImportingStrategy(importingData, currentImportAlias); + } + + public static String getSetTagForCurrentPath(JinjavaInterpreter interpreter) { + return EagerReconstructionUtils.buildBlockOrInlineSetTag( + RelativePathResolver.CURRENT_PATH_CONTEXT_KEY, + interpreter + .getContext() + .getCurrentPathStack() + .peek() + .orElseGet( + () -> + (String) interpreter + .getContext() + .getOrDefault(RelativePathResolver.CURRENT_PATH_CONTEXT_KEY, "") + ), + interpreter + ); + } +} diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/FlatEagerImportingStrategy.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/FlatEagerImportingStrategy.java new file mode 100644 index 000000000..bd7fbfea4 --- /dev/null +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/FlatEagerImportingStrategy.java @@ -0,0 +1,103 @@ +package com.hubspot.jinjava.lib.tag.eager.importing; + +import com.google.common.annotations.VisibleForTesting; +import com.hubspot.jinjava.interpret.Context; +import com.hubspot.jinjava.interpret.DeferredValue; +import com.hubspot.jinjava.interpret.DeferredValueException; +import com.hubspot.jinjava.interpret.JinjavaInterpreter; +import com.hubspot.jinjava.lib.fn.MacroFunction; +import com.hubspot.jinjava.lib.tag.ImportTag; +import com.hubspot.jinjava.util.EagerReconstructionUtils; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Set; +import java.util.stream.Collectors; + +public class FlatEagerImportingStrategy implements EagerImportingStrategy { + private final ImportingData importingData; + + @VisibleForTesting + public FlatEagerImportingStrategy(ImportingData importingData) { + this.importingData = importingData; + } + + @Override + public String handleDeferredTemplateFile(DeferredValueException e) { + throw e; + } + + @Override + public void setup(JinjavaInterpreter child) { + // Do nothing + } + + @Override + public void integrateChild(JinjavaInterpreter child) { + JinjavaInterpreter parent = importingData.getOriginalInterpreter(); + for (MacroFunction macro : child.getContext().getGlobalMacros().values()) { + if (parent.getContext().isDeferredExecutionMode()) { + macro.setDeferred(true); + } + } + for (MacroFunction macro : child.getContext().getGlobalMacros().values()) { + parent.getContext().addGlobalMacro(macro); + } + Map childBindings = child.getContext().getSessionBindings(); + + childBindings.remove(Context.GLOBAL_MACROS_SCOPE_KEY); + childBindings.remove(Context.IMPORT_RESOURCE_ALIAS_KEY); + Map childBindingsWithoutImportResourcePath = ImportTag.getChildBindingsWithoutImportResourcePath( + childBindings + ); + if (parent.getContext().isDeferredExecutionMode()) { + childBindingsWithoutImportResourcePath + .keySet() + .forEach( + key -> + parent + .getContext() + .put(key, DeferredValue.instance(parent.getContext().get(key))) + ); + } else { + parent.getContext().putAll(childBindingsWithoutImportResourcePath); + } + } + + @Override + public String getFinalOutput( + String newPathSetter, + String output, + Map childBindings + ) { + if (importingData.getOriginalInterpreter().getContext().isDeferredExecutionMode()) { + Set metaContextVariables = importingData + .getOriginalInterpreter() + .getContext() + .getMetaContextVariables(); + // defer imported variables + EagerReconstructionUtils.buildSetTag( + childBindings + .entrySet() + .stream() + .filter( + entry -> + !(entry.getValue() instanceof DeferredValue) && entry.getValue() != null + ) + .filter(entry -> !metaContextVariables.contains(entry.getKey())) + .collect(Collectors.toMap(Entry::getKey, entry -> "")), + importingData.getOriginalInterpreter(), + true + ); + } + return ( + newPathSetter + + EagerImportingStrategy.getSetTagForDeferredChildBindings( + importingData.getOriginalInterpreter(), + null, + childBindings + ) + + output + + importingData.getInitialPathSetter() + ); + } +} diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/ImportingData.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/ImportingData.java new file mode 100644 index 000000000..95b9de1b6 --- /dev/null +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/ImportingData.java @@ -0,0 +1,40 @@ +package com.hubspot.jinjava.lib.tag.eager.importing; + +import com.hubspot.jinjava.interpret.JinjavaInterpreter; +import com.hubspot.jinjava.tree.parse.TagToken; +import java.util.List; + +public class ImportingData { + private final JinjavaInterpreter originalInterpreter; + private final TagToken tagToken; + private final List helpers; + private final String initialPathSetter; + + public ImportingData( + JinjavaInterpreter originalInterpreter, + TagToken tagToken, + List helpers, + String initialPathSetter + ) { + this.originalInterpreter = originalInterpreter; + this.tagToken = tagToken; + this.helpers = helpers; + this.initialPathSetter = initialPathSetter; + } + + public JinjavaInterpreter getOriginalInterpreter() { + return originalInterpreter; + } + + public TagToken getTagToken() { + return tagToken; + } + + public List getHelpers() { + return helpers; + } + + public String getInitialPathSetter() { + return initialPathSetter; + } +} 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 index 98436d7bf..ad7b65e3a 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java @@ -2,6 +2,7 @@ import static org.assertj.core.api.Assertions.assertThat; +import com.google.common.base.Strings; import com.google.common.io.Resources; import com.hubspot.jinjava.JinjavaConfig; import com.hubspot.jinjava.LegacyOverrides; @@ -12,11 +13,18 @@ import com.hubspot.jinjava.lib.tag.ImportTag; import com.hubspot.jinjava.lib.tag.ImportTagTest; import com.hubspot.jinjava.lib.tag.Tag; +import com.hubspot.jinjava.lib.tag.eager.importing.AliasedEagerImportingStrategy; +import com.hubspot.jinjava.lib.tag.eager.importing.EagerImportingStrategy; +import com.hubspot.jinjava.lib.tag.eager.importing.EagerImportingStrategyFactory; +import com.hubspot.jinjava.lib.tag.eager.importing.FlatEagerImportingStrategy; +import com.hubspot.jinjava.lib.tag.eager.importing.ImportingData; import com.hubspot.jinjava.loader.LocationResolver; import com.hubspot.jinjava.loader.RelativePathResolver; import com.hubspot.jinjava.loader.ResourceLocator; import com.hubspot.jinjava.mode.EagerExecutionMode; import com.hubspot.jinjava.objects.collections.PyMap; +import com.hubspot.jinjava.tree.parse.DefaultTokenScannerSymbols; +import com.hubspot.jinjava.tree.parse.TagToken; import java.io.IOException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; @@ -33,6 +41,8 @@ public class EagerImportTagTest extends ImportTagTest { private static final String CONTEXT_VAR = "context_var"; private static final String TEMPLATE_FILE = "template.jinja"; + private TagToken tagToken; + @Before public void eagerSetup() throws Exception { context.put("padding", 42); @@ -57,6 +67,36 @@ public void eagerSetup() throws Exception { context.registerTag(tag); context.put("deferred", DeferredValue.instance()); JinjavaInterpreter.pushCurrent(interpreter); + tagToken = + new TagToken( + String.format("{%% import foo as %s %%}", CONTEXT_VAR), + 0, + 0, + new DefaultTokenScannerSymbols() + ); + } + + private AliasedEagerImportingStrategy getAliasedStrategy( + String alias, + JinjavaInterpreter parentInterpreter + ) { + ImportingData importingData = EagerImportingStrategyFactory.getImportingData( + tagToken, + parentInterpreter + ); + + return new AliasedEagerImportingStrategy(importingData, alias); + } + + private FlatEagerImportingStrategy getFlatStrategy( + JinjavaInterpreter parentInterpreter + ) { + ImportingData importingData = EagerImportingStrategyFactory.getImportingData( + tagToken, + parentInterpreter + ); + + return new FlatEagerImportingStrategy(importingData); } @After @@ -70,7 +110,7 @@ public void itRemovesKeysFromChildBindings() { Map childBindings = child.getContext().getSessionBindings(); assertThat(childBindings.get(Context.IMPORT_RESOURCE_ALIAS_KEY)) .isEqualTo(CONTEXT_VAR); - EagerImportTag.integrateChild(CONTEXT_VAR, childBindings, child, interpreter); + getAliasedStrategy(CONTEXT_VAR, interpreter).integrateChild(child); assertThat(interpreter.getContext().get(CONTEXT_VAR)).isInstanceOf(Map.class); assertThat(((Map) interpreter.getContext().get(CONTEXT_VAR)).keySet()) .doesNotContain(Context.IMPORT_RESOURCE_ALIAS_KEY); @@ -83,18 +123,8 @@ public void itHandlesMultiLayer() { JinjavaInterpreter child2 = getChildInterpreter(child, ""); child2.getContext().put("foo", "foo val"); child.getContext().put("bar", "bar val"); - EagerImportTag.integrateChild( - "", - child2.getContext().getSessionBindings(), - child2, - child - ); - EagerImportTag.integrateChild( - "", - child.getContext().getSessionBindings(), - child, - interpreter - ); + getFlatStrategy(child).integrateChild(child2); + getFlatStrategy(interpreter).integrateChild(child); assertThat(interpreter.getContext().get("foo")).isEqualTo("foo val"); assertThat(interpreter.getContext().get("bar")).isEqualTo("bar val"); } @@ -109,18 +139,9 @@ public void itHandlesMultiLayerAliased() { child2.render("{% set foo = 'foo val' %}"); child.render("{% set bar = 'bar val' %}"); - EagerImportTag.integrateChild( - child2Alias, - child2.getContext().getSessionBindings(), - child2, - child - ); - EagerImportTag.integrateChild( - CONTEXT_VAR, - child.getContext().getSessionBindings(), - child, - interpreter - ); + getAliasedStrategy(child2Alias, child).integrateChild(child2); + getAliasedStrategy(CONTEXT_VAR, interpreter).integrateChild(child); + assertThat(interpreter.getContext().get(CONTEXT_VAR)).isInstanceOf(Map.class); assertThat( ((Map) interpreter.getContext().get(CONTEXT_VAR)).get(child2Alias) @@ -152,18 +173,9 @@ public void itHandlesMultiLayerAliasedAndDeferred() { child.render("{% set bar = 'bar val' %}"); child2.render("{% set foo_d = deferred %}"); - EagerImportTag.integrateChild( - child2Alias, - child2.getContext().getSessionBindings(), - child2, - child - ); - EagerImportTag.integrateChild( - CONTEXT_VAR, - child.getContext().getSessionBindings(), - child, - interpreter - ); + getAliasedStrategy(child2Alias, child).integrateChild(child2); + getAliasedStrategy(CONTEXT_VAR, interpreter).integrateChild(child); + assertThat(interpreter.getContext().get(CONTEXT_VAR)).isInstanceOf(PyMap.class); assertThat( ((Map) interpreter.getContext().get(CONTEXT_VAR)).get(child2Alias) @@ -199,18 +211,9 @@ public void itHandlesMultiLayerAliasedAndNullDeferred() { child.render("{% set bar = 'bar val' %}"); child2.render("{% set foo_d = deferred %}"); - EagerImportTag.integrateChild( - child2Alias, - child2.getContext().getSessionBindings(), - child2, - child - ); - EagerImportTag.integrateChild( - CONTEXT_VAR, - child.getContext().getSessionBindings(), - child, - interpreter - ); + getAliasedStrategy(child2Alias, child).integrateChild(child2); + getAliasedStrategy(CONTEXT_VAR, interpreter).integrateChild(child); + assertThat(interpreter.getContext().get(CONTEXT_VAR)).isInstanceOf(PyMap.class); assertThat( ((Map) interpreter.getContext().get(CONTEXT_VAR)).get(child2Alias) @@ -243,18 +246,8 @@ public void itHandlesMultiLayerDeferred() { child2.getContext().put("foo", DeferredValue.instance("foo val")); child.getContext().put("bar", DeferredValue.instance("bar val")); - EagerImportTag.integrateChild( - "", - child2.getContext().getSessionBindings(), - child2, - child - ); - EagerImportTag.integrateChild( - "", - child.getContext().getSessionBindings(), - child, - interpreter - ); + getFlatStrategy(child).integrateChild(child2); + getFlatStrategy(interpreter).integrateChild(child); assertThat(interpreter.getContext().get("foo")).isInstanceOf(DeferredValue.class); assertThat( (((DeferredValue) (interpreter.getContext().get("foo"))).getOriginalValue()) @@ -271,34 +264,19 @@ public void itHandlesMultiLayerDeferred() { @Test @SuppressWarnings("unchecked") public void itHandlesMultiLayerSomeAliased() { - String child2Alias = ""; String child3Alias = "triple_child"; JinjavaInterpreter child = getChildInterpreter(interpreter, CONTEXT_VAR); - JinjavaInterpreter child2 = getChildInterpreter(child, child2Alias); + JinjavaInterpreter child2 = getChildInterpreter(child, ""); JinjavaInterpreter child3 = getChildInterpreter(child2, child3Alias); child2.render("{% set foo = 'foo val' %}"); child.render("{% set bar = 'bar val' %}"); child3.render("{% set foobar = 'foobar val' %}"); - EagerImportTag.integrateChild( - child3Alias, - child3.getContext().getSessionBindings(), - child3, - child2 - ); - EagerImportTag.integrateChild( - child2Alias, - child2.getContext().getSessionBindings(), - child2, - child - ); - EagerImportTag.integrateChild( - CONTEXT_VAR, - child.getContext().getSessionBindings(), - child, - interpreter - ); + getAliasedStrategy(child3Alias, child2).integrateChild(child3); + getFlatStrategy(child).integrateChild(child2); + getAliasedStrategy(CONTEXT_VAR, interpreter).integrateChild(child); + assertThat(interpreter.getContext().get(CONTEXT_VAR)).isInstanceOf(Map.class); assertThat( ((Map) interpreter.getContext().get(CONTEXT_VAR)).get(child3Alias) @@ -337,24 +315,10 @@ public void itHandlesMultiLayerAliasedAndParallel() { child.render("{% set bar = 'bar val' %}"); child2B.render("{% set foo_b = 'foo_b val' %}"); - EagerImportTag.integrateChild( - child2Alias, - child2.getContext().getSessionBindings(), - child2, - child - ); - EagerImportTag.integrateChild( - child2BAlias, - child2B.getContext().getSessionBindings(), - child2B, - child - ); - EagerImportTag.integrateChild( - CONTEXT_VAR, - child.getContext().getSessionBindings(), - child, - interpreter - ); + getAliasedStrategy(child2Alias, child).integrateChild(child2); + getAliasedStrategy(child2BAlias, child).integrateChild(child2B); + getAliasedStrategy(CONTEXT_VAR, interpreter).integrateChild(child); + assertThat(interpreter.getContext().get(CONTEXT_VAR)).isInstanceOf(Map.class); assertThat( ((Map) interpreter.getContext().get(CONTEXT_VAR)).get(child2Alias) @@ -758,7 +722,7 @@ public void itDoesNotDeferImportedVariablesWhenNotInDeferredExecutionMode() { ); } - private static JinjavaInterpreter getChildInterpreter( + private JinjavaInterpreter getChildInterpreter( JinjavaInterpreter interpreter, String alias ) { @@ -767,7 +731,13 @@ private static JinjavaInterpreter getChildInterpreter( .getInterpreterFactory() .newInstance(interpreter); child.getContext().put(Context.IMPORT_RESOURCE_PATH_KEY, TEMPLATE_FILE); - EagerImportTag.setupImportAlias(alias, child, interpreter); + EagerImportingStrategy eagerImportingStrategy; + if (Strings.isNullOrEmpty(alias)) { + eagerImportingStrategy = getFlatStrategy(interpreter); + } else { + eagerImportingStrategy = getAliasedStrategy(alias, interpreter); + } + eagerImportingStrategy.setup(child); return child; } From 2ae6f73abfb5ab2a59ce0630f5a13afeac13c229 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Wed, 4 Oct 2023 14:12:09 -0400 Subject: [PATCH 3/8] When importing with an alias, reconstruct to a temporary variable rather than directly to the alias, as there may be name clashing inside of the imported file --- .../jinjava/lib/tag/eager/EagerImportTag.java | 2 +- .../lib/tag/eager/EagerSetTagStrategy.java | 37 ++--- .../AliasedEagerImportingStrategy.java | 146 ++++++++---------- .../importing/EagerImportingStrategy.java | 6 +- .../importing/FlatEagerImportingStrategy.java | 8 +- 5 files changed, 90 insertions(+), 109 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTag.java index 1683b0ce5..5a7b1cb86 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTag.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTag.java @@ -98,7 +98,7 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter return ""; } return EagerReconstructionUtils.wrapInTag( - eagerImportingStrategy.getFinalOutput(newPathSetter, output, childBindings), + eagerImportingStrategy.getFinalOutput(newPathSetter, output, child), DoTag.TAG_NAME, interpreter, true 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 index 9c4cdead0..8e170c4c2 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagStrategy.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagStrategy.java @@ -3,6 +3,7 @@ import com.google.common.annotations.Beta; import com.hubspot.jinjava.interpret.JinjavaInterpreter; import com.hubspot.jinjava.lib.tag.SetTag; +import com.hubspot.jinjava.lib.tag.eager.importing.AliasedEagerImportingStrategy; import com.hubspot.jinjava.tree.TagNode; import com.hubspot.jinjava.util.EagerReconstructionUtils; import com.hubspot.jinjava.util.PrefixToPreserveState; @@ -154,30 +155,22 @@ protected String getSuffixToPreserveState( JinjavaInterpreter interpreter ) { StringBuilder suffixToPreserveState = new StringBuilder(); - Optional maybeFullImportAlias = interpreter - .getContext() - .getImportResourceAlias(); - if (maybeFullImportAlias.isPresent()) { - String currentImportAlias = maybeFullImportAlias - .get() - .substring(maybeFullImportAlias.get().lastIndexOf(".") + 1); - String filteredVariables = Arrays - .stream(variables.split(",")) - .filter(var -> !var.equals(currentImportAlias)) - .collect(Collectors.joining(",")); - if (!filteredVariables.isEmpty()) { - String updateString = getUpdateString(filteredVariables); - suffixToPreserveState.append( - interpreter.render( - EagerReconstructionUtils.buildDoUpdateTag( - currentImportAlias, - updateString, - interpreter - ) + Optional maybeTemporaryImportAlias = AliasedEagerImportingStrategy.getTemporaryImportAlias( + interpreter.getContext() + ); + if (maybeTemporaryImportAlias.isPresent()) { + String updateString = getUpdateString(variables); + suffixToPreserveState.append( + interpreter.render( + EagerReconstructionUtils.buildDoUpdateTag( + maybeTemporaryImportAlias.get(), + updateString, + interpreter ) - ); - } + ) + ); } + return suffixToPreserveState.toString(); } 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 index 588fc2d69..43559c019 100644 --- 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 @@ -1,6 +1,7 @@ package com.hubspot.jinjava.lib.tag.eager.importing; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableMap; import com.hubspot.jinjava.interpret.Context; import com.hubspot.jinjava.interpret.DeferredValue; import com.hubspot.jinjava.interpret.DeferredValueException; @@ -12,18 +13,33 @@ import com.hubspot.jinjava.objects.serialization.PyishObjectMapper; import com.hubspot.jinjava.util.EagerReconstructionUtils; import com.hubspot.jinjava.util.PrefixToPreserveState; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.StringJoiner; -import java.util.stream.Collectors; import java.util.stream.Stream; public class AliasedEagerImportingStrategy implements EagerImportingStrategy { + private static final String TEMPORARY_IMPORT_ALIAS_FORMAT = "__temp_import_alias_%d__"; + + public static Optional getTemporaryImportAlias(Context context) { + return context + .getImportResourceAlias() + .map(AliasedEagerImportingStrategy::getTemporaryImportAlias); + } + + private static String getTemporaryImportAlias(String fullAlias) { + return String.format( + TEMPORARY_IMPORT_ALIAS_FORMAT, + Math.abs(Objects.hashCode(fullAlias)) + ); + } + private final ImportingData importingData; private final String currentImportAlias; + private final String fullImportAlias; @VisibleForTesting public AliasedEagerImportingStrategy( @@ -32,6 +48,16 @@ public AliasedEagerImportingStrategy( ) { this.importingData = importingData; this.currentImportAlias = currentImportAlias; + Optional maybeParentImportAlias = importingData + .getOriginalInterpreter() + .getContext() + .getImportResourceAlias(); + if (maybeParentImportAlias.isPresent()) { + fullImportAlias = + String.format("%s.%s", maybeParentImportAlias.get(), currentImportAlias); + } else { + fullImportAlias = currentImportAlias; + } } @Override @@ -54,26 +80,17 @@ public String handleDeferredTemplateFile(DeferredValueException e) { @Override public void setup(JinjavaInterpreter child) { - Optional maybeParentImportAlias = importingData + child.getContext().getScope().put(Context.IMPORT_RESOURCE_ALIAS_KEY, fullImportAlias); + child.getContext().put(Context.IMPORT_RESOURCE_ALIAS_KEY, fullImportAlias); + constructFullAliasPathMap(currentImportAlias, child); + Map currentContextAliasMap = getMapForCurrentContextAlias( + currentImportAlias, + child + ); + importingData .getOriginalInterpreter() .getContext() - .getImportResourceAlias(); - if (maybeParentImportAlias.isPresent()) { - child - .getContext() - .getScope() - .put( - Context.IMPORT_RESOURCE_ALIAS_KEY, - String.format("%s.%s", maybeParentImportAlias.get(), currentImportAlias) - ); - } else { - child - .getContext() - .getScope() - .put(Context.IMPORT_RESOURCE_ALIAS_KEY, currentImportAlias); - } - constructFullAliasPathMap(currentImportAlias, child); - getMapForCurrentContextAlias(currentImportAlias, child); + .put(getTemporaryImportAlias(fullImportAlias), DeferredValue.instance()); } @Override @@ -84,46 +101,16 @@ public void integrateChild(JinjavaInterpreter child) { macro.setDeferred(true); } } - if ( - child.getContext().isDeferredExecutionMode() && - child - .getContext() - .getDeferredTokens() - .stream() - .flatMap(deferredToken -> deferredToken.getSetDeferredWords().stream()) - .collect(Collectors.toSet()) - .contains(currentImportAlias) - ) { - // since a child scope will be used, the import alias would not be properly reconstructed - throw new DeferredValueException( - "Same-named variable as import alias: " + currentImportAlias - ); - } Map childBindings = child.getContext().getSessionBindings(); childBindings.putAll(child.getContext().getGlobalMacros()); + String temporaryImportAlias = getTemporaryImportAlias(fullImportAlias); Map mapForCurrentContextAlias = getMapForCurrentContextAlias( currentImportAlias, child ); // Remove layers from self down to original import alias to prevent reference loops - Arrays - .stream( - child - .getContext() - .getImportResourceAlias() - .orElse(currentImportAlias) - .split("\\.") - ) - .filter( - key -> - mapForCurrentContextAlias == - ( - childBindings.get(key) instanceof DeferredValue - ? ((DeferredValue) childBindings.get(key)).getOriginalValue() - : childBindings.get(key) - ) - ) - .forEach(childBindings::remove); + 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); @@ -134,24 +121,29 @@ public void integrateChild(JinjavaInterpreter child) { public String getFinalOutput( String newPathSetter, String output, - Map childBindings + JinjavaInterpreter child ) { + String temporaryImportAlias = getTemporaryImportAlias(fullImportAlias); return ( newPathSetter + - EagerReconstructionUtils.buildBlockOrInlineSetTagAndRegisterDeferredToken( - currentImportAlias, + EagerReconstructionUtils.buildBlockOrInlineSetTag( + temporaryImportAlias, Collections.emptyMap(), importingData.getOriginalInterpreter() ) + wrapInChildScope( - importingData.getOriginalInterpreter(), EagerImportingStrategy.getSetTagForDeferredChildBindings( importingData.getOriginalInterpreter(), currentImportAlias, - childBindings + child.getContext() ) + output, - currentImportAlias + child + ) + + EagerReconstructionUtils.buildSetTag( + ImmutableMap.of(currentImportAlias, temporaryImportAlias), + importingData.getOriginalInterpreter(), + true ) + importingData.getInitialPathSetter() ); @@ -229,30 +221,28 @@ private static Map getMapForCurrentContextAlias( } } - private static String wrapInChildScope( - JinjavaInterpreter interpreter, - String output, - String currentImportAlias - ) { - String combined = output + getDoTagToPreserve(interpreter, currentImportAlias); + private String wrapInChildScope(String output, JinjavaInterpreter child) { + String combined = + output + getDoTagToPreserve(importingData.getOriginalInterpreter(), child); // So that any set variables other than the alias won't exist outside the child's scope - return EagerReconstructionUtils.wrapInChildScope(combined, interpreter); + return EagerReconstructionUtils.wrapInChildScope( + combined, + importingData.getOriginalInterpreter() + ); } - @SuppressWarnings("unchecked") - private static String getDoTagToPreserve( + private String getDoTagToPreserve( JinjavaInterpreter interpreter, - String currentImportAlias + JinjavaInterpreter child ) { StringJoiner keyValueJoiner = new StringJoiner(","); - Object currentAliasMap = interpreter - .getContext() - .getSessionBindings() - .get(currentImportAlias); - for (Map.Entry entry : ( - (Map) ((DeferredValue) currentAliasMap).getOriginalValue() - ).entrySet()) { - if (entry.getKey().equals(currentImportAlias)) { + String temporaryImportAlias = getTemporaryImportAlias(fullImportAlias); + Map currentAliasMap = getMapForCurrentContextAlias( + currentImportAlias, + child + ); + for (Map.Entry entry : currentAliasMap.entrySet()) { + if (entry.getKey().equals(temporaryImportAlias)) { continue; } if (entry.getValue() instanceof DeferredValue) { @@ -269,7 +259,7 @@ private static String getDoTagToPreserve( } if (keyValueJoiner.length() > 0) { return EagerReconstructionUtils.buildDoUpdateTag( - currentImportAlias, + temporaryImportAlias, "{" + keyValueJoiner.toString() + "}", interpreter ); diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategy.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategy.java index cede53363..a149e3b2d 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategy.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/EagerImportingStrategy.java @@ -12,11 +12,7 @@ public interface EagerImportingStrategy { void setup(JinjavaInterpreter child); void integrateChild(JinjavaInterpreter child); - String getFinalOutput( - String newPathSetter, - String output, - Map childBindings - ); + String getFinalOutput(String newPathSetter, String output, JinjavaInterpreter child); static String getSetTagForDeferredChildBindings( JinjavaInterpreter interpreter, diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/FlatEagerImportingStrategy.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/FlatEagerImportingStrategy.java index bd7fbfea4..7004d3e11 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/FlatEagerImportingStrategy.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/FlatEagerImportingStrategy.java @@ -67,7 +67,7 @@ public void integrateChild(JinjavaInterpreter child) { public String getFinalOutput( String newPathSetter, String output, - Map childBindings + JinjavaInterpreter child ) { if (importingData.getOriginalInterpreter().getContext().isDeferredExecutionMode()) { Set metaContextVariables = importingData @@ -76,7 +76,9 @@ public String getFinalOutput( .getMetaContextVariables(); // defer imported variables EagerReconstructionUtils.buildSetTag( - childBindings + child + .getContext() + .getSessionBindings() .entrySet() .stream() .filter( @@ -94,7 +96,7 @@ public String getFinalOutput( EagerImportingStrategy.getSetTagForDeferredChildBindings( importingData.getOriginalInterpreter(), null, - childBindings + child.getContext().getSessionBindings() ) + output + importingData.getInitialPathSetter() From d5c198bc56b304ad236887908e28b14a8672bc22 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Wed, 4 Oct 2023 15:45:52 -0400 Subject: [PATCH 4/8] Fix expected test results --- .../java/com/hubspot/jinjava/EagerTest.java | 2 +- .../lib/tag/eager/EagerImportTagTest.java | 14 ++++---- ...variable-sharing-alias-name.expected.jinja | 7 ++-- ...-import-modification-in-for.expected.jinja | 32 +++++++++---------- ...andles-deferred-import-vars.expected.jinja | 6 ++-- ...-double-import-modification.expected.jinja | 18 +++++------ ...ndles-import-in-deferred-if.expected.jinja | 2 +- ...-in-deferred-imported-macro.expected.jinja | 4 +-- 8 files changed, 44 insertions(+), 41 deletions(-) diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java index 60c84916b..dd2512416 100644 --- a/src/test/java/com/hubspot/jinjava/EagerTest.java +++ b/src/test/java/com/hubspot/jinjava/EagerTest.java @@ -1428,7 +1428,7 @@ public void itDefersLoopSettingMetaContextVarSecondPass() { @Test public void itAllowsVariableSharingAliasName() { - expectedTemplateInterpreter.assertExpectedOutput( + expectedTemplateInterpreter.assertExpectedOutputNonIdempotent( "allows-variable-sharing-alias-name" ); } 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 index ad7b65e3a..e4d2c5f0e 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java @@ -426,11 +426,11 @@ public void itHandlesQuadLayerInDeferredIf() { ); assertThat(result) .isEqualTo( - "{% if deferred %}{% do %}{% set current_path = 'import-tree-b.jinja' %}{% set b = {} %}{% 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 a = {} %}{% for __ignored__ in [0] %}{% set something = 'somn' %}{% do a.update({'something': something}) %}\n" + - "{% set foo_a = 'a' %}{% do a.update({'foo_a': foo_a}) %}\n" + - "{% do a.update({'foo_a': 'a','import_resource_path': 'import-tree-a.jinja','something': 'somn'}) %}{% endfor %}{% set current_path = 'import-tree-b.jinja' %}{% enddo %}\n" + - "{% set foo_b = 'b' + a.foo_a %}{% do b.update({'foo_b': foo_b}) %}\n" + - "{% do b.update({'a': a,'foo_b': foo_b,'import_resource_path': 'import-tree-b.jinja'}) %}{% endfor %}{% set current_path = '' %}{% enddo %}{% endif %}" + "{% 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" + + "{% 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" + + "{% do __temp_import_alias_98__.update({'a': a,'foo_b': foo_b,'import_resource_path': 'import-tree-b.jinja'}) %}{% endfor %}{% set b = __temp_import_alias_98__ %}{% set current_path = '' %}{% enddo %}{% endif %}" ); removeDeferredContextKeys(); @@ -671,8 +671,8 @@ public void itDoesNotSilentlyOverrideVariable() { .isEqualTo( "a" + "{% set vars = {'foo': 'a', 'import_resource_path': 'var-a.jinja'} %}{% if deferred %}" + - "{% do %}{% set current_path = 'var-b.jinja' %}{% set vars = {} %}{% for __ignored__ in [0] %}{% set foo = 'b' %}{% do vars.update({'foo': foo}) %}\n" + - "{% do vars.update({'foo': 'b','import_resource_path': 'var-b.jinja'}) %}{% endfor %}{% set current_path = '' %}{% enddo %}" + + "{% do %}{% set current_path = 'var-b.jinja' %}{% set __temp_import_alias_3612204__ = {} %}{% for __ignored__ in [0] %}{% set foo = 'b' %}{% do __temp_import_alias_3612204__.update({'foo': foo}) %}\n" + + "{% do __temp_import_alias_3612204__.update({'foo': 'b','import_resource_path': 'var-b.jinja'}) %}{% endfor %}{% set vars = __temp_import_alias_3612204__ %}{% set current_path = '' %}{% enddo %}" + "{% endif %}" + "{{ vars.foo }}" ); 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 index 7d8c1dcab..0f6b6df67 100644 --- a/src/test/resources/eager/allows-variable-sharing-alias-name.expected.jinja +++ b/src/test/resources/eager/allows-variable-sharing-alias-name.expected.jinja @@ -1,4 +1,7 @@ -{% import 'filters.jinja' as filters %} +{% 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}) %} -{{ filters }} +{% set 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 }} diff --git a/src/test/resources/eager/does-not-override-import-modification-in-for.expected.jinja b/src/test/resources/eager/does-not-override-import-modification-in-for.expected.jinja index 2549956a9..6f6e2f083 100644 --- a/src/test/resources/eager/does-not-override-import-modification-in-for.expected.jinja +++ b/src/test/resources/eager/does-not-override-import-modification-in-for.expected.jinja @@ -1,40 +1,40 @@ {% set foo = 'start' %}{% for __ignored__ in [0] %} -{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set bar1 = {} %}{% for __ignored__ in [0] %}{% if deferred %} +{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set __temp_import_alias_3016318__ = {} %}{% for __ignored__ in [0] %}{% if deferred %} -{% set foo = 'starta' %}{% do bar1.update({'foo': foo}) %} +{% set foo = 'starta' %}{% do __temp_import_alias_3016318__.update({'foo': foo}) %} {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do bar1.update({'foo': foo}) %} -{% do bar1.update({'foo': foo,'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set current_path = '' %}{% enddo %} +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do __temp_import_alias_3016318__.update({'foo': foo}) %} +{% do __temp_import_alias_3016318__.update({'foo': foo,'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set bar1 = __temp_import_alias_3016318__ %}{% set current_path = '' %}{% enddo %} {{ bar1.foo }} -{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set bar2 = {} %}{% for __ignored__ in [0] %}{% if deferred %} +{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set __temp_import_alias_3016319__ = {} %}{% for __ignored__ in [0] %}{% if deferred %} -{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}{% do bar2.update({'foo': foo}) %} +{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}{% do __temp_import_alias_3016319__.update({'foo': foo}) %} {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do bar2.update({'foo': foo}) %} -{% do bar2.update({'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set current_path = '' %}{% enddo %} +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do __temp_import_alias_3016319__.update({'foo': foo}) %} +{% do __temp_import_alias_3016319__.update({'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set bar2 = __temp_import_alias_3016319__ %}{% set current_path = '' %}{% enddo %} {{ bar2.foo }} -{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set bar1 = {} %}{% for __ignored__ in [0] %}{% if deferred %} +{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set __temp_import_alias_3016318__ = {} %}{% for __ignored__ in [0] %}{% if deferred %} -{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}{% do bar1.update({'foo': foo}) %} +{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}{% do __temp_import_alias_3016318__.update({'foo': foo}) %} {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do bar1.update({'foo': foo}) %} -{% do bar1.update({'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set current_path = '' %}{% enddo %} +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do __temp_import_alias_3016318__.update({'foo': foo}) %} +{% do __temp_import_alias_3016318__.update({'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set bar1 = __temp_import_alias_3016318__ %}{% set current_path = '' %}{% enddo %} {{ bar1.foo }} -{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set bar2 = {} %}{% for __ignored__ in [0] %}{% if deferred %} +{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set __temp_import_alias_3016319__ = {} %}{% for __ignored__ in [0] %}{% if deferred %} -{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}{% do bar2.update({'foo': foo}) %} +{% set foo = filter:join.filter([foo, 'a'], ____int3rpr3t3r____, '') %}{% do __temp_import_alias_3016319__.update({'foo': foo}) %} {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do bar2.update({'foo': foo}) %} -{% do bar2.update({'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set current_path = '' %}{% enddo %} +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do __temp_import_alias_3016319__.update({'foo': foo}) %} +{% do __temp_import_alias_3016319__.update({'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set bar2 = __temp_import_alias_3016319__ %}{% set current_path = '' %}{% enddo %} {{ bar2.foo }} {% endfor %} {{ foo }} diff --git a/src/test/resources/eager/handles-deferred-import-vars.expected.jinja b/src/test/resources/eager/handles-deferred-import-vars.expected.jinja index ada801023..5f59e3cbc 100644 --- a/src/test/resources/eager/handles-deferred-import-vars.expected.jinja +++ b/src/test/resources/eager/handles-deferred-import-vars.expected.jinja @@ -4,8 +4,8 @@ Hello {{ myname }} {% enddo %}foo: Hello {{ myname }} bar: {{ bar }} --- -{% set myname = deferred + 7 %}{% do %}{% set current_path = 'macro-and-set.jinja' %}{% set simple = {} %}{% for __ignored__ in [0] %} -{% set bar = myname + 19 %}{% do simple.update({'bar': bar}) %} +{% set myname = deferred + 7 %}{% do %}{% set current_path = 'macro-and-set.jinja' %}{% set __temp_import_alias_902286926__ = {} %}{% for __ignored__ in [0] %} +{% set bar = myname + 19 %}{% do __temp_import_alias_902286926__.update({'bar': bar}) %} Hello {{ myname }} -{% do simple.update({'import_resource_path': 'macro-and-set.jinja'}) %}{% endfor %}{% set current_path = '' %}{% enddo %}simple.foo: {% set deferred_import_resource_path = 'macro-and-set.jinja' %}{% macro simple.foo() %}Hello {{ myname }}{% endmacro %}{% set deferred_import_resource_path = null %}{{ simple.foo() }} +{% do __temp_import_alias_902286926__.update({'import_resource_path': 'macro-and-set.jinja'}) %}{% endfor %}{% set simple = __temp_import_alias_902286926__ %}{% set current_path = '' %}{% enddo %}simple.foo: {% set deferred_import_resource_path = 'macro-and-set.jinja' %}{% macro simple.foo() %}Hello {{ myname }}{% endmacro %}{% set deferred_import_resource_path = null %}{{ simple.foo() }} simple.bar: {{ simple.bar }} diff --git a/src/test/resources/eager/handles-double-import-modification.expected.jinja b/src/test/resources/eager/handles-double-import-modification.expected.jinja index 3b1c32401..1be87dffe 100644 --- a/src/test/resources/eager/handles-double-import-modification.expected.jinja +++ b/src/test/resources/eager/handles-double-import-modification.expected.jinja @@ -1,20 +1,20 @@ -{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set bar1 = {} %}{% for __ignored__ in [0] %}{% if deferred %} +{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set __temp_import_alias_3016318__ = {} %}{% for __ignored__ in [0] %}{% if deferred %} -{% set foo = 'a' %}{% do bar1.update({'foo': foo}) %} +{% set foo = 'a' %}{% do __temp_import_alias_3016318__.update({'foo': foo}) %} {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do bar1.update({'foo': foo}) %} -{% do bar1.update({'foo': foo,'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set current_path = '' %}{% enddo %} +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do __temp_import_alias_3016318__.update({'foo': foo}) %} +{% do __temp_import_alias_3016318__.update({'foo': foo,'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set bar1 = __temp_import_alias_3016318__ %}{% set current_path = '' %}{% enddo %} --- -{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set bar2 = {} %}{% for __ignored__ in [0] %}{% if deferred %} +{% do %}{% set current_path = 'deferred-modification.jinja' %}{% set __temp_import_alias_3016319__ = {} %}{% for __ignored__ in [0] %}{% if deferred %} -{% set foo = 'a' %}{% do bar2.update({'foo': foo}) %} +{% set foo = 'a' %}{% do __temp_import_alias_3016319__.update({'foo': foo}) %} {% endif %} -{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do bar2.update({'foo': foo}) %} -{% do bar2.update({'foo': foo,'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set current_path = '' %}{% enddo %} +{% set foo = filter:join.filter([foo, 'b'], ____int3rpr3t3r____, '') %}{% do __temp_import_alias_3016319__.update({'foo': foo}) %} +{% do __temp_import_alias_3016319__.update({'foo': foo,'import_resource_path': 'deferred-modification.jinja'}) %}{% endfor %}{% set bar2 = __temp_import_alias_3016319__ %}{% set current_path = '' %}{% enddo %} --- {{ bar1.foo }} -{{ bar2.foo }} +{{ bar2.foo }} \ No newline at end of file diff --git a/src/test/resources/eager/handles-import-in-deferred-if.expected.jinja b/src/test/resources/eager/handles-import-in-deferred-if.expected.jinja index 67e20be43..8a6edc388 100644 --- a/src/test/resources/eager/handles-import-in-deferred-if.expected.jinja +++ b/src/test/resources/eager/handles-import-in-deferred-if.expected.jinja @@ -1,5 +1,5 @@ {% set primary_line_height = 100 %}{% if deferred %} -{% do %}{% set current_path = '../settag/set-val.jinja' %}{% set simple = {} %}{% for __ignored__ in [0] %}{% set primary_line_height = 42 %}{% do simple.update({'primary_line_height': primary_line_height}) %}{% do simple.update({'primary_line_height': 42,'import_resource_path': '../settag/set-val.jinja'}) %}{% endfor %}{% set current_path = '' %}{% enddo %} +{% do %}{% set current_path = '../settag/set-val.jinja' %}{% set __temp_import_alias_902286926__ = {} %}{% for __ignored__ in [0] %}{% set primary_line_height = 42 %}{% do __temp_import_alias_902286926__.update({'primary_line_height': primary_line_height}) %}{% do __temp_import_alias_902286926__.update({'primary_line_height': 42,'import_resource_path': '../settag/set-val.jinja'}) %}{% endfor %}{% set simple = __temp_import_alias_902286926__ %}{% set current_path = '' %}{% enddo %} {% else %} {% do %}{% set current_path = '../settag/set-val.jinja' %}{% set primary_line_height = 42 %}{% set current_path = '' %}{% enddo %} {% endif %} diff --git a/src/test/resources/eager/reconstructs-value-used-in-deferred-imported-macro.expected.jinja b/src/test/resources/eager/reconstructs-value-used-in-deferred-imported-macro.expected.jinja index 3881b072b..47d603370 100644 --- a/src/test/resources/eager/reconstructs-value-used-in-deferred-imported-macro.expected.jinja +++ b/src/test/resources/eager/reconstructs-value-used-in-deferred-imported-macro.expected.jinja @@ -1,6 +1,6 @@ -{% do %}{% set current_path = 'uses-deferred-value-in-macro.jinja' %}{% set macros = {} %}{% for __ignored__ in [0] %}{% set value = deferred %}{% do macros.update({'value': value}) %} +{% do %}{% set current_path = 'uses-deferred-value-in-macro.jinja' %}{% set __temp_import_alias_1081745881__ = {} %}{% for __ignored__ in [0] %}{% set value = deferred %}{% do __temp_import_alias_1081745881__.update({'value': value}) %} -{% do macros.update({'import_resource_path': 'uses-deferred-value-in-macro.jinja','value': value}) %}{% endfor %}{% set current_path = '' %}{% enddo %} +{% do __temp_import_alias_1081745881__.update({'import_resource_path': 'uses-deferred-value-in-macro.jinja','value': value}) %}{% endfor %}{% set macros = __temp_import_alias_1081745881__ %}{% set current_path = '' %}{% enddo %} {% set deferred_import_resource_path = 'uses-deferred-value-in-macro.jinja' %}{% macro macros.getValueAnd(other) %}{% set value = macros.value %} {{ value ~ ' ' ~ other }} From dfcd35b03996c74b9d720bf2641b73bf0233c438 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Wed, 4 Oct 2023 16:15:24 -0400 Subject: [PATCH 5/8] Fix setup of old test which was not properly testing any real logic --- .../lib/tag/eager/EagerImportTagTest.java | 73 ++++++------------- .../tags/eager/importtag/layer-one.jinja | 2 + .../tags/eager/importtag/layer-two.jinja | 2 + 3 files changed, 26 insertions(+), 51 deletions(-) create mode 100644 src/test/resources/tags/eager/importtag/layer-one.jinja create mode 100644 src/test/resources/tags/eager/importtag/layer-two.jinja 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 index e4d2c5f0e..ec8dd4069 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java @@ -9,6 +9,7 @@ import com.hubspot.jinjava.interpret.Context; import com.hubspot.jinjava.interpret.DeferredValue; import com.hubspot.jinjava.interpret.JinjavaInterpreter; +import com.hubspot.jinjava.interpret.RenderResult; import com.hubspot.jinjava.lib.filter.Filter; import com.hubspot.jinjava.lib.tag.ImportTag; import com.hubspot.jinjava.lib.tag.ImportTagTest; @@ -22,12 +23,12 @@ import com.hubspot.jinjava.loader.RelativePathResolver; import com.hubspot.jinjava.loader.ResourceLocator; import com.hubspot.jinjava.mode.EagerExecutionMode; -import com.hubspot.jinjava.objects.collections.PyMap; import com.hubspot.jinjava.tree.parse.DefaultTokenScannerSymbols; import com.hubspot.jinjava.tree.parse.TagToken; import java.io.IOException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.util.HashMap; import java.util.Map; import java.util.Map.Entry; import java.util.Optional; @@ -165,58 +166,20 @@ public void itHandlesMultiLayerAliased() { @Test @SuppressWarnings("unchecked") public void itHandlesMultiLayerAliasedAndDeferred() { + setupResourceLocator(); String child2Alias = "double_child"; - JinjavaInterpreter child = getChildInterpreter(interpreter, CONTEXT_VAR); - JinjavaInterpreter child2 = getChildInterpreter(child, child2Alias); - - child2.render("{% set foo = 'foo val' %}"); - child.render("{% set bar = 'bar val' %}"); - child2.render("{% set foo_d = deferred %}"); - - getAliasedStrategy(child2Alias, child).integrateChild(child2); - getAliasedStrategy(CONTEXT_VAR, interpreter).integrateChild(child); + RenderResult result = jinjava.renderForResult( + "{% import 'layer-one.jinja' as context_var %}", + new HashMap<>() + ); - assertThat(interpreter.getContext().get(CONTEXT_VAR)).isInstanceOf(PyMap.class); - assertThat( - ((Map) interpreter.getContext().get(CONTEXT_VAR)).get(child2Alias) - ) - .isInstanceOf(DeferredValue.class); + assertThat(result.getContext().get(CONTEXT_VAR)).isInstanceOf(DeferredValue.class); assertThat( ( - ( - (Map) ( - (DeferredValue) ( - (Map) (interpreter.getContext().get(CONTEXT_VAR)) - ).get(child2Alias) - ).getOriginalValue() - ).get("foo") - ) - ) - .isEqualTo("foo val"); - - assertThat( - (((Map) interpreter.getContext().get(CONTEXT_VAR)).get("bar")) - ) - .isEqualTo("bar val"); - } - - @Test - @SuppressWarnings("unchecked") - public void itHandlesMultiLayerAliasedAndNullDeferred() { - String child2Alias = "double_child"; - JinjavaInterpreter child = getChildInterpreter(interpreter, CONTEXT_VAR); - JinjavaInterpreter child2 = getChildInterpreter(child, child2Alias); - - child2.render("{% set foo = 'foo val' %}"); - child.render("{% set bar = 'bar val' %}"); - child2.render("{% set foo_d = deferred %}"); - - getAliasedStrategy(child2Alias, child).integrateChild(child2); - getAliasedStrategy(CONTEXT_VAR, interpreter).integrateChild(child); - - assertThat(interpreter.getContext().get(CONTEXT_VAR)).isInstanceOf(PyMap.class); - assertThat( - ((Map) interpreter.getContext().get(CONTEXT_VAR)).get(child2Alias) + (Map) ( + (DeferredValue) result.getContext().get(CONTEXT_VAR) + ).getOriginalValue() + ).get(child2Alias) ) .isInstanceOf(DeferredValue.class); assertThat( @@ -224,7 +187,11 @@ public void itHandlesMultiLayerAliasedAndNullDeferred() { ( (Map) ( (DeferredValue) ( - (Map) interpreter.getContext().get(CONTEXT_VAR) + ( + (Map) ( + (DeferredValue) result.getContext().get(CONTEXT_VAR) + ).getOriginalValue() + ) ).get(child2Alias) ).getOriginalValue() ).get("foo") @@ -233,7 +200,11 @@ public void itHandlesMultiLayerAliasedAndNullDeferred() { .isEqualTo("foo val"); assertThat( - (((Map) interpreter.getContext().get(CONTEXT_VAR)).get("bar")) + ( + (Map) ( + (DeferredValue) result.getContext().get(CONTEXT_VAR) + ).getOriginalValue() + ).get("bar") ) .isEqualTo("bar val"); } diff --git a/src/test/resources/tags/eager/importtag/layer-one.jinja b/src/test/resources/tags/eager/importtag/layer-one.jinja new file mode 100644 index 000000000..163bd0c3d --- /dev/null +++ b/src/test/resources/tags/eager/importtag/layer-one.jinja @@ -0,0 +1,2 @@ +{% set bar = 'bar val' %} +{% import 'layer-two.jinja' as double_child %} \ No newline at end of file diff --git a/src/test/resources/tags/eager/importtag/layer-two.jinja b/src/test/resources/tags/eager/importtag/layer-two.jinja new file mode 100644 index 000000000..6c9be8f6f --- /dev/null +++ b/src/test/resources/tags/eager/importtag/layer-two.jinja @@ -0,0 +1,2 @@ +{% set foo = 'foo val' %} +{% set foo_d = deferred %} From 8b313d573e4ffd444193ff2b5661e09d8b48da5b Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Thu, 5 Oct 2023 09:41:29 -0400 Subject: [PATCH 6/8] Fix to ensure that when set tags are reconstructed, do-update's are reconstructed if that set variable should get imported into an alias --- .../lib/tag/eager/EagerSetTagStrategy.java | 23 ++++++++++++------- .../AliasedEagerImportingStrategy.java | 16 ++++++++----- .../util/EagerReconstructionUtils.java | 21 +++++++++++++---- .../java/com/hubspot/jinjava/EagerTest.java | 14 +++++++---- .../lib/tag/eager/EagerImportTagTest.java | 2 +- ...variable-sharing-alias-name.expected.jinja | 2 +- ...me-name-import-var.expected.expected.jinja | 1 + ...andles-same-name-import-var.expected.jinja | 10 ++++++++ .../eager/handles-same-name-import-var.jinja | 2 +- 9 files changed, 65 insertions(+), 26 deletions(-) create mode 100644 src/test/resources/eager/handles-same-name-import-var.expected.expected.jinja create mode 100644 src/test/resources/eager/handles-same-name-import-var.expected.jinja 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 index 8e170c4c2..50c74ef83 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerSetTagStrategy.java +++ b/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 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 ) ); } 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 index 43559c019..da03ff886 100644 --- 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 @@ -22,7 +22,9 @@ 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 getTemporaryImportAlias(Context context) { return context @@ -30,6 +32,11 @@ public static Optional getTemporaryImportAlias(Context context) { .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 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() ) + diff --git a/src/main/java/com/hubspot/jinjava/util/EagerReconstructionUtils.java b/src/main/java/com/hubspot/jinjava/util/EagerReconstructionUtils.java index 32cf7b710..d5370c891 100644 --- a/src/main/java/com/hubspot/jinjava/util/EagerReconstructionUtils.java +++ b/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( diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java index dd2512416..490e2aa98 100644 --- a/src/test/java/com/hubspot/jinjava/EagerTest.java +++ b/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 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 index ec8dd4069..0357a1192 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java +++ b/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" + 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 index 0f6b6df67..fe7bb0da9 100644 --- a/src/test/resources/eager/allows-variable-sharing-alias-name.expected.jinja +++ b/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 }} diff --git a/src/test/resources/eager/handles-same-name-import-var.expected.expected.jinja b/src/test/resources/eager/handles-same-name-import-var.expected.expected.jinja new file mode 100644 index 000000000..95ad60b2c --- /dev/null +++ b/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')] diff --git a/src/test/resources/eager/handles-same-name-import-var.expected.jinja b/src/test/resources/eager/handles-same-name-import-var.expected.jinja new file mode 100644 index 000000000..60785735d --- /dev/null +++ b/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 %} diff --git a/src/test/resources/eager/handles-same-name-import-var.jinja b/src/test/resources/eager/handles-same-name-import-var.jinja index 226c07f02..13f595b67 100644 --- a/src/test/resources/eager/handles-same-name-import-var.jinja +++ b/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 %} From d68410e232dd0f766e0799cc6256e51c8ddc3046 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Thu, 5 Oct 2023 10:02:27 -0400 Subject: [PATCH 7/8] Improve second-pass result checking by also directly checking the original file against the final expected result --- .../java/com/hubspot/jinjava/EagerTest.java | 63 +++++++++++++++++++ .../jinjava/ExpectedTemplateInterpreter.java | 63 ++++++++++++++----- .../lib/tag/eager/EagerExtendsTagTest.java | 9 +++ .../eager/handles-cycle-in-deferred-for.jinja | 5 +- ...onstructs-map-node.expected.expected.jinja | 2 - .../reconstructs-map-node.expected.jinja | 8 +-- .../eager/reconstructs-map-node.jinja | 3 +- 7 files changed, 127 insertions(+), 26 deletions(-) diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java index 490e2aa98..618f08c4b 100644 --- a/src/test/java/com/hubspot/jinjava/EagerTest.java +++ b/src/test/java/com/hubspot/jinjava/EagerTest.java @@ -551,6 +551,9 @@ public void itSetsMultipleVarsDeferredInChildSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "sets-multiple-vars-deferred-in-child.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "sets-multiple-vars-deferred-in-child.expected" + ); } @Test @@ -657,6 +660,8 @@ public void itEagerlyDefersMacro() { @Test public void itEagerlyDefersMacroSecondPass() { + localContext.put("foo", "I am foo"); + localContext.put("bar", "I am bar"); localContext.put("deferred", true); expectedTemplateInterpreter.assertExpectedOutput("eagerly-defers-macro.expected"); expectedTemplateInterpreter.assertExpectedNonEagerOutput( @@ -801,6 +806,9 @@ public void itHandlesDeferredImportVarsSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "handles-deferred-import-vars.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "handles-deferred-import-vars.expected" + ); } @Test @@ -822,6 +830,9 @@ public void itHandlesDeferredFromImportAsSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "handles-deferred-from-import-as.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "handles-deferred-from-import-as.expected" + ); } @Test @@ -889,6 +900,9 @@ public void itHandlesDeferredInNamespaceSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "handles-deferred-in-namespace.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "handles-deferred-in-namespace.expected" + ); } @Test @@ -918,6 +932,9 @@ public void itHandlesBlockSetInDeferredIfSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "handles-block-set-in-deferred-if.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "handles-block-set-in-deferred-if.expected" + ); } @Test @@ -1001,6 +1018,9 @@ public void itHandlesDoubleImportModification() { @Test public void itHandlesDoubleImportModificationSecondPass() { interpreter.getContext().put("deferred", false); + expectedTemplateInterpreter.assertExpectedOutput( + "handles-double-import-modification.expected" + ); expectedTemplateInterpreter.assertExpectedNonEagerOutput( "handles-double-import-modification.expected" ); @@ -1016,6 +1036,9 @@ public void itHandlesSameNameImportVar() { @Test public void itHandlesSameNameImportVarSecondPass() { interpreter.getContext().put("deferred", "resolved"); + expectedTemplateInterpreter.assertExpectedOutput( + "handles-same-name-import-var.expected" + ); expectedTemplateInterpreter.assertExpectedNonEagerOutput( "handles-same-name-import-var.expected" ); @@ -1069,6 +1092,9 @@ public void itFullyDefersFilteredMacroSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "fully-defers-filtered-macro.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "fully-defers-filtered-macro.expected" + ); } @Test @@ -1103,6 +1129,7 @@ public void itReconstructsMapNode() { @Test public void itReconstructsMapNodeSecondPass() { interpreter.getContext().put("deferred", "resolved"); + expectedTemplateInterpreter.assertExpectedOutput("reconstructs-map-node.expected"); expectedTemplateInterpreter.assertExpectedNonEagerOutput( "reconstructs-map-node.expected" ); @@ -1123,6 +1150,9 @@ public void itDefersCallTagWithDeferredArgument() { @Test public void itDefersCallTagWithDeferredArgumentSecondPass() { interpreter.getContext().put("deferred", "resolved"); + expectedTemplateInterpreter.assertExpectedOutput( + "defers-call-tag-with-deferred-argument.expected" + ); expectedTemplateInterpreter.assertExpectedNonEagerOutput( "defers-call-tag-with-deferred-argument.expected" ); @@ -1145,6 +1175,9 @@ public void itHandlesHigherScopeReferenceModification() { @Test public void itHandlesHigherScopeReferenceModificationSecondPass() { interpreter.getContext().put("deferred", "b"); + expectedTemplateInterpreter.assertExpectedOutput( + "handles-higher-scope-reference-modification.expected" + ); expectedTemplateInterpreter.assertExpectedNonEagerOutput( "handles-higher-scope-reference-modification.expected" ); @@ -1181,6 +1214,9 @@ public void itDoesNotOverrideImportModificationInFor() { @Test public void itDoesNotOverrideImportModificationInForSecondPass() { interpreter.getContext().put("deferred", "resolved"); + expectedTemplateInterpreter.assertExpectedOutput( + "does-not-override-import-modification-in-for.expected" + ); expectedTemplateInterpreter.assertExpectedNonEagerOutput( "does-not-override-import-modification-in-for.expected" ); @@ -1196,6 +1232,9 @@ public void itHandlesDeferredForLoopVarFromMacro() { @Test public void itHandlesDeferredForLoopVarFromMacroSecondPass() { interpreter.getContext().put("deferred", "resolved"); + expectedTemplateInterpreter.assertExpectedOutput( + "handles-deferred-for-loop-var-from-macro.expected" + ); expectedTemplateInterpreter.assertExpectedNonEagerOutput( "handles-deferred-for-loop-var-from-macro.expected" ); @@ -1225,6 +1264,9 @@ public void itReconstructsNamespaceForSetTagsUsingPeriod() { @Test public void itReconstructsNamespaceForSetTagsUsingPeriodSecondPass() { interpreter.getContext().put("deferred", "resolved"); + expectedTemplateInterpreter.assertExpectedOutput( + "reconstructs-namespace-for-set-tags-using-period.expected" + ); expectedTemplateInterpreter.assertExpectedNonEagerOutput( "reconstructs-namespace-for-set-tags-using-period.expected" ); @@ -1241,6 +1283,9 @@ public void itUsesUniqueMacroNames() { public void itUsesUniqueMacroNamesSecondPass() { interpreter.getContext().put("deferred", "resolved"); expectedTemplateInterpreter.assertExpectedOutput("uses-unique-macro-names.expected"); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "uses-unique-macro-names.expected" + ); } @Test @@ -1298,6 +1343,9 @@ public void itKeepsMacroModificationsInScopeSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "keeps-macro-modifications-in-scope.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "keeps-macro-modifications-in-scope.expected" + ); } @Test @@ -1310,6 +1358,9 @@ public void itDoesNotReconstructVariableInWrongScope() { @Test public void itDoesNotReconstructVariableInWrongScopeSecondPass() { interpreter.getContext().put("deferred", true); + expectedTemplateInterpreter.assertExpectedOutput( + "does-not-reconstruct-variable-in-wrong-scope.expected" + ); expectedTemplateInterpreter.assertExpectedNonEagerOutput( "does-not-reconstruct-variable-in-wrong-scope.expected" ); @@ -1356,6 +1407,9 @@ public void itRreconstructsValueUsedInDeferredImportedMacroSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "reconstructs-value-used-in-deferred-imported-macro.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "reconstructs-value-used-in-deferred-imported-macro.expected" + ); } @Test @@ -1371,6 +1425,9 @@ public void itAllowsDeferredLazyReferenceToGetOverriddenSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "allows-deferred-lazy-reference-to-get-overridden.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "allows-deferred-lazy-reference-to-get-overridden.expected" + ); } @Test @@ -1411,6 +1468,9 @@ public void itReconstructsNestedValueInStringRepresentationSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "reconstructs-nested-value-in-string-representation.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "reconstructs-nested-value-in-string-representation.expected" + ); } @Test @@ -1425,6 +1485,9 @@ public void itDefersLoopSettingMetaContextVar() { public void itDefersLoopSettingMetaContextVarSecondPass() { interpreter.getContext().put("deferred", "resolved"); interpreter.getContext().getMetaContextVariables().add("content"); + expectedTemplateInterpreter.assertExpectedOutput( + "defers-loop-setting-meta-context-var.expected" + ); expectedTemplateInterpreter.assertExpectedNonEagerOutput( "defers-loop-setting-meta-context-var.expected" ); diff --git a/src/test/java/com/hubspot/jinjava/ExpectedTemplateInterpreter.java b/src/test/java/com/hubspot/jinjava/ExpectedTemplateInterpreter.java index 83daa4e1b..4f6748344 100644 --- a/src/test/java/com/hubspot/jinjava/ExpectedTemplateInterpreter.java +++ b/src/test/java/com/hubspot/jinjava/ExpectedTemplateInterpreter.java @@ -47,34 +47,65 @@ public String assertExpectedOutputNonIdempotent(String name) { } public String assertExpectedNonEagerOutput(String name) { - JinjavaInterpreter preserveInterpreter = new JinjavaInterpreter( - jinjava, - jinjava.getGlobalContextCopy(), - JinjavaConfig - .newBuilder() - .withExecutionMode(DefaultExecutionMode.instance()) - .withNestedInterpretationEnabled(true) - .withLegacyOverrides( - LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build() - ) - .withMaxMacroRecursionDepth(20) - .withEnableRecursiveMacroCalls(true) - .build() - ); + String output; try { + JinjavaInterpreter preserveInterpreter = new JinjavaInterpreter( + jinjava, + jinjava.getGlobalContextCopy(), + JinjavaConfig + .newBuilder() + .withExecutionMode(DefaultExecutionMode.instance()) + .withNestedInterpretationEnabled(true) + .withLegacyOverrides( + LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build() + ) + .withMaxMacroRecursionDepth(20) + .withEnableRecursiveMacroCalls(true) + .build() + ); JinjavaInterpreter.pushCurrent(preserveInterpreter); preserveInterpreter.getContext().putAll(interpreter.getContext()); String template = getFixtureTemplate(name); - String output = JinjavaInterpreter.getCurrent().render(template); + output = JinjavaInterpreter.getCurrent().render(template); assertThat(JinjavaInterpreter.getCurrent().getContext().getDeferredNodes()) .as("Ensure no deferred nodes were created") .isEmpty(); assertThat(output.trim()).isEqualTo(expected(name).trim()); - return output; } finally { JinjavaInterpreter.popCurrent(); } + if (name.contains(".expected")) { + String originalName = name.replace(".expected", ""); + try { + JinjavaInterpreter preserveInterpreter = new JinjavaInterpreter( + jinjava, + jinjava.getGlobalContextCopy(), + JinjavaConfig + .newBuilder() + .withExecutionMode(DefaultExecutionMode.instance()) + .withNestedInterpretationEnabled(true) + .withLegacyOverrides( + LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build() + ) + .withMaxMacroRecursionDepth(20) + .withEnableRecursiveMacroCalls(true) + .build() + ); + JinjavaInterpreter.pushCurrent(preserveInterpreter); + + preserveInterpreter.getContext().putAll(interpreter.getContext()); + String template = getFixtureTemplate(originalName); + output = JinjavaInterpreter.getCurrent().render(template); + assertThat(JinjavaInterpreter.getCurrent().getContext().getDeferredNodes()) + .as("Ensure no deferred nodes were created") + .isEmpty(); + assertThat(output.trim()).isEqualTo(expected(name).trim()); + } finally { + JinjavaInterpreter.popCurrent(); + } + } + return output; } public String getFixtureTemplate(String name) { diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerExtendsTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerExtendsTagTest.java index a0040c750..945a2e9a4 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerExtendsTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerExtendsTagTest.java @@ -54,6 +54,9 @@ public void itDefersBlockInExtendsChildSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "defers-block-in-extends-child.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "defers-block-in-extends-child.expected" + ); } @Test @@ -67,6 +70,9 @@ public void itDefersSuperBlockWithDeferredSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "defers-super-block-with-deferred.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "defers-super-block-with-deferred.expected" + ); } @Test @@ -83,6 +89,9 @@ public void itReconstructsDeferredOutsideBlockSecondPass() { expectedTemplateInterpreter.assertExpectedOutput( "reconstructs-deferred-outside-block.expected" ); + expectedTemplateInterpreter.assertExpectedNonEagerOutput( + "reconstructs-deferred-outside-block.expected" + ); } @Test diff --git a/src/test/resources/eager/handles-cycle-in-deferred-for.jinja b/src/test/resources/eager/handles-cycle-in-deferred-for.jinja index 75db4a6e2..9cbe73032 100644 --- a/src/test/resources/eager/handles-cycle-in-deferred-for.jinja +++ b/src/test/resources/eager/handles-cycle-in-deferred-for.jinja @@ -1,5 +1,8 @@ {% set foo = ['1','2','3'] %} +{% set one = '1' %} +{% set two = '2' %} +{% set three = '3' %} {%- for item in deferred %} {% cycle foo %} -{% cycle foo[0],foo[1],foo[2] %} +{% cycle one,two,three %} {%- endfor -%} diff --git a/src/test/resources/eager/reconstructs-map-node.expected.expected.jinja b/src/test/resources/eager/reconstructs-map-node.expected.expected.jinja index e66867c56..aea750573 100644 --- a/src/test/resources/eager/reconstructs-map-node.expected.expected.jinja +++ b/src/test/resources/eager/reconstructs-map-node.expected.expected.jinja @@ -1,5 +1,3 @@ -First key is foo. - foo ff bar bb ['resolved', 'resolved'] diff --git a/src/test/resources/eager/reconstructs-map-node.expected.jinja b/src/test/resources/eager/reconstructs-map-node.expected.jinja index 98b3f99fe..74c2a45a1 100644 --- a/src/test/resources/eager/reconstructs-map-node.expected.jinja +++ b/src/test/resources/eager/reconstructs-map-node.expected.jinja @@ -1,13 +1,11 @@ {% if deferred %} {% set foo = [fn:map_entry('foo', 'ff'), fn:map_entry('bar', 'bb')] %} {% endif %} -First key is {{ foo[0].key }}. -{% set my_list = [] %}{% for __ignored__ in [0] %}{% do my_list.append(deferred) %} -foo ff{% do my_list.append(deferred) %} -bar bb{% endfor %} +{% set my_list = [] %}{% for key, val in foo %}{% do my_list.append(deferred) %} +{{ key ~ ' ' ~ val }}{% endfor %} {{ my_list }} {% set my_list = [] %}{% for __ignored__ in [0] %}{% do my_list.append(deferred) %} foo{% do my_list.append(deferred) %} bar{% endfor %} -{{ my_list }} \ No newline at end of file +{{ my_list }} diff --git a/src/test/resources/eager/reconstructs-map-node.jinja b/src/test/resources/eager/reconstructs-map-node.jinja index 80acfeb80..4c95bd29b 100644 --- a/src/test/resources/eager/reconstructs-map-node.jinja +++ b/src/test/resources/eager/reconstructs-map-node.jinja @@ -2,8 +2,7 @@ {% if deferred %} {% set foo = {'foo': 'ff', 'bar': 'bb'}.items() %} {% endif %} -First key is {{ foo[0].key }}. -{% for key, val in {'foo': 'ff', 'bar': 'bb'}.items() -%} +{% for key, val in foo -%} {% do my_list.append(deferred) %} {{ key ~ ' ' ~ val }} {%- endfor %} From c9a11652064dd05c0ed78174298f5211bba4760c Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Thu, 5 Oct 2023 11:41:45 -0400 Subject: [PATCH 8/8] Remove comment which is no longer relevant --- .../lib/tag/eager/importing/AliasedEagerImportingStrategy.java | 1 - 1 file changed, 1 deletion(-) 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 index da03ff886..48c5c6523 100644 --- 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 @@ -112,7 +112,6 @@ public void integrateChild(JinjavaInterpreter child) { currentImportAlias, child ); - // Remove layers from self down to original import alias to prevent reference loops childBindings.remove(temporaryImportAlias); importingData.getOriginalInterpreter().getContext().remove(temporaryImportAlias); // Remove meta keys