Skip to content
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.

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

Avoid tag cycles when keeping track of parent paths for blocks #363

Merged
merged 3 commits into from
Aug 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import com.hubspot.jinjava.random.DeferredRandomNumberGenerator;
import com.hubspot.jinjava.tree.Node;
import com.hubspot.jinjava.tree.TreeParser;
import com.hubspot.jinjava.tree.output.BlockInfo;
import com.hubspot.jinjava.tree.output.BlockPlaceholderOutputNode;
import com.hubspot.jinjava.tree.output.OutputList;
import com.hubspot.jinjava.tree.output.OutputNode;
Expand All @@ -58,7 +59,7 @@

public class JinjavaInterpreter {

private final Multimap<String, List<? extends Node>> blocks = ArrayListMultimap.create();
private final Multimap<String, BlockInfo> blocks = ArrayListMultimap.create();
private final LinkedList<Node> extendParentRoots = new LinkedList<>();

private Context context;
Expand Down Expand Up @@ -113,8 +114,8 @@ public void addExtendParentRoot(Node root) {
extendParentRoots.add(root);
}

public void addBlock(String name, LinkedList<? extends Node> value) {
blocks.put(name, value);
public void addBlock(String name, BlockInfo blockInfo) {
blocks.put(name, blockInfo);
}

/**
Expand Down Expand Up @@ -282,17 +283,28 @@ private void resolveBlockStubs(OutputList output, Stack<String> blockNames) {
for (BlockPlaceholderOutputNode blockPlaceholder : output.getBlocks()) {

if (!blockNames.contains(blockPlaceholder.getBlockName())) {
Collection<List<? extends Node>> blockChain = blocks.get(blockPlaceholder.getBlockName());
List<? extends Node> block = Iterables.getFirst(blockChain, null);
Collection<BlockInfo> blockChain = blocks.get(blockPlaceholder.getBlockName());
BlockInfo block = Iterables.getFirst(blockChain, null);

if (block != null) {
List<? extends Node> superBlock = Iterables.get(blockChain, 1, null);
if (block != null && block.getNodes() != null) {
List<? extends Node> superBlock = Optional.ofNullable(Iterables.get(blockChain, 1, null))
.map(BlockInfo::getNodes).orElse(null);
context.setSuperBlock(superBlock);

OutputList blockValueBuilder = new OutputList(config.getMaxOutputSize());

for (Node child : block) {
for (Node child : block.getNodes()) {
boolean pushedParentPathOntoStack = false;
if (block.getParentPath().isPresent() && !getContext().getCurrentPathStack().contains(block.getParentPath().get())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will only push the parent path onto the stack if the stack does not already contain the parent path

getContext().getCurrentPathStack().push(block.getParentPath().get(), lineNumber, position);
pushedParentPathOntoStack = true;
}

blockValueBuilder.addNode(child.render(this));

if (pushedParentPathOntoStack) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also will only pop the parent path off the stack if it was pushed onto the stack to begin with

getContext().getCurrentPathStack().pop();
}
}

blockNames.push(blockPlaceholder.getBlockName());
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/hubspot/jinjava/lib/tag/BlockTag.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
import com.hubspot.jinjava.interpret.TemplateSyntaxException;
import com.hubspot.jinjava.tree.TagNode;
import com.hubspot.jinjava.tree.output.BlockInfo;
import com.hubspot.jinjava.tree.output.BlockPlaceholderOutputNode;
import com.hubspot.jinjava.tree.output.OutputNode;
import com.hubspot.jinjava.util.HelperStringTokenizer;
Expand Down Expand Up @@ -57,7 +58,7 @@ public OutputNode interpretOutput(TagNode tagNode, JinjavaInterpreter interprete

String blockName = WhitespaceUtils.unquote(tagData.next());

interpreter.addBlock(blockName, tagNode.getChildren());
interpreter.addBlock(blockName, new BlockInfo(tagNode.getChildren(), interpreter.getContext().getCurrentPathStack().peek()));

return new BlockPlaceholderOutputNode(blockName);
}
Expand Down
27 changes: 27 additions & 0 deletions src/main/java/com/hubspot/jinjava/tree/output/BlockInfo.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package com.hubspot.jinjava.tree.output;

import java.util.List;
import java.util.Optional;

import com.hubspot.jinjava.tree.Node;

@SuppressWarnings("OptionalUsedAsFieldOrParameterType")
public class BlockInfo {

private final List<? extends Node> nodes;

private final Optional<String> parentPath;

public BlockInfo(List<? extends Node> nodes, Optional<String> parentPath) {
this.nodes = nodes;
this.parentPath = parentPath;
}

public List<? extends Node> getNodes() {
return nodes;
}

public Optional<String> getParentPath() {
return parentPath;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import java.time.ZonedDateTime;
import java.util.HashMap;
import java.util.Optional;

import org.junit.Before;
import org.junit.Test;
Expand All @@ -14,6 +15,7 @@
import com.hubspot.jinjava.interpret.JinjavaInterpreter.InterpreterScopeClosable;
import com.hubspot.jinjava.interpret.TemplateError.ErrorReason;
import com.hubspot.jinjava.tree.TextNode;
import com.hubspot.jinjava.tree.output.BlockInfo;
import com.hubspot.jinjava.tree.parse.TextToken;

public class JinjavaInterpreterTest {
Expand All @@ -40,14 +42,16 @@ public void resolveBlockStubsWithMissingNamedBlock() {

@Test
public void resolveBlockStubs() {
interpreter.addBlock("foobar", Lists.newLinkedList(Lists.newArrayList((new TextNode(new TextToken("sparta", -1, -1))))));
interpreter.addBlock("foobar", new BlockInfo(Lists.newLinkedList(Lists.newArrayList((new TextNode(new TextToken("sparta", -1, -1))))),
Optional.empty()));
String content = "this is {% block foobar %}foobar{% endblock %}!";
assertThat(interpreter.render(content)).isEqualTo("this is sparta!");
}

@Test
public void resolveBlockStubsWithSpecialChars() {
interpreter.addBlock("foobar", Lists.newLinkedList(Lists.newArrayList(new TextNode(new TextToken("$150.00", -1, -1)))));
interpreter.addBlock("foobar", new BlockInfo(Lists.newLinkedList(Lists.newArrayList(new TextNode(new TextToken("$150.00", -1, -1)))),
Optional.empty()));
String content = "this is {% block foobar %}foobar{% endblock %}!";
assertThat(interpreter.render(content)).isEqualTo("this is $150.00!");
}
Expand Down
14 changes: 14 additions & 0 deletions src/test/java/com/hubspot/jinjava/lib/tag/ExtendsTagTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,20 @@ public void itExtendsViaRelativePath() throws IOException {
assertThat(result).contains("This is a relative path extends");
}

@Test
public void itHandlesRelativePathsInBlocksInExtendingTemplate() throws IOException {
jinjava.getGlobalContext().put(CURRENT_PATH_CONTEXT_KEY, "relative/relative-block.jinja");
String result = jinjava.render(locator.fixture("relative/relative-block.jinja"), new HashMap<>());
assertThat(result).contains("hello");
}

@Test
public void itHandlesRelativePathsInBlocksFromExtendedTemplate() throws IOException {
jinjava.getGlobalContext().put(CURRENT_PATH_CONTEXT_KEY, "relative/relative-block-2.jinja");
String result = jinjava.render(locator.fixture("relative/relative-block-2.jinja"), new HashMap<>());
assertThat(result).contains("hello");
}

private static class ExtendsTagTestResourceLocator implements ResourceLocator {
private RelativePathResolver relativePathResolver = new RelativePathResolver();

Expand Down
1 change: 1 addition & 0 deletions src/test/resources/tags/extendstag/hello.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hello
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{% block test %}
{% include "./hello.html" %}
{% endblock test %}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{% extends "./../relative-block-2-base.jinja" %}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{% extends "./../super-base.html" %}
{% block sidebar %}
{% include "./../hello.html" %}
{% endblock %}