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

[Stefan Rijnhart (Opener)]

Apparently, OpenERP 7.0 attempts to remove dropped columns and obsolete model's tables, triggered when an existing ir_model_data entry of the models 'ir.model' and 'ir.model.field' is not instanciated after a succesful module upgrade. That could be undesirable under certain circumstances, leading to data loss. Also, when whole models are missing there is the problem that the table cannot be retrieved anymore because this data is not in the database but only available from the instanciated model itself. This causes a lot of ugly errors in the log file. This branch logs the attempts to drop columns or tables but does not perform them.

Additionally, the deferred migration step is moved to a separate file and its call is optimized not to run at every pool initialization but only when at least some modules are being upgraded.

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

[Sylvain LE GAL (GRAP) (sylvain-legal)]

Hi.

1/ About the _drop_column & _drop_table calls. I agree with the concept but I will not be able to reproduce the call to these functions after a migration. Can you detail me when the problems occurs ? (an example of table or field that OpenERP try to delete).

2/ deferred migration move & conditional call : agreed.

Remark :
diff, line 20. "Not dropping the table of model ..." --> "Not dropping the table or view of model ..."

review: Needs Information

[lp:~therp-nl/openupgrade-server/7.0-deferred_steps]

4623. By Stefan Rijnhart (Opener) on 2013-09-13

[MRG] Merged with target branch to resolve conflict

4624. By Stefan Rijnhart (Opener) on 2013-09-13

[IMP] Debug string, courtesy of Sylvain Le Gal

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

Thanks for the suggestion, I just committed it.

About the dropping of columns and tables, this is triggered in the unlink method of ir.model.fields and ir.model respectively. I checked the logs from when I crafted this branch and all problematic models were related to deprecated modules that were not going to be reinstalled on 7.0 (such as our module to process a tax increase which occurred last year).

[Sylvain LE GAL (GRAP) (sylvain-legal)]

Ok.
If I unsterstood, I can not test this part of code if I don't have specific module ?

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

Yes, although there were several of such modules in the database that we migrated (mostly wizard or reporting modules) so I expect this will be an issue in many migrations.

You can reproduce by installing this module in the 6.1 database, then upgrade the database without the module in your addons-path (the module is not available for 7.0): https://apps.openerp.com/apps/6.1/trp_update_tax/

=== modified file 'openerp/addons/base/ir/ir_model.py'
--- openerp/addons/base/ir/ir_model.py	2013-05-07 07:08:52 +0000
+++ openerp/addons/base/ir/ir_model.py	2013-09-13 19:42:27 +0000
@@ -35,7 +35,7 @@
 from openerp.tools.translate import _
 from openerp.osv.orm import except_orm, browse_record
 
-from openerp.openupgrade import openupgrade_log
+from openerp.openupgrade import openupgrade_log, openupgrade
 
 _logger = logging.getLogger(__name__)
 
@@ -147,6 +147,12 @@
 
     def _drop_table(self, cr, uid, ids, context=None):
         for model in self.browse(cr, uid, ids, context):
+            # OpenUpgrade: do not run the new table cleanup
+            openupgrade.message(
+                cr, 'Unknown', False, False,
+                "Not dropping the table or view of model %s", model.model)
+            continue
+
             model_pool = self.pool.get(model.model)
             cr.execute('select relkind from pg_class where relname=%s', (model_pool._table,))
             result = cr.fetchone()
@@ -302,6 +308,12 @@
 
     def _drop_column(self, cr, uid, ids, context=None):
         for field in self.browse(cr, uid, ids, context):
+            # OpenUpgrade: do not run the new column cleanup
+            openupgrade.message(
+                cr, 'Unknown', False, False,
+                "Not dropping the column of field %s of model %s", field.name, field.model)
+            continue
+
             model = self.pool.get(field.model)
             cr.execute('select relkind from pg_class where relname=%s', (model._table,))
             result = cr.fetchone()

=== modified file 'openerp/modules/loading.py'
--- openerp/modules/loading.py	2013-07-23 20:00:30 +0000
+++ openerp/modules/loading.py	2013-09-13 19:42:27 +0000
@@ -44,7 +44,7 @@
 from openerp.modules.module import initialize_sys_path, \
     load_openerp_module, init_module_models, adapt_version
 
-from openerp.openupgrade import openupgrade_loading
+from openerp.openupgrade import openupgrade_loading, deferred_70
 _logger = logging.getLogger(__name__)
 
 def open_openerp_namespace():
@@ -419,6 +419,10 @@
             # Cleanup orphan records
             pool.get('ir.model.data')._process_end(cr, SUPERUSER_ID, processed_modules)
 
