Document replacement: Layering dependency integration

This PS integrates document replacement with document layering. The case
works something like this:

  GIVEN:
  - Parent A
  - Child B
  - Child C

  WHEN:
  - Child B is a replacement for A

  THEN:
  - B must layer with A, then C must layer with B, rather than A,
    as B replaces A.

This is the most basic scenario and there are certainly far more
intricate ones, involving interplay with substitution as well.

To implement this new functionality, relatively minor coding changes
were made, mostly in whether to consider a document's parent or its
parent's replacement while layering, as well as determining the
dependency chain for document sorting.

Unit tests surrounding replacement have been moved into their own files
and a scenario has been added for the case described above. In addition
the same case is tested via a functional test scenario.

The unit tests have been "hardened" to run the layering scenarios twice:
once by passing in the documents in their original order, an order which
is usually written for human maintainability (i.e. B depends on A, so
make the order A followed by B). However, in reality the order of the
documents will be randomized, so every layering unit test is also
run a second time with the documents in reverse order to better ensure
that the dependency chain is resolved correctly.

Change-Id: Ieb058267f3a46b78e899922b6bc5fd726ed15a1b

deckhand/db/sqlalchemy/api.py

@@ -303,20 +303,23 @@ def _documents_create(bucket_name, documents, session=None):
     return changed_documents
 
 
-def _fill_in_metadata_defaults(values):
-    values['meta'] = values.pop('metadata')
-    values['name'] = values['meta']['name']
+def _fill_in_metadata_defaults(document):
+    document['meta'] = document.pop('metadata')
+    document['name'] = document['meta']['name']
 
-    if not values['meta'].get('storagePolicy', None):
-        values['meta']['storagePolicy'] = 'cleartext'
+    if not document['meta'].get('storagePolicy', None):
+        document['meta']['storagePolicy'] = 'cleartext'
 
-    values['meta'].setdefault('layeringDefinition', {})
-    values['layer'] = values['meta']['layeringDefinition'].get('layer')
+    document['meta'].setdefault('layeringDefinition', {})
+    document['layer'] = document['meta']['layeringDefinition'].get('layer')
 
-    if 'abstract' not in values['meta']['layeringDefinition']:
-        values['meta']['layeringDefinition']['abstract'] = False
+    if 'abstract' not in document['meta']['layeringDefinition']:
+        document['meta']['layeringDefinition']['abstract'] = False
 
-    return values
+    if 'replacement' not in document['meta']:
+        document['meta']['replacement'] = False
+
+    return document
 
 
 def _make_hash(data):

deckhand/engine/layering.py

@@ -58,31 +58,66 @@ class DocumentLayering(object):
 
     def _calc_replacements_and_substitutions(
             self, substitution_sources):
+
+        def _check_document_with_replacement_field_has_parent(
+                parent_meta, parent, document):
+            if not parent_meta or not parent:
+                error_message = (
+                    'Document replacement requires that the document with '
+                    '`replacement: true` have a parent.')
+                raise errors.InvalidDocumentReplacement(
+                    schema=document.schema, name=document.name,
+                    layer=document.layer, reason=error_message)
+
+        def _check_replacement_and_parent_same_schema_and_name(
+                parent, document):
+            # This checks that a document can only be a replacement for
+            # another document with the same `metadata.name` and `schema`.
+            if not (document.schema == parent.schema and
+                    document.name == parent.name):
+                error_message = (
+                    'Document replacement requires that both documents '
+                    'have the same `schema` and `metadata.name`.')
+                raise errors.InvalidDocumentReplacement(
+                    schema=document.schema, name=document.name,
+                    layer=document.layer, reason=error_message)
+
+        def _check_non_replacement_and_parent_different_schema_and_name(
+                parent, document):
+            if (parent and document.schema == parent.schema and
+                    document.name == parent.name):
+                error_message = (
+                    'Non-replacement documents cannot have the same `schema` '
+                    'and `metadata.name` as their parent. Either add '
+                    '`replacement: true` to the document or give the document '
+                    'a different name.')
+                raise errors.InvalidDocumentReplacement(
+                    schema=document.schema, name=document.name,
+                    layer=document.layer, reason=error_message)
+
+        def _check_replacement_not_itself_replaced_by_another(src_ref):
+            # If the document has a replacement, use the replacement as the
+            # substitution source instead.
+            if src_ref.is_replacement:
+                error_message = ('A replacement document cannot itself'
+                                 ' be replaced by another document.')
+                raise errors.InvalidDocumentReplacement(
+                    schema=src_ref.schema, name=src_ref.name,
+                    layer=src_ref.layer, reason=error_message)
+
         for document in self._documents_by_index.values():
+            parent_meta = self._parents.get(document.meta)
+            parent = self._documents_by_index.get(parent_meta)
+
             if document.is_replacement:
