Repository: juju-solutions/charms.reactive

Issue:
charms.reactive.relations assumes the relation name is a key
The RelationBase and Conversation model assumes that a relation name name is a key, rather than using the relation id + unit name. This will cause a reactive charm to fail, depending how the related service chooses to use it. For example, here is a metadata.yaml snippet from a client that stores its data in three separate silos:

```yaml
requires:
    session:
        interface: pgsql
    public:
        interface: pgsql
    confidential:
        interface: pgsql
```

On the client side, everything is fine. However, on the server side we have this:

```yaml
provides:
    db:
        interface: pgsql
```

If the client is related three times to the same server, which would be common for development and staging, then the reactive model fails as the relation name can not be used as a key to identify which of the three db interfaces is needed. The relation id must be used.

Discussion:

Patch:
diff --git a/charms/reactive/__init__.py b/charms/reactive/__init__.py
index 1cdcca0..15209ec 100644
--- a/charms/reactive/__init__.py
+++ b/charms/reactive/__init__.py
@@ -36,6 +36,7 @@ from .decorators import only_once  # noqa
 from .decorators import when_file_changed  # noqa
 
 from . import bus
+from . import relations
 from charmhelpers.core import hookenv
 from charmhelpers.core import unitdata
 
@@ -59,6 +60,9 @@ def main(relation_name=None):
     if 'JUJU_HOOK_NAME' not in os.environ:
         os.environ['JUJU_HOOK_NAME'] = os.path.basename(sys.argv[0])
 
+    # update data to be backwards compatible after fix for issue 28
+    relations._migrate_conversations()
+
     def flush_kv():
         if unitdata._KV:
             unitdata._KV.flush()
diff --git a/charms/reactive/relations.py b/charms/reactive/relations.py
index fe513ac..efdfcc4 100644
--- a/charms/reactive/relations.py
+++ b/charms/reactive/relations.py
@@ -22,6 +22,7 @@ from six import with_metaclass
 from charmhelpers.core import hookenv
 from charmhelpers.core import unitdata
 from charmhelpers.cli import cmdline
+from charms.reactive.bus import get_states
 from charms.reactive.bus import get_state
 from charms.reactive.bus import set_state
 from charms.reactive.bus import remove_state
@@ -377,17 +378,25 @@ class Conversation(object):
     Conversations use the idea of :class:`scope` to determine how units and
     services are grouped together.
     """
-    def __init__(self, relation_name=None, units=None, scope=None):
-        self.relation_name = relation_name or hookenv.relation_type()
-        self.units = set(units or [hookenv.remote_unit()])
-        self.scope = scope or hookenv.remote_unit()
+    def __init__(self, namespace, units, scope):
+        self.namespace = namespace
+        self.units = set(units)
+        self.scope = scope
+
+    @classmethod
+    def _key(cls, namespace, scope):
+        return 'reactive.conversations.%s.%s' % (namespace, scope)
 
     @property
     def key(self):
         """
         The key under which this conversation will be stored.
         """
-        return 'reactive.conversations.%s.%s' % (self.relation_name, self.scope)
+        return self._key(self.namespace, self.scope)
+
+    @property
+    def relation_name(self):
+        return self.namespace.split(':')[0]
 
     @property
     def relation_ids(self):
@@ -395,12 +404,13 @@ class Conversation(object):
         The set of IDs of the specific relation instances that this conversation
         is communicating with.
         """
-        relation_ids = []
-        services = set(unit.split('/')[0] for unit in self.units)
-        for relation_id in hookenv.relation_ids(self.relation_name):
-            if hookenv.remote_service_name(relation_id) in services:
-                relation_ids.append(relation_id)
-        return relation_ids
+        if self.scope == scopes.GLOBAL:
+            # the namespace is the relation name and this conv speaks for all
+            # connected instances of that relation
+            return hookenv.relation_ids(self.namespace)
+        else:
+            # the namespace is the relation ID
+            return [self.namespace]
 
     @classmethod
     def join(cls, scope):
