Issue 10105

Title
_changed_values do not support fields.Dict modifications
Priority
bug
Status
resolved
Nosy list
ced, nicoe, reviewbot, roundup-bot
Assigned to
nicoe
Keywords
review

Created on 2021-02-15.22:02:23 by nicoe, last changed 1 month ago by roundup-bot.

Messages

New changeset ab87d83fd2e8 by Nicolas Évrard in branch 'default':
Use immutable datastructures for Dict and MultiSelection fields
https://hg.tryton.org/tryton-env/rev/ab87d83fd2e8
New changeset e7526f0686ec by Nicolas Évrard in branch 'default':
Use immutable datastructures for Dict and MultiSelection fields
https://hg.tryton.org/trytond/rev/e7526f0686ec
Author: [hidden] (nicoe) Tryton committer
Date: 2021-02-24.19:04:43
* Cédric Krier  [2021-02-16 00:52 +0100]: 

>frozendict is not standard but I do not really think we should add a
>new dependency. Instead we should do like for the xxx2Many and
>duplicate the dict value set via `__set__` (using a simple copy).
>I think the same should be done for the list value set on MultiSelection.

The latests patch implements this.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-02-16.19:00:04
On 2021-02-16 18:50, Nicolas Évrard wrote:
> 
> Nicolas Évrard <nicoe@b2ck.com> added the comment:
> 
> >> > The original design is that fields values are immutable (ex: int,
> >> > str, tuple).
> >> 
> >> Which is a false assumption as field values can also be a list of
> >> record or a record.
> > 
> > Normally there are no list as value only tuple.
> > If it is not the case in some places, it should be fixed.
> 
> The value of a M2O field is a record which is mutable.
> Anyway this is not related to this issue and should go in an issue of
> its own or on discuss.

But this mutation are tracked so it is not a problem.
Author: [hidden] (nicoe) Tryton committer
Date: 2021-02-16.18:50:45
>> > The original design is that fields values are immutable (ex: int,
>> > str, tuple).
>> 
>> Which is a false assumption as field values can also be a list of
>> record or a record.
> 
> Normally there are no list as value only tuple.
> If it is not the case in some places, it should be fixed.

The value of a M2O field is a record which is mutable.
Anyway this is not related to this issue and should go in an issue of
its own or on discuss.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-02-16.17:14:06
On 2021-02-16 14:20, Nicolas Évrard wrote:
> * Cédric Krier [février 16, 2021 13:03]:
> > 
> > Cédric Krier <cedric.krier@b2ck.com> added the comment:
> > 
> > On 2021-02-16 12:37, Nicolas Évrard wrote:
> >> The idea of the patch is that self._init_values should be
> >> frozen instead of relying on calls to __set__ to modify what's stored
> >> in _values.
> > 
> > But the problem is not only with _init_values.
> 
> I guess you mean the issue is there also with _save_values.
> Indeed it should be fixed with using __set__.

Yes it is there also.

> Maybe the rest of the discussion should be another issue about the
> problem of the implementation details that leak when users have to
> make "self.dict = self.dict" in order to get the right behaviour.

For me it is linked. It is about mutable values for fields.

> > The original design is that fields values are immutable (ex: int,
> > str, tuple).
> 
> Which is a false assumption as field values can also be a list of
> record or a record.

Normally there are no list as value only tuple.
If it is not the case in some places, it should be fixed.

> > But this was broken by the introduction of Dict (and also
> > MultiSelection). For me we must restore this design instead of
> > having adhoc fix.
> 
> It was broken from the beginning as only some field values are
> immutable.

Initially they were all immutable. It is only the new Dict and
MultiSelection that break the design.
Author: [hidden] (nicoe) Tryton committer
Date: 2021-02-16.14:20:34
* Cédric Krier [février 16, 2021 13:03]:
> 
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
> 
> On 2021-02-16 12:37, Nicolas Évrard wrote:
>> The idea of the patch is that self._init_values should be
>> frozen instead of relying on calls to __set__ to modify what's stored
>> in _values.
> 
> But the problem is not only with _init_values.

I guess you mean the issue is there also with _save_values.
Indeed it should be fixed with using __set__.

Maybe the rest of the discussion should be another issue about the
problem of the implementation details that leak when users have to
make "self.dict = self.dict" in order to get the right behaviour.

> The original design is that fields values are immutable (ex: int,
> str, tuple).

Which is a false assumption as field values can also be a list of
record or a record.

