Issue with modifying conditionGroup via preExecute event/hook

Agree that there is a bug/regression here, took me a quite a long time to track down.

In my case, I was testing with a block ( as we embed the search through block_field in content), and the behavior is different when using ?keywords=Foo vs. using ajax-based submission when pressing enter. in my test, the problem is that the lower-casing didn't happen anymore and then it didn't find any results when searching with an upper case string.

Testing this fix now.

Ah, misunderstood the fix. I think this should happen transparently, possibly when changing the keys and maybe other conditions too? Of course the cache results would be wrong then, which is also not good.

This works for me, but it might not work for you, not sure.

Patch at #6 worked for me.

In my case, we are embedding the exposed filters block into the header of every page. Presumably, the $view->preExecute() is being called when the exposed filters block is built and it's not being called again for the actual execution. As a result, the preprocess query processors are not being executed since there appears to be no keys. I couldn't figure out why the keys value was empty, but I'm guessing it has something to do with preExecute() not being executed a second time in \Drupal\search_api\Query.

Thanks a lot for reporting this problem and for your detailed analysis!
Well, when you mix complex systems like Views and caching, it’s little wonder there are unforeseen consequences …

My first approach would be to just not call preExecute() anymore in the getCache*() methods. That should resolve the problem immediately, in all its variations. (See the attached patch.)
Of course, as there was a reason this call was added, even though it sounds more like a theoretical problem judging from pfrenssen’s comments in #2824640-77: Views cached results are not taking full cacheability metadata into account. So, we might cause other problems by doing this – but at least not a regression compared to pre-#2824640.
Would be interesting if any modules (especially Facets) have already implemented something to provide caching information for their use case via query preprocessing. (At a glance, doesn’t seem that way, at least for Facets.)
If not, I guess we can just remove this call again and, if the need arises, think of something different to solve that use case. (E.g., a new processor method just for this, or a new event.) In any case, this should resolve this regression with minimal complication.

Allowing preExecute() to be called twice on the same query object seems to be a rather large change of the current contract, which I’d shy away from. No way of telling what problems that could cause.

OK, this fails exactly the test that we put in place to ensure this keeps working. Makes sense.
However, for this use case, just altering the view when it is saved would be sufficient, right? The whole process of adding cache metadata to a view by putting it on the search query during a fake pre-execute seems less than ideal anyways.

But any input/feedback would be very welcome!

I'm afraid this last patch is going to break some things. This test is ensuring that the Search API Query can have its cacheability metadata altered through hook_search_api_query_alter().

The test is not adding the cacheability metadata to the View. It is adding it to the Query. Having the correct metadata available on the Query benefits not on Views, but any consumer of the cacheability metadata. For example if a site is displaying the output of a query in a custom block or page controller they have to inherit the metadata from the query, or it will not cache correctly.

The use case that will break for us is the Facets module. Facets are dynamically altered into the query and their cacheability metadata can depend on the options that are chosen in the facets UI.

Here is an example: we have a search facet that allows users to filter the results to show only the content that they have authored themselves (ref. MyContentArbitraryFacet). For this to be cached correctly we need to add the cache context that varies by the content author to the query.

I don't have time right now to dig into this deeper to investigate a possible other solution, but I will make a ticket for this and ask to get some time in the next sprint to work on this.

I think it is possible that the general idea of the patch is correct and we should not call preExecute() there but then we also have to prevent these methods from being called if the cacheability metadata is not available yet. Possibly Views is collecting the cacheability metadata too early.

Thanks a lot for weighing in here. I hate dealing with both Views and caching, so any help/input is very welcome.

I think the proper solution for the Facets module (if we go with my currently proposed solution) would be to implement hook_view_presave() to add the necessary cache metadata to the view. Ugly and complicated, I know – but calling preExecute() at such an early stage, as we see here, just won’t work. And if Views is requesting the cache metadata this early, we can’t change that.

