Repository: coverahealth/dataspec

Issue:
Keys marked with `s.opt` in a mapping Spec will be out of order after conformation
The default conformer for mapping Specs using `s.opt` always places optional keys at the end of the conformed map. No one should be relying on the ordering of the conformed keys, but it may still be worthwhile to preserve that key order since Python dicts preserve it.

Discussion:
chrisrink10: 1. I added a test. I'm not yet clear on whether I want to guarantee this behavior, so I was (and am) hesitant to encode it anywhere.

2. Merged Specs insertion orders are based in the order mapping Specs are passed into the `s.merge` function. The first map defines the starting insertion order and subsequent maps end up being inserted after the first map. If a key from a prior map appears in a later map, they will be `s.all`'ed together and insertion order for that key will be preserved.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1a66036..91cde72 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -29,6 +29,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   Previously, Spec factories such as `s.str` would inject tags for child validators
   such as `str_matches_regex` into `via`, making it difficult to programmatically
   determine which Spec the input value violated (#78)
+- Mapping spec default conformers will now use the same key insertion order as the
+  original mapping spec predicate. Optional keys will now retain their insertion
+  position, rather than being appended at the end of the conformed map. (#82)
 
 ### Fixed
 - Fixed a bug where `s(None)` is not a valid alias for `s(type(None))` (#61)
diff --git a/src/dataspec/base.py b/src/dataspec/base.py
index 6ff3923..7eb65e9 100644
--- a/src/dataspec/base.py
+++ b/src/dataspec/base.py
@@ -570,16 +570,24 @@ class CollSpec(Spec):
             yield from _enrich_errors(self._spec.validate(e), self.tag, i)
 
 
+T_hashable = TypeVar("T_hashable", bound=Hashable)
+
+
 @attr.s(auto_attribs=True, frozen=True, slots=_USE_SLOTS_FOR_GENERIC)
-class OptionalKey(Generic[T]):
-    key: T
+class OptionalKey(Generic[T_hashable]):
+    key: T_hashable
+
+
+@attr.s(auto_attribs=True, frozen=True, slots=True)
+class _KeySpec:
+    spec: Spec
+    is_optional: bool = False
 
 
 @attr.s(auto_attribs=True, frozen=True, slots=True)
 class DictSpec(Spec):
     tag: Tag
-    _reqkeyspecs: Mapping[Hashable, Spec] = attr.ib(factory=dict)
-    _optkeyspecs: Mapping[Hashable, Spec] = attr.ib(factory=dict)
+    _keyspecs: Mapping[Hashable, _KeySpec] = attr.ib(factory=dict)
     conformer: Optional[Conformer] = None
 
     @classmethod
@@ -589,53 +597,55 @@ class DictSpec(Spec):
         kvspec: Mapping[Hashable, SpecPredicate],
         conformer: Optional[Conformer] = None,
     ) -> Spec:
-        reqkeys = {}
-        optkeys: MutableMapping[Any, Spec] = {}
+        keyspecs = {}
         for k, v in kvspec.items():
             if isinstance(k, OptionalKey):
-                if k.key in reqkeys:
-                    raise KeyError(
-                        f"Optional key '{k.key}' duplicates key already defined in required keys"
-                    )
-                optkeys[k.key] = make_spec(v)
+                if k.key in keyspecs:
+                    raise KeyError(f"Optional key '{k.key}' duplicates existing key")
+                keyspecs[k.key] = _KeySpec(make_spec(v), is_optional=True)
             else:
-                if k in optkeys:
-                    raise KeyError(
-                        f"Required key '{k}' duplicates key already defined in optional keys"
-                    )
-                reqkeys[k] = make_spec(v)
+                if k in keyspecs:
+                    raise KeyError(f"Required key '{k}' duplicates existing key")
+                keyspecs[k] = _KeySpec(make_spec(v))
 
         def conform_mapping(d: Mapping) -> Mapping:
             conformed_d = {}
-            for k, spec in reqkeys.items():
-                conformed_d[k] = spec.conform(d[k])
-
-            for k, spec in optkeys.items():
-                if k in d:
-                    conformed_d[k] = spec.conform(d[k])
+            for k, keyspec in keyspecs.items():
+                if keyspec.is_optional:
+                    if k in d:
+                        conformed_d[k] = keyspec.spec.conform(d[k])
+                else:
+                    conformed_d[k] = keyspec.spec.conform(d[k])
 
             return conformed_d
 
         return cls(
             tag or "map",
-            reqkeyspecs=reqkeys,
-            optkeyspecs=optkeys,
+            keyspecs=keyspecs,
             conformer=compose_conformers(conform_mapping, conformer),
         )
 
     def validate(self, d) -> Iterator[ErrorDetails]:  # pylint: disable=arguments-differ
         try:
-            for k, vspec in self._reqkeyspecs.items():
-                if k in d:
-                    yield from _enrich_errors(vspec.validate(d[k]), self.tag, k)
+            for k, keyspec in self._keyspecs.items():
+                if keyspec.is_optional:
+                    if k in d:
+                        yield from _enrich_errors(
+                            keyspec.spec.validate(d[k]), self.tag, k
+                        )
                 else:
-                    yield ErrorDetails(
-                        message=f"Mapping missing key {k}",
-                        pred=vspec,
-                        value=d,
-                        via=[self.tag],
-                        path=[k],
-                    )
+                    if k in d:
+                        yield from _enrich_errors(
+                            keyspec.spec.validate(d[k]), self.tag, k
+                        )
+                    else:
+                        yield ErrorDetails(
+                            message=f"Mapping missing key {k}",
+                            pred=keyspec.spec,
+                            value=d,
+                            via=[self.tag],
+                            path=[k],
+                        )
         except (AttributeError, TypeError):
             yield ErrorDetails(
                 message="Value is not a mapping type",
@@ -645,10 +655,6 @@ class DictSpec(Spec):
             )
             return
 
-        for k, vspec in self._optkeyspecs.items():
-            if k in d:
-                yield from _enrich_errors(vspec.validate(d[k]), self.tag, k)
-
     # pylint: disable=protected-access
     @classmethod
     def merge(
@@ -661,11 +667,10 @@ class DictSpec(Spec):
 
         map_pred: MutableMapping[Hashable, List[SpecPredicate]] = defaultdict(list)
         for spec in specs:
-            for k, pred in spec._reqkeyspecs.items():
-                map_pred[k].append(pred)
-
-            for k, pred in spec._optkeyspecs.items():
-                map_pred[OptionalKey(k)].append(pred)
+            for k, keyspec in spec._keyspecs.items():
+                map_pred[OptionalKey(k) if keyspec.is_optional else k].append(
+                    keyspec.spec
+                )
 
         return cls.from_val(
             tag or f"merge-of-{'-'.join(spec.tag for spec in specs)}",


Test patch:
diff --git a/tests/test_base.py b/tests/test_base.py
index a8185c2..0834554 100644
--- a/tests/test_base.py
+++ b/tests/test_base.py
@@ -400,23 +400,35 @@ class TestDictSpecConformation:
                 s.opt("date_of_birth"): s.str(
                     "date_of_birth", format_="iso-date", conformer=fromisoformat
                 ),
+                "favorite_color": {"Red", "Green", "Blue"},
             }
         )
 
     def test_dict_conformation(self, dict_spec: Spec):
         conformed = dict_spec.conform(
-            {"first_name": "chris", "last_name": "rink", "date_of_birth": "1990-01-14"}
+            {
+                "first_name": "chris",
+                "last_name": "rink",
+                "date_of_birth": "1990-01-14",
+                "favorite_color": "Blue",
+            }
         )
         assert isinstance(conformed, dict)
         assert "Chris" == conformed["first_name"]
         assert "Rink" == conformed["last_name"]
         assert date(year=1990, month=1, day=14) == conformed["date_of_birth"]
+        assert ["first_name", "last_name", "date_of_birth", "favorite_color"] == list(
+            conformed.keys()
+        )
 
-        conformed = dict_spec.conform({"first_name": "peter", "last_name": "criss"})
+        conformed = dict_spec.conform(
+            {"first_name": "peter", "last_name": "criss", "favorite_color": "Red"}
+        )
         assert isinstance(conformed, dict)
         assert "Peter" == conformed["first_name"]
         assert "Criss" == conformed["last_name"]
         assert None is conformed.get("date_of_birth")
+        assert ["first_name", "last_name", "favorite_color"] == list(conformed.keys())
 
 
 class TestKVSpecValidation: