Interpreter overrides the child bindings in recursive context

[buremba]

We have a Jinja expression as follows:

{{someotherexpression}}

The someotherexpression is actually another Jinja expression and we render the actual value in the toString() of someotherexpression. Since they run in the same thread, JinjavaInterpreter.getCurrent() returns the parent interpreter for someothercontext but we're not able to use our bindings in this context.

In the case above, Jinjava gets the context for the parent interpreter and overrides all the values in the current context as follows: https://github.com/HubSpot/jinjava/blob/master/src/main/java/com/hubspot/jinjava/Jinjava.java#L234

I believe that we should use the following snippet in order not to override the child context:

// removed this:  bindingsWithParentContext.putAll(parentInterpreter.getContext());
for (Map.Entry<String, Object> entry : bindingsWithParentContext.entrySet()) {
    bindingsWithParentContext.putIfAbsent(entry.getKey(), entry.getValue());
}

[jasmith-hs]

@hs-lsong this seems right to me, but I don't know the full context around the time when this code was initially added in https://github.com/HubSpot/jinjava/pull/447

[buremba]

@jasmith-hs Just you give you some context, I had to make the change that I suggested in order to make it work for my use-case. I had a snippet something like jinjava.render('{{myvalue}}', mapOf('myvalue' to 5)) and somehow the output was 10 instead of 5. It was because the parent context has a variable myvalue set to 10 so no matter which value I pass, it was overridden.

[hs-lsong]

Thanks for filing this issue. Do you have a complete test so that we can run and confirm?

[buremba]

Sure, here it is:

    @Test
    fun recursiveExpression() {
        val renderer = Jinjava()
        println(renderer.render("{{proxy_call}}", mapOf("proxy_call" to ProxyCall(renderer), "globalvar" to 10)))
    }

    class ProxyCall(val renderer: Jinjava) {
        override fun toString(): String {
            return renderer.render("{{globalvar}}", mapOf("globalvar" to 5))
        }
    }

[hs-lsong]

Thanks for the example. I think it is dangerous to have recursive call for the rendering. Probably we should disable this.

[buremba]

@hs-lsong any alternative approach for what I'm trying to do? I don't want to use functions for convenience (better UX for us) and I can only render the values in a lazy way since it's expensive to fetch the values.

[hs-lsong]

It seems you can just call this directly, no? renderer.render("{{globalvar}}", mapOf("globalvar" to 5))

[buremba]

@hs-lsong, I probably oversimplified the example. :) Here is our renderer: https://github.com/metriql/metriql/blob/master/src/main/java/com/metriql/service/jinja/JinjaRendererService.kt

We let users define their data models (metrics) as Jinja expressions embedded into SQL and we render SQL for the underlying database using Jinja under the hood.

[rdehuyss]

I'm also having troubles with this.

@hs-lsong - why do you think it is dangerous to have recursive call for rendering? It allows for some really clean code and nice constructs.

[boulter]

If you trust the code, then recursion is fine. In HubSpot's case, we have code submitted by users who often don't terminate their code correctly or create cycles which are run in shared environments which is why we avoid recursion.