Render discrepancies with Python Jinja2 (none vs None, true vs True, double vs single quotes, tuple)

[tjsmith-meta]

I had created an issue previously for this here.
#660

I already commented on that closed issue, but thought folks may not see the latest comment due to it being closed (and I am unable to reopen the issue, hence creating this new one).

Rendering of tuples with parens and single quotes for Python Jinja2 compat is important for my use case (to be able to switch from Python Jinja2 to MiniJinja), so I'm wondering how and if that might be achievable with MiniJinja.

Would appreciate any guidance here and clarity on whether you would be open to such a change in MiniJinja. Thanks!

[mitsuhiko]

@tjsmith-meta I would love to learn a bit why that difference matters. I was hoping that this difference can be avoided because it adds another sequence type to the engine that for all intents and purposes works the same except for the rendering on display.

[tjsmith-meta]

Thank you very much for taking a look and responding!

Here's the more specific context (which I had left as a comment on #660 after it closed). Resharing here.

There's a pattern of using Jinja for SQL templating, like the following. (It's not tenable for us to change/"fix" the source Jinja templates.)

{% set foo = ('bar','baz') %}
SELECT *
FROM table
WHERE col IN {{ foo }}

With Python Jinja2 this renders to valid SQL.

SELECT *
FROM table
WHERE col IN ('bar', 'baz')

SQL requires parens and single quotes for the IN syntax. Swapping out for square brackets and/or double quotes is not valid SQL.

The double vs single quote difference between MiniJinja and Python Jinja2 for rendering lists is also important for a similar (albeit perhaps less prevalent for me) reason. (I'm assuming that if we change double quotes to single for rendered tuples that we would also change it for lists as well.)

{% set foo = ['bar','baz'] %}
SELECT ARRAY{{ foo }}

With Python Jinja2 this renders to valid SQL (needs single quotes). (MiniJinja does double quotes here instead.)

{% set foo = ['bar','baz'] %}
SELECT ARRAY['bar', 'baz']

For list vs tuple, my mental model is that you could have a wrapper "array type" that includes a type flag that indicates whether it is "list" or "tuple", where the type flag is only used in rendering the array type in the template. (In this way, the bulk of the MiniJinja engine operates over and treats them at one common type, and only rendering inspects that one flag, and parsing has a slight difference to populate that flag appropriately.)

For the switch to single quotes, I assume that should be a fairly straight-forward change scoped to the "renderer" component. (I'm guessing / assuming without having looked much at MiniJinja code.)

If you are concerned about backwards compatibility, I wonder if we could hide these behavior changes behind a "python compat flag" that I would be able to opt into (like via minijinja_contrib::pycompat).

I'm happy to help contribute a change here, assuming we can achieve alignment on moving forward and you provide me with guidance on how to go about making the change.

(There's also a small rendering difference for booleans -- true/false (MiniJinja) vs True/False (Python Jinja2), which is also an issue for me. Seems like if the double->single quote rendering is changed and it's an opt-in feature, then it would be easy enough to replicate a similar change for this bool difference as well.)

[tjsmith-meta]

I took a pass over the code to understand it a bit better. Here are some hacky incomplete changes that address rendering for none/bool/quotes, but doesn't touch the Tuple "()" part. I noticed the "map" handling code there as well using ":?" / debug, so I suspect there would need to be changes there as well.
none-bool-quotes-list.txt

Here's the test template I was using

{{ none }}
{{ true }}
{{ [none, true, 1, 1.1, 'str\'str'] }}
{{ (none, true, 1, 1.1, 'str\'str') }}
{{ [none, true, 1, 1.1, 'str"str'] }}
{{ (none, true, 1, 1.1, 'str"str') }}
{{ [none, true, 1, 1.1, 'str\'str"str'] }}
{{ (none, true, 1, 1.1, 'str\'str"str') }}

Python Jinja2 output

None
True
[None, True, 1, 1.1, "str'str"]
(None, True, 1, 1.1, "str'str")
[None, True, 1, 1.1, 'str"str']
(None, True, 1, 1.1, 'str"str')
[None, True, 1, 1.1, 'str\'str"str']
(None, True, 1, 1.1, 'str\'str"str')

Note that Python uses double quotes if the string contains single quotes and no double quotes inside it. For my use case, I believe this behavior will be unimportant and that just always using single quotes will work fine.

MiniJinja 2.9.0

none
true
[none, true, 1, 1.1, "str'str"]
[none, true, 1, 1.1, "str'str"]
[none, true, 1, 1.1, "str\"str"]
[none, true, 1, 1.1, "str\"str"]
[none, true, 1, 1.1, "str'str\"str"]
[none, true, 1, 1.1, "str'str\"str"]

MiniJinja with my patch above

None
True
[None, True, 1, 1.1, 'str\'str']
[None, True, 1, 1.1, 'str\'str']
[None, True, 1, 1.1, 'str"str']
[None, True, 1, 1.1, 'str"str']
[None, True, 1, 1.1, 'str\'str"str']
[None, True, 1, 1.1, 'str\'str"str']

Again, my hacky changes don't handle tuple '()', map/dict (switch away from debug), and likely could be written better / more performantly, etc. (I'm a Rust noob.)

[mitsuhiko]

So the issue you are running into here is that this only really worked by accident. For instance if you had the value ('foo\'', 42) as tuple it would render as ("foo'", 42) in Jinja2 which is invalid SQL too. So this already was a hazard independent of the tuple support.

The string representation of values within sequences was not stable in either version of Jinja. In the Python version that is up to whatever is underlying. For instance on Jinja2 in Python 2 you would have commonly seen [u'bar', u"'baz'"] show up, in Python 3 you will get inconsistent quoting. In MiniJinja it consistently quotes with " today but even that is unspecified.

Those were intended as debug outputs, not for you to rely on.

I think the issues here are deeper than missing tuple support. It's quite likely you are running into pre-existing templates that are buggy themselves.

[tjsmith-meta]

Mmm. Yeah, there are definitely edge cases where Python Jinja2 (we are on some version of Python 3) will not work for SQL generation, due to the string containing a single quote or some other cases like you pointed out.

That said, in practice, we are relying on the current behavior of Python Jinja2 in practice regarding how these elements are rendered, where "mostly single quotes" is important for us (with parens for Tuples). (In addition, capitalized None, True, False are also important in practice, for parity with Python Jinja2. And again, changing/fixing our existing templates isn't a realistic option for us.)

Would you be open to a pull request that adjusts this behavior for better Python Jinja2 parity behind a pycompat flag? I'm happy to do the work, as this has become a critical blocker for us. Would prefer to see this upstreamed if possible vs having a patch in our local copy in perpetuity.

[mitsuhiko]

I'm not sure that putting it behind a flag will help. The conversion is happening all over the place and no config is passed around. Eg: a filter with a String input type will run to_string() on it which will cause eg a bool to turn into true and not True.

I'm not really sure what a good move is here. It comes down to this:

• tuple: entirely unsupported in MiniJinja, would require adding a type and change all filters
• none -> None: this one is probably a rather uncontroversial change, it's just choice. People asking in the past for this to actually render into an empty string instead.
• true/false -> True/False: this was intentionally changed because it makes rendering basic flags to javascript and JSON easy without a filter. Maybe that's not a good reason, but outside of Python nothing really wants capitalized.

One option would be to use --cfg to change this, but that would not solve the tuple problem at all. Also would not help if you use this via the python binding.

A simple workaround for now would be to use a custom finalizer. That at least addresses the final rendering.

[tjsmith-meta]

Thank you for sharing context around some of the challenges and nuance here, and enumerating some potential options and considerations here. This is super helpful.

Also would not help if you use this via the python binding.

We use MiniJinja via the Rust binding, so perhaps --cfg could be viable for us as the "flag" mechanism to change the behavior. You mentioned "no config is passed around", so maybe your thought on potentially using --cfg was implying that the corresponding config object related to that might be plumbed around?

Can you help me understand the "custom finalizer" mechanism that you are referring to and how that might be more specifically leveraged here? Will this work for Tuple (and quotes), None and True/False?

[mitsuhiko]

You mentioned "no config is passed around", so maybe your thought on potentially using --cfg was implying that the corresponding config object related to that might be plumbed around?

--cfg is global so it would work. What i mean is that string conversion happens in places where there is no API to carry a config from the Environment around.

Can you help me understand the "custom finalizer" mechanism that you are referring to and how that might be more specifically leveraged here? Will this work for Tuple (and quotes), None and True/False?

You can use set_formatter to influence how stuff renders just before printing: https://docs.rs/minijinja/latest/minijinja/struct.Environment.html#method.set_formatter

[mitsuhiko]

So for the non tuple case I think we should be doing these changes:

1. introduce a new pycompat_rendering flag on the environment (including setter and getter)
2. introduce a new thread local for that flag
3. set that flag for render and eval to what's on the environment
4. update the value's Debug and Display implementation to honor the flag.

If the flag is true, then we match the exact rendering for True/False/None and strings from Python (single quotes preferred).

[tjsmith-meta]

I noticed that this issue is tagged "needs-more-information". Any information needed from my side here, or is that tag lingering from before?

I also see you've had a pull request for Python compatible rendering in #804 open for a bit. Any plans to get that Python compatible rendering landed? (Seems like it's been sitting idle for a bit now.)

[mitsuhiko]

I am still considering it. The needs more information is from your side because I am not sure to which degree what is planned actually addresses your question. Note that #804 still does not solve your tuple issue.