-                parent_meta = self._parents.get(document.meta)
-                parent = self._documents_by_index.get(parent_meta)
-
-                if not parent_meta or not parent:
-                    error_message = (
-                        'Document replacement requires that the document with '
-                        '`replacement: true` have a parent.')
-                    raise errors.InvalidDocumentReplacement(
-                        schema=document.schema, name=document.name,
-                        layer=document.layer, reason=error_message)
-
-                # This checks that a document can only be a replacement for
-                # another document with the same `metadata.name` and `schema`.
-                if (document.schema == parent.schema and
-                        document.name == parent.name):
-                    parent.replaced_by = document
-                else:
-                    error_message = (
-                        'Document replacement requires that both documents '
-                        'have the same `schema` and `metadata.name`.')
-                    raise errors.InvalidDocumentReplacement(
-                        schema=document.schema, name=document.name,
-                        layer=document.layer, reason=error_message)
+                _check_document_with_replacement_field_has_parent(
+                    parent_meta, parent, document)
+                _check_replacement_and_parent_same_schema_and_name(
+                    parent, document)
+                parent.replaced_by = document
+            else:
+                _check_non_replacement_and_parent_different_schema_and_name(
+                    parent, document)
 
         # Since a substitution source only provides the document's
         # `metadata.name` and `schema`, their tuple acts as the dictionary key.
@@ -94,16 +129,8 @@ class DocumentLayering(object):
             src_ref = document_wrapper.DocumentDict(src)
             if src_ref.meta in self._documents_by_index:
                 src_ref = self._documents_by_index[src_ref.meta]
-                # If the document has a replacement, use the replacement as the
-                # substitution source instead.
                 if src_ref.has_replacement:
-                    if src_ref.is_replacement:
-                        error_message = ('A replacement document cannot itself'
-                                         ' be replaced by another document.')
-                        raise errors.InvalidDocumentReplacement(
-                            schema=src_ref.schema, name=src_ref.name,
-                            layer=src_ref.layer, reason=error_message)
-
+                    _check_replacement_not_itself_replaced_by_another(src_ref)
                     src_ref = src_ref.replaced_by
             substitution_source_map[(src_ref.schema, src_ref.name)] = src_ref
 
@@ -272,10 +299,12 @@ class DocumentLayering(object):
 
         g = networkx.DiGraph()
         for document in self._documents_by_index.values():
-            if document.parent_selector:
-                parent = self._parents.get(document.meta)
-                if parent:
-                    g.add_edge(document.meta, parent)
+            if document.has_replacement:
+                g.add_edge(document.meta, document.replaced_by.meta)
+            elif document.parent_selector and not document.is_replacement:
+                parent_meta = self._parents.get(document.meta)
+                if parent_meta:
+                    g.add_edge(document.meta, parent_meta)
 
             for sub in document.substitutions:
                 # Retrieve the correct substitution source using
@@ -556,6 +585,22 @@ class DocumentLayering(object):
 
         return overall_data
 
+    def _get_parent_or_replacement(self, doc, parent_meta):
+        """Returns the document's parent or the document's parent's
+        replacement.
+
+        :param DocumentDict doc: Document used to get parent.
+        :param tuple parent_meta: Unique parent identifier.
+        :returns: Parent document or its replacement if the parent has one.
+        :rtype: DocumentDict
+        """
+        parent = self._documents_by_index.get(parent_meta)
+        # Return the parent's replacement, but if that replacement is the
+        # document itself then return the parent.
+        if parent and parent.has_replacement and parent.replaced_by is not doc:
+            parent = parent.replaced_by
+        return parent
+
     def render(self):
         """Perform layering on the list of documents passed to ``__init__``.
 
@@ -582,7 +627,7 @@ class DocumentLayering(object):
                 parent_meta = self._parents.get(doc.meta)
 
                 if parent_meta:
-                    parent = self._documents_by_index[parent_meta]
+                    parent = self._get_parent_or_replacement(doc, parent_meta)
 
                     if doc.actions:
                         rendered_data = parent

deckhand/tests/functional/gabbits/document-render-success-single-bucket-with-replacement.yaml

@@ -3,6 +3,11 @@
 # 1. Purges existing data to ensure test isolation
 # 2. Adds initial documents with one site that that replaces its parent
 # 3. Verify that replacement of a substitution source works
+# 4. Adds initial documents with one replacement site and one non-replacement
+#    site document.
+# 5. Verify that the replacement document replaces its parent and that the
+#    non-replacement site document in effect layers with the replacement
+#    document.
 
 defaults:
   request_headers:
@@ -17,7 +22,7 @@ tests:
     status: 204
     response_headers: null
 
-  - name: create_documents_for_validating_replacement
+  - name: create_documents_for_validating_replacement_of_sub_source
     desc: |
       Create documents for validating replacement of a substitution source.
     PUT: /api/v1.0/buckets/mop/documents
@@ -91,9 +96,7 @@ tests:
       is used as the substitution source.
     GET: /api/v1.0/revisions/$RESPONSE['$.[0].status.revision']/rendered-documents
     query_parameters:
-      sort:
-        - metadata.name
-        - metadata.layeringDefinition.layer
+      sort: metadata.name
     status: 200
     response_multidoc_jsonpaths:
       $.`len`: 3
@@ -115,3 +118,128 @@ tests:
             bar: override
 
       $.[2].metadata.name: layering-policy
+
+  - name: create_documents_for_validating_replacement_of_layering_source
+    desc: |
+      Create documents for validating replacement of a layering source.
+    PUT: /api/v1.0/buckets/mop/documents
+    status: 200
+    data: |
+      ---
+      schema: deckhand/LayeringPolicy/v1
+      metadata:
+        schema: metadata/Control/v1
+        name: layering-policy
+      data:
+        layerOrder:
+          - global
+          - site
+      ---
+      schema: aic/Versions/v1
+      metadata:
+        schema: metadata/Document/v1
+        name: a
+        labels:
+          selector: foo
+        layeringDefinition:
+          abstract: False
+          layer: global
+      data:
+        conf:
+          foo: default
+      ---
+      schema: aic/Versions/v1
+      metadata:
+        schema: metadata/Document/v1
+        name: a
+        labels:
+          selector: baz
+        replacement: true
+        layeringDefinition:
+          abstract: False
+          layer: site
+          parentSelector:
+            selector: foo
+          actions:
+            - method: merge
+              path: .
+      data:
+        conf:
+          bar: override
+      ---
+      schema: aic/Versions/v1
+      metadata:
+        schema: metadata/Document/v1
+        name: b
+        labels:
+          selector: qux
+        layeringDefinition:
+          abstract: False
+          layer: site
+          parentSelector:
+            selector: foo
+          actions:
+            - method: merge
+              path: .
+      data:
+        conf:
+          baz: another
+      ---
+      schema: armada/Chart/v1
+      metadata:
+        schema: metadata/Document/v1
+        name: c
+        layeringDefinition:
+          abstract: False
+          layer: global
+        substitutions:
+          - src:
+              schema: aic/Versions/v1
+              name: a
+              path: .conf
+            dest:
+              path: .application.conf
+      data:
+        application:
+          conf: {}
+      ...
+
+  - name: verify_document_replaces_layering_source
+    desc: |
+      Check that document replacement works when a document replaces
+      its parent and yet another document layers with the same parent. In other
+      words, the non-replacement child document should in effect layer with the
+      other replacement child document.
+    GET: /api/v1.0/revisions/$RESPONSE['$.[0].status.revision']/rendered-documents
+    query_parameters:
+      sort: metadata.name
+    status: 200
+    response_multidoc_jsonpaths:
+      $.`len`: 4
+
+      $.[0].metadata.name: a
+      $.[0].metadata.layeringDefinition.layer: site
+      $.[0].metadata.replacement: true
+      $.[0].data:
+        conf:
+          foo: default
+          bar: override
+
+      $.[1].metadata.name: b
+      $.[1].metadata.layeringDefinition.layer: site
+      $.[1].metadata.replacement: false
+      $.[1].data:
+        conf:
+          foo: default
+          bar: override
+          baz: another
+
+      $.[2].metadata.name: c
+      $.[2].metadata.layeringDefinition.layer: global
+      $.[2].data:
+        application:
+          conf:
+            foo: default
+            bar: override
+
+      $.[3].metadata.name: layering-policy

deckhand/tests/unit/engine/test_document_layering.py

@@ -28,67 +28,86 @@ class TestDocumentLayering(test_base.DeckhandTestCase):
 
     def _test_layering(self, documents, site_expected=None,
                        region_expected=None, global_expected=None,
-                       validate=False, **kwargs):
-        document_layering = layering.DocumentLayering(
-            documents, validate=validate, **kwargs)
+                       validate=False, strict=True, **kwargs):
+        # TODO(fmontei): Refactor all tests to work with strict=True.
 
-        site_docs = []
-        region_docs = []
-        global_docs = []
+        # Test layering twice: once by passing in the documents in the normal
+        # order and again with the documents in reverse order for good measure,
+        # to verify that the documents are being correctly sorted by their
+        # substitution dependency chain.
+        for documents in (documents, list(reversed(documents))):
+            document_layering = layering.DocumentLayering(
+                documents, validate=validate, **kwargs)
 
-        # The layering policy is not returned as it is immutable. So all docs
-        # should have a metadata.layeringDefinitionn.layer section.
-        rendered_documents = document_layering.render()
-        for doc in rendered_documents:
-            # No need to validate the LayeringPolicy: it remains unchanged.
-            if doc['schema'].startswith(types.LAYERING_POLICY_SCHEMA):
-                continue
-            layer = doc['metadata']['layeringDefinition']['layer']
-            if layer == 'site':
-                site_docs.append(doc.get('data'))
-            if layer == 'region':
-                region_docs.append(doc.get('data'))
-            if layer == 'global':
-                global_docs.append(doc.get('data'))
+            site_docs = []
+            region_docs = []
+            global_docs = []
 
-        if site_expected is not None:
-            if not isinstance(site_expected, list):
-                site_expected = [site_expected]
+            # The layering policy is not returned as it is immutable. So all
+            # docs should have a metadata.layeringDefinitionn.layer section.
+            rendered_documents = document_layering.render()
+            for doc in rendered_documents:
+                # No need to validate the LayeringPolicy: it remains unchanged.
+                if doc['schema'].startswith(types.LAYERING_POLICY_SCHEMA):
+                    continue
+                layer = doc['metadata']['layeringDefinition']['layer']
+                if layer == 'site':
+                    site_docs.append(doc.get('data'))
+                if layer == 'region':
+                    region_docs.append(doc.get('data'))
+                if layer == 'global':
+                    global_docs.append(doc.get('data'))
 
-            for expected in site_expected:
-                self.assertIn(expected, site_docs)
-                idx = site_docs.index(expected)
-                self.assertEqual(expected, site_docs[idx],
-                                 'Actual site data does not match expected.')
-                site_docs.remove(expected)
-        else:
-            self.assertEmpty(site_docs)
+            if site_expected is not None:
+                if not isinstance(site_expected, list):
+                    site_expected = [site_expected]
 
-        if region_expected is not None:
-            if not isinstance(region_expected, list):
-                region_expected = [region_expected]
+                if strict:
+                    self.assertEqual(len(site_expected), len(site_docs))
 
-            for expected in region_expected:
-                self.assertIn(expected, region_docs)
-                idx = region_docs.index(expected)
-                self.assertEqual(expected, region_docs[idx],
-                                 'Actual region data does not match expected.')
-                region_docs.remove(expected)
-        else:
-            self.assertEmpty(region_docs)
+                for expected in site_expected:
+                    self.assertIn(expected, site_docs)
+                    idx = site_docs.index(expected)
+                    self.assertEqual(
+                        expected, site_docs[idx],
+                        'Actual site data does not match expected.')
+                    site_docs.remove(expected)
+            else:
+                self.assertEmpty(site_docs)
 
-        if global_expected is not None:
-            if not isinstance(global_expected, list):
-                global_expected = [global_expected]
+            if region_expected is not None:
+                if not isinstance(region_expected, list):
+                    region_expected = [region_expected]
 
-            for expected in global_expected:
-                self.assertIn(expected, global_docs)
-                idx = global_docs.index(expected)
-                self.assertEqual(expected, global_docs[idx],
-                                 'Actual global data does not match expected.')
-                global_docs.remove(expected)
-        else:
-            self.assertEmpty(global_docs)
+                if strict:
+                    self.assertEqual(len(region_expected), len(region_docs))
+
+                for expected in region_expected:
+                    self.assertIn(expected, region_docs)
+                    idx = region_docs.index(expected)
+                    self.assertEqual(
+                        expected, region_docs[idx],
+                        'Actual region data does not match expected.')
+                    region_docs.remove(expected)
+            else:
+                self.assertEmpty(region_docs)
+
+            if global_expected is not None:
+                if not isinstance(global_expected, list):
+                    global_expected = [global_expected]
+
+                if strict:
+                    self.assertEqual(len(global_expected), len(global_docs))
+
+                for expected in global_expected:
+                    self.assertIn(expected, global_docs)
+                    idx = global_docs.index(expected)
+                    self.assertEqual(
+                        expected, global_docs[idx],
+                        'Actual global data does not match expected.')
+                    global_docs.remove(expected)
+            else:
+                self.assertEmpty(global_docs)
 
 
 class TestDocumentLayeringScenarios(TestDocumentLayering):
@@ -1313,111 +1332,3 @@ class TestDocumentLayering3Layers2Regions2Sites(TestDocumentLayering):
         global_expected = None
         self._test_layering(documents, site_expected, region_expected,
                             global_expected)
-
-
-class TestDocumentLayeringWithReplacement(TestDocumentLayering):
-
-    def setUp(self):
-        super(TestDocumentLayeringWithReplacement, self).setUp()
-        self.documents = list(yaml.safe_load_all("""
----
-schema: deckhand/LayeringPolicy/v1
-metadata:
-  schema: metadata/Control/v1
-  name: layering-policy
-data:
-  layerOrder:
-    - global
-    - site
----
-schema: aic/Versions/v1
-metadata:
-  schema: metadata/Document/v1
-  name: a
-  labels:
-    selector: foo
-  layeringDefinition:
-    abstract: False
-    layer: global
-data:
-  conf:
-    foo: default
----
-schema: aic/Versions/v1
-metadata:
-  schema: metadata/Document/v1
-  name: a
-  labels:
-    selector: baz
-  replacement: true
-  layeringDefinition:
-    abstract: False
-    layer: site
-    parentSelector:
-      selector: foo
-    actions:
-      - method: merge
-        path: .
-data:
-  conf:
-    bar: override
----
-schema: armada/Chart/v1
-metadata:
-  schema: metadata/Document/v1
-  name: c
-  layeringDefinition:
-    abstract: False
-    layer: global
-  substitutions:
-    - src:
-        schema: aic/Versions/v1
-        name: a
-        path: .conf
-      dest:
-        path: .application.conf
-data:
-  application:
-    conf: {}
-...
-"""))
-
-    def test_basic_replacement(self):
-        """Verify that the replacement document is the only one returned."""
-        site_expected = [{"conf": {"foo": "default", "bar": "override"}}]
-        global_expected = None
-
-        self.documents = self.documents[:-1]
-
-        self._test_layering(self.documents, site_expected,
-                            global_expected=global_expected)
-
-    def test_replacement_with_substitution_from_replacer(self):
-        """Verify that using a replacement document as a substitution source
-        works.
-        """
-        site_expected = [{"conf": {"foo": "default", "bar": "override"}}]
-        global_expected = [
-            {"application": {"conf": {"foo": "default", "bar": "override"}}}]
-        # Pass in the replacee and replacer as substitution sources. The
-        # replacer should be used as the source.
-        self._test_layering(self.documents, site_expected,
-                            global_expected=global_expected,
-                            substitution_sources=self.documents[1:3])
-        # Attempt the same scenario but reverse the order of the substitution
-        # sources, which verifies that the replacer always takes priority.
-        self._test_layering(
-            self.documents, site_expected, global_expected=global_expected,
-            substitution_sources=list(reversed(self.documents[1:3])))
-
-        # Pass in the replacee as the only substitution source. The replacer
-        # should replace it and be used as the source.
-        self._test_layering(self.documents, site_expected,
-                            global_expected=global_expected,
-                            substitution_sources=[self.documents[1]])
-
-        # Pass in the replacer as the only substitution source, which should be
-        # used as the source.
-        self._test_layering(self.documents, site_expected,
-                            global_expected=global_expected,
-                            substitution_sources=[self.documents[2]])

deckhand/tests/unit/engine/test_document_layering_and_replacement.py

@@ -0,0 +1,214 @@
+# Copyright 2018 AT&T Intellectual Property.  All other rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import itertools
+import yaml
+
+from deckhand.tests.unit.engine import test_document_layering
+
+
+class TestDocumentLayeringWithReplacement(
+        test_document_layering.TestDocumentLayering):
+
+    def setUp(self):
+        super(TestDocumentLayeringWithReplacement, self).setUp()
+        self.documents = list(yaml.safe_load_all("""
+---
+schema: deckhand/LayeringPolicy/v1
+metadata:
+  schema: metadata/Control/v1
+  name: layering-policy
+data:
+  layerOrder:
+    - global
+    - site
+---
+schema: aic/Versions/v1
+metadata:
+  schema: metadata/Document/v1
+  name: a
+  labels:
+    selector: foo
+  layeringDefinition:
+    abstract: False
+    layer: global
+data:
+  conf:
+    foo: default
+---
+schema: aic/Versions/v1
+metadata:
+  schema: metadata/Document/v1
+  name: a
+  labels:
+    selector: baz
+  replacement: true
+  layeringDefinition:
+    abstract: False
+    layer: site
+    parentSelector:
+      selector: foo
+    actions:
+      - method: merge
+        path: .
+data:
+  conf:
+    bar: override
+---
+schema: armada/Chart/v1
+metadata:
+  schema: metadata/Document/v1
+  name: c
+  layeringDefinition:
+    abstract: False
+    layer: global
+  substitutions:
+    - src:
+        schema: aic/Versions/v1
+        name: a
+        path: .conf
+      dest:
+        path: .application.conf
+data:
+  application:
+    conf: {}
+...
+"""))
+
+    def test_basic_replacement(self):
+        """Verify that the replacement document is the only one returned."""
+        site_expected = [{"conf": {"foo": "default", "bar": "override"}}]
+        global_expected = None
+
+        self.documents = self.documents[:-1]
+
+        self._test_layering(self.documents, site_expected,
+                            global_expected=global_expected)
+
+    def test_replacement_with_substitution_from_replacer(self):
+        """Verify that using a replacement document as a substitution source
+        works.
+        """
+        site_expected = [{"conf": {"foo": "default", "bar": "override"}}]
+        global_expected = [
+            {"application": {"conf": {"foo": "default", "bar": "override"}}}]
+        # The replacer should be used as the source.
+        self._test_layering(self.documents, site_expected,
+                            global_expected=global_expected)
+        # Attempt the same scenario but reverse the order of the documents,
+        # which verifies that the replacer always takes priority.
+        self._test_layering(self.documents, site_expected,
+                            global_expected=global_expected)
+
+    def test_replacement_with_layering_with_replacer(self):
+        """Verify that replacement works alongside layering. This scenario
+        involves a parent, a child that replaces its parent, and yet another
+        child that layers with the replacement document.
+        """
+        self.documents = list(yaml.safe_load_all("""
+---
+schema: deckhand/LayeringPolicy/v1
+metadata:
+  schema: metadata/Control/v1
+  name: layering-policy
+data:
+  layerOrder:
+    - global
+    - site
+---
+schema: aic/Versions/v1
+metadata:
+  schema: metadata/Document/v1
+  name: a
+  labels:
+    selector: foo
+  layeringDefinition:
+    abstract: False
+    layer: global
+data:
+  conf:
+    foo: default
+---
+schema: aic/Versions/v1
+metadata:
+  schema: metadata/Document/v1
+  name: a
+  labels:
+    selector: baz
+  replacement: true
+  layeringDefinition:
+    abstract: False
+    layer: site
+    parentSelector:
+      selector: foo
+    actions:
+      - method: merge
+        path: .
+data:
+  conf:
+    bar: override
+---
+schema: aic/Versions/v1
+metadata:
+  schema: metadata/Document/v1
+  name: b
+  labels:
+    selector: qux
+  layeringDefinition:
+    abstract: False
+    layer: site
+    parentSelector:
+      selector: foo
+    actions:
+      - method: merge
+        path: .
+data:
+  conf:
+    baz: another
+---
+schema: armada/Chart/v1
+metadata:
+  schema: metadata/Document/v1
+  name: c
+  layeringDefinition:
+    abstract: False
+    layer: global
+  substitutions:
+    - src:
+        schema: aic/Versions/v1
+        name: a
+        path: .conf
+      dest:
+        path: .application.conf
+data:
+  application:
+    conf: {}
+...
+"""))
+
+        site_expected = [
+            {"conf": {"foo": "default", "bar": "override"}},
+            {"conf": {"foo": "default", "bar": "override", "baz": "another"}},
+        ]
+        global_expected = [
+            {"application": {"conf": {"foo": "default", "bar": "override"}}}
+        ]
+        self._test_layering(self.documents, site_expected=site_expected,
+                            global_expected=global_expected)
+
+        # Try different permutations of document orders for good measure.
+        for documents in list(itertools.permutations(self.documents))[:10]:
+            self._test_layering(
+                documents, site_expected=site_expected,
+                global_expected=global_expected)

deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py

@@ -0,0 +1,101 @@
+# Copyright 2018 AT&T Intellectual Property.  All other rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from deckhand import errors
+from deckhand import factories
+from deckhand.tests.unit.engine import test_document_layering
+
+
+class TestDocumentLayeringReplacementNegative(
+        test_document_layering.TestDocumentLayering):
+
+    def test_replacement_with_incompatible_name_or_schema_raises_exc(self):
+        """Validate that attempting to replace a child with its parent when
+        they don't have the same ``metadata.name`` and ``schema`` results in
+        exception.
+        """
+        doc_factory = factories.DocumentFactory(2, [1, 1])
+        documents = doc_factory.gen_test({})
+
+        # Validate case where names mismatch.
+        documents[1]['metadata']['name'] = 'foo'
+        documents[2]['metadata']['replacement'] = True
+        documents[2]['metadata']['name'] = 'bar'
+
+        error_re = (r'.*Document replacement requires that both documents '
+                     'have the same `schema` and `metadata.name`.')
+        self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
+                                self._test_layering, documents)
+
+        # Validate case where schemas mismatch.
+        documents[1]['metadata']['schema'] = 'example/Kind/v1'
+        documents[2]['metadata']['replacement'] = True
+        documents[2]['metadata']['schema'] = 'example/Other/v1'
+
+        error_re = (r'Document replacement requires that both documents '
+                     'have the same `schema` and `metadata.name`.')
+        self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
+                                self._test_layering, documents)
+
+    def test_non_replacement_same_name_and_schema_as_parent_raises_exc(self):
+        """Validate that a non-replacement document (i.e. regular document
+        without `replacement: true`) cannot have the same schema/name as
+        another document.
+        """
+        doc_factory = factories.DocumentFactory(2, [1, 1])
+        documents = doc_factory.gen_test({})
+
+        documents[1]['metadata']['name'] = 'foo'
+        documents[2]['metadata']['replacement'] = False
+        documents[2]['metadata']['name'] = 'foo'
+
+        error_re = (r'.*Non-replacement documents cannot have the same '
+                    '`schema` and `metadata.name`.*')
+        self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
+                                self._test_layering, documents)
+
+    def test_replacement_without_parent_raises_exc(self):
+        """Validate that attempting to do replacement without a parent document
+        raises an exception.
+        """
+        doc_factory = factories.DocumentFactory(2, [1, 1])
+        documents = doc_factory.gen_test({})
+
+        documents[2]['metadata']['replacement'] = True
+        documents[2]['metadata']['layeringDefinition'].pop('parentSelector')
+
+        error_re = (r'Document replacement requires that the document with '
+                     '`replacement: true` have a parent.')
+        self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
+                                self._test_layering, documents)
+
+    def test_replacement_that_is_replaced_raises_exc(self):
+        """Validate that attempting replace a replacement document raises an
+        exception.
+        """
+        doc_factory = factories.DocumentFactory(3, [1, 1, 1])
+        documents = doc_factory.gen_test({}, region_abstract=False,
+                                         site_abstract=False)
+
+        for document in documents[1:]:
+            document['metadata']['name'] = 'foo'
+            document['schema'] = 'example/Kind/v1'
+
+        documents[2]['metadata']['replacement'] = True
+        documents[3]['metadata']['replacement'] = True
+
+        error_re = (r'A replacement document cannot itself be replaced by '
+                     'another document.')
+        self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
+                                self._test_layering, documents)

deckhand/tests/unit/engine/test_document_layering_and_substitution.py

@@ -55,7 +55,7 @@ class TestDocumentLayeringWithSubstitution(
         site_expected = {'a': {'x': 1, 'y': 2}, 'b': 4, 'c': 'global-secret'}
 
         self._test_layering(documents, site_expected=site_expected,
-                            global_expected=global_expected)
+                            global_expected=global_expected, strict=False)
 
     def test_layering_and_substitution_no_children(self):
         """Validate that a document with no children undergoes substitution.
@@ -97,7 +97,7 @@ class TestDocumentLayeringWithSubstitution(
         site_expected = {'b': 4}
 
         self._test_layering(documents, site_expected=site_expected,
-                            global_expected=global_expected)
+                            global_expected=global_expected, strict=False)
 
     def test_substitution_without_parent_document(self):
         """Validate that a document with no parent undergoes substitution.
@@ -140,7 +140,7 @@ class TestDocumentLayeringWithSubstitution(
         site_expected = {'b': 4, 'c': 'site-secret'}
 
         self._test_layering(documents, site_expected=site_expected,
-                            global_expected=global_expected)
+                            global_expected=global_expected, strict=False)
 
     def test_parent_and_child_layering_and_substitution_different_paths(self):
         """Validate that parent and child documents both undergo layering and
@@ -196,7 +196,7 @@ class TestDocumentLayeringWithSubstitution(
 
         self._test_layering(
             documents, site_expected=site_expected,
-            global_expected=global_expected)
+            global_expected=global_expected, strict=False)
 
     def test_parent_and_child_layering_and_substitution_same_paths(self):
         """Validate that parent and child documents both undergo layering and
@@ -251,7 +251,7 @@ class TestDocumentLayeringWithSubstitution(
 
         self._test_layering(
             documents, site_expected=site_expected,
-            global_expected=global_expected)
+            global_expected=global_expected, strict=False)
 
     def test_parent_with_multi_child_layering_and_sub_different_paths(self):
         """Validate that parent and children documents both undergo layering
