7.0: res.partner - name_search applies limit too early

Bug #1203727 reported by Alexandre Fayolle - camptocamp
24
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Odoo Server (MOVED TO GITHUB)
Fix Released
Low
OpenERP Publisher's Warranty Team

Bug Description

the name_search method of res.partner applies the limit argument twice, and the first one is too soon.

Steps to reproduce the bug:

* connect to runbot
* create and save 8 suppliers called AAA1, AAA2, ... AAA8, with the is_company field checked
* create and save 1 customer called AAA10, with a is_company field checked
* open the creation form of a sale.order, set. In the "partner" field, type 'AA' (without quotes)

Expected result : a list of customers, with AAA10 included

Actual result: the list of customers has 1 single customer, "Best Designers, Ayaan Agarwal"

If you type a 3rd 'A' in the field, then AAA10 is displayed.

The cause of the issue is that the limit parameter (set to 8 by the web client) is used first in the SQL query (SELECT partner.id FROM res_partner partner[...] + limit_str which returns a list of at most 8 ids in this case. Then the list of ids is passed to search, which will further reduce the number of entries, which means that in some extreme cases, if the 1st SELECT query only returns records which don't match the args criteria added to the search call, the suggestion list will be empty.

Tags: maintenance

Related branches

Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

Here is a yaml test illustrating the issue. Please make sure that the patch for the bug features the fix (and that the tests are green)

tags: added: maintenance
Amit Parik (amit-parik)
Changed in openobject-server:
assignee: nobody → OpenERP Publisher's Warranty Team (openerp-opw)
Changed in openobject-server:
status: New → Fix Committed
Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

The steps to reproduce work as expected when I try them (i.e. no bug), and I think it's because the behavior of the limit-restricted name_search is not deterministic. This odd behavior is not caused by the limit being applied twice but by the absence of ORDER BY clause in the pure SQL pre-filtering. The YAML test proposed on the merge proposals also never failed when I tried it, for the same reasons.

This non-deterministic behavior is fixed in the third branch[1] linked to this bug, at the cost of a performance hit due to the extra ORDER BY clause. This sort operation seems unfortunately inevitable in order to apply the limit correctly, and that requires sorting the whole list of results - possibly the complete table if the search criterion is very wide, such as " ". From my tests the name_search still returns under 0.5s for a search on " " on a database with 150k partners, which is not totally unusable.

The point about the limit being applied twice is however valid, and there is a very good performance reason why it is so, as explained in comment #3 on MP[2]. One possible fix for this issue is discussed on MP[2] too.

[1]: https://code.launchpad.net/~openerp-dev/openobject-server/7.0-bug-1203727-order-by
[2]: https://code.launchpad.net/~tsabi/openobject-server/7.0-tsabi-res_partner_name_search_limit_fix/+merge/176050

Changed in openobject-server:
importance: Undecided → Low
status: Fix Committed → Confirmed
Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

The patch mentioned in previous comment (branch [1]) has been merged in server 7.0 at revision 5067 rev-id: <email address hidden>.
It should decrease a lot the chances of seeing inconsistent results from one keystroke to the next during many2one auto-completion.
The double limit application remains, but should have a much more limited impact now.

Changed in openobject-server:
milestone: none → 7.0
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

unfortunately, the bug is still there: on runbot, with the current setup, if I type "aa" in the creation step of a Sale Order, I see AAA10 but not "Best Designers, Ayaan Agarwal", which suddenly pops up if I type an additional "n".

Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

in addition to my previous comment: lp:~openerp-dev/openobject-server/7.0-bug-1203727-order-by does not fix the issue

In the real case reported by my customer, the database has different subsidiary companies in the customers sharing common prefixes in their names. The impression that the users have is that the company they are looking for when typing the first letters of the name and getting a list of suggestions with only 1 element which is not the one they are looking for is that the partner does not exist in database. I can certainly tell them to type more letters but this is at best an ugly workaround for a real useability issue (what if the user does not remember enough of the name to properly type in more letters?)

Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote : Re: [Bug 1203727] Re: 7.0: res.partner - name_search applies limit too early

On 09/17/2013 04:44 PM, Alexandre Fayolle - camptocamp wrote:
> unfortunately, the bug is still there: on runbot, with the current
> setup, if I type "aa" in the creation step of a Sale Order, I see AAA10
> but not "Best Designers, Ayaan Agarwal", which suddenly pops up if I
> type an additional "n".

Yes, that's why I explicitly mentioned that the patch does not solve the bug.
If anything, it actually makes the bug more consistently reproducible, while it
used to be semi-random.

I've highlighted some possible solutions to the fundamental problem in comment
#3 of MP
https://code.launchpad.net/~tsabi/openobject-server/7.0-tsabi-res_partner_name_search_limit_fix/+merge/176050

However the suggested patch is ugly and complex, and the person writing it
should be sure to understand the performance implication of any alteration of
the current code.

As a possible workaround, I've pushed an experimental fix in branch
lp:~openerp-dev/openobject-addons/7.0-bug-1203727-hack. It might sufficiently
decrease the chance of triggering the bug, without incurring a noticeable
performance hit on most databases, and without the complexity of the full
patch. It is still possible to trigger the bug with pathological cases, so
testing it on real customer databases should tell us if it is an acceptable
workaround for v7.0.
v8 will feature a complete fix, but this requires a change in the ORM system
that is not suitable for 7.0 stable.

Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

Sorry, the branch mentioned in previous commit should read:
    lp:~openerp-dev/openobject-server/7.0-bug-1203727-hack

Revision history for this message
Daniel Hammerschmidt (redneck) wrote :

I figured some more problems with the current implementation.

- In the SQL query we should check for the active flag as Model.search() does.
- To let the web client display the 'Search more...' entry we should append empty entries to fill the result set up to the limit if there are more name-matching partners.
- To reduce the number of calls to name_search() we should use the default value (300msec) for many2one autocomplete widgets. (OpenERP is not Google).
- To reduce the number of empty entries in the result set we could do some more work in a loop. This is expensive for vague search expressions but the user gets what he or she expects.

The server part:
http://bazaar.launchpad.net/~redneck/+junk/7.0-server-res_partner_name_search_bug1203727-redneck/revision/
The web client part:
http://bazaar.launchpad.net/~redneck/+junk/7.0-web-many2one_autocomlete_default_delay-redneck/revision/

The server part still contains code for profiling. Just set profile in name_search() to True.

Revision history for this message
Martin Trigaux (OpenERP) (mat-openerp) wrote :

Hello guys,

I have created a patch applying Olivier's recommendations (execute one global sql query including the limit and the orm underlying search).
The name_search call will now return the correct number of records (and hopefully be faster).

Please note that this is not needed for trunk as display_name is stored.

revno: 5216 [merge]
revision-id: <email address hidden>

Changed in openobject-server:
status: Confirmed → Fix Released
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

Hello Martin,

Thanks, this is great news.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.