From cb8d7830b12bcc7126a65ce3b2482c05279571fe Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 22 Oct 2022 19:09:59 -0400 Subject: [PATCH 01/14] 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 91d8a4fc80fd52df7ef6332c0eb4da2a27230f36 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 22 Oct 2022 20:43:20 -0400 Subject: [PATCH 02/14] 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 9c917d792918156187ce439e13a28921c1b5c8e6 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 13 Oct 2022 23:52:47 -0400 Subject: [PATCH 03/14] yaml: Convert `salt.utils.yaml` tests to pytest --- changelog/63158.fixed | 2 +- tests/pytests/unit/utils/test_yaml.py | 187 ++++++++++++++++++++++++++ tests/unit/utils/test_yamlloader.py | 169 ----------------------- 3 files changed, 188 insertions(+), 170 deletions(-) create mode 100644 tests/pytests/unit/utils/test_yaml.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..9ebe8801b828 --- /dev/null +++ b/tests/pytests/unit/utils/test_yaml.py @@ -0,0 +1,187 @@ +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_load_binary_unpadded(): + """ + Test that !!binary values without base64 padding are accepted. + Regression test for https://github.com/saltstack/salt/issues/69207 + """ + # 'a1b2c3' is 6 chars; valid only after adding '==' padding + result = render_yaml("vdata: !!binary a1b2c3") + assert result == {"vdata": b"\x6b\x56\xf6\x73"} + + +def test_load_binary_padded(): + """ + Test that !!binary values with correct base64 padding still work. + """ + result = render_yaml("vdata: !!binary a1b2c3==") + assert result == {"vdata": b"\x6b\x56\xf6\x73"} + + +def test_load_binary_invalid(): + """ + Test that invalid data in a !!binary value still raises ConstructorError. + + Non-ASCII characters are used here because they reliably trigger the + UnicodeEncodeError path across all Python versions. Testing via the + binascii.Error path is not reliable: Python 3.10 silently discards + unrecognized ASCII characters in base64 data, while Python 3.14 is + stricter. The padding fix itself is covered by test_load_binary_unpadded. + """ + with pytest.raises(ConstructorError): + render_yaml("vdata: !!binary \xc3\xb1") + + +def test_not_yaml_monkey_patching(): + if hasattr(yaml, "CSafeLoader"): + assert yaml.SafeLoader != yaml.CSafeLoader diff --git a/tests/unit/utils/test_yamlloader.py b/tests/unit/utils/test_yamlloader.py deleted file mode 100644 index 3dfe51f75216..000000000000 --- a/tests/unit/utils/test_yamlloader.py +++ /dev/null @@ -1,169 +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_yaml_binary_unpadded(self): - """ - Test that !!binary values without base64 padding are accepted. - Regression test for https://github.com/saltstack/salt/issues/69207 - """ - # 'a1b2c3' is 6 chars; valid only after adding '==' padding - result = self.render_yaml("vdata: !!binary a1b2c3") - self.assertEqual(result, {"vdata": b"\x6b\x56\xf6\x73"}) - - def test_yaml_binary_padded(self): - """ - Test that !!binary values with correct base64 padding still work. - """ - result = self.render_yaml("vdata: !!binary a1b2c3==") - self.assertEqual(result, {"vdata": b"\x6b\x56\xf6\x73"}) - - def test_yaml_binary_invalid(self): - """ - Test that invalid data in a !!binary value still raises ConstructorError. - - Non-ASCII characters are used here because they reliably trigger the - UnicodeEncodeError path across all Python versions. Testing via the - binascii.Error path is not reliable: Python 3.10 silently discards - unrecognized ASCII characters in base64 data, while Python 3.14 is - stricter. The padding fix itself is covered by test_yaml_binary_unpadded. - """ - with self.assertRaises(ConstructorError): - self.render_yaml("vdata: !!binary \xc3\xb1") - - def test_not_yaml_monkey_patching(self): - if hasattr(yaml, "CSafeLoader"): - assert yaml.SafeLoader != yaml.CSafeLoader From 27b8d5f64f60abe7aeecbdecc1ff65bbc0376cd0 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 16 Oct 2022 15:56:00 -0400 Subject: [PATCH 04/14] 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 72c00365a865004b524a522b5183d6ad00d12236 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 02:25:30 -0400 Subject: [PATCH 05/14] 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 9aa49d7966b6..dc4b219d3cb6 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -93,6 +93,17 @@ def represent_listproxy(dumper, data): OrderedDumper.add_representer(HashableOrderedDict, represent_ordereddict) SafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict) SafeOrderedDumper.add_representer(HashableOrderedDict, 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) IndentedSafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict) IndentedSafeOrderedDumper.add_representer(HashableOrderedDict, represent_ordereddict) @@ -155,6 +166,7 @@ def represent_listproxy(dumper, data): MaskedList, yaml.representer.SafeRepresenter.represent_list ) +# 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 4790a48c102610032347cdf72b31bdd8b7d0c955 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 00:56:04 -0400 Subject: [PATCH 06/14] 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 dc4b219d3cb6..eaee03d80a29 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -35,17 +35,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. @@ -58,11 +47,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 4c307161d5af57bbff4314da474b36a0dfc4ede4 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 1 Dec 2022 01:09:39 -0500 Subject: [PATCH 07/14] yaml: Use a `for` loop to factor out duplicate code --- salt/utils/yamldumper.py | 102 ++++++++++++++++----------------------- 1 file changed, 41 insertions(+), 61 deletions(-) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index eaee03d80a29..3e5fadbd5ef4 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -78,67 +78,55 @@ def represent_listproxy(dumper, data): return dumper.represent_list(list(data)) -OrderedDumper.add_representer(OrderedDict, represent_ordereddict) -OrderedDumper.add_representer(HashableOrderedDict, represent_ordereddict) -SafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict) -SafeOrderedDumper.add_representer(HashableOrderedDict, 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 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? + # + # 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. + D.add_representer(None, represent_undefined) + D.add_representer(OrderedDict, represent_ordereddict) + D.add_representer(HashableOrderedDict, represent_ordereddict) + D.add_representer( + collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict + ) + D.add_representer( + salt.utils.context.NamespacedDictWrapper, + yaml.representer.SafeRepresenter.represent_dict, + ) + D.add_representer(OptsDict, represent_optsdict) + D.add_representer(DictProxy, represent_dictproxy) + D.add_representer(ListProxy, represent_listproxy) + # Pillar containers are wrapped in MaskedDict / MaskedList for repr redaction; + # they are still plain dict / list at the data level, so dump them as such + # instead of falling through to represent_undefined (which would emit NULL). + D.add_representer( + MaskedDict, yaml.representer.SafeRepresenter.represent_dict + ) + D.add_representer( + MaskedList, yaml.representer.SafeRepresenter.represent_list + ) + # 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 + IndentedSafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict) IndentedSafeOrderedDumper.add_representer(HashableOrderedDict, represent_ordereddict) - -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, -) - -OrderedDumper.add_representer(OptsDict, represent_optsdict) -SafeOrderedDumper.add_representer(OptsDict, represent_optsdict) -OrderedDumper.add_representer(DictProxy, represent_dictproxy) -SafeOrderedDumper.add_representer(DictProxy, represent_dictproxy) -OrderedDumper.add_representer(ListProxy, represent_listproxy) -SafeOrderedDumper.add_representer(ListProxy, represent_listproxy) -# Pillar containers are wrapped in MaskedDict / MaskedList for repr redaction; -# they are still plain dict / list at the data level, so dump them as such -# instead of falling through to represent_undefined (which would emit NULL). -OrderedDumper.add_representer( - MaskedDict, yaml.representer.SafeRepresenter.represent_dict -) -SafeOrderedDumper.add_representer( - MaskedDict, yaml.representer.SafeRepresenter.represent_dict -) IndentedSafeOrderedDumper.add_representer( MaskedDict, yaml.representer.SafeRepresenter.represent_dict ) -OrderedDumper.add_representer( - MaskedList, yaml.representer.SafeRepresenter.represent_list -) -SafeOrderedDumper.add_representer( - MaskedList, yaml.representer.SafeRepresenter.represent_list -) IndentedSafeOrderedDumper.add_representer( MaskedList, yaml.representer.SafeRepresenter.represent_list ) + # Also register with base YAML dumpers for salt.utils.yaml.dump() yaml.Dumper.add_representer(OptsDict, represent_optsdict) yaml.SafeDumper.add_representer(OptsDict, represent_optsdict) @@ -155,14 +143,6 @@ def represent_listproxy(dumper, data): MaskedList, yaml.representer.SafeRepresenter.represent_list ) -# 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 -) - def get_dumper(dumper_name): return { From e38272bbe2908a868c29901eeb0bb4152ad1ba52 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 10 Nov 2022 16:19:12 -0500 Subject: [PATCH 08/14] yaml: Register default representer with `OrderedDumper` too This does not change the behavior, but it does simplify the code. --- salt/utils/yamldumper.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 3e5fadbd5ef4..88f6b3b8a93c 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -88,10 +88,6 @@ def represent_listproxy(dumper, data): # # 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. D.add_representer(None, represent_undefined) D.add_representer(OrderedDict, represent_ordereddict) D.add_representer(HashableOrderedDict, represent_ordereddict) From e7c879218456e4bd066fe7480ca0579b1d6c1672 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 19 Oct 2022 22:37:47 -0400 Subject: [PATCH 09/14] 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 88f6b3b8a93c..261bf09b5d1d 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -167,7 +167,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 1845cfbe82165a0cfa940243fcab49811547cda4 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 00:14:58 -0400 Subject: [PATCH 10/14] 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 261bf09b5d1d..0a2bfdc38753 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -155,8 +155,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 81cb55bbfff4ca96b8f5c2564f2ef0219263a68d Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 10 Nov 2022 19:46:07 -0500 Subject: [PATCH 11/14] 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 0a2bfdc38753..dfd0a621ddad 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -110,8 +110,6 @@ def represent_listproxy(dumper, data): D.add_representer( MaskedList, yaml.representer.SafeRepresenter.represent_list ) - # 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 IndentedSafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict) From 6b0076a2c7484f42a408458757c4fd457cd5427f Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 26 Oct 2022 22:48:02 -0400 Subject: [PATCH 12/14] 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 adcbc47cdbe8..8da27b8fd097 100644 --- a/tests/pytests/unit/modules/test_schedule.py +++ b/tests/pytests/unit/modules/test_schedule.py @@ -9,6 +9,7 @@ import pytest import salt.modules.schedule as schedule +import salt.utils.yaml from salt.utils.event import SaltEvent from tests.support.mock import MagicMock, call, mock_open, patch @@ -338,7 +339,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 f3ccf609871a..031734d01838 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.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 9d38770dc59f91769a7733a07002b43c860ba498 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Mon, 15 Jun 2026 15:42:09 -0700 Subject: [PATCH 13/14] changelog: rename to .fixed.md format --- changelog/{63158.fixed => 63158.fixed.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog/{63158.fixed => 63158.fixed.md} (100%) diff --git a/changelog/63158.fixed b/changelog/63158.fixed.md similarity index 100% rename from changelog/63158.fixed rename to changelog/63158.fixed.md From 64b11cd166a26a1b287453b7fbcd7805eb53fdbd Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Mon, 15 Jun 2026 15:43:21 -0700 Subject: [PATCH 14/14] pre-commit: fix black formatting in yamldumper.py --- salt/utils/yamldumper.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index dfd0a621ddad..1c04b5fcfa7d 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -104,12 +104,8 @@ def represent_listproxy(dumper, data): # Pillar containers are wrapped in MaskedDict / MaskedList for repr redaction; # they are still plain dict / list at the data level, so dump them as such # instead of falling through to represent_undefined (which would emit NULL). - D.add_representer( - MaskedDict, yaml.representer.SafeRepresenter.represent_dict - ) - D.add_representer( - MaskedList, yaml.representer.SafeRepresenter.represent_list - ) + D.add_representer(MaskedDict, yaml.representer.SafeRepresenter.represent_dict) + D.add_representer(MaskedList, yaml.representer.SafeRepresenter.represent_list) del D IndentedSafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict)