From ff5f016d4f015af9c2c6975d3c44115bc94f713b Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Mon, 20 Jun 2016 11:40:17 +0100
Subject: [PATCH 01/22] A failing test

---
 flocker/control/test/test_diffing.py | 50 ++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/flocker/control/test/test_diffing.py b/flocker/control/test/test_diffing.py
index 8ba39db626..b7dfe60df6 100644
--- a/flocker/control/test/test_diffing.py
+++ b/flocker/control/test/test_diffing.py
@@ -202,3 +202,53 @@ def test_different_uuids(self):
             wire_decode(wire_encode(diff)).apply(object_a),
             Equals(object_b)
         )
+
+
+class DiffTestObjInvariant(PClass):
+    """
+    Simple pyrsistent object with an invariant.
+    """
+    a = field()
+    b = field()
+
+    def __invariant__(self):
+        if self.a == self.b:
+            return (False, "a must not equal b")
+        else:
+            return (True, "")
+
+
+class InvariantDiffTests(TestCase):
+    """
+    Tests for creating and applying diffs to objects with invariant checks.
+    """
+    def test_straight_swap(self):
+        o1 = DiffTestObjInvariant(
+            a=1,
+            b=2
+        )
+        o2 = DiffTestObjInvariant(
+            a=2,
+            b=1
+        )
+        diff = create_diff(o1, o2)
+        diff.apply(o1)
+
+    def test_deep_swap(self):
+        a = DiffTestObjInvariant(
+            a=1,
+            b=2
+        )
+        b = DiffTestObjInvariant(
+            a=3,
+            b=4
+        )
+        o1 = DiffTestObjInvariant(
+            a=a,
+            b=b
+        )
+        o2 = o1.transform(
+            ['a'], lambda o: o.evolver().set('a', 2).set('b', 1).persistent()
+        )
+        diff = create_diff(o1, o2)
+        diff.apply(o1)

From 835627a7dce3e064870506af56833b940b7040bc Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Tue, 21 Jun 2016 16:46:34 +0100
Subject: [PATCH 02/22] Add a proxy to collect and apply adjacent set
 operations to an evolver so as not to trigger the invariant check until all
 attributes have been set.

---
 flocker/control/_diffing.py          | 73 ++++++++++++++++++++++++++--
 flocker/control/test/test_diffing.py | 34 +++++++++----
 2 files changed, 95 insertions(+), 12 deletions(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index 038d628fde..ed2bbc0f90 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -15,6 +15,7 @@
     pvector,
     pvector_field,
 )
+from pyrsistent._transformations import _get
 
 from zope.interface import Interface, implementer
 
@@ -76,7 +77,9 @@ class _Set(PClass):
     value = field()
 
     def apply(self, obj):
-        return obj.transform(self.path, self.value)
+        return obj.transform(
+            self.path[:-1], lambda o: o.set(self.path[-1], self.value)
+        )
 
 
 @implementer(_IDiffChange)
@@ -95,6 +98,65 @@ def apply(self, obj):
         return obj.transform(self.path, lambda x: x.add(self.item))
 
 
+class _TransformProxy(object):
+    """
+    This attempts to bunch all the diff operations for a particular object into
+    a single transaction so that related attributes can be ``set`` without
+    triggering an in invariant error.
+    """
+    def __init__(self, original):
+        """
+        :param PClass original: The root object to which transformations will
+            be applied.
+        """
+        self._original_object = original
+        self._operations = []
+        self._current_path = None
+        self._current_object = original
+
+    def transform(self, path, operation):
+        """
+        Record an ``operation`` to be applied to ``original`` at ``path`` and
+        commit all outstanding operations when the ``path`` changes.
+
+        :param PVector path: The path relative to ``original`` which will be
+            operated on.
+        :param callable operation: A function to be applied to an evolver of
+             the object at ``path``
+        :returns: ``self``
+        """
+        if self._current_path is not None:
+            if path != self._current_path:
+                self.commit()
+        self._current_path = path
+        self._operations.append(operation)
+        return self
+
+    def commit(self):
+        """
+        Apply all outstanding operations, updating the current object and
+        return the result.
+
+        :returns: The transformed current object
+        """
+        if self._operations:
+            target = reduce(
+                lambda o, segment: _get(o, segment, None),
+                self._current_path,
+                self._current_object,
+            )
+            evolver = target.evolver()
+            for operation in self._operations:
+                operation(evolver)
+            target = evolver.persistent()
+            self._operations = []
+            self._current_object = self._current_object.transform(
+                self._current_path,
+                target,
+            )
+        return self._current_object
+
+
 @implementer(_IDiffChange)
 class Diff(PClass):
     """
@@ -111,9 +173,14 @@ class Diff(PClass):
     changes = pvector_field(object)
 
     def apply(self, obj):
+        proxy = _TransformProxy(original=obj)
         for c in self.changes:
-            obj = c.apply(obj)
-        return obj
+            if len(c.path) > 0:
+                proxy = c.apply(proxy)
+            else:
+                assert type(c) is _Set
+                proxy = _TransformProxy(original=c.value)
+        return proxy.commit()
 
 
 def _create_diffs_for_sets(current_path, set_a, set_b):
diff --git a/flocker/control/test/test_diffing.py b/flocker/control/test/test_diffing.py
index b7dfe60df6..98d38dea5f 100644
--- a/flocker/control/test/test_diffing.py
+++ b/flocker/control/test/test_diffing.py
@@ -183,7 +183,6 @@ def test_different_objects(self):
         object_a = DiffTestObj(a=pset(xrange(1000)))
         object_b = pmap({'1': 34})
         diff = create_diff(object_a, object_b)
-
         self.assertThat(
             wire_decode(wire_encode(diff)).apply(object_a),
             Equals(object_b)
@@ -223,32 +222,49 @@ class InvariantDiffTests(TestCase):
     Tests for creating and applying diffs to objects with invariant checks.
     """
     def test_straight_swap(self):
+        """
+        A diff composed of two separate ``set`` operations can be applied to an
+        object without triggering an invariant exception.
+        """
         o1 = DiffTestObjInvariant(
             a=1,
-            b=2
+            b=2,
         )
         o2 = DiffTestObjInvariant(
             a=2,
-            b=1
+            b=1,
         )
         diff = create_diff(o1, o2)
-        diff.apply(o1)
+        self.assertEqual(2, len(diff.changes))
+        self.assertEqual(
+            o2,
+            diff.apply(o1)
+        )
 
     def test_deep_swap(self):
+        """
+        A diff composed of two separate ``set`` operations can be applied to a
+        nested object without triggering an invariant exception.
+        """
         a = DiffTestObjInvariant(
             a=1,
-            b=2
+            b=2,
         )
         b = DiffTestObjInvariant(
             a=3,
-            b=4
+            b=4,
         )
         o1 = DiffTestObjInvariant(
             a=a,
-            b=b
+            b=b,
         )
         o2 = o1.transform(
-            ['a'], lambda o: o.evolver().set('a', 2).set('b', 1).persistent()
+            ['a'],
+            lambda o: o.evolver().set('a', 2).set('b', 1).persistent()
         )
         diff = create_diff(o1, o2)
-        diff.apply(o1)
+
+        self.assertEqual(
+            o2,
+            diff.apply(o1)
+        )

From 2e00493b4d8cc24c8dc203fe6c8f74f2bd814178 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Wed, 22 Jun 2016 17:58:57 +0100
Subject: [PATCH 03/22] Add logging so I can see what diffs are failing.

---
 flocker/control/_diffing.py          | 28 +++++++++++++++++++++++++++-
 flocker/control/test/test_diffing.py | 28 +++++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index ed2bbc0f90..3b6a82964c 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -7,6 +7,8 @@
 flocker configuration or the flocker state.
 """
 
+from eliot import MessageType, Field
+
 from pyrsistent import (
     PClass,
     PMap,
@@ -157,6 +159,22 @@ def commit(self):
         return self._current_object
 
 
+TARGET_OBJECT = Field(
+    u"target_object", repr,
+    u"The object to which the diff was applied."
+)
+CHANGES = Field(
+    u"changes", repr,
+    u"The changes being applied."
+)
+
+DIFF_COMMIT_ERROR = MessageType(
+    u"flocker:control:Diff:commit_error",
+    [TARGET_OBJECT, CHANGES],
+    u"The target and changes that failed to apply."
+)
+
+
 @implementer(_IDiffChange)
 class Diff(PClass):
     """
