IBX-10186 Add limits to count and subtree queries

[ryanolee]

🎫 Issue	IBX-10186

Warning

Please check ezsystems/ezplatform-kernel#413 for detailed reasons for this PR. Though the TLDR is count queries cause disproportionate impact on Ibexa Platform instances with large amounts of content (100,000+ content objects)

Related PRs:

• IBX-10186 Add $limit Option to Count ezsystems/ezplatform-kernel#413
• IBX-10174 Add limit option to getSubtreeSize ezsystems/ezplatform-kernel#412

Required by

ibexa/admin-ui#1605

Description:

This is a forward facing patch from issues present in Ibexa 3.3 (The version we are still using) to port the fixes and public API updates into 4.6.

Understandably considering this PR makes changes to Contracts it might need to go into 4.7 (If a release is planned beyond LTS) or v5 given implementing clients will break. I am not sure on the release cadence for contracts so this might be somewhat problematic.

Given Ibexa 4.6 has the same database schema as 3.3 (And makes the same DB Queries when performing counts) It is susceptible to the same issues present in 3.3 when querying large amounts of content.

For more details please read here where performance implications are described more clearly.
ezsystems/ezplatform-kernel#413

For QA:

Test using instructions given in ibexa/admin-ui#1605

Documentation:

Check ezsystems/ezplatform-kernel#413 for relevant details.

Notes

• This has been manually tested and confirmed to work on Ibexa 3.3 (And forward ported to v4.6.21)
• Tests Pass and have been updated with new test cases where they already existed

[adamwojs]

Thank you for valuable contribution @ryanolee! I've add some minor comments from my side and asked team to review PR.

[Steveb-p]

Makes sense from the functional standpoint. I was considering how those COUNT queries impact larger databases, and I'm happy to research this approach. Thanks for showing me this :)

I've given a few remarks that I would have in the following comments.

