Repository: canonical/operator

Issue:
Harness and Juju behave differently when using model.get_relation in leader-elected
In the `leader-elected` hook, trying to get a relation fails with an error when using Harness, but works (returning `None`) with Juju. For example, with this charm:

```yaml
requires:
    nonoptrel:
        interface: foo
        optional: false
    optrel:
        interface: bar
        optional: true
```

```python
import logging

import ops

logger = logging.getLogger(__name__)


class LeaderRelationCharm(ops.CharmBase):
    """Charm the application."""

    def __init__(self, framework):
        super().__init__(framework)
        framework.observe(self.on.leader_elected, self._on_leader)

    def _on_leader(self, event):
        logger.info("Optional rel: %s", self.model.get_relation("optrel"))
        logger.info("Non-optional rel: %s", self.model.get_relation("nonoptrel"))


if __name__ == "__main__":  # pragma: nocover
    ops.main(LeaderRelationCharm)  # type: ignore
```

There's this debug-log output:

```
unit-leader-relation-0: 21:36:03 INFO unit.leader-relation/0.juju-log Optional rel: None
unit-leader-relation-0: 21:36:03 INFO unit.leader-relation/0.juju-log Non-optional rel: None
```

But with this test:

```python
class TestCharm(unittest.TestCase):
    def setUp(self):
        self.harness = ops.testing.Harness(LeaderRelationCharm)
        self.addCleanup(self.harness.cleanup)
        self.harness.begin()

    def test_leader_elected(self):
        self.harness.set_leader(True)
```

you get:

```
Traceback (most recent call last):
  File "/usr/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
[..]
  File "/home/tameyer/scratch/leader-relation/.tox/unit/lib/python3.11/site-packages/ops/framework.py", line 955, in _reemit
    custom_handler(event)
  File "/home/tameyer/scratch/leader-relation/src/charm.py", line 22, in _on_leader
    logger.info("Optional rel: %s", self.model.get_relation("optrel"))
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tameyer/scratch/leader-relation/.tox/unit/lib/python3.11/site-packages/ops/model.py", line 242, in get_relation
    return self.relations._get_unique(relation_name, relation_id)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tameyer/scratch/leader-relation/.tox/unit/lib/python3.11/site-packages/ops/model.py", line 868, in _get_unique
    self._backend._validate_relation_access(
  File "/home/tameyer/scratch/leader-relation/.tox/unit/lib/python3.11/site-packages/ops/testing.py", line 2121, in _validate_relation_access
    raise RuntimeError(
RuntimeError: cannot access relation data without first adding the relation: use Harness.add_relation('optrel', <app>) before calling set_leader
```

This was introduced in #733, but from the PR description, the intention was to protect against trying to access a peer relation, because the peer relation will always be set up (by Juju) prior to leader-elected, so you need to add the peer relation in Harness as well.

However, this isn't the case with other relations, and it doesn't seem right that Harness is behaving differently to Juju in this respect.

It seems to me that it's maybe ok for Harness to protect users in the peer-relation case: it's saying "this can't possibly happen with Juju - you need to adjust your test" (although maybe it could say it a bit clearer?). However, in the non-peer case, I think we should stick with the Juju behaviour. I'm not totally sure about the peer-relation case - it seems consistency and a warning might be better? It would also be good to verify that Juju does indeed always set up the peer relation before emitting leader-elected.

