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

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`

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
 <https://yaml.org/type/omap>`_ 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
 ===============================

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

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"],
 )