@@ -414,14 +424,20 @@ class Conversation(object):
         :meth:`~charmhelpers.core.unitdata.Storage.flush` be called.
         """
         relation_name = hookenv.relation_type()
+        relation_id = hookenv.relation_id()
         unit = hookenv.remote_unit()
         service = hookenv.remote_service_name()
         if scope is scopes.UNIT:
             scope = unit
+            namespace = relation_id
         elif scope is scopes.SERVICE:
             scope = service
-        key = 'reactive.conversations.%s.%s' % (relation_name, scope)
-        conversation = cls.deserialize(unitdata.kv().get(key, {'scope': scope}))
+            namespace = relation_id
+        else:
+            namespace = relation_name
+        key = cls._key(namespace, scope)
+        data = unitdata.kv().get(key, {'namespace': namespace, 'scope': scope, 'units': []})
+        conversation = cls.deserialize(data)
         conversation.units.add(unit)
         unitdata.kv().set(key, cls.serialize(conversation))
         return conversation
@@ -454,8 +470,8 @@ class Conversation(object):
         Serialize a conversation instance for storage.
         """
         return {
-            'relation_name': conversation.relation_name,
-            'units': list(conversation.units),
+            'namespace': conversation.namespace,
+            'units': sorted(conversation.units),
             'scope': conversation.scope,
         }
 
@@ -643,6 +659,48 @@ class Conversation(object):
         return unitdata.kv().get(key, default)
 
 
+def _migrate_conversations():
+    """
+    Due to issue #28 (https://github.com/juju-solutions/charms.reactive/issues/28),
+    conversations needed to be updated to be namespaced per relation ID for SERVICE
+    and UNIT scope.  To ensure backwards compatibility, this updates all convs in
+    the old format to the new.
+
+    TODO: Remove in 2.0.0
+    """
+    for key, data in unitdata.kv().getrange('reactive.conversations.').items():
+        if 'namespace' in data:
+            continue
+        relation_name = data.pop('relation_name')
+        if data['scope'] == scopes.GLOBAL:
+            data['namespace'] = relation_name
+            unitdata.kv().set(key, data)
+        else:
+            # split the conv based on the relation ID
+            new_keys = []
+            for rel_id in hookenv.relation_ids(relation_name):
+                new_key = Conversation._key(rel_id, data['scope'])
+                new_units = set(hookenv.related_units(rel_id)) & set(data['units'])
+                if new_units:
+                    unitdata.kv().set(new_key, {
+                        'namespace': rel_id,
+                        'scope': data['scope'],
+                        'units': sorted(new_units),
+                    })
+                    new_keys.append(new_key)
+            unitdata.kv().unset(key)
+            # update the states pointing to the old conv key to point to the
+            # (potentially multiple) new key(s)
+            for state, value in get_states().items():
+                if not value:
+                    continue
+                if key not in value['conversations']:
+                    continue
+                value['conversations'].remove(key)
+                value['conversations'].extend(new_keys)
+                set_state(state, value)
+
+
 @cmdline.subcommand()
 def relation_call(method, relation_name=None, state=None, *args):
     """Invoke a method on the class implementing a relation via the CLI"""
diff --git a/tox.ini b/tox.ini
index cabe5d9..04bcddf 100644
--- a/tox.ini
+++ b/tox.ini
@@ -19,8 +19,8 @@ commands = flake8 --ignore=E501 {toxinidir}/charms
 
 [testenv:py2]
 basepython = python2
-commands = nosetests
+commands = nosetests {posargs}
 
 [testenv:py3]
 basepython = python3
-commands = nosetests
+commands = nosetests {posargs}


Test patch:
diff --git a/tests/test_relations.py b/tests/test_relations.py
index 197f210..c4977da 100644
--- a/tests/test_relations.py
+++ b/tests/test_relations.py
@@ -31,6 +31,7 @@ class TestAutoAccessors(unittest.TestCase):
         kv_p = mock.patch.object(relations.unitdata, 'kv')
         self.kv = kv_p.start()
         self.addCleanup(kv_p.stop)
+        self.kv.return_value.get.side_effect = lambda k, v=None: v
 
     def test_accessor_doc(self):
         self.assertEqual(DummyRelationSubclass.field_one.__doc__, 'Get the field-one, if available, or None.')
@@ -151,6 +152,25 @@ class TestRelationBase(unittest.TestCase):
         rb.conversation.assert_called_once_with('scope')
         conv.remove_state.assert_called_once_with('state')
 
