Merge lp://qastaging/~tsabi/openobject-server/7.0-tsabi-res_partner_name_search_limit_fix into lp://qastaging/openobject-server/7.0

Hello. I've linked your branch to lp:1203727. I added a YAML test for that bug. It would be great if you could include that test in your branch as it would help seeing if the fix works via the runbot.

YAML test diff : https://bugs.launchpad.net/openobject-server/+bug/1203727/+attachment/3747233/+files/res_partner_test_lp1203727.diff

Hi Csaba TOTH,

Don't you think your fix will by pass the access rules check on res.partner. i.e the user who is not intended to see a record will be able to see them. ?

Thanks,
Naresh Soni
OpenERP Enterprise Services !

Hi Naresh!
Yes you have right, i didn't thinked search() is used to filter out the prohibited items by ACL.
So the only solution is to limit the items at the last moment of processing: when we call that search() method.

Please review it again!

Thanks, BR,
tsabi

Concerning the observation of access rules, you might want to have a look at related bug 1094212 and the corresponding MP: https://code.launchpad.net/~openerp-community/openobject-server/fix-1094212-multicompany-res_partner/+merge/141936, for reference.

Concerning the patch, it is definitely not wise to remove the limit in the initial SQL query, because for large database you will make the name_search prohibitively slow (for the early search criterions, e.g "a" or " "). It is meant to be as fast as possible, remember that it can be triggered many times while you're typing the search criterion.

As was discussed on bug 1094212, the simplest fix for both issues would be to only conduct one search operation, either the SQL or the ORM. Since we can't do it in 7.0 using the ORM[1], the best option seems to do it in pure SQL only.
This means you'd have to modify your patch (again! sorry ;-)) to construct a full SQL query that includes the domain passed in `args` and the application access rules. Technically this means you need to replicate a part of what the ORM search() implementation[2] does: obtain a proper osv.Query object for the given domain using `_where_calc()`, apply the the access rules (ir.rule entries) on top of it, then alter the resulting query before executing it to get the desired results.
In trunk we will simplify this by storing the display name and replace this complexity with a single search() call.

I hope this is clear enough, otherwise do not hesitate to ask for more details.

One more minor remark (@Alexandre too): the YAML test case is useful but it would be best written in Python, now that v7.0 supports simpler Python-based tests. I certainly don't want to discourage writing YAML tests but you'd benefit from the unittest2 TestCase API and richer assertions in this case. You could easily add it in the base/tests/test_base.py TestCase[3].

Thanks for working on this!

[1] Because there is not stored version of the "display name" on which we need to match unless you install the `account_report_company` module, so we can't express that criterion in a regular domain in the `base` module.
[2] http://bazaar.launchpad.net/~openerp/openobject-server/7.0/view/5034/openerp/osv/orm.py#L4827
[3] http://bazaar.launchpad.net/~openerp/openobject-server/7.0/view/5034/openerp/addons/base/tests/test_base.py

@odony: point taken for the unittest. Will keep this in mind for future tests. In the meantime, please don't let this one be wasted, and pass the message to the support teams :-)

Dear Olivier,

i clearly understand what you requested from me, i thinked about this and here is my comments:

I don't agree it will be significant quicker if i copy the functionality of the ACL checking in the search() method into a big SQL code. It is an expensive sql query versus severe cheap one. Because i have only one database server, but several application server, i prefer to work the database server the less, because if i all the time calculate everything with the database server, than it wont be a load balance, but i have more processor power for the application servers.

And maybe search() have other functionlities what i can dismiss by coping its code to here, but i dont believe it will be significant quicker.

Another argument along these is the code duplication. This ACL check is already written in the search() method, if there is any error there we need to mark a comment that this functionality is duplicated so we need to fix at another place too.

So i still suggest to accept this patch, and accept the fact that because 7.0 is in frozen state, and without the pre construction of full_name field (what is in the trunk i think) name searching in 7.0 will be slower. This is a limitation in 7.0, and we should live with this.

On 07/25/2013 02:40 PM, Csaba TOTH wrote:
> I don't agree it will be significant quicker if i copy the functionality of
> the ACL checking in the search() method into a big SQL code. It is an
> expensive sql query versus severe cheap one.

That does not seem correct, or perhaps I was not quite clear.

Currently we're doing roughly these operations inside partner's name_search:
   1. Custom SQL: SELECT FROM res_partner WHERE display_name ilike <pattern> ...
      -> returns <ids>
   2. search() validates ACL: SELECT FROM res_partner WHERE id in <ids> AND
                                 company_id = <user_company_id> AND ...
      -> returns <final_ids>

What I meant is we could do this in one single operation instead:
   1. Custom SQL: SELECT FROM res_partner WHERE display_name ilike <pattern> AND
                                 company_id = <user_company_id> AND ...
      -> returns <final_ids>

That would actually make the database work slightly less, because it would only
have to do one SELECT instead of dividing the *same work* in 2 steps. And all
of the above (including ACL filtering) is absolutely required in order to
obtain correct results.

> Another argument along these is the code duplication. This ACL check is
> already written in the search() method, if there is any error there we need
> to mark a comment that this functionality is duplicated so we need to fix at
> another place too.

Yes the code duplication is a downside, but it's only a temporary low-risk fix
for 7.0, we'll drop that in trunk. The duplicated code is also quite unlikely
to receive changes or bugfixes in 7.0, as it's part of the stable core API.

> So i still suggest to accept this patch, and accept the fact that because
> 7.0 is in frozen state, and without the pre construction of full_name field
> (what is in the trunk i think) name searching in 7.0 will be slower. This is
> a limitation in 7.0, and we should live with this.

I'm afraid that would be unacceptable. You may not worry much about slightly
slower response times for small databases, but name_search() is involved not
only in UI auto-completion but also in the evaluation of any OpenERP domain
that uses a many2one field.
As a consequence, it could make many operations much slower for any database
with more than a few thousand partner entries, and this is not an option.

Note that you can perfectly well use the current patch on your deployments if
your prefer, but we'll have to take care of this in the final patch that is
used for OpenERP 7.0.

I'm marking the MP as rejected due to the performance issues it introduces, making it unsuitable for OpenERP 7.0.
I'll be glad to put it back to Work In Progress if you still plan to work on it, of course.