From fbdaf83df23bab15bf389228371923bd65f4c5de Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Wed, 9 Aug 2023 16:35:11 -0400 Subject: [PATCH 1/2] Committing test case --- .../jinjava/lib/fn/eager/EagerMacroFunction.java | 11 ++++++++++- src/test/java/com/hubspot/jinjava/EagerTest.java | 7 +++++++ ...llows-modification-in-aliased-macro.expected.jinja | 6 ++++++ .../eager/allows-modification-in-aliased-macro.jinja | 6 ++++++ src/test/resources/tags/macrotag/eager/settings.jinja | 5 +++++ 5 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 src/test/resources/eager/allows-modification-in-aliased-macro.expected.jinja create mode 100644 src/test/resources/eager/allows-modification-in-aliased-macro.jinja create mode 100644 src/test/resources/tags/macrotag/eager/settings.jinja diff --git a/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java b/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java index 7e56b7e9a..6893097fd 100644 --- a/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java +++ b/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java @@ -71,6 +71,15 @@ public Object doEvaluate( interpreter ); if (!result.getResult().isFullyResolved()) { + if ( + result + .getSpeculativeBindings() + .keySet() + .stream() + .anyMatch(key -> localContextScope.getScope().containsKey(key)) + ) { + throw new DeferredValueException("e"); + } result = eagerEvaluateInDeferredExecutionMode( () -> getEvaluationResultDirectly(argMap, kwargMap, varArgs, interpreter), @@ -294,7 +303,7 @@ private String getSetTagForAliasedVariables(String fullName) { .getCombinedScope() .entrySet() .stream() - .filter(entry -> entry.getValue() instanceof DeferredValue) + // .filter(entry -> entry.getValue() instanceof DeferredValue) .map(Entry::getKey) .collect(Collectors.toMap(Function.identity(), name -> aliasName + name)); return EagerReconstructionUtils.buildSetTag( diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java index 618f08c4b..532e4593a 100644 --- a/src/test/java/com/hubspot/jinjava/EagerTest.java +++ b/src/test/java/com/hubspot/jinjava/EagerTest.java @@ -1499,4 +1499,11 @@ public void itAllowsVariableSharingAliasName() { "allows-variable-sharing-alias-name" ); } + + @Test + public void itAllowsModificationInAliasedMacro() { + expectedTemplateInterpreter.assertExpectedOutput( + "allows-modification-in-aliased-macro" + ); + } } diff --git a/src/test/resources/eager/allows-modification-in-aliased-macro.expected.jinja b/src/test/resources/eager/allows-modification-in-aliased-macro.expected.jinja new file mode 100644 index 000000000..4d01c9c84 --- /dev/null +++ b/src/test/resources/eager/allows-modification-in-aliased-macro.expected.jinja @@ -0,0 +1,6 @@ +{% import 'eager/settings.jinja' as shared %} + +{% if deferred %} +{{ shared.load_settings() }} +{% endif %} +{{ shared.settings }} diff --git a/src/test/resources/eager/allows-modification-in-aliased-macro.jinja b/src/test/resources/eager/allows-modification-in-aliased-macro.jinja new file mode 100644 index 000000000..4d01c9c84 --- /dev/null +++ b/src/test/resources/eager/allows-modification-in-aliased-macro.jinja @@ -0,0 +1,6 @@ +{% import 'eager/settings.jinja' as shared %} + +{% if deferred %} +{{ shared.load_settings() }} +{% endif %} +{{ shared.settings }} diff --git a/src/test/resources/tags/macrotag/eager/settings.jinja b/src/test/resources/tags/macrotag/eager/settings.jinja new file mode 100644 index 000000000..7c9feac3c --- /dev/null +++ b/src/test/resources/tags/macrotag/eager/settings.jinja @@ -0,0 +1,5 @@ +{% set settings = {} %} + +{% macro load_settings() %} +{% do settings.put('foo', 'bar') %} +{% endmacro %} From 47405e7ff26e2c9633f891929072f94dd0df26a2 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Tue, 10 Oct 2023 14:44:50 -0400 Subject: [PATCH 2/2] Explicitly fail by creating a deferred node when attempting to defer modification of an alias outside of the context of an import. This is a niche situation that we'd rather explicitly fail on than have incorrect logic. Handling this correctly is complex and likely not worth it. Example: ``` {# 'eager/settings.jinja' #} {% set settings = {} %} {% macro load_settings() %} {% do settings.put('foo', 'bar') %} {% endmacro %} ``` ``` {# 'main.jinja' #} {% import 'eager/settings.jinja' as shared %} {% if deferred %} {{ shared.load_settings() }} {% endif %} {{ shared.settings }} ``` --- .../jinjava/lib/fn/eager/EagerMacroFunction.java | 11 +---------- .../jinjava/lib/tag/eager/EagerSetTagStrategy.java | 6 ++++++ .../importing/AliasedEagerImportingStrategy.java | 14 +++++++++++--- src/test/java/com/hubspot/jinjava/EagerTest.java | 9 ++++++--- .../jinjava/lib/tag/eager/EagerImportTagTest.java | 4 ++-- .../allows-modification-in-aliased-macro.jinja | 6 ------ ...lows-variable-sharing-alias-name.expected.jinja | 2 +- ...> fails-on-modification-in-aliased-macro.jinja} | 0 8 files changed, 27 insertions(+), 25 deletions(-) delete mode 100644 src/test/resources/eager/allows-modification-in-aliased-macro.jinja rename src/test/resources/eager/{allows-modification-in-aliased-macro.expected.jinja => fails-on-modification-in-aliased-macro.jinja} (100%) diff --git a/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java b/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java index 6893097fd..7e56b7e9a 100644 --- a/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java +++ b/src/main/java/com/hubspot/jinjava/lib/fn/eager/EagerMacroFunction.java @@ -71,15 +71,6 @@ public Object doEvaluate( interpreter ); if (!result.getResult().isFullyResolved()) { - if ( - result - .getSpeculativeBindings() - .keySet() - .stream() - .anyMatch(key -> localContextScope.getScope().containsKey(key)) - ) { - throw new DeferredValueException("e"); - } result = eagerEvaluateInDeferredExecutionMode( () -> getEvaluationResultDirectly(argMap, kwargMap, varArgs, interpreter), @@ -303,7 +294,7 @@ private String getSetTagForAliasedVariables(String fullName) { .getCombinedScope() .entrySet() .stream() - // .filter(entry -> entry.getValue() instanceof DeferredValue) + .filter(entry -> entry.getValue() instanceof DeferredValue) .map(Entry::getKey) .collect(Collectors.toMap(Function.identity(), name -> aliasName + name)); return EagerReconstructionUtils.buildSetTag( 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 50c74ef83..8ce0faac3 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 @@ -1,6 +1,7 @@ package com.hubspot.jinjava.lib.tag.eager; import com.google.common.annotations.Beta; +import com.hubspot.jinjava.interpret.DeferredValueException; import com.hubspot.jinjava.interpret.JinjavaInterpreter; import com.hubspot.jinjava.lib.tag.SetTag; import com.hubspot.jinjava.lib.tag.eager.importing.AliasedEagerImportingStrategy; @@ -166,6 +167,11 @@ public static String getSuffixToPreserveState( !AliasedEagerImportingStrategy.isTemporaryImportAlias(variables) && !interpreter.getContext().getMetaContextVariables().contains(variables) ) { + if (!interpreter.getContext().containsKey(maybeTemporaryImportAlias.get())) { + throw new DeferredValueException( + "Cannot modify temporary import alias outside of import tag" + ); + } String updateString = getUpdateString(variables); // Don't need to render because the temporary import alias's value is always deferred, and rendering will do nothing diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/AliasedEagerImportingStrategy.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/importing/AliasedEagerImportingStrategy.java index 48c5c6523..9558c4b4b 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 @@ -115,9 +115,17 @@ public void integrateChild(JinjavaInterpreter child) { childBindings.remove(temporaryImportAlias); importingData.getOriginalInterpreter().getContext().remove(temporaryImportAlias); // Remove meta keys - childBindings.remove(Context.GLOBAL_MACROS_SCOPE_KEY); - childBindings.remove(Context.IMPORT_RESOURCE_ALIAS_KEY); - mapForCurrentContextAlias.putAll(childBindings); + childBindings + .entrySet() + .stream() + .filter( + entry -> + !( + entry.getKey().equals(Context.GLOBAL_MACROS_SCOPE_KEY) || + entry.getKey().equals(Context.IMPORT_RESOURCE_ALIAS_KEY) + ) + ) + .forEach(entry -> mapForCurrentContextAlias.put(entry.getKey(), entry.getValue())); } @Override diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java index 532e4593a..7d484766a 100644 --- a/src/test/java/com/hubspot/jinjava/EagerTest.java +++ b/src/test/java/com/hubspot/jinjava/EagerTest.java @@ -1501,9 +1501,12 @@ public void itAllowsVariableSharingAliasName() { } @Test - public void itAllowsModificationInAliasedMacro() { - expectedTemplateInterpreter.assertExpectedOutput( - "allows-modification-in-aliased-macro" + public void itFailsOnModificationInAliasedMacro() { + String input = expectedTemplateInterpreter.getFixtureTemplate( + "fails-on-modification-in-aliased-macro" ); + interpreter.render(input); + // We don't support this + assertThat(interpreter.getContext().getDeferredNodes()).isNotEmpty(); } } diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java index 0357a1192..3414d64a2 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 @@ -344,7 +344,7 @@ public void itDefersTripleLayer() { context.put("b_val", "b"); context.put("c_val", "c"); String result = interpreter.render( - "{% import 'import-tree-c.jinja' as c %}{{ c|dictsort(false, 'key') }}" + "{% import 'import-tree-c.jinja' as c %}{{ c.b|dictsort(false, 'key') }}{{ c.foo_b }}{{ c.import_resource_path }}" ); assertThat(interpreter.render("{{ c.b.a.foo_a }}")).isEqualTo("{{ c.b.a.foo_a }}"); assertThat(interpreter.render("{{ c.b.foo_b }}")).isEqualTo("{{ c.b.foo_b }}"); @@ -355,7 +355,7 @@ public void itDefersTripleLayer() { assertThat(interpreter.render(result).trim()) .isEqualTo( interpreter.render( - "{% import 'import-tree-c.jinja' as c %}{{ c|dictsort(false, 'key') }}" + "{% import 'import-tree-c.jinja' as c %}{{ c.b|dictsort(false, 'key') }}{{ c.foo_b }}{{ c.import_resource_path }}" ) ); } diff --git a/src/test/resources/eager/allows-modification-in-aliased-macro.jinja b/src/test/resources/eager/allows-modification-in-aliased-macro.jinja deleted file mode 100644 index 4d01c9c84..000000000 --- a/src/test/resources/eager/allows-modification-in-aliased-macro.jinja +++ /dev/null @@ -1,6 +0,0 @@ -{% import 'eager/settings.jinja' as shared %} - -{% if deferred %} -{{ shared.load_settings() }} -{% endif %} -{{ shared.settings }} 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 fe7bb0da9..470103a36 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 @@ -2,6 +2,6 @@ {% set bar = deferred %}{% do __temp_import_alias_854547461__.update({'bar': bar}) %} {% set filters = {} %}{% do __temp_import_alias_854547461__.update({'filters': filters}) %}{% do filters.update(deferred) %} -{% do __temp_import_alias_854547461__.update({'bar': bar,'import_resource_path': 'filters.jinja','filters': filters,'foo': 123}) %}{% endfor %}{% set filters = __temp_import_alias_854547461__ %}{% set current_path = '' %}{% enddo %} +{% do __temp_import_alias_854547461__.update({'bar': bar,'foo': 123,'import_resource_path': 'filters.jinja','filters': filters}) %}{% endfor %}{% set filters = __temp_import_alias_854547461__ %}{% set current_path = '' %}{% enddo %} {{ filters }} diff --git a/src/test/resources/eager/allows-modification-in-aliased-macro.expected.jinja b/src/test/resources/eager/fails-on-modification-in-aliased-macro.jinja similarity index 100% rename from src/test/resources/eager/allows-modification-in-aliased-macro.expected.jinja rename to src/test/resources/eager/fails-on-modification-in-aliased-macro.jinja