Repository: ansible/ansible
Pull request: fix combine filter using undefined vars
##### SUMMARY
Fixes #55810

The combine filter ignores undefined variables leading to unexpected errors. Fail instead, with a helpful message.

##### ISSUE TYPE
- Bugfix Pull Request

##### COMPONENT NAME
lib/ansible/plugins/filter/core.py


Problem statement:
Combine filter does not produce dict(but text) if undefined variable is used
##### SUMMARY
When undefined variable is used inside combine filter and result output is not dict but plain text.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
combine() filter

##### ANSIBLE VERSION

```
ansible-playbook 2.7.9
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/var/lib/awx/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible-playbook
  python version = 2.7.5 (default, Oct 30 2018, 23:45:53) [GCC 4.8.5 20150623 (Red Hat 4.8.5-36)]
```

##### CONFIGURATION
```
```

##### OS / ENVIRONMENT
more test including AWX


##### STEPS TO REPRODUCE
```
---
- name: combine test
  hosts: localhost
  gather_facts: false
  vars:
    #non_ex: ok
    example:
      some: thing
      users:
        - user1
        - user2

  tasks:
    - name: "dump before"
      debug: var=example
    - debug: var=example.some
    - set_fact:
        example: "{{example | combine({'varb': non_ex})}}"
    - name: "dump after"
      debug:
        var: example
    - debug: var=example.some
```


##### EXPECTED RESULTS
Two options here:
1) "example.some": "thing"
2) FAIL: non_ex is undefined


##### ACTUAL RESULTS
```
TASK [debug] ****************************************************************************
task path: combine/test.yaml:22
Friday 26 April 2019  13:25:44 +0200 (0:00:00.240)       0:00:01.421 **********
ok: [localhost] => {
    "example.some": "VARIABLE IS NOT DEFINED!: 'unicode object' has no attribute 'some'"
}
META: ran handlers
```

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/plugins/filter](https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/filter)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->

Commit message: Add tests for the combine filter

Patch to review:
diff --git a/lib/ansible/plugins/filter/core.py b/lib/ansible/plugins/filter/core.py
index a739e2bd5fa41b..b10841f0881528 100644
--- a/lib/ansible/plugins/filter/core.py
+++ b/lib/ansible/plugins/filter/core.py
@@ -292,6 +292,20 @@ def mandatory(a):
     return a
 
 
+def recursive_check_defined(item):
+    from jinja2.runtime import Undefined
+
+    if isinstance(item, MutableMapping):
+        for key in item:
+            recursive_check_defined(item[key])
+    elif isinstance(item, list):
+        for i in item:
+            recursive_check_defined(i)
+    else:
+        if isinstance(item, Undefined):
+            raise AnsibleFilterError("{0} is undefined".format(item))
+
+
 def combine(*terms, **kwargs):
     recursive = kwargs.get('recursive', False)
     if len(kwargs) > 1 or (len(kwargs) == 1 and 'recursive' not in kwargs):
@@ -300,8 +314,10 @@ def combine(*terms, **kwargs):
     dicts = []
     for t in terms:
         if isinstance(t, MutableMapping):
+            recursive_check_defined(t)
             dicts.append(t)
         elif isinstance(t, list):
+            recursive_check_defined(t)
             dicts.append(combine(*t, **kwargs))
         else:
             raise AnsibleFilterError("|combine expects dictionaries, got " + repr(t))
diff --git a/test/integration/targets/filters/runme.sh b/test/integration/targets/filters/runme.sh
index d2a38f5900470e..0ca7bb8c2b0c96 100755
--- a/test/integration/targets/filters/runme.sh
+++ b/test/integration/targets/filters/runme.sh
@@ -7,6 +7,6 @@ source virtualenv.sh
 # Requirements have to be installed prior to running ansible-playbook
 # because plugins and requirements are loaded before the task runs
 
-pip install jmespath netaddr
+pip install jmespath netaddr jinja2 cryptography pyyaml
 
 ANSIBLE_ROLES_PATH=../ ansible-playbook filters.yml -i ../../inventory -e @../../integration_config.yml "$@"
diff --git a/test/integration/targets/filters/tasks/main.yml b/test/integration/targets/filters/tasks/main.yml
index 2e17daaef73b63..5f9593257817b4 100644
--- a/test/integration/targets/filters/tasks/main.yml
+++ b/test/integration/targets/filters/tasks/main.yml
@@ -277,3 +277,28 @@
   loop: "{{ hostvars|dict2items }}"
   loop_control:
     label: "{{ item.key }}"
+
+- name: Ensure combining two dictionaries containing undefined variables provides a helpful error
+  block:
+    - set_fact:
+        foo:
+          key1: value1
+
+    - set_fact:
+        combined: "{{ foo | combine({'key2': undef_variable}) }}"
+      ignore_errors: yes
+      register: result
+
+    - assert:
+        that:
+          - "result.msg.startswith('The task includes an option with an undefined variable')"
+
+    - set_fact:
+        key2: is_defined
+
+    - set_fact:
+        combined: "{{ foo | combine({'key2': key2}) }}"
+
+    - assert:
+        that:
+          - "combined.key2 == 'is_defined'"


Code review comments:
diff_hunk: @@ -292,6 +292,20 @@ def mandatory(a):
     return a
 
 
+def recursive_check_defined(item):
path: lib/ansible/plugins/filter/core.py
text: @user1:
this might be better housed in `template/__init__.py` or` template.py` rather than `filters/core.py`

@author:
Moved to `template/__init__.py`

diff_hunk: @@ -7,6 +7,6 @@ source virtualenv.sh
 # Requirements have to be installed prior to running ansible-playbook
 # because plugins and requirements are loaded before the task runs
 
-pip install jmespath netaddr
+pip install jmespath netaddr jinja2 cryptography pyyaml
path: test/integration/targets/filters/runme.sh
text: @user1:
wouldn't Ansible install itself take care of those dependencies?

@author:
Removed those. I have something weird going on in my normal setup of pyenv + virtualenv that breaks with all the recent virtualenv.sh test changes. :-(