From 00e9a70f96e0812fde733ba5e507cb0ec690edcc Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 22 Oct 2022 19:09:59 -0400 Subject: [PATCH 01/31] yaml: Fix documentation about `datetime` conversion I believe the behavior changed with commit 002aa88a97ed8a1c51f4ab1c965d22d064ea30a8 which was first released in Salt v2018.3.0. --- changelog/63158.fixed | 1 + .../troubleshooting/yaml_idiosyncrasies.rst | 53 ++++++------------- 2 files changed, 16 insertions(+), 38 deletions(-) create mode 100644 changelog/63158.fixed diff --git a/changelog/63158.fixed b/changelog/63158.fixed new file mode 100644 index 000000000000..158c1c018c4b --- /dev/null +++ b/changelog/63158.fixed @@ -0,0 +1 @@ +Updated YAML idiosyncrasies documentation diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 1ee1f5326f21..c9be42e08b60 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -382,50 +382,27 @@ Here's an example: Automatic ``datetime`` conversion ================================= -If there is a value in a YAML file formatted ``2014-01-20 14:23:23`` or -similar, YAML will automatically convert this to a Python ``datetime`` object. -These objects are not msgpack serializable, and so may break core salt -functionality. If values such as these are needed in a salt YAML file -(specifically a configuration file), they should be formatted with surrounding -strings to force YAML to serialize them as strings: +.. versionchanged:: 2018.3.0 -.. code-block:: pycon - - >>> import yaml - >>> yaml.safe_load("2014-01-20 14:23:23") - datetime.datetime(2014, 1, 20, 14, 23, 23) - >>> yaml.safe_load('"2014-01-20 14:23:23"') - '2014-01-20 14:23:23' + A YAML scalar node containing a timestamp now always produces a string. + Previously, Salt would attempt to create a Python ``datetime.datetime`` + object, even if the node contained an invalid date (for example, + ``4017-16-20``). -Additionally, numbers formatted like ``XXXX-XX-XX`` will also be converted (or -YAML will attempt to convert them, and error out if it doesn't think the date -is a real one). Thus, for example, if a minion were to have an ID of -``4017-16-20`` the minion would not start because YAML would complain that the -date was out of range. The workaround is the same, surround the offending -string with quotes: +Salt overrides PyYAML's default behavior and always loads YAML nodes that look +like timestamps (including nodes explicitly tagged with ``!!timestamp``) as +strings: .. code-block:: pycon - >>> import yaml - >>> yaml.safe_load("4017-16-20") - Traceback (most recent call last): - File "", line 1, in - File "/usr/local/lib/python2.7/site-packages/yaml/__init__.py", line 93, in safe_load - return load(stream, SafeLoader) - File "/usr/local/lib/python2.7/site-packages/yaml/__init__.py", line 71, in load - return loader.get_single_data() - File "/usr/local/lib/python2.7/site-packages/yaml/constructor.py", line 39, in get_single_data - return self.construct_document(node) - File "/usr/local/lib/python2.7/site-packages/yaml/constructor.py", line 43, in construct_document - data = self.construct_object(node) - File "/usr/local/lib/python2.7/site-packages/yaml/constructor.py", line 88, in construct_object - data = constructor(self, node) - File "/usr/local/lib/python2.7/site-packages/yaml/constructor.py", line 312, in construct_yaml_timestamp - return datetime.date(year, month, day) - ValueError: month must be in 1..12 - >>> yaml.safe_load('"4017-16-20"') - '4017-16-20' + >>> import salt.utils.yaml + >>> salt.utils.yaml.safe_load("2014-01-20 14:23:23") + '2014-01-20 14:23:23' + >>> salt.utils.yaml.safe_load("!!timestamp 2014-01-20 14:23:23") + '2014-01-20 14:23:23' +There is currently no way to force Salt to produce a Python +``datetime.datetime`` object from a timestamp in a YAML file. Keys Limited to 1024 Characters =============================== From db3b22a18c49544e3d1ff20c390514d6c9b5f860 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 22 Oct 2022 20:43:20 -0400 Subject: [PATCH 02/31] yaml: Document that `!!omap` should be avoided due to bugs --- .../troubleshooting/yaml_idiosyncrasies.rst | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index c9be42e08b60..8f5caf287b0a 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -404,6 +404,33 @@ strings: There is currently no way to force Salt to produce a Python ``datetime.datetime`` object from a timestamp in a YAML file. +Ordered Dictionaries +==================== + +The YAML specification defines an `ordered mapping type +`_ which is equivalent to a plain mapping except +iteration order is preserved. (YAML makes no guarantees about iteration order +for entries loaded from a plain mapping.) + +Ordered mappings are represented as an ``!!omap`` tagged sequence of +single-entry mappings: + +.. code-block:: yaml + + !!omap + - key1: value1 + - key2: value2 + +Starting with Python 3.6, plain ``dict`` objects iterate in insertion order so +there is no longer a strong need for the ``!!omap`` type. However, some users +may prefer the ``!!omap`` type over the plain ``!!map`` type because (1) it +makes it obvious that the order of entries is significant, and (2) it provides a +stronger guarantee of iteration order (plain mapping iteration order can be +thought of as a Salt implementation detail that may change in the future). + +Unfortunately, ``!!omap`` nodes should be avoided due to bugs in the way Salt +processes such nodes. + Keys Limited to 1024 Characters =============================== From 98b117a975a2836834fe4bfb4302236e35fe674b Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 13 Oct 2022 23:52:47 -0400 Subject: [PATCH 03/31] yaml: Convert `salt.utils.yaml` tests to pytest --- changelog/63158.fixed | 2 +- tests/pytests/unit/utils/test_yaml.py | 155 ++++++++++++++++++++++++++ tests/unit/utils/test_yamldumper.py | 37 ------ tests/unit/utils/test_yamlloader.py | 140 ----------------------- 4 files changed, 156 insertions(+), 178 deletions(-) create mode 100644 tests/pytests/unit/utils/test_yaml.py delete mode 100644 tests/unit/utils/test_yamldumper.py delete mode 100644 tests/unit/utils/test_yamlloader.py diff --git a/changelog/63158.fixed b/changelog/63158.fixed index 158c1c018c4b..2b0a8b6742a9 100644 --- a/changelog/63158.fixed +++ b/changelog/63158.fixed @@ -1 +1 @@ -Updated YAML idiosyncrasies documentation +Updated YAML idiosyncrasies documentation and improved YAML tests diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py new file mode 100644 index 000000000000..8d9ad5b21da7 --- /dev/null +++ b/tests/pytests/unit/utils/test_yaml.py @@ -0,0 +1,155 @@ +import textwrap + +import pytest +import yaml +from yaml.constructor import ConstructorError + +import salt.utils.files +import salt.utils.yaml as salt_yaml +from tests.support.mock import mock_open, patch + + +def test_dump(): + data = {"foo": "bar"} + assert salt_yaml.dump(data) == "{foo: bar}\n" + assert salt_yaml.dump(data, default_flow_style=False) == "foo: bar\n" + + +def test_safe_dump(): + data = {"foo": "bar"} + assert salt_yaml.safe_dump(data) == "{foo: bar}\n" + assert salt_yaml.safe_dump(data, default_flow_style=False) == "foo: bar\n" + + +def render_yaml(data): + """ + Takes a YAML string, puts it into a mock file, passes that to the YAML + SaltYamlSafeLoader and then returns the rendered/parsed YAML data + """ + with patch("salt.utils.files.fopen", mock_open(read_data=data)) as mocked_file: + with salt.utils.files.fopen(mocked_file) as mocked_stream: + return salt_yaml.SaltYamlSafeLoader(mocked_stream).get_data() + + +def test_load_basics(): + """ + Test parsing an ordinary path + """ + assert ( + render_yaml( + textwrap.dedent( + """\ + p1: + - alpha + - beta + """ + ) + ) + == {"p1": ["alpha", "beta"]} + ) + + +def test_load_merge(): + """ + Test YAML anchors + """ + # Simple merge test + assert ( + render_yaml( + textwrap.dedent( + """\ + p1: &p1 + v1: alpha + p2: + <<: *p1 + v2: beta + """ + ) + ) + == {"p1": {"v1": "alpha"}, "p2": {"v1": "alpha", "v2": "beta"}} + ) + + # Test that keys/nodes are overwritten + assert ( + render_yaml( + textwrap.dedent( + """\ + p1: &p1 + v1: alpha + p2: + <<: *p1 + v1: new_alpha + """ + ) + ) + == {"p1": {"v1": "alpha"}, "p2": {"v1": "new_alpha"}} + ) + + # Test merging of lists + assert ( + render_yaml( + textwrap.dedent( + """\ + p1: &p1 + v1: &v1 + - t1 + - t2 + p2: + v2: *v1 + """ + ) + ) + == {"p2": {"v2": ["t1", "t2"]}, "p1": {"v1": ["t1", "t2"]}} + ) + + +def test_load_duplicates(): + """ + Test that duplicates still throw an error + """ + with pytest.raises(ConstructorError): + render_yaml( + textwrap.dedent( + """\ + p1: alpha + p1: beta + """ + ) + ) + + with pytest.raises(ConstructorError): + render_yaml( + textwrap.dedent( + """\ + p1: &p1 + v1: alpha + p2: + <<: *p1 + v2: beta + v2: betabeta + """ + ) + ) + + +def test_load_with_plain_scalars(): + """ + Test that plain (i.e. unqoted) string and non-string scalars are + properly handled + """ + assert ( + render_yaml( + textwrap.dedent( + """\ + foo: + b: {foo: bar, one: 1, list: [1, two, 3]} + """ + ) + ) + == {"foo": {"b": {"foo": "bar", "one": 1, "list": [1, "two", 3]}}} + ) + + +def test_not_yaml_monkey_patching(): + if hasattr(yaml, "CSafeLoader"): + assert yaml.SafeLoader != yaml.CSafeLoader diff --git a/tests/unit/utils/test_yamldumper.py b/tests/unit/utils/test_yamldumper.py deleted file mode 100644 index 9a1a6ab103ba..000000000000 --- a/tests/unit/utils/test_yamldumper.py +++ /dev/null @@ -1,37 +0,0 @@ -""" - Unit tests for salt.utils.yamldumper -""" - -import salt.utils.yamldumper -from tests.support.unit import TestCase - - -class YamlDumperTestCase(TestCase): - """ - TestCase for salt.utils.yamldumper module - """ - - def test_yaml_dump(self): - """ - Test yaml.dump a dict - """ - data = {"foo": "bar"} - exp_yaml = "{foo: bar}\n" - - assert salt.utils.yamldumper.dump(data) == exp_yaml - - assert salt.utils.yamldumper.dump( - data, default_flow_style=False - ) == exp_yaml.replace("{", "").replace("}", "") - - def test_yaml_safe_dump(self): - """ - Test yaml.safe_dump a dict - """ - data = {"foo": "bar"} - assert salt.utils.yamldumper.safe_dump(data) == "{foo: bar}\n" - - assert ( - salt.utils.yamldumper.safe_dump(data, default_flow_style=False) - == "foo: bar\n" - ) diff --git a/tests/unit/utils/test_yamlloader.py b/tests/unit/utils/test_yamlloader.py deleted file mode 100644 index 04370a97083c..000000000000 --- a/tests/unit/utils/test_yamlloader.py +++ /dev/null @@ -1,140 +0,0 @@ -""" - Unit tests for salt.utils.yamlloader.SaltYamlSafeLoader -""" - -import textwrap - -from yaml.constructor import ConstructorError - -import salt.utils.files -from salt.utils.yamlloader import SaltYamlSafeLoader, yaml -from tests.support.mock import mock_open, patch -from tests.support.unit import TestCase - - -class YamlLoaderTestCase(TestCase): - """ - TestCase for salt.utils.yamlloader module - """ - - @staticmethod - def render_yaml(data): - """ - Takes a YAML string, puts it into a mock file, passes that to the YAML - SaltYamlSafeLoader and then returns the rendered/parsed YAML data - """ - with patch("salt.utils.files.fopen", mock_open(read_data=data)) as mocked_file: - with salt.utils.files.fopen(mocked_file) as mocked_stream: - return SaltYamlSafeLoader(mocked_stream).get_data() - - def test_yaml_basics(self): - """ - Test parsing an ordinary path - """ - self.assertEqual( - self.render_yaml( - textwrap.dedent( - """\ - p1: - - alpha - - beta""" - ) - ), - {"p1": ["alpha", "beta"]}, - ) - - def test_yaml_merge(self): - """ - Test YAML anchors - """ - # Simple merge test - self.assertEqual( - self.render_yaml( - textwrap.dedent( - """\ - p1: &p1 - v1: alpha - p2: - <<: *p1 - v2: beta""" - ) - ), - {"p1": {"v1": "alpha"}, "p2": {"v1": "alpha", "v2": "beta"}}, - ) - - # Test that keys/nodes are overwritten - self.assertEqual( - self.render_yaml( - textwrap.dedent( - """\ - p1: &p1 - v1: alpha - p2: - <<: *p1 - v1: new_alpha""" - ) - ), - {"p1": {"v1": "alpha"}, "p2": {"v1": "new_alpha"}}, - ) - - # Test merging of lists - self.assertEqual( - self.render_yaml( - textwrap.dedent( - """\ - p1: &p1 - v1: &v1 - - t1 - - t2 - p2: - v2: *v1""" - ) - ), - {"p2": {"v2": ["t1", "t2"]}, "p1": {"v1": ["t1", "t2"]}}, - ) - - def test_yaml_duplicates(self): - """ - Test that duplicates still throw an error - """ - with self.assertRaises(ConstructorError): - self.render_yaml( - textwrap.dedent( - """\ - p1: alpha - p1: beta""" - ) - ) - - with self.assertRaises(ConstructorError): - self.render_yaml( - textwrap.dedent( - """\ - p1: &p1 - v1: alpha - p2: - <<: *p1 - v2: beta - v2: betabeta""" - ) - ) - - def test_yaml_with_plain_scalars(self): - """ - Test that plain (i.e. unqoted) string and non-string scalars are - properly handled - """ - self.assertEqual( - self.render_yaml( - textwrap.dedent( - """\ - foo: - b: {foo: bar, one: 1, list: [1, two, 3]}""" - ) - ), - {"foo": {"b": {"foo": "bar", "one": 1, "list": [1, "two", 3]}}}, - ) - - def test_not_yaml_monkey_patching(self): - if hasattr(yaml, "CSafeLoader"): - assert yaml.SafeLoader != yaml.CSafeLoader From fc45763c4015f029e2d29721a5f416515d15ab86 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 16 Oct 2022 15:56:00 -0400 Subject: [PATCH 04/31] yaml: Add integration test for YAML map iteration order This demonstrates that https://github.com/saltstack/salt/issues/12161 has already been fixed (thanks to Python 3.6 changing `dict` to iterate in insertion order). --- .../pillar/test_pillar_map_order.py | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 tests/pytests/integration/pillar/test_pillar_map_order.py diff --git a/tests/pytests/integration/pillar/test_pillar_map_order.py b/tests/pytests/integration/pillar/test_pillar_map_order.py new file mode 100644 index 000000000000..42224abc7e1c --- /dev/null +++ b/tests/pytests/integration/pillar/test_pillar_map_order.py @@ -0,0 +1,95 @@ +import random +import textwrap + +import pytest + +pytestmark = [ + pytest.mark.slow_test, +] + + +@pytest.fixture(scope="module") +def minion_run(salt_minion, salt_cli): + """Convenience fixture that runs the ``salt`` CLI targeting the minion.""" + + def _run(*args, minion_tgt=salt_minion.id, **kwargs): + ret = salt_cli.run(*args, minion_tgt=minion_tgt, **kwargs) + assert ret.returncode == 0 + return ret.data + + yield _run + + +def test_pillar_map_order(salt_master, minion_run): + """Test iteration order of YAML map entries in a Pillar ``.sls`` file. + + This test generates a Pillar ``.sls`` file containing an ordinary YAML map + and tests whether the resulting Python object preserves iteration order. + Random keys are used to ensure that iteration order does not coincidentally + match. The generated Pillar YAML file looks like this: + + .. code-block:: yaml + + data: + k3334244338: 0 + k3444116829: 1 + k2072366017: 2 + # ... omitted for brevity ... + k1638299831: 19 + + A jinja template iterates over the entries in the resulting object to ensure + that iteration order is preserved. The expected output looks like: + + .. code-block:: text + + k3334244338 0 + k3444116829 1 + k2072366017 2 + ... omitted for brevity ... + k1638299831 19 + + Note: Python 3.6 switched to a new ``dict`` implementation that iterates in + insertion order. This behavior was made an official part of the ``dict`` + API in Python 3.7: + + * https://docs.python.org/3.6/whatsnew/3.6.html#new-dict-implementation + * https://mail.python.org/pipermail/python-dev/2017-December/151283.html + * https://docs.python.org/3.7/whatsnew/3.7.html + + Thus, this test may fail on Python 3.5 and older. However, Salt currently + requires a newer version of Python, so this should not be a problem. + + This is a regression test for: + https://github.com/saltstack/salt/issues/12161 + """ + # Filter the random keys through a set to avoid duplicates. + keys = list({f"k{random.getrandbits(32)}" for _ in range(20)}) + # Avoid unintended correlation with set()'s iteration order. + random.shuffle(keys) + items = [(k, i) for i, k in enumerate(keys)] + top_yaml = "base: {'*': [data]}\n" + top_sls = salt_master.pillar_tree.base.temp_file("top.sls", top_yaml) + data_yaml = "data:\n" + "".join(f" {k}: {v}\n" for k, v in items) + data_sls = salt_master.pillar_tree.base.temp_file("data.sls", data_yaml) + tmpl_jinja = textwrap.dedent( + """\ + {%- for k, v in pillar['data'].items() %} + {{ k }} {{ v }} + {%- endfor %} + """ + ) + want = "\n" + "".join(f"{k} {v}\n" for k, v in items) + try: + with top_sls, data_sls: + assert minion_run("saltutil.refresh_pillar", wait=True) is True + got = minion_run( + "file.apply_template_on_contents", + tmpl_jinja, + template="jinja", + context={}, + defaults={}, + saltenv="base", + ) + assert got == want + finally: + assert minion_run("saltutil.refresh_pillar", wait=True) is True From e6d2116fa45db7cfeb572962b69f9cb177081aeb Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 02:25:30 -0400 Subject: [PATCH 05/31] yaml: Add TODO comments next to puzzling code --- salt/utils/yamldumper.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index e5e937cac7d6..12e5bc720aa8 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -71,6 +71,17 @@ def represent_undefined(dumper, data): OrderedDumper.add_representer(OrderedDict, represent_ordereddict) SafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict) + +# This default registration matches types that don't match any other +# registration, overriding PyYAML's default behavior of raising an exception. +# This representer instead produces null nodes. +# +# TODO: Why does this registration exist? Isn't it better to raise an exception +# for unsupported types? +# +# TODO: This representer could also be registered with OrderedDumper without +# changing its behavior because Dumper has a multi representer registered +# for `object` that takes priority. SafeOrderedDumper.add_representer(None, represent_undefined) OrderedDumper.add_representer( @@ -88,6 +99,7 @@ def represent_undefined(dumper, data): yaml.representer.SafeRepresenter.represent_dict, ) +# TODO: These seem wrong: the first argument should be a type, not a tag. OrderedDumper.add_representer( "tag:yaml.org,2002:timestamp", OrderedDumper.represent_scalar ) From 46128e222bc535a97ee362f3aa67df0396f8cfdb Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 00:56:04 -0400 Subject: [PATCH 06/31] yaml: Delete unnecessary `IndentMixin` class to improve readability --- changelog/63158.fixed | 3 ++- salt/utils/yamldumper.py | 21 +++++---------------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/changelog/63158.fixed b/changelog/63158.fixed index 2b0a8b6742a9..76f019daf155 100644 --- a/changelog/63158.fixed +++ b/changelog/63158.fixed @@ -1 +1,2 @@ -Updated YAML idiosyncrasies documentation and improved YAML tests +Updated YAML idiosyncrasies documentation, improved YAML tests, and improved +readability of YAML code diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 12e5bc720aa8..4635030718de 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -31,17 +31,6 @@ ] -class IndentMixin(Dumper): - """ - Mixin that improves YAML dumped list readability - by indenting them by two spaces, - instead of being flush with the key they are under. - """ - - def increase_indent(self, flow=False, indentless=False): - return super().increase_indent(flow, False) - - class OrderedDumper(Dumper): """ A YAML dumper that represents python OrderedDict as simple YAML map. @@ -54,11 +43,11 @@ class SafeOrderedDumper(SafeDumper): """ -class IndentedSafeOrderedDumper(IndentMixin, SafeOrderedDumper): - """ - A YAML safe dumper that represents python OrderedDict as simple YAML map, - and also indents lists by two spaces. - """ +class IndentedSafeOrderedDumper(SafeOrderedDumper): + """Like ``SafeOrderedDumper``, except it indents lists for readability.""" + + def increase_indent(self, flow=False, indentless=False): + return super().increase_indent(flow, False) def represent_ordereddict(dumper, data): From 128547f1477b26309e9df2cbe7f00903fe0de472 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 1 Dec 2022 01:09:39 -0500 Subject: [PATCH 07/31] yaml: Use a `for` loop to factor out duplicate code --- salt/utils/yamldumper.py | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 4635030718de..2a1a7e120398 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -58,8 +58,9 @@ def represent_undefined(dumper, data): return dumper.represent_scalar("tag:yaml.org,2002:null", "NULL") -OrderedDumper.add_representer(OrderedDict, represent_ordereddict) -SafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict) +# OrderedDumper does not inherit from SafeOrderedDumper, so any applicable +# representers added to SafeOrderedDumper must also be explicitly added to +# OrderedDumper. # This default registration matches types that don't match any other # registration, overriding PyYAML's default behavior of raising an exception. @@ -73,28 +74,18 @@ def represent_undefined(dumper, data): # for `object` that takes priority. SafeOrderedDumper.add_representer(None, represent_undefined) -OrderedDumper.add_representer( - collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict -) -SafeOrderedDumper.add_representer( - collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict -) -OrderedDumper.add_representer( - salt.utils.context.NamespacedDictWrapper, - yaml.representer.SafeRepresenter.represent_dict, -) -SafeOrderedDumper.add_representer( - salt.utils.context.NamespacedDictWrapper, - yaml.representer.SafeRepresenter.represent_dict, -) - -# TODO: These seem wrong: the first argument should be a type, not a tag. -OrderedDumper.add_representer( - "tag:yaml.org,2002:timestamp", OrderedDumper.represent_scalar -) -SafeOrderedDumper.add_representer( - "tag:yaml.org,2002:timestamp", SafeOrderedDumper.represent_scalar -) +for D in (SafeOrderedDumper, OrderedDumper): + D.add_representer(OrderedDict, represent_ordereddict) + D.add_representer( + collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict + ) + D.add_representer( + salt.utils.context.NamespacedDictWrapper, + yaml.representer.SafeRepresenter.represent_dict, + ) + # TODO: This seems wrong: the first argument should be a type, not a tag. + D.add_representer("tag:yaml.org,2002:timestamp", Dumper.represent_scalar) +del D def get_dumper(dumper_name): From 8f46b4217352d24868fc4dccd0bae9db5d2d370f Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 10 Nov 2022 16:19:12 -0500 Subject: [PATCH 08/31] yaml: Register default representer with `OrderedDumper` too This does not change the behavior, but it does simplify the code. --- salt/utils/yamldumper.py | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 2a1a7e120398..23b03683ebec 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -61,20 +61,14 @@ def represent_undefined(dumper, data): # OrderedDumper does not inherit from SafeOrderedDumper, so any applicable # representers added to SafeOrderedDumper must also be explicitly added to # OrderedDumper. - -# This default registration matches types that don't match any other -# registration, overriding PyYAML's default behavior of raising an exception. -# This representer instead produces null nodes. -# -# TODO: Why does this registration exist? Isn't it better to raise an exception -# for unsupported types? -# -# TODO: This representer could also be registered with OrderedDumper without -# changing its behavior because Dumper has a multi representer registered -# for `object` that takes priority. -SafeOrderedDumper.add_representer(None, represent_undefined) - for D in (SafeOrderedDumper, OrderedDumper): + # This default registration matches types that don't match any other + # registration, overriding PyYAML's default behavior of raising an + # exception. This representer instead produces null nodes. + # + # TODO: Why does this registration exist? Isn't it better to raise an + # exception for unsupported types? + D.add_representer(None, represent_undefined) D.add_representer(OrderedDict, represent_ordereddict) D.add_representer( collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict From 51d9962ef89b9b9a3ca3ecab5c6a022cd78c8c72 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 19 Oct 2022 22:37:47 -0400 Subject: [PATCH 09/31] yaml: Factor out duplicate code in `salt.utils.yaml.safe_dump()` --- salt/utils/yamldumper.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 23b03683ebec..1883cce1d982 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -109,7 +109,4 @@ def safe_dump(data, stream=None, **kwargs): represented properly. Ensure that unicode strings are encoded unless explicitly told not to. """ - if "allow_unicode" not in kwargs: - kwargs["allow_unicode"] = True - kwargs.setdefault("default_flow_style", None) - return yaml.dump(data, stream, Dumper=SafeOrderedDumper, **kwargs) + return dump(data, stream, Dumper=SafeOrderedDumper, **kwargs) From bec44bd8aaf3262002f240bfba2ce3b4ee1c09f9 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 00:14:58 -0400 Subject: [PATCH 10/31] yaml: Improve readability of `salt.utils.yaml.dump()` --- salt/utils/yamldumper.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 1883cce1d982..ef593eb72349 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -97,8 +97,7 @@ def dump(data, stream=None, **kwargs): Helper that wraps yaml.dump and ensures that we encode unicode strings unless explicitly told not to. """ - if "allow_unicode" not in kwargs: - kwargs["allow_unicode"] = True + kwargs.setdefault("allow_unicode", True) kwargs.setdefault("default_flow_style", None) return yaml.dump(data, stream, **kwargs) From 4d16675c16e4ea36fdd295b3ecceae4655be595a Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 3 Dec 2022 00:13:42 -0500 Subject: [PATCH 11/31] Add missing `import` statement The `salt.loader` module is intentially imported late in `salt.config.call_id_function()` to avoid a circular import error. However, `salt.loader` is also used in `salt.config.mminion_config()`, so import it there as well. This avoids an error if `mminion_config()` is called before `call_id_function()`, or if the import statement in `call_id_function()` is removed in the future. --- salt/config/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/salt/config/__init__.py b/salt/config/__init__.py index 372555969d2a..dcf68d3aef2b 100644 --- a/salt/config/__init__.py +++ b/salt/config/__init__.py @@ -2310,6 +2310,7 @@ def mminion_config(path, overrides, ignore_config_errors=True): apply_sdb(opts) _validate_opts(opts) + import salt.loader opts["grains"] = salt.loader.grains(opts) opts["pillar"] = {} return opts From 65e2d3330cb8efb84be176b9b7998232434edde3 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 10 Nov 2022 19:46:07 -0500 Subject: [PATCH 12/31] yaml: Delete the ineffectual timestamp representer --- salt/utils/yamldumper.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index ef593eb72349..a1241a8ee596 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -77,8 +77,6 @@ def represent_undefined(dumper, data): salt.utils.context.NamespacedDictWrapper, yaml.representer.SafeRepresenter.represent_dict, ) - # TODO: This seems wrong: the first argument should be a type, not a tag. - D.add_representer("tag:yaml.org,2002:timestamp", Dumper.represent_scalar) del D From 7895efbd3be5dc1be25408fdd027327e16279e65 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 3 Dec 2022 00:24:59 -0500 Subject: [PATCH 13/31] Replace redundant imports with `global` declarations --- salt/loader/__init__.py | 5 +++-- salt/minion.py | 6 ++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/salt/loader/__init__.py b/salt/loader/__init__.py index 72a5e5440128..72219839e9b8 100644 --- a/salt/loader/__init__.py +++ b/salt/loader/__init__.py @@ -1060,8 +1060,9 @@ def grains(opts, force_refresh=False, proxy=None, context=None, loaded_base_name __grains__ = salt.loader.grains(__opts__) print __grains__['id'] """ - # Need to re-import salt.config, somehow it got lost when a minion is starting - import salt.config + # Without this global declaration, the late `import` statement below causes + # `salt` to become a function-local variable. + global salt # if we have no grains, lets try loading from disk (TODO: move to decorator?) cfn = os.path.join(opts["cachedir"], "grains.cache.p") diff --git a/salt/minion.py b/salt/minion.py index 7cbaa35f3e24..36884cca40d3 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -914,9 +914,11 @@ class SMinion(MinionBase): """ def __init__(self, opts, context=None): - # Late setup of the opts grains, so we can log from the grains module - import salt.loader + # Without this global declaration, the late `import` statement below + # causes `salt` to become a function-local variable. + global salt + # Late setup of the opts grains, so we can log from the grains module opts["grains"] = salt.loader.grains(opts) super().__init__(opts) From 6e8015f004f18cf55e2979f0cbea825771e20e6d Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 26 Oct 2022 22:48:02 -0400 Subject: [PATCH 14/31] tests: Use `salt.utils.yaml` to generate reference YAML The same YAML value can be represented in different ways, so tests that compare a generated YAML string with a manually typed string are fragile. Use `salt.utils.yaml` to generate the reference YAML so that the implementation of the YAML dumper can change without breaking the tests. --- tests/pytests/unit/modules/test_schedule.py | 18 +++++++++++++++++- tests/pytests/unit/modules/test_seed.py | 3 ++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/tests/pytests/unit/modules/test_schedule.py b/tests/pytests/unit/modules/test_schedule.py index d3488559143f..ac0fb1a1388d 100644 --- a/tests/pytests/unit/modules/test_schedule.py +++ b/tests/pytests/unit/modules/test_schedule.py @@ -9,6 +9,7 @@ import salt.modules.schedule as schedule import salt.utils.odict +import salt.utils.yaml from salt.utils.event import SaltEvent from salt.utils.odict import OrderedDict from tests.support.mock import MagicMock, call, mock_open, patch @@ -278,7 +279,22 @@ def test_add(): ) == {"comment": comm1, "changes": changes1, "result": True} _call = call( - b"schedule:\n job3: {function: test.ping, seconds: 3600, maxrunning: 1, name: job3, enabled: true,\n jid_include: true}\n" + salt.utils.yaml.safe_dump( + { + "schedule": { + "job3": OrderedDict( + [ + ("function", "test.ping"), + ("seconds", 3600), + ("maxrunning", 1), + ("name", "job3"), + ("enabled", True), + ("jid_include", True), + ], + ), + }, + } + ).encode() ) write_calls = fopen_mock.filehandles[schedule_config_file][ 1 diff --git a/tests/pytests/unit/modules/test_seed.py b/tests/pytests/unit/modules/test_seed.py index ee51cdc3a5cd..360e10dff3b2 100644 --- a/tests/pytests/unit/modules/test_seed.py +++ b/tests/pytests/unit/modules/test_seed.py @@ -11,6 +11,7 @@ import salt.modules.seed as seed import salt.utils.files import salt.utils.odict +import salt.utils.yaml from tests.support.mock import MagicMock, patch @@ -28,7 +29,7 @@ def test_mkconfig_odict(): data = seed.mkconfig(ddd, approve_key=False) with salt.utils.files.fopen(data["config"]) as fic: fdata = fic.read() - assert fdata == "b: b\na: b\nmaster: foo\n" + assert fdata == salt.utils.yaml.safe_dump(ddd, default_flow_style=False) def test_prep_bootstrap(): From 3bfda3d85fbd9bd4f1fce9f4b06cb3e56d34a2c7 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 3 Dec 2022 01:33:35 -0500 Subject: [PATCH 15/31] Delete unused and redundant `import` statements The delayed `import` statements match `import` statements at the top of the file so they are effectively no-ops. --- salt/minion.py | 1 - salt/utils/args.py | 6 ------ salt/utils/decorators/__init__.py | 1 - 3 files changed, 8 deletions(-) diff --git a/salt/minion.py b/salt/minion.py index 36884cca40d3..ac7fa40e3a5c 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -123,7 +123,6 @@ def resolve_dns(opts, fallback=True): "use_master_when_local", False ): check_dns = False - import salt.utils.network if check_dns is True: try: diff --git a/salt/utils/args.py b/salt/utils/args.py index 536aea38166c..0cbc1592a7df 100644 --- a/salt/utils/args.py +++ b/salt/utils/args.py @@ -170,9 +170,6 @@ def yamlify_arg(arg): return arg try: - # Explicit late import to avoid circular import. DO NOT MOVE THIS. - import salt.utils.yaml - original_arg = arg if "#" in arg: # Only yamlify if it parses into a non-string type, to prevent @@ -360,9 +357,6 @@ def test_mode(**kwargs): "Test" in any variation on capitalization (i.e. "TEST", "Test", "TeSt", etc) contains a True value (as determined by salt.utils.data.is_true). """ - # Once is_true is moved, remove this import and fix the ref below - import salt.utils - for arg, value in kwargs.items(): try: if arg.lower() == "test" and salt.utils.data.is_true(value): diff --git a/salt/utils/decorators/__init__.py b/salt/utils/decorators/__init__.py index 1f62d5f3d654..a735a7aa1c1f 100644 --- a/salt/utils/decorators/__init__.py +++ b/salt/utils/decorators/__init__.py @@ -13,7 +13,6 @@ from functools import wraps import salt.utils.args -import salt.utils.data import salt.utils.versions from salt.exceptions import ( CommandExecutionError, From b3554d5d482fe176b6a0cd6f52ebb69d26f60f2a Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 14 Nov 2022 19:14:56 -0500 Subject: [PATCH 16/31] decorators: New `@classproperty` decorator --- salt/utils/decorators/__init__.py | 39 +++++++++++++++ tests/pytests/unit/utils/test_decorators.py | 53 +++++++++++++++++++++ 2 files changed, 92 insertions(+) create mode 100644 tests/pytests/unit/utils/test_decorators.py diff --git a/salt/utils/decorators/__init__.py b/salt/utils/decorators/__init__.py index a735a7aa1c1f..46a17b403798 100644 --- a/salt/utils/decorators/__init__.py +++ b/salt/utils/decorators/__init__.py @@ -865,3 +865,42 @@ def wrapped(*args, **kwargs): return function(*args, **kwargs) return wrapped + + +class classproperty(property): + """Decorator like :py:func:`property`, but for class properties. + + Usage example: + + .. code-block:: python + + class Foo: + @classproperty + def custom_prop(cls): + return "hello world" + + assert Foo.custom_prop == "hello world" + + """ + + def __init__(self, fget=None, fset=None, fdel=None, doc=None): + if fset is not None: + raise NotImplementedError( + "classproperty setters are not supported due to a limitation " + "in Python: " + ) + if fdel is not None: + raise NotImplementedError( + "classproperty deleters are not supported due to a limitation " + "in Python: " + ) + super().__init__(fget, doc=doc) + + def __get__(self, obj, objtype=None): + if objtype is None: + objtype = type(obj) + # The second argument probably doesn't matter, but @classmethod does + # `__get__(objtype, objtype)`, not `__get__(objtype, type(objtype))`, so + # the same thing is done here for consistency. + # https://github.com/python/cpython/blob/v3.11.0/Objects/funcobject.c#L900-L901 + return super().__get__(objtype, objtype) diff --git a/tests/pytests/unit/utils/test_decorators.py b/tests/pytests/unit/utils/test_decorators.py new file mode 100644 index 000000000000..f45229810137 --- /dev/null +++ b/tests/pytests/unit/utils/test_decorators.py @@ -0,0 +1,53 @@ +import pytest + +import salt.utils.decorators + + +def test_classproperty(): + class Foo: + @salt.utils.decorators.classproperty + def prop(cls): # pylint: disable=no-self-argument + return cls + + assert Foo.prop is Foo + assert Foo().prop is Foo + + +def test_classproperty_getter(): + class Foo: + @salt.utils.decorators.classproperty + def prop(cls): # pylint: disable=no-self-argument + return "old" + + @prop.getter + def prop(cls): # pylint: disable=no-self-argument + return "new" + + assert Foo.prop == "new" + assert Foo().prop == "new" + + +def test_classproperty_setter(): + with pytest.raises(NotImplementedError): + + class Foo: + @salt.utils.decorators.classproperty + def prop(cls): # pylint: disable=no-self-argument + return "getter" + + @prop.setter + def prop(cls, val): # pylint: disable=no-self-argument + return "setter" + + +def test_classproperty_deleter(): + with pytest.raises(NotImplementedError): + + class Foo: + @salt.utils.decorators.classproperty + def prop(cls): # pylint: disable=no-self-argument + return "getter" + + @prop.deleter + def prop(cls): # pylint: disable=no-self-argument + return "deleter" From 5a53f982fd1d4cccec7a7cca6bce8b1c6d032574 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 10 Nov 2022 00:57:38 -0500 Subject: [PATCH 17/31] yaml: Improve inheritance of registered constructors/representers This commit is a prerequisite to adding version-selected behavior changes. --- salt/utils/_yaml_common.py | 76 ++++++++++++++++++++++++++++++++++++ salt/utils/yamldumper.py | 80 +++++++++++++++++++++++--------------- salt/utils/yamlloader.py | 13 ++++++- 3 files changed, 136 insertions(+), 33 deletions(-) create mode 100644 salt/utils/_yaml_common.py diff --git a/salt/utils/_yaml_common.py b/salt/utils/_yaml_common.py new file mode 100644 index 000000000000..08f94c0663b7 --- /dev/null +++ b/salt/utils/_yaml_common.py @@ -0,0 +1,76 @@ +import collections + + +class InheritMapMixin: + """Adds :py:class:`collections.ChainMap` class attributes based on MRO. + + The added class attributes provide each subclass with its own mapping that + works just like method resolution: each ancestor type in ``__mro__`` is + visited until an entry is found in the ancestor-owned map. + + For example, for an inheritance DAG of ``InheritMapMixin`` <- ``Foo`` <- + ``Bar`` <- ``Baz`` and a desired attribute name of ``"_m"``: + + 1. ``Foo._m`` is set to ``ChainMap({})``. + 2. ``Bar._m`` is set to ``ChainMap({}, Foo._m.maps[0])``. + 3. ``Baz._m`` is set to ``ChainMap({}, Bar._m.maps[0], Foo._m.maps[0])``. + + This mixin makes it possible to improve PyYAML's representer and constructor + registration APIs. Currently, the PyYAML + :py:class:`yaml.representer.BaseRepresenter` and + :py:class:`yaml.constructor.BaseConstructor` classes do not use base class + inheritance of registered representers/constructors. Instead, the base + class's registrations are copied to the subclass's registration dict the + first time a registration is added to the subclass. (Before the first + registration is added to the subclass, MRO lookup finds the base class's + registration dict.) This means that any registrations added to a base class + after a registration is added to a subclass do not automatically appear in + the subclass. This results in the need to either add all registrations + before any subclass registrations are added, or manually add registrations + to each subclass in addition to the base class. + + If this mixin is used to provide ``yaml_representers`` and + ``yaml_multi_representers`` class attributes for PyYAML dumper classes, + then registered representers are truly inherited, not copied, from their + base classes. The same goes for constructors registered in the + ``yaml_constructors`` and ``yaml_multi_constructors`` class attributes for + PyYAML loader classes. + + """ + + @classmethod + def __init_subclass__(cls, *, inherit_map_attrs=None, **kwargs): + """Adds :py:class:`collections.ChainMap` class attributes based on MRO. + + :param inherit_map_attrs: + Optional mapping: + + * Each key in the mapping is the name of a class attribute that will + be set to a :py:class:`~collections.ChainMap` containing the + MRO-based list of ancestor maps. + * Each value is an optional (may be ``None``) fallback class + attribute name to try if an ancestor class in the MRO does not + have a chain map provided by ``InheritMapMixin``. For such + ancestor classes, if it has a non-``None`` attribute with that + name, the value is added to the chain if it is not already + present. + + """ + super().__init_subclass__(**kwargs) + attrs = getattr(cls, "_inherit_map_attrs", {}) + if inherit_map_attrs: + attrs = {**attrs, **inherit_map_attrs} + cls._inherit_map_attrs = attrs + for name, fallback_name in attrs.items(): + maps = [{}] + for c in cls.__mro__[1:]: # cls.__mro__[0] is cls itself. + if issubclass(InheritMapMixin, c): + continue # Ignore InheritMapMixin itself and object. + c_attrs = getattr(c, "_inherit_map_attrs", {}) + if issubclass(c, InheritMapMixin) and name in c_attrs: + maps.append(getattr(c, name).maps[0]) + elif fallback_name: + m = getattr(c, fallback_name, None) + if m is not None and m not in maps: + maps.append(m) + setattr(cls, name, collections.ChainMap(*maps)) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index a1241a8ee596..88b0758aadf9 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -11,6 +11,7 @@ import yaml # pylint: disable=blacklisted-import +import salt.utils._yaml_common as _yaml_common import salt.utils.context from salt.utils.odict import OrderedDict @@ -31,13 +32,58 @@ ] -class OrderedDumper(Dumper): +class _InheritedRepresentersMixin( + _yaml_common.InheritMapMixin, + inherit_map_attrs={ + "yaml_representers": "yaml_representers", + "yaml_multi_representers": "yaml_multi_representers", + }, +): + pass + + +class _CommonMixin( + _InheritedRepresentersMixin, + yaml.representer.BaseRepresenter, +): + def _rep_ordereddict(self, data): + return self.represent_dict(list(data.items())) + + def _rep_default(self, data): + """Represent types that don't match any other registered representers. + + PyYAML's default behavior for unsupported types is to raise an + exception. This representer instead produces null nodes. + + Note: This representer does not affect ``Dumper``-derived classes + because ``Dumper`` has a multi representer registered for ``object`` + that will match every object before PyYAML falls back to this + representer. + + """ + return self.represent_scalar("tag:yaml.org,2002:null", "NULL") + + +# TODO: Why does this registration exist? Isn't it better to raise an exception +# for unsupported types? +_CommonMixin.add_representer(None, _CommonMixin._rep_default) +_CommonMixin.add_representer(OrderedDict, _CommonMixin._rep_ordereddict) +_CommonMixin.add_representer( + collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict +) +_CommonMixin.add_representer( + salt.utils.context.NamespacedDictWrapper, + yaml.representer.SafeRepresenter.represent_dict, +) + + +class OrderedDumper(_CommonMixin, Dumper): """ A YAML dumper that represents python OrderedDict as simple YAML map. """ -class SafeOrderedDumper(SafeDumper): +class SafeOrderedDumper(_CommonMixin, SafeDumper): """ A YAML safe dumper that represents python OrderedDict as simple YAML map. """ @@ -50,36 +96,6 @@ def increase_indent(self, flow=False, indentless=False): return super().increase_indent(flow, False) -def represent_ordereddict(dumper, data): - return dumper.represent_dict(list(data.items())) - - -def represent_undefined(dumper, data): - return dumper.represent_scalar("tag:yaml.org,2002:null", "NULL") - - -# OrderedDumper does not inherit from SafeOrderedDumper, so any applicable -# representers added to SafeOrderedDumper must also be explicitly added to -# OrderedDumper. -for D in (SafeOrderedDumper, OrderedDumper): - # This default registration matches types that don't match any other - # registration, overriding PyYAML's default behavior of raising an - # exception. This representer instead produces null nodes. - # - # TODO: Why does this registration exist? Isn't it better to raise an - # exception for unsupported types? - D.add_representer(None, represent_undefined) - D.add_representer(OrderedDict, represent_ordereddict) - D.add_representer( - collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict - ) - D.add_representer( - salt.utils.context.NamespacedDictWrapper, - yaml.representer.SafeRepresenter.represent_dict, - ) -del D - - def get_dumper(dumper_name): return { "OrderedDumper": OrderedDumper, diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index 25b4b3bb9360..8cd07c428f44 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -7,6 +7,7 @@ from yaml.constructor import ConstructorError from yaml.nodes import MappingNode, SequenceNode +import salt.utils._yaml_common as _yaml_common import salt.utils.stringutils # prefer C bindings over python when available @@ -16,8 +17,18 @@ __all__ = ["SaltYamlSafeLoader", "load", "safe_load"] +class _InheritedConstructorsMixin( + _yaml_common.InheritMapMixin, + inherit_map_attrs={ + "yaml_constructors": "yaml_constructors", + "yaml_multi_constructors": "yaml_multi_constructors", + }, +): + pass + + # with code integrated from https://gist.github.com/844388 -class SaltYamlSafeLoader(BaseLoader): +class SaltYamlSafeLoader(_InheritedConstructorsMixin, BaseLoader): """ Create a custom YAML loader that uses the custom constructor. This allows for the YAML loading defaults to be manipulated based on needs within salt From f3690f267988d692865c8ad08fe8596332a470ea Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 28 Oct 2022 00:53:34 -0400 Subject: [PATCH 18/31] yaml: New option to control YAML compatibility This will make it possible for users to switch to an older YAML loader/dumper behavior in case a change breaks something. Note that several top-of-file imports were moved into functions to avoid circular import errors. --- doc/ref/configuration/master.rst | 39 +++ doc/ref/configuration/minion.rst | 40 +++ .../troubleshooting/yaml_idiosyncrasies.rst | 5 + salt/config/__init__.py | 5 +- salt/loader/__init__.py | 2 +- salt/utils/_yaml_common.py | 238 ++++++++++++++++++ salt/utils/args.py | 2 +- salt/utils/data.py | 2 +- salt/utils/decorators/__init__.py | 3 +- salt/utils/yamldumper.py | 38 ++- salt/utils/yamlloader.py | 42 +++- tests/conftest.py | 7 +- tests/pytests/unit/utils/test_yaml.py | 69 +++++ tests/support/pytest/yaml.py | 99 ++++++++ 14 files changed, 579 insertions(+), 12 deletions(-) create mode 100644 tests/support/pytest/yaml.py diff --git a/doc/ref/configuration/master.rst b/doc/ref/configuration/master.rst index 2e2086e0e789..2b1a8d8a0f3a 100644 --- a/doc/ref/configuration/master.rst +++ b/doc/ref/configuration/master.rst @@ -1177,6 +1177,45 @@ See the :ref:`2019.2.1 release notes ` for more details. use_yamlloader_old: False +.. conf_master:: yaml_compatibility + +``yaml_compatibility`` +---------------------- + +.. versionadded:: 3006.0 + +Default: ``None`` + +Force the behavior of the YAML loader (parser) and dumper (generator) to match +the default behavior from a specific version of Salt. Omitting this or setting +it to ``None`` (in YAML: ``null``, ``~``, or an empty value) is equivalent to +setting it to the current version of Salt. + +Changes that are expected in a future version of Salt can be previewed by +setting this to that future version number. + +This setting does not take effect until after the config file is parsed (so +setting this value does not change how the config file itself is parsed). + +Note that pillar `.sls` files are parsed on the master so setting this on the +master affects pillar YAML files. State `.sls` files are parsed by the minions, +so setting this on the master does not affect how state YAML files are parsed. + +.. code-block:: yaml + + # Use the default behavior. + yaml_compatibility: null + + # Any of the following will force the YAML loader/dumper to behave like + # it did in Salt v3006.0: + yaml_compatibility: 3006 + yaml_compatibility: 3006.0 + yaml_compatibility: v3006 + yaml_compatibility: v3006.0 + yaml_compatibility: sulfur + yaml_compatibility: Sulfur + yaml_compatibility: SULFUR + .. conf_master:: req_server_niceness ``req_server_niceness`` diff --git a/doc/ref/configuration/minion.rst b/doc/ref/configuration/minion.rst index bf2fe0e69586..6c80518df6f8 100644 --- a/doc/ref/configuration/minion.rst +++ b/doc/ref/configuration/minion.rst @@ -1687,6 +1687,46 @@ See the :ref:`2019.2.1 release notes ` for more details. use_yamlloader_old: False +.. conf_minion:: yaml_compatibility + +``yaml_compatibility`` +---------------------- + +.. versionadded:: 3006.0 + +Default: ``None`` + +Force the behavior of the YAML loader (parser) and dumper (generator) to match +the default behavior from a specific version of Salt. Omitting this or setting +it to ``None`` (in YAML: ``null``, ``~``, or an empty value) is equivalent to +setting it to the current version of Salt. + +Changes that are expected in a future version of Salt can be previewed by +setting this to that future version number. + +This setting does not take effect until after the config file is parsed (so +setting this value does not change how the config file itself is parsed). + +Note that pillar `.sls` files are parsed on the master so setting this on the +minion does not affect pillar YAML files. State `.sls` files are parsed by the +minions, so setting this on the minion does affect how state YAML files are +parsed. + +.. code-block:: yaml + + # Use the default behavior. + yaml_compatibility: null + + # Any of the following will force the YAML loader/dumper to behave like + # it did in Salt v3006.0: + yaml_compatibility: 3006 + yaml_compatibility: 3006.0 + yaml_compatibility: v3006 + yaml_compatibility: v3006.0 + yaml_compatibility: sulfur + yaml_compatibility: Sulfur + yaml_compatibility: SULFUR + Docker Configuration ==================== diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 8f5caf287b0a..6664a4a9b217 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -13,6 +13,11 @@ unexpected times. .. _`YAML`: https://yaml.org/spec/1.1/ +.. versionchanged:: 3006.0 + + All of the behavior changes slated for Salt v3007.0 can be previewed by + setting the ``yaml_compatibility`` option to 3007. + Spaces vs Tabs ============== diff --git a/salt/config/__init__.py b/salt/config/__init__.py index dcf68d3aef2b..2e9532517355 100644 --- a/salt/config/__init__.py +++ b/salt/config/__init__.py @@ -27,7 +27,6 @@ import salt.utils.validate.path import salt.utils.versions import salt.utils.xdg -import salt.utils.yaml from salt._logging import ( DFLT_LOG_DATEFMT, DFLT_LOG_DATEFMT_LOGFILE, @@ -979,6 +978,7 @@ def _gather_buffer_space(): "pass_gnupghome": str, # pass renderer: Set PASSWORD_STORE_DIR env for Pass "pass_dir": str, + "yaml_compatibility": (type(None), str, int), } ) @@ -1283,6 +1283,7 @@ def _gather_buffer_space(): "global_state_conditions": None, "reactor_niceness": None, "fips_mode": False, + "yaml_compatibility": None, } ) @@ -1626,6 +1627,7 @@ def _gather_buffer_space(): "pass_gnupghome": "", "pass_dir": "", "netapi_enable_clients": [], + "yaml_compatibility": None, } ) @@ -1995,6 +1997,7 @@ def _read_conf_file(path): """ Read in a config file from a given path and process it into a dictionary """ + import salt.utils.yaml log.debug("Reading configuration from %s", path) append_file_suffix_YAMLError = False with salt.utils.files.fopen(path, "r") as conf_file: diff --git a/salt/loader/__init__.py b/salt/loader/__init__.py index 72219839e9b8..956ec701a062 100644 --- a/salt/loader/__init__.py +++ b/salt/loader/__init__.py @@ -12,7 +12,6 @@ import time import types -import salt.config import salt.defaults.events import salt.defaults.exitcodes import salt.loader.context @@ -1077,6 +1076,7 @@ def grains(opts, force_refresh=False, proxy=None, context=None, loaded_base_name return {} grains_deep_merge = opts.get("grains_deep_merge", False) is True if "conf_file" in opts: + import salt.config pre_opts = {} pre_opts.update( salt.config.load_config( diff --git a/salt/utils/_yaml_common.py b/salt/utils/_yaml_common.py index 08f94c0663b7..6351b8239f6d 100644 --- a/salt/utils/_yaml_common.py +++ b/salt/utils/_yaml_common.py @@ -1,4 +1,242 @@ import collections +import contextvars +import logging +import warnings + +import salt.utils.versions +import salt.version +from salt.utils.decorators import classproperty +from salt.version import SaltStackVersion + +__opts__ = { + "yaml_compatibility": None, +} +__salt_loader__ = None + +log = logging.getLogger(__name__) + +_compat_opt = contextvars.ContextVar(f"{__name__}._compat_opt") +_compat_ver = contextvars.ContextVar(f"{__name__}._compat_ver") + +# Current Salt release. Changing this value potentially changes the default +# behavior of the YAML loader/dumper logic (the behavior when the +# `yaml_compatibility` option is left unset). This should be kept in sync with +# `SaltStackVersion.current_release()` (modulo +# https://github.com/saltstack/salt/issues/62972). +# +# This variable could be initialized to `SaltStackVersion.current_release()` to +# avoid the need to manually keep it in sync, but updating it separately has a +# few advantages: +# * It is easier to create a new release because doing so can't trigger YAML +# behavior changes that cause test failures that are difficult to root +# cause. +# * Updating this in a separate commit makes it possible for `git bisect` to +# reveal that a mysterious new bug is caused by a YAML behavior change. +# * Updating this in a separate PR provides a good opportunity to mention in +# the changelog that previously announced opt-in YAML behavior changes are +# now opt-out. +_current_ver = SaltStackVersion(3006) + + +class OverrideNotice(UserWarning): + pass + + +class UnsupportedValueWarning(UserWarning): + pass + + +# Delayed import of salt.loader.context to avoid a circular import error. +def _init(): + global __salt_loader__, __opts__ + if __salt_loader__ is None: + import salt.loader.context + __salt_loader__ = salt.loader.context.LoaderContext() + __opts__ = __salt_loader__.named_context("__opts__", __opts__) + + +def compat_ver(): + """Gets the Salt version the YAML loader/dumper should try to match + + Occasionally, the behavior of :py:mod:`~salt.utils.yaml` is changed in ways + that can subtly break backwards compatibility. This function returns a + :py:class:`salt.version.SaltStackVersion` object that represents the parsed + and validated value of the ``yaml_compatibility`` configuration option. + That option provides a way for users to force the behavior of the YAML + loader/dumper to match that of a specific version of Salt until they work + out any compatibility issues. + """ + _init() + v = _compat_ver.get(None) + opt = __opts__.get("yaml_compatibility") + # This function is called very early during start-up -- before the config is + # loaded -- so __opts__["yaml_compatibility"] might not be populated yet. + if v is None or opt != _compat_opt.get(None): + _compat_opt.set(opt) + + def _warn(msg, category): + log.warning(msg) + warnings.warn(msg, category) + + # Warn devs to update _current_ver after each release. + actual_current_ver = SaltStackVersion.current_release() + # TODO: This loop can be removed after fixing + # https://github.com/saltstack/salt/issues/62972 + for v in salt.version.SaltVersionsInfo.versions(): + if not v.released: + actual_current_ver = SaltStackVersion(*v.info) + break + if _current_ver < actual_current_ver: + _warn( + f"{__name__}._current_ver is behind ({_current_ver}); please " + f"update it to the current release ({actual_current_ver})", + RuntimeWarning, + ) + + v = _current_ver + if opt: + if isinstance(opt, SaltStackVersion): + v = opt + elif isinstance(opt, int): + v = SaltStackVersion(opt) + else: + v = SaltStackVersion.parse(opt) + opt_supported = True # Whether the value in opt is supported. + for changev, dropv in [ + # List of version pairs. The first version in the pair indicates + # when a change in the default behavior took (or will take) effect. + # The second version in the pair indicates when support for + # yaml_compatibility values less than the first version will be + # dropped. + (3006, 3006), # Versions < 3006 have never been supported. + (3007, 3011), # Support for < 3007 will be dropped in 3011. + ]: + changev = SaltStackVersion(changev) + dropv = SaltStackVersion(dropv) + if v < changev: + if _current_ver >= dropv: + opt_supported = False + v = changev + elif opt: + _warn( + "support for yaml_compatibility option values less " + f"than {changev} will be removed in Salt {dropv}", + FutureWarning, + ) + if not opt and changev > _current_ver and v < changev: + _warn( + "Salt's default YAML processing behavior will change in " + f"version {changev}; to preview the changes set the " + f"yaml_compatibility option to {changev} or higher", + FutureWarning, + ) + if not opt_supported: + # This is a bug in the user's config (or in Salt tests). + _warn( + f"the value of the yaml_compatibility option is less than the " + f"minimum supported value {v}: {opt!r}", + UnsupportedValueWarning, + ) + if opt: + _warn( + f"forcing YAML processing behavior to match Salt version {v} " + "due to yaml_compatibility option", + OverrideNotice, + ) + log.debug(f"YAML compatibility version: {v}") + _compat_ver.set(v) + return v + + +class VersionedSubclassesMixin: + """Automatically adds version-specific subclasses as class attributes. + + For each main type that inherits this mixin, an additional subclass per + supported ``yaml_compatibility`` version range is automatically defined and + saved as an attribute of the main type. Each of these version-specific + subtypes has multiple base classes: the associated main type, and the + version-specific subtypes associated with the main type's base types. For + example, with types Foo◀─Bar◀─Baz and versions 3006 and 3007, the + inheritance DAG will look like this:: + + VersionedSubclassesMixin + ▲ + │ + Foo◀─────╮──────────╮ + ▲ │ │ + │ Foo.V3006 Foo.V3007 + │ ▲ ▲ + Bar◀───╮─│────────╮ │ + ▲ ╰─┤ ╰─┤ + │ Bar.V3006 Bar.V3007 + │ ▲ ▲ + Baz◀───╮─│────────╮ │ + ╰─┤ ╰─┤ + Baz.V3006 Baz.V3007 + + """ + + @classmethod + def __init_subclass__( + cls, *, versioned_properties=(), _versioned_subclass=False, **kwargs + ): + """Automatically adds version-specific subclasses as class attributes. + + :param versioned_properties: + Optional iterable of names that refer to getters decorated by + :py:func:`~salt.utils.decorators.classproperty`. For each name, a + new ``classproperty``-decorated getter is defined that uses the + return value of :py:func:`compat_ver` to automatically select the + getter from the appropriate versioned subclass. + + """ + super().__init_subclass__(**kwargs) + if _versioned_subclass: + return + + for name in versioned_properties: + + def make_getter(name): # Capture the loop value in the closure. + def getter(cls): + # This getter is inherited by the versioned subclasses. + if getattr(cls, "_versioned_subclass_for", None): + return getattr(super(), name) + elif compat_ver() < SaltStackVersion(3007): + return getattr(cls.V3006, name) + else: + return getattr(cls.V3007, name) + + getter.__name__ = name + getter.__qualname__ = f"{cls.__qualname__}.{name}" + return getter + + setattr(cls, name, classproperty(make_getter(name))) + + def add_versioned_subclass(attr): + bases = tuple( + [cls] + + [ + getattr(b, attr) + for b in cls.__bases__ + if ( + issubclass(b, VersionedSubclassesMixin) + and not issubclass(VersionedSubclassesMixin, b) + ) + ] + ) + subcls = type( + f"{cls.__name__}.{attr}", + bases, + {"_versioned_subclass_for": cls}, + # This new type is derived from VersionedSubclassesMixin, so we + # need to prevent infinite recursion (versioned subclasses + # should not have their own nested versioned subclasses). + _versioned_subclass=True, + ) + setattr(cls, attr, subcls) + + add_versioned_subclass("V3006") + add_versioned_subclass("V3007") class InheritMapMixin: diff --git a/salt/utils/args.py b/salt/utils/args.py index 0cbc1592a7df..7aaf7c8b3400 100644 --- a/salt/utils/args.py +++ b/salt/utils/args.py @@ -13,7 +13,6 @@ import salt.utils.data import salt.utils.jid import salt.utils.versions -import salt.utils.yaml from salt.exceptions import SaltInvocationError log = logging.getLogger(__name__) @@ -134,6 +133,7 @@ def yamlify_arg(arg): """ yaml.safe_load the arg """ + import salt.utils.yaml if not isinstance(arg, str): return arg diff --git a/salt/utils/data.py b/salt/utils/data.py index f50ebbab789f..947e5100f226 100644 --- a/salt/utils/data.py +++ b/salt/utils/data.py @@ -16,7 +16,6 @@ import salt.utils.dictupdate import salt.utils.stringutils -import salt.utils.yaml from salt.defaults import DEFAULT_TARGET_DELIM from salt.exceptions import SaltException from salt.utils.decorators.jinja import jinja_filter @@ -1042,6 +1041,7 @@ def repack_dictlist(data, strict=False, recurse=False, key_cb=None, val_cb=None) repacks into a single dictionary. """ if isinstance(data, str): + import salt.utils.yaml try: data = salt.utils.yaml.safe_load(data) except salt.utils.yaml.parser.ParserError as err: diff --git a/salt/utils/decorators/__init__.py b/salt/utils/decorators/__init__.py index 46a17b403798..f8c97413d300 100644 --- a/salt/utils/decorators/__init__.py +++ b/salt/utils/decorators/__init__.py @@ -12,7 +12,6 @@ from collections import defaultdict from functools import wraps -import salt.utils.args import salt.utils.versions from salt.exceptions import ( CommandExecutionError, @@ -116,6 +115,7 @@ class wide dependency_dict @staticmethod def run_command(dependency, mod_name, func_name): + import salt.utils.args full_name = "{}.{}".format(mod_name, func_name) log.trace("Running '%s' for '%s'", dependency, full_name) if IS_WINDOWS: @@ -255,6 +255,7 @@ def timing(function): @wraps(function) def wrapped(*args, **kwargs): + import salt.utils.args start_time = time.time() ret = function(*args, **salt.utils.args.clean_kwargs(**kwargs)) end_time = time.time() diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 88b0758aadf9..b7aeb6c550e4 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -13,6 +13,7 @@ import salt.utils._yaml_common as _yaml_common import salt.utils.context +from salt.utils.decorators import classproperty from salt.utils.odict import OrderedDict try: @@ -35,16 +36,47 @@ class _InheritedRepresentersMixin( _yaml_common.InheritMapMixin, inherit_map_attrs={ - "yaml_representers": "yaml_representers", - "yaml_multi_representers": "yaml_multi_representers", + # The ChainMap used for yaml_representers is not saved directly to + # cls.yaml_representers because _yaml_common.VersionedSubclassesMixin is + # used to automatically switch between versioned variants of the + # ChainMap, and we still need access to the unversioned ChainMap (for + # add_representer(), and for subclasses to chain off of). + "_reps": "yaml_representers", + # Same goes for _mreps. + "_mreps": "yaml_multi_representers", }, +): + @classproperty + def yaml_representers(cls): # pylint: disable=no-self-argument + return cls._reps + + @classproperty + def yaml_multi_representers(cls): # pylint: disable=no-self-argument + return cls._mreps + + @classmethod + def add_representer(cls, data_type, rep): + cls._reps[data_type] = rep + + @classmethod + def add_multi_representer(cls, data_type, rep): + cls._mreps[data_type] = rep + + +class _VersionedRepresentersMixin( + _yaml_common.VersionedSubclassesMixin, + versioned_properties=( + "yaml_implicit_resolvers", + "yaml_multi_representers", + "yaml_representers", + ), ): pass class _CommonMixin( + _VersionedRepresentersMixin, _InheritedRepresentersMixin, - yaml.representer.BaseRepresenter, ): def _rep_ordereddict(self, data): return self.represent_dict(list(data.items())) diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index 8cd07c428f44..551f9597497b 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -9,6 +9,7 @@ import salt.utils._yaml_common as _yaml_common import salt.utils.stringutils +from salt.utils.decorators import classproperty # prefer C bindings over python when available BaseLoader = getattr(yaml, "CSafeLoader", yaml.SafeLoader) @@ -20,15 +21,50 @@ class _InheritedConstructorsMixin( _yaml_common.InheritMapMixin, inherit_map_attrs={ - "yaml_constructors": "yaml_constructors", - "yaml_multi_constructors": "yaml_multi_constructors", + # The ChainMap used for yaml_constructors is not saved directly to + # cls.yaml_constructors because _yaml_common.VersionedSubclassesMixin is + # used to automatically switch between versioned variants of the + # ChainMap, and we still need access to the unversioned ChainMap (for + # add_constructor(), and for subclasses to chain off of). + "_ctors": "yaml_constructors", + # Same goes for _mctors. + "_mctors": "yaml_multi_constructors", }, +): + @classproperty + def yaml_constructors(cls): # pylint: disable=no-self-argument + return cls._ctors + + @classproperty + def yaml_multi_constructors(cls): # pylint: disable=no-self-argument + return cls._mctors + + @classmethod + def add_constructor(cls, tag, constructor): + cls._ctors[tag] = constructor + + @classmethod + def add_multi_constructor(cls, tag_prefix, multi_constructor): + cls._mctors[tag_prefix] = multi_constructor + + +class _VersionedConstructorsMixin( + _yaml_common.VersionedSubclassesMixin, + versioned_properties=( + "yaml_constructors", + "yaml_implicit_resolvers", + "yaml_multi_constructors", + ), ): pass # with code integrated from https://gist.github.com/844388 -class SaltYamlSafeLoader(_InheritedConstructorsMixin, BaseLoader): +class SaltYamlSafeLoader( + _VersionedConstructorsMixin, + _InheritedConstructorsMixin, + BaseLoader, +): """ Create a custom YAML loader that uses the custom constructor. This allows for the YAML loading defaults to be manipulated based on needs within salt diff --git a/tests/conftest.py b/tests/conftest.py index d0672f95d779..5299a786c0ee 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -73,7 +73,12 @@ os.environ["COVERAGE_PROCESS_START"] = str(COVERAGERC_FILE) # Define the pytest plugins we rely on -pytest_plugins = ["tempdir", "helpers_namespace"] +pytest_plugins = [ + "tempdir", + "helpers_namespace", + # Suppress YAML compatibility warnings by default. + "tests.support.pytest.yaml", +] # Define where not to collect tests from collect_ignore = ["setup.py"] diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 8d9ad5b21da7..c9d41ca58471 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -1,14 +1,83 @@ +import re import textwrap +import warnings import pytest import yaml from yaml.constructor import ConstructorError +import salt.utils._yaml_common as _yc import salt.utils.files import salt.utils.yaml as salt_yaml +from salt.version import SaltStackVersion from tests.support.mock import mock_open, patch +@pytest.mark.parametrize( + "yaml_compatibility,want", + [ + (None, SaltStackVersion(3006)), + (3005, SaltStackVersion(3006)), + (3006, SaltStackVersion(3006)), + ("3006", SaltStackVersion(3006)), + ("3006.0", SaltStackVersion(3006)), + ("v3006.0", SaltStackVersion(3006)), + ("sulfur", SaltStackVersion(3006)), + ("Sulfur", SaltStackVersion(3006)), + ("SULFUR", SaltStackVersion(3006)), + (3007, SaltStackVersion(3007)), + ], + indirect=["yaml_compatibility"], +) +def test_compat_ver(yaml_compatibility, want): + with warnings.catch_warnings(): + warnings.filterwarnings( + "ignore", category=_yc.UnsupportedValueWarning, module=_yc.__name__ + ) + got = _yc.compat_ver() + assert got == want + + +@pytest.mark.show_yaml_compatibility_warnings +@pytest.mark.parametrize( + "yaml_compatibility,want", + [ + (None, [(FutureWarning, r"behavior will change in version 3007(?:\.0)?")]), + ( + 3005, + [ + (FutureWarning, r"less than 3007(?:\.0)? will be removed in Salt 3011(?:\.0)?"), + (_yc.UnsupportedValueWarning, r"minimum supported value 3006(?:\.0)?"), + (_yc.OverrideNotice, r"3006(?:\.0)?"), + ], + ), + ( + 3006, + [ + (FutureWarning, r"less than 3007(?:\.0)? will be removed in Salt 3011(?:\.0)?"), + (_yc.OverrideNotice, r"3006(?:\.0)?"), + ], + ), + (3007, [(_yc.OverrideNotice, r"3007(?:\.0)?")]), + ], + indirect=["yaml_compatibility"], +) +def test_compat_ver_warnings(yaml_compatibility, want): + with warnings.catch_warnings(record=True) as got: + _yc.compat_ver() + for category, regexp in want: + found = False + for warn in got: + if warn.category is not category: + continue + if not re.search(regexp, str(warn.message)): + continue + found = True + break + assert found, f"no {category.__name__} warning with message matching {regexp!r}; all warnings: {[warn.message for warn in got]!r}" + assert len(got) == len(want) + + def test_dump(): data = {"foo": "bar"} assert salt_yaml.dump(data) == "{foo: bar}\n" diff --git a/tests/support/pytest/yaml.py b/tests/support/pytest/yaml.py new file mode 100644 index 000000000000..a979e47fe7db --- /dev/null +++ b/tests/support/pytest/yaml.py @@ -0,0 +1,99 @@ +import logging +import warnings + +import pytest + +import salt.utils._yaml_common as _yc +from tests.support.mock import patch + +log = logging.getLogger(__name__) + + +def pytest_configure(config): + config.addinivalue_line("markers", "show_yaml_compatibility_warnings") + + +@pytest.fixture(autouse=True) +def suppress_yaml_compatibility_warnings(request): + """Silence the warnings produced by certain ``yaml_compatibility`` values. + + This is an autouse fixture, so warnings are always suppressed by default + assuming this module is loaded. To un-suppress the warnings, you can either + redefine this fixture to be a no-op: + + .. code-block:: python + + @pytest.fixture + def suppress_yaml_compatibility_warnings(): + pass + + or you can mark your tests with ``show_yaml_compatibility_warnings``: + + .. code-block:: python + + @pytest.mark.show_yaml_compatibility_warnings + @pytest.mark.parametrize("yaml_compatibility", [3006], indirect=True) + def test_yaml_compatibility_warning_3006(yaml_compatibility): + with pytest.warns(FutureWarning): + salt.utils.yaml.load("{}") + + """ + if "show_yaml_compatibility_warnings" in request.keywords: + yield + return + log.debug("suppressing YAML compatibility warnings") + with warnings.catch_warnings(): + # Don't suppress UnsupportedValueWarning in _yc.compat_ver() -- that + # warning means that a test needs to be updated. + for category in [FutureWarning, _yc.OverrideNotice]: + warnings.filterwarnings("ignore", category=category, module=_yc.__name__) + yield + + +@pytest.fixture +def clear_internal_compat_ver_state(): + """Reprocess ``yaml_compatibility`` next time ``compat_ver`` is called. + + This temporarily clears the internal state containing the processed + ``yaml_compatibility`` version, forcing the next ``compat_ver`` call to + reprocess the ``yaml_compatibility`` option. + + """ + # It would be nice if contextvars.copy_context() could be used as a context + # manager, but alas we have to individually back up the context variables + # and restore them later. https://github.com/python/cpython/issues/99633 + opt_tok = _yc._compat_opt.set(None) + ver_tok = _yc._compat_ver.set(None) + yield + _yc._compat_ver.reset(ver_tok) + _yc._compat_opt.reset(opt_tok) + + +@pytest.fixture +def yaml_compatibility( + request, suppress_yaml_compatibility_warnings, clear_internal_compat_ver_state +): + """Temporarily override the ``yaml_compatibility`` option. + + To specify the desired ``yaml_compatibility`` value, use indirect + parametrization: + + .. code-block:: python + + @pytest.mark.parametrize("yaml_compatibility", [3006], indirect=True) + def test_foo(yaml_compatibility): + assert yaml_compatibility == 3006 + want = salt.version.SaltStackVersion(3006) + got = salt.utils._yaml_common.compat_ver() + assert got == want + + If a parameter is not given, ``yaml_compatibility`` behaves as if the + parameter was ``None``. + + """ + v = getattr(request, "param", None) + log.debug(f"setting yaml_compatibility to {v!r}") + _yc._init() # Ensure _yc.__opts__ is initialized before patching it. + with patch.dict(_yc.__opts__, {"yaml_compatibility": v}): + yield v + log.debug(f"restoring yaml_compatibility") From 5f8ad0f73f7b9633c8bff7286104980c342e0883 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 4 Dec 2022 00:02:33 -0500 Subject: [PATCH 19/31] yaml: New `yaml_compatibility_warnings` option to hide warnings --- doc/ref/configuration/master.rst | 17 +++++++++++++++++ doc/ref/configuration/minion.rst | 17 +++++++++++++++++ .../troubleshooting/yaml_idiosyncrasies.rst | 5 +++++ salt/config/__init__.py | 3 +++ salt/utils/_yaml_common.py | 15 ++++++++++++--- 5 files changed, 54 insertions(+), 3 deletions(-) diff --git a/doc/ref/configuration/master.rst b/doc/ref/configuration/master.rst index 2b1a8d8a0f3a..22f7965925e4 100644 --- a/doc/ref/configuration/master.rst +++ b/doc/ref/configuration/master.rst @@ -1216,6 +1216,23 @@ so setting this on the master does not affect how state YAML files are parsed. yaml_compatibility: Sulfur yaml_compatibility: SULFUR +.. conf_master:: yaml_compatibility_warnings + +``yaml_compatibility_warnings`` +------------------------------- + +.. versionadded:: 3006.0 + +Default: ``True`` + +Boolean indicating whether to print warnings about upcoming changes to the YAML +loader/dumper that could affect compatibility. Set to `False` to silence the +warnings. + +.. code-block:: yaml + + yaml_compatibility_warnings: True + .. conf_master:: req_server_niceness ``req_server_niceness`` diff --git a/doc/ref/configuration/minion.rst b/doc/ref/configuration/minion.rst index 6c80518df6f8..593a013e875c 100644 --- a/doc/ref/configuration/minion.rst +++ b/doc/ref/configuration/minion.rst @@ -1727,6 +1727,23 @@ parsed. yaml_compatibility: Sulfur yaml_compatibility: SULFUR +.. conf_minion:: yaml_compatibility_warnings + +``yaml_compatibility_warnings`` +------------------------------- + +.. versionadded:: 3006.0 + +Default: ``True`` + +Boolean indicating whether to print warnings about upcoming changes to the YAML +loader/dumper that could affect compatibility. Set to `False` to silence the +warnings. + +.. code-block:: yaml + + yaml_compatibility_warnings: True + Docker Configuration ==================== diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 6664a4a9b217..2f6d3fad98f5 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -18,6 +18,11 @@ unexpected times. All of the behavior changes slated for Salt v3007.0 can be previewed by setting the ``yaml_compatibility`` option to 3007. +.. versionchanged:: 3006.0 + + YAML compatibility warnings can be disabled by setting the + ``yaml_compatibility_warnings`` option to False. + Spaces vs Tabs ============== diff --git a/salt/config/__init__.py b/salt/config/__init__.py index 2e9532517355..9214f885377a 100644 --- a/salt/config/__init__.py +++ b/salt/config/__init__.py @@ -979,6 +979,7 @@ def _gather_buffer_space(): # pass renderer: Set PASSWORD_STORE_DIR env for Pass "pass_dir": str, "yaml_compatibility": (type(None), str, int), + "yaml_compatibility_warnings": (type(None), bool), } ) @@ -1284,6 +1285,7 @@ def _gather_buffer_space(): "reactor_niceness": None, "fips_mode": False, "yaml_compatibility": None, + "yaml_compatibility_warnings": True, } ) @@ -1628,6 +1630,7 @@ def _gather_buffer_space(): "pass_dir": "", "netapi_enable_clients": [], "yaml_compatibility": None, + "yaml_compatibility_warnings": True, } ) diff --git a/salt/utils/_yaml_common.py b/salt/utils/_yaml_common.py index 6351b8239f6d..c8edb2427e9f 100644 --- a/salt/utils/_yaml_common.py +++ b/salt/utils/_yaml_common.py @@ -10,6 +10,8 @@ __opts__ = { "yaml_compatibility": None, + # Suppress the warnings until the config file has been loaded. + "yaml_compatibility_warnings": False, } __salt_loader__ = None @@ -73,10 +75,14 @@ def compat_ver(): # loaded -- so __opts__["yaml_compatibility"] might not be populated yet. if v is None or opt != _compat_opt.get(None): _compat_opt.set(opt) + show_warnings = __opts__.get("yaml_compatibility_warnings") + if show_warnings is None: + show_warnings = True - def _warn(msg, category): + def _warn(msg, category, *, force_show=False): log.warning(msg) - warnings.warn(msg, category) + if show_warnings or force_show: + warnings.warn(msg, category) # Warn devs to update _current_ver after each release. actual_current_ver = SaltStackVersion.current_release() @@ -91,6 +97,7 @@ def _warn(msg, category): f"{__name__}._current_ver is behind ({_current_ver}); please " f"update it to the current release ({actual_current_ver})", RuntimeWarning, + force_show=True, ) v = _current_ver @@ -131,11 +138,13 @@ def _warn(msg, category): FutureWarning, ) if not opt_supported: - # This is a bug in the user's config (or in Salt tests). _warn( f"the value of the yaml_compatibility option is less than the " f"minimum supported value {v}: {opt!r}", UnsupportedValueWarning, + # This is a bug in the user's config (or in Salt tests), so + # always show the warning. + force_show=True, ) if opt: _warn( From 6c9045ef4f3737627d741719250072f06a0e264b Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 19 Oct 2022 22:33:57 -0400 Subject: [PATCH 20/31] yaml: Default to `OrderedDumper` in `salt.utils.yaml.dump()` I believe this was the original intention. Even if it was not, the symmetry with `salt.utils.yaml.save_dump()` makes `dump()` less surprising now, and it makes it easier to introduce representer changes that affect both `dump()` and `safe_dump()`. --- changelog/62932.fixed | 5 +++++ salt/utils/yamldumper.py | 9 +++++++++ tests/pytests/unit/utils/test_yaml.py | 24 ++++++++++++++++++++++++ 3 files changed, 38 insertions(+) create mode 100644 changelog/62932.fixed diff --git a/changelog/62932.fixed b/changelog/62932.fixed new file mode 100644 index 000000000000..8c83e5eb6e55 --- /dev/null +++ b/changelog/62932.fixed @@ -0,0 +1,5 @@ +Improvements to YAML processing (`salt.utils.yaml`) that are expected to take +effect in Salt 3007 but can be previewed now by setting the option +`yaml_compatibility` to `3007`: + * `salt.utils.yaml.dump()` will default to `salt.utils.yaml.OrderedDumper` + instead of `yaml.Dumper`. diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index b7aeb6c550e4..8079c34dc374 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -15,6 +15,7 @@ import salt.utils.context from salt.utils.decorators import classproperty from salt.utils.odict import OrderedDict +from salt.version import SaltStackVersion try: from yaml import CDumper as Dumper @@ -140,11 +141,19 @@ def dump(data, stream=None, **kwargs): """ .. versionadded:: 2018.3.0 + .. versionchanged:: 3007.0 + + The default ``Dumper`` class is now ``OrderedDumper`` instead of + ``yaml.Dumper``. Set the ``yaml_compatibility`` option to "3006" to + revert to the previous behavior. + Helper that wraps yaml.dump and ensures that we encode unicode strings unless explicitly told not to. """ kwargs.setdefault("allow_unicode", True) kwargs.setdefault("default_flow_style", None) + if "Dumper" not in kwargs and _yaml_common.compat_ver() >= SaltStackVersion(3007): + kwargs["Dumper"] = OrderedDumper return yaml.dump(data, stream, **kwargs) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index c9d41ca58471..5e9bdcbba7b2 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -90,6 +90,30 @@ def test_safe_dump(): assert salt_yaml.safe_dump(data, default_flow_style=False) == "foo: bar\n" +@pytest.mark.parametrize( + "yaml_compatibility,input_dumper,want_dumper", + [ + (3006, None, None), + (3006, yaml.Dumper, yaml.Dumper), + (3007, None, salt_yaml.OrderedDumper), + (3007, yaml.Dumper, yaml.Dumper), + ], + indirect=["yaml_compatibility"], +) +def test_dump_default_dumper(yaml_compatibility, input_dumper, want_dumper): + with patch.object(yaml, "dump") as mock: + kwargs = {} + if input_dumper is not None: + kwargs["Dumper"] = input_dumper + salt_yaml.dump([], **kwargs) + mock.assert_called_once() + got_kwargs = mock.mock_calls[0].kwargs + if want_dumper is None: + assert "Dumper" not in got_kwargs + else: + assert got_kwargs["Dumper"] is want_dumper + + def render_yaml(data): """ Takes a YAML string, puts it into a mock file, passes that to the YAML From 9a7703076344bc4b23ee324ba2f78960bb0dc9ab Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 17 Oct 2022 00:51:55 -0400 Subject: [PATCH 21/31] yaml: Fix IndentedSafeOrderedDumper indentation Before, the indentation would only be increased if the compiled module `yaml.CSafeDumper` did not exist. Now it is indented even if it does. Behavior before (assuming `yaml.CSafeDumper` exists): ```pycon >>> import salt.utils.yaml as y >>> print(y.dump({"foo": ["bar"]}, Dumper=y.IndentedSafeOrderedDumper, default_flow_style=False)) foo: - bar ``` Behavior after: ```pycon >>> import salt.utils.yaml as y >>> print(y.dump({"foo": ["bar"]}, Dumper=y.IndentedSafeOrderedDumper, default_flow_style=False)) foo: - bar ``` --- changelog/62932.fixed | 2 ++ salt/utils/yamldumper.py | 41 ++++++++++++++++++++++++++- tests/pytests/unit/utils/test_yaml.py | 23 +++++++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index 8c83e5eb6e55..732325f15019 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -3,3 +3,5 @@ effect in Salt 3007 but can be previewed now by setting the option `yaml_compatibility` to `3007`: * `salt.utils.yaml.dump()` will default to `salt.utils.yaml.OrderedDumper` instead of `yaml.Dumper`. + * Dumping a YAML sequence with `salt.utils.yaml.IndentedSafeOrderedDumper` + will be properly indented. diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 8079c34dc374..7aa4d0234b02 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -122,12 +122,51 @@ class SafeOrderedDumper(_CommonMixin, SafeDumper): """ -class IndentedSafeOrderedDumper(SafeOrderedDumper): +# This must inherit from yaml.SafeDumper, not yaml.CSafeDumper, because the +# increase_indent hack doesn't work with yaml.CSafeDumper. +# https://github.com/yaml/pyyaml/issues/234#issuecomment-786026671 +class IndentedSafeOrderedDumper(_CommonMixin, yaml.SafeDumper): """Like ``SafeOrderedDumper``, except it indents lists for readability.""" def increase_indent(self, flow=False, indentless=False): return super().increase_indent(flow, False) + # TODO: Everything below this point is to provide backwards compatibility. + # It can be removed once support for yaml_compatibility=3006 is dropped. + + class _Legacy(SafeOrderedDumper): + def increase_indent(self, flow=False, indentless=False): + return super().increase_indent(flow, False) + + def __init__(self, *args, **kwargs): + super().__setattr__("_in_init", True) + try: + super().__init__(*args, **kwargs) + Legacy = super().__getattribute__("_Legacy") + super().__setattr__("_legacy", Legacy(*args, **kwargs)) + finally: + super().__setattr__("_in_init", False) + + def _use_legacy(self): + if super().__getattribute__("_in_init"): + return False + return _yaml_common.compat_ver() < SaltStackVersion(3007) + + def __getattribute__(self, name): + if super().__getattribute__("_use_legacy")(): + return getattr(super().__getattribute__("_legacy"), name) + return super().__getattribute__(name) + + def __setattr__(self, name, value): + if super().__getattribute__("_use_legacy")(): + return setattr(super().__getattribute__("_legacy"), name, value) + return super().__setattr__(name, value) + + def __delattr__(self, name): + if super().__getattribute__("_use_legacy")(): + return delattr(super().__getattribute__("_legacy"), name) + return super().__delattr__(name) + def get_dumper(dumper_name): return { diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 5e9bdcbba7b2..c6384eb73f6f 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -114,6 +114,29 @@ def test_dump_default_dumper(yaml_compatibility, input_dumper, want_dumper): assert got_kwargs["Dumper"] is want_dumper +@pytest.mark.parametrize( + "yaml_compatibility,want", + [ + # With v3006, sometimes it indents and sometimes it doesn't depending on + # whether yaml.CSafeDumper exists on the system. + (3006, re.compile(r"foo:\n(?: )?- bar\n")), + (3007, "foo:\n - bar\n"), + ], + indirect=["yaml_compatibility"], +) +def test_dump_indented(yaml_compatibility, want): + data = {"foo": ["bar"]} + got = salt_yaml.dump( + data, + Dumper=salt_yaml.IndentedSafeOrderedDumper, + default_flow_style=False, + ) + try: + assert want.fullmatch(got) + except AttributeError: + assert got == want + + def render_yaml(data): """ Takes a YAML string, puts it into a mock file, passes that to the YAML From 147cfb425849f9580985b8a7ccecefec36e71bb4 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 29 Oct 2022 01:45:00 -0400 Subject: [PATCH 22/31] yaml: Fix custom YAML to object constructor registration The `SaltYamlSafeLoader.add_constructor()` method is a class method, not an instance method. Therefore, any call to that method in an instance method (such as `__init__`) affects all future instances, not just the current instance (`self`). That's not a problem if the registration calls simply re-register the same constructor functions over and over, but that was not the case here: different functions were registered depending on the value of the `dictclass` parameter to `__init__`. The net effect was that an `!!omap` node was correctly processed as a sequence of single-entry maps (and a list of (key, value) tuples returned) until the first time a SaltYamlSafeLoader was constructed with a non-`dict` class. After that, every `!!omap` node was always incorrectly processed as a map node regardless of `dictclass`. Now `!!omap` processing is performed as originally intended: * If the `dictclass` parameter is `dict` (the default), the behavior when loading a `!!map` or `!!omap` node is unchanged from the base class's behavior. * If the `dictclass` parameter is not `dict`: * `!!map` nodes are loaded like they are in the base class except the custom class is used instead of `dict`. * `!!omap` nodes are loaded the same as `!!map` nodes. (This is a bug because an `!!omap` node is a sequence node of single-valued map nodes, not an ordinary map node. A future commit will fix this.) Behavior before: ```pycon >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] >>> y.SaltYamlSafeLoader("", dictclass=collections.OrderedDict) # created for side-effect only >>> y.load("!!omap [{foo: bar}, {baz: bif}]") # exact same as before Traceback (most recent call last): File "", line 1, in File "salt/utils/yamlloader.py", line 159, in load return yaml.load(stream, Loader=Loader) File "venv/lib/python3.8/site-packages/yaml/__init__.py", line 81, in load return loader.get_single_data() File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 51, in get_single_data return self.construct_document(node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 60, in construct_document for dummy in generator: File "salt/utils/yamlloader.py", line 45, in construct_yaml_map value = self.construct_mapping(node) File "salt/utils/yamlloader.py", line 56, in construct_mapping raise ConstructorError( yaml.constructor.ConstructorError: expected a mapping node, but found sequence in "", line 1, column 1 ``` Behavior after: ```pycon >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] >>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data() Traceback (most recent call last): File "", line 1, in File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 51, in get_single_data return self.construct_document(node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 60, in construct_document for dummy in generator: File "salt/utils/yamlloader.py", line 42, in construct_yaml_omap return (yield from self.construct_yaml_map(node)) File "salt/utils/yamlloader.py", line 36, in construct_yaml_map value = self.construct_mapping(node) File "salt/utils/yamlloader.py", line 52, in construct_mapping raise ConstructorError( yaml.constructor.ConstructorError: expected a mapping node, but found sequence in "", line 1, column 1 >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] ``` This commit also adds a unit test for the `dictclass` parameter, though it's important to note that the new test is not a regression test for the bug fixed by this commit. (I don't think it would be worthwhile to write a regression test because the test code would be complicated and unreadable.) --- changelog/62932.fixed | 11 +++++--- salt/utils/yamlloader.py | 38 +++++++++++++++++++-------- tests/pytests/unit/utils/test_yaml.py | 12 +++++++++ 3 files changed, 47 insertions(+), 14 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index 732325f15019..d9e730a9f25e 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -1,6 +1,11 @@ -Improvements to YAML processing (`salt.utils.yaml`) that are expected to take -effect in Salt 3007 but can be previewed now by setting the option -`yaml_compatibility` to `3007`: +Improvements to YAML processing (`salt.utils.yaml`) that take effect +immediately: + * Passing a non-`dict` class to the `salt.utils.yaml.SaltYamlSafeLoader` + constructor no longer causes all future `!!omap` nodes to raise an exception + when loaded. + +Improvements to YAML processing that are expected to take effect in Salt 3007 +but can be previewed now by setting the option `yaml_compatibility` to `3007`: * `salt.utils.yaml.dump()` will default to `salt.utils.yaml.OrderedDumper` instead of `yaml.Dumper`. * Dumping a YAML sequence with `salt.utils.yaml.IndentedSafeOrderedDumper` diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index 551f9597497b..d31083331558 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -73,25 +73,21 @@ class SaltYamlSafeLoader( def __init__(self, stream, dictclass=dict): super().__init__(stream) - if dictclass is not dict: - # then assume ordered dict and use it for both !map and !omap - self.add_constructor("tag:yaml.org,2002:map", type(self).construct_yaml_map) - self.add_constructor( - "tag:yaml.org,2002:omap", type(self).construct_yaml_map - ) - self.add_constructor("tag:yaml.org,2002:str", type(self).construct_yaml_str) - self.add_constructor( - "tag:yaml.org,2002:python/unicode", type(self).construct_unicode - ) - self.add_constructor("tag:yaml.org,2002:timestamp", type(self).construct_scalar) self.dictclass = dictclass def construct_yaml_map(self, node): + if self.dictclass is dict: + return (yield from super().construct_yaml_map(node)) data = self.dictclass() yield data value = self.construct_mapping(node) data.update(value) + def construct_yaml_omap(self, node): + if self.dictclass is dict: + return (yield from super().construct_yaml_omap(node)) + return (yield from self.construct_yaml_map(node)) + def construct_unicode(self, node): return node.value @@ -202,6 +198,26 @@ def flatten_mapping(self, node): node.value = mergeable_items + node.value +# The add_constructor() method is a class method, not an instance method, so +# custom constructors should be registered at class creation time, not instance +# creation time. +SaltYamlSafeLoader.add_constructor( + "tag:yaml.org,2002:map", SaltYamlSafeLoader.construct_yaml_map +) +SaltYamlSafeLoader.add_constructor( + "tag:yaml.org,2002:omap", SaltYamlSafeLoader.construct_yaml_omap +) +SaltYamlSafeLoader.add_constructor( + "tag:yaml.org,2002:python/unicode", SaltYamlSafeLoader.construct_unicode +) +SaltYamlSafeLoader.add_constructor( + "tag:yaml.org,2002:str", SaltYamlSafeLoader.construct_yaml_str +) +SaltYamlSafeLoader.add_constructor( + "tag:yaml.org,2002:timestamp", SaltYamlSafeLoader.construct_scalar +) + + def load(stream, Loader=SaltYamlSafeLoader): return yaml.load(stream, Loader=Loader) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index c6384eb73f6f..1e0679f41afd 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -1,3 +1,4 @@ +import collections import re import textwrap import warnings @@ -266,6 +267,17 @@ def test_load_with_plain_scalars(): ) +@pytest.mark.parametrize("dictclass", [dict, collections.OrderedDict]) +def test_load_dictclass(dictclass): + l = salt_yaml.SaltYamlSafeLoader("k1: v1\nk2: v2\n", dictclass=dictclass) + try: + d = l.get_single_data() + finally: + l.dispose() + assert isinstance(d, dictclass) + assert d == dictclass([("k1", "v1"), ("k2", "v2")]) + + def test_not_yaml_monkey_patching(): if hasattr(yaml, "CSafeLoader"): assert yaml.SafeLoader != yaml.CSafeLoader From 61f63fab90280d6f9b2015b10755d0e75c3feae0 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 29 Oct 2022 02:00:55 -0400 Subject: [PATCH 23/31] yaml: Load `!!timestamp` nodes as `datetime.datetime` objects YAML nodes tagged with `!!timestamp` nodes now become Python `datetime` objects rather than strings. To preserve compatibility with existing YAML files, timestamp-like strings without `!!timestamp` are still loaded as strings. Behavior before: ```pycon >>> import datetime >>> import salt.utils.yaml as y >>> y.load("'2022-10-21T18:16:03.1-04:00'") '2022-10-21T18:16:03.1-04:00' >>> y.load("!!timestamp '2022-10-21T18:16:03.1-04:00'") '2022-10-21T18:16:03.1-04:00' ``` Behavior after: ```pycon >>> import datetime >>> import salt.utils.yaml as y >>> y.load("'2022-10-21T18:16:03.1-04:00'") '2022-10-21T18:16:03.1-04:00' >>> y.load("!!timestamp '2022-10-21T18:16:03.1-04:00'") datetime.datetime(2022, 10, 21, 18, 16, 3, 100000, tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=72000))) ``` --- changelog/62932.fixed | 2 ++ .../troubleshooting/yaml_idiosyncrasies.rst | 25 ++++++++++++----- salt/utils/yamlloader.py | 14 +++++++++- tests/pytests/unit/utils/test_yaml.py | 27 +++++++++++++++++++ 4 files changed, 61 insertions(+), 7 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index d9e730a9f25e..b64831fb65b5 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -6,6 +6,8 @@ immediately: Improvements to YAML processing that are expected to take effect in Salt 3007 but can be previewed now by setting the option `yaml_compatibility` to `3007`: + * Loading an explicitly tagged `!!timestamp` node will produce a + `datetime.datetime` object instead of a string. * `salt.utils.yaml.dump()` will default to `salt.utils.yaml.OrderedDumper` instead of `yaml.Dumper`. * Dumping a YAML sequence with `salt.utils.yaml.IndentedSafeOrderedDumper` diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 2f6d3fad98f5..fc457c660846 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -399,20 +399,33 @@ Automatic ``datetime`` conversion object, even if the node contained an invalid date (for example, ``4017-16-20``). -Salt overrides PyYAML's default behavior and always loads YAML nodes that look -like timestamps (including nodes explicitly tagged with ``!!timestamp``) as -strings: +.. versionchanged:: 3007.0 + + Loading a YAML ``!!timestamp`` node now produces a ``datetime.datetime`` + object. Previously, nodes tagged with ``!!timestamp`` produced strings. + Set the ``yaml_compatibility`` option to 3006 to revert to the previous + behavior. + +Salt overrides PyYAML's default behavior and loads YAML nodes that look like +timestamps as strings: .. code-block:: pycon >>> import salt.utils.yaml >>> salt.utils.yaml.safe_load("2014-01-20 14:23:23") '2014-01-20 14:23:23' + +To force Salt to produce a ``datetime.datetime`` object instead of a string, +explicitly tag the node with ``!!timestamp``: + +.. code-block:: pycon + + >>> import salt.utils.yaml >>> salt.utils.yaml.safe_load("!!timestamp 2014-01-20 14:23:23") - '2014-01-20 14:23:23' + datetime.datetime(2014, 1, 20, 14, 23, 23) -There is currently no way to force Salt to produce a Python -``datetime.datetime`` object from a timestamp in a YAML file. +Beware that Salt is currently unable to serialize ``datetime.datetime`` objects, +so ``!!timestamp`` nodes cannot be used in pillar SLS files. Ordered Dictionaries ==================== diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index d31083331558..0b6fb08c7017 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -71,6 +71,14 @@ class SaltYamlSafeLoader( to make things like sls file more intuitive. """ + @classmethod + def remove_implicit_resolver(cls, tag): + """Remove a previously registered implicit resolver for a tag.""" + cls.yaml_implicit_resolvers = { + first_char: [r for r in resolver_list if r[0] != tag] + for first_char, resolver_list in cls.yaml_implicit_resolvers.items() + } + def __init__(self, stream, dictclass=dict): super().__init__(stream) self.dictclass = dictclass @@ -213,10 +221,14 @@ def flatten_mapping(self, node): SaltYamlSafeLoader.add_constructor( "tag:yaml.org,2002:str", SaltYamlSafeLoader.construct_yaml_str ) -SaltYamlSafeLoader.add_constructor( +SaltYamlSafeLoader.V3006.add_constructor( "tag:yaml.org,2002:timestamp", SaltYamlSafeLoader.construct_scalar ) +# Require users to explicitly provide the `!!timestamp` tag if a datetime object +# is desired. +SaltYamlSafeLoader.V3007.remove_implicit_resolver("tag:yaml.org,2002:timestamp") + def load(stream, Loader=SaltYamlSafeLoader): return yaml.load(stream, Loader=Loader) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 1e0679f41afd..45a4a2f6132a 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -1,4 +1,5 @@ import collections +import datetime import re import textwrap import warnings @@ -278,6 +279,32 @@ def test_load_dictclass(dictclass): assert d == dictclass([("k1", "v1"), ("k2", "v2")]) +@pytest.mark.parametrize( + "yaml_compatibility,input_yaml,want", + [ + ( + 3006, + "!!timestamp 2022-10-21T18:16:03.1-04:00", + "2022-10-21T18:16:03.1-04:00", + ), + ( + 3007, + "!!timestamp 2022-10-21T18:16:03.1-04:00", + datetime.datetime( + *(2022, 10, 21, 18, 16, 3, 100000), + tzinfo=datetime.timezone(datetime.timedelta(hours=-4)), + ), + ), + (3006, "2022-10-21T18:16:03.1-04:00", "2022-10-21T18:16:03.1-04:00"), + (3007, "2022-10-21T18:16:03.1-04:00", "2022-10-21T18:16:03.1-04:00"), + ], + indirect=["yaml_compatibility"], +) +def test_load_timestamp(yaml_compatibility, input_yaml, want): + got = salt_yaml.load(input_yaml) + assert got == want + + def test_not_yaml_monkey_patching(): if hasattr(yaml, "CSafeLoader"): assert yaml.SafeLoader != yaml.CSafeLoader From 56e01d02095dacfa9474719aa711ab772ad18fda Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Tue, 1 Nov 2022 03:04:07 -0400 Subject: [PATCH 24/31] yaml: Load `!!omap` nodes as sequences of mappings Before, `!!omap` nodes were incorrectly assumed to be mapping nodes when `dictclass` was not `dict`. Now they are correctly processed as sequences of single-entry mappings. The resulting Python object has type `dictclass` (usually `collections.OrderedDict` or a subclass). This commit does not change the behavior when `dictclass` is `dict`; loading an `!!omap` node still returns a list of (key, value) tuples (PyYAML's default behavior). I consider that to be a bug in PyYAML, so a future commit may change the behavior in the `dict` case to match the non-`dict` behavior. (This commit uses `dictclass` for the non-`dict` case to match what appears to be the original intention.) Behavior before: ```pycon >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] >>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data() Traceback (most recent call last): File "", line 1, in File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 51, in get_single_data return self.construct_document(node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 60, in construct_document for dummy in generator: File "salt/utils/yamlloader.py", line 42, in construct_yaml_omap return (yield from self.construct_yaml_map(node)) File "salt/utils/yamlloader.py", line 36, in construct_yaml_map value = self.construct_mapping(node) File "salt/utils/yamlloader.py", line 52, in construct_mapping raise ConstructorError( yaml.constructor.ConstructorError: expected a mapping node, but found sequence in "", line 1, column 1 ``` Behavior after: ```pycon >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] >>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data() OrderedDict([('foo', 'bar'), ('baz', 'bif')]) ``` Relevant bug: https://github.com/saltstack/salt/issues/12161 --- changelog/62932.fixed | 6 + salt/utils/yamlloader.py | 29 +++- .../pillar/test_pillar_map_order.py | 61 ++++++- tests/pytests/unit/utils/test_yaml.py | 152 ++++++++++++++++++ 4 files changed, 240 insertions(+), 8 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index b64831fb65b5..3b67108a65d8 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -6,6 +6,12 @@ immediately: Improvements to YAML processing that are expected to take effect in Salt 3007 but can be previewed now by setting the option `yaml_compatibility` to `3007`: + * Loading a well-formed `!!omap` node (a sequence of single-entry mapping + nodes) with a `salt.utils.yaml.SaltYamlSafeLoader` that was constructed with + a non-`dict` mapping class will produce an object of that mapping type + instead of raise an exception. Mapping nodes tagged with `!!omap` will + raise an exception due to invalid syntax (currently such nodes produce a + mapping object of the given type even though the syntax is invalid). * Loading an explicitly tagged `!!timestamp` node will produce a `datetime.datetime` object instead of a string. * `salt.utils.yaml.dump()` will default to `salt.utils.yaml.OrderedDumper` diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index 0b6fb08c7017..4382c4face4c 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -91,11 +91,35 @@ def construct_yaml_map(self, node): value = self.construct_mapping(node) data.update(value) - def construct_yaml_omap(self, node): + def construct_yaml_omap_3006(self, node): if self.dictclass is dict: return (yield from super().construct_yaml_omap(node)) return (yield from self.construct_yaml_map(node)) + def construct_yaml_omap(self, node): + if self.dictclass is dict: + return (yield from super().construct_yaml_omap(node)) + # BaseLoader.construct_yaml_omap() returns a list of (key, value) + # tuples, which doesn't match the semantics of the `!!omap` YAML type. + # Convert the list of tuples to an OrderedDict. + d = self.dictclass() + yield d + (entries,) = super().construct_yaml_omap(node) + # All of the following lines could be replaced with `d.update(entries)`, + # but we want to detect and reject any duplicate keys in `entries`. + if hasattr(entries, "keys"): + entries = ((k, entries[k]) for k in entries.keys()) + for k, v in entries: + if k in d: + raise ConstructorError( + f"while constructing an ordered map", + node.start_mark, + f"duplicate key encountered: {k!r}", + # TODO: Can we get the location of the duplicate key? + node.start_mark, + ) + d[k] = v + def construct_unicode(self, node): return node.value @@ -215,6 +239,9 @@ def flatten_mapping(self, node): SaltYamlSafeLoader.add_constructor( "tag:yaml.org,2002:omap", SaltYamlSafeLoader.construct_yaml_omap ) +SaltYamlSafeLoader.V3006.add_constructor( + "tag:yaml.org,2002:omap", SaltYamlSafeLoader.construct_yaml_omap_3006 +) SaltYamlSafeLoader.add_constructor( "tag:yaml.org,2002:python/unicode", SaltYamlSafeLoader.construct_unicode ) diff --git a/tests/pytests/integration/pillar/test_pillar_map_order.py b/tests/pytests/integration/pillar/test_pillar_map_order.py index 42224abc7e1c..b3ef96163387 100644 --- a/tests/pytests/integration/pillar/test_pillar_map_order.py +++ b/tests/pytests/integration/pillar/test_pillar_map_order.py @@ -7,6 +7,36 @@ pytest.mark.slow_test, ] +# TODO: Remove the salt_master, salt_cli, and salt_minion fixtures below after +# 3006 is released to revert to the same-named fixtures provided in conftest.py. +# (Leaving these won't hurt, but it will no longer be necessary). + + +@pytest.fixture(scope="module") +def salt_master(salt_factories): + m = salt_factories.salt_master_daemon( + "master-test_pillar_map_order", + overrides={"yaml_compatibility": 3007}, + ) + with m.started(): + yield m + + +@pytest.fixture(scope="module") +def salt_cli(salt_master): + assert salt_master.is_running() + return salt_master.salt_cli() + + +@pytest.fixture(scope="module") +def salt_minion(salt_master): + m = salt_master.salt_minion_daemon( + "minion-test_pillar_map_order", + overrides={"yaml_compatibility": 3007}, + ) + with m.started(): + yield m + @pytest.fixture(scope="module") def minion_run(salt_minion, salt_cli): @@ -20,13 +50,15 @@ def _run(*args, minion_tgt=salt_minion.id, **kwargs): yield _run -def test_pillar_map_order(salt_master, minion_run): +@pytest.mark.parametrize("omap", [False, True]) +def test_pillar_map_order(salt_master, minion_run, omap, yaml_compatibility): """Test iteration order of YAML map entries in a Pillar ``.sls`` file. - This test generates a Pillar ``.sls`` file containing an ordinary YAML map - and tests whether the resulting Python object preserves iteration order. - Random keys are used to ensure that iteration order does not coincidentally - match. The generated Pillar YAML file looks like this: + This test generates a Pillar ``.sls`` file containing either an ordinary + YAML map or a YAML `!!omap` and tests whether the resulting Python object + preserves iteration order. Random keys are used to ensure that iteration + order does not coincidentally match. Depending on the `omap` parameter, the + generated Pillar YAML file looks like this: .. code-block:: yaml @@ -37,6 +69,17 @@ def test_pillar_map_order(salt_master, minion_run): # ... omitted for brevity ... k1638299831: 19 + or like this: + + .. code-block:: yaml + + data: !!omap + - k3334244338: 0 + - k3444116829: 1 + - k2072366017: 2 + # ... omitted for brevity ... + - k1638299831: 19 + A jinja template iterates over the entries in the resulting object to ensure that iteration order is preserved. The expected output looks like: @@ -59,7 +102,7 @@ def test_pillar_map_order(salt_master, minion_run): Thus, this test may fail on Python 3.5 and older. However, Salt currently requires a newer version of Python, so this should not be a problem. - This is a regression test for: + The non-``!!omap`` case is a regression test for: https://github.com/saltstack/salt/issues/12161 """ # Filter the random keys through a set to avoid duplicates. @@ -69,7 +112,11 @@ def test_pillar_map_order(salt_master, minion_run): items = [(k, i) for i, k in enumerate(keys)] top_yaml = "base: {'*': [data]}\n" top_sls = salt_master.pillar_tree.base.temp_file("top.sls", top_yaml) - data_yaml = "data:\n" + "".join(f" {k}: {v}\n" for k, v in items) + data_yaml = "data:" + if omap: + data_yaml += " !!omap\n" + "".join(f" - {k}: {v}\n" for k, v in items) + else: + data_yaml += "\n" + "".join(f" {k}: {v}\n" for k, v in items) data_sls = salt_master.pillar_tree.base.temp_file("data.sls", data_yaml) tmpl_jinja = textwrap.dedent( """\ diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 45a4a2f6132a..4b07005f532e 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -1,5 +1,6 @@ import collections import datetime +import random import re import textwrap import warnings @@ -15,6 +16,11 @@ from tests.support.mock import mock_open, patch +class _OrderedDictLoader(salt_yaml.SaltYamlSafeLoader): + def __init__(self, stream): + super().__init__(stream, dictclass=collections.OrderedDict) + + @pytest.mark.parametrize( "yaml_compatibility,want", [ @@ -279,6 +285,152 @@ def test_load_dictclass(dictclass): assert d == dictclass([("k1", "v1"), ("k2", "v2")]) +@pytest.mark.parametrize( + # Parameters: + # - yaml_compatibility: Force the YAML loader to be compatible with this + # version of Salt. + # - seq_input: Boolean. True if the input YAML node should be a sequence + # of single-entry mappings, False if it should be a mapping. + # - Loader: YAML Loader class. + # - wantclass: Expected return type. + "yaml_compatibility,seq_input,Loader,wantclass", + [ + # Salt v3006 and earlier required !!omap nodes to be mapping nodes if + # the SaltYamlSafeLoader dictclass argument is not dict. To preserve + # compatibility, that erroneous behavior is preserved if + # yaml_compatibility is set to 3006. + (3006, False, _OrderedDictLoader, collections.OrderedDict), + # However, with dictclass=dict (the default), an !!omap node was + # correctly required to be a sequence of mapping nodes. Unfortunately, + # the return value was not a Mapping type -- it was a list of (key, + # value) tuples (PyYAML's default behavior for !!omap nodes). + (3006, True, None, list), + # Starting with Salt v3007, an !!omap node is always required to be a + # sequence of mapping nodes. + (3007, True, _OrderedDictLoader, collections.OrderedDict), + # Unfortunately, the return value is still a list of (key, value) + # tuples when dictclass=dict. + (3007, True, None, list), + ], + indirect=["yaml_compatibility"], +) +def test_load_omap(yaml_compatibility, seq_input, Loader, wantclass): + """Test loading of `!!omap` YAML nodes. + + This test uses random keys to ensure that iteration order does not + coincidentally match. The generated items look like this: + + .. code-block:: python + + [ + ("k3334244338", 0), + ("k3444116829", 1), + ("k2072366017", 2), + # ... omitted for brevity ... + ("k1638299831", 19), + ] + """ + # Filter the random keys through a set to avoid duplicates. + keys = list({f"k{random.getrandbits(32)}" for _ in range(20)}) + # Avoid unintended correlation with set()'s iteration order. + random.shuffle(keys) + items = [(k, i) for i, k in enumerate(keys)] + input_yaml = "!!omap\n" + if seq_input: + input_yaml += "".join(f"- {k}: {v}\n" for k, v in items) + else: + input_yaml += "".join(f"{k}: {v}\n" for k, v in items) + kwargs = {} + if Loader is not None: + kwargs["Loader"] = Loader + got = salt_yaml.load(input_yaml, **kwargs) + assert isinstance(got, wantclass) + if isinstance(got, list): + assert got == items + else: + assert got == collections.OrderedDict(items) + assert list(got.items()) == items + + +@pytest.mark.parametrize( + "yaml_compatibility,input_yaml,Loader,want", + [ + # See comments in test_load_omap() above for the differences in !!omap + # loading behavior between Salt v3006 and v3007. + (3006, "!!omap {}\n", _OrderedDictLoader, collections.OrderedDict()), + (3006, "!!omap []\n", None, []), + (3007, "!!omap []\n", _OrderedDictLoader, collections.OrderedDict()), + (3007, "!!omap []\n", None, []), + ], + indirect=["yaml_compatibility"], +) +def test_load_omap_empty(yaml_compatibility, input_yaml, Loader, want): + kwargs = {} + if Loader is not None: + kwargs["Loader"] = Loader + got = salt_yaml.load(input_yaml, **kwargs) + assert isinstance(got, type(want)) + assert got == want + + +@pytest.mark.parametrize( + "yaml_compatibility,input_yaml,Loader", + [ + # Buggy v3006 behavior kept for compatibility. See comments in + # test_load_omap() above for details. + (3006, "!!omap []\n", _OrderedDictLoader), # Not a mapping node. + (3006, "!!omap\ndup key: 0\ndup key: 1\n", _OrderedDictLoader), + # Invald because the !!omap node is not a sequence node. + (3006, "!!omap {}\n", None), + (3007, "!!omap {}\n", _OrderedDictLoader), + (3007, "!!omap {}\n", None), + # Invalid because a sequence entry is not a mapping node. + (3006, "!!omap\n- this is a str not a map\n", None), + (3007, "!!omap\n- this is a str not a map\n", _OrderedDictLoader), + (3007, "!!omap\n- this is a str not a map\n", None), + # Invalid because a sequence entry's mapping has multiple entries. + (3006, "!!omap\n- k1: v\n k2: v\n", None), + (3007, "!!omap\n- k1: v\n k2: v\n", _OrderedDictLoader), + (3007, "!!omap\n- k1: v\n k2: v\n", None), + # Invalid because a sequence entry's mapping has no entries. + (3006, "!!omap [{}]\n", None), + (3007, "!!omap [{}]\n", _OrderedDictLoader), + (3007, "!!omap [{}]\n", None), + # Invalid because there are duplicate keys. Note that the Loader=None + # cases for v3006 and v3007 are missing here; this is because the + # default Loader matches PyYAML's behavior, and PyYAML permits duplicate + # keys in !!omap nodes. + (3007, "!!omap\n- dup key: 0\n- dup key: 1\n", _OrderedDictLoader), + ], + indirect=["yaml_compatibility"], +) +def test_load_omap_invalid(yaml_compatibility, input_yaml, Loader): + kwargs = {} + if Loader is not None: + kwargs["Loader"] = Loader + with pytest.raises(ConstructorError): + salt_yaml.load(input_yaml, **kwargs) + + +@pytest.mark.parametrize("yaml_compatibility", [3006, 3007], indirect=True) +@pytest.mark.parametrize("Loader", [_OrderedDictLoader, None]) +def test_load_untagged_omaplike_is_seq(yaml_compatibility, Loader): + # The YAML spec allows the loader to interpret something that looks like an + # !!omap but doesn't actually have an !!omap tag as an !!omap. (If the user + # intends to express a sequence of single-entry maps and not an ordered map, + # the user must explicitly tag the sequence node with !seq.) Out of concern + # for backwards compatibility, and to avoid ambiguity with an empty + # sequence, implicit !!omap behavior is currently not supported. That may + # change in the future, but for now make sure that sequences are not + # interpreted as ordered maps. + kwargs = {} + if Loader is not None: + kwargs["Loader"] = Loader + got = salt_yaml.load("- a: 0\n- b: 1\n", **kwargs) + assert not isinstance(got, collections.OrderedDict) + assert got == [{"a": 0}, {"b": 1}] + + @pytest.mark.parametrize( "yaml_compatibility,input_yaml,want", [ From 0cc3208edb23ae26d1978aed7e26eadf88713ddd Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Tue, 1 Nov 2022 21:16:18 -0400 Subject: [PATCH 25/31] yaml: Load `!!omap` nodes as `collections.OrderedDict` objects Now the behavior is consistent regardless of the value of the Loader's `dictclass` constructor parameter. Starting with Python 3.6, Python `dict` objects always iterate in insertion order, so iteration order was already guaranteed when using an ordinary YAML map. However, `!!omap` provides a stronger guarantee to users (plain map iteration order can be thought of as a Salt implementation detail that might change in the future), and it allows them to make their `.sls` files self-documenting. For example, instead of: ```yaml my_pillar_data: key1: val1 key2: val2 ``` users can now do: ```yaml my_pillar_data: !!omap - key1: val1 - key2: val2 ``` to make it clear to readers that the entries are intended to be processed in order. Behavior before: ```pycon >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] >>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data() OrderedDict([('foo', 'bar'), ('baz', 'bif')]) ``` Behavior after: ```pycon >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") OrderedDict([('foo', 'bar'), ('baz', 'bif')]) >>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data() OrderedDict([('foo', 'bar'), ('baz', 'bif')]) ``` Relevant bug: https://github.com/saltstack/salt/issues/12161 --- changelog/62932.fixed | 14 +++++++----- .../troubleshooting/yaml_idiosyncrasies.rst | 22 +++++++++++++++++-- salt/utils/yamlloader.py | 6 ++--- tests/pytests/unit/utils/test_yaml.py | 15 ++++++------- 4 files changed, 39 insertions(+), 18 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index 3b67108a65d8..767ff2547923 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -7,11 +7,15 @@ immediately: Improvements to YAML processing that are expected to take effect in Salt 3007 but can be previewed now by setting the option `yaml_compatibility` to `3007`: * Loading a well-formed `!!omap` node (a sequence of single-entry mapping - nodes) with a `salt.utils.yaml.SaltYamlSafeLoader` that was constructed with - a non-`dict` mapping class will produce an object of that mapping type - instead of raise an exception. Mapping nodes tagged with `!!omap` will - raise an exception due to invalid syntax (currently such nodes produce a - mapping object of the given type even though the syntax is invalid). + nodes) will always produce a `collections.OrderedDict` object. (Currently + it sometimes produces a `list` of `tuple` (key, value) pairs and sometimes + raises an exception, depending on the value of + `salt.utils.yaml.SaltYamlSafeLoader`'s `dictclass` constructor parameter.) + * Loading a mapping node tagged with `!!omap` will always raise an exception + due to invalid syntax. (Currently it sometimes raises an exception and + sometimes produces a `collections.OrderedDict` object, depending on the + value of `salt.utils.yaml.SaltYamlSafeLoader`'s `dictclass` constructor + parameter.) * Loading an explicitly tagged `!!timestamp` node will produce a `datetime.datetime` object instead of a string. * `salt.utils.yaml.dump()` will default to `salt.utils.yaml.OrderedDumper` diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index fc457c660846..68ad28f94b91 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -430,6 +430,14 @@ so ``!!timestamp`` nodes cannot be used in pillar SLS files. Ordered Dictionaries ==================== +.. versionchanged:: 3007.0 + + Loading a YAML ``!!omap`` node now reliably produces a + ``collections.OrderedDict`` object. Previously, an ``!!omap`` node would + sometimes produce a ``list`` of ``tuple`` (key, value) pairs and sometimes + raise an exception. Set the ``yaml_compatibility`` option to 3006 to revert + to the previous behavior. + The YAML specification defines an `ordered mapping type `_ which is equivalent to a plain mapping except iteration order is preserved. (YAML makes no guarantees about iteration order @@ -451,8 +459,18 @@ makes it obvious that the order of entries is significant, and (2) it provides a stronger guarantee of iteration order (plain mapping iteration order can be thought of as a Salt implementation detail that may change in the future). -Unfortunately, ``!!omap`` nodes should be avoided due to bugs in the way Salt -processes such nodes. +Salt produces a ``collections.OrderedDict`` object when it loads an ``!!omap`` +node. (Salt's behavior differs from PyYAML's default behavior, which is to +produce a ``list`` of (key, value) ``tuple`` objects.) These objects are a +subtype of ``dict``, so ``!!omap`` is a drop-in replacement for a plain mapping. + +Unfortunately, ``collections.OrderedDict`` objects should be avoided when +creating YAML programmatically (such as with the ``yaml`` Jinja filter) due to +bugs in the way ``collections.OrderedDict`` objects are converted to YAML. + +Beware that Salt currently serializes ``collections.OrderedDict`` objects the +same way it serializes plain ``dict`` objects, so they become plain ``dict`` +objects when deserialized by the recipient. Keys Limited to 1024 Characters =============================== diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index 4382c4face4c..1e7694988c80 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -3,6 +3,8 @@ """ +import collections + import yaml # pylint: disable=blacklisted-import from yaml.constructor import ConstructorError from yaml.nodes import MappingNode, SequenceNode @@ -97,12 +99,10 @@ def construct_yaml_omap_3006(self, node): return (yield from self.construct_yaml_map(node)) def construct_yaml_omap(self, node): - if self.dictclass is dict: - return (yield from super().construct_yaml_omap(node)) # BaseLoader.construct_yaml_omap() returns a list of (key, value) # tuples, which doesn't match the semantics of the `!!omap` YAML type. # Convert the list of tuples to an OrderedDict. - d = self.dictclass() + d = collections.OrderedDict() yield d (entries,) = super().construct_yaml_omap(node) # All of the following lines could be replaced with `d.update(entries)`, diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 4b07005f532e..00419061ac27 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -306,11 +306,9 @@ def test_load_dictclass(dictclass): # value) tuples (PyYAML's default behavior for !!omap nodes). (3006, True, None, list), # Starting with Salt v3007, an !!omap node is always required to be a - # sequence of mapping nodes. + # sequence of mapping nodes, and always returns an OrderedDict. (3007, True, _OrderedDictLoader, collections.OrderedDict), - # Unfortunately, the return value is still a list of (key, value) - # tuples when dictclass=dict. - (3007, True, None, list), + (3007, True, None, collections.OrderedDict), ], indirect=["yaml_compatibility"], ) @@ -360,7 +358,7 @@ def test_load_omap(yaml_compatibility, seq_input, Loader, wantclass): (3006, "!!omap {}\n", _OrderedDictLoader, collections.OrderedDict()), (3006, "!!omap []\n", None, []), (3007, "!!omap []\n", _OrderedDictLoader, collections.OrderedDict()), - (3007, "!!omap []\n", None, []), + (3007, "!!omap []\n", None, collections.OrderedDict()), ], indirect=["yaml_compatibility"], ) @@ -397,10 +395,11 @@ def test_load_omap_empty(yaml_compatibility, input_yaml, Loader, want): (3007, "!!omap [{}]\n", _OrderedDictLoader), (3007, "!!omap [{}]\n", None), # Invalid because there are duplicate keys. Note that the Loader=None - # cases for v3006 and v3007 are missing here; this is because the - # default Loader matches PyYAML's behavior, and PyYAML permits duplicate - # keys in !!omap nodes. + # case for v3006 is missing here; this is because the default v3006 + # Loader matches PyYAML's behavior, and PyYAML permits duplicate keys in + # !!omap nodes. (3007, "!!omap\n- dup key: 0\n- dup key: 1\n", _OrderedDictLoader), + (3007, "!!omap\n- dup key: 0\n- dup key: 1\n", None), ], indirect=["yaml_compatibility"], ) From ac4805bd0dd62226a61197a14b5932dbcec99b6e Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 17 Oct 2022 17:44:37 -0400 Subject: [PATCH 26/31] yaml: Load `!!python/tuple` nodes as `tuple` objects My main motivation for adding this is to facilitate testing, though it also gives module authors greater flexibility (in particular: a `tuple` can be a `dict` key, but a `list` cannot because it is not hashable), and I don't see a strong reason why this shouldn't be added. Behavior before: ```pycon >>> import salt.utils.yaml as y >>> y.load("!!python/tuple [foo, bar]") Traceback (most recent call last): File "", line 1, in File "salt/utils/yamlloader.py", line 159, in load return yaml.load(stream, Loader=Loader) File "venv/lib/python3.8/site-packages/yaml/__init__.py", line 81, in load return loader.get_single_data() File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 51, in get_single_data return self.construct_document(node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 55, in construct_document data = self.construct_object(node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 100, in construct_object data = constructor(self, node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 427, in construct_undefined raise ConstructorError(None, None, yaml.constructor.ConstructorError: could not determine a constructor for the tag 'tag:yaml.org,2002:python/tuple' in "", line 1, column 1 ``` Behavior after: ```pycon >>> import salt.utils.yaml as y >>> y.load("!!python/tuple [foo, bar]") ('foo', 'bar') ``` --- changelog/62932.fixed | 2 ++ .../troubleshooting/yaml_idiosyncrasies.rst | 25 +++++++++++++++++++ salt/utils/yamlloader.py | 6 +++++ tests/pytests/unit/utils/test_yaml.py | 7 ++++++ 4 files changed, 40 insertions(+) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index 767ff2547923..b05e0eb7f62a 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -3,6 +3,8 @@ immediately: * Passing a non-`dict` class to the `salt.utils.yaml.SaltYamlSafeLoader` constructor no longer causes all future `!!omap` nodes to raise an exception when loaded. + * Loading a sequence node tagged with `!!python/tuple` is now supported, and + produces a Python `tuple` object. Improvements to YAML processing that are expected to take effect in Salt 3007 but can be previewed now by setting the option `yaml_compatibility` to `3007`: diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 68ad28f94b91..336314ff4c9a 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -472,6 +472,31 @@ Beware that Salt currently serializes ``collections.OrderedDict`` objects the same way it serializes plain ``dict`` objects, so they become plain ``dict`` objects when deserialized by the recipient. +Tuples +====== + +.. versionchanged:: 3006.0 + + Loading a YAML ``!!python/tuple`` node is now supported. + +The YAML ``!!python/tuple`` type can be used to produce a Python ``tuple`` +object when loaded: + +.. code-block:: yaml + + !!python/tuple + - first item + - second item + +When dumped to YAML with ``salt.utils.yaml.dump()``, a ``tuple`` object produces +a ``!!python/tuple`` node. When dumped to YAML with +``salt.utils.yaml.safe_dump()``, a ``tuple`` object produces a plain sequence +node (which will be loaded as a ``list`` object). + +Beware that Salt currently serializes ``tuple`` objects the same way it +serializes ``list`` objects, so they become ``list`` objects when deserialized +by the recipient. + Keys Limited to 1024 Characters =============================== diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index 1e7694988c80..eb37c40599b8 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -161,6 +161,9 @@ def construct_mapping(self, node, deep=False): mapping[key] = value return mapping + def construct_python_tuple(self, node): + return tuple(self.construct_sequence(node)) + def construct_scalar(self, node): """ Verify integers and pass them in correctly is they are declared @@ -242,6 +245,9 @@ def flatten_mapping(self, node): SaltYamlSafeLoader.V3006.add_constructor( "tag:yaml.org,2002:omap", SaltYamlSafeLoader.construct_yaml_omap_3006 ) +SaltYamlSafeLoader.add_constructor( + "tag:yaml.org,2002:python/tuple", SaltYamlSafeLoader.construct_python_tuple +) SaltYamlSafeLoader.add_constructor( "tag:yaml.org,2002:python/unicode", SaltYamlSafeLoader.construct_unicode ) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 00419061ac27..dc7f134fa527 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -456,6 +456,13 @@ def test_load_timestamp(yaml_compatibility, input_yaml, want): assert got == want +def test_load_tuple(): + input = "!!python/tuple\n- foo\n- bar\n" + got = salt_yaml.load(input) + want = ("foo", "bar") + assert got == want + + def test_not_yaml_monkey_patching(): if hasattr(yaml, "CSafeLoader"): assert yaml.SafeLoader != yaml.CSafeLoader From 53a26935a9c27eb91e89f5469f7f164951dfa1c2 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 10 Nov 2022 19:39:41 -0500 Subject: [PATCH 27/31] yaml: Dump `datetime.datetime` objects with `!!timestamp` tag Override the implicit resolver for timestamp strings so that `datetime.datetime` objects are always written with an accompaying `!!timestamp` tag. This is a behavior change: Any users that depend on a `datetime` becoming a plain YAML string (which would be read in as a `str` object) must convert the object to string themselves. This change preserves the semantics of the object, and it preserves round-trip identity. Behavior before: ```pycon >>> import datetime >>> import salt.utils.yaml as y >>> print(y.dump(datetime.datetime(2022, 10, 21, 18, 16, 3, 100000, tzinfo=datetime.timezone(datetime.timedelta(hours=-4))))) 2022-10-21 18:16:03.100000-04:00 ``` Behavior after: ```pycon >>> import datetime >>> import salt.utils.yaml as y >>> print(y.dump(datetime.datetime(2022, 10, 21, 18, 16, 3, 100000, tzinfo=datetime.timezone(datetime.timedelta(hours=-4))))) !!timestamp 2022-10-21 18:16:03.100000-04:00 ``` --- changelog/62932.fixed | 2 ++ .../troubleshooting/yaml_idiosyncrasies.rst | 11 +++++++ salt/utils/yamldumper.py | 32 +++++++++++++++++++ tests/pytests/unit/utils/test_yaml.py | 25 +++++++++++++++ 4 files changed, 70 insertions(+) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index b05e0eb7f62a..636403746c74 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -20,6 +20,8 @@ but can be previewed now by setting the option `yaml_compatibility` to `3007`: parameter.) * Loading an explicitly tagged `!!timestamp` node will produce a `datetime.datetime` object instead of a string. + * Dumping a `datetime.datetime` object will explicitly tag the node with + `!!timestamp`. (Currently the nodes are untagged.) * `salt.utils.yaml.dump()` will default to `salt.utils.yaml.OrderedDumper` instead of `yaml.Dumper`. * Dumping a YAML sequence with `salt.utils.yaml.IndentedSafeOrderedDumper` diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 336314ff4c9a..c7ac5c442a39 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -406,6 +406,13 @@ Automatic ``datetime`` conversion Set the ``yaml_compatibility`` option to 3006 to revert to the previous behavior. +.. versionchanged:: 3007.0 + + Dumping a ``datetime.datetime`` object to YAML now explicitly tags the node + with ``!!timestamp``. Previously, the ``!!timestamp`` tag was omitted. + Set the ``yaml_compatibility`` option to 3006 to revert to the previous + behavior. + Salt overrides PyYAML's default behavior and loads YAML nodes that look like timestamps as strings: @@ -424,6 +431,10 @@ explicitly tag the node with ``!!timestamp``: >>> salt.utils.yaml.safe_load("!!timestamp 2014-01-20 14:23:23") datetime.datetime(2014, 1, 20, 14, 23, 23) +When dumping a ``datetime.datetime`` object to YAML, Salt tags the node with +``!!timestamp`` so that it will be loaded back as a ``datetime.datetime`` +object. + Beware that Salt is currently unable to serialize ``datetime.datetime`` objects, so ``!!timestamp`` nodes cannot be used in pillar SLS files. diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 7aa4d0234b02..0f0423a82f8c 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -75,7 +75,39 @@ class _VersionedRepresentersMixin( pass +class _ExplicitTimestampMixin: + """Disables the implicit timestamp resolver for Salt 3007 and later. + + Once support for ``yaml_compatibility`` less than 3007 is dropped, we can + instead remove the implicit resolver entirely: + + .. code-block:: python + + class _RemoveImplicitResolverMixin: + @classmethod + def remove_implicit_resolver(cls, tag): + cls.yaml_implicit_resolvers = { + first_char: [r for r in resolver_list if r[0] != tag] + for first_char, resolver_list in cls.yaml_implicit_resolvers.items() + } + + _CommonMixin.remove_implicit_resolver("tag:yaml.org,2002:timestamp") + """ + + def resolve(self, kind, value, implicit): + tag = super().resolve(kind, value, implicit) + if ( + kind is yaml.ScalarNode + and implicit[0] + and tag == "tag:yaml.org,2002:timestamp" + and _yaml_common.compat_ver() >= SaltStackVersion(3007) + ): + return self.DEFAULT_SCALAR_TAG + return tag + + class _CommonMixin( + _ExplicitTimestampMixin, _VersionedRepresentersMixin, _InheritedRepresentersMixin, ): diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index dc7f134fa527..eb57e17ae944 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -145,6 +145,31 @@ def test_dump_indented(yaml_compatibility, want): assert got == want +@pytest.mark.parametrize( + "dumpercls", + [ + salt_yaml.OrderedDumper, + salt_yaml.SafeOrderedDumper, + salt_yaml.IndentedSafeOrderedDumper, + ], +) +@pytest.mark.parametrize( + "yaml_compatibility,want_tag", + [(3006, False), (3007, True)], + indirect=["yaml_compatibility"], +) +def test_dump_timestamp(yaml_compatibility, want_tag, dumpercls): + dt = datetime.datetime( + *(2022, 10, 21, 18, 16, 3, 100000), + tzinfo=datetime.timezone(datetime.timedelta(hours=-4)), + ) + got = salt_yaml.dump(dt, Dumper=dumpercls) + want_re = r"""(['"]?)2022-10-21[T ]18:16:03.10*-04:00\1\n(?:\.\.\.\n)?""" + if want_tag: + want_re = f"!!timestamp {want_re}" + assert re.fullmatch(want_re, got) + + def render_yaml(data): """ Takes a YAML string, puts it into a mock file, passes that to the YAML From 19a6584869b6745de3dea06847806e7d68aa74b2 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Tue, 1 Nov 2022 23:40:19 -0400 Subject: [PATCH 28/31] yaml: Dump all `OrderedDict` types the same way Behavior before: ```pycon >>> from salt.utils.odict import OrderedDict >>> import salt.utils.yaml as y >>> import collections >>> print(y.dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.safe_dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) !!python/object/apply:collections.OrderedDict - - - foo - bar >>> print(y.safe_dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) NULL ``` Behavior after: ```pycon >>> from salt.utils.odict import OrderedDict >>> import salt.utils.yaml as y >>> import collections >>> print(y.dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.safe_dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.safe_dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar ``` --- changelog/62932.fixed | 2 + .../troubleshooting/yaml_idiosyncrasies.rst | 14 +++++-- salt/utils/yamldumper.py | 14 ++++++- tests/pytests/unit/utils/test_yaml.py | 40 +++++++++++++++++++ 4 files changed, 66 insertions(+), 4 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index 636403746c74..b7c1f0d45a4e 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -18,6 +18,8 @@ but can be previewed now by setting the option `yaml_compatibility` to `3007`: sometimes produces a `collections.OrderedDict` object, depending on the value of `salt.utils.yaml.SaltYamlSafeLoader`'s `dictclass` constructor parameter.) + * Dumping a `collections.OrderedMap` will consistently produce a plain mapping + node. * Loading an explicitly tagged `!!timestamp` node will produce a `datetime.datetime` object instead of a string. * Dumping a `datetime.datetime` object will explicitly tag the node with diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index c7ac5c442a39..94a6481da290 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -449,6 +449,15 @@ Ordered Dictionaries raise an exception. Set the ``yaml_compatibility`` option to 3006 to revert to the previous behavior. +.. versionchanged:: 3007.0 + + Dumping any ``collections.OrderedDict`` object to YAML now reliably produces + a plain mapping node. Previously, only the subtype + ``salt.utils.odict.OrderedDict`` was supported, and other types produced + various constructs that could not be loaded back as any kind of ``dict``. + Set the ``yaml_compatibility`` option to 3006 to revert to the previous + behavior. + The YAML specification defines an `ordered mapping type `_ which is equivalent to a plain mapping except iteration order is preserved. (YAML makes no guarantees about iteration order @@ -475,9 +484,8 @@ node. (Salt's behavior differs from PyYAML's default behavior, which is to produce a ``list`` of (key, value) ``tuple`` objects.) These objects are a subtype of ``dict``, so ``!!omap`` is a drop-in replacement for a plain mapping. -Unfortunately, ``collections.OrderedDict`` objects should be avoided when -creating YAML programmatically (such as with the ``yaml`` Jinja filter) due to -bugs in the way ``collections.OrderedDict`` objects are converted to YAML. +When dumping a ``collections.OrderedDict`` object to YAML, Salt generates a +plain mapping, not an ``!!omap`` node. Beware that Salt currently serializes ``collections.OrderedDict`` objects the same way it serializes plain ``dict`` objects, so they become plain ``dict`` diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 0f0423a82f8c..3aae87e82480 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -132,7 +132,19 @@ def _rep_default(self, data): # TODO: Why does this registration exist? Isn't it better to raise an exception # for unsupported types? _CommonMixin.add_representer(None, _CommonMixin._rep_default) -_CommonMixin.add_representer(OrderedDict, _CommonMixin._rep_ordereddict) +_CommonMixin.V3006.add_representer(OrderedDict, _CommonMixin._rep_ordereddict) +# This multi representer covers collections.OrderedDict and all of its +# subclasses, including salt.utils.odict.OrderedDict. +_CommonMixin.V3007.add_multi_representer( + collections.OrderedDict, _CommonMixin._rep_ordereddict +) +# This non-multi representer may seem redundant given the multi representer +# registered above, but it is needed to override the non-multi representer +# that exists in the ancestor Representer class. (Non-multi representers +# take priority over multi representers.) +_CommonMixin.V3007.add_representer( + collections.OrderedDict, _CommonMixin._rep_ordereddict +) _CommonMixin.add_representer( collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict ) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index eb57e17ae944..4bc65d18cb6e 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -12,6 +12,7 @@ import salt.utils._yaml_common as _yc import salt.utils.files import salt.utils.yaml as salt_yaml +from salt.utils.odict import OrderedDict from salt.version import SaltStackVersion from tests.support.mock import mock_open, patch @@ -145,6 +146,45 @@ def test_dump_indented(yaml_compatibility, want): assert got == want +@pytest.mark.parametrize("yaml_compatibility", [3006, 3007], indirect=True) +@pytest.mark.parametrize("dictcls", [OrderedDict, collections.OrderedDict]) +@pytest.mark.parametrize( + "dumpercls", + [ + salt_yaml.OrderedDumper, + salt_yaml.SafeOrderedDumper, + salt_yaml.IndentedSafeOrderedDumper, + ], +) +def test_dump_omap(yaml_compatibility, dictcls, dumpercls): + # The random keys are filtered through a set to avoid duplicates. + keys = list({f"random key {random.getrandbits(32)}" for _ in range(20)}) + # Avoid unintended correlation with set()'s iteration order. + random.shuffle(keys) + items = [(k, i) for i, k in enumerate(keys)] + d = dictcls(items) + if yaml_compatibility == 3006 and dictcls is collections.OrderedDict: + # Buggy behavior preserved for backwards compatibility. + if dumpercls is salt_yaml.OrderedDumper: + want = ( + "!!python/object/apply:collections.OrderedDict\n-" + + "".join(f" - - {k}\n - {v}\n" for k, v in items)[1:] + ) + else: + # With v3006, sometimes it prints "..." on a new line as an end of + # document indicator depending on whether yaml.CSafeDumper is + # available on the system or not (yaml.CSafeDumper and + # yaml.SafeDumper do not always behave the same). + want = re.compile(r"NULL\n(?:\.\.\.\n)?") + else: + want = "".join(f"{k}: {v}\n" for k, v in items) + got = salt_yaml.dump(d, Dumper=dumpercls, default_flow_style=False) + try: + assert want.fullmatch(got) + except AttributeError: + assert got == want + + @pytest.mark.parametrize( "dumpercls", [ From 6c7dd349d314b9cfeeffa4283092e4b064936609 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 17 Oct 2022 01:01:43 -0400 Subject: [PATCH 29/31] yaml: Dump `OrderedDict` objects as `!!omap` nodes Before, an `OrderedDict` object was represented as a plain YAML mapping. Now it is represented as an `!!omap` node, which is a sequence of single-valued mappings. This is a behavior change: Any users that depend on an `OrderedDict` becoming a plain YAML mapping (which would be read in as a `dict` object) must first convert the `OrderedDict` to `dict`. This change preserves the semantics of the object, and it preserves round-trip identity. Behavior before: ```pycon >>> from salt.utils.odict import OrderedDict >>> import salt.utils.yaml as y >>> import collections >>> print(y.dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.safe_dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.safe_dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar ``` Behavior after: ```pycon >>> from salt.utils.odict import OrderedDict >>> import salt.utils.yaml as y >>> import collections >>> print(y.dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) !!omap - foo: bar >>> print(y.safe_dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) !!omap - foo: bar >>> print(y.dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) !!omap - foo: bar >>> print(y.safe_dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) !!omap - foo: bar ``` --- changelog/62932.fixed | 4 +-- .../troubleshooting/yaml_idiosyncrasies.rst | 14 ++++---- salt/utils/yamldumper.py | 33 ++++++++++++++----- .../utils/jinja/test_custom_extensions.py | 3 ++ tests/pytests/unit/utils/test_yaml.py | 7 +++- 5 files changed, 43 insertions(+), 18 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index b7c1f0d45a4e..46c9dd2ac0e3 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -18,8 +18,8 @@ but can be previewed now by setting the option `yaml_compatibility` to `3007`: sometimes produces a `collections.OrderedDict` object, depending on the value of `salt.utils.yaml.SaltYamlSafeLoader`'s `dictclass` constructor parameter.) - * Dumping a `collections.OrderedMap` will consistently produce a plain mapping - node. + * Dumping a `collections.OrderedMap` will consistently produce an `!!omap` + node (a sequence of single-entry mapping nodes). * Loading an explicitly tagged `!!timestamp` node will produce a `datetime.datetime` object instead of a string. * Dumping a `datetime.datetime` object will explicitly tag the node with diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 94a6481da290..e8c63c9ce54c 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -452,11 +452,11 @@ Ordered Dictionaries .. versionchanged:: 3007.0 Dumping any ``collections.OrderedDict`` object to YAML now reliably produces - a plain mapping node. Previously, only the subtype - ``salt.utils.odict.OrderedDict`` was supported, and other types produced - various constructs that could not be loaded back as any kind of ``dict``. - Set the ``yaml_compatibility`` option to 3006 to revert to the previous - behavior. + an ``!!omap`` node. Previously, only the subtype + ``salt.utils.odict.OrderedDict`` was supported, and it always produced a + plain mapping node. Other types produced various constructs that could not + be loaded back as any kind of ``dict``. Set the ``yaml_compatibility`` + option to 3006 to revert to the previous behavior. The YAML specification defines an `ordered mapping type `_ which is equivalent to a plain mapping except @@ -484,8 +484,8 @@ node. (Salt's behavior differs from PyYAML's default behavior, which is to produce a ``list`` of (key, value) ``tuple`` objects.) These objects are a subtype of ``dict``, so ``!!omap`` is a drop-in replacement for a plain mapping. -When dumping a ``collections.OrderedDict`` object to YAML, Salt generates a -plain mapping, not an ``!!omap`` node. +When dumping a ``collections.OrderedDict`` object to YAML, Salt produces an +``!!omap`` node. Beware that Salt currently serializes ``collections.OrderedDict`` objects the same way it serializes plain ``dict`` objects, so they become plain ``dict`` diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 3aae87e82480..37c8c8b0dbce 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -111,9 +111,13 @@ class _CommonMixin( _VersionedRepresentersMixin, _InheritedRepresentersMixin, ): - def _rep_ordereddict(self, data): + def _rep_ordereddict_as_plain_map(self, data): return self.represent_dict(list(data.items())) + def _rep_ordereddict(self, data): + seq = [{k: v} for k, v in data.items()] + return self.represent_sequence("tag:yaml.org,2002:omap", seq) + def _rep_default(self, data): """Represent types that don't match any other registered representers. @@ -132,7 +136,9 @@ def _rep_default(self, data): # TODO: Why does this registration exist? Isn't it better to raise an exception # for unsupported types? _CommonMixin.add_representer(None, _CommonMixin._rep_default) -_CommonMixin.V3006.add_representer(OrderedDict, _CommonMixin._rep_ordereddict) +_CommonMixin.V3006.add_representer( + OrderedDict, _CommonMixin._rep_ordereddict_as_plain_map +) # This multi representer covers collections.OrderedDict and all of its # subclasses, including salt.utils.odict.OrderedDict. _CommonMixin.V3007.add_multi_representer( @@ -154,15 +160,26 @@ def _rep_default(self, data): ) -class OrderedDumper(_CommonMixin, Dumper): - """ - A YAML dumper that represents python OrderedDict as simple YAML map. - """ +class SafeOrderedDumper(_CommonMixin, SafeDumper): + """A safe YAML dumper that uses the YAML ``!!omap`` type for ``OrderedDict`` + ``OrderedDict``s are represented as a a sequence of single-entry mappings + and tagged with ``!!omap``: -class SafeOrderedDumper(_CommonMixin, SafeDumper): + .. code-block:: yaml + + !!omap + - first key: first value + - second key: second value + + See https://yaml.org/type/omap.html for details. """ - A YAML safe dumper that represents python OrderedDict as simple YAML map. + + +class OrderedDumper(_CommonMixin, Dumper): + """A YAML dumper that uses the YAML ``!!omap`` type for ``OrderedDict`` + + See ``SafeOrderedDumper`` for details. """ diff --git a/tests/pytests/unit/utils/jinja/test_custom_extensions.py b/tests/pytests/unit/utils/jinja/test_custom_extensions.py index 4d004230fcbb..53d9852afd1f 100644 --- a/tests/pytests/unit/utils/jinja/test_custom_extensions.py +++ b/tests/pytests/unit/utils/jinja/test_custom_extensions.py @@ -3,6 +3,7 @@ """ import ast +import collections import itertools import os import pprint @@ -133,6 +134,8 @@ def test_serialize_yaml(): env = Environment(extensions=[SerializerExtension]) rendered = env.from_string("{{ dataset|yaml }}").render(dataset=dataset) assert dataset == salt.utils.yaml.safe_load(rendered) + assert isinstance(dataset["spam"], collections.OrderedDict) + assert isinstance(dataset["spam"]["foo"], collections.OrderedDict) def test_serialize_yaml_str(): diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 4bc65d18cb6e..ffad9a53dbc5 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -176,8 +176,13 @@ def test_dump_omap(yaml_compatibility, dictcls, dumpercls): # available on the system or not (yaml.CSafeDumper and # yaml.SafeDumper do not always behave the same). want = re.compile(r"NULL\n(?:\.\.\.\n)?") - else: + elif yaml_compatibility == 3006: want = "".join(f"{k}: {v}\n" for k, v in items) + else: + # Note that there is no extra indentation added for the + # IndentedSafeOrderedDumper case because the !!omap node is the + # top-level node so there is no indentation for the sequence elements. + want = "!!omap\n" + "".join(f"- {k}: {v}\n" for k, v in items) got = salt_yaml.dump(d, Dumper=dumpercls, default_flow_style=False) try: assert want.fullmatch(got) From bd2b75663cfd11c6d4628da22352e61fd14cc026 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 02:54:43 -0400 Subject: [PATCH 30/31] yaml: Dump `tuple` objects as `!!python/tuple` nodes Before, `!!python/tuple` was only used for `OrderedDumper`. Now it is also used for `SafeOrderedDumper` and `IndentedSafeOrderedDumper`. This is a behavior change: Any users that depended on a `tuple` becoming a plain YAML sequence (which would be read in as a `list` object) must first convert the `tuple` to `list`. This change preserves the semantics of the object, and it preserves round-trip identity. Preserving round-trip identity is particularly important if the tuple is used as a key in a `dict` because `list` objects are not hashable. Behavior before: ```pycon >>> import salt.utils.yaml as y >>> print(y.dump(("foo", "bar"), default_flow_style=False)) !!python/tuple - foo - bar >>> print(y.safe_dump(("foo", "bar"), default_flow_style=False)) - foo - bar ``` Behavior after: ```pycon >>> import salt.utils.yaml as y >>> print(y.dump(("foo", "bar"), default_flow_style=False)) !!python/tuple - foo - bar >>> print(y.safe_dump(("foo", "bar"), default_flow_style=False)) !!python/tuple - foo - bar ``` --- changelog/62932.fixed | 3 +++ .../troubleshooting/yaml_idiosyncrasies.rst | 13 +++++++---- salt/utils/yamldumper.py | 4 ++++ .../utils/jinja/test_custom_extensions.py | 1 + tests/pytests/unit/utils/test_yaml.py | 22 +++++++++++++++++++ 5 files changed, 39 insertions(+), 4 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index 46c9dd2ac0e3..3eb317d134b2 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -24,6 +24,9 @@ but can be previewed now by setting the option `yaml_compatibility` to `3007`: `datetime.datetime` object instead of a string. * Dumping a `datetime.datetime` object will explicitly tag the node with `!!timestamp`. (Currently the nodes are untagged.) + * Dumping a `tuple` object will consistently produce a sequence node + explicitly tagged with `!!python/tuple`. (Currently `safe_dump()` omits the + tag and `dump()` includes it.) * `salt.utils.yaml.dump()` will default to `salt.utils.yaml.OrderedDumper` instead of `yaml.Dumper`. * Dumping a YAML sequence with `salt.utils.yaml.IndentedSafeOrderedDumper` diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index e8c63c9ce54c..f0118bb4fc88 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -498,6 +498,13 @@ Tuples Loading a YAML ``!!python/tuple`` node is now supported. +.. versionchanged:: 3007.0 + + Dumping a ``tuple`` object to YAML now always produces a sequence node + tagged with ``!!python/tuple``. Previously, ``salt.utils.yaml.safe_dump()`` + did not tag the node. Set the ``yaml_compatibility`` option to 3006 to + revert to the previous behavior. + The YAML ``!!python/tuple`` type can be used to produce a Python ``tuple`` object when loaded: @@ -507,10 +514,8 @@ object when loaded: - first item - second item -When dumped to YAML with ``salt.utils.yaml.dump()``, a ``tuple`` object produces -a ``!!python/tuple`` node. When dumped to YAML with -``salt.utils.yaml.safe_dump()``, a ``tuple`` object produces a plain sequence -node (which will be loaded as a ``list`` object). +When dumped to YAML, a ``tuple`` object produces a sequence node tagged with +``!!python/tuple``. Beware that Salt currently serializes ``tuple`` objects the same way it serializes ``list`` objects, so they become ``list`` objects when deserialized diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 37c8c8b0dbce..54365f092a22 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -158,6 +158,10 @@ def _rep_default(self, data): salt.utils.context.NamespacedDictWrapper, yaml.representer.SafeRepresenter.represent_dict, ) +# SafeDumper represents tuples as lists, but Dumper's behavior (sequence +# tagged with `!!python/tuple`) is safe, so use it for all dumpers. +_CommonMixin.add_multi_representer(tuple, Dumper.yaml_representers[tuple]) +_CommonMixin.add_representer(tuple, Dumper.yaml_representers[tuple]) class SafeOrderedDumper(_CommonMixin, SafeDumper): diff --git a/tests/pytests/unit/utils/jinja/test_custom_extensions.py b/tests/pytests/unit/utils/jinja/test_custom_extensions.py index 53d9852afd1f..fbb26c6238c4 100644 --- a/tests/pytests/unit/utils/jinja/test_custom_extensions.py +++ b/tests/pytests/unit/utils/jinja/test_custom_extensions.py @@ -130,6 +130,7 @@ def test_serialize_yaml(): "baz": [1, 2, 3], "qux": 2.0, "spam": OrderedDict([("foo", OrderedDict([("bar", "baz"), ("qux", 42)]))]), + "tuple": ("foo", "bar"), } env = Environment(extensions=[SerializerExtension]) rendered = env.from_string("{{ dataset|yaml }}").render(dataset=dataset) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index ffad9a53dbc5..8f53974f3666 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -215,6 +215,28 @@ def test_dump_timestamp(yaml_compatibility, want_tag, dumpercls): assert re.fullmatch(want_re, got) +@pytest.mark.parametrize( + "mktuple", + [ + lambda *args: tuple(args), + collections.namedtuple("TestTuple", "a b"), + ], +) +@pytest.mark.parametrize( + "dumpercls", + [ + salt_yaml.OrderedDumper, + salt_yaml.SafeOrderedDumper, + salt_yaml.IndentedSafeOrderedDumper, + ], +) +def test_dump_tuple(mktuple, dumpercls): + data = mktuple("foo", "bar") + want = "!!python/tuple [foo, bar]\n" + got = salt_yaml.dump(data, Dumper=dumpercls) + assert got == want + + def render_yaml(data): """ Takes a YAML string, puts it into a mock file, passes that to the YAML From ac8d18e3df3a6ab2b69c47020d591ae29bde519b Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 11 Nov 2022 17:19:16 -0500 Subject: [PATCH 31/31] yaml: Raise an exception when dumping unsupported types Failure to represent an object is a bug so it should be noisy, not quietly produce `NULL`. The representer this commit disables was added in commit 2e2ce5f1078b794eb87fe781d13b8af745c865d5. Behavior before: ```pycon >>> import salt.utils.yaml as y >>> y.safe_dump(type("GeneratedObjectSubclass", (), {})) 'NULL\n' ``` Behavior after: ```pycon >>> import salt.utils.yaml as y >>> y.safe_dump(type("GeneratedObjectSubclass", (), {})) Traceback (most recent call last): File "", line 1, in File "salt/utils/yamldumper.py", line 270, in safe_dump return dump(data, stream, Dumper=SafeOrderedDumper, **kwargs) File "salt/utils/yamldumper.py", line 261, in dump return yaml.dump(data, stream, **kwargs) File "venv/lib/python3.8/site-packages/yaml/__init__.py", line 253, in dump return dump_all([data], stream, Dumper=Dumper, **kwds) File "venv/lib/python3.8/site-packages/yaml/__init__.py", line 241, in dump_all dumper.represent(data) File "venv/lib/python3.8/site-packages/yaml/representer.py", line 27, in represent node = self.represent_data(data) File "venv/lib/python3.8/site-packages/yaml/representer.py", line 58, in represent_data node = self.yaml_representers[None](self, data) File "venv/lib/python3.8/site-packages/yaml/representer.py", line 231, in represent_undefined raise RepresenterError("cannot represent an object", data) yaml.representer.RepresenterError: ('cannot represent an object', ) ``` --- changelog/62932.fixed | 4 ++ .../troubleshooting/yaml_idiosyncrasies.rst | 9 +++++ salt/utils/yamldumper.py | 4 +- tests/pytests/unit/utils/test_yaml.py | 37 +++++++++++++++++++ 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index 3eb317d134b2..c4a737a7a0d2 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -31,3 +31,7 @@ but can be previewed now by setting the option `yaml_compatibility` to `3007`: instead of `yaml.Dumper`. * Dumping a YAML sequence with `salt.utils.yaml.IndentedSafeOrderedDumper` will be properly indented. + * Dumping an object with an unsupported type via `salt.utils.yaml.safe_dump()` + (or via `dump()` with the `SafeOrderedDumper` or `IndentedSafeOrderedDumper` + classes) will raise an exception. (Currently such objects produce a null + node.) diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index f0118bb4fc88..a49d682a2f65 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -23,6 +23,15 @@ unexpected times. YAML compatibility warnings can be disabled by setting the ``yaml_compatibility_warnings`` option to False. +.. versionchanged:: 3007.0 + + Dumping an object of unsupported type to YAML with a one of the safe dumpers + (:py:func:`~salt.utils.yaml.safe_dump`, or :py:func:`~salt.utils.yaml.dump` + with the :py:class:`~salt.utils.yaml.SafeOrderedDumper` or + :py:class:`~salt.utils.yaml.IndentedSafeOrderedDumper` classes) now raises + an exception. Previously it produced a YAML ``NULL`` node. Set the + ``yaml_compatibility`` option to 3006 to revert to the previous behavior. + Spaces vs Tabs ============== diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 54365f092a22..121ac038fa63 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -133,9 +133,7 @@ def _rep_default(self, data): return self.represent_scalar("tag:yaml.org,2002:null", "NULL") -# TODO: Why does this registration exist? Isn't it better to raise an exception -# for unsupported types? -_CommonMixin.add_representer(None, _CommonMixin._rep_default) +_CommonMixin.V3006.add_representer(None, _CommonMixin._rep_default) _CommonMixin.V3006.add_representer( OrderedDict, _CommonMixin._rep_ordereddict_as_plain_map ) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 8f53974f3666..59824b831d7a 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -237,6 +237,43 @@ def test_dump_tuple(mktuple, dumpercls): assert got == want +@pytest.mark.parametrize("obj", [type("Generated", (), {})()]) +@pytest.mark.parametrize( + "yaml_compatibility,dumpercls,want", + [ + ( + 3006, + salt_yaml.OrderedDumper, + "!!python/object:tests.pytests.unit.utils.test_yaml.Generated {}\n", + ), + # With v3006, sometimes it prints "..." on a new line as an end of + # document indicator depending on whether yaml.CSafeDumper is available + # on the system or not (yaml.CSafeDumper and yaml.SafeDumper do not + # always behave the same). + (3006, salt_yaml.SafeOrderedDumper, re.compile(r"NULL\n(?:\.\.\.\n)?")), + (3006, salt_yaml.IndentedSafeOrderedDumper, re.compile(r"NULL\n(?:\.\.\.\n)?")), + ( + 3007, + salt_yaml.OrderedDumper, + "!!python/object:tests.pytests.unit.utils.test_yaml.Generated {}\n", + ), + (3007, salt_yaml.SafeOrderedDumper, yaml.representer.RepresenterError), + (3007, salt_yaml.IndentedSafeOrderedDumper, yaml.representer.RepresenterError), + ], + indirect=["yaml_compatibility"], +) +def test_dump_unsupported(yaml_compatibility, dumpercls, obj, want): + if isinstance(want, type) and issubclass(want, Exception): + with pytest.raises(want): + salt_yaml.dump(obj, Dumper=dumpercls) + else: + got = salt_yaml.dump(obj, Dumper=dumpercls) + try: + assert want.fullmatch(got) + except AttributeError: + assert got == want + + def render_yaml(data): """ Takes a YAML string, puts it into a mock file, passes that to the YAML