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

Add support for the Environment to optionally return native types. #708

Merged
merged 5 commits into from Oct 31, 2017
Merged

Add support for the Environment to optionally return native types. #708

merged 5 commits into from Oct 31, 2017

Conversation

jctanner
Copy link
Contributor

This works by having an alternate CodeGenerator that avoids doing to_string
after the yield statement and a new version of concat that handles the returned
generator with a bit more "intelligence".

Related to ansible/ansible#23943

We use jinja heavily in the ansible project. Although it seems to target a text based destination for the renderers, our users have a desire to preserve the types of their templated vars. We also do a lot of internal intercept and post-processing to preserve those types but it's hit or miss and never obvious to the end user what will work and what won't. Therefore, I'm trying to extend jinja beyond what it's original usecase might have been.

This is a first pass and I hope to drive discussion with it and shape it into something the rest of the jinja community is happy with.

try:
native_tmpl.render()
failed = False
except UndefinedError:
Copy link
Member

Choose a reason for hiding this comment

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

Should use pytest.raises.

jinja2/utils.py Outdated
element.
'''
invals = [x for x in invals]
if isinstance(invals, list):
Copy link
Member

Choose a reason for hiding this comment

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

This will always be True because of the previous line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

jinja2/utils.py Outdated
invals = invals[0]
elif len(invals) > 1:
# cast to unicode and join
invals = u''.join([u'%s' % x for x in invals])
Copy link
Member

Choose a reason for hiding this comment

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

Implicit encoding seems dangerous, should use text_type(x, 'utf8') here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

jinja2/utils.py Outdated
invals = invals[0]
elif len(invals) > 1:
# cast to unicode and join
invals = u''.join([u'%s' % x for x in invals])
Copy link
Member

Choose a reason for hiding this comment

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

Why do you convert multiple nodes to a string? Wouldn't you want to return the list?

@davidism
Copy link
Member

davidism commented May 1, 2017

It took me a while to figure out what this was actually doing. Definitely needs some docs, but looks interesting.

The term "native" is a bit confusing, since I also associate it with discussion about what Python's str is in 2 vs 3.

How is Ansible using this? That is, when do users need access to the results of a render and don't have access to the original objects instead?

Does this need to be in Jinja, as opposed to a separate package providing a different Environment? Admittedly, there would be a little code duplication since the native checks happen in the middle of a couple methods. It just seems so radically different than what Jinja usually does.

@jctanner
Copy link
Contributor Author

jctanner commented May 1, 2017

Hey @davidism

I'm working on addressing your inline code comments at the moment.

In terms of how Ansible will be using this... It's going to be transparent to our users, except that when they have chained template operations inside a playbook, types will be preserved all the way through.

Here's a basic example:

- hosts: el6host
  connection: local
  gather_facts: False
  vars:
      adict:
          foo: "1"
  tasks:
    - set_fact:
        tempres: "{{ adict.foo|int + 3 }}"
    - debug: var=tempres
    - debug: msg="{{ tempres | type_debug }}"

The type for "tempres" is -always- unicode in Ansible right now. With this feature, it will be an integer.

jinja2/utils.py Outdated
elif len(invals) > 1:
# cast to unicode and join
try:
invals = u''.join([text_type(x, 'utf8') for x in invals])
Copy link
Member

@davidism davidism May 1, 2017

Choose a reason for hiding this comment

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

Never mind, I messed up the review for this one. Should use text_type, which is what runtime.to_string is. But don't specify an encoding, since that's only relevant if the object is bytes. Should remove the except Exception block.

jinja2/utils.py Outdated
invals = u''.join([text_type(x, 'utf8') for x in invals])
except Exception as e:
pass
return invals
Copy link
Member

Choose a reason for hiding this comment

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

Should this be return None? An empty list doesn't make much sense.

Copy link
Member

Choose a reason for hiding this comment

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

Also need a test case for this path.

jinja2/utils.py Outdated
native, the list is artificial and we should return just the first
element.
'''
invals = [x for x in invals]
Copy link
Member

