defer variables containing deferred vals & track deferred nodes in context #327

jkollmann · 2019-04-30T10:31:54Z

Following #282

Make deferred infectious for variables
It's possible that variables contain deferred values, in that case the assigned variable(s) should be deferred too, since expressions depending on variables containing deferred values can't and shouldn't be evaluated & rendered.

Example:

Input:

<html>
{% set some_var = "email:" + contact.email %} {% if contact.email  %}{{ contact.email }}{% endif %}
<head></head>
<body></body>
</html>

Render before the change:

<html>
{% set some_var = "email:" + contact.email %}
<head></head>
<body></body>
</html>

After the change:

<html>
{% set some_var = "email:" + contact.email %}
{% if contact.email  %}
{{ contact.email }}
{% endif %}
<head></head>
<body></body>
</html>

Add list to the context to track deferred nodes
We need some way of tracking which nodes were deferred so that we can escape them before post-processing the rendered template with Jsoup. If Jsoup encounters these kinds of tags in a place where no characters are allowed it changes the template and even ignores following tags inside of that tag.

Example:

Input:

<html>
{% set some_var = "email:" + contact.email %}
{% if contact.email  %}
{{ contact.email }}
{% endif %}
<head>
<meta name="description" content="some content">
</head>
<body></body>
</html>

Jsoup output:

<html>
 <head></head>
 <body>
{% set some_var = "email:" + contact.email %}
{% if contact.email %}
{{ contact.email }}
{% endif %}
  <meta name="description" content="some content">
 </body>
</html>

In order to escape these tags properly we need to know which tags were deferred.

We also want to handle the case where a deferred tag contains not-deferred variables. We need to be able to take these out of the context later for further processing.

Example:

Input:

<html>
{% set defined_var = "defined" %}
{% set some_var = "email:" + contact.email + defined_var  %} 
{% if contact.email  %}
{{ contact.email }}
{% endif %}
<head>
<meta name="description" content="some content">
</head>
<body></body>
</html>

Output:

<html>
{% set some_var = "email:" + contact.email + defined_var  %} 
{% if contact.email  %}
{{ contact.email }}
{% endif %}
<head>
<meta name="description" content="some content">
</head>
<body></body>
</html>

defined_var disappears but some_var will not be rendered since it contains a deferred value. If we know which tags have been deferred after the render we can take the vars out of the context and pass them along for a second render.

Considerations: I've tried a couple of different approaches, including partial evaluation and extracting variables of deferred tags. Both are a lot more complex than I initially thought and too specific to our use-case, especially given the fact that we don't own ExpressionFactoryImpl and all the code that this class depends on. IMO this is the most flexible solution, since users can decide what to do with the deferred nodes after rendering is done.

codecov-io · 2019-04-30T10:43:17Z

Codecov Report

Merging #327 into master will increase coverage by 0.02%.
The diff coverage is 90%.

@@             Coverage Diff              @@
##             master     #327      +/-   ##
============================================
+ Coverage     71.47%   71.49%   +0.02%     
- Complexity     1572     1575       +3     
============================================
  Files           239      239              
  Lines          4939     4954      +15     
  Branches        797      799       +2     
============================================
+ Hits           3530     3542      +12     
- Misses         1124     1126       +2     
- Partials        285      286       +1

Impacted Files	Coverage Δ	Complexity Δ
.../hubspot/jinjava/interpret/JinjavaInterpreter.java	`78.53% <0%> (-0.45%)`	`53 <0> (ø)`
.../java/com/hubspot/jinjava/tree/ExpressionNode.java	`80.95% <100%> (+0.95%)`	`7 <0> (ø)`	⬇️
.../main/java/com/hubspot/jinjava/lib/tag/SetTag.java	`77.77% <100%> (+3.86%)`	`8 <1> (+1)`	⬆️
...in/java/com/hubspot/jinjava/interpret/Context.java	`80.52% <85.71%> (+0.19%)`	`87 <2> (+2)`	⬆️
...rc/main/java/com/hubspot/jinjava/tree/TagNode.java	`73.68% <88.88%> (-1.32%)`	`9 <0> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3355490...4cc4a12. Read the comment docs.

src/main/java/com/hubspot/jinjava/interpret/Context.java

pfarrel · 2019-05-03T12:35:25Z

src/main/java/com/hubspot/jinjava/lib/tag/SetTag.java

+        interpreter.getContext().put(var, interpreter.resolveELExpression(expr, tagNode.getLineNumber()));
+      }
+    } catch (DeferredValueException e) {
+      Stream.of(varTokens).forEach(varToken -> interpreter.getContext().put(varToken.trim(), DeferredValue.instance()));


This is the key bit that makes "deferred-ness" infectious, for any other readers trying to see what actually changed through the whitespace diff.

pfarrel

Looks good

jkollmann requested review from boulter and pfarrel April 30, 2019 10:31

boulter requested changes Apr 30, 2019

View reviewed changes

src/main/java/com/hubspot/jinjava/interpret/Context.java Show resolved Hide resolved

jkollmann force-pushed the set-tag-deferred-value branch from 84be45e to e5db0b4 Compare May 1, 2019 10:21

jkollmann requested a review from boulter May 1, 2019 10:33

jkollmann force-pushed the set-tag-deferred-value branch from e5db0b4 to 43a36ab Compare May 2, 2019 14:42

pfarrel reviewed May 3, 2019

View reviewed changes

pfarrel approved these changes May 3, 2019

View reviewed changes

jkollmann added 5 commits May 3, 2019 14:45

Defer set tag with deferred val & store deferred nodes in list

c712d4a

Formatting

42b05d9

clear deferredNodes on reset

c3e14fa

Fix reconstructImage, create new HashMap instead of clear

935136e

Use set instead of list

4cc4a12

jkollmann force-pushed the set-tag-deferred-value branch from 2a99110 to 4cc4a12 Compare May 3, 2019 13:47

jkollmann merged commit 3c165b6 into master May 3, 2019

jkollmann deleted the set-tag-deferred-value branch May 3, 2019 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

defer variables containing deferred vals & track deferred nodes in context #327

defer variables containing deferred vals & track deferred nodes in context #327

jkollmann commented Apr 30, 2019 •

edited

codecov-io commented Apr 30, 2019 •

edited

pfarrel May 3, 2019

pfarrel left a comment

defer variables containing deferred vals & track deferred nodes in context #327

defer variables containing deferred vals & track deferred nodes in context #327

Conversation

jkollmann commented Apr 30, 2019 • edited

codecov-io commented Apr 30, 2019 • edited

Codecov Report

pfarrel May 3, 2019

Choose a reason for hiding this comment

pfarrel left a comment

Choose a reason for hiding this comment

jkollmann commented Apr 30, 2019 •

edited

codecov-io commented Apr 30, 2019 •

edited