feat!: simplify testing secret management

README.md

@@ -804,22 +804,27 @@ Scenario has secrets. Here's how you use them.
 state = scenario.State(
     secrets={
         scenario.Secret(
-            {0: {'key': 'public'}},
-            id='foo',
-        ),
-    },
+            tracked_content={'key': 'public'},
+            latest_content={'key': 'public', 'cert': 'private'},
+        )
+    ]
 )
 ```
 
-The only mandatory arguments to Secret are its secret ID (which should be unique) and its 'contents': that is, a mapping
-from revision numbers (integers) to a `str:str` dict representing the payload of the revision.
+The only mandatory arguments to Secret is the `tracked_content` dict: a `str:str`
+mapping representing the content of the revision. If there is a newer revision
+of the content than the one the unit that's handling the event is tracking, then
+`latest_content` should also be provided - if it's not, then Scenario assumes
+that `latest_content` is the `tracked_content`. If there are other revisions of
+the content, simply don't include them: the unit has no way of knowing about
+these.
 
 There are three cases:
 - the secret is owned by this app but not this unit, in which case this charm can only manage it if we are the leader
 - the secret is owned by this unit, in which case this charm can always manage it (leader or not)
-- (default) the secret is not owned by this app nor unit, which means we can't manage it but only view it
+- (default) the secret is not owned by this app nor unit, which means we can't manage it but only view it (this includes user secrets)
 
-Thus by default, the secret is not owned by **this charm**, but, implicitly, by some unknown 'other charm', and that other charm has granted us view rights.
+Thus by default, the secret is not owned by **this charm**, but, implicitly, by some unknown 'other charm' (or a user), and that other has granted us view rights.
 
 The presence of the secret in `State.secrets` entails that we have access to it, either as owners or as grantees. Therefore, if we're not owners, we must be grantees. Absence of a Secret from the known secrets list means we are not entitled to obtaining it in any way. The charm, indeed, shouldn't even know it exists.
 
@@ -833,29 +838,48 @@ To specify a secret owned by this unit (or app):
 state = scenario.State(
     secrets={
         scenario.Secret(
-            {0: {'key': 'private'}},
-            id='foo',
+            {'key': 'private'},
             owner='unit',  # or 'app'
+            # The secret owner has granted access to the "remote" app over some relation with ID 0:
             remote_grants={0: {"remote"}}
-            # the secret owner has granted access to the "remote" app over some relation with ID 0
-        ),
-    },
+        )
+    }
 )
 ```
 
-To specify a secret owned by some other application and give this unit (or app) access to it:
+To specify a secret owned by some other application, or a user secret, and give this unit (or app) access to it:
 
 ```python
 state = scenario.State(
     secrets={
         scenario.Secret(
-            {0: {'key': 'public'}},
-            id='foo',
+            {'key': 'public'},
             # owner=None, which is the default
-            revision=0,  # the revision that this unit (or app) is currently tracking
-        ),
-    },
+        )
+    }
+)
+```
+
+When handling the `secret-expired` and `secret-remove` events, the charm must remove the specified revision of the secret. For `secret-remove`, the revision will no longer be in the `State`, because it's no longer in use (which is why the `secret-remove` event was triggered). To ensure that the charm is removing the secret, check the context for the history of secret removal:
+
+```python
+class SecretCharm(ops.CharmBase):
+    def __init__(self, framework):
+        super().__init__(framework)
+        self.framework.observe(self.on.secret_remove, self._on_secret_remove)
+
+    def _on_secret_remove(self, event):
+        event.secret.remove_revision(event.revision)
+
+
+ctx = Context(SecretCharm, meta={"name": "foo"})
+secret = Secret({"password": "xxxxxxxx"}, owner="app")
+old_revision = 42
+state = ctx.run(
+    ctx.on.secret_remove(secret, revision=old_revision),
+    State(leader=True, secrets={secret})
 )
+assert ctx.removed_secret_revisions == [old_revision]
 ```
 
 ## StoredState

scenario/context.py

@@ -376,6 +376,7 @@ class Context:
     - :attr:`app_status_history`: record of the app statuses the charm has set
     - :attr:`unit_status_history`: record of the unit statuses the charm has set
     - :attr:`workload_version_history`: record of the workload versions the charm has set
