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

Prefer order filter on canonical field #1171

Closed
tseho opened this issue Jun 10, 2017 · 9 comments
Closed

Prefer order filter on canonical field #1171

tseho opened this issue Jun 10, 2017 · 9 comments

Comments

@tseho
Copy link
Contributor

tseho commented Jun 10, 2017

When using the Doctrine ORM OrderFilter, I had a case when I had to sort on a canonical version of a field for valid results. (And, I hope, faster queries with proper indexes)

I created a custom version of OrderFilter, checking if the sorted field has a canonical column (eg: name => name_canonical) and uses it in the orderBy instead of the normal one.

It is something you would accept if I do a PR ?

@tseho tseho changed the title Prefer filter on canonical field Prefer order filter on canonical field Jun 10, 2017
@teohhanhui
Copy link
Contributor

You may write a custom filter which extends the OrderFilter. Specifically, you only have to override the filterProperty method.

@tseho
Copy link
Contributor Author

tseho commented Jun 28, 2017

@teohhanhui Thanks for the reply.

I already did that, I was wondering if this something we could add to api-platform/core.
For example, we could use ApiProperty to specify the canonical version of a property, to be used in OrderFilter.

@Simperfit
Copy link
Contributor

I don't mind if we implement this feature, it can be useful IMHO.
@tseho do you want to implement it ?

@tseho
Copy link
Contributor Author

tseho commented Apr 9, 2018

@Simperfit Sure ! It's a funny coincidence, I finally upgraded my app last week to use the ApiFilter annotation everywhere and I updated the way my CanonicalOrderFilter works.

The resulting syntax is the following:

    /**
     * @ORM\Column(type="string")
     * @ApiFilter(CanonicalOrderFilter::class)
     */
    protected $email;

    /**
     * @ORM\Column(type="string")
     */
    protected $emailCanonical;

I will make a PR tonight so you can check it.

@Simperfit
Copy link
Contributor

@tseho That's one of my ability, if you are a french people, I can read though your mind just by reading the issue :p.

@soyuka
Copy link
Member

soyuka commented Apr 9, 2018

Mhh looks to me that this is a specific use case. I don't see a fit in the core though.

@tseho
Copy link
Contributor Author

tseho commented Apr 9, 2018

@soyuka It's not really my call :)
Canonical fields are not that rare, there are some in FOSUserBundle. If this feature shouldn't be part of the core, I don't mind providing an example and some documentation on how to do it.

@Simperfit I have been running more tests and found out that having the CanonicalOrderFilter on a field is against the spirit of OrderFilter because you cannot control the order on which the filters are applied.

I think a proper solution would be to add a new option:

/**
 * @ApiResource
 * @ApiFilter(OrderFilter::class, properties={"email": { "canonical": "emailCanonical", "default_direction": "ASC" }})
 */
class Offer
{
    // ...
}

The change to OrderFilter could be then something like this:

protected function filterProperty(string $property, $direction, QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, string $operationName = null)
{
    // ...

    if (null !== $canonical = $this->properties[$property]['canonical'] ?? null) {
        $queryBuilder->addOrderBy(sprintf('%s.%s', $alias, $canonical), $direction);
    }

    $queryBuilder->addOrderBy(sprintf('%s.%s', $alias, $field), $direction);
}

@tseho
Copy link
Contributor Author

tseho commented Apr 20, 2018

Hey @soyuka, @Simperfit, should I open a PR with my last proposal ?

@soyuka
Copy link
Member

soyuka commented Apr 7, 2019

probably related to #2090

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants