Repository: remind101/stacker
Pull request #507 (2017-11-15 00:53:02)

Allow duplicate keys for most mappings except stacks.
	modified:   stacker/tests/test_config.py
	modified:   stacker/util.py

acmcelwee: Is this to allow defaults with overrides or something along those lines? If so, what about warn on dupes if they're not in the hard fail set of keys?
russellballestrini: Yeah, I prefer my original attempt to fix this issue.

It was found that some end users may be setting a default for a some keys and then redefine the keys later on to override.

https://github.com/remind101/stacker/pull/500

The YAML spec is sort of confusing. It seems to indicate that keys "MUST" be unique in some places.

```
JSON's RFC4627 requires that mappings keys merely “SHOULD” be unique, while YAML insists they “MUST” be. Technically, YAML therefore complies with the JSON spec, choosing to treat duplicates as an error. In practice, since JSON is silent on the semantics of such duplicates, the only portable JSON files are those with unique keys, which are therefore valid YAML files.
```

Reference:  http://www.yaml.org/spec/1.2/spec.html section 1.3. Relation to JSON



Patch:
diff --git a/stacker/util.py b/stacker/util.py
index 3301d4b..bcc5d81 100644
--- a/stacker/util.py
+++ b/stacker/util.py
@@ -313,6 +313,25 @@ def yaml_to_ordered_dict(stream, loader=yaml.SafeLoader):
         Returns an OrderedDict instead of a Dict.
         """
 
+        # keys which require no duplicate siblings.
+        NO_DUPE_SIBLINGS = ["stacks", "class_path"]
+        # keys which require no duplicate children keys.
+        NO_DUPE_CHILDREN = ["stacks"]
+
+        def _error_mapping_on_dupe(self, node, node_name):
+            """check mapping node for dupe children keys."""
+            if isinstance(node, MappingNode):
+                mapping = {}
+                for n in node.value:
+                    a = n[0]
+                    b = mapping.get(a.value, None)
+                    if b:
+                        msg = "{} mapping cannot have duplicate keys {} {}"
+                        raise ConstructorError(
+                            msg.format(node_name, b.start_mark, a.start_mark)
+                        )
+                    mapping[a.value] = a
+
         def _validate_mapping(self, node, deep=False):
             if not isinstance(node, MappingNode):
                 raise ConstructorError(
@@ -329,12 +348,15 @@ def yaml_to_ordered_dict(stream, loader=yaml.SafeLoader):
                         "while constructing a mapping", node.start_mark,
                         "found unhashable key (%s)" % exc, key_node.start_mark
                     )
-                # check for duplicate keys
-                if key in mapping:
+                # prevent duplicate sibling keys for certain "keywords".
+                if key in mapping and key in self.NO_DUPE_SIBLINGS:
+                    msg = "{} key cannot have duplicate siblings {} {}"
                     raise ConstructorError(
-                        "while constructing a mapping", node.start_mark,
-                        "found duplicate key", key_node.start_mark
+                        msg.format(key, node.start_mark, key_node.start_mark)
                     )
+                if key in self.NO_DUPE_CHILDREN:
+                    # prevent duplicate children keys for this mapping.
+                    self._error_mapping_on_dupe(value_node, key_node.value)
                 value = self.construct_object(value_node, deep=deep)
                 mapping[key] = value
             return mapping


Test patch:
diff --git a/stacker/tests/test_config.py b/stacker/tests/test_config.py
index ab96862..da837fa 100644
--- a/stacker/tests/test_config.py
+++ b/stacker/tests/test_config.py
@@ -437,7 +437,24 @@ stacks:
         config.validate()
         self.assertEquals(config.namespace, "prod")
 
-    def test_raise_constructor_error_on_duplicate_key(self):
+    def test_allow_most_keys_to_be_duplicates_for_overrides(self):
+        yaml_config = """
+        namespace: prod
+        stacks:
+          - name: vpc
+            class_path: blueprints.VPC
+            variables:
+              CIDR: 192.168.1.0/24
+              CIDR: 192.168.2.0/24
+        """
+        doc = parse(yaml_config)
+        self.assertEqual(
+            doc["stacks"][0]["variables"]["CIDR"], "192.168.2.0/24"
+        )
+
+    def test_raise_constructor_error_on_keyword_duplicate_key(self):
+        """Some keys should never have a duplicate sibling. For example we
+        treat `class_path` as a special "keyword" and disallow dupes."""
         yaml_config = """
         namespace: prod
         stacks:
@@ -448,7 +465,9 @@ stacks:
         with self.assertRaises(ConstructorError):
             parse(yaml_config)
 
-    def test_raise_construct_error_on_duplicate_stack_name(self):
+    def test_raise_construct_error_on_duplicate_stack_name_dict(self):
+        """Some mappings should never have a duplicate children. For example we
+        treat `stacks` as a special mapping and disallow dupe children keys."""
         yaml_config = """
         namespace: prod
         stacks: