canonical / operator

Pure Python framework for writing Juju charms
Apache License 2.0
244 stars 115 forks source link

Add item assignment support for 'RelationData' object #817

Open sed-i opened 1 year ago

sed-i commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

benhoyt commented 2 months ago

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.