From b9845fa72c079dadb05e741aa9134cf03bf3ce48 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Thu, 1 Mar 2018 22:14:00 +0000 Subject: [PATCH] Allow layering paths to include numeric indices This PS makes updates to layering merge actions to allow for numeric indices to work. jsonpath_* utility methods are used instead for merging and replacing. Added unit tests to verify that layering scenarios for replace, merge and delete work with numeric indices. Change-Id: Id6d592231cb90144bb5857bee48cecf9f6478692 --- deckhand/engine/layering.py | 109 +++++++------ deckhand/engine/utils.py | 35 +++++ .../unit/engine/test_document_layering.py | 147 ++++++++++++++++++ 3 files changed, 236 insertions(+), 55 deletions(-) diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index 585ede84..e3da005d 100644 --- a/deckhand/engine/layering.py +++ b/deckhand/engine/layering.py @@ -26,6 +26,7 @@ from deckhand.engine import secrets_manager from deckhand.engine import utils as engine_utils from deckhand import errors from deckhand import types +from deckhand import utils LOG = logging.getLogger(__name__) @@ -46,7 +47,8 @@ class DocumentLayering(object): together into a fully rendered document. """ - SUPPORTED_METHODS = ('merge', 'replace', 'delete') + _SUPPORTED_METHODS = (_MERGE_ACTION, _REPLACE_ACTION, _DELETE_ACTION) = ( + 'merge', 'replace', 'delete') def _replace_older_parent_with_younger_parent(self, child, parent, all_children): @@ -369,78 +371,75 @@ class DocumentLayering(object): Supported actions include: - * `merge` - a "deep" merge that layers new and modified data onto + * ``merge`` - a "deep" merge that layers new and modified data onto existing data - * `replace` - overwrite data at the specified path and replace it + * ``replace`` - overwrite data at the specified path and replace it with the data given in this document - * `delete` - remove the data at the specified path + * ``delete`` - remove the data at the specified path :raises UnsupportedActionMethod: If the layering action isn't found among ``self.SUPPORTED_METHODS``. :raises MissingDocumentKey: If a layering action path isn't found - in both the parent and child documents being layered together. + in the child document. """ method = action['method'] - if method not in self.SUPPORTED_METHODS: + if method not in self._SUPPORTED_METHODS: raise errors.UnsupportedActionMethod( action=action, document=child_data) # Use copy to prevent these data from being updated referentially. overall_data = copy.deepcopy(overall_data) child_data = copy.deepcopy(child_data) - rendered_data = overall_data - # Remove empty string paths and ensure that "data" is always present. - path = action['path'].split('.') - path = [p for p in path if p != ''] - path.insert(0, 'data') - last_key = 'data' if not path[-1] else path[-1] + action_path = action['path'] + if action_path.startswith('.data'): + action_path = action_path[5:] - for attr in path: - if attr == path[-1]: - break - rendered_data = rendered_data.get(attr) - child_data = child_data.get(attr) - - if method == 'delete': - # If the entire document is passed (i.e. the dict including - # metadata, data, schema, etc.) then reset data to an empty dict. - if last_key == 'data': - rendered_data['data'] = {} - elif last_key in rendered_data: - del rendered_data[last_key] - elif last_key not in rendered_data: - # If the key does not exist in `rendered_data`, this is a - # validation error. - raise errors.MissingDocumentKey( - child=child_data, parent=rendered_data, key=last_key) - elif method == 'merge': - if last_key in rendered_data and last_key in child_data: - # If both entries are dictionaries, do a deep merge. Otherwise - # do a simple merge. - if (isinstance(rendered_data[last_key], dict) - and isinstance(child_data[last_key], dict)): - engine_utils.deep_merge( - rendered_data[last_key], child_data[last_key]) - else: - rendered_data.setdefault(last_key, child_data[last_key]) - elif last_key in child_data: - rendered_data.setdefault(last_key, child_data[last_key]) + if method == self._DELETE_ACTION: + if action_path == '.': + overall_data.data = {} else: - # If the key does not exist in the child document, this is a - # validation error. + from_child = utils.jsonpath_parse(overall_data.data, + action_path) + if from_child is None: + raise errors.MissingDocumentKey( + child=child_data.data, + parent=overall_data.data, + key=action_path) + + engine_utils.deep_delete(from_child, overall_data.data, None) + + elif method == self._MERGE_ACTION: + from_parent = utils.jsonpath_parse(overall_data.data, action_path) + from_child = utils.jsonpath_parse(child_data.data, action_path) + + if from_child is None: raise errors.MissingDocumentKey( - child=child_data, parent=rendered_data, key=last_key) - elif method == 'replace': - if last_key in rendered_data and last_key in child_data: - rendered_data[last_key] = child_data[last_key] - elif last_key in child_data: - rendered_data.setdefault(last_key, child_data[last_key]) - elif last_key not in child_data: - # If the key does not exist in the child document, this is a - # validation error. + child=child_data.data, + parent=overall_data.data, + key=action_path) + + if (isinstance(from_parent, dict) + and isinstance(from_child, dict)): + engine_utils.deep_merge(from_parent, from_child) + + if from_parent is not None: + overall_data.data = utils.jsonpath_replace( + overall_data.data, from_parent, action_path) + else: + overall_data.data = utils.jsonpath_replace( + overall_data.data, from_child, action_path) + elif method == self._REPLACE_ACTION: + from_child = utils.jsonpath_parse(child_data.data, action_path) + + if from_child is None: raise errors.MissingDocumentKey( - child=child_data, parent=rendered_data, key=last_key) + child=child_data.data, + parent=overall_data.data, + key=action_path) + + overall_data.data = utils.jsonpath_replace( + overall_data.data, from_child, action_path) return overall_data @@ -466,10 +465,10 @@ class DocumentLayering(object): if doc.parent_selector: parent_meta = self._parents.get((doc.schema, doc.name)) - # Apply each action to the current document. if parent_meta: parent = self._documents_by_index[parent_meta] rendered_data = parent + # Apply each action to the current document. for action in doc.actions: LOG.debug('Applying action %s to document with ' 'name=%s, schema=%s, layer=%s.', action, diff --git a/deckhand/engine/utils.py b/deckhand/engine/utils.py index b821a461..0d05ee63 100644 --- a/deckhand/engine/utils.py +++ b/deckhand/engine/utils.py @@ -34,3 +34,38 @@ def deep_merge(dct, merge_dct): deep_merge(dct[k], merge_dct[k]) else: dct[k] = merge_dct[k] + + +def deep_delete(target, value, parent): + """Recursively search for then delete ``target`` from ``parent``. + + :param target: Target value to remove. + :param value: Current value in a list or dict to compare against + ``target`` and removed from ``parent`` given match. + :param parent: Tracks the parent data structure from which ``value`` + is removed. + :type parent: list or dict + :returns: Whether ``target`` was found. + :rtype: bool + """ + + if value == target: + if isinstance(parent, list): + parent.remove(value) + return True + elif isinstance(parent, dict): + for k, v in parent.items(): + if v == value: + parent.pop(k) + return True + elif isinstance(value, list): + for v in value: + found = deep_delete(target, v, value) + if found: + return True + elif isinstance(value, dict): + for v in value.values(): + found = deep_delete(target, v, value) + if found: + return True + return False diff --git a/deckhand/tests/unit/engine/test_document_layering.py b/deckhand/tests/unit/engine/test_document_layering.py index 0b42ce1c..2e9ccd36 100644 --- a/deckhand/tests/unit/engine/test_document_layering.py +++ b/deckhand/tests/unit/engine/test_document_layering.py @@ -305,6 +305,153 @@ class TestDocumentLayering2Layers(TestDocumentLayering): site_expected = {'a': {'x': 1, 'y': 2}, 'b': 4} self._test_layering(documents, site_expected) + def test_layering_default_scenario_merge_with_numeric_in_path(self): + # Check that 2 dicts are merged together for [0] index. + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": [{"x": 1, "y": 2}]}}, + "_SITE_DATA_1_": {"data": {"a": [{"z": 3}]}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "merge", "path": ".data.a[0]"}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + site_expected = {'a': [{'x': 1, 'y': 2, 'z': 3}]} + self._test_layering(documents, site_expected) + + # Check that 2 dicts are merged together for [0] index with [1] index + # data carried over. + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": [{"x": 1, "y": 2}, {"z": 3}]}}, + "_SITE_DATA_1_": {"data": {"a": [{}]}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "merge", "path": ".data.a[0]"}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + site_expected = {'a': [{'x': 1, 'y': 2}, {'z': 3}]} + self._test_layering(documents, site_expected) + + # Check that 2 dicts are merged together for [0] index with deep merge. + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": [{"x": 1, "y": 2}]}}, + "_SITE_DATA_1_": {"data": {"a": [{"x": 3}]}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "merge", "path": ".data.a[0]"}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + site_expected = {'a': [{'x': 3, 'y': 2}]} + self._test_layering(documents, site_expected) + + # Check that 2 dicts are merged together for [1] index. + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": ["test", {"x": 1, "y": 2}]}}, + "_SITE_DATA_1_": {"data": {"a": [{}, {"z": 3}]}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "merge", "path": ".data.a[1]"}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + site_expected = {'a': ["test", {'x': 1, 'y': 2, 'z': 3}]} + self._test_layering(documents, site_expected) + + # Check that merging works for an attribute within an index. + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": ["test", {"x": {"b": 5}}]}}, + "_SITE_DATA_1_": {"data": {"a": [{}, {"x": {"a": 8}}]}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "merge", "path": ".data.a[1].x"}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + site_expected = {"a": ["test", {"x": {"a": 8, "b": 5}}]} + self._test_layering(documents, site_expected) + + def test_layering_default_scenario_replace_with_numeric_in_path(self): + # Check that replacing the first index works. + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": [{"x": 1, "y": 2}]}}, + "_SITE_DATA_1_": {"data": {"a": [{"z": 3}]}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "replace", "path": ".data.a[0]"}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + site_expected = {'a': [{'z': 3}]} + self._test_layering(documents, site_expected) + + # Check that replacing the second index works. + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": [{"x": 1}, {"y": 2}]}}, + "_SITE_DATA_1_": {"data": {"a": [{}, {"y": 3}]}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "replace", "path": ".data.a[1]"}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + site_expected = {'a': [{'x': 1}, {'y': 3}]} + self._test_layering(documents, site_expected) + + # Check that replacing an attribute within an index works. + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": [{}, {"x": 1, "y": 2}]}}, + "_SITE_DATA_1_": {"data": {"a": [{}, {"y": 3}]}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "replace", "path": ".data.a[1].y"}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + site_expected = {'a': [{}, {'x': 1, 'y': 3}]} + self._test_layering(documents, site_expected) + + def test_layering_default_scenario_delete_with_numeric_in_path(self): + # Check that removing the first index results in empty array. + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": [{"x": 1, "y": 2}]}}, + "_SITE_DATA_1_": {"data": {}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "delete", "path": ".data.a[0]"}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + site_expected = {'a': []} + self._test_layering(documents, site_expected) + + # Check that removing one index retains the other. + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": [{"x": 1}, {"y": 2}]}}, + "_SITE_DATA_1_": {"data": {}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "delete", "path": ".data.a[0]"}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + site_expected = {'a': [{'y': 2}]} + self._test_layering(documents, site_expected) + + # Check that removing an attribute within an index works. + mapping = { + "_GLOBAL_DATA_1_": {"data": {"a": [{"x": 1, "y": 2}]}}, + "_SITE_DATA_1_": {"data": {}}, + "_SITE_ACTIONS_1_": { + "actions": [{"method": "delete", "path": ".data.a[0].x"}]} + } + doc_factory = factories.DocumentFactory(2, [1, 1]) + documents = doc_factory.gen_test(mapping, site_abstract=False) + + site_expected = {'a': [{"y": 2}]} + self._test_layering(documents, site_expected) + def test_layering_default_scenario_multi_parentselector(self): mapping = { "_GLOBAL_DATA_1_": {"data": {"a": {"x": 1, "y": 2}}},