aiida-core
aiida-core copied to clipboard
👌 Return `frozendict` for stored `Dict` nodes
Fixes #5970
@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.
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.
@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 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.