👌 Return `frozendict` for stored `Dict` nodes

[mbercx]

trafficstars

Fixes #5970

[mbercx]

@sphuber another user ran into #5970 and I wanted to try this frozendict solution. Although I agree we have to be very careful with such a change, I think the issue is problematic enough to warrant a discussion.

In general, it seems to me that in case returning a frozendict for stored Dict nodes breaks a piece of code, it should be broken. Either they are trying to change the contents of the Dict node, or trying to obtain a dict they can mutate, but this should be done with the get_dict method.

Here's the example usage:

In [1]: d = Dict({'nested': {'a': 1, 'more_nested': {'b': 2}}})

In [2]: d['nested']['a'] = 3; d.get_dict()
Out[2]: {'nested': {'a': 3, 'more_nested': {'b': 2}}}

In [3]: d.store()
Out[3]: <Dict: uuid: b0719fbb-96aa-4a0a-ad58-82349af0c25b (pk: 44249)>

In [4]: d['nested'] = 5
---------------------------------------------------------------------------
< CONTENT REMOVED>

File ~/project/core/code/aiida-core/aiida/orm/nodes/node.py:226, in Node._check_mutability_attributes(self, keys)
    219 """Check if the entity is mutable and raise an exception if not.
    220
    221 This is called from `NodeAttributes` methods that modify the attributes.
    222
    223 :param keys: the keys that will be mutated, or all if None
    224 """
    225 if self.is_stored:
--> 226     raise exceptions.ModificationNotAllowed('the attributes of a stored entity are immutable')

ModificationNotAllowed: the attributes of a stored entity are immutable

In [5]: d['nested']['a'] = 5
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[5], line 1
----> 1 d['nested']['a'] = 5

TypeError: 'frozendict.frozendict' object does not support item assignment

Note: I haven't checked implications for the other methods implemented on Dict yet (e.g. items). I just wanted to test the usage above and restart this discussion.

[sphuber]

Thanks @mbercx . I think you should at the very least also add these guards to items and dict because currently the user is still vulnerable there. For example:

In [1]: d = Dict({'nested': {'a': 1, 'more_nested': {'b': 2}}}).store()

In [2]: d.dict.nested['a'] = 2

In [3]: d.get_dict()
Out[3]: {'nested': {'a': 1, 'more_nested': {'b': 2}}}

In [4]: for key, value in d.items():
   ...:     value['a'] = 2
   ...: 

In [5]: d.get_dict()
Out[5]: {'nested': {'a': 1, 'more_nested': {'b': 2}}}

Could you also add the dependency to the requirements files so that the tests actually run. Good to see whether these changes already show something in our own tests or not.

[mbercx]

@sphuber sure, will do! Just wanted to make sure my suggestion wasn't shot down immediately for a reason I didn't think of before committing too much time. ^^

[sphuber]

@sphuber sure, will do! Just wanted to make sure my suggestion wasn't shot down immediately for a reason I didn't think of before committing too much time. ^^

Might be a good idea to open a discussion on Discourse to get feedback from others.