HubSpot / jinjava

Jinja template engine for Java
Apache License 2.0
690 stars 168 forks source link

Handle deferred modification of an outer scoped variable inside a call tag #1144

Closed jasmith-hs closed 8 months ago

jasmith-hs commented 8 months ago

A {% call %} tag has some tricky scoping.

It functions by creating a child scope and putting temporary macro function caller() on that scope. When the macro function evaluates, it will also create a child scope. Part of eager execution's logic is to make sure that we don't reconstruct a variable on the wrong scope. So it won't reconstruct a variable on an inner scope if it was originally declared on an outer scope.

What this means is that we must ask it to reconstruct the macro function and the modified variable separately, since they both need to be deferred as a result of the call tag, but exist on different scopes since the call tag creates multiple scopes:

1. {"my_list": ['a']} // outer scope
-2. {"__macros__": [caller()]} // scope created to hold temporary caller macro
--3. {} // macro function's scope

So we must call getPrefixToPreserveState() when on level 2 and level 1.

hs-lsong commented 8 months ago

I have been looking at partial rendering simplifications. When I looked at this example, I wonder if we can produce the partially rendered result like this

[a, b, {{deferred}}]
[a, b, {{deferred}}, d]

I tried to modify a few lines of the code, and was able to produce that. However, what would be the unexpected consequence of this simplification? This piece of code in isolation should be safe, but it may not be applicable in complex nested situations.

I just throw some thoughts on this.

jasmith-hs commented 8 months ago

I have been looking at partial rendering simplifications. When I looked at this example, I wonder if we can produce the partially rendered result like this

[a, b, {{deferred}}]
[a, b, {{deferred}}, d]

I tried to modify a few lines of the code, and was able to produce that. However, what would be the unexpected consequence of this simplification? This piece of code in isolation should be safe, but it may not be applicable in complex nested situations.

I just throw some thoughts on this.

@hs-lsong I like the exploration of alternative options here, and implementing something like you suggested would be possible.

The eager execution output would look simpler in cases where the list is just output to a string, but in most other cases it would unfortunately not look simpler (the first phase output would look about the same) and it would be more difficult to construct.

It would require extra context about what the operations do. For the List#append operation, we'd append a DeferredValue to the list, but for some other operation, what would we do? The EL expression Ast nodes operate without knowledge of the context which they are used in. AstIdentifier is what converts deferred -> DeferredValue.of(), foo -> "my foo value", etc, but it doesn't know in what context that value will be used. Right now, the AstIdentifier will throw a DeferredParsingException when encountering a deferred value. If we didn't handle that there are the most fundamental level, we'd need to handle DeferredValue inputs to any parameter and throw DeferredParsingException throughout any operation, function, filter registered in jinjava.

For example, the + operator would now need to handle DeferredValue as a possible input. For my_list + deferred possibly we could allow adding a DeferredValue to the my_list collection, but 5 + deferred would have to throw a DeferredParsingException.

If we can make the changes to fully resolve EL expressions containing deferred values, we would end up with a more difficult list that we'd need to reconstruct if some jinjava code attempts to access the deferred value. We'd need to store in the list a reference to what the deferred value is when we want to access it:

{% macro my_macro(input) %}
{% set next_value = input %}
{% if next_value ==0 %}
{% set next_value = deferred %}
{% endif %}
{% do my_list.append(next_value) %}
{% endmacro %}

If we just stored that as DeferredValue.of("{{next_value}}") or something similar that points to next_value, then it will not exist when we want to use my_list, since next_value is a temporary variable that exists in the scope of the macro function.

Another problem we would run into would be that, for functions, filters, etc that take in Collection or Map arguments, they would also need to handle DeferredValue being an element of that collection or map. For example, the |sort filter would need to know that it can't sort a list if some elements are deferred because they aren't yet sortable.

There are more hurdles that we'd have to work through if we wanted to go with such a solution. It would be possible to do, but even if we did solve all hurdles, I don't it as being worth the effort as we'd still be handling the resolving and reconstructing of the DeferredValue somewhere.

What I've found to be the simplest solution to these problems of how to treat a DeferredValue is what is currently implemented: Don't allow the resolving of any value that is deferred, therefore it can't be used in any functions, filters, macro calls, operations, etc. This strategy significantly limits the complexity of how a DeferredValue is handled. Even with that limit: the complexity of scoping, variables referencing each other, and other cool features of Jinjava make it very complex to correctly reconstruct a DeferredValue.

hs-lsong commented 8 months ago

Thanks for the new example and explanation. Since that new example is just a macro, after rendering it would be empty. I add some caller, and found that would still produce simple, more readable output.

"{%- set my_list = ['a'] -%}\n" +
 "{%- call my_macro(0) -%}{%- endcall -%}\n" +
 "{{ my_list }}";

I defined a list, and call the macro. If the input=0, I will get the {{deferred}},

"[a, {{deferred}}]"

If the input is not 0, I get the input, e.g.,

"['a', 100]"

or, if the input is deferred context.put("deferred2", DeferredValue.instance());,

"[a, {{deferred2}}]"
jasmith-hs commented 7 months ago

What are you storing in the my_list?