Strange Behavior of the Method useFetchJoinCollection in PaginationExtension

[jsamouh]

Hello,

I was doing tests with pagination in API platform.

The pagination FAILED for me. In several pages during my pager navigation, it displays sometimes an object already proposed in a previous page.

I tried to investigate the problem and apparently, it seems to come from the method useFetchJoinCollection developed in the class PaginationExtension.

This method, in a simple entity to paginate return true, even for a simple entity (no relation with another entity).
The result is that Doctrine apply a TreeWalker to DISTINCT the data and apply a LIMIT and OFFSET.

More, I do not understand the logic function

private function useFetchJoinCollection(QueryBuilder $queryBuilder): bool { return !QueryChecker::hasRootEntityWithCompositeIdentifier($queryBuilder, $this->managerRegistry); }

If the entity has a composite identifier, beacause of the "!", it returns false. Why Did you put a "!" ?
If I delete the "!" operator, it works, cause my Entity does not need to fetchJoinCollection.

Any suggestion?

[teohhanhui]

Please see the Doctrine issue linked in the PHPDoc: doctrine/orm#2910

By the way, $fetchJoinCollection defaults to true in the Doctrine Paginator. We're only disabling it when it causes problems.

[teohhanhui]

In any case, it'd be great if you can add a failing test case.

[jsamouh]

@teohhanhui : Thanks for the answer.
But for the example, I do not apply a composite primary key in my Entity

Are we supposed to put fetchJoinCollection to TRUE even my Request is a Simple SELECT without JOIN ?
That is happen in API Platform. For a Simple Request, fetchJoinCollection is set to TRUE.

[jsamouh]

@sroze , any idea ? :-)

[dunglas]

Hi @jordscream, thanks for finding and reporting this issue. Has you seem to have found a fix, do you see you can provide a PR (targetting the 2.0 branch)? Or at least a failing test to allow us to fix this?

[teohhanhui]

@jordscream:

Are we supposed to put fetchJoinCollection to TRUE even my Request is a Simple SELECT without JOIN ?

Yes, that is the default. Please open an issue on https://github.com/doctrine/doctrine2 if you've found a bug with the Doctrine Paginator.

[jsamouh]

After investigation:
doctrine/orm#5622 is the problem

MySQL 5.7 and postgresql x.x: (https://www.postgresql.org/docs/9.3/static/queries-limit.html)

When you create a request with limit/offset or distinct without an ORDER clause, the request will give inconsistent results.

The only solution is to put a default order in your request.

[dunglas]

Thanks for the report. Is it possible to patch Doctrine directly to fix this? Or at least API Platform?

[soyuka]

Thought we already had an ORDER clause by default, if not we can just add one.

[jsamouh]

@soyuka @dunglas , Thanks for your response.

As suggest @soyuka , we can effectively add a default order in API platform.

The question is:

• Where to put this code ?
• PaginationExtension of API Platform can be a good candidate? It is responsible class to prepare the query sending to DoctrineOrmPaginator.
• Developpement of a Method addDefaultOrder in PaginationExtension. Check if there is no order and consequently add it. Which field we use to put a default order ?

What do you think ?

Thanks

[soyuka]

Indeed, I'd put this in the PaginationExtension.

[jsamouh]

@soyuka , cool :-) , The method applyToCollection seems correct to add the code in it.

@dunglas : What do you think ? do you want a PR ? 2.0 ?

Thanks
Jordan

[soyuka]

@jordscream Yes as it's a bug fix you can PR on 2.0 :)

[teohhanhui]

We already have https://github.com/api-platform/core/blob/master/src/Bridge/Doctrine/Orm/Extension/OrderExtension.php

[soyuka]

Hmm okay I remembered correctly then, why isn't it being called in this use case? Maybe just a wrong priority in extensions?

…

On Thu, 9 Mar 2017 at 10:35, Teoh Han Hui ***@***.***> wrote: We already have https://github.com/api-platform/core/blob/master/src/Bridge/Doctrine/Orm/Extension/OrderExtension.php — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#965 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQr87Ga_J5FavO9daTPjcv2ZNpY12msks5rj8fJgaJpZM4MSA8k> .

[teohhanhui]

No, because the default is null. This should be a no-op because it's a Doctrine bug when using a newer version of MySQL (more standards-compliant).

[jsamouh]

@teohhanhui , @soyuka Good Point, OrderExtension seems right.

With the setting of parameter %api_platform.collection.order% to DESC or ASC for example, it applies a default order in ClassMetadata identifier.

Question is : Can we put this parameter to a default value (DESC for example) ?
Without this, any person who use API platform with Mysql Last version and Postgresql will have the problems

[teohhanhui]

Okay, that makes sense since the Doctrine Paginator bug should affect all standards-compliant DBMS.