Preserve Raw Tags config by jasmith-hs · Pull Request #518 · HubSpot/jinjava

jasmith-hs · 2020-10-12T16:37:14Z

Adds an option to the JinjavaConfig to preserve raw tags. This option can be set while performing a pre-render with the possibility that there will be deferred values. If a value like contact is deferred, and there is a raw tag like: {% raw %}Hello {{contact.firstname}}!{% endraw %}, the raw tags must be preserved during the first rendering pass, otherwise, on the second pass, Hello {{contact.firstname}}! will get resolved like Hello Jack! instead of remaining, verbatim.

When no values are being deferred, (ie the final render), preserveRawTags should be set to false, and then raw tags will be removed from the rendered output, and leave their children as they were written.

boulter

I have a vague recollection of some other workaround for raw tags, and I'm surprised that @Joeoh's work doesn't already cover it. I could be wrong though.

boulter · 2020-10-13T14:26:31Z


+  public void handlePreservedRawTag() {
+    hasPreservedRawTags = true;
+    if (getParent() != null) {


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

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

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

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?

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?

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

boulter · 2020-10-13T14:30:14Z

+      jinjava.getGlobalContextCopy(),
+      JinjavaConfig.newBuilder().withPreserveRawTags(true).build()
+    );
+    Throwable throwable = catchThrowable(


You can use assertThatThrownBy() here.

hs-lsong · 2020-10-20T20:34:00Z

    );
+    if (interpreter.getConfig().isPreserveRawTags()) {
+      result.append(renderNodeRaw(tagNode));
+      throw new PreservedRawTagException(


Why throw exception here instead of

return result.toString();

?

After working with the code base a bit more, I agree that it is simpler to return rather than throw an exception, and call interpreter.getContext().handlePreservedRawTag(); from here instead of in the TagNode

hs-lsong · 2020-10-20T20:44:36Z

+      .newBuilder()
+      .withPreserveRawTags(true)
+      .build();
+    String input = "1{% raw %}2{% endraw %}3";


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?

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

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.

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?

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.

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.

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.

<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>

We wouldn't want it to render like:

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

Got it. Thanks.

hs-lsong · 2020-10-21T17:16:30Z

+    }
+  }
+
+  public boolean getHasPreservedRawTags() {


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

It's for rendering implementations such as with HubSpot's internal pre-renderer

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?

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.

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).

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.

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.

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

jasmith-hs · 2020-10-23T15:29:32Z

I simplified this further by removing the handle method, and made the config option more generic: preserveForSecondPass.
Let me know if this looks good to merge.

hs-lsong

LGTM. However, I think preserveRawTags is more explicit. Just reading preserveForSecondPass, people may not clear on what are preserved.

boulter · 2020-10-26T14:30:03Z

Agreed with @hs-lsong on the naming. What if we wanted to render raw tags on the nth pass?

jasmith-hs · 2020-10-26T14:43:38Z

My reasoning is that we may want to preserve more than just raw tags when pre-rendering so rather than creating config options for each type of tag to preserve, it would be better to have one config option that signifies that a second pass will happen, and raw, etc. tags should be preserved.

I could take away the "second" which implies 2 passes total and make it preserveForFinalPass

jasmith-hs added 6 commits October 9, 2020 17:24

Add preserving raw tags for double-renderings

f632489

Include image in exception, allow length limiting

4f120e5

Handle preserved raw tag exception in TagNode

2798066

Test preserving raw tags

09f47a2

Exception isn't thrown here

dd642d0

Add license

cc431d4

jasmith-hs requested review from boulter and hs-lsong October 12, 2020 17:09

boulter requested review from Joeoh and gobimcp October 13, 2020 14:27

boulter reviewed Oct 13, 2020

View reviewed changes

hs-lsong reviewed Oct 20, 2020

View reviewed changes

jasmith-hs added 2 commits October 21, 2020 09:20

Refactor to remove PreservedRawTagException

229b277

Add deferred preserving test

17c5a1c

hs-lsong reviewed Oct 21, 2020

View reviewed changes

jasmith-hs added 3 commits October 23, 2020 11:21

Removed hasPreservedRawTags

bccbc98

Use more generic config flag name: "preserveForSecondPass"

277cc64

Add trailing newline to test jinja resource

1e041b1

jasmith-hs requested review from boulter and hs-lsong October 23, 2020 15:29

hs-lsong approved these changes Oct 23, 2020

View reviewed changes

Trim before comparing in raw test

05b5b9c

jasmith-hs merged commit cc5905f into HubSpot:master Oct 26, 2020

jasmith-hs deleted the preserve-raw-first-pass branch October 26, 2020 14:22

jasmith-hs mentioned this pull request Oct 26, 2020

Change config name to preserveForFinalPass #520

Merged

Uh oh!

Conversation

jasmith-hs commented Oct 12, 2020

Uh oh!

boulter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasmith-hs Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hs-lsong Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasmith-hs Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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

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

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasmith-hs commented Oct 23, 2020

Uh oh!

hs-lsong left a comment

Choose a reason for hiding this comment

Uh oh!

boulter commented Oct 26, 2020

Uh oh!

jasmith-hs commented Oct 26, 2020

Uh oh!

Reviewers

jasmith-hs Oct 21, 2020 •

edited

Loading

hs-lsong Oct 21, 2020 •

edited

Loading

jasmith-hs Oct 21, 2020 •

edited

Loading