-
Notifications
You must be signed in to change notification settings - Fork 17
IBX-10186 Add limits to count and subtree queries #600
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
Open
ryanolee
wants to merge
23
commits into
ibexa:4.6
Choose a base branch
from
ryanolee:feature/IBX-10186
base: 4.6
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
00e41bc
IBX-10176 Forward patch Ibexa count modifications
2283e73
IBX-10186 Fix location service
e0112db
IBX-10186 CS Fixes
88043e8
Update tests/integration/Core/Repository/LocationServiceTest.php
ryanolee d886f29
Update src/lib/Persistence/Legacy/Traits/Doctrine/LimitedCountQueryTr…
ryanolee 3beb674
IBX-10186 Use symfony BC break approach
fa1ee68
IBX10186 CS fixup
5b2fe00
IBX-10186
804f1a7
IBX-10186 Hack to get Contract BC breaks fixed
12fcae7
IBX-10186 Add assert not null
2720425
IBX-10186 Pre-emptiveley remove call to deprecated call
3674837
IBX-10186 Add prompts for lazy loaded value holders
7d55dd0
IBX-10186 Backwards compat and test fixes.
1f33f28
IBX-10186 Fix more php 8.1 regressions
162806f
IBX-10186 - PHP stan fixes
8c99daf
IBX-10186 Run CS
9d2d107
IBX-10186 Fix exception type
80fe453
IBX-10186 Fix merge issues
f25060d
Regenerated PHPStan baseline
Steveb-p eb83b30
IBX-10186 Update baseline
500b400
IBX-10186 Update exception type
2c1b555
IBX-10186 Run everything like 20 times and to not play wack-a-mole wi…
a54d922
IBX-10186 Fix PSR deciding to update their libs
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
56 changes: 56 additions & 0 deletions
56
src/lib/Persistence/Legacy/Traits/Doctrine/LimitedCountQueryTrait.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @copyright Copyright (C) Ibexa AS. All rights reserved. | ||
| * @license For full copyright and license information view LICENSE file distributed with this source code. | ||
| */ | ||
| declare(strict_types=1); | ||
|
|
||
| namespace Ibexa\Core\Persistence\Legacy\Traits\Doctrine; | ||
|
|
||
| use Doctrine\DBAL\Query\QueryBuilder; | ||
|
|
||
| /** | ||
| * Limited Count count trait. Used to allow for proper limiting of count queries | ||
| * when using Doctrine DBAL QueryBuilder. | ||
| */ | ||
| trait LimitedCountQueryTrait | ||
| { | ||
| /** | ||
| * Takes a QueryBuilder and wraps it in a count query. | ||
| * This performs the following transformation to the passed query | ||
| * SELECT DISTINCT COUNT(DISTINCT someField) FROM XXX WHERE YYY; | ||
| * To | ||
| * SELECT COUNT(*) FROM (SELECT DISTINCT someField FROM XXX WHERE YYY LIMIT N) AS csub;. | ||
| * | ||
| * @param \Doctrine\DBAL\Query\QueryBuilder $queryBuilder | ||
| * @param string $countableField | ||
| * @param mixed $limit | ||
| * | ||
| * @return \Doctrine\DBAL\Query\QueryBuilder | ||
ryanolee marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| */ | ||
| protected function wrapCountQuery( | ||
| QueryBuilder $queryBuilder, | ||
| string $countableField, | ||
| ?int $limit, | ||
| ): QueryBuilder { | ||
| $useLimit = $limit !== null && $limit > 0; | ||
ryanolee marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if (!$useLimit) { | ||
| return $queryBuilder; | ||
| } | ||
|
|
||
| $querySql = $queryBuilder->select($countableField) | ||
| ->setMaxResults($limit) | ||
| ->getSQL(); | ||
|
|
||
| $countQuery = $this->connection->createQueryBuilder(); | ||
|
|
||
| return $countQuery | ||
| ->select( | ||
| $queryBuilder->getConnection()->getDatabasePlatform()->getCountExpression('*') | ||
| ) | ||
| ->from('(' . $querySql . ')', 'csub') | ||
| ->setParameters($queryBuilder->getParameters(), $queryBuilder->getParameterTypes()); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a fan of traits under certain circumstances. I somewhat prefer object composition approach unless there are valid reasons.
In this case, trait is used to prevent code duplication only, and it requires presence of
connectionproperty (which is an implicit assumption not visible outside of trait).Since the method added is using
protectedvisibility, it is immediately available to descendants. While not a big deal, I would personally prefer it to useprivatevisibility (if left as-is, that is).Additionally, since it is a trait, unit tests are more difficult to provide.
Overall, I'd suggest making it it's own class, and injecting it into relevant gateways via constructor. To facilitate usage of the correct
Connectionobject, it could be passed as part of the method arguments.WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit awkward given it is multiple inheritance of a shared bit of functionality. Though do agree it should probably be refactored into it's own thing. Will take a look at refactoring into something that can be unit tested and refactor. The connection was misued here as it can be pulled from the passed query builder so there is no implicit requirement on there being a
connectionpresent as seen with a few lines below where $queryBuilder->getConnection()->getDatabasePlatform()->getCountExpression('*') is used. But based on everything else that is a mute point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, right, now I've noticed it.
By the by, note that
Platform::getCountExpression()is deprecated and removed in DBAL v3. We are now expected to use"COUNT(*)"directly, without asking platform for their variant. I assume that is becauseCOUNT()is ANSI SQL, and there were never any platform differences to begin with.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that makes sense I am currently aiming to keep things as closeley aligned with the way ibexa currently generates it's queries internally. It is likely
Platform::getCountExpressionis better placed to be updated globally in a separate ticket considering that is what is already in use now and this is not a wide sweeping change targeting specifically thatThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is updated in 5.x. I'd only ask to not use it here, as it will make merge up simpler :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh that makes sense! Sorry saw all of the other references to it and got a bit confused. Updated as outlined here https://github.com/doctrine/dbal/blob/4.2.x/UPGRADE.md#deprecated-redundant-abstractplatform-methods