Merge lp:~therp-nl/openupgrade-server/7.0-lp1131653_check_recursion_fix_false_positives into lp:openupgrade-server

[Stefan Rijnhart (Opener)]

The bug that this branch aims to fix was discovered when testing the migration scripts for the base module.

[Holger Brunn (Therp) (hbrunn)]

You can do that a lot simpler and save fetches by letting the server build a common table expression that checks for cycles:

create table rec_test (id int, parent_id int) -- that's just for the example

with recursive check_cycle(id, parent_id, is_cycle) as (select id, parent_id, False from rec_test union all select check_cycle.id, rec_test.parent_id, check_cycle.id = rec_test.parent_id from check_cycle join rec_test on check_cycle.parent_id=rec_test.id and rec_test.parent_id is not null and not is_cycle) select * from check_cycle -- that's the real thing

Filter for the ids to check in the non-recursive part, then select rows with is_cycle=True and you're done with just this statement.

review: Needs Fixing

[Holger Brunn (Therp) (hbrunn)]

PS: This fix should also go to lp:ocb-server, right?

[Stefan Rijnhart (Opener) (stefan-opener)]

Amazing, thanks! I had already set this branch to 'work in progress' because my fix still had the original flaw under different circumstances. I'd like to work this out but I will not have the time now. Maybe if the overhead of the workaround (https://code.launchpad.net/~therp-nl/openupgrade-server/7.0-lp1131653_workaround/+merge/150071) starts to bother me...

As for lp:ocb-server, I cannot say that it is affected by this issue as I only ever encountered it in the OpenUpgrade process. Note that the bug was never reported before while the code has been around for some time.

[Mohammad Alhashash (alhashash)]

Please check my proposed fix using CTE at https://code.launchpad.net/~alhashash/openobject-server/v7_fix_check_recursion_bug_1131653/+merge/169942

review: Needs Fixing

Unmerged revisions

4605. By Stefan Rijnhart (Opener) on 2013-02-22

[FIX] _check_recursion gives false positive if ids include a parent/child pair

=== modified file 'openerp/osv/orm.py'
--- openerp/osv/orm.py	2013-02-19 21:32:25 +0000
+++ openerp/osv/orm.py	2013-02-22 14:20:30 +0000
@@ -1943,7 +1943,10 @@
                 msg = "\n * ".join([r[0] for r in res])
                 msg += "\n\nEither you wrongly customized this view, or some modules bringing those views are not compatible with your current data model"
                 _logger.error(msg)
-                raise except_orm('View error', msg)
+                # OpenUpgrade: do not raise for unknown fields, as these
+                # can be triggered on old views that still have to be
+                # replaced by the upgrade procedure
+                # raise except_orm('View error', msg)
         return arch, fields
 
     def _get_default_form_view(self, cr, user, context=None):
@@ -5084,18 +5087,24 @@
 
         if not parent:
             parent = self._parent_name
-        ids_parent = ids[:]
-        query = 'SELECT distinct "%s" FROM "%s" WHERE id IN %%s' % (parent, self._table)
+        # OpenUpgrade: include a fix for https://bugs.launchpad.net/bugs/1131653
+        # as this bug was discovered writing default values for new fields
+        query = 'SELECT id, "%s" FROM "%s" WHERE id IN %%s' % (parent, self._table)
+        ids_parent = ids
+        hierarchy = []
         while ids_parent:
             ids_parent2 = []
-            for i in range(0, len(ids), cr.IN_MAX):
+            for i in range(0, len(ids_parent), cr.IN_MAX):
                 sub_ids_parent = ids_parent[i:i+cr.IN_MAX]
                 cr.execute(query, (tuple(sub_ids_parent),))
-                ids_parent2.extend(filter(None, map(lambda x: x[0], cr.fetchall())))
+                for row in cr.fetchall():
+                    if row[1]:
+                        if row in hierarchy:
+                            return False
+                        hierarchy.append(row)
+                        if row[1] not in ids_parent2:
+                            ids_parent2.append(row[1])
             ids_parent = ids_parent2
-            for i in ids_parent:
-                if i in ids:
-                    return False
         return True
 
     def _get_external_ids(self, cr, uid, ids, *args, **kwargs):