Skip to content

Commit

Permalink
Explicitly fail by creating a deferred node when attempting to defer …
Browse files Browse the repository at this point in the history
…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 }}
```
  • Loading branch information
jasmith-hs committed Oct 10, 2023
1 parent fbdaf83 commit 9f74a8b
Show file tree
Hide file tree
Showing 8 changed files with 30 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,20 @@ 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.remove(Context.GLOBAL_MACROS_SCOPE_KEY);
// childBindings.remove(Context.IMPORT_RESOURCE_ALIAS_KEY);
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()));
// mapForCurrentContextAlias.putAll(childBindings);
}

@Override
Expand Down
9 changes: 6 additions & 3 deletions src/test/java/com/hubspot/jinjava/EagerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}");
Expand All @@ -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 }}"
)
);
}
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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 }}

0 comments on commit 9f74a8b

Please sign in to comment.