fix(testing): set JUJU_REMOTE_APP and allow fetching relation data in…

… relation-broken (#1130)

As far as the Juju team and I have been able to tell (see [comments in the Juju issue](https://bugs.launchpad.net/juju/+bug/1960934)) `JUJU_REMOTE_APP` is always set in relation-broken events.

This PR removes the `Harness` behaviour that simulates `JUJU_REMOTE_APP` *not* being available (which was never consistently the case anyway) and the model code that blocks getting remote data during relation-broken (which is discouraged in the case where the entire integration is going away, but is permitted).

Fixes #1128.

CHANGES.md

@@ -1,6 +1,7 @@
 # 2.11.0
 
 * `StopEvent`, `RemoveEvent`, and all `LifeCycleEvent`s are no longer deferrable, and will raise a `RuntimeError` if `defer()` is called on the event object.
+* The remote app name (and its databag) is now consistently available in relation-broken events.
 * Added `ActionEvent.id`, exposing the JUJU_ACTION_UUID environment variable.
 * Added support for creating `pebble.Plan` objects by passing in a `pebble.PlanDict`, the
   ability to compare two `Plan` objects with `==`, and the ability to create an empty Plan with `Plan()`.

ops/model.py

@@ -1550,15 +1550,6 @@ def __iter__(self):
         return iter(self._data)
 
     def __getitem__(self, key: Union['Unit', 'Application']):
-        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):

ops/testing.py

@@ -2167,21 +2167,9 @@ def relation_remote_app_name(self, relation_id: int) -> Optional[str]:
         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: int, member_name: str, is_app: bool):
-        if 'relation_broken' in self._hook_is_running and not self.relation_remote_app_name(
-                relation_id) and member_name != self.app_name and member_name != self.unit_name:
-            # 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_raw:

test/test_testing.py

@@ -33,7 +33,6 @@
 import uuid
 from unittest.mock import MagicMock, patch
 
-import pytest
 import yaml
 
 import ops
@@ -344,24 +343,6 @@ def _on_cluster_relation_changed(self, event: ops.EventBase):
         self.assertTrue(len(harness.charm.observed_events), 1)
         self.assertIsInstance(harness.charm.observed_events[0], ops.RelationEvent)
 
-    def test_relation_get_when_broken(self):
-        harness = ops.testing.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)  # type: ignore
-
-        with pytest.raises(KeyError, match='trying to access remote app data'):
-            harness.remove_relation(rel_id)
-
     def test_remove_relation(self):
         harness = ops.testing.Harness(RelationEventCharm, meta='''
             name: test-app
@@ -3234,14 +3215,6 @@ def _observe_relation_event(self, event_name: str, event: ops.RelationEvent):
         self.changes.append(recording)
 
 
-class RelationBrokenTester(RelationEventCharm):
-    """Access inaccessible relation data."""
-
-    def _on_relation_broken(self, event: ops.RelationBrokenEvent):
-        # We expect this to fail, because the relation has broken.
-        event.relation.data[event.relation.app]['bar']  # type: ignore
-
-
 class ContainerEventCharm(RecordingCharm):
     """Record events related to container lifecycles."""