@@ -180,7 +198,15 @@ def apply(self, obj):
             else:
                 assert type(c) is _Set
                 proxy = _TransformProxy(original=c.value)
-        return proxy.commit()
+        try:
+            return proxy.commit()
+        except:
+            from ._persistence import wire_encode
+            DIFF_COMMIT_ERROR(
+                target_object=wire_encode(obj),
+                changes=wire_encode(self.changes),
+            ).write()
+            raise
 
 
 def _create_diffs_for_sets(current_path, set_a, set_b):
diff --git a/flocker/control/test/test_diffing.py b/flocker/control/test/test_diffing.py
index 98d38dea5f..6dd3c2b426 100644
--- a/flocker/control/test/test_diffing.py
+++ b/flocker/control/test/test_diffing.py
@@ -7,11 +7,12 @@
 from json import dumps
 from uuid import uuid4
 
+from eliot.testing import capture_logging, assertHasMessage
 from hypothesis import given
 import hypothesis.strategies as st
-from pyrsistent import PClass, field, pmap, pset
+from pyrsistent import PClass, field, pmap, pset, InvariantException
 
-from .._diffing import create_diff, compose_diffs
+from .._diffing import create_diff, compose_diffs, DIFF_COMMIT_ERROR
 from .._persistence import wire_encode, wire_decode
 from .._model import Node, Port
 from ..testtools import (
@@ -207,11 +208,12 @@ class DiffTestObjInvariant(PClass):
     """
     Simple pyrsistent object with an invariant.
     """
+    _perform_invariant_check = True
     a = field()
     b = field()
 
     def __invariant__(self):
-        if self.a == self.b:
+        if self._perform_invariant_check and self.a == self.b:
             return (False, "a must not equal b")
         else:
             return (True, "")
@@ -268,3 +270,23 @@ def test_deep_swap(self):
             o2,
             diff.apply(o1)
         )
+
+    @capture_logging(assertHasMessage, DIFF_COMMIT_ERROR)
+    def test_error_logging(self, logger):
+        """
+        Failures while applying a diff emit a log message containing the full
+        diff.
+        """
+        o1 = DiffTestObjInvariant(
+            a=1,
+            b=2,
+        )
+        DiffTestObjInvariant._perform_invariant_check = False
+        o2 = o1.set('b', 1)
+        DiffTestObjInvariant._perform_invariant_check = True
+        diff = create_diff(o1, o2)
+        self.assertRaises(
+            InvariantException,
+            diff.apply,
+            o1,
+        )

From 962daa57d3bffec8b0b3808ac72a7914ab6393e5 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Thu, 23 Jun 2016 11:09:19 +0100
Subject: [PATCH 04/22] Tests for the diffs which modify a Node applications
 and manifestations in such a way as to trigger an invariant error

---
 flocker/control/test/test_diffing.py | 44 ++++++++++++++++++++++++++++
 flocker/control/testtools.py         | 41 ++++++++++++++++++++------
 2 files changed, 76 insertions(+), 9 deletions(-)

diff --git a/flocker/control/test/test_diffing.py b/flocker/control/test/test_diffing.py
index 6dd3c2b426..06c51aadc4 100644
--- a/flocker/control/test/test_diffing.py
+++ b/flocker/control/test/test_diffing.py
@@ -18,6 +18,7 @@
 from ..testtools import (
     application_strategy,
     deployment_strategy,
+    node_strategy,
     related_deployments_strategy
 )
 
@@ -290,3 +291,46 @@ def test_error_logging(self, logger):
             diff.apply,
             o1,
         )
+
+    def test_application_add(self):
+        """
+        A diff on a Node, which *adds* and application with a volume *and* the
+        manifestation for the volume, can be applied without triggering an
+        invariant error on the Node.
+        """
+        node2 = node_strategy(min_number_of_applications=1).example()
+        application = node2.applications.values()[0]
+        node1 = node2.transform(
+            ['applications'],
+            lambda o: o.remove(application.name)
+        ).transform(
+            ['manifestations'],
+            lambda o: o.remove(application.volume.manifestation.dataset_id)
+        )
+        diff = create_diff(node1, node2)
+        self.assertEqual(
+            node2,
+            diff.apply(node1),
+        )
+
+    def test_application_modify(self):
+        """
+        A diff on a Node, which adds a volume to an *existing* application
+        volume *and* the manifestation for the volume, can be applied without
+        triggering an invariant error on the Node.
+        """
+        node2 = node_strategy(min_number_of_applications=1).example()
+        application = node2.applications.values()[0]
+        volume = application.volume
+        node1 = node2.transform(
+            ['applications', application.name],
+            lambda o: o.set('volume', None)
+        ).transform(
+            ['manifestations'],
+            lambda o: o.remove(volume.manifestation.dataset_id)
+        )
+        diff = create_diff(node1, node2)
+        self.assertEqual(
+            node2,
+            diff.apply(node1),
+        )
diff --git a/flocker/control/testtools.py b/flocker/control/testtools.py
index 32248a24fd..0070fa284e 100644
--- a/flocker/control/testtools.py
+++ b/flocker/control/testtools.py
@@ -3,12 +3,14 @@
 """
 Tools for testing :py:module:`flocker.control`.
 """
+from uuid import uuid4
 
 from zope.interface.verify import verifyObject
 
 from twisted.internet.endpoints import TCP4ServerEndpoint
 from twisted.internet.ssl import ClientContextFactory
 from twisted.internet.task import Clock
+from twisted.python.filepath import FilePath
 from twisted.test.proto_helpers import MemoryReactor
 
 from ..testtools import TestCase
@@ -21,10 +23,13 @@
 from ._registry import IStatePersister, InMemoryStatePersister
 from ._model import (
     Application,
+    AttachedVolume,
+    Dataset,
     DatasetAlreadyOwned,
     Deployment,
     DockerImage,
     Lease,
+    Manifestation,
     Node,
     PersistentState,
     Port,
@@ -227,6 +232,7 @@ def application_strategy(draw, min_number_of_ports=0):
             max_value=max(8, min_number_of_ports+1)
         )
     )
+    dataset_id = unicode(uuid4())
     return Application(
         name=draw(unique_name_strategy()),
         image=draw(docker_image_strategy()),
@@ -235,6 +241,16 @@ def application_strategy(draw, min_number_of_ports=0):
                 internal_port=8000+i,
                 external_port=8000+i+1
             ) for i in xrange(num_ports)
+        ),
+        volume=AttachedVolume(
+            manifestation=Manifestation(
+                dataset=Dataset(
+                    dataset_id=dataset_id,
+                    deleted=False,
+                ),
+                primary=True,
+            ),
+            mountpoint=FilePath('/flocker').child(dataset_id)
         )
     )
 
@@ -242,6 +258,7 @@ def application_strategy(draw, min_number_of_ports=0):
 @composite
 def node_strategy(
         draw,
+        min_number_of_applications=0,
         uuid=st.uuids(),
         applications=application_strategy()
 ):