However, see my comment #10 – I didn’t actually find any code adding cache metadata to the query in the Facets module. Can you point me to the code in question? Or is this still theoretical? (I also didn’t find an event subscriber in your joinup profile.)

From how I understand things, what should ideally happen is that Views first builds the query to the point where it’s ready to be executed and only then requests the caching information. (Query caching isn’t supported anyways, other than theoretically, so there will always be a completely built query object for retrieving the cache metadata.) But Views doesn’t seem to do that, trying to retrieve cache metadata way too early in the process.
Or maybe we should just take care of this ourselves, triggering a query build, if not built already, in the \Drupal\search_api\Plugin\views\query\SearchApiQuery::getCache*() methods? It doesn’t seem right, but could that be the solution?
The interesting question here is under which circumstances the Views query plugins’ getCache*() methods are called. (And whether there is a good reason for retrieving cache metadata without building the query.)

Another possible solution would be to add a more complex system for adding cache metadata to a query – like preExecute(), but just for cache metadata. Then we could just do that instead of preExecute() when the getCache*() methods get called and not mess with the actual query execution that might come only later.
Would need its own event and processor method (maybe even stage), though, so a pretty complex addition. (Or maybe instead of adding a new processor method, we could just check for processors that implement CacheableDependencyInterface?)

Just brainstorming here, but maybe we can come to a good solution here after all.
However, unless anything will actually break, my first step is probably still going to be removing this preExecute() call. From what I’ve seen so far, I’m pretty convinced it can’t stay there no matter which solution we find, and it’s clear that the way it currently is breaks way more than facet caching in edge cases.

I think the proper solution for the Facets module (if we go with my currently proposed solution) would be to implement hook_view_presave() to add the necessary cache metadata to the view. Ugly and complicated, I know – but calling preExecute() at such an early stage, as we see here, just won’t work.

I had a quick look at our code base and this could work for us, but only because at this time we are not setting anything dynamically. We are only setting cache contexts which are fixed per facet type, and these could be saved in the view. For example the facet linked above sets a cache context that varies per user. However this doesn't seem like a correct solution.

And if Views is requesting the cache metadata this early, we can’t change that.

Not in Search API, but we can change it in Views. I need to investigate this, but my gut feeling says that this is the correct solution. Views should allow to set or alter the cache metadata at the moment that modules had the opportunity to assemble all necessary information, right up to just before executing the query. We cannot depend on the query results itself for the cache metad

However, see my comment #10 – I didn’t actually find any code adding cache metadata to the query in the Facets module. Can you point me to the code in question? Or is this still theoretical? (I also didn’t find an event subscriber in your joinup profile.)

It happens here, in DefaultFacetManager::alterQuery(). The Facets plugin manager passes the Search API query object to the facet plugins so they can alter it to add conditions and the cache metadata that depend on the conditions:

  public function alterQuery(&$query, $facetsource_id) {
    foreach ($this->getFacetsByFacetSourceId($facetsource_id) as $facet) {
      $query_type_plugin = $this->queryTypePluginManager->createInstance($facet->getQueryType(), ['query' => $query, 'facet' => $facet]); // <<<< here
      $query_type_plugin->execute();
    }
  }

The Facet plugins included in Facets itself don't provide cache metadata at the moment. The Facets documentation mentions that it is not possible to cache faceted search, but this is a misconception arising from the fact that they don't yet provide the necessary metadata. We have dozens of different faceted Search API views using our own custom Facet plugins and they are all cached correctly.

I will dig into this and try provide more information later today.

I investigated this deeper. My initial idea was wrong, Views is in fact not requesting the cache metadata early during normal operation. The metadata is only requested during rendering of the results, which is obviously after the results have been retrieved, and the preExecute() has already been called at that moment.

However there is one case where $this->preExecute() is not called, and this is during the saving of a view entity. @drunken monkey is on the right track in comment #15. The metadata is retrieved on a non-executed view in DisplayPluginBase::calculateCacheMetadata() and if we don't call pre-execute then our hooks don't fire and the metadata is not altered. This is probably also why we were calling $this->preExecute() during retrieval of the cache metadata.

What is really interesting in Views is that the cache contexts that are provided live by the cache plugins during query execution is disregarded, in DisplayPluginBase::applyDisplayCacheabilityMetadata():

    $cache = $this->getPlugin('cache');

    (new CacheableMetadata())
      ->setCacheTags(Cache::mergeTags($this->view->getCacheTags(), isset($this->display['cache_metadata']['tags']) ? $this->display['cache_metadata']['tags'] : [])) // <<<< reads cache tags indirectly from the cache plugin, OK
      ->setCacheContexts(isset($this->display['cache_metadata']['contexts']) ? $this->display['cache_metadata']['contexts'] : []) // <<<< ignores cache contexts from $cache?!?! NOT OK!
      ->setCacheMaxAge(Cache::mergeMaxAges($cache->getCacheMaxAge(), isset($this->display['cache_metadata']['max-age']) ? $this->display['cache_metadata']['max-age'] : Cache::PERMANENT)) // <<<< reads max age from $cache, OK

Luckily Views provides a different location where we can alter the cache metadata while saving the view. A cache plugin can implement ::alterCacheMetadata() and we can do the pre-execute here.

OK a green result, it seems to work! I will test this patch in my work project later this week and will provide feedback.

@dwinters @berdir @JonMcL can you try this patch and see if it solves the problem?

I created a MR in our project with the updated patch, I expect it will work. I believe #4 is a scenario that we should be able to relatively easily write a test for. Basically submit an ajax views (block) with e.g. upper/lowercase search query to make sure that the processors are applied.

Can confirm that this passed our tests and we've switched to the latest patch now.

@berdir thanks for testing!

I can also confirm my work project that uses cached Search API views has passed the tests with this patch.

Thanks a lot for investigating, pfrenssen! This now really looks like a good solution.
Thanks also to you and Berdir for verifying that this indeed solves the problem!

However, I tried to reproduce the original problem myself but failed – see the attached view. Anything I’m missing? Berdir, can you maybe elaborate on the steps to reproduce, or take a look at what could be different in my view? (I did try with several different cache plugins for the view – but it always worked fine.)

Also, and relatedly, it would be really great to have a regression test for this, to make sure we don’t break this again when trying to improve caching. (That’s also why I was setting up that test view.) Would one of you be willing to work on that? (I don’t think I managed to get JS tests running locally lately, so would have a hard time doing it myself. But if I manage to reproduce it, at least, I could try starting on it and see how it goes.)

NW for the tests.

@Berdir, I would be happy to work on a test. Could you give more details on how to reproduce #4?

I'm sorry I cannot be of assistance here, I have never been able to reproduce the reported problem myself. In my testing I never found a case where the cache metadata is requested before the query is executed. I arrived at the solution purely on theoretical grounds.

@dwinters @berdir @JonMcL Do you happen to know some more details on how to replicate this? Also a backtrace could be helpful to pinpoint the code path responsible.

Not sure what else to provide, other than a views ajax block and then compare passing in query parameters vs. an ajax based form submission. It's possible that something in our project further complicates it, I'll try to reproduce on a clean D8 site.

With the help of the explanation in #3173480: All Query preprocessors in search blocks disabled by new Query Cache functions added in 1.16 I was finally able to reproduce this, and consequently write a regression test. (Funnily enough, the important thing was to turn AJAX off – at least for calmforce and me.)
Please review, then we can finally resolve this!

Yay a failing test! This looks good to me, the test demonstrates that the block doesn't show up as expected, and the patch solves the bug. I can however not RTBC this since I wrote the fix for the issue.

Oops, sorry for the additional long delay, but looks all good now.
So: committed. Thanks again, everyone!