Changing nested content of stored `Dict` nodes does not raise

[mbercx]

Describe the bug

Trying to change a top-level key of a stored Dict node raises (as expected):

In [1]: d = Dict({'a': 1})
   ...: d['b'] = 2
   ...: print(d.get_dict())
{'a': 1, 'b': 2}

In [2]: d.store()
   ...: d['c'] = 3
   ...: print(d.get_dict())
---------------------------------------------------------------------------
ModificationNotAllowed                    Traceback (most recent call last)
Cell In[2], line 2
      1 d.store()
----> 2 d['c'] = 3
      3 print(d.get_dict())

File ~/envs/aiida-dev/code/aiida-core/aiida/orm/nodes/data/dict.py:71, in Dict.__setitem__(self, key, value)
     70 def __setitem__(self, key, value):
---> 71     self.base.attributes.set(key, value)

File ~/envs/aiida-dev/code/aiida-core/aiida/orm/nodes/attributes.py:118, in NodeAttributes.set(self, key, value)
    110 def set(self, key: str, value: Any) -> None:
    111     """Set an attribute to the given value.
    112 
    113     :param key: name of the attribute
   (...)
    116     :raise aiida.common.ModificationNotAllowed: if the entity is stored
    117     """
--> 118     self._node._check_mutability_attributes([key])  # pylint: disable=protected-access
    119     self._backend_node.set_attribute(key, value)

File ~/envs/aiida-dev/code/aiida-core/aiida/orm/nodes/node.py:209, in Node._check_mutability_attributes(self, keys)
    202 """Check if the entity is mutable and raise an exception if not.
    203 
    204 This is called from `NodeAttributes` methods that modify the attributes.
    205 
    206 :param keys: the keys that will be mutated, or all if None
    207 """
    208 if self.is_stored:
--> 209     raise exceptions.ModificationNotAllowed('the attributes of a stored entity are immutable')

ModificationNotAllowed: the attributes of a stored entity are immutable

But trying to change nested content of the Dict node fails silently:

In [3]: d = Dict({'a': {'b': None, 'c': None}})
   ...: d['a']['b'] = 2
   ...: print(d.get_dict())
{'a': {'b': 2, 'c': None}}

In [4]: d.store()
   ...: d['a']['c'] = 3
   ...: print(d.get_dict())
{'a': {'b': 2, 'c': None}}

Expected behavior

Trying to change the nested content of a stored Dict node should raise.

Your environment

• Operating system [e.g. Linux]: Ubuntu 18.04.6 LTS
• Python version [e.g. 3.7.1]: Python 3.9.16
• aiida-core version [e.g. 1.2.1]: v2.3.0 (release branch)

Additional context

This came up here:

https://github.com/aiidateam/aiida-quantumespresso/pull/902#issuecomment-1501149498

In the context of setting QE input parameters. A fairly common use case where this would come up is when trying to use the get_restart_builder() and changing one of the inputs:

restart_builder.pw.parameters['CONTROL']['restart_mode'] = 'restart'

@sphuber already gave an extensive answer to why this problem is somewhat tricky to solve. I'll add it below for reference.

---

From @sphuber: I definitely remember writing about this in an issue and basically coming to the conclusion that it will be very difficult to come up with a solution for this. Essentially what is happening with the following code:

restart_builder.pw.parameters['CONTROL']['restart_mode'] = 'restart'

First restart_builder.pw.parameters['CONTROL'] calls _getitem__ with CONTROL as key. As the implementation shows:

    def __getitem__(self, key):
        try:
            return self.base.attributes.get(key)
        except AttributeError as exc:
            raise KeyError from exc

this will get the attribute from the database with that name and return it. The return type is a plain Python dictionary, let's call it d. So next, the code calls d['restart_mode'] = 'restart , which will indeed set the restart_mode key to restart, but that is of the Python dictionary d and not of the node attributes.

The only "solution" I see here is to have Dict.__getitem__ not return a plain dict but some custom class that implements all dict methods, but overrides all setters and delete attributes (and potentially others) and either raises or emits a warning. Now while this may be possible, I see many potential pitfalls that will be difficult to foresee.