Skip to content

Jinjava 3.0: Disable nested interpretation by default #1295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
jasmith-hs wants to merge 1 commit into
base: 3.0/legacy-overrides-on
Choose a base branch
from
Draft

Jinjava 3.0: Disable nested interpretation by default #1295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/hubspot/jinjava/JinjavaConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ default boolean isFailOnUnknownTokens() {

@Value.Default
default boolean isNestedInterpretationEnabled() {
return true;
return false; // Default changed to false in 3.0
}

@Value.Default
Expand Down
110 changes: 73 additions & 37 deletions src/test/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
import static org.assertj.core.api.Assertions.assertThat;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.hubspot.jinjava.Jinjava;
import com.hubspot.jinjava.JinjavaConfig;
import com.hubspot.jinjava.LegacyOverrides;
import com.hubspot.jinjava.interpret.Context;
import com.hubspot.jinjava.interpret.Context.Library;
import com.hubspot.jinjava.interpret.DeferredValue;
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
Expand All @@ -14,15 +17,24 @@
import com.hubspot.jinjava.objects.collections.PyList;
import com.hubspot.jinjava.tree.ExpressionNodeTest;
import java.util.ArrayList;
import java.util.Collections;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

public class EagerExpressionStrategyTest extends ExpressionNodeTest {

private Jinjava jinjava;

class EagerExecutionModeNoRaw extends EagerExecutionMode {

@Override
public boolean isPreserveRawTags() {
return false; // So that we can run all the ExpressionNodeTest tests without having the extra `{% raw %}` tags inserted
}
}

@Before
public void eagerSetup() throws Exception {
jinjava = new Jinjava(JinjavaConfig.newBuilder().build());
jinjava
.getGlobalContext()
.registerFunction(
Expand All @@ -35,51 +47,52 @@ public void eagerSetup() throws Exception {
interpreter =
new JinjavaInterpreter(
jinjava,
context,
new Context(),
JinjavaConfig
.newBuilder()
.withExecutionMode(new EagerExecutionModeNoRaw())
.build()
);
nestedInterpreter =
new JinjavaInterpreter(
jinjava,
interpreter.getContext(),
JinjavaConfig
.newBuilder()
.withNestedInterpretationEnabled(true)
.withLegacyOverrides(
LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build()
)
.withExecutionMode(EagerExecutionMode.instance())
.build()
);
JinjavaInterpreter.pushCurrent(interpreter);
context.put("deferred", DeferredValue.instance());
}

@After
public void teardown() {
JinjavaInterpreter.popCurrent();
interpreter.getContext().put("deferred", DeferredValue.instance());
nestedInterpreter.getContext().put("deferred", DeferredValue.instance());
}

@Test
public void itPreservesRawTags() {
interpreter =
new JinjavaInterpreter(
jinjava,
context,
new Context(),
JinjavaConfig
.newBuilder()
.withNestedInterpretationEnabled(false)
.withLegacyOverrides(
LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build()
)
.withExecutionMode(EagerExecutionMode.instance())
.build()
);
JinjavaInterpreter.pushCurrent(interpreter);
try {
assertExpectedOutput(
"{{ '{{ foo }}' }} {{ '{% something %}' }} {{ 'not needed' }}",
"{% raw %}{{ foo }}{% endraw %} {% raw %}{% something %}{% endraw %} not needed"
);
} finally {
JinjavaInterpreter.popCurrent();
}
assertExpectedOutput(
interpreter,
"{{ '{{ foo }}' }} {{ '{% something %}' }} {{ 'not needed' }}",
"{% raw %}{{ foo }}{% endraw %} {% raw %}{% something %}{% endraw %} not needed"
);
}

@Test
public void itPreservesRawTagsNestedInterpretation() {
context.put("bar", "bar");
nestedInterpreter.getContext().put("bar", "bar");
assertExpectedOutput(
nestedInterpreter,
"{{ '{{ 12345 }}' }} {{ '{% print bar %}' }} {{ 'not needed' }}",
"12345 bar not needed"
);
Expand All @@ -88,24 +101,27 @@ public void itPreservesRawTagsNestedInterpretation() {
@Test
public void itPrependsMacro() {
assertExpectedOutput(
interpreter,
"{% macro foo(bar) %} {{ bar }} {% endmacro %}{{ foo(deferred) }}",
"{% macro foo(bar) %} {{ bar }} {% endmacro %}{{ foo(deferred) }}"
);
}

@Test
public void itPrependsSet() {
context.put("foo", new PyList(new ArrayList<>()));
interpreter.getContext().put("foo", new PyList(new ArrayList<>()));
assertExpectedOutput(
interpreter,
"{{ foo.append(deferred) }}",
"{% set foo = [] %}{{ foo.append(deferred) }}"
);
}

@Test
public void itDoesConcatenation() {
context.put("foo", "y'all");
interpreter.getContext().put("foo", "y'all");
assertExpectedOutput(
interpreter,
"{{ 'oh, ' ~ foo ~ foo ~ ' toaster' }}",
"oh, y'ally'all toaster"
);
Expand All @@ -116,6 +132,7 @@ public void itHandlesQuotesLikeJinja() {
// {{ 'a|\'|\\\'|\\\\\'|"|\"|\\"|\\\\"|a ' ~ " b|\"|\\\"|\\\\\"|'|\'|\\'|\\\\'|b" }}
// --> a|'|\'|\\'|"|"|\"|\\"|a b|"|\"|\\"|'|'|\'|\\'|b
assertExpectedOutput(
interpreter,
"{{ 'a|\\'|\\\\\\'|\\\\\\\\\\'|\"|\\\"|\\\\\"|\\\\\\\\\"|a ' " +
"~ \" b|\\\"|\\\\\\\"|\\\\\\\\\\\"|'|\\'|\\\\'|\\\\\\\\'|b\" }}",
"a|'|\\'|\\\\'|\"|\"|\\\"|\\\\\"|a b|\"|\\\"|\\\\\"|'|'|\\'|\\\\'|b"
Expand All @@ -125,6 +142,7 @@ public void itHandlesQuotesLikeJinja() {
@Test
public void itGoesIntoDeferredExecutionMode() {
assertExpectedOutput(
interpreter,
"{{ is_deferred_execution_mode() }}" +
"{% if deferred %}{{ is_deferred_execution_mode() }}{% endif %}" +
"{{ is_deferred_execution_mode() }}",
Expand All @@ -135,6 +153,7 @@ public void itGoesIntoDeferredExecutionMode() {
@Test
public void itGoesIntoDeferredExecutionModeWithMacro() {
assertExpectedOutput(
interpreter,
"{% macro def() %}{{ is_deferred_execution_mode() }}{% endmacro %}" +
"{{ def() }}" +
"{% if deferred %}{{ def() }}{% endif %}" +
Expand All @@ -145,20 +164,28 @@ public void itGoesIntoDeferredExecutionModeWithMacro() {

@Test
public void itDoesNotGoIntoDeferredExecutionModeUnnecessarily() {
assertExpectedOutput("{{ is_deferred_execution_mode() }}", "false");
assertExpectedOutput(interpreter, "{{ is_deferred_execution_mode() }}", "false");
interpreter.getContext().setDeferredExecutionMode(true);
assertExpectedOutput("{{ is_deferred_execution_mode() }}", "true");
assertExpectedOutput(interpreter, "{{ is_deferred_execution_mode() }}", "true");
}

@Test
public void itDoesNotNestedInterpretIfThereAreFakeNotes() {
assertExpectedOutput("{{ '{#something_to_{{keep}}' }}", "{#something_to_{{keep}}");
assertExpectedOutput(
nestedInterpreter,
"{{ '{#something_to_{{keep}}' }}",
"{#something_to_{{keep}}"
);
}

@Test
public void itDoesNotReconstructWithDoubleCurlyBraces() {
interpreter.getContext().put("foo", ImmutableMap.of("foo", ImmutableMap.of()));
assertExpectedOutput("{{ deferred ~ foo }}", "{{ deferred ~ {'foo': {} } }}");
assertExpectedOutput(
interpreter,
"{{ deferred ~ foo }}",
"{{ deferred ~ {'foo': {} } }}"
);
}

@Test
Expand All @@ -167,6 +194,7 @@ public void itDoesNotReconstructWithNestedDoubleCurlyBraces() {
.getContext()
.put("foo", ImmutableMap.of("foo", ImmutableMap.of("bar", ImmutableMap.of())));
assertExpectedOutput(
interpreter,
"{{ deferred ~ foo }}",
"{{ deferred ~ {'foo': {'bar': {} } } }}"
);
Expand All @@ -175,6 +203,7 @@ public void itDoesNotReconstructWithNestedDoubleCurlyBraces() {
@Test
public void itDoesNotReconstructDirectlyWrittenWithDoubleCurlyBraces() {
assertExpectedOutput(
interpreter,
"{{ deferred ~ {\n'foo': {\n'bar': deferred\n}\n}\n }}",
"{{ deferred ~ {'foo': {'bar': deferred} } }}"
);
Expand All @@ -184,6 +213,7 @@ public void itDoesNotReconstructDirectlyWrittenWithDoubleCurlyBraces() {
public void itReconstructsWithNestedInterpretation() {
interpreter.getContext().put("foo", "{{ print 'bar' }}");
assertExpectedOutput(
interpreter,
"{{ deferred ~ foo }}",
"{{ deferred ~ '{{ print \\'bar\\' }}' }}"
);
Expand All @@ -192,18 +222,24 @@ public void itReconstructsWithNestedInterpretation() {
@Test
public void itDoesNotDoNestedInterpretationWithSyntaxErrors() {
try (
InterpreterScopeClosable c = interpreter.enterScope(
ImmutableMap.of(Library.TAG, Collections.singleton("print"))
InterpreterScopeClosable c = nestedInterpreter.enterScope(
ImmutableMap.of(Library.TAG, ImmutableSet.of("print"))
)
) {
interpreter.getContext().put("foo", "{% print 'bar' %}");
nestedInterpreter.getContext().put("foo", "{% print 'bar' %}");
// Rather than rendering this to an empty string
assertThat(interpreter.render("{{ foo }}")).isEqualTo("{% print 'bar' %}");
assertExpectedOutput(nestedInterpreter, "{{ foo }}", "{% print 'bar' %}");
}
}

private void assertExpectedOutput(String inputTemplate, String expectedOutput) {
assertThat(interpreter.render(inputTemplate)).isEqualTo(expectedOutput);
private void assertExpectedOutput(
JinjavaInterpreter interpreter,
String inputTemplate,
String expectedOutput
) {
try (var a = JinjavaInterpreter.closeablePushCurrent(interpreter).get()) {
assertThat(a.value().render(inputTemplate)).isEqualTo(expectedOutput);
}
}

public static boolean isDeferredExecutionMode() {
Expand Down
47 changes: 25 additions & 22 deletions src/test/java/com/hubspot/jinjava/lib/tag/MacroTagTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,38 +282,41 @@ public void itEnforcesMacroRecursionWithMaxDepth() throws IOException {
.build()
)
.newInterpreter();
JinjavaInterpreter.pushCurrent(interpreter);

try {
try (var a = JinjavaInterpreter.closeablePushCurrent(interpreter).get()) {
String template = fixtureText("ending-recursion");
String out = interpreter.render(template);
assertThat(interpreter.getErrorsCopy().get(0).getMessage())
.contains("Max recursion limit of 2 reached for macro 'hello'");
assertThat(out).contains("Hello Hello");
} finally {
JinjavaInterpreter.popCurrent();
}
}

@Test
public void itPreventsRecursionForMacroWithVar() {
String jinja =
"{%- macro func(var) %}" +
"{%- for f in var %}" +
"{{ f.val }}" +
"{%- endfor %}" +
"{%- endmacro %}" +
"{%- set var = {" +
" 'f' : {" +
" 'val': '{{ self }}'," +
" }" +
"} %}" +
"{% set self='{{var}}' %}" +
"{{ func(var) }}" +
"";
Node node = new TreeParser(interpreter, jinja).buildTree();
assertThat(interpreter.render(node))
.isEqualTo("{'f': {'val': '{'f': {'val': '{{ self }}'} }'} }");
interpreter =
new Jinjava(
JinjavaConfig.newBuilder().withNestedInterpretationEnabled(true).build()
)
.newInterpreter();
try (var a = JinjavaInterpreter.closeablePushCurrent(interpreter).get()) {
String jinja =
"{%- macro func(var) %}" +
"{%- for k,f in var.items() %}" +
"{{ f.val }}" +
"{%- endfor %}" +
"{%- endmacro %}" +
"{%- set var = {" +
" 'f' : {" +
" 'val': '{{ self }}'," +
" }" +
"} %}" +
"{% set self='{{var}}' %}" +
"{{ func(var) }}" +
"";
Node node = new TreeParser(interpreter, jinja).buildTree();
assertThat(interpreter.render(node))
.isEqualTo("{'f': {'val': '{'f': {'val': '{{ self }}'} }'} }");
}
}

@Test
Expand Down
29 changes: 25 additions & 4 deletions src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerExtendsTagTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,18 @@ public class EagerExtendsTagTest extends ExtendsTagTest {

@Before
public void eagerSetup() {
eagerSetup(false);
}

void eagerSetup(boolean nestedInterpretation) {
JinjavaInterpreter.popCurrent();
interpreter =
new JinjavaInterpreter(
jinjava,
context,
JinjavaConfig
.newBuilder()
.withNestedInterpretationEnabled(nestedInterpretation)
.withExecutionMode(EagerExecutionMode.instance())
.withLegacyOverrides(
LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build()
Expand Down Expand Up @@ -66,23 +72,38 @@ public void itDefersBlockInExtendsChildSecondPass() {

@Test
public void itDefersSuperBlockWithDeferred() {
eagerSetup(true);
expectedTemplateInterpreter.assertExpectedOutputNonIdempotent(
"defers-super-block-with-deferred"
);
}

@Test
public void itDefersSuperBlockWithDeferredSecondPass() {
eagerSetup(true);
context.put("deferred", "Resolved now");
expectedTemplateInterpreter.assertExpectedOutput(
"defers-super-block-with-deferred.expected"
);
context.remove(RelativePathResolver.CURRENT_PATH_CONTEXT_KEY);
expectedTemplateInterpreter.assertExpectedNonEagerOutput(
"defers-super-block-with-deferred.expected"
);
}

@Test
public void itDefersSuperBlockWithDeferredNestedInterp() {
eagerSetup(true);
expectedTemplateInterpreter.assertExpectedOutputNonIdempotent(
"defers-super-block-with-deferred-nested-interp"
);
}

@Test
public void itDefersSuperBlockWithDeferredNestedInterpSecondPass() {
eagerSetup(true);
context.put("deferred", "Resolved now");
expectedTemplateInterpreter.assertExpectedOutput(
"defers-super-block-with-deferred-nested-interp.expected"
);
}

@Test
public void itReconstructsDeferredOutsideBlock() {
expectedTemplateInterpreter.assertExpectedOutputNonIdempotent(
Expand Down
Loading