fix: Secret.set_info and Secret.set_content can be called in the same hook

ops/charm.py

@@ -941,7 +941,12 @@ def secret(self) -> model.Secret:
         until :meth:`Secret.get_content()` is called.
         """
         backend = self.framework.model._backend
-        return model.Secret(backend=backend, id=self._id, label=self._label)
+        return model.Secret(
+            backend=backend,
+            id=self._id,
+            label=self._label,
+            _secret_set_cache=self.framework.model._cache._secret_set_cache,
+        )
 
     def snapshot(self) -> Dict[str, Any]:
         """Used by the framework to serialize the event to disk.

ops/model.py

@@ -14,6 +14,7 @@
 
 """Representations of Juju's model, application, unit, and other entities."""
 
+import collections
 import dataclasses
 import datetime
 import enum
@@ -304,7 +305,13 @@ def get_secret(self, *, id: Optional[str] = None, label: Optional[str] = None) -
             # Canonicalize to "secret:<id>" form for consistency in backend calls.
             id = Secret._canonicalize_id(id, self.uuid)
         content = self._backend.secret_get(id=id, label=label)
-        return Secret(self._backend, id=id, label=label, content=content)
+        return Secret(
+            self._backend,
+            id=id,
+            label=label,
+            content=content,
+            _secret_set_cache=self._cache._secret_set_cache,
+        )
 
     def get_cloud_spec(self) -> 'CloudSpec':
         """Get details of the cloud in which the model is deployed.
@@ -333,6 +340,9 @@ class _ModelCache:
     def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend'):
         self._meta = meta
         self._backend = backend
+        self._secret_set_cache: collections.defaultdict[str, Dict[str, Any]] = (
+            collections.defaultdict(dict)
+        )
         self._weakrefs: _WeakCacheType = weakref.WeakValueDictionary()
 
     @typing.overload
@@ -502,7 +512,13 @@ def add_secret(
             rotate=rotate,
             owner='application',
         )
-        return Secret(self._backend, id=id, label=label, content=content)
+        return Secret(
+            self._backend,
+            id=id,
+            label=label,
+            content=content,
+            _secret_set_cache=self._cache._secret_set_cache,
+        )
 
 
 def _calculate_expiry(
@@ -685,7 +701,13 @@ def add_secret(
             rotate=rotate,
             owner='unit',
         )
-        return Secret(self._backend, id=id, label=label, content=content)
+        return Secret(
+            self._backend,
+            id=id,
+            label=label,
+            content=content,
+            _secret_set_cache=self._cache._secret_set_cache,
+        )
 
     def open_port(
         self, protocol: typing.Literal['tcp', 'udp', 'icmp'], port: Optional[int] = None
@@ -1270,6 +1292,7 @@ def __init__(
         id: Optional[str] = None,
         label: Optional[str] = None,
         content: Optional[Dict[str, str]] = None,
+        _secret_set_cache: Optional['collections.defaultdict[str, Dict[str, Any]]'] = None,
     ):
         if not (id or label):
             raise TypeError('Must provide an id or label, or both')
@@ -1279,6 +1302,9 @@ def __init__(
         self._id = id
         self._label = label
         self._content = content
+        self._secret_set_cache: collections.defaultdict[str, Dict[str, Any]] = (
+            collections.defaultdict(dict) if _secret_set_cache is None else _secret_set_cache
+        )
 
     def __repr__(self):
         fields: List[str] = []
@@ -1483,7 +1509,26 @@ def set_content(self, content: Dict[str, str]):
         self._validate_content(content)
         if self._id is None:
             self._id = self.get_info().id
-        self._backend.secret_set(typing.cast(str, self.id), content=content)
+
+        # If `_backend.secret_set` has already been called, this call will
+        # overwrite anything done in the previous call (even if it was in a
+        # different event handler, as long as it was in the same Juju hook
+        # context). We want to provide more predictable behavior, so we cache
+        # the values and provide them in subsequent calls.
+        cached_data = self._secret_set_cache[self._id]
+        expire = _calculate_expiry(cached_data['expire']) if 'expire' in cached_data else None
+        # Don't store the actual secret content in memory, but put a sentinel
+        # there to indicate that the content has been set during this hook.
+        cached_data['content'] = object()
+
+        self._backend.secret_set(
+            typing.cast(str, self.id),
+            content=content,
+            label=cached_data.get('label'),
+            description=cached_data.get('description'),
+            expire=expire,
+            rotate=cached_data.get('rotate'),
+        )
         # We do not need to invalidate the cache here, as the content is the
         # same until `refresh` is used, at which point the cache is invalidated.
 
@@ -1513,12 +1558,33 @@ def set_info(
         """
         if label is None and description is None and expire is None and rotate is None:
             raise TypeError(
-                'Must provide a label, description, expiration time, ' 'or rotation policy'
+                'Must provide a label, description, expiration time, or rotation policy'
             )
         if self._id is None:
             self._id = self.get_info().id