+    - :attr:`removed_secret_revisions`: record of the secret revisions the charm has removed
     - :attr:`emitted_events`: record of the events (including custom) that the charm has processed
 
     This allows you to write assertions not only on the output state, but also, to some
@@ -441,48 +442,6 @@ def __init__(
     ):
         """Represents a simulated charm's execution context.
 
-        It is the main entry point to running a scenario test.
-
-        It contains: the charm source code being executed, the metadata files associated with it,
-        a charm project repository root, and the juju version to be simulated.
-
-        After you have instantiated Context, typically you will call one of `run()` or
-        `run_action()` to execute the charm once, write any assertions you like on the output
-        state returned by the call, write any assertions you like on the Context attributes,
-        then discard the Context.
-        Each Context instance is in principle designed to be single-use:
-        Context is not cleaned up automatically between charm runs.
-        You can call `.clear()` to do some clean up, but we don't guarantee all state will be gone.
-
-        Any side effects generated by executing the charm, that are not rightful part of the State,
-        are in fact stored in the Context:
-        - ``juju_log``: record of what the charm has sent to juju-log
-        - ``app_status_history``: record of the app statuses the charm has set
-        - ``unit_status_history``: record of the unit statuses the charm has set
-        - ``workload_version_history``: record of the workload versions the charm has set
-        - ``emitted_events``: record of the events (including custom ones) that the charm has
-            processed
-
-        This allows you to write assertions not only on the output state, but also, to some
-        extent, on the path the charm took to get there.
-
-        A typical scenario test will look like:
-
-        >>> from scenario import Context, State
-        >>> from ops import ActiveStatus
-        >>> from charm import MyCharm, MyCustomEvent  # noqa
-        >>>
-        >>> def test_foo():
-        >>>     # Arrange: set the context up
-        >>>     c = Context(MyCharm)
-        >>>     # Act: prepare the state and emit an event
-        >>>     state_out = c.run(c.update_status(), State())
-        >>>     # Assert: verify the output state is what you think it should be
-        >>>     assert state_out.unit_status == ActiveStatus('foobar')
-        >>>     # Assert: verify the Context contains what you think it should
-        >>>     assert len(c.emitted_events) == 4
-        >>>     assert isinstance(c.emitted_events[3], MyCustomEvent)
-
         :arg charm_type: the CharmBase subclass to call ``ops.main()`` on.
         :arg meta: charm metadata to use. Needs to be a valid metadata.yaml format (as a dict).
             If none is provided, we will search for a ``metadata.yaml`` file in the charm root.
@@ -497,16 +456,6 @@ def __init__(
         :arg app_trusted: whether the charm has Juju trust (deployed with ``--trust`` or added with
             ``juju trust``). Defaults to False.
         :arg charm_root: virtual charm root the charm will be executed with.
-            If the charm, say, expects a `./src/foo/bar.yaml` file present relative to the
-            execution cwd, you need to use this. E.g.:
-
-            >>> import scenario
-            >>> import tempfile
-            >>> virtual_root = tempfile.TemporaryDirectory()
-            >>> local_path = Path(local_path.name)
-            >>> (local_path / 'foo').mkdir()
-            >>> (local_path / 'foo' / 'bar.yaml').write_text('foo: bar')
-            >>> scenario.Context(... charm_root=virtual_root).run(...)
         """
 
         if not any((meta, actions, config)):
@@ -551,6 +500,7 @@ def __init__(
         self.app_status_history: List["_EntityStatus"] = []
         self.unit_status_history: List["_EntityStatus"] = []
         self.workload_version_history: List[str] = []
+        self.removed_secret_revisions: List[int] = []
         self.emitted_events: List[EventBase] = []
         self.requested_storages: Dict[str, int] = {}
 

scenario/mocking.py

@@ -2,7 +2,6 @@
 # Copyright 2023 Canonical Ltd.
 # See LICENSE file for licensing details.
 import datetime
-import random
 import shutil
 from io import StringIO
 from pathlib import Path
@@ -202,21 +201,16 @@ def _get_secret(self, id=None, label=None):
             return secrets[0]
 
         elif label:
-            secrets = [s for s in self._state.secrets if s.label == label]
-            if not secrets:
-                raise SecretNotFoundError(label)
-            return secrets[0]
+            try:
+                return self._state.get_secret(label=label)
+            except KeyError:
+                raise SecretNotFoundError(label) from None
 
         else:
             # if all goes well, this should never be reached. ops.model.Secret will check upon
             # instantiation that either an id or a label are set, and raise a TypeError if not.
             raise RuntimeError("need id or label.")
 
-    @staticmethod
-    def _generate_secret_id():
-        id = "".join(map(str, [random.choice(list(range(10))) for _ in range(20)]))
-        return f"secret:{id}"
-
     def _check_app_data_access(self, is_app: bool):
         if not isinstance(is_app, bool):
             raise TypeError("is_app parameter to relation_get must be a boolean")
@@ -368,10 +362,8 @@ def secret_add(
     ) -> str:
         from scenario.state import Secret
 
-        secret_id = self._generate_secret_id()
         secret = Secret(
-            id=secret_id,
-            contents={0: content},
+            content,
             label=label,
             description=description,
             expire=expire,
@@ -381,7 +373,7 @@ def secret_add(
         secrets = set(self._state.secrets)
         secrets.add(secret)
         self._state._update_secrets(frozenset(secrets))
-        return secret_id
+        return secret.id
 
     def _check_can_manage_secret(
         self,
@@ -411,19 +403,19 @@ def secret_get(
         secret = self._get_secret(id, label)
         juju_version = self._context.juju_version
         if not (juju_version == "3.1.7" or juju_version >= "3.3.1"):
-            # in this medieval juju chapter,
+            # In this medieval Juju chapter,
             # secret owners always used to track the latest revision.
             # ref: https://bugs.launchpad.net/juju/+bug/2037120
             if secret.owner is not None:
                 refresh = True
 
-        revision = secret.revision
         if peek or refresh:
-            revision = max(secret.contents.keys())
             if refresh:
-                secret._set_revision(revision)
+                secret._track_latest_revision()
+            assert secret.latest_content is not None
+            return secret.latest_content
 
-        return secret.contents[revision]
+        return secret.tracked_content
 
     def secret_info_get(
         self,
@@ -439,7 +431,7 @@ def secret_info_get(
         return SecretInfo(
             id=secret.id,
             label=secret.label,
-            revision=max(secret.contents),
+            revision=secret._latest_revision,
             expires=secret.expire,
             rotation=secret.rotate,
             rotates=None,  # not implemented yet.
@@ -458,6 +450,15 @@ def secret_set(
         secret = self._get_secret(id, label)
         self._check_can_manage_secret(secret)
 
+        if content == secret.latest_content:
+            # In Juju 3.6 and higher, this is a no-op, but it's good to warn
+            # charmers if they are doing this, because it's not generally good
+            # practice.
+            # https://bugs.launchpad.net/juju/+bug/2069238
+            logger.warning(
+                f"secret {id} contents set to the existing value: new revision not needed",
+            )
+
         secret._update_metadata(
             content=content,
             label=label,
@@ -496,10 +497,32 @@ def secret_remove(self, id: str, *, revision: Optional[int] = None):
         secret = self._get_secret(id)
         self._check_can_manage_secret(secret)
 
-        if revision:
-            del secret.contents[revision]
-        else:
-            secret.contents.clear()
+        # Removing all revisions means that the secret is removed, so is no
+        # longer in the state.
+        if revision is None:
+            secrets = set(self._state.secrets)
+            secrets.remove(secret)
+            self._state._update_secrets(frozenset(secrets))
+            return
+
+        # Juju does not prevent removing the tracked or latest revision, but it
+        # is always a mistake to do this. Rather than having the state model a
+        # secret where the tracked/latest revision cannot be retrieved but the
+        # secret still exists, we raise instead, so that charms know that there
+        # is a problem with their code.
+        if revision in (secret._tracked_revision, secret._latest_revision):
+            raise ValueError(
+                "Charms should not remove the latest revision of a secret. "
+                "Add a new revision with `set_content()` instead, and the previous "
+                "revision will be cleaned up by the secret owner when no longer in use.",
+            )
+
+        # For all other revisions, the content is not visible to the charm
+        # (this is as designed: the secret is being removed, so it should no
+        # longer be in use). That means that the state does not need to be
+        # modified - however, unit tests should be able to verify that the remove call was
+        # executed, so we track that in a history list in the context.
+        self._context.removed_secret_revisions.append(revision)
 
     def relation_remote_app_name(
         self,