+    def test_is_state(self):
+        conv = mock.Mock(name='conv')
+        rb = relations.RelationBase('relname', 'unit')
+        rb.conversation = mock.Mock(return_value=conv)
+        rb.conversation.return_value.is_state.return_value = False
+        assert not rb.is_state('state', 'scope')
+        rb.conversation.assert_called_once_with('scope')
+        conv.is_state.assert_called_once_with('state')
+        rb.conversation.return_value.is_state.return_value = True
+        assert rb.is_state('state', 'scope')
+
+    def test_toggle_state(self):
+        conv = mock.Mock(name='conv')
+        rb = relations.RelationBase('relname', 'unit')
+        rb.conversation = mock.Mock(return_value=conv)
+        rb.toggle_state('state', 'active', 'scope')
+        rb.conversation.assert_called_once_with('scope')
+        conv.toggle_state.assert_called_once_with('state', 'active')
+
     def test_set_remote(self):
         conv = mock.Mock(name='conv')
         rb = relations.RelationBase('relname', 'unit')
@@ -190,82 +210,86 @@ class TestConversation(unittest.TestCase):
         if not hasattr(cls, 'assertItemsEqual'):
             cls.assertItemsEqual = cls.assertCountEqual
 
-    @mock.patch.object(relations, 'hookenv')
-    def test_init(self, hookenv):
-        hookenv.relation_type.return_value = 'relation_type'
-        hookenv.remote_unit.return_value = 'remote_unit'
-
-        c1 = relations.Conversation()
-        self.assertEqual(c1.relation_name, 'relation_type')
-        self.assertEqual(c1.units, set(['remote_unit']))
-        self.assertEqual(c1.scope, 'remote_unit')
-
-        c2 = relations.Conversation('rel', ['unit'], 'scope')
-        self.assertEqual(c2.relation_name, 'rel')
-        self.assertEqual(c2.units, set(['unit']))
-        self.assertEqual(c2.scope, 'scope')
-
     def test_key(self):
         c1 = relations.Conversation('rel', ['unit'], 'scope')
         self.assertEqual(c1.key, 'reactive.conversations.rel.scope')
 
-    @mock.patch.object(relations.hookenv, 'remote_service_name')
     @mock.patch.object(relations.hookenv, 'relation_ids')
-    def test_relation_ids(self, relation_ids, remote_service_name):
-        relation_ids.return_value = ['rel:1', 'rel:2', 'rel:3']
-        remote_service_name.side_effect = ['foo', 'bar', 'foo']
-        c1 = relations.Conversation('rel', ['foo/1', 'qux/1', 'foo/2'], 'scope')
-        self.assertEqual(c1.relation_ids, ['rel:1', 'rel:3'])
-        self.assertEqual(remote_service_name.call_args_list, [
-            mock.call('rel:1'),
-            mock.call('rel:2'),
-            mock.call('rel:3'),
-        ])
+    def test_relation_ids(self, relation_ids):
+        relation_ids.return_value = ['rel:1', 'rel:2']
+        c1 = relations.Conversation('rel:0', [], 'scope')
+        self.assertEqual(c1.relation_ids, ['rel:0'])
+        assert not relation_ids.called
+
+        c2 = relations.Conversation('rel', [], relations.scopes.GLOBAL)
+        self.assertEqual(c2.relation_ids, ['rel:1', 'rel:2'])
         relation_ids.assert_called_once_with('rel')
 
     @mock.patch.object(relations, 'unitdata')
     @mock.patch.object(relations, 'hookenv')
     def test_join(self, hookenv, unitdata):
         hookenv.relation_type.return_value = 'relation_type'
+        hookenv.relation_id.return_value = 'relation_type:0'
         hookenv.remote_unit.return_value = 'service/0'
         hookenv.remote_service_name.return_value = 'service'
         unitdata.kv().get.side_effect = [
-            {'scope': 'scope'},
             {
-                'relation_name': 'relation_type',
-                'units': {'service/1'},
+                'namespace': 'relation_type',
+                'units': [],
+                'scope': 'my-global',
+            },
+            {
+                'namespace': 'relation_type:0',
+                'units': ['service/1'],
                 'scope': 'service',
             },
-            {'scope': 'service/0'},
+            {
+                'namespace': 'relation_type:0',
+                'units': [],
+                'scope': 'service/0',
+            },
         ]
