HubSpot / jinjava

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

Allow imported files to have variables sharing alias of import name #1118

Closed jasmith-hs closed 11 months ago

jasmith-hs commented 11 months ago

There's a bug where if you use a variable that shares the same name as the alias which you're importing the current file, eager execution will not reconstruct the output properly.

This was attempted to be handled in a graceful failure way, but it didn't catch all cases: https://github.com/HubSpot/jinjava/blob/15f33ad9a3b97f3e1aa86e9f9bc7aa6547feecb1/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTag.java#L461-L475 This PR handles it in a successful way instead, including handling additional cases.

The issue is easily explained through an example.

{# child.jinja #}
{% set where_am_i = 'here' %}
{% set my_var = {} %}
{% do my_var.update(deferred) %}
{# main.jinja #}
{% import 'child.jinja' as my_var %}
{{ my_var }}

The eager execution output was like (prettified):

{% do %}
  {% set current_path = 'child.jinja' %}
  {% set my_var = {}  %}
  {% for __ignored__ in [0] %}
    {% set my_var = {}  %}
    {% do my_var.update(deferred) %}
    {% do my_var.update({'where_am_i': 'here','import_resource_path': 'child.jinja'}) %} // <- We're storing where_am_i on the wrong map!
  {% endfor %}
  {% set current_path = '' %}
{% enddo %}
{{ my_var }}

Since my_var is declared again inside of the child scope, when we try to add where_am_i to the aliased my_var, we're actually just adding it to the other dictionary, which will result in the alias ultimately being empty.

Now the output is like (prettified):

{% do %}
  {% set current_path = 'child.jinja' %}
  {% set __temp_import_alias_1059697132__ = {}  %}
  {% for __ignored__ in [0] %}
    {% set my_var = {}  %}
    {% do __temp_import_alias_1059697132__.update({'my_var': my_var}) %}
    {% do my_var.update(deferred) %}
    {% do __temp_import_alias_1059697132__.update({'where_am_i': 'here','my_var': my_var,'import_resource_path': 'child.jinja'}) %}
  {% endfor %}
  {% set my_var = __temp_import_alias_1059697132__ %}
  {% set current_path = '' %}
{% enddo %}
{{ my_var }}

Now we're using a temporary alias, which uses the hashcode of the full import alias (which in the case of nested importing could look like x.y.z) so that we avoid name clashing. The my_var is created in the child scope, but it is updated on the temporary alias, which exists in the higher scope, and eventually we set the higher-scoped my_var alias to the temporary alias and all is well.


In making this change, I also split out the EagerImportTag's logic into two separate strategies:

because there were so many places where we'd check if the alias is empty, do this otherwise do that. There's enough of a distinction between the way that {% import 'child.jinja' %} and {% import 'child.jinja' as child %} function that using the strategy pattern is beneficial. Here is the diff, omitting that refactor: https://github.com/HubSpot/jinjava/pull/1118/files/504e96bc1c2893e1a04f923fb5c5beeb62163047..c9a11652064dd05c0ed78174298f5211bba4760c

The changes really boil down to two commits: