From 76b3eda05013799b478370f5f0f2035c03b34811 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Tue, 6 Sep 2022 13:58:33 +0200 Subject: [PATCH 01/17] Simplify AnsibleJ2Vars by using ChainMap for vars ci_complete --- lib/ansible/template/vars.py | 133 ++++++++++------------------------- 1 file changed, 39 insertions(+), 94 deletions(-) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index fd1b812458f066..450c5b64adc748 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -1,24 +1,7 @@ # (c) 2012, Michael DeHaan -# -# This file is part of Ansible -# -# Ansible is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# Ansible is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Ansible. If not, see . - -# Make coding more python3-ish -from __future__ import (absolute_import, division, print_function) -__metaclass__ = type +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) +from collections import ChainMap from collections.abc import Mapping from jinja2.utils import missing @@ -30,99 +13,61 @@ __all__ = ['AnsibleJ2Vars'] -class AnsibleJ2Vars(Mapping): - ''' - Helper class to template all variable content before jinja2 sees it. This is - done by hijacking the variable storage that jinja2 uses, and overriding __contains__ - and __getitem__ to look like a dict. Added bonus is avoiding duplicating the large - hashes that inject tends to be. +def _process_locals(_l): + if _l is None: + return {} + # FIXME is this needed? why? + return {k:v for k, v in _l.items() if v is not missing and k not in ('context', 'environment', 'template')} - To facilitate using builtin jinja2 things like range, globals are also handled here. - ''' - def __init__(self, templar, globals, locals=None): - ''' - Initializes this object with a valid Templar() object, as - well as several dictionaries of variables representing - different scopes (in jinja2 terminology). - ''' +class AnsibleJ2Vars(Mapping): + """Helper variable storage class that allows for nested variables templating: `foo: "{{ bar }}"`.""" + def __init__(self, templar, globals, locals=None): self._templar = templar - self._globals = globals - self._locals = dict() - if isinstance(locals, dict): - for key, val in locals.items(): - if val is not missing: - if key[:2] == 'l_': - self._locals[key[2:]] = val - elif key not in ('context', 'environment', 'template'): - self._locals[key] = val - - def __contains__(self, k): - if k in self._locals: - return True - if k in self._templar.available_variables: - return True - if k in self._globals: - return True - return False + self._variables = ChainMap(_process_locals(locals), self._templar.available_variables, globals) def __iter__(self): - keys = set() - keys.update(self._templar.available_variables, self._locals, self._globals) - return iter(keys) + return iter(self._variables) def __len__(self): - keys = set() - keys.update(self._templar.available_variables, self._locals, self._globals) - return len(keys) + return len(self._variables) def __getitem__(self, varname): - if varname in self._locals: - return self._locals[varname] - if varname in self._templar.available_variables: - variable = self._templar.available_variables[varname] - elif varname in self._globals: - return self._globals[varname] - else: - raise KeyError("undefined variable: %s" % varname) - - # HostVars is special, return it as-is, as is the special variable - # 'vars', which contains the vars structure + variable = self._variables[varname] + from ansible.vars.hostvars import HostVars - if isinstance(variable, dict) and varname == "vars" or isinstance(variable, HostVars) or hasattr(variable, '__UNSAFE__'): + if (isinstance(variable, dict) and varname == "vars") or isinstance(variable, HostVars) or hasattr(variable, '__UNSAFE__'): return variable - else: - value = None - try: - value = self._templar.template(variable) - except AnsibleUndefinedVariable as e: - # Instead of failing here prematurely, return an Undefined - # object which fails only after its first usage allowing us to - # do lazy evaluation and passing it into filters/tests that - # operate on such objects. - return self._templar.environment.undefined( - hint=f"{variable}: {e.message}", - name=varname, - exc=AnsibleUndefinedVariable, - ) - except Exception as e: - msg = getattr(e, 'message', None) or to_native(e) - raise AnsibleError("An unhandled exception occurred while templating '%s'. " - "Error was a %s, original message: %s" % (to_native(variable), type(e), msg)) - - return value + + try: + return self._templar.template(variable) + except AnsibleUndefinedVariable as e: + # Instead of failing here prematurely, return an Undefined + # object which fails only after its first usage allowing us to + # do lazy evaluation and passing it into filters/tests that + # operate on such objects. + return self._templar.environment.undefined( + hint=f"{variable}: {e.message}", + name=varname, + exc=AnsibleUndefinedVariable, + ) + except Exception as e: + msg = getattr(e, 'message', None) or to_native(e) + raise AnsibleError( + "An unhandled exception occurred while templating'{to_native(variable)}'. " + "Error was a {type(e)}, original message: {msg}" + ) def add_locals(self, locals): - ''' - If locals are provided, create a copy of self containing those + """If locals are provided, create a copy of self containing those locals in addition to what is already in this variable proxy. - ''' + """ if locals is None: return self # prior to version 2.9, locals contained all of the vars and not just the current # local vars so this was not necessary for locals to propagate down to nested includes - new_locals = self._locals | locals + new_locals = self._variables.maps[0] | locals - return AnsibleJ2Vars(self._templar, self._globals, locals=new_locals) + return AnsibleJ2Vars(self._templar, self._variables.maps[2], locals=new_locals) From 5935d5d743fab40321e9b7ceae723d44f218edc7 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Tue, 6 Sep 2022 14:58:22 +0200 Subject: [PATCH 02/17] no locals processing ci_complete --- lib/ansible/template/vars.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index 450c5b64adc748..67dc483304b757 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -17,7 +17,7 @@ def _process_locals(_l): if _l is None: return {} # FIXME is this needed? why? - return {k:v for k, v in _l.items() if v is not missing and k not in ('context', 'environment', 'template')} + return {k: v for k, v in _l.items() if v is not missing and k not in ('context', 'environment', 'template')} class AnsibleJ2Vars(Mapping): @@ -25,7 +25,8 @@ class AnsibleJ2Vars(Mapping): def __init__(self, templar, globals, locals=None): self._templar = templar - self._variables = ChainMap(_process_locals(locals), self._templar.available_variables, globals) + self._variables = ChainMap(locals, self._templar.available_variables, globals) + # self._variables = ChainMap(_process_locals(locals), self._templar.available_variables, globals) def __iter__(self): return iter(self._variables) From 6cd165d5b74f02e65db5f65d3ee34c71c22e3ae0 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Tue, 6 Sep 2022 15:04:21 +0200 Subject: [PATCH 03/17] ... --- lib/ansible/template/vars.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index 67dc483304b757..4ea9eebe891ec3 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -25,7 +25,7 @@ class AnsibleJ2Vars(Mapping): def __init__(self, templar, globals, locals=None): self._templar = templar - self._variables = ChainMap(locals, self._templar.available_variables, globals) + self._variables = ChainMap(locals or {}, self._templar.available_variables, globals) # self._variables = ChainMap(_process_locals(locals), self._templar.available_variables, globals) def __iter__(self): From de4cbded3732943c5a9d0006a7cce094d03ad277 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Tue, 6 Sep 2022 15:09:47 +0200 Subject: [PATCH 04/17] f strings --- lib/ansible/template/vars.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index 4ea9eebe891ec3..9741524f054248 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -56,8 +56,8 @@ def __getitem__(self, varname): except Exception as e: msg = getattr(e, 'message', None) or to_native(e) raise AnsibleError( - "An unhandled exception occurred while templating'{to_native(variable)}'. " - "Error was a {type(e)}, original message: {msg}" + f"An unhandled exception occurred while templating'{to_native(variable)}'. " + f"Error was a {type(e)}, original message: {msg}" ) def add_locals(self, locals): From 7d1ede20a9d291b0e28d535b103510fb3ef429e2 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Wed, 7 Sep 2022 09:08:05 +0200 Subject: [PATCH 05/17] comment on precedence --- lib/ansible/template/vars.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index 9741524f054248..ceb02fc91e4c45 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -25,6 +25,7 @@ class AnsibleJ2Vars(Mapping): def __init__(self, templar, globals, locals=None): self._templar = templar + # ChainMap lookups search from the first mapping to the last so locals have the highest precedence self._variables = ChainMap(locals or {}, self._templar.available_variables, globals) # self._variables = ChainMap(_process_locals(locals), self._templar.available_variables, globals) From fa7abc020b2cba3d87ee0eba1cb8b4a4e5244f9e Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Wed, 7 Sep 2022 09:09:22 +0200 Subject: [PATCH 06/17] explaining vars --- lib/ansible/template/vars.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index ceb02fc91e4c45..b8b4878e85e0d0 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -68,8 +68,11 @@ def add_locals(self, locals): if locals is None: return self + current_locals = self._variables.maps[0] + current_globals = self._variables.maps[2] + # prior to version 2.9, locals contained all of the vars and not just the current # local vars so this was not necessary for locals to propagate down to nested includes - new_locals = self._variables.maps[0] | locals + new_locals = current_locals | locals - return AnsibleJ2Vars(self._templar, self._variables.maps[2], locals=new_locals) + return AnsibleJ2Vars(self._templar, current_globals, locals=new_locals) From b8de82334cb28163ce238cd80a8b8ee572554ff5 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Wed, 7 Sep 2022 10:32:34 +0200 Subject: [PATCH 07/17] comments --- lib/ansible/template/vars.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index b8b4878e85e0d0..5f63e2d624f311 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -16,8 +16,11 @@ def _process_locals(_l): if _l is None: return {} - # FIXME is this needed? why? - return {k: v for k, v in _l.items() if v is not missing and k not in ('context', 'environment', 'template')} + return { + k: v for k, v in _l.items() + if v is not missing + and k not in ('context', 'environment', 'template') # NOTE is this really needed? + } class AnsibleJ2Vars(Mapping): @@ -25,9 +28,11 @@ class AnsibleJ2Vars(Mapping): def __init__(self, templar, globals, locals=None): self._templar = templar - # ChainMap lookups search from the first mapping to the last so locals have the highest precedence - self._variables = ChainMap(locals or {}, self._templar.available_variables, globals) - # self._variables = ChainMap(_process_locals(locals), self._templar.available_variables, globals) + self._variables = ChainMap( + _process_locals(locals), # first mapping has the highest precedence + self._templar.available_variables, + globals, + ) def __iter__(self): return iter(self._variables) From 283af73d83ed0550e893af30b911ee1f6ddd6650 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Wed, 7 Sep 2022 10:34:31 +0200 Subject: [PATCH 08/17] ci_complete From 18336e4c5294cc2b60a317844d80d1e81a805b65 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Wed, 7 Sep 2022 11:16:45 +0200 Subject: [PATCH 09/17] fix units ci_complete --- test/units/template/test_vars.py | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/test/units/template/test_vars.py b/test/units/template/test_vars.py index 514104f23bf5a3..f43cfac462da69 100644 --- a/test/units/template/test_vars.py +++ b/test/units/template/test_vars.py @@ -19,23 +19,16 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type -from units.compat import unittest -from unittest.mock import MagicMock - +from ansible.template import Templar from ansible.template.vars import AnsibleJ2Vars -class TestVars(unittest.TestCase): - def setUp(self): - self.mock_templar = MagicMock(name='mock_templar') +def test_globals_empty(): + assert isinstance(dict(AnsibleJ2Vars(Templar(None), {})), dict) - def test_globals_empty(self): - ajvars = AnsibleJ2Vars(self.mock_templar, {}) - res = dict(ajvars) - self.assertIsInstance(res, dict) - def test_globals(self): - res = dict(AnsibleJ2Vars(self.mock_templar, {'foo': 'bar', 'blip': [1, 2, 3]})) - self.assertIsInstance(res, dict) - self.assertIn('foo', res) - self.assertEqual(res['foo'], 'bar') +def test_globals(): + res = dict(AnsibleJ2Vars(Templar(None), {'foo': 'bar', 'blip': [1, 2, 3]})) + assert isinstance(res, dict) + assert 'foo' in res + assert res['foo'] == 'bar' From a59e4b116c779913b50088cfd60d3a1238b6defb Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Wed, 7 Sep 2022 12:14:16 +0200 Subject: [PATCH 10/17] Add changelog --- changelogs/fragments/ansiblej2vars-chainmap.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/fragments/ansiblej2vars-chainmap.yml diff --git a/changelogs/fragments/ansiblej2vars-chainmap.yml b/changelogs/fragments/ansiblej2vars-chainmap.yml new file mode 100644 index 00000000000000..205cdac8aab47a --- /dev/null +++ b/changelogs/fragments/ansiblej2vars-chainmap.yml @@ -0,0 +1,2 @@ +minor_changes: + - ``AnsibleJ2Vars`` class that acts as a storage for all variables for templating purposes now uses ``collections.ChainMap`` internally. From 4db5d62f0e0f454ae3b7b5e183c74ad0ceb0f94f Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Wed, 7 Sep 2022 12:23:31 +0200 Subject: [PATCH 11/17] Fix changelog --- changelogs/fragments/ansiblej2vars-chainmap.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/fragments/ansiblej2vars-chainmap.yml b/changelogs/fragments/ansiblej2vars-chainmap.yml index 205cdac8aab47a..04175e332c2b67 100644 --- a/changelogs/fragments/ansiblej2vars-chainmap.yml +++ b/changelogs/fragments/ansiblej2vars-chainmap.yml @@ -1,2 +1,2 @@ minor_changes: - - ``AnsibleJ2Vars`` class that acts as a storage for all variables for templating purposes now uses ``collections.ChainMap`` internally. + - "``AnsibleJ2Vars`` class that acts as a storage for all variables for templating purposes now uses ``collections.ChainMap`` internally." From d23fe89c048076fb5e198af7ac88372b0ad12d09 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Mon, 12 Sep 2022 12:17:28 +0200 Subject: [PATCH 12/17] Subclass ChainMap instead --- lib/ansible/template/vars.py | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index 5f63e2d624f311..3d5c1ebd4fb861 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -23,25 +23,19 @@ def _process_locals(_l): } -class AnsibleJ2Vars(Mapping): +class AnsibleJ2Vars(ChainMap): """Helper variable storage class that allows for nested variables templating: `foo: "{{ bar }}"`.""" def __init__(self, templar, globals, locals=None): self._templar = templar - self._variables = ChainMap( + super().__init__( _process_locals(locals), # first mapping has the highest precedence self._templar.available_variables, globals, ) - def __iter__(self): - return iter(self._variables) - - def __len__(self): - return len(self._variables) - def __getitem__(self, varname): - variable = self._variables[varname] + variable = super().__getitem__(varname) from ansible.vars.hostvars import HostVars if (isinstance(variable, dict) and varname == "vars") or isinstance(variable, HostVars) or hasattr(variable, '__UNSAFE__'): @@ -73,8 +67,8 @@ def add_locals(self, locals): if locals is None: return self - current_locals = self._variables.maps[0] - current_globals = self._variables.maps[2] + current_locals = self.maps[0] + current_globals = self.maps[2] # prior to version 2.9, locals contained all of the vars and not just the current # local vars so this was not necessary for locals to propagate down to nested includes From 95c22cdb6985382db7a70f005ea4a2db5f87eca7 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Mon, 12 Sep 2022 12:21:44 +0200 Subject: [PATCH 13/17] ci_complete From db3699d094905f01dc00b7ba5b1596ef7026ea35 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Mon, 12 Sep 2022 12:54:24 +0200 Subject: [PATCH 14/17] Unused import --- lib/ansible/template/vars.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index 3d5c1ebd4fb861..a1665cd4f2985b 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -2,7 +2,6 @@ # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) from collections import ChainMap -from collections.abc import Mapping from jinja2.utils import missing From c2a2f23f0a99bfdc9dd953cacb556e503a051043 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Mon, 12 Sep 2022 12:58:23 +0200 Subject: [PATCH 15/17] space --- lib/ansible/template/vars.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index a1665cd4f2985b..06b61c0e571472 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -55,7 +55,7 @@ def __getitem__(self, varname): except Exception as e: msg = getattr(e, 'message', None) or to_native(e) raise AnsibleError( - f"An unhandled exception occurred while templating'{to_native(variable)}'. " + f"An unhandled exception occurred while templating '{to_native(variable)}'. " f"Error was a {type(e)}, original message: {msg}" ) From 1326d5facdf2b92e7c0391ae004be159b26ddc84 Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Thu, 8 Dec 2022 16:15:45 +0100 Subject: [PATCH 16/17] Update lib/ansible/template/vars.py Co-authored-by: Matt Martz --- lib/ansible/template/vars.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index 06b61c0e571472..4a57f368164f14 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -18,7 +18,7 @@ def _process_locals(_l): return { k: v for k, v in _l.items() if v is not missing - and k not in ('context', 'environment', 'template') # NOTE is this really needed? + and k not in {'context', 'environment', 'template'} # NOTE is this really needed? } From b896383a04b9eccf56f4213f842f2220577473ac Mon Sep 17 00:00:00 2001 From: Martin Krizek Date: Thu, 8 Dec 2022 16:16:03 +0100 Subject: [PATCH 17/17] Update lib/ansible/template/vars.py Co-authored-by: Matt Martz --- lib/ansible/template/vars.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ansible/template/vars.py b/lib/ansible/template/vars.py index 4a57f368164f14..a7a9402ce897fc 100644 --- a/lib/ansible/template/vars.py +++ b/lib/ansible/template/vars.py @@ -37,7 +37,7 @@ def __getitem__(self, varname): variable = super().__getitem__(varname) from ansible.vars.hostvars import HostVars - if (isinstance(variable, dict) and varname == "vars") or isinstance(variable, HostVars) or hasattr(variable, '__UNSAFE__'): + if (varname == "vars" and isinstance(variable, dict)) or isinstance(variable, HostVars) or hasattr(variable, '__UNSAFE__'): return variable try: