From 4cc801bcc43c032c5cf64332861e4dbae916fb6b Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Sun, 28 Jan 2018 16:33:40 -0500 Subject: [PATCH] Allow parentSelector to use multiple labels to select parent document This PS updates the layering module so that a child document with a compound parentSelector like: [{'a': 'b'}, {'c': 'd'}] can select a parent with the exact same labels. Further, this works if child's parentSelector is a sub-subset of parent's labels. Adds unit tests for positive and negative test cases. Change-Id: I7c4aac583365d90803eda77b82763decd78cfdcf --- deckhand/engine/layering.py | 21 ++++-- .../design-doc-layering-sample-2-layers.yaml | 2 + .../unit/engine/test_document_layering.py | 68 +++++++++++++++++++ .../engine/test_document_layering_negative.py | 30 ++++++++ 4 files changed, 114 insertions(+), 7 deletions(-) diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index 63bbea41..bdb5ef05 100644 --- a/deckhand/engine/layering.py +++ b/deckhand/engine/layering.py @@ -55,7 +55,9 @@ class DocumentLayering(object): if is_potential_child: parent_selector = potential_child.parent_selector labels = document.labels - return parent_selector == labels + # Labels are key-value pairs which are unhashable, so use ``all`` + # instead. + return all(labels.get(x) == y for x, y in parent_selector.items()) return False def _calc_document_children(self, document): @@ -66,8 +68,12 @@ class DocumentLayering(object): # The lowest layer has been reached, so no children. return - potential_children = self._documents_by_labels.get( - str(document.labels), []) + potential_children = set() + for label_key, label_val in document.labels.items(): + _potential_children = self._documents_by_labels.get( + (label_key, label_val), []) + potential_children |= set(_potential_children) + for potential_child in potential_children: if self._is_actual_child_document(document, potential_child, child_layer): @@ -189,10 +195,11 @@ class DocumentLayering(object): self._documents_by_layer.setdefault(document.layer, []) self._documents_by_layer[document.layer].append(document) if document.parent_selector: - self._documents_by_labels.setdefault( - str(document.parent_selector), []) - self._documents_by_labels[ - str(document.parent_selector)].append(document) + for label_key, label_val in document.parent_selector.items(): + self._documents_by_labels.setdefault( + (label_key, label_val), []) + self._documents_by_labels[ + (label_key, label_val)].append(document) if self._layering_policy is None: error_msg = ( diff --git a/deckhand/tests/functional/gabbits/resources/design-doc-layering-sample-2-layers.yaml b/deckhand/tests/functional/gabbits/resources/design-doc-layering-sample-2-layers.yaml index 7d9122a7..fa1c306e 100644 --- a/deckhand/tests/functional/gabbits/resources/design-doc-layering-sample-2-layers.yaml +++ b/deckhand/tests/functional/gabbits/resources/design-doc-layering-sample-2-layers.yaml @@ -14,6 +14,7 @@ metadata: name: global-1234 labels: key1: value1 + key2: value2 layeringDefinition: abstract: true layer: global @@ -30,6 +31,7 @@ metadata: layer: site parentSelector: key1: value1 + key2: value2 actions: - method: merge path: . diff --git a/deckhand/tests/unit/engine/test_document_layering.py b/deckhand/tests/unit/engine/test_document_layering.py index ad1cb4b8..f0b81455 100644 --- a/deckhand/tests/unit/engine/test_document_layering.py +++ b/deckhand/tests/unit/engine/test_document_layering.py @@ -96,6 +96,74 @@ class TestDocumentLayering2Layers(TestDocumentLayering): site_expected = {'a': {'x': 1, 'y': 2}, 'b': 4} self._test_layering(documents, site_expected) + def test_layering_default_scenario_multi_parentselector(self): + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": {"x": 1, "y": 2}}}, + "_SITE_DATA_1_": {"data": {"b": 4}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "merge", "path": "."}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + # Test case where the same number of labels are found in parent + # labels and child's parentSelector. + labels = {'foo': 'bar', 'baz': 'qux'} + documents[1]['metadata']['labels'] = labels + documents[-1]['metadata']['layeringDefinition']['parentSelector'] = ( + labels) + site_expected = {'a': {'x': 1, 'y': 2}, 'b': 4} + self._test_layering(documents, site_expected) + + # Test case where child's parentSelector is a subset of parent's + # labels. + documents[-1]['metadata']['layeringDefinition']['parentSelector'] = { + 'foo': 'bar'} + site_expected = {'a': {'x': 1, 'y': 2}, 'b': 4} + self._test_layering(documents, site_expected) + + documents[-1]['metadata']['layeringDefinition']['parentSelector'] = { + 'baz': 'qux'} + site_expected = {'a': {'x': 1, 'y': 2}, 'b': 4} + self._test_layering(documents, site_expected) + + def test_layering_default_scenario_multi_parentselector_no_match(self): + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": {"x": 1, "y": 2}}}, + "_SITE_DATA_1_": {"data": {"b": 4}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "merge", "path": "."}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + labels = {'a': 'b', 'c': 'd'} + documents[1]['metadata']['labels'] = labels + + # Test case where none of the labels in parentSelector match. + documents[-1]['metadata']['layeringDefinition']['parentSelector'] = { + 'w': 'x', 'y': 'z' + } + self._test_layering(documents, site_expected={}) + + # Test case where parentSelector has one too many labels to be a match. + documents[-1]['metadata']['layeringDefinition']['parentSelector'] = { + 'a': 'b', 'c': 'd', 'e': 'f' + } + self._test_layering(documents, site_expected={}) + + # Test case where parentSelector keys match (but not values). + documents[-1]['metadata']['layeringDefinition']['parentSelector'] = { + 'a': 'x', 'c': 'y' + } + self._test_layering(documents, site_expected={}) + + # Test case where parentSelector values match (but not keys). + documents[-1]['metadata']['layeringDefinition']['parentSelector'] = { + 'x': 'b', 'y': 'd' + } + self._test_layering(documents, site_expected={}) + def test_layering_method_delete(self): site_expected = [{}, {'c': 9}, {"a": {"x": 1, "y": 2}}] doc_factory = factories.DocumentFactory(2, [1, 1]) diff --git a/deckhand/tests/unit/engine/test_document_layering_negative.py b/deckhand/tests/unit/engine/test_document_layering_negative.py index 9f8fb0b3..8e73edd5 100644 --- a/deckhand/tests/unit/engine/test_document_layering_negative.py +++ b/deckhand/tests/unit/engine/test_document_layering_negative.py @@ -84,6 +84,23 @@ class TestDocumentLayeringNegative( ' during the layering process.') mock_log.info.reset_mock() + @mock.patch.object(layering, 'LOG', autospec=True) + def test_layering_with_invalid_layer(self, mock_log): + doc_factory = factories.DocumentFactory(1, [1]) + documents = doc_factory.gen_test({}, site_abstract=False) + documents[-1]['metadata']['layeringDefinition']['layer'] = 'invalid' + + self._test_layering(documents, global_expected={}) + mock_log.info.assert_has_calls([ + mock.call( + '%s is an empty layer with no documents. It will be discarded ' + 'from the layerOrder during the layering process.', 'global'), + mock.call('Either the layerOrder in the LayeringPolicy was empty ' + 'to begin with or no document layers were found in the ' + 'layerOrder, causing it to become empty. No layering ' + 'will be performed.') + ]) + @mock.patch.object(layering, 'LOG', autospec=True) def test_layering_child_with_invalid_parent_selector(self, mock_log): doc_factory = factories.DocumentFactory(2, [1, 1]) @@ -174,3 +191,16 @@ class TestDocumentLayeringNegative( documents = doc_factory.gen_test({}, site_abstract=False)[1:] self.assertRaises(errors.LayeringPolicyNotFound, layering.DocumentLayering, documents) + + @mock.patch.object(layering, 'LOG', autospec=True) + def test_multiple_layering_policy_logs_warning(self, mock_log): + doc_factory = factories.DocumentFactory(1, [1]) + documents = doc_factory.gen_test({}, site_abstract=False) + # Copy the same layering policy so that 2 are passed in, causing a + # warning to be raised. + documents.append(documents[0]) + self._test_layering(documents, site_expected={}) + mock_log.warning.assert_called_once_with( + 'More than one layering policy document was passed in. Using the ' + 'first one found: [%s] %s.', documents[0]['schema'], + documents[0]['metadata']['name'])