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

Custom jinja Undefined class for handling nested undefined attributes #51768

Merged
merged 1 commit into from Feb 12, 2019

Conversation

agaffney
Copy link
Contributor

@agaffney agaffney commented Feb 5, 2019

SUMMARY

This commit creates a custom Jinja2 Undefined class that returns Undefined for any further accesses, rather than raising an exception. This allows doing something like foo.bar.baz | default('whatever') rather than ((foo | default({})).bar | default({})).baz | default('whatever') when you don't know if all levels of the structure are defined.

This was created as an alternative to #50706

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

templates

ADDITIONAL INFORMATION

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.8 This issue/PR affects Ansible v2.8 feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Feb 5, 2019
@agaffney agaffney force-pushed the nested_undefined branch 3 times, most recently from f1136ce to 91729c2 Compare February 5, 2019 21:27
@nitzmahone
Copy link
Member

nitzmahone commented Feb 5, 2019

This is technically probably a breaking change, since we're reimplementing . as a null-conditional operator (eg C#'s ?.)... That said, I'm having a really hard time coming up with a scenario that wouldn't have just blown up catastrophically anyway, since you can't try/catch in an Ansible template. Maybe something in a "real" template? Dunno...

I'm a big +1 for the change in behavior, so long as nobody can come up with a scenario where the current behavior actually makes sense (in which case we'd probably have to talk some combination of switch and deprecation warning).

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Feb 5, 2019
@jwitko
Copy link
Contributor

jwitko commented Feb 5, 2019

Huge fan of this behavior change. Like @nitzmahone , I can think of no situation where this would break that isn't already breaking.

@agaffney agaffney changed the title [WIP] Custom jinja Undefined class Custom jinja Undefined class Feb 5, 2019
@agaffney agaffney changed the title Custom jinja Undefined class Custom jinja Undefined class for handling nested undefined attributes Feb 5, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Feb 5, 2019
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Feb 6, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 6, 2019
@jhawkesworth
Copy link
Contributor

jhawkesworth commented Feb 6, 2019

I'd like this too. This is something I've wanted to do in when: conditions and also when using ternary filter in the past. I've added to my list to test.

Copy link
Contributor

@dagwieers dagwieers left a comment

Choose a reason for hiding this comment

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

Tested and works fine. My only concern is that people have been doing ugly tricks to circumvent the original behavior that may be bitten by this. (ignore_errors?) But honestly, they deserve breakage :-) This is for the better.

@tonk
Copy link
Contributor

tonk commented Feb 8, 2019

I agree with @dagwieers and I'm a +1 for this. This makes life a lot easier.

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed core_review In order to be merged, this PR must follow the core review workflow. labels Feb 8, 2019
@felixfontein
Copy link
Contributor

I guess it should mentioned in the porting guide and changelog. Besides that, +1 from me, too!

Copy link
Contributor

@gundalow gundalow left a comment

Choose a reason for hiding this comment

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

@agaffney Seems like people are happy, though could you please add a changelog/fragment and an entry in the porting guide?

Once you've done that please poke me on IRC and I'll merge ASAP (as porting guides can end up in merge conflicts)

Adding requires changes as currently this has sh1pit

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Feb 11, 2019
@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Feb 11, 2019
@ansibot
Copy link
Contributor

ansibot commented Feb 11, 2019

@ansibot ansibot added the docs This issue/PR relates to or includes documentation. label Feb 11, 2019
@agaffney
Copy link
Contributor Author

Added changelog fragment and section in the porting guide

Copy link
Contributor

@acozine acozine left a comment

Choose a reason for hiding this comment

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

This is a nice addition - I've made some suggestions to make the documentation even better. Thanks @agaffney for the PR.

docs/docsite/rst/porting_guides/porting_guide_2.8.rst Outdated Show resolved Hide resolved
docs/docsite/rst/porting_guides/porting_guide_2.8.rst Outdated Show resolved Hide resolved
docs/docsite/rst/porting_guides/porting_guide_2.8.rst Outdated Show resolved Hide resolved
@agaffney agaffney force-pushed the nested_undefined branch 2 times, most recently from 66b6c7f to 62b0ec0 Compare February 12, 2019 13:33
This commit creates a custom Jinja2 Undefined class that returns
Undefined for any further accesses, rather than raising an exception
Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

Looks good to me, and adding a toggle if this breaks things for people is a good back-up plan. Here's an example playbook that does change behavior:

---
- hosts: localhost
  connection: local
  gather_facts: no
  tasks:
  - name: test
    command: echo "{{ dict_var.bar.baz | default('test') }}"
    ignore_errors: yes

ignore_errors also prevents templating errors - e.g.

    ignore_errors: yes
    when: dict_var.bar.baz | default('test') is defined

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 12, 2019
@samdoran samdoran merged commit 9c35f18 into ansible:devel Feb 12, 2019
@jamescassell
Copy link
Contributor

Happy to see this change merged. Could we extend this further?

I'd like to do:

my_network: '{{ (my_project ~ "-netprivate") | default("netpublic") }}'

Currently, I have to:

my_network: '{{ (my_project is defined) | ternary(my_project ~ "-netprivate", "netpublic") }}'

Would it be possible to override the concatenation operator in the same way?

@agaffney
Copy link
Contributor Author

Doing something similar for the concatenation operator would be much more involved, and probably not worth the effort. You can use the python-style ternary syntax for that, instead.

my_network: '{{ (my_project ~ "-netprivate") if my_project is defined else "netpublic" }}'

@Kassec
Copy link

Kassec commented Mar 26, 2019

Hi,

Thanks for this PR. I backported it to 2.7 and it saved my life ;)

One thing I'm wondering about: is it normal this new code doesn't handle accessing data with bracket notation ?

- hosts: all

  tasks:
  - name: Check that nested undefined values return Undefined
    vars:
      dict_var:
        bar: {}
      list_var:
      - foo: {}

    assert:
      that:
      - dict_var is defined
      - dict_var.bar is defined
      - dict_var.bar.baz is not defined
      - dict_var.bar.baz | default('DEFAULT') == 'DEFAULT'
      - dict_var.bar.baz.abc is not defined
      - dict_var.bar.baz.abc | default('DEFAULT') == 'DEFAULT'
      - dict_var.bar.baz['abc'] | default('DEFAULT') == 'DEFAULT'
      - dict_var.baz is not defined
      - dict_var.baz.abc is not defined
      - dict_var.baz.abc | default('DEFAULT') == 'DEFAULT'
      - list_var.0 is defined
      - list_var.1 is not defined
      - list_var.0.foo is defined
      - list_var.0.foo.bar is not defined
      - list_var.0.foo.bar | default('DEFAULT') == 'DEFAULT'
      - list_var.1.foo is not defined
      - list_var.1.foo | default('DEFAULT') == 'DEFAULT'

FAILED! => {"msg": "The conditional check 'dict_var.bar.baz['abc'] | default('DEFAULT') == 'DEFAULT'' failed. The error was: error while evaluating conditional (dict_var.bar.baz['abc'] | default('DEFAULT') == 'DEFAULT'): 'dict object' has no attribute 'baz'"}

@agaffney agaffney deleted the nested_undefined branch March 26, 2019 18:25
@agaffney
Copy link
Contributor Author

All this PR does is replace the class that Jinja returns when it detects an undefined value. It does not touch/change the logic for determining what is undefined or how that is handled.

@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. feature This issue/PR relates to a feature request. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet