DDC-3174: Query Cache not correct working when using SQLFilter

[doctrinebot]

Jira issue originally created by user benno:

We have an SQLFilter to filter on entities with a specific Trait implemented. The filter is very easy:
bq. $res = $targetTableAlias . '.agency_id = ' . $this->getCurrentAgencyId();
On our system we have the query cache enabled, this works as long the "AgencyId" doesn't change. When the ID changes, the query cache seems to return the wrong (old cache) query.

[doctrinebot]

Comment created by @Ocramius:

I'm not sure if this case should be contemplated by the ORM. Filters are low-level and supposed to be stateless (services).

[doctrinebot]

Comment created by benno:

OK, we can disable the query cache for this case. But then should at least the documentation be updated, which explicitly mentions to use filter for locales, which are also not stateless: http://doctrine-orm.readthedocs.org/en/latest/reference/filters.html#example-filter-class

Also in the query cache chapter: http://doctrine-orm.readthedocs.org/en/latest/reference/caching.html#query-cache
bq. It is highly recommended that in a production environment you cache the transformation of a DQL query to its SQL counterpart. It doesn’t make sense to do this parsing multiple times as it doesn’t change unless you alter the DQL query.

[doctrinebot]

Comment created by @Ocramius:

[~benno] can you eventually provide a pull request?

[doctrinebot]

Comment created by jamesblizzardbrowserlondon:

I would just like to say that we're having exactly the same issue. I'd love some method (official or not) of having filters being taken into account in this situation.

[doctrinebot]

Comment created by telegu:

I have the same problem when is generated QueryCacheId It consider only the name of active filters and not the value of the filter
This is the code at line 646 of class \Doctrine\ORM\Query
protected function _getQueryCacheId()
{
ksort($this->_hints);

    return md5(
        $this->getDql() . var*export($this->*hints, true) .
        ($this->*em->hasFilters() ? $this->*em->getFilters()->getHash() : '') .
        '&firstResult=' . $this->*firstResult . '&maxResult=' . $this->*maxResults .
        '&hydrationMode='.$this->*hydrationMode.'DOCTRINE_QUERY_CACHE*SALT'
    );
}

[doctrinebot]

Comment created by odiaseo:

I also have the same issue, there are circumstances where filters are dynamic and not stateless particularly when dealing with multi-site / multi-lingual platforms. Is there an appetite for the ORM to take this into consideration.

[doctrinebot]

Comment created by bramstroker:

I have the same issue. We are using SQL filters a lot to filter entities by website and locale. It would be nice if the filter values can be taken into account as well. For now I disabled the query cache in the concerning repositories.

[doctrinebot]

Comment created by csolis:

Same issue here, we are using filters for soft deletion and it would be nice if we can use query cache.

[doctrinebot]

Comment created by tom.pryor:

We are currently working around this by naming the filter based on the value we apply in the filter. So in the agency_id example if we were filtering on an agency_id of 5 we'd name the filter something like 'agency_filter_5'. Would be good if Doctrine took into account the parameter values of the filters when generating the cache id though.

[PastisD]

Hello,

Old issue, I know, but I have the same issue. For now i just disabled the query cache.
Any news about that ? :)

[EmilePerron]

Unless I misunderstood the issue, I believe this has been fixed.

Doctrine takes into account the parameters that are passed to the filter when the query cache is enabled. However, you have to pass those parameters via the filter's setParameter() method, otherwise it will not work. Below is a working example:

# Enable the Doctrine SQL filter for entity locales
$filter = $this->em->getFilters()->enable('localeFilter');
$filter->setParameter('locale', $request->getLocale());

Then, in your filter constraint, you can get the value with the getParameter() method, as such:

public function addFilterConstraint(ClassMetadata $targetEntity, $targetTableAlias)
{
    $locale = $this->getParameter('locale');

    return sprintf('%1$s.locale = %2$s OR %1$s.locale = "" OR %1$s.locale IS NULL', $targetTableAlias, $locale);
}

[DamienHarper]

@beberlei is query cache really usable with doctrine filters as of now (think also about softdeletable filter with no parameter) ?
What about doctrine 3: will filters be available and resulting queries cacheable?

[SenseException]

@DamienHarper Does the example of @EmilePerron not work?

[DamienHarper]

@SenseException No it still doesn't work for me.

[beberlei]

The fix here is to introduce a new final method getHash on SQLFilter that generates a hash based on the called class name and all the parameters, then modify FilterCollection::getHash() to use that.

Tricky bit is probably, how to handle case where parameter is an object, because serializing that would be expensive, and since its part of the query cache, must be stable across multiple requests.

[oojacoboo]

Geez, this was a fun bug to run into. Luckily it hasn't been too dangerous, but could have been much worse, executing queries on wrong records.

Why is addFilterConstraint returning the correct SQL WHERE clause, yet Doctrine is using a different set from the query cache? I stepped through the execution for most of this and it's absolutely clear the filter is returning the correct SQL, but the final composed SQL statement includes another ID value which happens to be the one set after clearing the cache, aka from the first cache warming. Also, disabling the query cache resolves the issue.

We're just going to have to leave query cache disabled until this issue is resolved. Any other insights into this would be appreciated.

[FabienPapet]

I ran into the same issue, looks like disabling cache is the current way to fix this.

My problem was while adding a filter to add and user.company= :company depending on a user's company.

Here's how to reproduce the problem on a Symfony 5.1 with ApiPlatform v2.4

<?php

namespace App\Doctrine\Filter;

use App\Doctrine\Annotation\CompanyAware;
use App\Entity\User;
use Doctrine\Common\Annotations\Reader;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Query\Filter\SQLFilter;
use Symfony\Component\Security\Core\Security;

