[FIX] models, fields: reorganize model setup to retrieve all inherited fields

The model setup sometimes misses entries in _inherit_fields and _all_columns.
This is because those dictionaries are computed from parent models which are
not guaranteed to be completely set up: sometimes a parent field is only
partially set up, and columns are missing (they are generated from fields after
their setup).

To avoid this bug, the setup has been split in three phases:
(1) determine all inherited and custom fields on models;
(2) setup fields, except for recomputation triggers, and generate columns;
(3) add recomputation triggers and complete the setup of the model.

Making these three phases explicit brings good invariants:
- when setting up a field, all models know all their fields;
- when adding recomputation triggers, you know that fields have been set up.

openerp/fields.py

@@ -403,33 +403,40 @@ class Field(object):
         self.inverse_fields = []
 
     def setup(self, env):
-        """ Complete the setup of `self` (dependencies, recomputation triggers,
+        """ Make sure that `self` is set up, except for recomputation triggers. """
-            and other properties). This method is idempotent: it has no effect
-            if `self` has already been set up.
-        """
         if not self.setup_done:
-            self._setup(env)
+            if self.related:
+                self._setup_related(env)
+            else:
+                self._setup_regular(env)
             self.setup_done = True
 
-    def _setup(self, env):
+    #
-        """ Do the actual setup of `self`. """
+    # Setup of non-related fields
-        if self.related:
+    #
-            self._setup_related(env)
+
+    def _setup_regular(self, env):
+        """ Setup the attributes of a non-related field. """
+        recs = env[self.model_name]
+
+        def make_depends(deps):
+            return tuple(deps(recs) if callable(deps) else deps)
+
+        # convert compute into a callable and determine depends
+        if isinstance(self.compute, basestring):
+            # if the compute method has been overridden, concatenate all their _depends
+            self.depends = ()
+            for method in resolve_all_mro(type(recs), self.compute, reverse=True):
+                self.depends += make_depends(getattr(method, '_depends', ()))
+            self.compute = getattr(type(recs), self.compute)
         else:
-            self._setup_regular(env)
+            self.depends = make_depends(getattr(self.compute, '_depends', ()))
 
-        # put invalidation/recomputation triggers on field dependencies
+        # convert inverse and search into callables
-        model = env[self.model_name]
+        if isinstance(self.inverse, basestring):
-        for path in self.depends:
+            self.inverse = getattr(type(recs), self.inverse)
-            self._setup_dependency([], model, path.split('.'))
+        if isinstance(self.search, basestring):
-
+            self.search = getattr(type(recs), self.search)
-        # put invalidation triggers on model dependencies
-        for dep_model_name, field_names in model._depends.iteritems():
-            dep_model = env[dep_model_name]
-            dep_model._setup_fields()
-            for field_name in field_names:
-                field = dep_model._fields[field_name]
-                field._triggers.add((self, None))
 
     #
     # Setup of related fields
@@ -445,7 +452,6 @@ class Field(object):
         recs = env[self.model_name]
         fields = []
         for name in self.related:
-            recs._setup_fields()
             field = recs._fields[name]
             field.setup(env)
             recs = recs[name]
@@ -524,31 +530,14 @@ class Field(object):
         return self.related_field if self.inherited else self
 
     #
-    # Setup of non-related fields
+    # Setup of field triggers
     #
 
