Core: Bug: use output walkers in Doctrine bridge

[CvekCoding]

API Platform version(s) affected: 2.5

Description
Wrong recognition whether the Doctrine ORM Paginator should use output walkers.
Previously (in APIP2.4) it worked fine, but after recent code changes it's broken.

How to reproduce
In my custom filter I use the following code:

$queryBuilder->addSelect(sprintf('CASE WHEN %s.%s = FALSE THEN 0 ELSE 1 END AS HIDDEN %s', $alias, 'showOnGrid', $nullRankHiddenField));

It's necessary to add this expression into OrderBy clause.

In QueryChecker::hasOrderByOnFetchJoinedToManyAssociation() there is a following logic:

            foreach (QueryBuilderHelper::traverseJoins($orderByAlias, $queryBuilder, $managerRegistry) as $alias => [$metadata, $association]) {
                if ($inToManyContext && \in_array($alias, $selectAliases, true)) {
                    return true;
                }

                if (null !== $association && $metadata->isCollectionValuedAssociation($association)) {
                    $inToManyContext = true;
                }
            }

As you can see, you check join name against selectAlias, but selectAlias is smth like "case ..." and the code above will never return true.

Possible Solution
As this moment I reverted back the previous version of this method.
Additional Context

[teohhanhui]

Actually, this is intended behaviour. Our guessing is best effort and we're not able to support all edge cases.

You can override it like this: https://api-platform.com/docs/core/pagination/#controlling-the-behavior-of-the-doctrine-orm-paginator

[CvekCoding]

@teohhanhui wow, I didnt see this before. Thanks.

[CvekCoding]

@teohhanhui anyway it's not clear for me how I can use pagination_use_output_walkers in filter.
Right now I use my custom filter, where I customize DB query the way, that I need to use output walkers. But your link contains example with custom query. Custom query resolver doesnt have an access to queryBuilder, so I cant customize query there.
And it looks to me that I cant set pagination_use_output_walkers right in custom filter too.

Or Im wrong?

[CvekCoding]

Of course I can set pagination_use_output_walkers in attributes in @ApiResource annotation, so it will be enabled for all queries by default (for this entity), but this is not an efficient way.

[teohhanhui]

Yes, you're right. You can only set this at the global / "defaults" level (if using 2.6 / master), the resource level, or the operation level.

It's difficult to customize this for each request / query because we cannot set hints on the QueryBuilder, only on Query.

[teohhanhui]

But your link contains example with custom query. Custom query resolver doesnt have an access to queryBuilder, so I cant customize query there.

Sorry, I don't get what you mean... Are you reading the right section of the documentation?

[CvekCoding]

@teohhanhui I read this http://joxi.ru/l2ZORBOTE8aqjA
There is an example how I can use output walkers on operation level.

So I think that if I could change context in my filter, this would solve my issue - in this case I can pass changed context into next extensions.
What if we create another abstract class AbstractReferenceContextAwareFilter where we will passing context by reference? This way I can add pagination_use_output_walkers into context and Paginator will get it properly.
WDYT?

[teohhanhui]

No, that's not a solution. We will not pass the $context by reference. That will be such a mess.

You can already set this attribute at the operation level.

[CvekCoding]

@teohhanhui only small part of requests use my custom filter, so using output walkers with every operation request is unefficient. This is why I thought that it will be good if I can set this option right in the filter.

[CvekCoding]

@teohhanhui I could create custom operation and setup output walkers on it. And then I can agree with front-devs that if they want to use my custom filter, they have to use this custom operation. And then in filter I can check that $operation is proper. Otherwise throw exception.

Do you think this is the best solution for me now?

[teohhanhui]

I'd suggest just setting the attribute at the operation level. You lose some performance but it's much better for maintainability.

[CvekCoding]

@teohhanhui here:
https://github.com/api-platform/core/blob/2e491f7c6a17066b09e86858ed96e2682b27a89c/src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php#L353-L355

You try to find attribure pagination_use_output_walkers in a list of REST operations. But in a case if we use graphql the operation is graphql operation, and therefore it cant be found. As a result pagination_use_output_walkers doesnt work for graphql.

[teohhanhui]

/cc @alanpoulain Thoughts?

[CvekCoding]

I created PR. If Im wrong with it - simply decline it:).