Merge lp:~openerp-dev/openobject-server/6.1-res_partner_address_name_search-rco into lp:openobject-server/6.1

[Raphael Collet (OpenERP)]

Make the method name_search of res.partner.address consistent with method name_get.
It searches on all the fields that are used in name_get in the same context.

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

Functionally it looks good to me, with one exception: despite name_get() not using the `zip` column, it seems a useful thing to allow searching on zipcodes, as it could be a useful way to get a shortlist of possible candidates. Seeing that it used to have the priority over other columns in the previous implementation, dropping it completely sounds dangerous for a stable version. We could simply add it to the default list of fields, this still means that name_search will behave as expected wrt name_get (especially since zip codes can hardly be confused with other column values).

On a technical level I have one performance question: have you tested this name_search() implementation on a large database containing 10k+ randomly distributed addresses? Past experience told us that performance can become terrible when searching on an OR'ed domain involving translatable records (such as countries here!), due to the complexity of the query being executed on the database. For instance for product.name_search() we had to specifically execute the various sub-queries separately and merge the results in-memory, otherwise the system became very slow with 10k products [1]. I think it makes sense to double-check this here too - creating 10k random addresses is easy with a few `INSERT SELECT` queries.

Finally, some minor technical remarks:
- l.10,l.23: while you're changing the code, it would be nice to use proper spacing for == operators ;-)
- l.25: using a list comprehension is usually much more readable than map():
   [(f, operator, name) for f in fields] vs map(lambda f: (f, operator, name), fields)
- l.25: using expression.OR(domains...) might be more readable than hand-crafting the OR'ed domain

Thanks!

[1] See the product.name_search() implementation note: http://bazaar.launchpad.net/~openerp/openobject-addons/6.1/view/head:/product/product.py#L619

review: Needs Information

[Raphael Collet (OpenERP) (rco-openerp)]

Thanks, I was not aware of the poor performance of these queries. I'll rewrite the code to follow the former style "incremental search on fields".

[lp:~openerp-dev/openobject-server/6.1-res_partner_address_name_search-rco]

4115. By Raphael Collet (OpenERP) on 2012-03-21

[IMP] res.partner.address: improve name_search() to avoid performance issues

4116. By Raphael Collet (OpenERP) on 2012-03-21

[IMP] res.partner.address: in name_search, add handle case where name is empty

[Raphael Collet (OpenERP) (rco-openerp)]

Reworked name_search() to make a series of incremental search statements instead of a big disjunction. This should handle the potential performance issue. It also stops searching when the search limit has been reached.
Added some comments to justify the code as well.

review: Needs Resubmitting

[lp:~openerp-dev/openobject-server/6.1-res_partner_address_name_search-rco]

4117. By Raphael Collet (OpenERP) on 2012-03-21

[IMP] res.partner.address: in name_search, search on partner_id before other fields

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

Looks just fine now, thanks!

review: Approve

=== modified file 'openerp/addons/base/res/res_partner.py'
--- openerp/addons/base/res/res_partner.py	2012-01-12 10:50:43 +0000
+++ openerp/addons/base/res/res_partner.py	2012-03-21 13:24:20 +0000
@@ -349,18 +349,33 @@
             args=[]
         if not context:
             context={}
-        if context.get('contact_display', 'contact')=='partner ' or context.get('contact_display', 'contact')=='partner_address '  :
-            ids = self.search(cr, user, [('partner_id',operator,name)], limit=limit, context=context)
+
+        if not name:
+            ids = self.search(cr, user, args, limit=limit, context=context)
+        elif context.get('contact_display', 'contact')=='partner':
+            ids = self.search(cr, user, [('partner_id', operator, name)] + args, limit=limit, context=context)
         else:
-            if not name:
-                ids = self.search(cr, user, args, limit=limit, context=context)
+            # first lookup zip code, as it is a common and efficient way to search on these data
+            ids = self.search(cr, user, [('zip', '=', name)] + args, limit=limit, context=context)
+            # then search on other fields:
+            if context.get('contact_display', 'contact')=='partner_address':
+                fields = ['partner_id', 'name', 'country_id', 'city', 'street']
             else:
-                ids = self.search(cr, user, [('zip','=',name)] + args, limit=limit, context=context)
-            if not ids:
-                ids = self.search(cr, user, [('city',operator,name)] + args, limit=limit, context=context)
-            if name:
-                ids += self.search(cr, user, [('name',operator,name)] + args, limit=limit, context=context)
-                ids += self.search(cr, user, [('partner_id',operator,name)] + args, limit=limit, context=context)
+                fields = ['name', 'country_id', 'city', 'street']
+            # Here we have to search the records that satisfy the domain:
+            #       OR([[(f, operator, name)] for f in fields])) + args
+            # Searching on such a domain can be dramatically inefficient, due to the expansion made
+            # for field translations, and the handling of the disjunction by the DB engine itself.
+            # So instead, we search field by field until the search limit is reached.
+            while len(ids) < limit and fields:
+                f = fields.pop(0)
+                new_ids = self.search(cr, user, [(f, operator, name)] + args, limit=limit, context=context)
+                # extend ids with the ones in new_ids that are not in ids yet (and keep order)
+                old_ids = set(ids)
+                ids.extend([id for id in new_ids if id not in old_ids])
+
+        if len(ids) > limit:
+            ids = ids[:limit]
         return self.name_get(cr, user, ids, context=context)
 
     def get_city(self, cr, uid, id):