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

👌 Return frozendict for stored Dict nodes #6185

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mbercx
Copy link
Member

@mbercx mbercx commented Nov 20, 2023

Fixes #5970

@mbercx
Copy link
Member Author

mbercx commented Nov 20, 2023

@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
Copy link
Contributor

sphuber commented Nov 21, 2023

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
Copy link
Member Author

mbercx commented Nov 21, 2023

@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
Copy link
Contributor

sphuber commented Nov 21, 2023

@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.

@sphuber
Copy link
Contributor

sphuber commented Jul 2, 2024

What's the plan for this one @mbercx ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing nested content of stored Dict nodes does not raise
2 participants