Move undefined check from concat to finalize (#78165)

* Move undefined check from concat to finalize

In the classic Jinja2's Environment str() is called on the return value of the
finalize method to potentially trigger the undefined error. That is not
the case in NativeEnvironment where string conversion of the return value is
not desired. We workaround that by checking for Undefined in all of our concat
functions. It seems simpler to do it earlier in the finalize method(s) instead.
As a side-effect it fixes an undefined variable detection in imported templates.

Fixes #78156

ci_complete

* Fix sanity

* ...

* sigh

changelogs/fragments/78156-undefined-check-in-finalize.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - Move undefined check from concat to finalize (https://github.com/ansible/ansible/issues/78156)

lib/ansible/template/__init__.py

@@ -609,6 +609,28 @@ def get_fqcr_and_name(resource, collection='ansible.builtin'):
     return fqcr, name
 
 
+def _fail_on_undefined(data):
+    """Recursively find an undefined value in a nested data structure
+    and properly raise the undefined exception.
+    """
+    if isinstance(data, Mapping):
+        for value in data.values():
+            _fail_on_undefined(value)
+    elif is_sequence(data):
+        for item in data:
+            _fail_on_undefined(item)
+    else:
+        if isinstance(data, StrictUndefined):
+            # To actually raise the undefined exception we need to
+            # access the undefined object otherwise the exception would
+            # be raised on the next access which might not be properly
+            # handled.
+            # See https://github.com/ansible/ansible/issues/52158
+            # and StrictUndefined implementation in upstream Jinja2.
+            str(data)
+    return data
+
+
 @_unroll_iterator
 def _ansible_finalize(thing):
     """A custom finalize function for jinja2, which prevents None from being
@@ -622,7 +644,7 @@ def _ansible_finalize(thing):
     which can produce a generator in the middle of a template are already
     wrapped with ``_unroll_generator`` in ``JinjaPluginIntercept``.
     """
-    return thing if thing is not None else ''
+    return thing if _fail_on_undefined(thing) is not None else ''
 
 
 class AnsibleEnvironment(NativeEnvironment):
@@ -651,7 +673,7 @@ class AnsibleNativeEnvironment(AnsibleEnvironment):
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
-        self.finalize = _unroll_iterator(lambda thing: thing)
+        self.finalize = _unroll_iterator(_fail_on_undefined)
 
 
 class Templar:

lib/ansible/template/native_helpers.py

@@ -10,10 +10,7 @@ import ast
 from itertools import islice, chain
 from types import GeneratorType
 
-from jinja2.runtime import StrictUndefined
-
 from ansible.module_utils._text import to_text
-from ansible.module_utils.common.collections import is_sequence, Mapping
 from ansible.module_utils.six import string_types
 from ansible.parsing.yaml.objects import AnsibleVaultEncryptedUnicode
 from ansible.utils.native_jinja import NativeJinjaText
@@ -34,29 +31,6 @@ class Json2Python(ast.NodeTransformer):
         return ast.Constant(value=_JSON_MAP[node.id])
 
 
-def _fail_on_undefined(data):
-    """Recursively find an undefined value in a nested data structure
-    and properly raise the undefined exception.
-    """
-    if isinstance(data, Mapping):
-        for value in data.values():
-            _fail_on_undefined(value)
-    elif is_sequence(data):
-        for item in data:
-            _fail_on_undefined(item)
-    else:
-        if isinstance(data, StrictUndefined):
-            # To actually raise the undefined exception we need to
-            # access the undefined object otherwise the exception would
-            # be raised on the next access which might not be properly
-            # handled.
-            # See https://github.com/ansible/ansible/issues/52158
-            # and StrictUndefined implementation in upstream Jinja2.
-            str(data)
-
-    return data
-
-
 def ansible_eval_concat(nodes):
     """Return a string of concatenated compiled nodes. Throw an undefined error
     if any of the nodes is undefined.
@@ -73,7 +47,7 @@ def ansible_eval_concat(nodes):
         return ''
 
     if len(head) == 1:
-        out = _fail_on_undefined(head[0])
+        out = head[0]
 
         if isinstance(out, NativeJinjaText):
             return out
@@ -82,7 +56,7 @@ def ansible_eval_concat(nodes):
     else:
         if isinstance(nodes, GeneratorType):
             nodes = chain(head, nodes)
-        out = ''.join([to_text(_fail_on_undefined(v)) for v in nodes])
+        out = ''.join([to_text(v) for v in nodes])
 
     # if this looks like a dictionary, list or bool, convert it to such
     if out.startswith(('{', '[')) or out in ('True', 'False'):
@@ -111,7 +85,7 @@ def ansible_concat(nodes):
 
     Used in Templar.template() when jinja2_native=False and convert_data=False.
     """
-    return ''.join([to_text(_fail_on_undefined(v)) for v in nodes])
+    return ''.join([to_text(v) for v in nodes])
 
 
 def ansible_native_concat(nodes):
@@ -129,7 +103,7 @@ def ansible_native_concat(nodes):
         return None
 
     if len(head) == 1:
-        out = _fail_on_undefined(head[0])
+        out = head[0]
 
         # TODO send unvaulted data to literal_eval?
         if isinstance(out, AnsibleVaultEncryptedUnicode):
@@ -151,7 +125,7 @@ def ansible_native_concat(nodes):
     else:
         if isinstance(nodes, GeneratorType):
             nodes = chain(head, nodes)
-        out = ''.join([to_text(_fail_on_undefined(v)) for v in nodes])
+        out = ''.join([to_text(v) for v in nodes])
 
     try:
         return ast.literal_eval(

test/integration/targets/template/runme.sh

@@ -42,3 +42,5 @@ ansible-playbook unsafe.yml -v "$@"
 ansible-playbook in_template_overrides.yml -v "$@"
 
 ansible-playbook lazy_eval.yml -i ../../inventory -v "$@"
+
+ansible-playbook undefined_in_import.yml -i ../../inventory -v "$@"

test/integration/targets/template/undefined_in_import-import.j2

@@ -0,0 +1 @@
+{{ undefined_variable }}

test/integration/targets/template/undefined_in_import.j2

@@ -0,0 +1 @@
+{% import 'undefined_in_import-import.j2' as t %}

test/integration/targets/template/undefined_in_import.yml

@@ -0,0 +1,11 @@
+- hosts: localhost
+  gather_facts: false
+  tasks:
+    - debug:
+        msg: "{{ lookup('template', 'undefined_in_import.j2') }}"
+      ignore_errors: true
+      register: res
+
+    - assert:
+        that:
+          - "\"'undefined_variable' is undefined\" in res.msg"