Merge lp:~therp-nl/openobject-server/7.0-lp1238042-browse_record_refresh_clear_ids into lp:openobject-server/7.0

[Stefan Rijnhart (Opener)]

This branch fixes problems with inaccessible record ids ending up in the browse cache when fetching many2one relationships. A read operation is attempted on these records when fetching new data such as function fields or binary fields or any field after a refresh(). Here for example, the parent company with id 3 is inaccessible but a read operation is still attempted on it after it was added when reading its value from the record's parent_id, together will all other plain fields:

(Pdb) co = self.pool.get('res.company').browse(cr, uid, 1)
(Pdb) co.currency_id.id
1
(Pdb) co._data.keys()
[1, 3]
(Pdb) co.street
*** except_orm: (u'Access Denied', u'The requested operation cannot be completed due to security restrictions. Please contact your system administrator.\n\n(Document type: Companies, Operation: read)')

This branch solves the problem by filtering out inaccessible records from the cache after values for a many2one field were fetched.

Revno. 5101 is green on runbot.

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

Setting to work in progress as Olivier Dony pointed out that this prevents fetching a list of browse records efficiently.

[lp:~therp-nl/openobject-server/7.0-lp1238042-browse_record_refresh_clear_ids]

5102. By Stefan Rijnhart (Opener) on 2013-10-11

[RFR] s/o2m/m2o/

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

Retracting because an alternative fix by Olivier was merged.

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

Hi Stefan,

As you've seen, I tried to come up with a minimalist patch[1] to fix this behavior without incurring the penalty of a second level of ir.rule check for each browse prefetch.
Your LBYL approach works fine but most models have multiple m2o relationships, so I believe the cost would be significant - probably a couple of extra SQL roundtrips for each browse fetch (Discl: I did not benchmark your patch). Browse_records are designed to only cost 1 or 2 SQL queries for the basic `for r in self.browse(..)` loop that only involves database-stored fields, regardless of the number of fields and records being browsed. If you add a couple of second-level ir.rule check queries to that every time, it seems like it could make browse() perform a couple of times slower on average.

The patch I wrote uses an EAFP approach instead and tries to detect the cases where the prefetching system causes an "artificial" access error. When that happens, it retries the operation only for the record being browsed, effectively turning off prefetching. As the bug occurred rather seldom, this should hopefully not affect average performance in a significant way. However when it happens, prefetching is silently [2] disabled, and further browse() usage will be slower. This may or may not be desirable, depending on occurrence frequency.

Watching the log for the "Prefetching attempt for fields [..] on [model] failed..." should tell us if that happens too often.

Thanks a lot for your work on this issue, I hope the patch works for you!

[1] For the record: server 7.0 revno 5136 rev-id: <email address hidden>
[2] It's actually logged at INFO level, but the user/developer may not notice it - possibly missing a bad ir.rule definition that uselessly impacts performance.

Unmerged revisions

5102. By Stefan Rijnhart (Opener) on 2013-10-11

[RFR] s/o2m/m2o/

5101. By Stefan Rijnhart (Opener) on 2013-10-10

[FIX] Bypass model access check to allow for mail.message's bypass of
 this mechanism

5100. By Stefan Rijnhart (Opener) on 2013-10-10

[FIX] Don't _search when superuser, as this causes a loop when records
 are browsed as record rules are applied

5099. By Stefan Rijnhart (Opener) on 2013-10-10

[RFT] Filter out inaccessible ids from the browse record cache
 after fetching

5098. By Stefan Rijnhart (Opener) on 2013-10-10

[RVT] Undo previous change as it does not allow to efficiently fetch
 data for a list of browse records (thanks to Olivier Dony for
  pointing that out)

5097. By Stefan Rijnhart (Opener) on 2013-10-10

[FIX] Also fix an additional access error when the id of an inaccessible
 record is in the _data dictionary and you ask for the value of
 any function field for the first time.

5096. By Stefan Rijnhart (Opener) on 2013-10-10

[FIX] Don't preserve resource ids when clearing browse record cache, to
 prevent access errors after a refresh when the cache contains ids
 of unreadable records (for instance, of a parent company)
 Make accessing browse records robust against missing keys from
 the dictionary because of the first change.

=== modified file 'openerp/osv/orm.py'
--- openerp/osv/orm.py	2013-10-10 17:07:18 +0000
+++ openerp/osv/orm.py	2013-10-11 07:22:21 +0000
@@ -420,6 +420,7 @@
                 _logger.warning("No field_values found for ids %s in %s", ids, self)
                 raise KeyError('Field %s not found in %s'%(name, self))
             # create browse records for 'remote' objects
+            new_m2o = {}
             for result_line in field_values:
                 new_data = {}
                 for field_name, field_column in fields_to_fetch:
@@ -448,6 +449,8 @@
                                         context=self._context,
                                         list_class=self._list_class,
                                         fields_process=self._fields_process)
+                                    if value not in new_m2o.get(obj, []):
+                                        new_m2o.setdefault(obj, []).append(value)
                                 else:
                                     new_data[field_name] = value
                             else:
@@ -474,6 +477,15 @@
                         new_data[field_name] = result_line[field_name]
                 self._data[result_line['id']].update(new_data)
 
+            if self._uid != SUPERUSER_ID:
+                for obj, obj_ids in new_m2o.iteritems():
+                    accessible_ids = obj._search(
+                        self._cr, self._uid, [('id', 'in', obj_ids)],
+                        context=self._context, access_rights_uid=SUPERUSER_ID)
+                    for obj_id in obj_ids:
+                        if obj_id not in accessible_ids:
+                            self._cache[obj._name].pop(obj_id)
+
         if not name in self._data[self._id]:
             # How did this happen? Could be a missing model due to custom fields used too soon, see above.
             self.__logger.error("Fields to fetch: %s, Field values: %s", field_names, field_values)