> But this was broken by the introduction of Dict (and also
> MultiSelection). For me we must restore this design instead of
> having adhoc fix.

It was broken from the beginning as only some field values are
immutable.

> The question is if we do also a copy when field value is accessed as
> attribute because mutation will not be detected. I think it will be too
> expensive for the few cases that it will solve.

It's not only a performance problem as it leaks the implementation
details as I said above.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-02-16.13:03:03
On 2021-02-16 12:37, Nicolas Évrard wrote:
> The idea of the patch is that self._init_values should be
> frozen instead of relying on calls to __set__ to modify what's stored
> in _values.

But the problem is not only with _init_values. The original design is
that fields values are immutable (ex: int, str, tuple). But this was
broken by the introduction of Dict (and also MultiSelection). For me we
must restore this design instead of having adhoc fix.
So clearly __set__ must make a copy of mutable value because we cannot
know if it will not be mutated later.
The question is if we do also a copy when field value is accessed as
attribute because mutation will not be detected. I think it will be too
expensive for the few cases that it will solve.
Now it may be interesting to see if we can not implement a simple
frozendict and use tuple for MultiSelection. That will not add cost if
we already make a copy in __set__.
Author: [hidden] (nicoe) Tryton committer
Date: 2021-02-16.12:37:02
* Cédric Krier [février 16, 2021 11:33]:
> 
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
> 
> Modify the `_copy` method only solve a specific use case which
> relies on the method being called in `_changed_values` for the
> one2many. But it does not solve the real design issue.

I don't understand.

The copy method is not called in _changed_values but at the __init__
of a Model. The idea of the patch is that self._init_values should be
frozen instead of relying on calls to __set__ to modify what's stored
in _values.
-- 
Nicolas Évrard - B2CK SPRL
E-mail/Jabber: nicolas.evrard@b2ck.com
Tel: +32 472 54 46 59
Website: http://www.b2ck.com/
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-02-16.11:33:29

Modify the _copy method only solve a specific use case which relies on the method being called in _changed_values for the one2many. But it does not solve the real design issue.

Author: [hidden] (nicoe) Tryton committer
Date: 2021-02-16.11:29:51

The patch in fact does a shallow copy of every attribute except when it's a dictionary in this case it's a deep copy (but it shouldn't be a problem since it's not relational values).

And indeed I first though about a frozendict but as you I noticed that the PEP proposing it was rejected.

The advantage of doing it in the _copy() call is that the user don't have to do this ugly reassignment because it's the initial_value that is frozen. It might even be extended to the xxx2Many (but it's indeed a recursive process).

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-02-16.00:52:57

frozendict is not standard but I do not really think we should add a new dependency. Instead we should do like for the xxx2Many and duplicate the dict value set via __set__ (using a simple copy).
I think the same should be done for the list value set on MultiSelection.
Of course there is always the problem of mutable values inside but we already have the issue with xxx2Many for which we have to re-assign the value to detect the change. I think we can live with the requirement that user must not modify mutable values of a Dict or MultiSelection. This worth the performance benefit to not have to make deepcopy of every mutable value.

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2021-02-15.22:21:34

I do not think it is a good solution to make deepcopy everywhere. I think it will be better that Dict field returned a frozendict forcing user to assign a new dict if he wants to modify the value.

Author: [hidden] (nicoe) Tryton committer
Date: 2021-02-15.22:02:23

When modifying a Dict field in an on_change call nothing happens because _changed_values() does not detect a change.

This is due to the fact that the copy creating _initial_values during the instanciation is a shallow copy.

History
Date User Action Args
2021-04-12 19:39:39roundup-botsetmessages: + msg66497
2021-04-12 19:39:36roundup-botsetmessages: + msg66496
nosy: + roundup-bot
status: chatting -> resolved
2021-04-11 18:28:26reviewbotsetmessages: + msg66416
2021-04-09 15:26:58reviewbotsetmessages: + msg66224
2021-03-31 19:46:42reviewbotsetmessages: + msg65958
2021-03-17 18:10:18reviewbotsetmessages: + msg65606
2021-03-17 17:38:46reviewbotsetmessages: + msg65605
2021-03-04 12:16:43reviewbotsetmessages: + msg65092
2021-02-24 19:14:39reviewbotsetmessages: + msg64824
2021-02-24 19:04:43nicoesetmessages: + msg64822

Showing 10 items. Show all history (warning: this could be VERY long)