Merge lp:~openerp-dev/openobject-server/trunk-view-dependencies-xmo into lp:openobject-server

[Xavier (Open ERP)]

Fixes conflict between inheriting from a view and depending on one:

* A module A defines a view a
* A module B defines a view b, which depends on (uses, e.g. via a join) a
* A module C inherits from (and alters) a

There is no interdependency between B and C. If C is loaded after B it will cause a new call to a.init(), which will DROP VIEW a CASCADE, which will drop b. The system ends up with view b missing even though the corresponding module (and model) are installed. There are cases of this happening in standard OpenERP modules e.g. the view crm_partner_report_assign joins on account_invoice_report, sale_crm overrides account_invoice_report to add a new field.

If sale_crm is installed after crm_partner_report_assign (which it is on e.g. -all runbots, or if one installs sale and crm_partner_report_assign), the crm_partner_report_assign view disappears and the Partnership Analysis reporting view is broken. An other example is event_sale destroying the Events Analysis.

Rejected fix method:
A consideration was to use Postgres's dependency analysis after view creation to infer view dependencies and correctly handle their reloading, this was deemed to magical and overly complex (getting dependencies between views without false positives is not trivial).

Fix method: explicit declaration of dependencies and a special view-creation method which attempts to explicitly drop and re-init dependents on the fly. The default path will use a non-cascading DROP VIEW and in case of error fall back on a cascading drop view with an error message logged.

[Xavier (Open ERP) (xmo-deactivatedaccount)]

Initial strategy of a method call turns out to not work right (I think) for more than 1 level of dependency, there may be views init-ed multiple times and ordering can be completely broken.

Replaced the whole concept by a specific Model subclass, the developer provides the view's query (as a method or a class-level attribute) and the class can then cleanly handle destructions and recreations in case of reinit.

[Xavier (Open ERP) (xmo-deactivatedaccount)]

See https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-view-dependencies-xmo/+merge/206121 for addons fixes

Unmerged revisions

5071. By Xavier (Open ERP) on 2014-02-12

[FIX] correctly handle drop failure (by forcing)

Drop failure can be caused by unregistered dependent views, or some other type
of dependent. If the case is encountered, log it and fallback on the old
behavior of forcing everything to drop using CASCADE.

Looked into diag object (>9.3) for possibly more informative error message,
but nothing is available outside of the exception's base info messages.

5070. By Xavier (Open ERP) on 2014-02-12

[ADD] View class for dependency handling

5069. By Xavier (Open ERP) on 2014-02-12

[IMP] move _name validation to create_instance

5068. By Xavier (Open ERP) on 2014-02-12

[IMP] minor style improvements

5067. By Xavier (Open ERP) on 2014-02-12

[REF] extract special attributes handling from create_instance

not sure this should be a module-level global thing though

=== modified file 'openerp/modules/registry.py'
--- openerp/modules/registry.py	2014-02-09 00:40:05 +0000
+++ openerp/modules/registry.py	2014-02-12 16:56:55 +0000
@@ -22,7 +22,7 @@
 """ Models registries.
 
 """
-from collections import Mapping
+from collections import Mapping, defaultdict
 from contextlib import contextmanager
 import logging
 import threading
@@ -49,6 +49,7 @@
         self.models = {}    # model name/model instance mapping
         self._sql_error = {}
         self._store_function = {}
+        self._model_dependents = defaultdict(set)
         self._init = True
         self._init_parent = {}
         self._assertion_report = assertion_report.assertion_report()

=== modified file 'openerp/osv/orm.py'
--- openerp/osv/orm.py	2014-02-06 11:04:23 +0000
+++ openerp/osv/orm.py	2014-02-12 16:56:55 +0000
@@ -723,6 +723,9 @@
     # 2. uses column_info instead of a triple.
     _all_columns = {}
 
+    # list of model names on which this one depends
+    _model_dependencies = set()
+
     _table = None
     _invalids = set()
     _log_create = False
@@ -852,88 +855,78 @@
         cr.commit()
 
     #
-    # Goal: try to apply inheritance at the instanciation level and
+    # Goal: try to apply inheritance at the instantiation level and
     #       put objects in the pool var
     #
     @classmethod
     def create_instance(cls, pool, cr):
-        """ Instanciate a given model.
+        """ Instantiate a given model.
 