@@ -253,17 +270,23 @@ def node_strategy(
     :param applications: The strategy to use to generate the applications on
         the Node.
     """
-    applications = draw(st.lists(
-        application_strategy(),
-        min_size=0,
-        average_size=2,
-        max_size=5
-    ))
+    applications = {
+        a.name: a for a in
+        draw(
+            st.lists(
+                application_strategy(),
+                min_size=min_number_of_applications,
+                average_size=2,
+                max_size=5
+            )
+        )
+    }
     return Node(
         uuid=draw(uuid),
-        applications={
-            a.name: a
-            for a in applications
+        applications=applications,
+        manifestations={
+            a.volume.manifestation.dataset_id: a.volume.manifestation
+            for a in applications.values()
         }
     )
 

From adcf738e4804de47a2194949cd97d87d6e925bd0 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Thu, 23 Jun 2016 13:10:42 +0100
Subject: [PATCH 05/22] A more flexible approach....not quite complete

---
 flocker/control/_diffing.py          | 97 +++++++++++++---------------
 flocker/control/test/test_diffing.py | 10 ++-
 flocker/control/testtools.py         | 35 ++++++----
 3 files changed, 74 insertions(+), 68 deletions(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index 3b6a82964c..c4e176e818 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -100,63 +100,56 @@ def apply(self, obj):
         return obj.transform(self.path, lambda x: x.add(self.item))
 
 
-class _TransformProxy(object):
-    """
-    This attempts to bunch all the diff operations for a particular object into
-    a single transaction so that related attributes can be ``set`` without
-    triggering an in invariant error.
-    """
+_sentinel = object()
+
+
+class _EvolverProxy(object):
     def __init__(self, original):
-        """
-        :param PClass original: The root object to which transformations will
-            be applied.
-        """
-        self._original_object = original
+        self._original = original
+        self._evolver = original.evolver()
+        self._children = {}
         self._operations = []
-        self._current_path = None
-        self._current_object = original
 
     def transform(self, path, operation):
-        """
-        Record an ``operation`` to be applied to ``original`` at ``path`` and
-        commit all outstanding operations when the ``path`` changes.
-
-        :param PVector path: The path relative to ``original`` which will be
-            operated on.
-        :param callable operation: A function to be applied to an evolver of
-             the object at ``path``
-        :returns: ``self``
-        """
-        if self._current_path is not None:
-            if path != self._current_path:
-                self.commit()
-        self._current_path = path
-        self._operations.append(operation)
+        def _child(parent, segment):
+            child = parent._children.get(segment, _sentinel)
+            if child is not _sentinel:
+                return child
+            child = _get(parent._original, segment, _sentinel)
+            if child is _sentinel:
+                raise KeyError(
+                    'Segment not found in path. '
+                    'Parent: {}, '
+                    'Segment: {}, '
+                    'Path: {}.'.format(parent, segment, path)
+                )
+            proxy_for_child = _EvolverProxy(child)
+            parent._children[segment] = proxy_for_child
+            return proxy_for_child
+
+        target = self
+        for segment in path:
+            target = _child(target, segment)
+        operation(target._evolver)
         return self
 
-    def commit(self):
-        """
-        Apply all outstanding operations, updating the current object and
-        return the result.
+    def add(self, item):
+        self._evolver.add(item)
+        return self
 
-        :returns: The transformed current object
-        """
-        if self._operations:
-            target = reduce(
-                lambda o, segment: _get(o, segment, None),
-                self._current_path,
-                self._current_object,
-            )
-            evolver = target.evolver()
-            for operation in self._operations:
-                operation(evolver)
-            target = evolver.persistent()
-            self._operations = []
-            self._current_object = self._current_object.transform(
-                self._current_path,
-                target,
-            )
-        return self._current_object
+    def set(self, key, item):
+        self._evolver.set(key, item)
+        return self
+
+    def remove(self, item):
+        self._evolver.remove(item)
+        return self
+
+    def commit(self):
+        for segment, child_evolver_proxy in self._children.items():
+            child = child_evolver_proxy.commit()
+            self._evolver.set(segment, child)
+        return self._evolver.persistent()
 
 
 TARGET_OBJECT = Field(
@@ -191,13 +184,13 @@ class Diff(PClass):
     changes = pvector_field(object)
 
     def apply(self, obj):
-        proxy = _TransformProxy(original=obj)
+        proxy = _EvolverProxy(original=obj)
         for c in self.changes:
             if len(c.path) > 0:
                 proxy = c.apply(proxy)
             else:
                 assert type(c) is _Set
-                proxy = _TransformProxy(original=c.value)
+                proxy = _EvolverProxy(original=c.value)
         try:
             return proxy.commit()
         except:
diff --git a/flocker/control/test/test_diffing.py b/flocker/control/test/test_diffing.py
index 06c51aadc4..aab86ea0f9 100644
--- a/flocker/control/test/test_diffing.py
+++ b/flocker/control/test/test_diffing.py
@@ -298,7 +298,10 @@ def test_application_add(self):
         manifestation for the volume, can be applied without triggering an
         invariant error on the Node.
         """
-        node2 = node_strategy(min_number_of_applications=1).example()
+        node2 = node_strategy(
+            min_number_of_applications=1,
+            stateful_applications=True,
+        ).example()
         application = node2.applications.values()[0]
         node1 = node2.transform(
             ['applications'],
@@ -319,7 +322,10 @@ def test_application_modify(self):
         volume *and* the manifestation for the volume, can be applied without
         triggering an invariant error on the Node.
         """
-        node2 = node_strategy(min_number_of_applications=1).example()
+        node2 = node_strategy(
+            min_number_of_applications=1,
+            stateful_applications=True,
+        ).example()
         application = node2.applications.values()[0]
         volume = application.volume
         node1 = node2.transform(
diff --git a/flocker/control/testtools.py b/flocker/control/testtools.py
index 0070fa284e..47e14dc3bd 100644
--- a/flocker/control/testtools.py
+++ b/flocker/control/testtools.py
@@ -3,7 +3,7 @@
 """
 Tools for testing :py:module:`flocker.control`.
 """
-from uuid import uuid4
+from uuid import uuid4, UUID
 
 from zope.interface.verify import verifyObject
 
@@ -219,7 +219,7 @@ def docker_image_strategy(
 
 
 @composite
-def application_strategy(draw, min_number_of_ports=0):
+def application_strategy(draw, min_number_of_ports=0, stateful=False):
     """
     A hypothesis strategy to generate an ``Application``
 
@@ -233,7 +233,7 @@ def application_strategy(draw, min_number_of_ports=0):
         )
     )
     dataset_id = unicode(uuid4())