class CompanyFilter extends SQLFilter
{
    private ?Reader $reader = null;
    private ?Security $security = null;

    public function addFilterConstraint(ClassMetadata $targetEntity, $targetTableAlias): string
    {
        $companyAware = $this->reader->getClassAnnotation($targetEntity->getReflectionClass(), CompanyAware::class);
        if (!$companyAware) {
            return '';
        }

        /** @var User $user */
        $user = $this->security->getUser();

        return sprintf('%s.company_id = %s', $targetTableAlias, $user->getCompany()->getId());
    }

    public function setSecurity(Security $security): void
    {
        $this->security = $security;
    }

    public function setAnnotationReader(Reader $reader): void
    {
        $this->reader = $reader;
    }
}

class CompanyFilterConfigurator
{
    private EntityManagerInterface $em;
    private Reader $reader;
    private Security $security;

    public function __construct(EntityManagerInterface $em, Security $security, Reader $reader)
    {
        $this->em = $em;
        $this->reader = $reader;
        $this->security = $security;
    }

    public function onKernelRequest(): void
    {
        /** @var CompanyFilter $filter */
        $filter = $this->em->getFilters()->enable('company_filter');
        $filter->setAnnotationReader($this->reader);
        $filter->setSecurity($this->security);
    }
}

doctrine configuration :

doctrine:
    orm:
        auto_generate_proxy_classes: false
        metadata_cache_driver:
            type: service
            id: doctrine.system_cache_provider
        query_cache_driver:
            type: service
            id: doctrine.system_cache_provider
        result_cache_driver:
            type: service
            id: doctrine.result_cache_provider

services:
    doctrine.result_cache_provider:
        class: Symfony\Component\Cache\DoctrineProvider
        public: false
        arguments:
            - '@doctrine.result_cache_pool'
    doctrine.system_cache_provider:
        class: Symfony\Component\Cache\DoctrineProvider
        public: false
        arguments:
            - '@doctrine.system_cache_pool'

framework:
    cache:
        pools:
            doctrine.result_cache_pool:
                adapter: cache.app
            doctrine.system_cache_pool:
                adapter: cache.system

[beberlei]

@oojacoboo @FabienPapet see my last comment please, it explains what the problem is and it details how this can be fixed. It should be relatively simple if you want to give it a try.

[Th-julien]

Hello,
Any news ? Can I help ?

[mpdude]

Remarks @beberlei regarding the suggestion in #3955 (comment):

1. Remark

Here is how the query cache ID is calculated for Query:

orm/lib/Doctrine/ORM/Query.php

Lines 792 to 803 in 152c04c

	protected function getQueryCacheId(): string
	{
	ksort($this->_hints);
	return md5(
	$this->getDQL() . serialize($this->_hints) .
	'&platform=' . get_debug_type($this->getEntityManager()->getConnection()->getDatabasePlatform()) .
	($this->_em->hasFilters() ? $this->_em->getFilters()->getHash() : '') .
	'&firstResult=' . $this->firstResult . '&maxResult=' . $this->maxResults .
	'&hydrationMode=' . $this->_hydrationMode . '&types=' . serialize($this->parsedTypes) . 'DOCTRINE_QUERY_CACHE_SALT'
	);
	}

In that part, parameters (from AbstractQuery::$parameters) are not taken into account. To my understanding, that's for a good reason: When we deal with the same query repeatedly and only a parameter value changes, we want to re-use the same cache entry. DQL->SQL translation does not depend on the parameter value, which is bound only later on when the query is executed.

The parameters used for filters are from a different set, namely SQLFilter::$parameters (which is an independent, per-filter array). But when we consider them when computing the per-filter hash, they will effectively end up in the the query cache ID hash as well.

That is – for every different parameter value combination set for filters, different query cache entries will be used. When the filter is something like a soft delete with only two possible parameter values (on/off), that might not be a big deal. When it does something like filtering by tenantId in a multi-tenancy app, it might bloat the query cache and/or affect cache efficiency (but, at least, providing correct results).

Probably there is no easy way to fix this, since the filter returns "raw" SQL and has no way of adding parameters for the final query execution. Would require more investigation to find out how/if parameters could be returned from the filter and passed on into the Executor.

By the way: Why does Query::getQueryCacheId() look at all filters instead of calling getEnabledFilters() before like e. g. the SQLWalker does here? The getHash() method iterates over enabled filters only. (Thanks @MalteWunsch!)

2. Remark

If you have a look at filter implementations like SoftDeletable, you'll see that this filter generates SQL based on data obtained at run-time through a listener. No parameters are in use in that case. They added a setParameter() call to bust the query cache, but that's a rather recent fix and seems a bit like a workaround.

So, maybe the suggested SQLFilter::getHash() method should not be final, but instead be just a default implementation considering all parameters. Implementing filter subclasses may need to provide their own implementation, and this should probably be pointed out in the documentation as well. (#9522 for the code change)

[mpdude]

@MalteWunsch just pointed out to me that FilterCollection::getHash() will effectively cast filters to strings when computing the hash:

orm/lib/Doctrine/ORM/Query/FilterCollection.php

Lines 193 to 195 in 152c04c

	foreach ($this->enabledFilters as $name => $filter) {
	$filterHash .= $name . $filter;
	}

Since SQLFilter implements __toString by serializing its ::$parameters...

orm/lib/Doctrine/ORM/Query/Filter/SQLFilter.php

Lines 178 to 181 in 152c04c

	final public function __toString()
	{
	return serialize($this->parameters);
	}

... the solution described by @beberlei should effectively be in place for a long time already?