@@ -325,7 +325,7 @@ class TestDocumentLayeringWithSubstitution(
 
         self._test_layering(
             documents, site_expected=site_expected,
-            global_expected=global_expected)
+            global_expected=global_expected, strict=False)
 
     def test_parent_with_multi_child_layering_and_sub_same_path(self):
         """Validate that parent and children documents both undergo layering
@@ -399,7 +399,7 @@ class TestDocumentLayeringWithSubstitution(
 
         self._test_layering(
             documents, site_expected=site_expected,
-            global_expected=global_expected)
+            global_expected=global_expected, strict=False)
 
     def test_parent_with_multi_child_layering_and_multi_substitutions(self):
         """Validate that parent and children documents both undergo layering
@@ -505,7 +505,7 @@ class TestDocumentLayeringWithSubstitution(
 
         self._test_layering(
             documents, site_expected=site_expected,
-            global_expected=global_expected)
+            global_expected=global_expected, strict=False)
 
     @mock.patch('deckhand.engine.layering.LOG', autospec=True)
     def test_parent_and_child_undergo_layering_and_substitution_empty_layers(
@@ -576,7 +576,7 @@ class TestDocumentLayeringWithSubstitution(
 
         self._test_layering(
             documents, site_expected=site_expected,
-            global_expected=global_expected)
+            global_expected=global_expected, strict=False)
 
         expected_message = (
             '%s is an empty layer with no documents. It will be discarded '
@@ -666,15 +666,14 @@ class TestDocumentLayeringWithSubstitution(
 
         # Pass in the documents in reverse order to ensure that the dependency
         # chain by default is not linear and thus requires sorting.
-        self._test_layering(
-            list(reversed(documents)), site_expected=site_expected,
-            global_expected=global_expected)
+        self._test_layering(documents, site_expected=site_expected,
+                            global_expected=global_expected, strict=False)
 
         # Try different permutations of document orders for good measure.
-        for document_order in list(itertools.permutations(documents))[:10]:
+        for documents in list(itertools.permutations(documents))[:10]:
             self._test_layering(
-                document_order, site_expected=site_expected,
-                global_expected=global_expected)
+                documents, site_expected=site_expected,
+                global_expected=global_expected, strict=False)
 
     def test_layering_and_substitution_site_abstract_and_global_concrete(self):
         """Verifies that if a global document is abstract, yet has
@@ -711,4 +710,4 @@ class TestDocumentLayeringWithSubstitution(
                          "site": "stuff"}
         global_expected = None
         self._test_layering(documents, site_expected,
-                            global_expected=global_expected)
+                            global_expected=global_expected, strict=False)

deckhand/tests/unit/engine/test_document_layering_negative.py

@@ -186,7 +186,7 @@ class TestDocumentLayeringNegative(
         # warning to be raised.
         documents.append(documents[0])
         self._test_layering(documents, global_expected={})
-        mock_log.warning.assert_called_once_with(
+        mock_log.warning.assert_called_with(
             'More than one layering policy document was passed in. Using the '
             'first one found: [%s] %s.', documents[0]['schema'],
             documents[0]['metadata']['name'])
@@ -287,70 +287,3 @@ class TestDocumentLayeringValidationNegative(
         self.assertRaisesRegexp(
             errors.InvalidDocumentFormat, error_re, self._test_layering,
             [layering_policy, document], validate=True)
-
-
-class TestDocumentLayeringReplacementNegative(
-        test_document_layering.TestDocumentLayering):
-
-    def test_replacement_with_incompatible_name_or_schema_raises_exc(self):
-        """Validate that attempting to replace a child with its parent when
-        they don't have the same ``metadata.name`` and ``schema`` results in
-        exception.
-        """
-        doc_factory = factories.DocumentFactory(2, [1, 1])
-        documents = doc_factory.gen_test({})
-
-        # Validate case where names mismatch.
-        documents[1]['metadata']['name'] = 'foo'
-        documents[2]['metadata']['replacement'] = True
-        documents[2]['metadata']['name'] = 'bar'
-
-        error_re = (r'.*Document replacement requires that both documents '
-                     'have the same `schema` and `metadata.name`.')
-        self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
-                                self._test_layering, documents)
-
-        # Validate case where schemas mismatch.
-        documents[1]['metadata']['schema'] = 'example/Kind/v1'
-        documents[2]['metadata']['replacement'] = True
-        documents[2]['metadata']['schema'] = 'example/Other/v1'
-
-        error_re = (r'Document replacement requires that both documents '
-                     'have the same `schema` and `metadata.name`.')
-        self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
-                                self._test_layering, documents)
-
-    def test_replacement_without_parent_raises_exc(self):
-        """Validate that attempting to do replacement without a parent document
-        raises an exception.
-        """
-        doc_factory = factories.DocumentFactory(2, [1, 1])
-        documents = doc_factory.gen_test({})
-
-        documents[2]['metadata']['replacement'] = True
-        documents[2]['metadata']['layeringDefinition'].pop('parentSelector')
-
-        error_re = (r'Document replacement requires that the document with '
-                     '`replacement: true` have a parent.')
-        self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
-                                self._test_layering, documents)
-
-    def test_replacement_that_is_replaced_raises_exc(self):
-        """Validate that attempting replace a replacement document raises an
-        exception.
-        """
-        doc_factory = factories.DocumentFactory(3, [1, 1, 1])
-        documents = doc_factory.gen_test({})
-
-        for document in documents[1:]:
-            document['metadata']['name'] = 'foo'
-            document['schema'] = 'example/Kind/v1'
-
-        documents[2]['metadata']['replacement'] = True
-        documents[3]['metadata']['replacement'] = True
-
-        error_re = (r'A replacement document cannot itself be replaced by '
-                     'another document.')
-        self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
-                                self._test_layering, documents,
-                                substitution_sources=documents[1:])