refactor: Clean up jsonpath_replace method

This patch set cleans up jsonpath_replace method to increase
its readability. Currently, there is some weird legacy code in
there which doesn't make much sense and is most likely dead code.
In fact, before this change, there was no unit test for validating
that `MissingDocumentPattern` is raised correctly.

The `jsonpath_replace` is refactored to only call the inner
private function _do_replace once, after performing data
expansion (populating the data dictionary with nonexistent
nested keys).

Unit tests have been added to validate the exception above.

Change-Id: I1c18c4f8c79c1b9d3124747f8aa04743f27434eb

deckhand/common/utils.py

@@ -20,7 +20,6 @@ from beaker.cache import CacheManager
 from beaker.util import parse_cache_config_options
 import jsonpath_ng
 from oslo_log import log as logging
-from oslo_utils import excutils
 import six
 
 from deckhand.conf import config
@@ -107,8 +106,8 @@ def jsonpath_parse(data, jsonpath, match_all=False):
         return result if match_all else result[0]
 
 
-def _populate_data_with_attributes(jsonpath, data):
+def _execute_data_expansion(jsonpath, data):
-    # Populates ``data`` with any path specified in ``jsonpath``. For example,
+    # Expand ``data`` with any path specified in ``jsonpath``. For example,
     # if jsonpath is ".foo[0].bar.baz" then for each subpath -- foo[0], bar,
     # and baz -- that key will be added to ``data`` if missing.
     d = data
@@ -179,41 +178,38 @@ def jsonpath_replace(data, value, jsonpath, pattern=None):
         raise ValueError('The provided jsonpath %s does not begin with "." '
                          'or "$"' % jsonpath)
 
-    def _do_replace():
+    def _execute_replace(path, path_to_change):
-        p = _jsonpath_parse(jsonpath)
+        if path_to_change:
-        p_to_change = p.find(data)
-
-        if p_to_change:
             new_value = value
             if pattern:
-                to_replace = p_to_change[0].value
+                to_replace = path_to_change[0].value
                 # `new_value` represents the value to inject into `to_replace`
                 # that matches the `pattern`.
                 try:
+                    # A pattern requires us to look up the data located at
+                    # data[jsonpath] and then figure out what
+                    # re.match(data[jsonpath], pattern) is (in pseudocode).
+                    # Raise an exception in case the path isn't present in the
+                    # data and a pattern has been provided since it is
+                    # otherwise impossible to do the look-up.
                     new_value = re.sub(pattern, str(value), to_replace)
                 except TypeError as e:
-                    with excutils.save_and_reraise_exception():
+                    LOG.error('Failed to substitute the value %s into %s '
-                        LOG.error('Failed to substitute the value %s into %s '
+                              'using pattern %s. Details: %s', str(value),
-                                  'using pattern %s. Details: %s', str(value),
+                              to_replace, pattern, six.text_type(e))
-                                  to_replace, pattern, six.text_type(e))
+                    raise errors.MissingDocumentPattern(jsonpath=jsonpath,
-            return p.update(data, new_value)
+                                                        pattern=pattern)
 
-    result = _do_replace()
+            return path.update(data, new_value)
-    if result:
-        return result
 
-    # A pattern requires us to look up the data located at data[jsonpath]
+    # Deckhand should be smart enough to create the nested keys in the
-    # and then figure out what re.match(data[jsonpath], pattern) is (in
-    # pseudocode). But raise an exception in case the path isn't present in the
-    # data and a pattern has been provided since it is impossible to do the
-    # look-up.
-    if pattern:
-        raise errors.MissingDocumentPattern(path=jsonpath, pattern=pattern)
-
-    # However, Deckhand should be smart enough to create the nested keys in the
     # data if they don't exist and a pattern isn't required.
-    _populate_data_with_attributes(jsonpath, data)
+    path = _jsonpath_parse(jsonpath)
-    return _do_replace()
+    path_to_change = path.find(data)
+    if not path_to_change:
+        _execute_data_expansion(jsonpath, data)
+        path_to_change = path.find(data)
+    return _execute_replace(path, path_to_change)
 
 
 def multisort(data, sort_by=None, order_by=None):

deckhand/control/revision_documents.py

@@ -131,6 +131,7 @@ class RenderedDocumentsResource(api_base.BaseResource):
                 errors.IndeterminateDocumentParent,
                 errors.LayeringPolicyNotFound,
                 errors.MissingDocumentKey,
+                errors.MissingDocumentPattern,
                 errors.SubstitutionSourceDataNotFound,
                 errors.SubstitutionSourceNotFound,
                 errors.UnknownSubstitutionError,

deckhand/tests/unit/common/test_utils.py

@@ -19,6 +19,7 @@ from testtools.matchers import Equals
 from testtools.matchers import MatchesAny
 
 from deckhand.common import utils
+from deckhand import errors
 from deckhand.tests.unit import base as test_base
 
 
@@ -48,6 +49,30 @@ class TestJSONPathReplace(test_base.DeckhandTestCase):
         result = utils.jsonpath_replace({}, 'foo', path)
         self.assertEqual(expected, result)
 
+    def test_jsonpath_replace_with_pattern(self):
+        path = ".values.endpoints.admin"
+        body = {"values": {"endpoints": {"admin": "REGEX_FRESH"}}}
+        expected = {"values": {"endpoints": {"admin": "EAT_FRESH"}}}
+        result = utils.jsonpath_replace(body, "EAT", jsonpath=path,
+                                        pattern="REGEX")
+        self.assertEqual(expected, result)
+
+
+class TestJSONPathReplaceNegative(test_base.DeckhandTestCase):
+    """Validate JSONPath replace negative scenarios."""
+
+    def test_jsonpath_replace_without_expected_pattern_raises_exc(self):
+        empty_body = {}
+        error_re = (".*missing the pattern %s specified under .* at path %s.*")
+
+        self.assertRaisesRegex(errors.MissingDocumentPattern,
+                               error_re % ("way invalid", "\$.path"),
+                               utils.jsonpath_replace,
+                               empty_body,
+                               value="test",
+                               jsonpath=".path",
+                               pattern="way invalid")
+
 
 class TestJSONPathUtilsCaching(test_base.DeckhandTestCase):
     """Validate that JSONPath caching works."""