YAML loading and dumping improvements #62932

I believe the behavior changed with commit 002aa88 which was first released in Salt v2018.3.0.

This demonstrates that saltstack#12161 has already been fixed (thanks to Python 3.6 changing `dict` to iterate in insertion order).

This does not change the behavior, but it does simplify the code.

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.

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.

The delayed `import` statements match `import` statements at the top of the file so they are effectively no-ops.

This commit is a prerequisite to adding version-selected behavior changes.

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.

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()`.

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 ```

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 <salt.utils.yamlloader.SaltYamlSafeLoader object at 0xc31a10> >>> y.load("!!omap [{foo: bar}, {baz: bif}]") # exact same as before Traceback (most recent call last): File "<stdin>", line 1, in <module> 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 "<unicode string>", 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 "<stdin>", line 1, in <module> 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 "<unicode string>", 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.)

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))) ```

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 "<stdin>", line 1, in <module> 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 "<unicode string>", 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: saltstack#12161

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: saltstack#12161

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 "<stdin>", line 1, in <module> 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 "<unicode string>", line 1, column 1 ``` Behavior after: ```pycon >>> import salt.utils.yaml as y >>> y.load("!!python/tuple [foo, bar]") ('foo', 'bar') ```

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 ```

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 ```

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 ```

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 ```

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 2e2ce5f. 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 "<stdin>", line 1, in <module> 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', <class '__main__.GeneratedObjectSubclass'>) ```

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

YAML loading and dumping improvements #62932

YAML loading and dumping improvements #62932

Commits on Dec 19, 2022