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.

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

Preserve Raw Tags config #518

Merged
merged 12 commits into from
Oct 26, 2020
+120 −3
Merged

Preserve Raw Tags config #518

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f632489
Add preserving raw tags for double-renderings
jasmith-hs Oct 9, 2020
4f120e5
Include image in exception, allow length limiting
jasmith-hs Oct 12, 2020
2798066
Handle preserved raw tag exception in TagNode
jasmith-hs Oct 12, 2020
09f47a2
Test preserving raw tags
jasmith-hs Oct 12, 2020
dd642d0
Exception isn't thrown here
jasmith-hs Oct 12, 2020
cc431d4
Add license
jasmith-hs Oct 12, 2020
229b277
Refactor to remove PreservedRawTagException
jasmith-hs Oct 21, 2020
17c5a1c
Add deferred preserving test
jasmith-hs Oct 21, 2020
bccbc98
Removed hasPreservedRawTags
jasmith-hs Oct 23, 2020
277cc64
Use more generic config flag name: "preserveForSecondPass"
jasmith-hs Oct 23, 2020
1e041b1
Add trailing newline to test jinja resource
jasmith-hs Oct 23, 2020
05b5b9c
Trim before comparing in raw test
jasmith-hs Oct 23, 2020
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
20 changes: 18 additions & 2 deletions 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 @@ -56,6 +56,7 @@ public class JinjavaConfig {
private TokenScannerSymbols tokenScannerSymbols;
private ELResolver elResolver;
private final boolean iterateOverMapKeys;
private final boolean preserveRawTags;

public static Builder newBuilder() {
return new Builder();
Expand Down Expand Up @@ -85,6 +86,7 @@ public JinjavaConfig(InterpreterFactory interpreterFactory) {
interpreterFactory,
new DefaultTokenScannerSymbols(),
JinjavaInterpreterResolver.DEFAULT_RESOLVER_READ_ONLY,
false,
false
);
}
Expand Down Expand Up @@ -114,6 +116,7 @@ public JinjavaConfig(
new JinjavaInterpreterFactory(),
new DefaultTokenScannerSymbols(),
JinjavaInterpreterResolver.DEFAULT_RESOLVER_READ_ONLY,
false,
false
);
}
Expand All @@ -137,7 +140,8 @@ private JinjavaConfig(
InterpreterFactory interpreterFactory,
TokenScannerSymbols tokenScannerSymbols,
ELResolver elResolver,
boolean iterateOverMapKeys
boolean iterateOverMapKeys,
boolean preserveRawTags
) {
this.charset = charset;
this.locale = locale;
Expand All @@ -158,6 +162,7 @@ private JinjavaConfig(
this.tokenScannerSymbols = tokenScannerSymbols;
this.elResolver = elResolver;
this.iterateOverMapKeys = iterateOverMapKeys;
this.preserveRawTags = preserveRawTags;
}

public Charset getCharset() {
Expand Down Expand Up @@ -240,6 +245,10 @@ public boolean isIterateOverMapKeys() {
return iterateOverMapKeys;
}

public boolean isPreserveRawTags() {
return preserveRawTags;
}

public static class Builder {
private Charset charset = StandardCharsets.UTF_8;
private Locale locale = Locale.ENGLISH;
Expand All @@ -263,6 +272,7 @@ public static class Builder {
private TokenScannerSymbols tokenScannerSymbols = new DefaultTokenScannerSymbols();
private ELResolver elResolver = JinjavaInterpreterResolver.DEFAULT_RESOLVER_READ_ONLY;
private boolean iterateOverMapKeys;
private boolean preserveRawTags;

private Builder() {}

Expand Down Expand Up @@ -371,6 +381,11 @@ public Builder withIterateOverMapKeys(boolean iterateOverMapKeys) {
return this;
}

public Builder withPreserveRawTags(boolean preserveRawTags) {
this.preserveRawTags = preserveRawTags;
return this;
}

public JinjavaConfig build() {
return new JinjavaConfig(
charset,
Expand All @@ -391,7 +406,8 @@ public JinjavaConfig build() {
interpreterFactory,
tokenScannerSymbols,
elResolver,
iterateOverMapKeys
iterateOverMapKeys,
preserveRawTags
);
}
}
Expand Down
16 changes: 16 additions & 0 deletions src/main/java/com/hubspot/jinjava/interpret/Context.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ public enum Library {
private final Set<String> resolvedFunctions = new HashSet<>();

private Set<Node> deferredNodes = new HashSet<>();
private boolean hasPreservedRawTags = false;

private final ExpTestLibrary expTestLibrary;
private final FilterLibrary filterLibrary;
Expand Down Expand Up @@ -290,6 +291,21 @@ public Set<Node> getDeferredNodes() {
return ImmutableSet.copyOf(deferredNodes);
}

public void handlePreservedRawTag() {
hasPreservedRawTags = true;
if (getParent() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this check all the way up the stack (excluding global) instead of just one level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not too sure what you mean. I am doing a recursive call, which I believe checks up the stack, excluding global: getParent().handlePreservedRawTag();

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I was looking for a while and I missed the recursion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if (parent.getParent() != null) { will execute the handlePreservedRawTag() for Global as well. Can you verify whether we need to have parent.getParent().getParent() != null to exclude the global one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like in #449, we don't want the shared global context to be altered here as it can be shared across threads. My goal with marking a context as hasPreservedTags is so that we know that a second pass must always be done if raw tags get preserved.
In the instance that we preserve all the raw tags, but nothing gets deferred, a second pass can be done when pre-rendering. If something is deferred, that second pass will happen at serve-time. I want to exclude it for global because I just want it to mark for one run of renderForResult as mentioned here: #447.
Does that answer what you were wondering?

Copy link
Contributor Author

@jasmith-hs jasmith-hs Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I misread that question. We only need to check if the parent of what we are calling handlePreservedTag() on is not null. I think maybe line 300: getParent().handlePreservedRawTag() is confusing, as it would make more sense for it to be parent.handlePreservedRawTag().
The if statement is checking to make sure that parent's parent is not null, if so then handle the preserved raw tag for parent

With a context tree shape like G-a-b, here's some sudo-sequence-code that uses questionable syntax:

this == b
parent == getParent() == a
parent.getParent() == G != null
parent.handlePreservedRawTag():
-this == a
-parent == getParent() == G
-parent.getParent() == null
-return
return

Context parent = getParent();
//Ignore global context
if (parent.getParent() != null) {
getParent().handlePreservedRawTag();
}
}
}

public boolean getHasPreservedRawTags() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this function is used. Is this needed?

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's for rendering implementations such as with HubSpot's internal pre-renderer

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I am missing the whole picture. Is this the case, the caller set the config to preserve raw tags, later it wants to confirm it did render with the flag set?

Copy link
Contributor Author

@jasmith-hs jasmith-hs Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So if the caller sets to preserve raw tags, but there are no raw tags to preserve and no deferred values, the pre-rendered template can be served as-is.

If however, there are any deferred values, then the second render will also get deferred until later.

Finally, if there are no deferred values, but raw tags are preserved, then the second-pass should happen immediately with the flag off to remove the raw tags and the result can be served as-is.

This is because there isn't a way of knowing if there are going to be deferred values down the line so it must always preserve raw tags if the flag is set. 3 examples assuming deferred is deferred:

Example 1, no deferred values, but a raw tag is preserved:

{% raw %}{{ foo.bar }}{% endraw %}
{{ resolved.variable }}

With preserveRawTags=true the output of the first render is:

{% raw %}{{ foo.bar }}{% endraw %}
I am resolved.

Since there are no deferred values, it should be rendered again with preserveRawTags=false. The output of the second render is:

{{ foo.bar }}
I am resolved.

Example 2, there are no deferred values or raw tags preserved:

Hello
{{ resolved.variable }}

With preserveRawTags=true the output of the first render is:

Hello
I am resolved.

Since there are no deferred values or preserved tags, the output of the first render can be served as-is.

Example 3, there is a deferred value and preserved raw tag:

{% raw %}{{ foo.bar }}{% endraw %}
{{ deferred.variable }}

With preserveRawTags=true the output of the first render is:

{% raw %}{{ foo.bar }}{% endraw %}
{{ deferred.variable }}

Since there is a deferred variable, it should be rendered again at serve-time with preserveRawTags=false. The output of the second render is:

{{ foo.bar }}
I was deferred, but am resolved at serve-time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the detailed examples. It helps a lot. But I still don't see where it needs to call getHasPreservedRawTags(). If the caller set it right before then calling the rendering, it should know it (local variable, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're mistaking the config option for this. The config option tells the interpreter to preserve the raw tags. getHasPreservedRawTags() is only true if a raw tag was encountered and preserved. If there were no raw tags, then getHasPreservedRawTags() will be false.

Essentially, the method lets the caller know if any raw tags were preserved. It prevents an unnecessary second pass from happening. If this method didn't exist, it wouldn't break anything, but Example 2 would have an extra rendering call that would yield the same result, which can be avoided by using this method.

There isn't a method to set private boolean hasPreservedRawTags, it is only set in the context when handlePreservedRawTag() gets called.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, my bad. I mis-read it. I am OK with this. However, if we do not have this, as you said, in example 2, we would have an extra rendering call -- it should be fast because there is nothing to render.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is true. I think it would be a minimal tradeoff of speed for more simplicity, so I'll just take this out.
This has shown me that it is too confusing to be worth the minimal performance gain

return hasPreservedRawTags;
}

public List<? extends Node> getSuperBlock() {
if (superBlock != null) {
return superBlock;
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/hubspot/jinjava/lib/tag/RawTag.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ public String getName() {

@Override
public String interpret(TagNode tagNode, JinjavaInterpreter interpreter) {
if (interpreter.getConfig().isPreserveRawTags()) {
interpreter.getContext().handlePreservedRawTag();
return renderNodeRaw(tagNode);
}

LengthLimitingStringBuilder result = new LengthLimitingStringBuilder(
interpreter.getConfig().getMaxOutputSize()
);
Expand Down
20 changes: 20 additions & 0 deletions src/test/java/com/hubspot/jinjava/interpret/JinjavaInterpreterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,4 +220,24 @@ public void itLimitsOutputSizeWhenSumOfNodeSizesExceedsMax() {
assertThat(renderResult.getErrors().get(0).getMessage())
.contains("OutputTooBigException");
}

@Test
public void itCanPreserveRawTags() {
JinjavaConfig preserveRawTagsConfig = JinjavaConfig
.newBuilder()
.withPreserveRawTags(true)
.build();
String input = "1{% raw %}2{% endraw %}3";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also add a test for deferred values? I thought deferred values will not get evaluated normally. Do they get evaluated inside raw tags?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My testing shows that deferred inside raw does not get evaluated either. https://github.com/HubSpot/jinjava/compare/test-deferred?expand=1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing, including deferred tags, will get evaluated inside raw tags. However without the preserveRawTags flag, on the second pass, the deferred values will get evaluated once the real value is put in the context.

Copy link
Collaborator

@hs-lsong hs-lsong Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In my test above, the deferred values do not get evaluated in the second rendering (there is no preserveRawTags) . Maybe we are not talking about the same case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After line 83:

interpreter.getContext().put("deferred", "resolved value");

In the second pass, deferred values will no longer be deferred (such as a contact, which have the actual contact value in the second pass, rather than being a deferred value). If you run the test after putting that line in, I believe that it will get resolved, which is not what is desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what Jack meant by second pass is another renderer pass without having DeferredValue.instance() in the context. But in your test case, the test setUp has localContext.put("deferred", DeferredValue.instance()); so it gets deferred in the second pass as well. Yo need to remove that from the context to actually resolve them in the second pass.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, what is the use case for those? First round rendering makes it deferred, then second round makes it not deferred, but still don't want to render it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<div>
Hi {{ contact.firstname }}!
I personalized this with Jinjava:
{% raw %}Hi {{ contact.firstname }}!{% endraw %}
</div>

This should display for someone like:

<div>
Hi Jack!
I personalized this with Jinjava:
Hi {{ contact.firstname }}!
</div>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We wouldn't want it to render like:

<div>
Hi Jack!
I personalized this with Jinjava:
Hi Jack!
</div>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. Thanks.

String normalOutput = "123";
String preservedOutput = "1{% raw %}2{% endraw %}3";

RenderResult renderResult = new Jinjava().renderForResult(input, new HashMap<>());
assertThat(renderResult.getOutput()).isEqualTo(normalOutput);
assertThat(renderResult.hasErrors()).isFalse();

renderResult =
new Jinjava(preserveRawTagsConfig).renderForResult(input, new HashMap<>());
assertThat(renderResult.getOutput()).isEqualTo(preservedOutput);
assertThat(renderResult.hasErrors()).isFalse();
}
}
77 changes: 76 additions & 1 deletion src/test/java/com/hubspot/jinjava/lib/tag/RawTagTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

import com.google.common.io.Resources;
import com.hubspot.jinjava.Jinjava;
import com.hubspot.jinjava.JinjavaConfig;
import com.hubspot.jinjava.interpret.DeferredValue;
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
import com.hubspot.jinjava.tree.Node;
import com.hubspot.jinjava.tree.TagNode;
Expand All @@ -17,12 +19,14 @@
import org.junit.Test;

public class RawTagTest {
Jinjava jinjava;
JinjavaInterpreter interpreter;
RawTag tag;

@Before
public void setup() {
interpreter = new Jinjava().newInterpreter();
jinjava = new Jinjava();
interpreter = jinjava.newInterpreter();
tag = new RawTag();
}

Expand Down Expand Up @@ -91,6 +95,77 @@ public void itDoesntProcessJinjaCommentsWithinARawBlock() {
.contains("{{#each people}}");
}

@Test
public void itPreservesRawTags() {
TagNode tagNode = fixture("hubl");
JinjavaInterpreter preserveInterpreter = new JinjavaInterpreter(
jinjava,
jinjava.getGlobalContextCopy(),
JinjavaConfig.newBuilder().withPreserveRawTags(true).build()
);
String result = tag.interpret(tagNode, preserveInterpreter);
try {
assertThat(result)
.isEqualTo(
Resources.toString(
Resources.getResource("tags/rawtag/hubl.jinja"),
StandardCharsets.UTF_8
)
);
} catch (IOException e) {
throw new RuntimeException(e);
}
}

@Test
public void itPreservesDeferredWhilePreservingRawTags() {
TagNode tagNode = fixture("deferred");
JinjavaInterpreter preserveInterpreter = new JinjavaInterpreter(
jinjava,
jinjava.getGlobalContextCopy(),
JinjavaConfig.newBuilder().withPreserveRawTags(true).build()
);
preserveInterpreter.getContext().put("deferred", DeferredValue.instance());
interpreter.getContext().put("deferred", DeferredValue.instance());

String preservedResult = tag.interpret(tagNode, preserveInterpreter);
String nonPreservedResult = tag.interpret(tagNode, interpreter);
try {
assertThat(preservedResult)
.isEqualTo(
Resources.toString(
Resources.getResource("tags/rawtag/deferred.jinja"),
StandardCharsets.UTF_8
)
);
} catch (IOException e) {
throw new RuntimeException(e);
}
assertThat(nonPreservedResult).isEqualTo("{{ deferred }}");

// Should not get evaluated because it's wrapped in a raw tag.
String deferredRealValue = "Resolved value.";
preserveInterpreter.getContext().put("deferred", deferredRealValue);
interpreter.getContext().put("deferred", deferredRealValue);
String preservedIdempotent = tag.interpret(
(TagNode) new TreeParser(preserveInterpreter, preservedResult)
.buildTree()
.getChildren()
.getFirst(),
preserveInterpreter
);
String secondPass = tag.interpret(
(TagNode) new TreeParser(interpreter, preservedResult)
.buildTree()
.getChildren()
.getFirst(),
interpreter
);

assertThat(preservedIdempotent).isEqualTo(preservedResult);
assertThat(secondPass).isEqualTo("{{ deferred }}");
}

private TagNode fixture(String name) {
return (TagNode) fixtures(name).getFirst();
}
Expand Down
1 change: 1 addition & 0 deletions src/test/resources/tags/rawtag/deferred.jinja
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{% raw %}{{ deferred }}{% endraw %}