Choose a reason for hiding this comment

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

I don't like building this list, seems wasteful of memory, although I guess there shouldn't be many nodes for the use case you're solving. Could change this to head = list(islice(invals, 2)) to test the length, then either return head[0] or build a list from invals in join.

self.native = native
if self.native:
self.code_generator_class = NativeCodeGenerator
#import pytest; pytest.set_trace()
Copy link
Member

Choose a reason for hiding this comment

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

🐒

@@ -335,6 +336,11 @@ def __init__(self,
self.enable_async = enable_async
self.is_async = self.enable_async and have_async_gen

self.native = native
if self.native:
self.code_generator_class = NativeCodeGenerator
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this. I'd rather add a native_code_generator_class class attribute and pick this one here. Otherwise there's no way of using a custom native code generator class without setting it after instantiating the environment (instead of setting it on the class level on a subclass)

Copy link
Member

Choose a reason for hiding this comment

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

Or just make this a completely separate NativeEnvironment, rather than an option on the base environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that mean you'd prefer a subclassed Environment?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that would be more clear.

@ThiefMaster
Copy link
Member

Not sure if this really its in the Jinja core. Any chance this would be possible fully on the Ansible side by using a custom Environment/CodeGenerator/Runtime (I made the last two overridable on the Environment level some time ago)

native_env = Environment(native=True)
native_tmpl = native_env.from_string("{% for x in listone %}{{ x }}{% endfor %}")
result = native_tmpl.render(listone=['a', 'b', 'c', 'd'])
assert isinstance(result, unicode)
Copy link
Member

Choose a reason for hiding this comment

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

text_type not unicode

@davidism
Copy link
Member

davidism commented May 1, 2017

Possible concat implementation:

def concat(nodes):
    head = list(islice(nodes, 2))

    if not head:
        return None

    if len(head) == 1:
        out = head[0]
    else:
        out = u''.join([text_type(v) for v in nodes])

    try:
        return literal_eval(out)
    except (ValueError, SyntaxError, MemoryError):  # possibly RecursionError
        return out

@jctanner
Copy link
Contributor Author

jctanner commented May 1, 2017

@davidism that concat example breaks this test ...

        native_env = Environment(native=True)
        native_tmpl = native_env.from_string("{% for x in listone %}{{ x }}{% endfor %}")
        result = native_tmpl.render(listone=['a', 'b', 'c', 'd'])
        assert isinstance(result, unicode)
        assert result == 'abcd'

The result is "cd", which I presume comes from having iterated over the first two values in the generator and not being able to seek backwards.

@davidism
Copy link
Member

davidism commented May 1, 2017

chain(head, nodes) fixes that.

@jctanner
Copy link
Contributor Author

jctanner commented May 2, 2017

@davidism @ThiefMaster I think I've addressed everything suggested up till now. Any further thoughts?

@davidism
Copy link
Member

davidism commented May 2, 2017

Needs documentation and changelog. Can be moved to a separate module, like the sandboxed env.

I still think this needs a clearer name than just "native". TypedOutputEnvironment? NativeTypeEnv? PythonTypeEnv? Something else? Or not?

@jctanner
Copy link
Contributor Author

jctanner commented May 2, 2017

@davidism Would you want just the new code in environment.py moved to a separate file, or should I also move the new code in utils.py as well?

@davidism
Copy link
Member

davidism commented May 2, 2017

All of it.

@jctanner
Copy link
Contributor Author

jctanner commented May 2, 2017

@davidism all code is in a separate "nativetypes.py" file now. I honestly don't know what makes sense in terms of naming the classes/files. I chose "native" because it espoused how I think about it, but I realize that's not how everyone thinks. If you or the rest of the team want to decide and pick a name, I'll do the renames.

For the docs, how much do you want in the docstrings versus in the "docs" directory? Is there a make script for the "docs" dir ... I didn't see anything obvious. How does the dev team build and examine docs prior to push?

@davidism
Copy link
Member

davidism commented May 2, 2017

Install sphinx, cd to docs, make html, open _build/html/index.html. Don't worry if the theme doesn't look the same. See docs/sandbox.rst as a possible template for your docs.

@jctanner
Copy link
Contributor Author

jctanner commented May 4, 2017

@davidism first pass on webdocs is done. I wasn't sure where to put it in the TOC, so I placed last in the list, beneath tips n tricks.

Here's a rendered example http://tannerjc.net/tmp/jinja/html/nativetypes.html

Native Python Types
===================

The Jinja2 :class:`NativeEnvironment` class can be used instead of :class:`Environment` to override jinja's default behavior of returning only strings. This can be useful if you are using jinja outside the context of creating text files.
Copy link
Member

Choose a reason for hiding this comment

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

"Jinja", without 2, capitalized.



def native_concat(nodes):
'''
Copy link
Member

Choose a reason for hiding this comment

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

Use double quotes, remove blank first line, for all docstrings.


class NativeCodeGenerator(CodeGenerator):
'''
A custom code generator, which avoids injecting to_string() calls around
Copy link
Member

Choose a reason for hiding this comment

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

``to_string()``


def visit_Output(self, node, frame):
'''
Slightly modified from the same method in CodeGenerator,
Copy link
Member

Choose a reason for hiding this comment

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

:class:CodeGenerator (linked appropriately)

def visit_Output(self, node, frame):
'''
Slightly modified from the same method in CodeGenerator,
so that to_string() is not inserted afer the yield keyword.
Copy link
Member

Choose a reason for hiding this comment

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

``to_string()``


def test_loops(self, env):

# FIXME - is this what we want?
Copy link
Member

Choose a reason for hiding this comment

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

Obsolete comment?


def test_loop_look_alike(self, env):

# FIXME - conflicts with test_loops
Copy link
Member

Choose a reason for hiding this comment

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

Obsolete comment?


:copyright: (c) 2017 by the Jinja Team.
:license: BSD, see LICENSE for more details.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Remove header and encoding.

"""
import pytest

from jinja2 import Markup, Environment
Copy link
Member

Choose a reason for hiding this comment

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

Clean up unused imports.



@pytest.mark.test_tests
class TestNativeTestsCase(object):
Copy link
Member

Choose a reason for hiding this comment

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

TestNativeEnvironment

@@ -12,6 +12,7 @@ Jinja2 Documentation
integration
switching
tricks
nativetypes
Copy link
Member

@davidism davidism May 9, 2017

Choose a reason for hiding this comment

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

This makes more sense below sandbox.

@davidism
Copy link
Member

davidism commented May 9, 2017

I'm still hesitant to add this to Jinja. It's such a standalone thing that it would make sense as a Jinja-NativeEnv package.

Our philosophy is to avoid adding more features unless they're absolutely necessary, since we have limited resources and want to keep the packages focused on one obvious use case. "Render to Python types" is not an obvious use case, given the features in the rest of the library.

Will you be willing to monitor this repository and keep this feature in sync with the rest of the code? Is there a plan within Ansible to provide maintenance for this?

@untitaker @ThiefMaster can you help make a decision about this?

@sivel
Copy link
Contributor

sivel commented May 9, 2017

Speaking from my POV, and not on behalf of @jctanner

I had initially voiced concern about this living outside of jinja, as the likelihood for a change to crop up in jinja, that impacts this code could be pretty high.

My initial recommendation was to get buy in from the authors of jinja, to keep this in mind, and perform external validation to ensure they were not breaking the code in Ansible to perform this, and potentially keeping us informed of such changes. Obviously, this route has some problems.

Including in jinja helps us ensure that they code is functional. However at some level I do imagine we would need to vendor this code as well, to support older versions of jinja, such as those an OS packaging system would provide.

We already have some of this problem currently. An example was the 2.9 release, where changes broke some things for our users, that we had to deal with it, because we aren't tightly integrated from a community perspective.

I'm not necessarily recommending a particular solution, just voicing some concerns.

@jctanner
Copy link
Contributor Author

jctanner commented May 9, 2017

Will you be willing to monitor this repository and keep this feature in sync with the rest of the code? Is there a plan within Ansible to provide maintenance for this?

We are going to have to "vendor" a copy of this code for older versions of jinja that our consumers might have, so yeah we have to keep ourselves in sync with upstream and do everything we can to keep the upstream functional.

My eventual goal is to see if the jinja dev team would consider allowing me to break up CodeGenerator.visitOutput into a few smaller functions. If so, the nativetypes code can be drastically smaller and maintenance will be much simpler. I did not want to muddy the waters and confuse that refactor with this feature, so I didn't bring it up yet.

Our philosophy is to avoid adding more features unless they're absolutely necessary, since we have limited resources and want to keep the packages focused on one obvious use case. "Render to Python types" is not an obvious use case, given the features in the rest of the library.

I fully understand that mentality. I'm still trying to build a list of useful examples of this feature that make sense outside ansible, but I also think that the community might have some input too.

@mitsuhiko
Copy link
Contributor

I think I'm generally in favour of adding that. In the past I used similar things. I will have a look at this.

@mitsuhiko mitsuhiko self-assigned this May 30, 2017
finalize = lambda x: text_type(
self.environment.finalize(self.environment, x))
else:
finalize = lambda x: text_type(self.environment.finalize(x))
Copy link
Member

Choose a reason for hiding this comment

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

Should this text_type be removed?

else:
finalize = lambda x: text_type(self.environment.finalize(x))
else:
finalize = text_type
Copy link
Member

Choose a reason for hiding this comment

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

Should this be replaced with a no-op?

close = 0
if frame.eval_ctx.volatile:
self.write('(escape if context.eval_ctx.autoescape'
' else to_string)(')
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed? Does autoescaping even make sense for native types?

close = 0
if frame.eval_ctx.volatile:
self.write('(escape if context.eval_ctx.autoescape else'
' to_string)(')
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed? Does autoescaping even make sense for native types?

getattr(func, 'evalcontextfunction', False):
allow_constant_finalize = False
elif getattr(func, 'environmentfunction', False):
finalize = lambda x: text_type(
Copy link
Member

Choose a reason for hiding this comment

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

Should this text_type be removed?

@jctanner
Copy link
Contributor Author

jctanner commented Jun 8, 2017

@davidism I removed all of the text_type refs and set the finalize noop per your suggestions. Tests still seem to pass for me.

@jctanner
Copy link
Contributor Author

anything else I can do to help out here?

@davidism
Copy link
Member

It's been a while (sorry!) and there was some confusion over what we were waiting on here. https://botbot.me/freenode/pocoo/2017-10-31/?msg=92959650&page=3 Our conclusion was that adding has_safe_repr there was appropriate and fixed the issue I came up with.

@davidism davidism merged commit d17c7db into pallets:master Oct 31, 2017
@jctanner
Copy link
Contributor Author

@davidism thank you so much! You've made many ansible devs happy today!

@davidism
Copy link
Member

davidism commented Nov 8, 2017

@jctanner I just released Jinja 2.10 with this included. I tried to ping you on Twitter but couldn't find your username. 😄

@jctanner
Copy link
Contributor Author

jctanner commented Nov 9, 2017

@davidism I do all my microblogging in github comments =)

New downstream patch for ansible ansible/ansible#32738

@arodier
Copy link

arodier commented Apr 2, 2018

Hello,
Thank for your hard work. In which version of Ansible this has been fixed?

@jctanner
Copy link
Contributor Author

jctanner commented Apr 2, 2018

@arodier we're going to merge ansible/ansible#32738 at the very beginning of the ansible 2.7 development cycle.

@arodier
Copy link

arodier commented Apr 2, 2018

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

6 participants