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

BUG Update DataQuery::exists to return false when limit causes no result to be returned #9946

Merged

Conversation

maxime-rainville
Copy link
Contributor

This was raised by @mfendeksilverstripe.

While upgrading from 4.5 to 4.7, he discovered that DataList who are constrained by a limit into retuning 0 results, would still be considered "truthy" when a if statement was applied to them in an SS template:

4.5
<% if $Results %> <-- DataList which yields 0 results is treated as false
4.7
<% if $Results %> <-- DataList which yields 0 results is treated as true

$this->assertTrue(ObjectE::get()->exists());
$this->assertFalse(ObjectE::get()->where(['"Title" = ?' => 'Foo'])->exists());
$this->assertTrue(ObjectE::get()->dataQuery()->groupby('"SortOrder"')->exists());
$this->assertTrue(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the scenarios here to test both the DataList exist and the DataQuery exists. I've added scenarios for the limit and added explicit message for each assertion.

@bergice
Copy link
Contributor

bergice commented May 20, 2021

LGTM.

Needs a lint fix.

Wondering if it should also handle cases where ->limit(0) is applied? In that case this won't work because of vendor/silverstripe/framework/src/ORM/Connect/MySQLQueryBuilder.php:33.

@ScopeyNZ
Copy link
Contributor

Why would a limit return zero results? Is this in the instance where you're fetching page 3 or something, and your limit is LIMIT 20 OFFSET 40, but you only have 35 results?

I wonder how the engine handles an exists statement that has a limit - would you lose potential performance benefits when indexes may not be consulted?

I guess it'll have to be extra important that parts of the code that are looking for any record regardless of pagination ensure they have not set a limit on their DataQuery.

@maxime-rainville
Copy link
Contributor Author

Is this in the instance where you're fetching page 3 or something, and your limit is LIMIT 20 OFFSET 40, but you only have 35 results?

Yes. We can have a look at the performance impact, but this is a regression and we need to find some fix.

You can't trigger this behaviour with setLimit(0) because that removes any limit.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Weird behaviour is probably linked to this change 9159137

It's consistent with the original report (on slack) about count() working correct, though exists() being strange

The original report also had an replication example provided of $results->limit(4, 12811); which is a small limit with a huge offset, which is similar to what Guy mentioned earlier re LIMIT 20 OFFSET 40 on a 35 row table

@emteknetnz emteknetnz merged commit 472fc4e into silverstripe:4.7 May 31, 2021
@emteknetnz emteknetnz deleted the pulls/4.7/exist-honour-limit branch May 31, 2021 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants