Repository: canonical/operator

Issue:
OF should gracefully handle blank application names in relation-broken hooks
See the discussion here: https://bugs.launchpad.net/juju/+bug/1960934

In a relation-broken hook, Juju does not set JUJU_REMOTE_APP. This is Working as Intended for Juju, but it raises an exception in the framework, when iterating through relations.

We need to refactor so that we handle the empty value without throwing the exception. 

Discussion:
pengale: I like this approach. It matches what we agreed to do in conversation, and comes with no additional fluff or complication :-)

Slap some tests on it, and I think that we'd be good to merge!

Patch:
diff --git a/ops/model.py b/ops/model.py
index 8425736..45e6a48 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -887,6 +887,15 @@ class RelationData(Mapping['UnitOrApplication', 'RelationDataContent']):
         return iter(self._data)
 
     def __getitem__(self, key: 'UnitOrApplication'):
+        if key is None and self.relation.app is None:
+            # NOTE: if juju gets fixed to set JUJU_REMOTE_APP for relation-broken events, then that
+            # should fix the only case in which we expect key to be None - potentially removing the
+            # need for this error in future ops versions (i.e. if relation.app is guaranteed to not
+            # be None. See https://bugs.launchpad.net/juju/+bug/1960934.
+            raise KeyError(
+                'Cannot index relation data with "None".'
+                ' Are you trying to access remote app data during a relation-broken event?'
+                ' This is not allowed.')
         return self._data[key]
 
     def __repr__(self):
@@ -2081,7 +2090,7 @@ class _ModelBackend:
             event_relation_id = int(os.environ['JUJU_RELATION_ID'].split(':')[-1])
             if relation_id == event_relation_id:
                 # JUJU_RELATION_ID is this relation, use JUJU_REMOTE_APP.
-                return os.environ['JUJU_REMOTE_APP']
+                return os.getenv('JUJU_REMOTE_APP') or None
 
         # If caller is asking for information about another relation, use
         # "relation-list --app" to get it.


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index a116104..c11080c 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -1175,9 +1175,21 @@ class _TestingModelBackend:
         if relation_id not in self._relation_app_and_units:
             # Non-existent or dead relation
             return None
+        if 'relation_broken' in self._hook_is_running:
+            # TODO: if juju ever starts setting JUJU_REMOTE_APP in relation-broken hooks runs,
+            # then we should kill this if clause.
+            # See https://bugs.launchpad.net/juju/+bug/1960934
+            return None
         return self._relation_app_and_units[relation_id]['app']
 
     def relation_get(self, relation_id, member_name, is_app):
+        if 'relation_broken' in self._hook_is_running and not self.relation_remote_app_name(
+                relation_id):
+            # TODO: if juju gets fixed to set JUJU_REMOTE_APP for this case, then we may opt to
+            # allow charms to read/get that (stale) relation data.
+            # See https://bugs.launchpad.net/juju/+bug/1960934
+            raise RuntimeError(
+                'remote-side relation data cannot be accessed during a relation-broken event')
         if is_app and '/' in member_name:
             member_name = member_name.split('/')[0]
         if relation_id not in self._relation_data:
@@ -1185,6 +1197,10 @@ class _TestingModelBackend:
         return self._relation_data[relation_id][member_name].copy()
 
     def relation_set(self, relation_id, key, value, is_app):
+        if 'relation_broken' in self._hook_is_running and not self.relation_remote_app_name(
+                relation_id):
+            raise RuntimeError(
+                'remote-side relation data cannot be accessed during a relation-broken event')
         relation = self._relation_data[relation_id]
         if is_app:
             bucket_key = self.app_name
diff --git a/test/test_model.py b/test/test_model.py
index b672b84..4cd20a0 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -715,12 +715,9 @@ class TestModel(unittest.TestCase):
         self.assertEqual(len(model.storages), 2)
         self.assertEqual(model.storages.keys(), meta.storages.keys())
         self.assertIn('disks', model.storages)
-        try:
+
+        with pytest.raises(KeyError, match='Did you mean'):
             model.storages['does-not-exist']
-        except KeyError as err:
-            assert 'Did you mean' in str(err), 'got wrong error message'
-        except Exception as err:
-            assert False, 'got wrong exception type: ' + str(err)
 
         test_cases = {
             0: {'name': 'disks', 'location': pathlib.Path('/var/srv/disks/0')},
@@ -968,7 +965,6 @@ def test_recursive_push_and_pull(case):
 
     errors = []
     try:
-        print(push_path, case.dst)
         c.push_path(push_path, case.dst)
     except ops.model.MultiPushPullError as err:
         if not case.errors:
@@ -978,9 +974,6 @@ def test_recursive_push_and_pull(case):
     assert case.errors == errors, \
         'push_path gave wrong expected errors: want {}, got {}'.format(case.errors, errors)
     for fpath in case.want:
-
-        for f in ops.model.Container._list_recursive(c.list_files, pathlib.Path('/')):
-            print(f)
         assert c.exists(fpath), 'push_path failed: file {} missing at destination'.format(fpath)
 
     # create pull test case filesystem structure
diff --git a/test/test_testing.py b/test/test_testing.py
index 265f272..32326b9 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -25,6 +25,7 @@ import textwrap
 import unittest
 from io import BytesIO, StringIO
 
+import pytest
 import yaml
 
 import ops.testing
@@ -318,6 +319,24 @@ class TestHarness(unittest.TestCase):
         self.assertTrue(len(harness.charm.observed_events), 1)
         self.assertIsInstance(harness.charm.observed_events[0], RelationEvent)
 
+    def test_relation_get_when_broken(self):
+        harness = Harness(RelationBrokenTester, meta='''
+            name: test-app
+            requires:
+                foo:
+                    interface: foofoo
+            ''')
+        self.addCleanup(harness.cleanup)
+        harness.begin()
+        harness.charm.observe_relation_events('foo')
+
+        # relation remote app is None to mirror production juju behavior where juju doesn't
+        # communicate the remote app to ops.
+        rel_id = harness.add_relation('foo', None)
+
+        with pytest.raises(KeyError, match='trying to access remote app data'):
+            harness.remove_relation(rel_id)
+
     def test_remove_relation(self):
         harness = Harness(RelationEventCharm, meta='''
             name: test-app
@@ -2565,6 +2584,7 @@ class RelationEventCharm(RecordingCharm):
         self.record_relation_data_on_events = False
 
     def observe_relation_events(self, relation_name):
+        self.relation_name = relation_name
         self.framework.observe(self.on[relation_name].relation_created, self._on_relation_created)
         self.framework.observe(self.on[relation_name].relation_joined, self._on_relation_joined)
         self.framework.observe(self.on[relation_name].relation_changed, self._on_relation_changed)
@@ -2607,6 +2627,16 @@ class RelationEventCharm(RecordingCharm):
         self.changes.append(recording)
 
 
+class RelationBrokenTester(RelationEventCharm):
+    """Access inaccessible relation data."""
+
+    def __init__(self, framework):
+        super().__init__(framework)
+
+    def _on_relation_broken(self, event):
+        print(event.relation.data[event.relation.app]['bar'])
+
+
 class ContainerEventCharm(RecordingCharm):
     """Record events related to container lifecycles."""