-    def _setup_regular(self, env):
+    def setup_triggers(self, env):
-        """ Setup the attributes of a non-related field. """
+        """ Add the necessary triggers to invalidate/recompute `self`. """
-        recs = env[self.model_name]
+        model = env[self.model_name]
-
+        for path in self.depends:
-        def make_depends(deps):
+            self._setup_dependency([], model, path.split('.'))
-            return tuple(deps(recs) if callable(deps) else deps)
-
-        # convert compute into a callable and determine depends
-        if isinstance(self.compute, basestring):
-            # if the compute method has been overridden, concatenate all their _depends
-            self.depends = ()
-            for method in resolve_all_mro(type(recs), self.compute, reverse=True):
-                self.depends += make_depends(getattr(method, '_depends', ()))
-            self.compute = getattr(type(recs), self.compute)
-        else:
-            self.depends = make_depends(getattr(self.compute, '_depends', ()))
-
-        # convert inverse and search into callables
-        if isinstance(self.inverse, basestring):
-            self.inverse = getattr(type(recs), self.inverse)
-        if isinstance(self.search, basestring):
-            self.search = getattr(type(recs), self.search)
 
     def _setup_dependency(self, path0, model, path1):
         """ Make `self` depend on `model`; `path0 + path1` is a dependency of
@@ -558,7 +547,6 @@ class Field(object):
         env = model.env
         head, tail = path1[0], path1[1:]
 
-        model._setup_fields()
         if head == '*':
             # special case: add triggers on all fields of model (except self)
             fields = set(model._fields.itervalues()) - set([self])
@@ -571,8 +559,6 @@ class Field(object):
                 self.recursive = True
                 continue
 
-            field.setup(env)
-
             #_logger.debug("Add trigger on %s to recompute %s", field, self)
             field._triggers.add((self, '.'.join(path0 or ['id'])))
 
@@ -1050,8 +1036,8 @@ class Char(_String):
     type = 'char'
     size = None
 
-    def _setup(self, env):
+    def _setup_regular(self, env):
-        super(Char, self)._setup(env)
+        super(Char, self)._setup_regular(env)
         assert isinstance(self.size, (NoneType, int)), \
             "Char field %s with non-integer size %r" % (self, self.size)
 
@@ -1253,8 +1239,8 @@ class Selection(Field):
             selection = api.expected(api.model, selection)
         super(Selection, self).__init__(selection=selection, string=string, **kwargs)
 
-    def _setup(self, env):
+    def _setup_regular(self, env):
-        super(Selection, self)._setup(env)
+        super(Selection, self)._setup_regular(env)
         assert self.selection is not None, "Field %s without selection" % self
 
     def _setup_related(self, env):
@@ -1341,8 +1327,8 @@ class Reference(Selection):
     def __init__(self, selection=None, string=None, **kwargs):
         super(Reference, self).__init__(selection=selection, string=string, **kwargs)
 
-    def _setup(self, env):
+    def _setup_regular(self, env):
-        super(Reference, self)._setup(env)
+        super(Reference, self)._setup_regular(env)
         assert isinstance(self.size, (NoneType, int)), \
             "Reference field %s with non-integer size %r" % (self, self.size)
 
@@ -1378,8 +1364,8 @@ class _Relational(Field):
     domain = None                       # domain for searching values
     context = None                      # context for searching values
 
-    def _setup(self, env):
+    def _setup_regular(self, env):
-        super(_Relational, self)._setup(env)
+        super(_Relational, self)._setup_regular(env)
         if self.comodel_name not in env.registry:
             _logger.warning("Field %s with unknown comodel_name %r"
                             % (self, self.comodel_name))
@@ -1664,7 +1650,6 @@ class One2many(_RelationalMulti):
         if self.inverse_name:
             # link self to its inverse field and vice-versa
             comodel = env[self.comodel_name]
-            comodel._setup_fields()
             invf = comodel._fields[self.inverse_name]
             # In some rare cases, a `One2many` field can link to `Int` field
             # (res_model/res_id pattern). Only inverse the field if this is

openerp/models.py

@@ -693,7 +693,7 @@ class BaseModel(object):
                     pool._store_function[model].sort(key=lambda x: x[4])
 
     @classmethod
-    def _init_manual_fields(cls, cr, partial=False):
+    def _init_manual_fields(cls, cr, partial):
         # Check whether the query is already done
         if cls.pool.fields_by_model is not None:
             manual_fields = cls.pool.fields_by_model.get(cls._name, [])
@@ -2932,8 +2932,8 @@ class BaseModel(object):
                 cls._inherits[field.comodel_name] = field.name
 
     @api.model
-    def _prepare_setup_fields(self):
+    def _prepare_setup(self):
-        """ Prepare the setup of fields once the models have been loaded. """
+        """ Prepare the setup of the model and its fields. """
         type(self)._setup_done = False
         for name, field in self._fields.items():
             if field.inherited:
@@ -2942,36 +2942,36 @@ class BaseModel(object):
                 field.reset()
 
     @api.model
-    def _setup_fields(self, partial=False):
+    def _setup_base(self, partial):
-        """ Setup the fields (dependency triggers, etc).
+        """ Determine the inherited and custom fields of the model. """
-
-            :param partial: ``True`` if all models have not been loaded yet.
-        """
         cls = type(self)
         if cls._setup_done:
             return
-        cls._setup_done = True
 
-        # first make sure that parent models are all set up
+        # first make sure that parent models determine all their fields
-        for parent in self._inherits:
+        cls._inherits_check()
-            self.env[parent]._setup_fields()
+        for parent in cls._inherits:
+            self.env[parent]._setup_base(partial)
 
         # retrieve custom fields
-        cls._init_manual_fields(self._cr, partial=partial)
+        cls._init_manual_fields(self._cr, partial)
 
         # retrieve inherited fields
-        cls._inherits_check()
         cls._init_inherited_fields()
 
+        cls._setup_done = True
+
+    @api.model
+    def _setup_fields(self):
+        """ Setup the fields, except for recomputation triggers. """
+        cls = type(self)
+
         # set up fields, and update their corresponding columns
         for name, field in cls._fields.iteritems():
             field.setup(self.env)
             if field.store or field.column:
                 cls._columns[name] = field.to_column()
 
-        # determine old-api cls._inherit_fields and cls._all_columns
-        cls._inherits_reload()
-
         # group fields by compute to determine field.computed_fields
         fields_by_compute = defaultdict(list)
         for field in cls._fields.itervalues():
@@ -2981,6 +2981,27 @@ class BaseModel(object):
             else:
                 field.computed_fields = []
 
+    @api.model
+    def _setup_complete(self):
+        """ Setup recomputation triggers, and complete the model setup. """
+        cls = type(self)
+
+        # set up field triggers
+        for field in cls._fields.itervalues():
+            field.setup_triggers(self.env)
+
+        # add invalidation triggers on model dependencies
+        if cls._depends:
+            triggers = [(field, None) for field in cls._fields.itervalues()]
+            for model_name, field_names in cls._depends.iteritems():
+                model = self.env[model_name]
+                for field_name in field_names:
+                    field = model._fields[field_name]
+                    field._triggers.update(triggers)
+
+        # determine old-api cls._inherit_fields and cls._all_columns
+        cls._inherits_reload()
+
         # check constraints
         for func in cls._constraint_methods:
             if not all(name in cls._fields for name in func._constrains):

openerp/modules/registry.py

@@ -166,12 +166,18 @@ class Registry(Mapping):
 
         # prepare the setup on all models
         for model in self.models.itervalues():
-            model._prepare_setup_fields(cr, SUPERUSER_ID)
+            model._prepare_setup(cr, SUPERUSER_ID)
 
         # do the actual setup from a clean state
         self._m2m = {}
         for model in self.models.itervalues():
-            model._setup_fields(cr, SUPERUSER_ID, partial=partial)
+            model._setup_base(cr, SUPERUSER_ID, partial)
+
+        for model in self.models.itervalues():
+            model._setup_fields(cr, SUPERUSER_ID)
+
+        for model in self.models.itervalues():
+            model._setup_complete(cr, SUPERUSER_ID)
 
     def clear_caches(self):
         """ Clear the caches