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

groupby filter returns _GroupTuple instead of tuple #654

Closed
major opened this issue Jan 10, 2017 · 9 comments

Comments

@major
Copy link
Contributor

commented Jan 10, 2017

Expected Behavior

In jinja < 2.9, the groupby filter returns its data as a tuple.

Actual Behavior

In jinja >= 2.9, the groupby filter returns its data as a namedtuple.

Template Code

Here's what I'm using along with Ansible right now:

---

- hosts: all
  vars:
    fruits:
      - name: apple
        enjoy: yes
      - name: orange
        enjoy: no
      - name: strawberry
        enjoy: yes
  tasks:

    - name: Loop through my fruits
      debug:
        msg: |
          {% set fruit_list = [] %}
          {% for fruit_dict in item[1] %}
          {{ fruit_dict.name }}
          {% endfor %}
      with_items:
        - "{{ fruits | groupby('enjoy') }}"

In jinja 2.8, the result of the groupby() is a tuple and Ansible is able to parse it with python's AST module without an issue. In jinja 2.9 and later, the groupby() returns a namedtuple and this cannot be parsed with AST. This causes an exception in Ansible.

This may not be considered a bug within jinja, but this change is fairly disruptive for downstream users.

Ansible bug: ansible/ansible#20098

Your Environment

  • Python version: 2.7.12
  • Jinja version: 2.9.4
@ThiefMaster

This comment has been minimized.

Copy link
Member

commented Jan 10, 2017

This looks more like a bug or a bad type check in Ansible. A namedtuple is actually a subclass of tuple so code that can handle a tuple is supposed to handle a namedtuple just fine.

@ThiefMaster

This comment has been minimized.

Copy link
Member

commented Jan 10, 2017

hrm, looks like they actually check for a tuple ast node... https://github.com/ansible/ansible/blob/devel/lib/ansible/template/safe_eval.py#L73

openstack-gerrit pushed a commit to openstack/openstack-ansible-security that referenced this issue Jan 11, 2017

Remove groupby filter to avoid bug
This patch adjusts the package installation/removal tasks to get around
a bug. Jinja2 now returns namedtuples with the groupby filter and
Ansible is unable to convert that to a usable variable data type with
AST.

Jinja2 bug: pallets/jinja#654
Ansible bug: ansible/ansible#20098
Related-Bug: 1655397
Change-Id: I3ce764bfb643bda58c4b6ad282e71c312f41465e
@abadger

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

Yeah -- I'm not sure that this is a "bug" in jinja but the object of ansible's safe_eval() is to turn strings into python data but only if it is safe. Since Ansible is using jinja2 for "variables" the output of jinja is being passed through safe_eval() in order to turn the strings returned by jinja back into typed data (ints, floats, dicts, lists, etc).

Tuples have a literal representation which can be eval()'ed safely. NamedTuple does not. So safe_eval() is able to handle jinja2's old groupby output which returned a string representation of a tuple. It cannot handle the new groupby output which returns a representation of a NamedTuple.

My reading of the jinja2 commit message is that the NamedTuple is an implementation to ease the implementation of sorting the output: 6d356f1 So it should be possible to convert the NamedTuples into regular tuples post-sort if jinja2 would care to keep compatibility here.

@mitsuhiko

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

For what it's worth I ran into the same issue in Jinja2 on the way to fixing the contant folding. I explicitly had to disable tuple subclasses because of this change. I would be willing to back this out and convert it back to whatever object it was before. I can imagine this causing troubles.

@mitsuhiko

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

Actually looking at it closer it used to be a plain tuple so reverting to that would definitely remove a valuable feature. I don't think we can back this out now.

@mitsuhiko

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

One option would be to put it behind a policy but even then it requires a change in ansible to the Jinja config.

@mitsuhiko

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

Nevermind, we can back this out. It was a tuple subclass already just not a named tuple one. The issue is effectively that this changes repr and a few more things. We could back this out but I would like to have a dialog with ansible people about what behavior one would expect from changes in Jinja in general for this sort of thing.

@davidism

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

If there's an advantage to using namedtuple over a basic tuple subclass, and the repr is what causes the issue, we could just fix the repr.

class _GroupTuple(namedtuple('_GTBase', 'key value')):
    __slots__ = ()

    def __repr__(self):
        return super(self.__class__, self).__repr__()

    __str__ = __repr__

openstack-gerrit added a commit to openstack/openstack that referenced this issue Jan 12, 2017

Updated openstack/openstack
Project: openstack/requirements  f0fb896f4416fe100b18826444b6583a42fab46e

Avoid Jinja2 versions 2.9.[0-4]

There is a bug within ansible and/or jinja2 that causes an exception
when a string contains curly braces. A bug has already been opened
with the Ansible team to review the bug.

This patch pins the jinja2 release at a version less than 2.9. The
issue first appeared in the 2.9 release and is still present in
2.9.4 (latest available).

Jinja2 bug: pallets/jinja#654
Ansible bug: ansible/ansible#20098
Related-Bug: 1655397
Change-Id: I82561d3d738147683499697883d9d03ceb897f3d

openstack-gerrit pushed a commit to openstack/requirements that referenced this issue Jan 12, 2017

Avoid Jinja2 versions 2.9.[0-4]
There is a bug within ansible and/or jinja2 that causes an exception
when a string contains curly braces. A bug has already been opened
with the Ansible team to review the bug.

This patch pins the jinja2 release at a version less than 2.9. The
issue first appeared in the 2.9 release and is still present in
2.9.4 (latest available).

Jinja2 bug: pallets/jinja#654
Ansible bug: ansible/ansible#20098
Related-Bug: 1655397
Change-Id: I82561d3d738147683499697883d9d03ceb897f3d

openstack-gerrit added a commit to openstack/openstack that referenced this issue Jan 12, 2017

Updated openstack/openstack
Project: openstack/requirements  f0fb896f4416fe100b18826444b6583a42fab46e

Avoid Jinja2 versions 2.9.[0-4]

There is a bug within ansible and/or jinja2 that causes an exception
when a string contains curly braces. A bug has already been opened
with the Ansible team to review the bug.

This patch pins the jinja2 release at a version less than 2.9. The
issue first appeared in the 2.9 release and is still present in
2.9.4 (latest available).

Jinja2 bug: pallets/jinja#654
Ansible bug: ansible/ansible#20098
Related-Bug: 1655397
Change-Id: I82561d3d738147683499697883d9d03ceb897f3d

@mitsuhiko mitsuhiko closed this in 7854851 Jan 12, 2017

@major

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2017

Thanks for all the effort on this one, folks. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.