-        conv = relations.Conversation.join('scope')
-        self.assertEqual(conv.relation_name, 'relation_type')
+        conv = relations.Conversation.join('my-global')
+        self.assertEqual(conv.namespace, 'relation_type')
         self.assertEqual(conv.units, {'service/0'})
-        self.assertEqual(conv.scope, 'scope')
-        unitdata.kv().get.assert_called_with('reactive.conversations.relation_type.scope', {'scope': 'scope'})
-        unitdata.kv().set.assert_called_with('reactive.conversations.relation_type.scope', {
-            'relation_name': 'relation_type',
+        self.assertEqual(conv.scope, 'my-global')
+        unitdata.kv().get.assert_called_with('reactive.conversations.relation_type.my-global', {
+            'namespace': 'relation_type',
+            'scope': 'my-global',
+            'units': [],
+        })
+        unitdata.kv().set.assert_called_with('reactive.conversations.relation_type.my-global', {
+            'namespace': 'relation_type',
             'units': ['service/0'],
-            'scope': 'scope',
+            'scope': 'my-global',
         })
 
         conv = relations.Conversation.join(relations.scopes.SERVICE)
-        self.assertEqual(conv.relation_name, 'relation_type')
+        self.assertEqual(conv.namespace, 'relation_type:0')
         self.assertEqual(conv.units, {'service/0', 'service/1'})
         self.assertEqual(conv.scope, 'service')
-        unitdata.kv().get.assert_called_with('reactive.conversations.relation_type.service', {'scope': 'service'})
-        self.assertEqual(unitdata.kv().set.call_args[0][0], 'reactive.conversations.relation_type.service')
-        self.assertEqual(unitdata.kv().set.call_args[0][1]['relation_name'], 'relation_type')
-        self.assertItemsEqual(unitdata.kv().set.call_args[0][1]['units'], ['service/0', 'service/1'])
-        self.assertEqual(unitdata.kv().set.call_args[0][1]['scope'], 'service')
+        unitdata.kv().get.assert_called_with('reactive.conversations.relation_type:0.service', {
+            'namespace': 'relation_type:0',
+            'scope': 'service',
+            'units': [],
+        })
+        unitdata.kv().set.assert_called_with('reactive.conversations.relation_type:0.service', {
+            'namespace': 'relation_type:0',
+            'units': ['service/0', 'service/1'],
+            'scope': 'service',
+        })
 
         conv = relations.Conversation.join(relations.scopes.UNIT)
         self.assertEqual(conv.relation_name, 'relation_type')
         self.assertEqual(conv.units, {'service/0'})
         self.assertEqual(conv.scope, 'service/0')
-        unitdata.kv().get.assert_called_with('reactive.conversations.relation_type.service/0', {'scope': 'service/0'})
-        unitdata.kv().set.assert_called_with('reactive.conversations.relation_type.service/0', {
-            'relation_name': 'relation_type',
+        unitdata.kv().get.assert_called_with('reactive.conversations.relation_type:0.service/0', {
+            'namespace': 'relation_type:0',
+            'scope': 'service/0',
+            'units': [],
+        })
+        unitdata.kv().set.assert_called_with('reactive.conversations.relation_type:0.service/0', {
+            'namespace': 'relation_type:0',
             'units': ['service/0'],
             'scope': 'service/0',
         })
@@ -278,7 +302,7 @@ class TestConversation(unittest.TestCase):
         conv.depart()
         self.assertEqual(conv.units, {'service/1'}, 'scope')
         unitdata.kv().set.assert_called_with(conv.key, {
-            'relation_name': 'rel',
+            'namespace': 'rel',
             'units': ['service/1'],
             'scope': 'scope',
         })
@@ -293,16 +317,16 @@ class TestConversation(unittest.TestCase):
     @mock.patch.object(relations, 'unitdata')
     def test_load(self, unitdata):
         unitdata.kv().get.side_effect = [
-            {'relation_name': 'rel1', 'units': ['service/0'], 'scope': 'scope'},
+            {'namespace': 'rel:1', 'units': ['service/0'], 'scope': 'scope'},
             None,
-            {'relation_name': 'rel2', 'units': ['service/1'], 'scope': 'service'},
+            {'namespace': 'rel:2', 'units': ['service/1'], 'scope': 'service'},
         ]
         convs = relations.Conversation.load(['key1', 'key2', 'key3'])
         self.assertEqual(len(convs), 2)
-        self.assertEqual(convs[0].relation_name, 'rel1')
+        self.assertEqual(convs[0].relation_name, 'rel')
         self.assertEqual(convs[0].units, {'service/0'})
         self.assertEqual(convs[0].scope, 'scope')
-        self.assertEqual(convs[1].relation_name, 'rel2')
+        self.assertEqual(convs[1].relation_name, 'rel')
         self.assertEqual(convs[1].units, {'service/1'})
         self.assertEqual(convs[1].scope, 'service')
         self.assertEqual(unitdata.kv().get.call_args_list, [
@@ -460,6 +484,91 @@ class TestConversation(unittest.TestCase):
         kv().get.assert_called_once_with('reactive.conversations.rel.scope.local-data.foo', 'default')
 
 
+class TestMigrateConvs(unittest.TestCase):
+    @mock.patch.object(relations, 'set_state')
+    @mock.patch.object(relations, 'get_states')
+    @mock.patch.object(relations, 'hookenv')
+    @mock.patch.object(relations.unitdata, 'kv')
+    def test_migrate(self, kv, mhookenv, get_states, set_state):
+        kv().getrange.side_effect = [
+            {'reactive.conversations.rel:0.service': {
+                'namespace': 'rel:0',
+            }},
+            {'reactive.conversations.rel.global': {
+                'relation_name': 'rel',
+                'scope': 'global',
+                'units': ['service/0', 'service/1', 'service/3'],
+            }},
+            {'reactive.conversations.rel.service': {
+                'relation_name': 'rel',
+                'scope': 'service',
+                'units': ['service/0', 'service/1', 'service/3'],
+            }},
+            {'reactive.conversations.rel.service/3': {
+                'relation_name': 'rel',
+                'scope': 'service/3',
+                'units': ['service/3'],
+            }},
+        ]
+        mhookenv.relation_ids.return_value = ['rel:1', 'rel:2']
+        mhookenv.related_units.side_effect = [
+            ['service/0', 'service/2'], ['service/3'],
+            ['service/0', 'service/2'], ['service/3'],
+        ]
+        get_states.side_effect = [
+            {
+                'rel.joined': {'conversations': ['reactive.conversations.rel.service']},
+                'foo': None,
+            },
+            {
+                'rel.joined': {'conversations': ['reactive.conversations.rel.service/3']},
+                'foo': {'conversations': []},
+            },
+        ]
+        relations._migrate_conversations()
+        assert not kv().set.called
+
+        kv().set.reset_mock()
+        relations._migrate_conversations()
+        kv().set.assert_called_with('reactive.conversations.rel.global', {
+            'namespace': 'rel',
+            'scope': 'global',
+            'units': ['service/0', 'service/1', 'service/3'],
+        })
+        assert not kv().unset.called
+        assert not set_state.called
+
+        kv().set.reset_mock()
+        relations._migrate_conversations()
+        kv().set.assert_any_call('reactive.conversations.rel:1.service', {
+            'namespace': 'rel:1',
+            'scope': 'service',
+            'units': ['service/0'],
+        })
+        kv().set.assert_called_with('reactive.conversations.rel:2.service', {
+            'namespace': 'rel:2',
+            'scope': 'service',
+            'units': ['service/3'],
+        })
+        kv().unset.assert_called_with('reactive.conversations.rel.service')
+        set_state.assert_called_with('rel.joined', {'conversations': [
+            'reactive.conversations.rel:1.service',
+            'reactive.conversations.rel:2.service',
+        ]})
+
+        kv().set.reset_mock()
+        relations._migrate_conversations()
+        kv().set.assert_called_with('reactive.conversations.rel:2.service/3', {
+            'namespace': 'rel:2',
+            'scope': 'service/3',
+            'units': ['service/3'],
+        })
+        kv().unset.assert_called_with('reactive.conversations.rel.service/3')
+        set_state.assert_called_with('rel.joined', {'conversations': [
+            'reactive.conversations.rel:2.service/3',
+        ]})
+
+
 class TestRelationCall(unittest.TestCase):
     def setUp(self):
         self.r1 = mock.Mock(name='r1')