Merge lp:~openerp-community/openobject-server/stefan-therp_lp727727-6.1 into lp:openobject-server

[Stefan Rijnhart (Opener)]

This branch issues a warning when a value is passed to the ORM for a non existing field during create() or write(). These values are simply dropped, which can lead to data loss. Additionaly, this branch corrects non existing fields in the base module's XML files.

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

Hi Stefan,

I agree that silently ignoring unknown fields in create() and write() can be a source of errors, especially while creating new module and debugging, so it would be a good practice to enforce that.

However, I can also imagine many cases where this situation is abused at the moment, because it makes the code simpler (e.g. when working with "virtual" columns), or just because it was overlooked. Indeed it would be better to fix 100% of these cases (and raising an exception would help that), but that would also mean a possibly great number of fixes will be needed in many modules, both community and core. While useful, those fixes are mostly nice-to-have, and are not likely to fix real bugs in existing modules.
So in the current case, I think logging a WARNING message would be better as a first step. That would still help for developing/debugging, and allow a progressive cleanup of existing abuse cases without breaking too much stuff. Maybe you could even use warnings.DeprecationWarning to indicate that in a future version this may indeed be completely forbidden.

Some other minor remarks:
- for consistency, read()/_read_flat() should do the same w.r.t. to unknown fields
- in error messages, why not mention directly all unknown fields, rather than just the first one? Log messages must not be translated so using different messages will not be a concern anymore, if it's easier.

The cleanup of XML files is very welcome, too!

Many thanks for the clean merge proposal, as always.

PS: I'll change the merge prop status to "Work in Progress", do not forget to change it back when you request another review - this is to help us sort out the numerous merge props, as review comments do not directly allow this.

review: Needs Fixing

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

Hi Olivier,

thanks again for the valuable review.

On your suggestion, I have replaced the exception with a warning. I used a logging.warn instead of a warnings.DeprecationWarning as these cannot easily be redirected to the application log unless one requires Python 2.7 (with logging.captureWarnings()).

Also, it does indeed make sense to mention all unknown fields at once.

However, I cannot think of a situation that warrants the overhead of performing a similar check in every read operation. Usually, the absense of fields will become immediately clear when trying to read their values from the resulting data structure. For completeness, note that a check on non-existing fields in domain expressions does already exist in expression.py.

Regards,
Stefan.

review: Needs Resubmitting

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

I can live with logging.warn and not having an explicit check in read() :-)
It's less consistent, but it's true that data loss is not as much a concern as with create/write.

Thanks for the update, great job!

review: Approve

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

Hi Olivier,

Thanks for all your work on this review and other community branches!

Cheers,
Stefan.

=== modified file 'openerp/addons/base/base_data.xml'
--- openerp/addons/base/base_data.xml	2011-10-24 12:02:19 +0000
+++ openerp/addons/base/base_data.xml	2011-11-02 20:27:24 +0000
@@ -1580,7 +1580,6 @@
 
         <record id="res_bank_1" model="res.bank">
             <field name="name">Reserve</field>
-            <field name="code">RSV</field>
         </record>
 
         <record id="CRC" model="res.currency">
@@ -1600,7 +1599,7 @@
             <field name="name">localhost</field>
             <field name="smtp_host">localhost</field>
             <field eval="25" name="smtp_port"/>
-            <field eval="10" name="priority"/>
+            <field eval="10" name="sequence"/>
 	    </record>
 
         <record id="MUR" model="res.currency">

=== modified file 'openerp/addons/base/module/module_view.xml'
--- openerp/addons/base/module/module_view.xml	2011-07-27 12:34:44 +0000
+++ openerp/addons/base/module/module_view.xml	2011-11-02 20:27:24 +0000
@@ -75,7 +75,6 @@
             <field eval="'ir.module.category'" name="model"/>
             <field name="name">Categorized Modules</field>
             <field eval="'ir.actions.act_window,%d'%action_module_open_categ" name="value"/>
-            <field eval="True" name="object"/>
         </record>
 
 

=== modified file 'openerp/addons/base/res/res_partner_view.xml'
--- openerp/addons/base/res/res_partner_view.xml	2011-10-25 20:48:47 +0000
+++ openerp/addons/base/res/res_partner_view.xml	2011-11-02 20:27:24 +0000
@@ -651,7 +651,6 @@
             <field eval="'res.partner.category'" name="model"/>
             <field name="name">Open partners</field>
             <field eval="'ir.actions.act_window,%d'%action_partner_by_category" name="value"/>
-            <field eval="True" name="object"/>
         </record>
 
         <record id="action_partner_category_form" model="ir.actions.act_window">

=== modified file 'openerp/osv/orm.py'
--- openerp/osv/orm.py	2011-10-19 11:46:18 +0000
+++ openerp/osv/orm.py	2011-11-02 20:27:24 +0000
@@ -3808,6 +3808,7 @@
             for id in ids:
                 result += self._columns[field].set(cr, self, id, field, vals[field], user, context=rel_context) or []
 
+        unknown_fields = updend[:]
         for table in self._inherits:
             col = self._inherits[table]
             nids = []
@@ -3820,9 +3821,14 @@
             for val in updend:
                 if self._inherit_fields[val][0] == table:
                     v[val] = vals[val]
+                    unknown_fields.remove(val)
             if v:
                 self.pool.get(table).write(cr, user, nids, v, context)
 
+        if unknown_fields:
+            self.__logger.warn(
+                'No such field(s) in model %s: %s.',
+                self._name, ', '.join(unknown_fields))
         self._validate(cr, user, ids, context)
 
         # TODO: use _order to set dest at the right position and not first node of parent
@@ -3945,6 +3951,7 @@
                 tocreate[v] = {'id': vals[self._inherits[v]]}
         (upd0, upd1, upd2) = ('', '', [])
         upd_todo = []
+        unknown_fields = []
         for v in vals.keys():
             if v in self._inherit_fields:
                 (table, col, col_detail, original_parent) = self._inherit_fields[v]
@@ -3953,6 +3960,11 @@
             else:
                 if (v not in self._inherit_fields) and (v not in self._columns):
                     del vals[v]
+                    unknown_fields.append(v)
+        if unknown_fields:
+            self.__logger.warn(
+                'No such field(s) in model %s: %s.',
+                self._name, ', '.join(unknown_fields))
 
         # Try-except added to filter the creation of those records whose filds are readonly.
         # Example : any dashboard which has all the fields readonly.(due to Views(database views))