Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add item assignment support for 'RelationData' object #817

Open
sed-i opened this issue Aug 22, 2022 · 4 comments
Open

Add item assignment support for 'RelationData' object #817

sed-i opened this issue Aug 22, 2022 · 4 comments
Labels
24.10 feature New feature or request investigate Needs further investigation - do we want to do this?

Comments

@sed-i
Copy link
Contributor

sed-i commented Aug 22, 2022

It is common to see the following relation data update patterns in charm code:

for relation in self._charm.model.relations[self._relation_name]:
	relation.data[self._charm.app]["first"] = "foo"
	relation.data[self._charm.app].update({"second": "bar"})

However, attempting to assign relation data in a "single source of truth" approach like so

app_data = {"first": "foo", "second": "bar"}
for relation in self._charm.model.relations[self._relation_name]:
	relation.data[self._charm.app] = app_data

errors out with TypeError: 'RelationData' object does not support item assignment, and indeed __set_item__ isn't implemented.

It could be handy if it was possible to overwrite the entire thing, instead of relying on update + manual tidying up of leftovers.

That being said, NOT supporting item assignments may have the advantage of preventing unintentional overwrites in some cases.

@PietroPasotti
Copy link
Contributor

An issue I can see with that is that there'd be an asymmetry between __getitem__(item:str) -> RelationDataContents and __setitem__(str, Dict[str,str]). One might then reasonably expect to get a dict instead of RelationDataContents.

So, if we go for this, I'd be in favour of adding this functionality as RelationDataContents.replace().

Related: I often see various implementations of wipe_all_data(). Which we could implement as __delitem__, or, better, as a RelationDataContents.wipe() (or clear, erase, etc...) method.

@PietroPasotti
Copy link
Contributor

@niemeyer would you be on board with adding the following two methods?

RelationDataContents.clear()  # deletes all key/value pairs
RelationDataContents.replace(items: Dict[str,str])  # equivalent to clear(); update(items)

@rwcarlsen
Copy link
Contributor

An issue with using __setitem__ is ambiguity w.r.t. existing keys - are they erased? Are they merged/added to the new ones? I'm not convinced this is worth saving the one for k, v in app_data.items(): line

@PietroPasotti PietroPasotti added feature New feature or request needs design needs more thought or spec labels Oct 13, 2022
@benhoyt benhoyt added 24.04 investigate Needs further investigation - do we want to do this? and removed needs design needs more thought or spec labels Sep 29, 2023
@benhoyt benhoyt added low priority Non-urgent issue we may (or may not) consider later and removed 24.04 labels Mar 18, 2024
@benhoyt
Copy link
Collaborator

benhoyt commented May 17, 2024

Per Madrid discussion, this seems reasonable (to add the __setitem__ assignment ability). The raw dict would be validated and converted to a proper RelationDataContent object. Let's do a proof of concept at some point and discuss further.

@benhoyt benhoyt added 24.10 and removed low priority Non-urgent issue we may (or may not) consider later labels May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
24.10 feature New feature or request investigate Needs further investigation - do we want to do this?
Projects
None yet
Development

No branches or pull requests

4 participants