Discussion:
tonyandrewmeyer: > * [ ]  [Check with Juju team whether peer-relations are actually guaranteed to be set up before leader-elected](https://matrix.to/#/!xzmWHtGpPfVCXKivIh:ubuntu.com/$JUrZzqWEpE5nSQPelnpsG3Mbrd5sGOEiGY989MD-3hs?via=ubuntu.com&via=matrix.org&via=fsfe.org)

The answer is that this is current behaviour, not guaranteed behaviour. So it's extra bad for ops  (Harness) to behave like it's guaranteed.

Patch:
diff --git a/ops/model.py b/ops/model.py
index 9ccadcd..d028feb 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -45,7 +45,6 @@ from typing import (
     Mapping,
     MutableMapping,
     Optional,
-    Sequence,
     Set,
     TextIO,
     Tuple,
@@ -880,8 +879,6 @@ class RelationMapping(Mapping[str, List['Relation']]):
                                 self._our_unit, self._backend, self._cache, active=False)
         relations = self[relation_name]
         num_related = len(relations)
-        self._backend._validate_relation_access(
-            relation_name, relations)
         if num_related == 0:
             return None
         elif num_related == 1:
@@ -3043,14 +3040,6 @@ class _ModelBackend:
     def _is_relation_not_found(model_error: Exception) -> bool:
         return 'relation not found' in str(model_error)
 
-    def _validate_relation_access(self, relation_name: str, relations: Sequence['Relation']):
-        """Checks for relation usage inconsistent with the framework/backend state.
-
-        This is used for catching Harness configuration errors and the production implementation
-        here should remain empty.
-        """
-        pass
-
     def relation_ids(self, relation_name: str) -> List[int]:
         relation_ids = self._run('relation-ids', relation_name, return_output=True, use_json=True)
         relation_ids = typing.cast(Iterable[str], relation_ids)


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index 5d8cc83..befd5d8 100644
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -2171,22 +2171,6 @@ class _TestingModelBackend:
         self._running_action: Optional[_RunningAction] = None
         self._cloud_spec: Optional[model.CloudSpec] = None
 
-    def _validate_relation_access(self, relation_name: str, relations: List[model.Relation]):
-        """Ensures that the named relation exists/has been added.
-
-        This is called whenever relation data is accessed via model.get_relation(...).
-        """
-        if len(relations) > 0:
-            return
-
-        valid_relation_endpoints: List[str] = list(self._meta.peers.keys())
-        valid_relation_endpoints.extend(self._meta.requires.keys())
-        valid_relation_endpoints.extend(self._meta.provides.keys())
-        if self._hook_is_running == 'leader_elected' and relation_name in valid_relation_endpoints:
-            raise RuntimeError(
-                'cannot access relation data without first adding the relation: '
-                f'use Harness.add_relation({relation_name!r}, <app>) before calling set_leader')
-
     def _can_connect(self, pebble_client: '_TestingPebbleClient') -> bool:
         """Returns whether the mock client is active and can support API calls with no errors."""
         return self._pebble_clients_can_connect[pebble_client]
diff --git a/test/test_testing.py b/test/test_testing.py
index 504a851..883f1eb 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -45,21 +45,6 @@ from ops.testing import ExecResult, _TestingPebbleClient
 is_linux = platform.system() == 'Linux'
 
 
-class SetLeaderErrorTester(ops.CharmBase):
-    """Sets peer relation data inside leader-elected."""
-
-    def __init__(self, framework: ops.Framework):
-        super().__init__(framework)
-        self._peer_name = 'peer'
-        self.framework.observe(self.on.leader_elected,
-                               self._on_leader_elected)
-
-    def _on_leader_elected(self, event: ops.EventBase):
-        peers = self.model.get_relation(self._peer_name)
-        assert peers is not None
-        peers.data[self.app]["foo"] = "bar"
-
-
 class StorageTester(ops.CharmBase):
     """Record the relation-changed events."""
 
@@ -893,21 +878,6 @@ class TestHarness(unittest.TestCase):
         self.assertEqual(rel.data[harness.charm.model.unit]['key'], 'v4')
         self.assertEqual([], helper.changes)
 
-    def test_harness_leader_misconfig(self):
-        # language=YAML
-        harness = ops.testing.Harness(SetLeaderErrorTester, meta='''
-            name: postgresql
-            peers:
-              peer:
-                interface: foo
-            ''')
-        self.addCleanup(harness.cleanup)
-        harness.begin()
-
-        with self.assertRaises(RuntimeError) as cm:
-            harness.set_leader(is_leader=True)
-        self.assertTrue(cm.exception.args[0].find('use Harness.add_relation') != -1)
-
     def test_update_peer_relation_app_data(self):
         # language=YAML
         harness = ops.testing.Harness(ops.CharmBase, meta='''