diff --git a/deckhand/db/sqlalchemy/api.py b/deckhand/db/sqlalchemy/api.py index 43b5a855..f3a34ac8 100644 --- a/deckhand/db/sqlalchemy/api.py +++ b/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): diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index fbba9548..557d4de2 100644 --- a/deckhand/engine/layering.py +++ b/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 diff --git a/deckhand/tests/functional/gabbits/document-render-success-single-bucket-with-replacement.yaml b/deckhand/tests/functional/gabbits/document-render-success-single-bucket-with-replacement.yaml index 2fb16383..3e74e9fc 100644 --- a/deckhand/tests/functional/gabbits/document-render-success-single-bucket-with-replacement.yaml +++ b/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 diff --git a/deckhand/tests/unit/engine/test_document_layering.py b/deckhand/tests/unit/engine/test_document_layering.py index 2c559ff4..ca6e0046 100644 --- a/deckhand/tests/unit/engine/test_document_layering.py +++ b/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]]) diff --git a/deckhand/tests/unit/engine/test_document_layering_and_replacement.py b/deckhand/tests/unit/engine/test_document_layering_and_replacement.py new file mode 100644 index 00000000..f8957928 --- /dev/null +++ b/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) diff --git a/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py b/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py new file mode 100644 index 00000000..95a5fb9b --- /dev/null +++ b/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) diff --git a/deckhand/tests/unit/engine/test_document_layering_and_substitution.py b/deckhand/tests/unit/engine/test_document_layering_and_substitution.py index 2f45d0df..a7233d1c 100644 --- a/deckhand/tests/unit/engine/test_document_layering_and_substitution.py +++ b/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) diff --git a/deckhand/tests/unit/engine/test_document_layering_negative.py b/deckhand/tests/unit/engine/test_document_layering_negative.py index bb64650e..1fec64fd 100644 --- a/deckhand/tests/unit/engine/test_document_layering_negative.py +++ b/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:])