+            # OpenUpgrade: call deferred migration steps
+            if update_module:
+                deferred_70.migrate_deferred(cr, pool)
+
         for kind in ('init', 'demo', 'update'):
             tools.config[kind] = {}
 
@@ -444,10 +448,6 @@
                 else:
                     _logger.info('removed %d unused menus', cr.rowcount)
 
-        # STEP 5 1/2 (OpenUpgrade): deferred call to sync commercial partner fields
-        if update_module:
-            openupgrade_loading.sync_commercial_fields(cr, pool)
-
         # STEP 6: Uninstall modules to remove
         if update_module:
             # Remove records referenced from ir_model_data for modules to be

=== added file 'openerp/openupgrade/deferred_70.py'
--- openerp/openupgrade/deferred_70.py	1970-01-01 00:00:00 +0000
+++ openerp/openupgrade/deferred_70.py	2013-09-13 19:42:27 +0000
@@ -0,0 +1,55 @@
+# -*- coding: utf-8 -*-
+##############################################################################
+#
+#    OpenERP, Open Source Management Solution
+#    This module copyright (C) 2013 Therp BV (<http://therp.nl>)
+#
+#    This program is free software: you can redistribute it and/or modify
+#    it under the terms of the GNU Affero General Public License as
+#    published by the Free Software Foundation, either version 3 of the
+#    License, or (at your option) any later version.
+#
+#    This program is distributed in the hope that it will be useful,
+#    but WITHOUT ANY WARRANTY; without even the implied warranty of
+#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#    GNU Affero General Public License for more details.
+#
+#    You should have received a copy of the GNU Affero General Public License
+#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+##############################################################################
+
+"""
+This module contains a set of functions that should be called at the end of the
+migration. A migration may be run several times after corrections in the code
+or the configuration, and there is no way for OpenERP to detect a succesful
+result. Therefore, the functions in this module should be robust against
+being run multiple times on the same database.
+"""
+
+import logging
+from openerp import SUPERUSER_ID
+
+logger = logging.getLogger("OpenUpgrade")
+
+def sync_commercial_fields(cr, pool):
+    """
+    Take care of propagating the commercial fields
+    in the new partner model.
+    """
+    partner_obj = pool.get('res.partner')
+    partner_ids = partner_obj.search(
+        cr, SUPERUSER_ID,
+        [], 0, False, False, {'active_test': False})
+    logger.info("Syncing commercial fields between %s partners",
+                len(partner_ids))
+    for partner_id in partner_ids:
+        vals = partner_obj.read(
+            cr, SUPERUSER_ID, partner_id, [], load='_classic_write')
+        partner_obj._fields_sync(
+            cr, SUPERUSER_ID, 
+            partner_obj.browse(cr, SUPERUSER_ID, partner_id),
+            vals)                     
+
+def migrate_deferred(cr, pool):
+    sync_commercial_fields(cr, pool)

=== modified file 'openerp/openupgrade/openupgrade_loading.py'
--- openerp/openupgrade/openupgrade_loading.py	2013-08-02 12:43:11 +0000
+++ openerp/openupgrade/openupgrade_loading.py	2013-09-13 19:42:27 +0000
@@ -19,9 +19,8 @@
 #
 ##############################################################################
 
-import logging
 import types
-from openerp import release, SUPERUSER_ID
+from openerp import release
 from openerp.osv.orm import TransientModel
 from openerp.osv import fields
 from openerp.openupgrade.openupgrade import table_exists
@@ -30,8 +29,6 @@
 # A collection of functions used in 
 # openerp/modules/loading.py
 
-logger = logging.getLogger("OpenUpgrade")
-
 def add_module_dependencies(cr, module_list):
     """
     Select (new) dependencies from the modules in the list
@@ -186,23 +183,3 @@
                             (key, value, record_id)
                             )
                     old_field[key] = value
-
-def sync_commercial_fields(cr, pool):
-    """
-    Take care of propagating the commercial fields
-    in the new partner model. To be called after the
-    upgrade process has finished.
-    """
-    partner_obj = pool.get('res.partner')
-    partner_ids = partner_obj.search(
-        cr, SUPERUSER_ID,
-        [], 0, False, False, {'active_test': False})
-    logger.info("Syncing commercial fields between %s partners",
-                len(partner_ids))
-    for partner_id in partner_ids:
-        vals = partner_obj.read(
-            cr, SUPERUSER_ID, partner_id, [], load='_classic_write')
-        partner_obj._fields_sync(
-            cr, SUPERUSER_ID, 
-            partner_obj.browse(cr, SUPERUSER_ID, partner_id),
-            vals)