-    return Application(
+    application = Application(
         name=draw(unique_name_strategy()),
         image=draw(docker_image_strategy()),
         ports=frozenset(
@@ -242,23 +242,29 @@ def application_strategy(draw, min_number_of_ports=0):
                 external_port=8000+i+1
             ) for i in xrange(num_ports)
         ),
-        volume=AttachedVolume(
-            manifestation=Manifestation(
-                dataset=Dataset(
-                    dataset_id=dataset_id,
-                    deleted=False,
+    )
+    if stateful:
+        application = application.set(
+            'volume',
+            AttachedVolume(
+                manifestation=Manifestation(
+                    dataset=Dataset(
+                        dataset_id=dataset_id,
+                        deleted=False,
+                    ),
+                    primary=True,
                 ),
-                primary=True,
-            ),
-            mountpoint=FilePath('/flocker').child(dataset_id)
+                mountpoint=FilePath('/flocker').child(dataset_id)
+            )
         )
-    )
+    return application
 
 
 @composite
 def node_strategy(
         draw,
         min_number_of_applications=0,
+        stateful_applications=False,
         uuid=st.uuids(),
         applications=application_strategy()
 ):
@@ -274,7 +280,7 @@ def node_strategy(
         a.name: a for a in
         draw(
             st.lists(
-                application_strategy(),
+                application_strategy(stateful=stateful_applications),
                 min_size=min_number_of_applications,
                 average_size=2,
                 max_size=5
@@ -287,6 +293,7 @@ def node_strategy(
         manifestations={
             a.volume.manifestation.dataset_id: a.volume.manifestation
             for a in applications.values()
+            if a.volume is not None
         }
     )
 
@@ -364,7 +371,7 @@ def deployment_strategy(
         draw(
             lease_strategy(
                 dataset_id=st.just(dataset_id),
-                node_uuid=st.just(node_uuid)
+                node_id=st.just(node_uuid)
             )
         ) for dataset_id, node_uuid in (
             dataset_id_node_mapping.items()[i] for i in lease_indexes

From 0bec10c796e1e6f6ecd88c7e4bbb736749fad680 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Thu, 23 Jun 2016 17:00:44 +0100
Subject: [PATCH 06/22] Work in progress

---
 flocker/control/_diffing.py | 59 ++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index c4e176e818..7c9bf38fce 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -110,39 +110,56 @@ def __init__(self, original):
         self._children = {}
         self._operations = []
 
-    def transform(self, path, operation):
-        def _child(parent, segment):
-            child = parent._children.get(segment, _sentinel)
-            if child is not _sentinel:
-                return child
-            child = _get(parent._original, segment, _sentinel)
-            if child is _sentinel:
-                raise KeyError(
-                    'Segment not found in path. '
-                    'Parent: {}, '
-                    'Segment: {}, '
-                    'Path: {}.'.format(parent, segment, path)
-                )
-            proxy_for_child = _EvolverProxy(child)
-            parent._children[segment] = proxy_for_child
-            return proxy_for_child
+    def _child(self, segment, default=_sentinel):
+        import pdb; pdb.set_trace()
+        child = self._children.get(segment)
+        if child is not None:
+            return child
+        child = _get(self._original, segment, default)
+        if child is _sentinel:
+            raise KeyError(
+                'Segment not found in path. '
+                'Parent: {}, '
+                'Segment: {}'.format(self, segment)
+            )
+        proxy_for_child = _EvolverProxy(child)
+        self._children[segment] = proxy_for_child
+        return proxy_for_child
 
+    def transform(self, path, operation):
         target = self
         for segment in path:
-            target = _child(target, segment)
-        operation(target._evolver)
+            target = target._child(segment)
+        operation(target)
         return self
 
     def add(self, item):
-        self._evolver.add(item)
+        if hasattr(item, 'evolver'):
+            raise ValueError(
+                'Cannot ``add`` pyrsistent items. '
+                'Item: {}, '
+                'Parent: {}.'.format(item, self)
+            )
+        else:
+            self._evolver.add(item)
         return self
 
     def set(self, key, item):
-        self._evolver.set(key, item)
+        if hasattr(item, 'evolver'):
+            self._child(key, default=item)
+        else:
+            self._evolver.set(key, item)
         return self
 
     def remove(self, item):
-        self._evolver.remove(item)
+        if hasattr(item, 'evolver'):
+            raise ValueError(
+                'Cannot ``remove`` pyrsistent items. '
+                'Item: {}, '
+                'Parent: {}.'.format(item, self)
+            )
+        else:
+            self._evolver.remove(item)
         return self
 
     def commit(self):

From a9ce128575341f6291a73a37422282bfe42c04c6 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Fri, 24 Jun 2016 10:10:06 +0100
Subject: [PATCH 07/22] Handle add and remove of Pyrsistent objects

---
 flocker/control/_diffing.py | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index 7c9bf38fce..2284bfd8cc 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -110,12 +110,11 @@ def __init__(self, original):
         self._children = {}
         self._operations = []
 
-    def _child(self, segment, default=_sentinel):
-        import pdb; pdb.set_trace()
+    def _child(self, segment):
         child = self._children.get(segment)
         if child is not None:
             return child
-        child = _get(self._original, segment, default)
+        child = _get(self._original, segment, _sentinel)
         if child is _sentinel:
             raise KeyError(
                 'Segment not found in path. '
@@ -135,29 +134,22 @@ def transform(self, path, operation):
 
     def add(self, item):
         if hasattr(item, 'evolver'):
-            raise ValueError(
-                'Cannot ``add`` pyrsistent items. '
-                'Item: {}, '
-                'Parent: {}.'.format(item, self)
-            )
+            self._children[item] = _EvolverProxy(item)
         else:
             self._evolver.add(item)
         return self
 
     def set(self, key, item):
         if hasattr(item, 'evolver'):
-            self._child(key, default=item)
+            # This will replace any existing proxy.
+            self._children[key] = _EvolverProxy(item)
         else:
             self._evolver.set(key, item)
         return self
 
     def remove(self, item):
-        if hasattr(item, 'evolver'):
-            raise ValueError(
-                'Cannot ``remove`` pyrsistent items. '
-                'Item: {}, '
-                'Parent: {}.'.format(item, self)
-            )
+        if item in self._children:
+            self._children.pop(item)
         else:
             self._evolver.remove(item)
         return self

From 5502b7b630dcc3808ac18b21f6bbe18a5eca61d0 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Fri, 24 Jun 2016 10:15:06 +0100
Subject: [PATCH 08/22] Handle the freezing of map like objects and sets
 differently.

---
 flocker/control/_diffing.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index 2284bfd8cc..7d7139d609 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -157,7 +157,12 @@ def remove(self, item):
     def commit(self):
         for segment, child_evolver_proxy in self._children.items():
             child = child_evolver_proxy.commit()
-            self._evolver.set(segment, child)
+            # XXX this is ugly. Perhaps have a separate proxy for PClass, PMap
+            # and PSet collections
+            if hasattr(self._evolver, 'set'):
+                self._evolver.set(segment, child)
+            else:
+                self._evolver.add(child)
         return self._evolver.persistent()
 
 

From ae06a41cf5380d12db52248e2cdaed86c6976993 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Fri, 24 Jun 2016 10:32:07 +0100
Subject: [PATCH 09/22] remove unused import

---
 flocker/control/testtools.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/flocker/control/testtools.py b/flocker/control/testtools.py
index 47e14dc3bd..b7379eae7f 100644
--- a/flocker/control/testtools.py
+++ b/flocker/control/testtools.py
@@ -3,8 +3,6 @@
 """
 Tools for testing :py:module:`flocker.control`.
 """
-from uuid import uuid4, UUID
-
 from zope.interface.verify import verifyObject
 
 from twisted.internet.endpoints import TCP4ServerEndpoint

From 19d0d86359699e6a0e66279dc58dcfecbaac2ecf Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Fri, 24 Jun 2016 11:13:59 +0100
Subject: [PATCH 10/22] oops, I removed too much

---
 flocker/control/testtools.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/flocker/control/testtools.py b/flocker/control/testtools.py
index b7379eae7f..520b415a80 100644
--- a/flocker/control/testtools.py
+++ b/flocker/control/testtools.py
@@ -3,6 +3,8 @@
 """
 Tools for testing :py:module:`flocker.control`.
 """
+from uuid import uuid4
+
 from zope.interface.verify import verifyObject
 
 from twisted.internet.endpoints import TCP4ServerEndpoint

From ac9e6cbbfc27d2f440f745c74dd90837ec681808 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Fri, 24 Jun 2016 11:18:11 +0100
Subject: [PATCH 11/22] When removing an item attempt to remove it from the
 evolver too.

---
 flocker/control/_diffing.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index 7d7139d609..a97e7242f5 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -148,10 +148,13 @@ def set(self, key, item):
         return self
 
     def remove(self, item):
-        if item in self._children:
-            self._children.pop(item)
-        else:
+        self._children.pop(item, None)
+        # Attempt to remove the item from the evolver too.  It may be something
+        # that was replaced rather than added by a previous ``set`` operation.
+        try:
             self._evolver.remove(item)
+        except KeyError:
+            pass
         return self
 
     def commit(self):

From a982d7783f30e74cdd1d1bcb103d0269ed4672d3 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Fri, 24 Jun 2016 15:18:17 +0100
Subject: [PATCH 12/22] docstrings

---
 flocker/control/_diffing.py | 62 +++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index a97e7242f5..f599e16c44 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -104,7 +104,18 @@ def apply(self, obj):
 
 
 class _EvolverProxy(object):
+    """
+    This attempts to bunch all the diff operations for a particular object into
+    a single transaction so that related attributes can be ``set`` without
+    triggering an in invariant error.
+    Additionally, the leaf nodes are persisted first and in isolation, so as
+    not to trigger invariant errors in ancestor nodes.
+    """
     def __init__(self, original):
+        """
+        :param PClass original: The root object to which transformations will
+            be applied.
+        """
         self._original = original
         self._evolver = original.evolver()
         self._children = {}
@@ -126,6 +137,18 @@ def _child(self, segment):
         return proxy_for_child
 
     def transform(self, path, operation):
+        """
+        Traverse each segment of ``path`` to create a hierarchy of
+        ``_EvolverProxy`` objects and perform the ``operation`` on the
+        resulting leaf proxy object. This will infact perform the operation on
+        an evolver of the original Pyrsistent object.
+
+        :param PVector path: The path relative to ``original`` which will be
+            operated on.
+        :param callable operation: A function to be applied to an evolver of
+             the object at ``path``
+        :returns: ``self``
+        """
         target = self
         for segment in path:
             target = target._child(segment)
@@ -133,6 +156,16 @@ def transform(self, path, operation):
         return self
 
     def add(self, item):
+        """
+        Add ``item`` to the ``original`` ``Pset`` or if the item is itself a
+        Pyrsistent object, add a new proxy for that item so that further
+        operations can be performed on it without triggering invariant checks
+        until the tree is finally committed.
+
+        :param item: An object to be added to the ``PSet`` wrapped by this
+            proxy.
+        :returns: ``self``
+        """
         if hasattr(item, 'evolver'):
             self._children[item] = _EvolverProxy(item)
         else:
@@ -140,6 +173,17 @@ def add(self, item):
         return self
 
     def set(self, key, item):
+        """
+        Set the ``item`` in an evolver of the ``original`` ``PMap`` or
+        ``PClass`` or if the item is itself a Pyrsistent object, add a new
+        proxy for that item so that further operations can be performed on it
+        without triggering invariant checks until the tree is finally
+        committed.
+
+        :param item: An object to be added or set on the ``PMap`` wrapped by
+            this proxy.
+        :returns: ``self``
+        """
         if hasattr(item, 'evolver'):
             # This will replace any existing proxy.
             self._children[key] = _EvolverProxy(item)
@@ -148,6 +192,16 @@ def set(self, key, item):
         return self
 
     def remove(self, item):
+        """
+        Remove the ``item`` in an evolver of the ``original`` ``PMap``,
+        ``PClass``, or ``PSet`` and if the item is an uncommitted
+        ``_EvolverProxy`` remove it from the list of children so that the item
+        is not persisted when the structure is finally committed.
+
+        :param item: The object to be removed from the wrapped ``PSet`` or the
+            key to be removed from the wrapped ``PMap``
+        :returns: ``self``
+        """
         self._children.pop(item, None)
         # Attempt to remove the item from the evolver too.  It may be something
         # that was replaced rather than added by a previous ``set`` operation.
@@ -158,6 +212,13 @@ def remove(self, item):
         return self
 
     def commit(self):
+        """
+        Persist all the changes made to the descendants of this structure, then
+        persist the resulting sub-objects and local changes to this root object
+        and finally return the resulting immutable structure.
+
+        :returns: The updated and persisted version of ``original``.
+        """
         for segment, child_evolver_proxy in self._children.items():
             child = child_evolver_proxy.commit()
             # XXX this is ugly. Perhaps have a separate proxy for PClass, PMap
@@ -211,6 +272,7 @@ def apply(self, obj):
         try:
             return proxy.commit()
         except:
+            # Imported here to avoid circular dependencies.
             from ._persistence import wire_encode
             DIFF_COMMIT_ERROR(
                 target_object=wire_encode(obj),

From c4e281597f7065bb7dd2ef25d2965fabf8b2a865 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Tue, 5 Jul 2016 11:17:12 +0100
Subject: [PATCH 13/22] A clearer set up for the expected test object.

---
 flocker/control/test/test_diffing.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/flocker/control/test/test_diffing.py b/flocker/control/test/test_diffing.py
index aab86ea0f9..a7ee366823 100644
--- a/flocker/control/test/test_diffing.py
+++ b/flocker/control/test/test_diffing.py
@@ -238,7 +238,7 @@ def test_straight_swap(self):
             b=1,
         )
         diff = create_diff(o1, o2)
-        self.assertEqual(2, len(diff.changes))
+        self.expectThat(len(diff.changes), Equals(2))
         self.assertEqual(
             o2,
             diff.apply(o1)
@@ -263,10 +263,13 @@ def test_deep_swap(self):
         )
         o2 = o1.transform(
             ['a'],
-            lambda o: o.evolver().set('a', 2).set('b', 1).persistent()
+            DiffTestObjInvariant(
+                a=2,
+                b=1,
+            )
         )
         diff = create_diff(o1, o2)
-
+        self.expectThat(len(diff.changes), Equals(2))
         self.assertEqual(
             o2,
             diff.apply(o1)

From f8cfdd62368818e43841233680b9d48d6bcac82b Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Tue, 5 Jul 2016 11:52:22 +0100
Subject: [PATCH 14/22] Use twisted MonkeyPatcher directly....maybe overkill?!

---
 flocker/control/test/test_diffing.py | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/flocker/control/test/test_diffing.py b/flocker/control/test/test_diffing.py
index a7ee366823..f87e04194e 100644
--- a/flocker/control/test/test_diffing.py
+++ b/flocker/control/test/test_diffing.py
@@ -11,6 +11,7 @@
 from hypothesis import given
 import hypothesis.strategies as st
 from pyrsistent import PClass, field, pmap, pset, InvariantException
+from twisted.python.monkey import MonkeyPatcher
 
 from .._diffing import create_diff, compose_diffs, DIFF_COMMIT_ERROR
 from .._persistence import wire_encode, wire_decode
@@ -285,9 +286,17 @@ def test_error_logging(self, logger):
             a=1,
             b=2,
         )
-        DiffTestObjInvariant._perform_invariant_check = False
-        o2 = o1.set('b', 1)
-        DiffTestObjInvariant._perform_invariant_check = True
+        patcher = MonkeyPatcher()
+        patcher.addPatch(
+            DiffTestObjInvariant,
+            '_perform_invariant_check',
+            False
+        )
+        patcher.patch()
+        try:
+            o2 = o1.set('b', 1)
+        finally:
+            patcher.restore()
         diff = create_diff(o1, o2)
         self.assertRaises(
             InvariantException,

From 1e512d59aaa05a574b78b4774c843516cba2598b Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Tue, 5 Jul 2016 11:55:45 +0100
Subject: [PATCH 15/22] Explain the purpose of the DiffTestObjInvariant class.

---
 flocker/control/test/test_diffing.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/flocker/control/test/test_diffing.py b/flocker/control/test/test_diffing.py
index f87e04194e..d325b47bf1 100644
--- a/flocker/control/test/test_diffing.py
+++ b/flocker/control/test/test_diffing.py
@@ -208,7 +208,11 @@ def test_different_uuids(self):
 
 class DiffTestObjInvariant(PClass):
     """
-    Simple pyrsistent object with an invariant.
+    Simple pyrsistent object with an invariant that spans multiple fields.
+
+    Diffs which swap the values of the fields will trigger ``InvariantError`
+    unless ``_perform_invariant_check`` is set to ``False`` or the diff is
+    applied to an evolver object.
     """
     _perform_invariant_check = True
     a = field()

From ff7730fad965cca43d6628c4850d98e111c7dfef Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Thu, 7 Jul 2016 12:08:55 +0100
Subject: [PATCH 16/22] A note about why we're importing a private function
 from pyrsistent

---
 flocker/control/_diffing.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index f599e16c44..3a5d58b0b8 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -17,6 +17,10 @@
     pvector,
     pvector_field,
 )
+
+# XXX The _EvolverProxy (or something similar) should be provided by
+# Pyrsistent, but meanwhile we'll import this useful private function.
+# https://github.com/tobgu/pyrsistent/issues/89
 from pyrsistent._transformations import _get
 
 from zope.interface import Interface, implementer

From d452558c95979de8bfe0865a4329dbc1c166dc7a Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Thu, 7 Jul 2016 16:00:26 +0100
Subject: [PATCH 17/22] Add a  attribute to the _Set operation and a new
 _Replace operation for the special case where the root object is replaced
 with a new, unrelated object.

---
 flocker/control/_diffing.py | 50 ++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index 3a5d58b0b8..e51e4405f4 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -49,6 +49,19 @@ def apply(obj):
         """
 
 
+@implementer(_IDiffChange)
+class _Replace(PClass):
+    """
+    A ``_IDiffChange`` that returns a new root object.
+
+    :ivar value: The item to be the new root object for subsequent operations.
+    """
+    value = field()
+
+    def apply(self, obj):
+        return _EvolverProxy(self.value)
+
+
 @implementer(_IDiffChange)
 class _Remove(PClass):
     """
@@ -74,17 +87,17 @@ class _Set(PClass):
     A ``_IDiffChange`` that sets a field in a ``PClass`` or sets a key in a
     ``PMap``.
 
-    :ivar path: The path in the nested object to the field/key to be set to a
-        new value.
-
-    :ivar value: The value to set the field/key to.
+    :ivar path: The path in the nested object which supports `set` operations.
+    :ivar key: The key to set.
+    :ivar value: The value to set.
     """
     path = pvector_field(object)
+    key = field()
     value = field()
 
     def apply(self, obj):
         return obj.transform(
-            self.path[:-1], lambda o: o.set(self.path[-1], self.value)
+            self.path, lambda o: o.set(self.key, self.value)
         )
 
 
@@ -268,11 +281,7 @@ class Diff(PClass):
     def apply(self, obj):
         proxy = _EvolverProxy(original=obj)
         for c in self.changes:
-            if len(c.path) > 0:
-                proxy = c.apply(proxy)
-            else:
-                assert type(c) is _Set
-                proxy = _EvolverProxy(original=c.value)
+            proxy = c.apply(proxy)
         try:
             return proxy.commit()
         except:
@@ -345,7 +354,7 @@ def _create_diffs_for_mappings(current_path, mapping_a, mapping_b):
             )
     for key in b_keys.difference(a_keys):
         resulting_diffs.append(
-            _Set(path=current_path.append(key), value=mapping_b[key])
+            _Set(path=current_path, key=key, value=mapping_b[key])
         )
     for key in a_keys.difference(b_keys):
         resulting_diffs.append(
@@ -373,8 +382,6 @@ def _create_diffs_for(current_path, subobj_a, subobj_b):
     """
     if subobj_a == subobj_b:
         return pvector([])
-    elif type(subobj_a) != type(subobj_b):
-        return pvector([_Set(path=current_path, value=subobj_b)])
     elif isinstance(subobj_a, PClass) and isinstance(subobj_b, PClass):
         a_dict = subobj_a._to_dict()
         b_dict = subobj_b._to_dict()
@@ -388,7 +395,20 @@ def _create_diffs_for(current_path, subobj_a, subobj_b):
     # If the objects are not equal, and there is no intelligent way to recurse
     # inside the objects to make a smaller diff, simply set the current path
     # to the object in b.
-    return pvector([_Set(path=current_path, value=subobj_b)])
+    if len(current_path) > 0:
+        return pvector([
+            _Set(
+                path=current_path[:-1],
+                key=current_path[-1],
+                value=subobj_b
+            )
+        ])
+    # Or if there's no path we're replacing the root object to which subsequent
+    # ``_IDiffChange`` operations will be applied.
+    else:
+        return pvector([
+            _Replace(value=subobj_b)
+        ])
 
 
 def create_diff(object_a, object_b):
@@ -430,5 +450,5 @@ def compose_diffs(iterable_of_diffs):
 
 # Ensure that the representation of a ``Diff`` is entirely serializable:
 DIFF_SERIALIZABLE_CLASSES = [
-    _Set, _Remove, _Add, Diff
+    _Set, _Remove, _Add, Diff, _Replace
 ]

From a24841f00e89d8664f0598eac0f71140dc11b53b Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Thu, 7 Jul 2016 17:50:55 +0100
Subject: [PATCH 18/22] Use Zope Interface to mark classes that support
 Pyrsistent evolver method.

---
 flocker/control/_diffing.py | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index e51e4405f4..ede4de4ef6 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -23,7 +23,7 @@
 # https://github.com/tobgu/pyrsistent/issues/89
 from pyrsistent._transformations import _get
 
-from zope.interface import Interface, implementer
+from zope.interface import Interface, implementer, classImplements
 
 
 class _IDiffChange(Interface):
@@ -120,6 +120,21 @@ def apply(self, obj):
 _sentinel = object()
 
 
+class _IEvolvable(Interface):
+    """
+    An interface to mark classes that provide a ``Pyrsistent`` style
+    ``evolver`` method.
+    """
+    def evolver():
+        """
+        :returns: A mutable version of the underlying object.
+        """
+
+classImplements(PMap, _IEvolvable)
+classImplements(PSet, _IEvolvable)
+classImplements(PClass, _IEvolvable)
+
+
 class _EvolverProxy(object):
     """
     This attempts to bunch all the diff operations for a particular object into
@@ -183,7 +198,7 @@ def add(self, item):
             proxy.
         :returns: ``self``
         """
-        if hasattr(item, 'evolver'):
+        if _IEvolvable.providedBy(item):
             self._children[item] = _EvolverProxy(item)
         else:
             self._evolver.add(item)
@@ -201,7 +216,7 @@ def set(self, key, item):
             this proxy.
         :returns: ``self``
         """
-        if hasattr(item, 'evolver'):
+        if _IEvolvable.providedBy(item):
             # This will replace any existing proxy.
             self._children[key] = _EvolverProxy(item)
         else:

From 14e2af6028b226f276b29cf03b24e61bf959be7e Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Fri, 8 Jul 2016 09:58:30 +0100
Subject: [PATCH 19/22] Give clearer errors when the object supplied to
 _EvolverProxy.transform is not _IEvolvable.

---
 flocker/control/_diffing.py          |  14 ++-
 flocker/control/test/test_diffing.py | 138 ++++++++++++++++++++++++++-
 2 files changed, 148 insertions(+), 4 deletions(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index ede4de4ef6..16fb60a8f8 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -148,6 +148,14 @@ def __init__(self, original):
         :param PClass original: The root object to which transformations will
             be applied.
         """
+        if not _IEvolvable.providedBy(original):
+            raise TypeError(
+                "{!r} does not provide {}".format(
+                    original,
+                    _IEvolvable.__name__
+                )
+            )
+
         self._original = original
         self._evolver = original.evolver()
         self._children = {}
@@ -160,9 +168,9 @@ def _child(self, segment):
         child = _get(self._original, segment, _sentinel)
         if child is _sentinel:
             raise KeyError(
-                'Segment not found in path. '
-                'Parent: {}, '
-                'Segment: {}'.format(self, segment)
+                "Attribute or key '{}' not found in {}".format(
+                    segment, self._original
+                )
             )
         proxy_for_child = _EvolverProxy(child)
         self._children[segment] = proxy_for_child
diff --git a/flocker/control/test/test_diffing.py b/flocker/control/test/test_diffing.py
index d325b47bf1..b9de6f643d 100644
--- a/flocker/control/test/test_diffing.py
+++ b/flocker/control/test/test_diffing.py
@@ -13,7 +13,12 @@
 from pyrsistent import PClass, field, pmap, pset, InvariantException
 from twisted.python.monkey import MonkeyPatcher
 
-from .._diffing import create_diff, compose_diffs, DIFF_COMMIT_ERROR
+from .._diffing import (
+    create_diff,
+    compose_diffs,
+    DIFF_COMMIT_ERROR,
+    _EvolverProxy,
+)
 from .._persistence import wire_encode, wire_decode
 from .._model import Node, Port
 from ..testtools import (
@@ -356,3 +361,134 @@ def test_application_modify(self):
             node2,
             diff.apply(node1),
         )
+
+
+class EvolverProxyTests(TestCase):
+    """
+    Tests for ``_EvolverProxy``.
+    """
+    def test_type_error(self):
+        """
+        The wrapped object must provide _IEvolvable.
+        """
+        e = self.assertRaises(
+            TypeError,
+            _EvolverProxy,
+            1
+        )
+        self.assertEqual(
+            '1 does not provide _IEvolvable',
+            e.message,
+        )
+
+    def test_commit_no_change(self):
+        """
+        ``commit`` returns the original object if no changes have been
+        performed.
+        """
+        original = pmap()
+        self.assertIs(original, _EvolverProxy(original).commit())
+
+    def test_transform_keyerror(self):
+        """
+        ``transform`` raises ``KeyError`` if the supplied ``path`` is not
+        found.
+        """
+        e = self.assertRaises(
+            KeyError,
+            _EvolverProxy(pmap()).transform,
+            ['a'], 1
+        )
+        self.assertEqual(
+            "Attribute or key 'a' not found in pmap({})",
+            e.message,
+        )
+
+    def test_transform_typeerror(self):
+        """
+        ``transform`` raises ``TypeError`` if the object at the supplied
+        ``path`` does not provide ``_IEvolvable``.
+        """
+        proxy = _EvolverProxy(pmap({'a': 1}))
+
+        e = self.assertRaises(
+            TypeError,
+            proxy.transform,
+            ['a'], 2,
+        )
+        self.assertEqual(
+            "1 does not provide _IEvolvable",
+            e.message
+        )
+
+    def test_transform_empty_path(self):
+        """
+        If ``transform`` is supplied with an empty path, the operation is
+        performed on the root object.
+        """
+        proxy = _EvolverProxy(pmap({'a': 1}))
+        proxy.transform([], lambda o: o.set('a', 2))
+        self.assertEqual(
+            pmap({'a': 2}),
+            proxy.commit(),
+        )
+
+    def test_transform_deep_path(self):
+        """
+        If ``transform`` is supplied with a path containing multiple segments,
+        the operation is performed on the object corresponding to the last
+        segment.
+        """
+        proxy = _EvolverProxy(
+            pmap({
+                'a': pmap({
+                    'b': pmap({
+                        'c': 1
+                    })
+                })
+            })
+        )
+        proxy.transform(['a', 'b'], lambda o: o.set('c', 2))
+        self.assertEqual(
+            pmap({
+                'a': pmap({
+                    'b': pmap({
+                        'c': 2
+                    })
+                })
+            }),
+            proxy.commit(),
+        )
+
+    def test_transform_deep_evolver(self):
+        """
+        ``transform`` can perform operations on nested objects that have
+        invariant constraints, without triggering the InvariantException.
+        """
+        proxy = _EvolverProxy(
+            pmap({
+                'a': pmap({
+                    'b': pmap({
+                        'c': DiffTestObjInvariant(
+                            a=1, b=2
+                        )
+                    })
+                })
+            })
+        )
+        # If these operations were performed directly on the Pyrsistent
+        # structure it'd trigger InvariantException.
+        proxy.transform(['a', 'b', 'c'], lambda o: o.set('a', 2))
+        proxy.transform(['a', 'b', 'c'], lambda o: o.set('b', 1))
+        self.assertEqual(
+            pmap({
+                'a': pmap({
+                    'b': pmap({
+                        'c': DiffTestObjInvariant(
+                            a=2, b=1
+                        )
+                    })
+                })
+            }),
+            proxy.commit(),
+        )

From 574cf9218ac6abdd0ecf23f747fe0450b0ffc999 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Fri, 8 Jul 2016 10:12:08 +0100
Subject: [PATCH 20/22] docstring

---
 flocker/control/_diffing.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index 16fb60a8f8..f94252af08 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -183,6 +183,9 @@ def transform(self, path, operation):
         resulting leaf proxy object. This will infact perform the operation on
         an evolver of the original Pyrsistent object.
 
+        The object corresponding to the last segment of ``path`` must provide
+        the ``_IEvolvable`` interface.
+
         :param PVector path: The path relative to ``original`` which will be
             operated on.
         :param callable operation: A function to be applied to an evolver of

From 7ec0238c742ea2edf3ec6bd4f43cbb25f3fa9f53 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Fri, 8 Jul 2016 12:42:37 +0100
Subject: [PATCH 21/22] Refactor things...a lot...to get rid of hasattr checks
 and to have separate proxies for set and record types

---
 flocker/control/_diffing.py          | 274 +++++++++++++++++++--------
 flocker/control/test/test_diffing.py |  20 +-
 2 files changed, 202 insertions(+), 92 deletions(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index f94252af08..f7bfe34a88 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -23,7 +23,7 @@
 # https://github.com/tobgu/pyrsistent/issues/89
 from pyrsistent._transformations import _get
 
-from zope.interface import Interface, implementer, classImplements
+from zope.interface import Attribute, Interface, implementer, classImplements
 
 
 class _IDiffChange(Interface):
@@ -59,7 +59,7 @@ class _Replace(PClass):
     value = field()
 
     def apply(self, obj):
-        return _EvolverProxy(self.value)
+        return _TransformProxy(self.value)
 
 
 @implementer(_IDiffChange)
@@ -130,73 +130,69 @@ def evolver():
         :returns: A mutable version of the underlying object.
         """
 
-classImplements(PMap, _IEvolvable)
 classImplements(PSet, _IEvolvable)
+classImplements(PMap, _IEvolvable)
 classImplements(PClass, _IEvolvable)
 
 
-class _EvolverProxy(object):
-    """
-    This attempts to bunch all the diff operations for a particular object into
-    a single transaction so that related attributes can be ``set`` without
-    triggering an in invariant error.
-    Additionally, the leaf nodes are persisted first and in isolation, so as
-    not to trigger invariant errors in ancestor nodes.
-    """
-    def __init__(self, original):
+class _ISetType(Interface):
+    def add(item):
         """
-        :param PClass original: The root object to which transformations will
-            be applied.
         """
-        if not _IEvolvable.providedBy(original):
-            raise TypeError(
-                "{!r} does not provide {}".format(
-                    original,
-                    _IEvolvable.__name__
-                )
-            )
 
-        self._original = original
-        self._evolver = original.evolver()
-        self._children = {}
-        self._operations = []
-
-    def _child(self, segment):
-        child = self._children.get(segment)
-        if child is not None:
-            return child
-        child = _get(self._original, segment, _sentinel)
-        if child is _sentinel:
-            raise KeyError(
-                "Attribute or key '{}' not found in {}".format(
-                    segment, self._original
-                )
-            )
-        proxy_for_child = _EvolverProxy(child)
-        self._children[segment] = proxy_for_child
-        return proxy_for_child
+    def remove(item):
+        """
+        """
+classImplements(PSet, _ISetType)
 
-    def transform(self, path, operation):
+
+class _IRecordType(Interface):
+    def set(item):
+        """
         """
-        Traverse each segment of ``path`` to create a hierarchy of
-        ``_EvolverProxy`` objects and perform the ``operation`` on the
-        resulting leaf proxy object. This will infact perform the operation on
-        an evolver of the original Pyrsistent object.
 
-        The object corresponding to the last segment of ``path`` must provide
-        the ``_IEvolvable`` interface.
+    def remove(item):
+        """
+        """
+classImplements(PMap, _IRecordType)
+classImplements(PClass, _IRecordType)
 
-        :param PVector path: The path relative to ``original`` which will be
-            operated on.
-        :param callable operation: A function to be applied to an evolver of
-             the object at ``path``
-        :returns: ``self``
+
+class _IRecursiveEvolverProxy(Interface):
+    """
+    An interface which allows a structure of nested ``PClass``, ``PMap``, and
+    ``PSet`` to be evolved recursively.
+    """
+    _original = Attribute(
+        "The root Pyrsistent object that is being evolved. "
+        "Must provide ``_IEvolvable``"
+    )
+    _children = Attribute(
+        "A collection of child ``_IRecursiveEvolverProxy`` objects."
+    )
+
+    def commit():
         """
-        target = self
-        for segment in path:
-            target = target._child(segment)
-        operation(target)
-        return self
+        Recursively persist the structure rooted at ``_original`` starting with
+        leaf nodes.
+
+        :returns: The persisted immutable structure.
+        """
+
+
+@implementer(_IRecursiveEvolverProxy)
+@implementer(_ISetType)
+class _EvolverProxyForSet(object):
+    """
+    A proxy for recursively evolving a ``PSet``.
+    """
+    def __init__(self, original):
+        """
+        :param _ISetType original: See ``_IRecursiveEvolverProxy._original``.
+        """
+        self._original = original
+        self._evolver = original.evolver()
+        self._children = {}
 
     def add(self, item):
         """
@@ -210,11 +206,52 @@ def add(self, item):
         :returns: ``self``
         """
         if _IEvolvable.providedBy(item):
-            self._children[item] = _EvolverProxy(item)
+            self._children[item] = _proxy_for_evolvable_object(item)
         else:
             self._evolver.add(item)
         return self
 
+    def remove(self, item):
+        """
+        Remove the ``item`` in an evolver of the ``original`` ``PSet``, and if
+        the item is an uncommitted ``_EvolverProxy`` remove it from the list of
+        children so that the item is not persisted when the structure is
+        finally committed.
+
+        :param item: The object to be removed from the wrapped ``PSet``
+        :returns: ``self``
+        """
+        self._children.pop(item, None)
+        # Attempt to remove the item from the evolver too.  It may be something
+        # that was replaced rather than added by a previous ``set`` operation.
+        try:
+            self._evolver.remove(item)
+        except KeyError:
+            pass
+        return self
+
+    def commit(self):
+        for segment, child_evolver_proxy in self._children.items():
+            child = child_evolver_proxy.commit()
+            self._evolver.add(child)
+        return self._evolver.persistent()
+
+
+@implementer(_IRecursiveEvolverProxy)
+@implementer(_IRecordType)
+class _EvolverProxyForRecord(object):
+    """
+    A proxy for recursively evolving a ``PMap`` or ``PClass``.
+    """
+    def __init__(self, original):
+        """
+        :param _IRecordType original: See
+            ``_IRecursiveEvolverProxy._original``.
+        """
+        self._original = original
+        self._evolver = original.evolver()
+        self._children = {}
+
     def set(self, key, item):
         """
         Set the ``item`` in an evolver of the ``original`` ``PMap`` or
@@ -229,50 +266,123 @@ def set(self, key, item):
         """
         if _IEvolvable.providedBy(item):
             # This will replace any existing proxy.
-            self._children[key] = _EvolverProxy(item)
+            self._children[key] = _proxy_for_evolvable_object(item)
         else:
             self._evolver.set(key, item)
         return self
 
-    def remove(self, item):
+    def remove(self, key):
         """
-        Remove the ``item`` in an evolver of the ``original`` ``PMap``,
-        ``PClass``, or ``PSet`` and if the item is an uncommitted
-        ``_EvolverProxy`` remove it from the list of children so that the item
-        is not persisted when the structure is finally committed.
+        Remove the ``key`` in an evolver of the ``original`` ``PMap``, or
+        ``PClass`` and if the item is an uncommitted ``_EvolverProxy`` remove
+        it from the list of children so that the item is not persisted when the
+        structure is finally committed.
 
-        :param item: The object to be removed from the wrapped ``PSet`` or the
-            key to be removed from the wrapped ``PMap``
+        :param key: The key to be removed from the wrapped ``PMap``
         :returns: ``self``
         """
-        self._children.pop(item, None)
+        self._children.pop(key, None)
         # Attempt to remove the item from the evolver too.  It may be something
         # that was replaced rather than added by a previous ``set`` operation.
         try:
-            self._evolver.remove(item)
+            self._evolver.remove(key)
         except KeyError:
             pass
         return self
 
     def commit(self):
-        """
-        Persist all the changes made to the descendants of this structure, then
-        persist the resulting sub-objects and local changes to this root object
-        and finally return the resulting immutable structure.
-
-        :returns: The updated and persisted version of ``original``.
-        """
         for segment, child_evolver_proxy in self._children.items():
             child = child_evolver_proxy.commit()
-            # XXX this is ugly. Perhaps have a separate proxy for PClass, PMap
-            # and PSet collections
-            if hasattr(self._evolver, 'set'):
-                self._evolver.set(segment, child)
-            else:
-                self._evolver.add(child)
+            self._evolver.set(segment, child)
         return self._evolver.persistent()
 
 
+def _proxy_for_evolvable_object(obj):
+    """
+    :returns: an ``_IRecursiveEvolverProxy`` suitable for the type of ``obj``.
+    """
+    if not _IEvolvable.providedBy(obj):
+        raise TypeError(
+            "{!r} does not provide {}".format(
+                obj,
+                _IEvolvable.__name__
+            )
+        )
+    if _ISetType.providedBy(obj):
+        return _EvolverProxyForSet(obj)
+    elif _IRecordType.providedBy(obj):
+        return _EvolverProxyForRecord(obj)
+    else:
+        raise TypeError("Object '{}' does not provide a supported interface")
+
+
+def _get_or_add_proxy_child(parent_proxy, segment):
+    """
+    Returns a proxy wrapper around the ``_IEvolvable`` object corresponding to
+    ``segment``. A new proxy is created if one does not already exist and it is
+    added to ``parent_proxy._children``.
+
+    :param _IParentProxy parent_proxy: The parent.
+    :param unicode segment: The label in a ``path`` supplied to ``transform``.
+    :returns:
+    """
+    child = parent_proxy._children.get(segment)
+    if child is not None:
+        return child
+    child = _get(parent_proxy._original, segment, _sentinel)
+    if child is _sentinel:
+        raise KeyError(
+            "Attribute or key '{}' not found in {}".format(
+                segment, parent_proxy._original
+            )
+        )
+    proxy_for_child = _proxy_for_evolvable_object(child)
+    parent_proxy._children[segment] = proxy_for_child
+    return proxy_for_child
+
+
+@implementer(_IRecursiveEvolverProxy)
+class _TransformProxy(object):
+    """
+    This attempts to bunch a the ``transform`` operations performed when
+    applying a sequence of diffs into a single transaction so that related
+    attributes can be ``set`` without triggering an in invariant error.
+    Leaf nodes are persisted first and in isolation, so as not to trigger
+    invariant errors in ancestor nodes.
+    """
+    def __init__(self, original):
+        """
+        :param _IEvolvable original: The root object to which transformations
+            will be applied.
+        """
+        self._root = _proxy_for_evolvable_object(original)
+
+    def transform(self, path, operation):
+        """
+        Traverse each segment of ``path`` to create a hierarchy of
+        ``_EvolverProxy`` objects and perform the ``operation`` on the
+        resulting leaf proxy object. This will infact perform the operation on
+        an evolver of the original Pyrsistent object.
+
+        The object corresponding to the last segment of ``path`` must provide
+        the ``_IEvolvable`` interface.
+
+        :param PVector path: The path relative to ``original`` which will be
+            operated on.
+        :param callable operation: A function to be applied to an evolver of
+             the object at ``path``
+        :returns: ``self``
+        """
+        target = self._root
+        for segment in path:
+            target = _get_or_add_proxy_child(target, segment)
+        operation(target)
+        return self
+
+    def commit(self):
+        return self._root.commit()
+
+
 TARGET_OBJECT = Field(
     u"target_object", repr,
     u"The object to which the diff was applied."
@@ -305,7 +415,7 @@ class Diff(PClass):
     changes = pvector_field(object)
 
     def apply(self, obj):
-        proxy = _EvolverProxy(original=obj)
+        proxy = _TransformProxy(original=obj)
         for c in self.changes:
             proxy = c.apply(proxy)
         try:
diff --git a/flocker/control/test/test_diffing.py b/flocker/control/test/test_diffing.py
index b9de6f643d..0970c64701 100644
--- a/flocker/control/test/test_diffing.py
+++ b/flocker/control/test/test_diffing.py
@@ -17,7 +17,7 @@
     create_diff,
     compose_diffs,
     DIFF_COMMIT_ERROR,
-    _EvolverProxy,
+    _TransformProxy,
 )
 from .._persistence import wire_encode, wire_decode
 from .._model import Node, Port
@@ -363,9 +363,9 @@ def test_application_modify(self):
         )
 
 
-class EvolverProxyTests(TestCase):
+class TransformProxyTests(TestCase):
     """
-    Tests for ``_EvolverProxy``.
+    Tests for ``_TransformProxy``.
     """
     def test_type_error(self):
         """
@@ -373,7 +373,7 @@ def test_type_error(self):
         """
         e = self.assertRaises(
             TypeError,
-            _EvolverProxy,
+            _TransformProxy,
             1
         )
         self.assertEqual(
@@ -387,7 +387,7 @@ def test_commit_no_change(self):
         performed.
         """
         original = pmap()
-        self.assertIs(original, _EvolverProxy(original).commit())
+        self.assertIs(original, _TransformProxy(original).commit())
 
     def test_transform_keyerror(self):
         """
@@ -396,7 +396,7 @@ def test_transform_keyerror(self):
         """
         e = self.assertRaises(
             KeyError,
-            _EvolverProxy(pmap()).transform,
+            _TransformProxy(pmap()).transform,
             ['a'], 1
         )
         self.assertEqual(
@@ -409,7 +409,7 @@ def test_transform_typeerror(self):
         ``transform`` raises ``TypeError`` if the object at the supplied
         ``path`` does not provide ``_IEvolvable``.
         """
-        proxy = _EvolverProxy(pmap({'a': 1}))
+        proxy = _TransformProxy(pmap({'a': 1}))
 
         e = self.assertRaises(
             TypeError,
@@ -426,7 +426,7 @@ def test_transform_empty_path(self):
         If ``transform`` is supplied with an empty path, the operation is
         performed on the root object.
         """
-        proxy = _EvolverProxy(pmap({'a': 1}))
+        proxy = _TransformProxy(pmap({'a': 1}))
         proxy.transform([], lambda o: o.set('a', 2))
         self.assertEqual(
             pmap({'a': 2}),
@@ -439,7 +439,7 @@ def test_transform_deep_path(self):
         the operation is performed on the object corresponding to the last
         segment.
         """
-        proxy = _EvolverProxy(
+        proxy = _TransformProxy(
             pmap({
                 'a': pmap({
                     'b': pmap({
@@ -465,7 +465,7 @@ def test_transform_deep_evolver(self):
         ``transform`` can perform operations on nested objects that have
         invariant constraints, without triggering the InvariantException.
         """
-        proxy = _EvolverProxy(
+        proxy = _TransformProxy(
             pmap({
                 'a': pmap({
                     'b': pmap({

From 7abd8a8530714a4f0ef3390dcd096b80fcad5284 Mon Sep 17 00:00:00 2001
From: Richard Wall <richard.wall@clusterhq.com>
Date: Fri, 8 Jul 2016 12:53:05 +0100
Subject: [PATCH 22/22] Add missing docstrings

---
 flocker/control/_diffing.py | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/flocker/control/_diffing.py b/flocker/control/_diffing.py
index f7bfe34a88..ea20ba5c0f 100644
--- a/flocker/control/_diffing.py
+++ b/flocker/control/_diffing.py
@@ -136,24 +136,36 @@ def evolver():
 
 
 class _ISetType(Interface):
+    """
+    The operations that can be performed when transforming a ``PSet`` object.
+    """
     def add(item):
         """
+        Add ``item`` to set.
         """
 
     def remove(item):
         """
+        Remove ``item`` from set.
         """
+
 classImplements(PSet, _ISetType)
 
 
 class _IRecordType(Interface):
-    def set(item):
+    """
+    The operations that can be performed when transforming a ``PSet`` object.
+    """
+    def set(key, value):
         """
+        Add or replace the ``key`` in a ``PMap`` with ``value``.
         """
 
     def remove(item):
         """
+        Remove the ``key`` in a ``PMap``.
         """
+
 classImplements(PMap, _IRecordType)
 classImplements(PClass, _IRecordType)