-        This class method instanciates the class of some model (i.e. a class
+        This class method instantiates the class of some model (i.e. a class
         deriving from osv or osv_memory). The class might be the class passed
         in argument or, if it inherits from another class, a class constructed
         by combining the two classes.
 
-        The ``attributes`` argument specifies which parent class attributes
-        have to be combined.
-
         TODO: the creation of the combined class is repeated at each call of
         this method. This is probably unnecessary.
 
         """
-        attributes = ['_columns', '_defaults', '_inherits', '_constraints',
-            '_sql_constraints']
-
         parent_names = getattr(cls, '_inherit', None)
         if parent_names:
-            if isinstance(parent_names, (str, unicode)):
-                name = cls._name or parent_names
+            name = cls._name
+            if isinstance(parent_names, basestring):
+                name = name or parent_names
                 parent_names = [parent_names]
-            else:
-                name = cls._name
+
             if not name:
                 raise TypeError('_name is mandatory in case of multiple inheritance')
 
-            for parent_name in ((type(parent_names)==list) and parent_names or [parent_names]):
-                if parent_name not in pool:
-                    raise TypeError('The model "%s" specifies an unexisting parent class "%s"\n'
-                        'You may need to add a dependency on the parent class\' module.' % (name, parent_name))
-                parent_model = pool[parent_name]
+            assert isinstance(parent_names, list)
+            for parent_name in parent_names:
+                parent_model = pool.get(parent_name)
+                if parent_model is None:
+                    raise TypeError(
+                        "The model \"%s\" specifies a non-existent parent "
+                        "class \"%s\"\n. You may need to add a dependency on "
+                        "the parent class's module." % (name, parent_name))
+
                 if not getattr(cls, '_original_module', None) and name == parent_model._name:
                     cls._original_module = parent_model._original_module
-                parent_class = parent_model.__class__
+                parent_class = type(parent_model)
                 nattr = {}
-                for s in attributes:
-                    new = copy.copy(getattr(parent_model, s, {}))
-                    if s == '_columns':
-                        # Don't _inherit custom fields.
-                        for c in new.keys():
-                            if new[c].manual:
-                                del new[c]
-                    if hasattr(new, 'update'):
-                        new.update(cls.__dict__.get(s, {}))
-                    elif s=='_constraints':
-                        for c in cls.__dict__.get(s, []):
-                            exist = False
-                            for c2 in range(len(new)):
-                                #For _constraints, we should check field and methods as well
-                                if new[c2][2]==c[2] and (new[c2][0] == c[0] \
-                                        or getattr(new[c2][0],'__name__', True) == \
-                                            getattr(c[0],'__name__', False)):
-                                    # If new class defines a constraint with
-                                    # same function name, we let it override
-                                    # the old one.
-
-                                    new[c2] = c
-                                    exist = True
-                                    break
-                            if not exist:
-                                new.append(c)
-                    else:
-                        new.extend(cls.__dict__.get(s, []))
-                    nattr[s] = new
+                new_data = cls.__dict__
+                for attr in model_attributes:
+                    parent_item = getattr(parent_model, attr.name, attr.empty())
+                    new = attr.copy(parent_item)
+
+                    new_item = new_data.get(attr.name, attr.empty())
+                    attr.update(new, new_item)
+
+                    nattr[attr.name] = new
 
                 # Keep links to non-inherited constraints, e.g. useful when exporting translations
-                nattr['_local_constraints'] = cls.__dict__.get('_constraints', [])
-                nattr['_local_sql_constraints'] = cls.__dict__.get('_sql_constraints', [])
+                nattr['_local_constraints'] = new_data.get('_constraints', [])
+                nattr['_local_sql_constraints'] = new_data.get('_sql_constraints', [])
+                nattr['_register'] = False
 
-                cls = type(name, (cls, parent_class), dict(nattr, _register=False))
+                cls = type(name, (cls, parent_class), nattr)
         else:
             cls._local_constraints = getattr(cls, '_constraints', [])
             cls._local_sql_constraints = getattr(cls, '_sql_constraints', [])
 
         if not getattr(cls, '_original_module', None):
             cls._original_module = cls._module
+
+        if not cls._name:
+            msg = "The class %s must have a _name attribute" % cls.__name__
+            _logger.error(msg)
+            raise except_orm('ValueError', msg)
+
+        # convert cls -> dependencies mapping to dependencies -> self, so a
+        # model can easily know who its dependents are
+        for dependency in getattr(cls, '_model_dependencies', ()):
+            pool._model_dependents[dependency].add(cls._name)
+
         obj = object.__new__(cls)
 
         if hasattr(obj, '_columns'):
@@ -983,13 +976,6 @@
         pool.add(self._name, self)
         self.pool = pool
 
-        if not self._name and not hasattr(self, '_inherit'):
-            name = type(self).__name__.split('.')[0]
-            msg = "The class %s has to have a _name attribute" % name
-
-            _logger.error(msg)
-            raise except_orm('ValueError', msg)
-
         if not self._description:
             self._description = self._name
         if not self._table:
@@ -5093,6 +5079,77 @@
     _register = False # not visible in ORM registry, meant to be python-inherited only
     _transient = False
 
+class View(Model):
+    """ Dedicated :class:`Model` subtype for database views (reporting).
+
+    Automatically generates or drops the view named through ``_table``
+    (automatically inferred from ``_name`` if not specified, as with normal
+    :class:`Model` instances), the view query can be specified either via
+    the :attr:`._query` class attribute or by overriding the :meth:`.query`
+    method (for dynamic/overridable views).
+
+    Other views onto which this one depends should be declared via
+    :attr:`._model_dependencies`, so that views overriding can be handled
+    correc
+
+    :class:`View` provides its own ``init`` method.
+    """
+    _auto = False
+    _register = False
+
+    _query = None
+
+    def query(self, cr):
+        """ Returns the SQL query used to generate a view. By default, returns
+        the value of the _query argument but can be overridden for dynamically
+        generated or overridable view queries
+        """
+        return self._query
+
+    def _get_dependents(self):
+        # Instead of recursive, maybe build flat list of dependents in topo
+        # order?
+        return [self.pool[name] for name in self.pool._model_dependents[self._name]]
+
+    def _drop_view(self, cr, dropped):
+        if self._name in dropped: return
+
+        for model in self._get_dependents():
+            model._drop_view(cr, dropped)
+
+        try:
+            cr.execute('SAVEPOINT before_drop_attempt')
+            cr.execute('DROP VIEW IF EXISTS %s' % self._table)
+        except psycopg2.Error:
+            _logger.exception(
+                "Attempted to drop view '%s' during reload of model '%s', "
+                "trying again with CASCADE, dependents will be silently "
+                "discarded.\n"
+                "\n"
+                "If dependents are views, see orm.View and "
+                "_model_dependencies in order to correctly handle their "
+                "upgrade.",
+                self._table, self._name)
+            cr.execute('ROLLBACK TO SAVEPOINT before_drop_attempt')
+            cr.execute('DROP VIEW %s CASCADE' % self._table)
+        else:
+            cr.execute('RELEASE SAVEPOINT before_drop_attempt')
+        dropped.add(self._name)
+
+    def _create_view(self, cr, created):
+        if self._name in created: return
+
+        created.add(self._name)
+        cr.execute('CREATE VIEW %s AS (%s)' % (self._table, self.query(cr)))
+
+        # avoid double-init? Avoid redrop?
+        for model in self._get_dependents():
+            model._create_view(cr, created)
+
+    def init(self, cr):
+        self._drop_view(cr, set())
+        self._create_view(cr, set())
+
 def itemgetter_tuple(items):
     """ Fixes itemgetter inconsistency (useful in some cases) of not returning
     a tuple if len(items) == 1: always returns an n-tuple where n = len(items)
@@ -5148,4 +5205,36 @@
     # unique constraint error
     '23505': convert_pgerror_23505,
 })
