feat: efficient implementation of RelationDataContent.update #1586
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benhoyt
reviewed
Feb 24, 2025
james-garner-canonical
force-pushed
the
2025-02/feat/relationdata-assignment-v1
branch
2 times, most recently
from
f50f18e to
0d0df7d
Compare
IronCore864
approved these changes
Feb 26, 2025
|
I've broken the tests. I'm going to iterate on the efficient approach to unify the single/multiple update code paths as much as possible, and then I'll update the test side of things (may need some little modifications to harness/scenario). |
james-garner-canonical
force-pushed
the
2025-02/feat/relationdata-assignment-v1
branch
from
487fec5 to
f1f1776
Compare
There was a problem hiding this comment.
I think this is definitely the right approach. It's looking really great!
On testing:
- You should definitely add a testing/tests test, probably in test_relations. Basically the same kind of thing as you have for harness/test_testing, but for Scenario. I'd also include one that checks that removing a key works (which I think will fail currently!).
- I don't think there's a test for the "I gave setitem an empty dict" case raising an error. It would be good to have one if it's not too complicated.
- I don't think there's any relation-set checking in test_main at the moment, but it feels like this is maybe a good opportunity to add some, which could include this. Basically just verifying that relation-get and relation-set are called properly, including in this new flow. At the moment there's quite an assumption that it works ok since the tests (like much of test_model!) rely on Harness.
james-garner-canonical
force-pushed
the
2025-02/feat/relationdata-assignment-v1
branch
2 times, most recently
from
676f25a to
ed71134
Compare
…l sites TODO: update tests and call sites so that update_relation_data can have a sane signature
_ModelBackend.relation_set in ops now handles a dictionary of key-value pairs instead of a single key and value
This is an efficient implementation of update that replaces the original, inefficient implementation inherited from MutableMapping.
james-garner-canonical
force-pushed
the
2025-02/feat/relationdata-assignment-v1
branch
from
ed71134 to
8854be8
Compare
|
The docs fail because it looks like your code is a "block quote". You can fix it like: diff --git a/ops/model.py b/ops/model.py
index 61ed63d..2e564af 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -1947,11 +1947,12 @@ class RelationDataContent(LazyMapping, MutableMapping[str, str]):
) -> None:
"""Efficiently write multiple keys and values to the databag.
- Has the same ultimate result as this, but uses a single relation-set call:
- for k, v in dict(data).items():
- self[k] = v
- for k, v in kwargs.items():
- self[k] = v
+ Has the same ultimate result as this, but uses a single relation-set call::
+
+ for k, v in dict(data).items():
+ self[k] = v
+ for k, v in kwargs.items():
+ self[k] = v
"""
data = dict(data)
data.update(kwargs)Although I think maybe it's only the extra The PR title needs updating, as you did with the description. |
|
We probably need a comment on #817 that explains why |
james-garner-canonical
changed the title
| @@ -513,7 +531,6 @@ def test_relation_data_del_missing_key(self, harness: ops.testing.Harness[ops.Ch | |||
| ('relation_ids', 'db1'), | |||
| ('relation_list', relation_id), | |||
| ('relation_get', relation_id, 'myapp/0', False), | |||
| ('update_relation_data', relation_id, harness.model.unit, 'port', ''), | |||
There was a problem hiding this comment.
Deleting an already missing key no longer hits the backend
tonyandrewmeyer
approved these changes
Mar 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes it efficient to add/update/delete multiple keys and values in your relation data, by overriding the default implementation of
RelationDataContent.update(provided byMutableMapping) with an efficient implementation that calls therelation-sethook tool only once. The changes to internals that back the newupdateimplementation are exercised by existing tests interacting withRelationDataContent.updateandRelationDataContent.__setitem__.Both Harness and Scenario's model backend implementations require updates to accommodate this. The Harness backend is exercised in the existing Ops tests. The Scenario updates are exercised in new tests added to
testing/tests/test_e2e/test_relations.py.Original description, now outdated, but retained to explain the original motivation for this work:
This PR resolves #817 by adding
RelationData.__setitem__(self, key, value). This allows users to writemy_relation[app_or_unit] = {'key-value': 'pairs'}, instead of having to fetch theRelationDataContentobject withmy_relation[app_or_unit]and then update its keys and values directly.The implementation of
RelationData.__setitem__gets the currentRelationDataContentfor thekey, callsRelationDataContent.clear()(provided byRelationDataContent's inheritance fromMutableMapping-- the inherited implementation will callRelationDataContent's custom__delitem__), and then calls the existingRelationDataContent.update(value)method. This ensures that validation and updating the backend occurs exactly as it would have if the user interacted withRelationDataContentdirectly.RelationData.__setitem__will raise aKeyErrorif thekeyis not actually a knownApplicationorUnit.