Merge lp:~thibaut-dirlik/openobject-server/fix-inherits-bug-303455 into lp:openobject-server

[Thibaut DIRLIK (Logica)]

The _inherits problem is complicated to solve with the current implementation. Because of the multiple-inheritance allowed by _inherits, parents can have fields with the same name that children.

When you create a new objects which uses _inherits, OpenERP will fill ONE of its parent field with the value specified in a field. For example,n if "active" = True, OpenERP will create the parent and set active to True for it. But if the object have a "active" field too, it won't be defined.

This small patch make OpenERP sets the value for the current object first, and, if the current object does not have a column, but the parent does, for its parent.

Their are implementation choices to do :

- What do to in case of multiple parents ? Currently, self._inherit_fields is a "mixin" and indeteminated because of the non-ordered state of the _inherits dict. So, the last read _inherits value is placed in the _inherit_fields attributen in the case multiple parent avec common fields.

- Unless this implementation change, it won't be possible to modify all the parent at once.

I wanted to set the value for all parents + the current object, but it's not possible unless we change the current _inherits implementation. So, I just did that.

[Olivier Dony (Odoo) (odo-openerp)]

Thibaut,

Great job with the merge proposal and the explanation!
So in fact the issue was only with create() and not with write(), as I understood previously.

After looking at the patch and reviewing the ORM code, it seems all good for me. The behavior will now be consistent between all operations: read-write-create: all will get/set the field value at the child level and not the parent(s), if the child defines it. This is correct wrt to the semantics of 'inherits'.
And in case the child does not redefine the field but has multiple parents that have it, the one that will be read/written is the first one according to the "natural order" of the _inherit_fields dict. Not a very smart algorithm, but at least it will be consistent across multiple operations, once the server is started and the columns aren't modified - good enough until we change the _inherits implementation.

BTW I have not had time to actually test this patch with a real use case. As this is the ORM code, it would be great if someone could confirm the fix works for them in a real case? (I'm assigning review to community for that purpose)

Thanks a lot!

review: Approve ((patch review, not tested))

[Thibaut DIRLIK (Logica) (thibaut-dirlik)]

Thanks for your comment! Hope it will help. Moreover, the _inherits implementation should be reviewed, maybe for 6.2 ? It's a very powerful tool, but currently too limited. I think that xrc propose an other implementation, but I don't know where it is.

[Vo Minh Thu (thu)]

When the field is present in multiple parents, it is the right-most in _inherits that will be recorded in _inherit_fields.

I think this is also consistent with python inheritance in the case of multiple parents (should be verified though).

But in the case of multiple level of inheritance, the parent recorded in _inherit_fields is not the lowest in the chain. This seems to mean the fix is enough for a two-level parent-child hierarchy, but not more.

[Thibaut DIRLIK (Logica) (thibaut-dirlik)]

I'm not an expert in Python internal, but I thought that "dict" objects (like _inherits) were not ordered. So, when you say the "right most in _inherits", we can't rely on it.

Thanks for your review,

[Vo Minh Thu (thu)]

Yes they are unordered. I was thinking at the same time to _inherit (which is (possibly) a list).

[Olivier Dony (Odoo) (odo-openerp)]

Finally got around to merging it, thanks again!

[Thibaut DIRLIK (Logica) (thibaut-dirlik)]

Thank you for merging it ;-)

=== modified file 'openerp/osv/orm.py'
--- openerp/osv/orm.py	2011-08-11 18:12:17 +0000
+++ openerp/osv/orm.py	2011-08-25 15:17:17 +0000
@@ -4050,7 +4050,7 @@
         (upd0, upd1, upd2) = ('', '', [])
         upd_todo = []
         for v in vals.keys():
-            if v in self._inherit_fields:
+            if v in self._inherit_fields and v not in self._columns:
                 (table, col, col_detail) = self._inherit_fields[v]
                 tocreate[table][v] = vals[v]
                 del vals[v]