+
+def _merge_constraints(copied, new_constraints):
+    """
+    If new class defines a constraint with the name fields and function
+    (~name) as its ancestors, override existing constraint. Otherwise add
+    new constraint to list
+    """
+    for constraint in new_constraints:
+        for idx in range(len(copied)):
+            if _matching_constraints(copied[idx], constraint):
+                copied[idx] = constraint
+                break
+        else:
+            copied.append(constraint)
+def _matching_constraints(c1, c2):
+    # match fields list, then functions
+    return c1[2] == c2[2] and _matching_functions(c1[0], c2[0])
+def _matching_functions(v1, v2, ph1=object(), ph2=object()):
+    # same function or different function with same name
+    return v1 == v2 or getattr(v1,'__name__', ph1) == getattr(v2,'__name__', ph2)
+
+ModelMerge = collections.namedtuple('ModelMerge', 'name empty copy update')
+model_attributes = [
+    ModelMerge('_columns', dict,
+               lambda it: dict((k, v) for k, v in it.iteritems() if not v.manual),
+               dict.update),
+    ModelMerge('_defaults', dict, copy.copy, dict.update),
+    ModelMerge('_inherits', dict, copy.copy, dict.update),
+    ModelMerge('_constraints', list, copy.copy, _merge_constraints),
+    ModelMerge('_sql_constraints', list, copy.copy, list.extend),
+    ModelMerge('_model_dependencies', set, copy.copy, set.update)
+]
 # vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4:

=== modified file 'openerp/tools/sql.py'
--- openerp/tools/sql.py	2012-02-27 16:57:37 +0000
+++ openerp/tools/sql.py	2014-02-12 16:56:55 +0000
@@ -20,6 +20,10 @@
 ##############################################################################
 
 def drop_view_if_exists(cr, viewname):
+    """
+    .. deprecated:: 8.0
+        use :class:`~openerp.osv.orm.View` instead
+    """
     cr.execute("DROP view IF EXISTS %s CASCADE" % (viewname,))
     cr.commit()