+
+        # If `_backend.secret_set` has already been called, this call will
+        # overwrite anything done in the previous call (even if it was in a
+        # different event handler, as long as it was in the same Juju hook
+        # context). We want to provide more predictable behavior, so we cache
+        # the values and provide them in subsequent calls.
+        cached_data = self._secret_set_cache[self._id]
+        label = cached_data.get('label') if label is None else label
+        cached_data['label'] = label
+        description = cached_data.get('description') if description is None else description
+        cached_data['description'] = description
+        expire = cached_data.get('expire') if expire is None else expire
+        cached_data['expire'] = expire
+        rotate = cached_data.get('rotate') if rotate is None else rotate
+        cached_data['rotate'] = rotate
+        # Get the previous content from the unit agent's cache.
+        content = (
+            self._backend.secret_get(id=self._id, peek=True) if 'content' in cached_data else None
+        )
+
         self._backend.secret_set(
             typing.cast(str, self.id),
+            content=content,
             label=label,
             description=description,
             expire=_calculate_expiry(expire),
@@ -3664,7 +3730,8 @@ def secret_set(
         if expire is not None:
             args.extend(['--expire', expire.isoformat()])
         if rotate is not None:
-            args += ['--rotate', rotate.value]
+            args.extend(['--rotate', rotate.value])
+
         with tempfile.TemporaryDirectory() as tmp:
             # The content is None or has already been validated with Secret._validate_content
             for k, v in (content or {}).items():
@@ -3691,9 +3758,9 @@ def secret_add(
         if expire is not None:
             args.extend(['--expire', expire.isoformat()])
         if rotate is not None:
-            args += ['--rotate', rotate.value]
+            args.extend(['--rotate', rotate.value])
         if owner is not None:
-            args += ['--owner', owner]
+            args.extend(['--owner', owner])
         with tempfile.TemporaryDirectory() as tmp:
             # The content has already been validated with Secret._validate_content
             for k, v in content.items():

test/test_charm.py

@@ -797,6 +797,32 @@ def on_secret_expired(self, event: ops.SecretExpiredEvent):
     ]
 
 
+def test_secret_event_caches_secret_set(request: pytest.FixtureRequest, fake_script: FakeScript):
+    class MyCharm(ops.CharmBase):
+        def __init__(self, framework: ops.Framework):
+            super().__init__(framework)
+            self.secrets: typing.List[ops.Secret] = []
+            self.framework.observe(self.on.secret_changed, self.on_secret_changed)
+
+        def on_secret_changed(self, event: ops.SecretChangedEvent):
+            event.secret.set_info(description='desc')
+            event.secret.set_content({'key': 'value'})
+            self.secrets.append(event.secret)
+
+    fake_script.write('secret-get', """echo '{"key": "value"}'""")
+    fake_script.write('secret-set', 'exit 0')
+
+    framework = create_framework(request)
+    charm = MyCharm(framework)
+
+    charm.on.secret_changed.emit('secret:changed', None)
+    charm.on.secret_changed.emit('secret:changed', None)
+    cache = charm.secrets[0]._secret_set_cache
+    assert cache is charm.secrets[1]._secret_set_cache
+    assert charm.secrets[0]._secret_set_cache['secret:changed']['description'] == 'desc'
+    assert 'content' in cache['secret:changed']
+
+
 def test_collect_app_status_leader(request: pytest.FixtureRequest, fake_script: FakeScript):
     class MyCharm(ops.CharmBase):
         def __init__(self, framework: ops.Framework):

test/test_model.py

@@ -3823,6 +3823,62 @@ def test_set_info(self, model: ops.Model, fake_script: FakeScript):
         with pytest.raises(TypeError):
             secret.set_info()  # no args provided
 
+    def test_set_content_then_info(self, model: ops.Model, fake_script: FakeScript):
+        fake_script.write('secret-set', """exit 0""")
+        fake_script.write('secret-get', """echo '{"foo": "bar"}'""")
+
+        secret = self.make_secret(model, id='q')
+        secret.set_content({'foo': 'bar'})
+        description = 'desc'
+        secret.set_info(description=description)
+
+        calls = fake_script.calls(clear=True)
+        assert calls == [
+            ['secret-set', f'secret://{model._backend.model_uuid}/q', mock.ANY],
+            ['secret-get', f'secret://{model._backend.model_uuid}/q', '--peek', '--format=json'],
+            [
+                'secret-set',
+                f'secret://{model._backend.model_uuid}/q',
+                '--description',
+                description,
+                mock.ANY,
+            ],
+        ]
+        # For this test we don't need to check that the content was in the temporary file, but we
+        # do want to be certain that there was a file passed to the call (but don't care about
+        # what the name of the file was).
+        assert calls[0][-1].startswith('foo#file=') and calls[0][-1].endswith('/foo')
+        assert calls[2][-1].startswith('foo#file=') and calls[2][-1].endswith('/foo')
+
+    def test_set_info_then_content(self, model: ops.Model, fake_script: FakeScript):
+        fake_script.write('secret-set', """exit 0""")
+
+        secret = self.make_secret(model, id='q')
+        description = 'desc'
+        secret.set_info(description=description)
+        secret.set_content({'foo': 'bar'})
+
+        calls = fake_script.calls(clear=True)
+        assert calls == [
+            [
+                'secret-set',
+                f'secret://{model._backend.model_uuid}/q',
+                '--description',
+                description,
+            ],
+            [
+                'secret-set',
+                f'secret://{model._backend.model_uuid}/q',
+                '--description',
+                description,
+                mock.ANY,
+            ],
+        ]
+        # For this test we don't need to check that the content was in the temporary file, but we
+        # do want to be certain that there was a file passed to the call (but don't care about
+        # what the name of the file was).
+        assert calls[1][-1].startswith('foo#file=') and calls[1][-1].endswith('/foo')
+
     def test_grant(self, model: ops.Model, fake_script: FakeScript):
         fake_script.write('relation-list', """echo '[]'""")
         fake_script.write('secret-grant', """exit 0""")