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

Allow partial evaluation of templates #282

Merged
merged 9 commits into from Feb 20, 2019
Merged

Conversation

pfarrel
Copy link
Contributor

@pfarrel pfarrel commented Feb 18, 2019

This PR implements partial evaluation of templates, or, deferring the evaluation of parts of a template, however you want to think about it.

The aim here is to support a use case for HubSpot's email infrastructure. We want to evaluate as much as possible of an email's template, except for the parts which are specific to an individual contact. This will allow us to minimise the work which needs to be repeated per-contact for large email sends to 100,000s of contacts. In this case the CPU time for evaluating the template isn't really a concern, but many Tags perform I/O, and this is the part we really don't want to repeat more than necessary.

How it works:

  • I've defined a new "marker" object - DeferredValue.
  • The user can insert these objects into their template context wherever they want. For example, in email's use case we would do context.put("contact, DeferredValue.instance()), because we want to defer the per-contact properties which will appear in the email.
  • When the ExpressionResolver encounters one of these objects, for example when encountering the expression {{contact.firstname}} while rendering a template, it will throw a DeferredValueException.
  • DeferredValueException is caught in a few specific places. When caught the engine attempts to just echo the Node / Tag / Expression which caused the exception, leaving it untouched.
  • A later run over the output can then fill in the parts which were not evaluated.

Concerns:

  • {% raw %} wrapped content will be lost in the deferred evaluation scenario, because they will be evaluated twice. I could add a new mode to the tag which rewrapped its output in another raw wrapper?
  • I'm pretty unsure about what I've done in TagNode. It seems to work, but will definitely need some more work.
  • Yes this is using exceptions for flow control, but it works out really elegantly. The other alternative would be constantly checking the resolvedExpressions collection and throwing if we see something being added which we don't like. Maybe this would be better? I think the exception approach is more flexible, because we can do things like making functions throw DeferredValueException under certain circumstances, like if the function returned the current time and we are in deferred evaluation mode.

@codecov-io
Copy link

codecov-io commented Feb 18, 2019

Codecov Report

Merging #282 into master will decrease coverage by <.01%.
The diff coverage is 58.62%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #282      +/-   ##
============================================
- Coverage     72.35%   72.35%   -0.01%     
- Complexity     1483     1503      +20     
============================================
  Files           231      234       +3     
  Lines          4597     4662      +65     
  Branches        737      743       +6     
============================================
+ Hits           3326     3373      +47     
- Misses         1016     1034      +18     
  Partials        255      255
Impacted Files Coverage Δ Complexity Δ
...a/com/hubspot/jinjava/interpret/DeferredValue.java 100% <100%> (ø) 3 <3> (?)
.../jinjava/random/RandomNumberGeneratorStrategy.java 100% <100%> (ø) 1 <0> (ø) ⬇️
...rc/main/java/com/hubspot/jinjava/tree/TagNode.java 70% <100%> (+10.9%) 9 <2> (+2) ⬆️
...ava/com/hubspot/jinjava/el/ExpressionResolver.java 90.19% <100%> (+0.83%) 11 <0> (+2) ⬆️
...spot/jinjava/interpret/DeferredValueException.java 100% <100%> (ø) 2 <2> (?)
.../hubspot/jinjava/interpret/JinjavaInterpreter.java 79.88% <71.42%> (+1.57%) 53 <0> (+3) ⬆️
.../jinjava/random/DeferredRandomNumberGenerator.java 9.09% <9.09%> (ø) 2 <2> (?)
...a/com/hubspot/jinjava/lib/filter/FormatFilter.java 100% <0%> (ø) 6% <0%> (+3%) ⬆️
... and 4 more

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 8de4520...a9fb902. Read the comment docs.

Copy link
Contributor

@boulter boulter left a comment

Choose a reason for hiding this comment

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

This is pretty neat!

builder.append(n.getMaster().getImage());
}

builder.append("{% ").append(getEndName()). append(" %}");
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use/create constants for {% and %}? People have requested to make these configurable and I'd like to consolidate them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@@ -84,4 +86,17 @@ public Tag getTag() {
return tag;
}


private String reconstructImage() {
StringBuilder builder = new StringBuilder().append(master.getImage());
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could store a reference to the original template string and an index value of the start and end n the node object. I think this would solve the raw case and preserve all the whitespace.

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'll try to get that working in a separate PR. If I can get it to work, I'll loop back and update this PR to use 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.

Actually, it looks like it might require a larger version bump because I will need to mess around with Tag / Node etc constructors. I'll go ahead with this for now and come back to that as I refine this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants