Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] never return less than LIMIT results if they exist in Model.search() #32093

Open
wants to merge 3 commits into
base: 10.0
from

Conversation

Projects
None yet
4 participants
@hbrunn
Copy link
Contributor

commented Mar 25, 2019

Description of the issue/feature this PR addresses: In https://github.com/odoo/odoo/blob/10.0/addons/web/controllers/main.py#L865, we assume that a search will always return limit records if there are enough records, and the rest of the web UI relies on the values returned here. As https://github.com/odoo/odoo/blob/10.0/odoo/models.py#L4278 correctly points out, auto_joined fields will break this behavior, because we filter out duplicates here. This is quite harmful, as searching auto joined fields won't be reliable as soon as there is a limit and duplicates involved, meaning we can't really use auto_join on fields a user searches for.

Current behavior before PR: If there are duplicates when searching for an auto joined field, the UI will eventually show less results than there actually are, because the first search(...limit=) with duplicates will look to the rest of the code as if it's the last page of the result list. In my example case there should be thousands of matches, but the user only sees 78 because already the first page contains duplicates.

Desired behavior after PR is merged: We have reliable behavior with auto_join, meaning search(...limit=limit) always returns limit records (if there are enough records to return), there are no duplicates, and passing count=True returns the amount of records we get when we do an unrestricted search.

Cf #5806, #21526 and my previous attempt to fix this in v7 with which I disagree by now. The current proposal is much faster, more intelligible and more elegant.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@hbrunn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

PS: the same applies to v11, v12, master

@hbrunn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

now with a test to demonstrate the problem

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 27, 2019

@robodoo robodoo removed the CI 🤖 label Mar 29, 2019

@hbrunn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

and now the OCA review process made this better because it now only takes the performance hit when really necessary. Still super puzzled that uniquifying is faster locally than in the database.

@pedrobaeza

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

What I don't like too much are the changes in methods signatures, although really weird, remember for example the problem we had with the upstream change in OCA/server-backend@4ee3ee6

@hbrunn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

this problem was in a method in the ORM. there, it indeed can be hurtful because if any override doesn't provide the keyword argument other code expects, you basically mask the new argument and everything goes to hell.
But this is a pure Python object. It's very improbable people go and inherit and do something with it, and more to the point, save for some tricky monkey patching nobody will change the original class, and that's what's proposed to change here.
If not convincing, I'd propose to replace the constructor parameter by a direct manipulation of the object, that won't break any other code for sure. But it will introduce very hacky stuff into core, I don't believe that's what we should want.

@robodoo robodoo added the CI 🤖 label Mar 29, 2019

@pedrobaeza

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

@StefanRijnhart what do you think?

@StefanRijnhart

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

@pedrobaeza Good point. Mabye a substring match on \bJOIN\b is not such a bad idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.