In general, I would love to have this trait converted into a class, and unit tests added for it (which should be easier once it's not a trait).
For 3.3 version, since it's in "maintenance mode", it will probably not be added there. It's supposed to be security fixes only.

As for the API changes, we might consider taking Symfony's approach and adding additional argument via func_get_arg() + comment. Depends on how likely it is that those services are reimplemented. In majority of cases a decorator exists (afaik) that should be used, but I don't want to break someone's application regardless of our API contract.

We'll discuss it internally probably at the start of next week.

[Steveb-p]

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 connection property (which is an implicit assumption not visible outside of trait).

Since the method added is using protected visibility, it is immediately available to descendants. While not a big deal, I would personally prefer it to use private visibility (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 Connection object, it could be passed as part of the method arguments.

WDYT?

[ryanolee]

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 connection present as seen with a few lines below where $queryBuilder->getConnection()->getDatabasePlatform()->getCountExpression('*') is used. But based on everything else that is a mute point.

[Steveb-p]

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 because COUNT() is ANSI SQL, and there were never any platform differences to begin with.

[ryanolee]

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::getCountExpression is 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 that

[Steveb-p]

It is updated in 5.x. I'd only ask to not use it here, as it will make merge up simpler :)

[ryanolee]

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

[ryanolee]

Makes sense from the functional standpoint. I was considering how those COUNT queries impact larger databases, and I'm happy to research this approach. Thanks for showing me this :)

I've given a few remarks that I would have in the following comments.

In general, I would love to have this trait converted into a class, and unit tests added for it (which should be easier once it's not a trait). For 3.3 version, since it's in "maintenance mode", it will probably not be added there. It's supposed to be security fixes only.

As for the API changes, we might consider taking Symfony's approach and adding additional argument via func_get_arg() + comment. Depends on how likely it is that those services are reimplemented. In majority of cases a decorator exists (afaik) that should be used, but I don't want to break someone's application regardless of our API contract.

We'll discuss it internally probably at the start of next week.

Thanks for the review 🤩

Just as a follow up for this the func_get_arg is somewhat problematic due to https://github.com/FriendsOfPHP/proxy-manager-lts dropping the extra arg in lazy loaded servies

See the following:
https://github.com/Ocramius/ProxyManager/blob/2.15.x/docs/lazy-loading-value-holder.md#known-limitations

In order to add BC break compatibility this PR might need pushing to main and the BC break be introduced to v5 where the major version change might allow for contracts I have confirmed the behavior where the value proxy drops the extra limit parameter.

The only other option I can think of is in 4.6 to have the admin UI ensure it uses instances of the Location and Content services that are either not lazy loaded or have back channels used specifically by the admin UI/ For instance directly call the underlying persistence handler for queries until v5 where the fix can be pulled up to the overall API . In either case this is really less than ideal 😓

It does work when the "lazy" option is removed from the inner service. But not making it lazy will likely have much more far widespread performance implications so likely needs to be weighed if the internal admin optimisations make sense with it. For now have removed the "lazy" from the location service to get this to work

Let me know what you think appropriate next steps are @adamwojs @Steveb-p as the Symfony BC break strategy will probably not work due reliably to the number of lazy loaded services in Ibexa core 🤔

The suggested changes as given above

• Trait to -> DI Class
• Symfony style contract changes have been made available
• Unit tests have been added to new class
• Tests pass and CS Runs

See: ibexa/admin-ui#1605 for the relevant places the queries have been changed

If you would like me to port this further to Ibexa version 5 please do considering the impact of these queries as shown in prior PR's can become significant.

[ryanolee]

Quality Gate failed

Failed conditions 3.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

1 liners required for BC breaks. Would make no sense to extract into it's own function and based on the func_* function requirements nigh impossible. Seems to be complaining about a single function call also?

[ryanolee]

All comments should be addressed 🙌

@adamwojs @Steveb-p
This should be ready for a re-review pending further discussion of BC break strategies for updating Contracts and the problems that come from it (especially surrounding problems with value proxies)

In line with this ibexa/admin-ui#1605 leverages these new changes to make the admin UI much faster for users with larger databases (Many sub items in commonly traveled paths)

Thanks for having a look an let me know if there is anything else you need from me with relation to this!

[Steveb-p]

All comments should be addressed 🙌

@adamwojs @Steveb-p This should be ready for a re-review pending further discussion of BC break strategies for updating Contracts and the problems that come from it (especially surrounding problems with value proxies)

In line with this ibexa/admin-ui#1605 leverages these new changes to make the admin UI much faster for users with larger databases (Many sub items in commonly traveled paths)

Thanks for having a look an let me know if there is anything else you need from me with relation to this!

I was thinking how to deal with proxies being generated without the right amount of arguments, and there is technically an option of asking the proxy generator to generate a proxy against a specific interface.

And while the original interface does not contain $limit argument, we could create a new, forward facing one, which would extend the original and add the missing argument (without actually introducing said interface to implementations, nor intending to use it in 5.x - just for the purposes of lazy).

Weird and going against common sense maybe, but it would solve the functional issue of proxy generation.

Regardless of approach, I'm pretty sure we will also introduce a rector rule to allow this new concept of $limit on count to be introduced in all implementations of a specific interface. ArgumentAdder already exists in Rector, so all that will be needed is to configure the rule properly.

[ryanolee]

Ah thanks for the feedback @Steveb-p that actually seems to work quite nicely.
I have stolen naming from the python's concept of [__future__](https://docs.python.org/3/library/__future__.html) given this seems to fill a similar role. Was looking for a way of controlling the underlying code-gen but could not not find one at a cursory glance. That all seems to be working now without any api bc breaks and lazy loaded value holders working as intended. Prefixed them with Future as to avoid polluting IDE intelisense with compatibility layers given no end user should really ever be aiming to use them. Or using then with caution and complete lack of guarantee if they do.

Code gen is also working as expected

 public function getSubtreeSize(\Ibexa\Contracts\Core\Repository\Values\Content\Location $location, ?int $limit = null) : int
    {
        $this->initializerdb498 && ($this->initializerdb498->__invoke($valueHolder69884, $this, 'getSubtreeSize', array('location' => $location, 'limit' => $limit), $this->initializerdb498) || 1) && $this->valueHolder69884 = $valueHolder69884;

        if ($this->valueHolder69884 === $returnValue = $this->valueHolder69884->getSubtreeSize($location, $limit)) {
            return $this;
        }

        return $returnValue;
    }

All working as expected. Thanks!

[ryanolee]

Ok should all be fixed up. Tests pass on PHP 7.4 and 8.1
PHP Stan really does not like the bc break stuff

Note: Using configuration file [PATH]/core/phpstan.neon.dist.
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------- ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line    LocationServiceTest.php                                                                                                                                                                                                     
 ------- ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
          Ignored error pattern #^Method Ibexa\\Tests\\Integration\\Core\\Repository\\LocationServiceTest\:\:testGetSubtreeSize\(\) should return Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Location but returns           
          Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Location\|null\.$# (return.type) in path /home/ryanolee/projects/personal/ibexa-new/core/tests/integration/Core/Repository/LocationServiceTest.php was not matched in  
          reported errors.                                                                                                                                                                                                            
          Ignored error pattern #^Parameter \#1 \$location of method Ibexa\\Contracts\\Core\\Repository\\LocationService\:\:getSubtreeSize\(\) expects Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Location,                 
          Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Location\|null given\.$# (argument.type) in path /home/ryanolee/projects/personal/ibexa-new/core/tests/integration/Core/Repository/LocationServiceTest.php was not     
          matched in reported errors.                                                                                                                                                                                                 
  :1757   Ignored error pattern #^Cannot access property \$id on Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Location\|null\.$# (property.nonObject) in path                                                                 
         [PATH]/core/tests/integration/Core/Repository/LocationServiceTest.php is expected to occur 11 times, but occurred only 9 times.                                                         
 ------- ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 


                                                                                                                        
 [ERROR] Found 3 errors                                                                                                 
                                                                                                                        
                                                                                      

Whittled down the errors to these. I don't have any IDE tooling for controlling ignore errors in the phpstan.dist.neon. Looks like you might need a php stan pro licence to properly configure it?

In either case spammed a bunch of @php-stan-ignore *'s in the relevant places and fixed the rest. If someone has better tooling to update the phpstan.dist.neon it would be much appreciated if they could make the relevant changes 🙏 .

[ryanolee]

Fixed remaining issues beyond the aforementioned @Steveb-p @adamwojs "php stan ignore" Issue mentioned above. Let me know if there is anything else you need from me for either PR 🙌

[Steveb-p]

Fixed remaining issues beyond the aforementioned @Steveb-p @adamwojs "php stan ignore" Issue mentioned above. Let me know if there is anything else you need from me for either PR 🙌

Don't want to keep you hanging there without a response 😅

We were considering if we want to pursue the "Symfony way" of introducing a new argument to primary API methods, or if we should instead add that argument immediately. This is somewhat due to the implicit expectation that ContentService and similar classes are not to be re-implemented in developer code.

We also have a few team members that wanted to review this properly themselves and see if they can come up with a different solution.

As for the PHPStan errors, we will address them once my colleagues are happy with the approach :)

The work you've done is awesome and I hope you give it a little bit more time? 🙇

[ryanolee]

Ahh certainly no rush @Steveb-p ! We have implemented https://github.com/cweagans/composer-patches to backport this to our version of 3.3 so for us internally the problem is solved 🙌

Much appreciated once again for you looking into it. Can understand this has spiraled a bit from just being a performance fix to setting some standards on how Contract level BC breaks are handled when doing minor semver releases, and extending that to various other parts of the project from a process perspective.

If there is a better way of fixing this problem (Especially one that does not require surfacing this new "limit" argument to core contract API's) on the count queries (Like adding new methods / query constraints to the find method / off loading in problematic places to solr) internally please feel free to close this issue! Understand there are many different things backing this. So long as the underlying performance problems are fixed at some stage relating to COUNT queries I'm happy ⭐ .

[Steveb-p]

@ryanolee I've taken the liberty to update PHPStan baseline to check how the actual tests look.

As for the approach for PHPStan and ignoring errors, we decided to prefer in majority of cases putting errors into baseline file. Our reasoning is that this is a list of "issues" that PHPStan has detected that we intend to fix at some point.

We only use @phpstan-ignore and @phpstan-ignore-next-line if the issue reported is intentional. For example, if we pass an invalid value to a method in a test case to check if right exception is generated.

Easy way to regenerate baseline is by calling:

composer run phpstan -- --generate-baseline

(note the additional -- which instructs composer that the following options are for the script itself, not for composer)

[ryanolee]

Thanks for the reply @Steveb-p. Test should hopefully pass this time. The seem to work fine locally 🤞

[sonarqubecloud]

Quality Gate failed

Failed conditions
4.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

[ryanolee]

Ok all tests seem to be passing locally now so hopefully should be the last pass on this. https://gist.github.com/ryanolee/8129399f45cd0e803b9c87967758f8c8 all the tests pass as expected using https://github.com/nektos/act (The ones that will run successfully in there) @Steveb-p 🫠

[ryanolee]

Woo all tests pass but the sonar cube one. (Which I assume just needs a manual exclusion adding) Let me know if you need anything else from me @Steveb-p 👏

[ryanolee]

Any further progress on this @Steveb-p or likely going to wait until Ibexa 5 is out 🤔 ?

[adamwojs]

@ryanolee Sorry for delay, as you noticed we are currently occupied with v5 release. I will back to you as soon as possible.