Closed (fixed)
Project:
Search API
Version:
8.x-1.x-dev
Component:
Plugins
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
20 May 2017 at 12:26 UTC
Updated:
1 Jul 2017 at 07:15 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
anton4uk commentedComment #3
borisson_I have no idea why this would make a difference or what's actually going on here.
So we should write a test that describes the problem here.
PS: It seems you (like many others – it's really easy to misinterpret) are confused by the "Issue tags" field. As the guidelines state, they aren't meant for free text tags related to the issue, but only for specific categorization purposes, usually by module maintainers.
So, if you aren't sure your current usage is correct, please just leave the field empty.
Comment #4
drunken monkeyThanks a lot for reporting this and already providing a fix!
Joris: Admittedly, the issue description isn't very helpful, but the patch points clearly to the problem: if we don't specify an explicit sort, the order of returned result rows is undefined. Therefore, if we don't request all results at once but use paging, it's not guaranteed that separate pages will actually contain different results. (Especially if paging through all results takes some time, which means new rows might be added in the meantime.)
MySQL generally sorts by the time a row was added, as far as I know, so this problem doesn't exist there and apparently we just relied on that. It's, however, completely possible that for other DBMSs (or maybe even special MySQL setups or scenarios) this will fail, returning some item IDs multiple times and others not at all. (From the issue summary it sounds like Postgres might even have a check for that: if someone requests a range but specifies no sort, just return the first
$limitresults regarding of$offset– serves them right, the dirty, MySQL-loving bugger!)So, adding a sort there is definitely the clean thing to do.
Regarding tests, it turns out (as far as I can see) that that method was always just tested implicitly, but never explicitly, even though it's very easy to test. I've now added some tests, and actually discovered a different bug in the process, or even multiple ones.
Patch attached, with two different "test only" variants so we can also see whether we can reproduce the original problem. (Might not be the case, though – as said, it might depend on the specific DBMS setup.)
Comment #6
drunken monkeyAs feared, the patch without the added sort doesn't actually fail. But, it also definitely doesn't break anything, shouldn't matter much performance-wise (I hope) and, as explained, I'm pretty sure it's the correct way to do this, even if it also works without the sort in most cases.
Comment #7
borisson_Sure, now that you explain it, I understand the problem, fix looks good and the added testcode looks solid.
Comment #9
drunken monkeyOK, good to hear. Thanks for your feedback!
Committed.
Thanks again also to Anton4yk for the initial patch!