Merge lp:~openerp-dev/openobject-server/6.0-opw-577963-msh into lp:openobject-server/6.0

[Mohammed Shekha(Open ERP)]

Hello,

Fixed the issue of ir_attachment which reads all the records for checking ir.access due to which ir_attachment tree view becomes to slow even if it loads only 20 records, changed the code and do the same with DISTINCT res_model, if user don't have rights then and only then read the ids of that object and remove it from ids.

When we click on document menu so it will read first 20 records and then it will call search_count(obviously without limit) for pager limit, search_count will call search method which is overridden in ir_atachment.py, and as there is not limit(search is called from search_count) so it will read all the attachments and then check for the user access on res_model, if user dont have rights then that attachment should not be shown to him but reading of all record will take time when there are lots of attachment and here we don't need to read all record because we only remove those ids of model on which user don't have access.

So fetched all distinct res_model and check it with ir.access and read only those ids on model on which user don't have access, this way it will save the time of fetching search result.

I have scenario in which I have 90000 attachments and loading of document tree view with only 20 records taking approximately 10 second, after this patch it will take 3 to 4 second.

Thanks.

[Mohammed Shekha(Open ERP) (msh-openerp)]

review: Abstain

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

This is not correct, the logic for filtering based on access rights was already done efficiently.
Your patch makes it check the access right of only 1 document of each model, but every document may have different access restrictions depending on access rules (e.g. multicompany, etc.).

BTW the OPW case has already been closed and a different patch was merged, so you don't need to work on this anymore.
The bulk of the performance problem with large databases was caused by the numerous calls to list.remove(), which are very slow on large lists. Using a set() instead made it a lot faster (50s -> 3s for a real database with 100k random attachments). Your measures were a lost faster than that, probably because most attachments were duplicates and therefore attached to the same (model,res_id) - biasing the result heavily.

Thanks,

PS: why did you review your own MP with "Abstain"?

review: Disapprove

[Mohammed Shekha(Open ERP) (msh-openerp)]

Hello Olivier,

Thanks for the review, yes I am agree that logic is not correct, I have checked with scenario in which this was improving latency but afterward I found that this will not be the proper fix, hence I have temporarily set it as a abstain and decided to improve it but afterward it has been improved with other fix and case is closed.

Hence now this branch is no more needed.

Thanks.

Unmerged revisions

3633. By Mohammed Shekha(Open ERP) on 2012-08-17

[FIX]Fixed the issue of ir_attachment which reads all the records for checking ir.access due to which ir_attachment tree view becomes to slow even if it loads only 20 records, changed the code and do the same with DISTINCT res_model, if user don't have rights then and only then read the ids of that object and remove it from ids.

=== modified file 'bin/addons/base/ir/ir_attachment.py'
--- bin/addons/base/ir/ir_attachment.py	2011-05-02 11:57:10 +0000
+++ bin/addons/base/ir/ir_attachment.py	2012-08-17 10:56:22 +0000
@@ -68,26 +68,21 @@
         # For attachments, the permissions of the document they are attached to
         # apply, so we must remove attachments for which the user cannot access
         # the linked document.
-        targets = super(ir_attachment,self).read(cr, uid, ids, ['id', 'res_model', 'res_id'])
         model_attachments = {}
-        for target_dict in targets:
-            if not (target_dict['res_id'] and target_dict['res_model']):
-                continue
-            # model_attachments = { 'model': { 'res_id': [id1,id2] } }
-            model_attachments.setdefault(target_dict['res_model'],{}).setdefault(target_dict['res_id'],set()).add(target_dict['id'])
-
         # To avoid multiple queries for each attachment found, checks are
         # performed in batch as much as possible.
         ima = self.pool.get('ir.model.access')
-        for model, targets in model_attachments.iteritems():
+        cr.execute("select DISTINCT res_model, res_id from ir_attachment")
+        for model, res_id in cr.fetchall():
+            if model and res_id:
+                model_attachments[model] = res_id
+        for model,targets in model_attachments.iteritems():
             if not ima.check(cr, uid, model, 'read', raise_exception=False, context=context):
-                # remove all corresponding attachment ids
-                for attach_id in itertools.chain(*targets.values()):
+                read_ids = super(ir_attachment,self).search(cr, uid, [('res_model', '=', model)], context=context)
+                for attach_id in read_ids:
                     ids.remove(attach_id)
                 continue # skip ir.rule processing, these ones are out already
-
-            # filter ids according to what access rules permit
-            target_ids = targets.keys()
+            target_ids = [targets]
             allowed_ids = self.pool.get(model).search(cr, uid, [('id', 'in', target_ids)], context=context)
             disallowed_ids = set(target_ids).difference(allowed_ids)
             for res_id in disallowed_ids: