Repository: canonical/operator
Pull request #1226 (2024-05-22 19:33:50)

fix: the `other` argument to `RelatationDataContent.update(...)` should be optional
Fixes the signature for `RelationDataContent.update` to match `MutableMapping`, where `other` is optional (a regression introduced in #1883).

The type for `other` has been simplified to `Any`. It should really be `Mapping|_SupportsKeysAndGetItem[str,str]` plus a minimal type that supports `.values`, but it was already messy pulling in `_SupportsKeysAndGetItem` in #1183, and we're just passing this through to `MutableMapping` so it doesn't seem like the tight typing is providing enough benefit to justify the complexity of the signature. [typeshed has three overloads](https://github.com/python/typeshed/blob/f7c03486ee01c8ea74823db75e017341bf3c2ad0/stdlib/typing.pyi#L726), so we could match that (as we did in #1883, just incompletely), if that is desirable.

Fixes: #1225

addyess: @tonyandrewmeyer and @benhoyt  I appreciate your speedy review as this likely break a few releases queued up for this week.
dimaqq: The change makes sense, I believe Tony volunteered to tests for this.
tonyandrewmeyer: > Super-minor/bikeshedding: I suppose the test could be more concise (either parametrised of performing all 3 updates on the same object/within same test).

Fair point. I have never really been in the habit of using `unittest.subTest` for this, but now that we are (very nearly) switched over to pytest, using `pytest.mark.parametrize` would be cleaner.
addyess: Wow, thanks for the improved solution @dimaqq!
dimaqq: @tonyandrewmeyer deserves the credit. I've only reviewed the PR 🙇🏻 
addyess: > @tonyandrewmeyer deserves the credit. I've only reviewed the PR 🙇🏻

Sorry, it was late and i was blurry-eyed 👀 .  Thanks so much @tonyandrewmeyer!
addyess: @IronCore864 Thanks so much also for offering a review

Patch:
diff --git a/ops/model.py b/ops/model.py
index 7fa8f30..3bafa88 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -91,16 +91,6 @@ _NetworkDict = TypedDict('_NetworkDict', {
 })
 
 
-# Copied from typeshed.
-_KT = typing.TypeVar("_KT")
-_VT_co = typing.TypeVar("_VT_co", covariant=True)
-
-
-class _SupportsKeysAndGetItem(typing.Protocol[_KT, _VT_co]):
-    def keys(self) -> typing.Iterable[_KT]: ...
-    def __getitem__(self, __key: _KT) -> _VT_co: ...
-
-
 logger = logging.getLogger(__name__)
 
 MAX_LOG_LINE_LEN = 131071  # Max length of strings to pass to subshell.
@@ -1722,7 +1712,7 @@ class RelationDataContent(LazyMapping, MutableMapping[str, str]):
         self._validate_read()
         return super().__getitem__(key)
 
-    def update(self, other: _SupportsKeysAndGetItem[str, str], **kwargs: str):
+    def update(self, other: typing.Any = (), /, **kwargs: str):
         """Update the data from dict/iterable other and the kwargs."""
         super().update(other, **kwargs)
 


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index 1f7c212..2037585 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -232,6 +232,32 @@ class TestModel:
                 relation_id, harness.model.app) == harness.get_relation_data(
                 relation_id, local_app) == {'foo': 'bar'}
 
+    @pytest.mark.parametrize('args,kwargs', [
+        (({'foo': 'baz'}, ), {}),
+        (([('foo', 'baz')], ), {}),
+        ((), {'foo': 'baz'})
+    ])
+    def test_update_app_relation_data(
+        self,
+        args: typing.Tuple[typing.Any, ...],
+        kwargs: typing.Dict[str, str],
+        harness: ops.testing.Harness[ops.CharmBase],
+    ):
+        harness.set_leader(True)
+        harness.begin()
+        relation_id = harness.add_relation('db1', 'remote')
+        harness.add_relation_unit(relation_id, 'remote/0')
+        with harness._event_context('foo_event'):
+            harness.update_relation_data(
+                relation_id,
+                harness.model.app.name,
+                {'foo': 'bar'})
+            rel = harness.model.get_relation('db1', relation_id)
+            assert rel is not None
+            rel.data[harness.model.app].update(*args, **kwargs)
+            assert harness.get_relation_data(
+                relation_id, harness.model.app) == {'foo': 'baz'}
+
     def test_unit_relation_data(self, harness: ops.testing.Harness[ops.CharmBase]):
         relation_id = harness.add_relation('db1', 'remoteapp1')
         harness.add_relation_unit(relation_id, 'remoteapp1/0')