Facet conditions not added to SearchApiCachePluginTrait::generateResultsKey causing caching issue for AJAX view

The preExecute() seems to decrease performance, I wonder if we can just include facets array from request.

Fix for notice:

Exception: Notice: Undefined property: Drupal\search_api\Plugin\views\cache\SearchApiTimeCache::$requestStack
Drupal\search_api\Plugin\views\cache\SearchApiTimeCache->generateResultsKey()() (Line: 199)

+++ b/src/Plugin/views/cache/SearchApiCachePluginTrait.php
@@ -197,6 +212,13 @@ trait SearchApiCachePluginTrait {
+      $request_stack = $this->getRequestStack();
+      $facets = $request_stack->getCurrentRequest()->request->get('f');

The f query parameter is configurable per Facet source, so this will not always return the correct data.

Possible solutions:
1. Use the Facets services to collect the active facets
2. Allow Facets to hook into the key data

Faced with an additional issue that exposed input didn't get in the results key and it causes weird behavior sometimes.
Updated patch with that.

Concerning #10, I have no solution for now.

@ seanB: Thanks for reporting this issue and already providing a great patch!
Also, sorry for being so slow to respond!
Ideally, with a bug this complex (in other words: concerning both Views and caching), which might easily pop back up due to future changes, we include a regression test with the patch. Would you be able to work on something like that?

Also, you are saying that this can only be reproduced with AJAX views, and normal views work correctly, right? Any idea why that is? (Might give us a clue for a better solution.)

That being said, your patch already looks pretty good, the change seems to make sense to me. I just adapted it a little so that we don’t include the query twice in the cache key – I think that makes sense? Please test/review!

@ kiseleva.t: Thank you, too, for your work on this!
The problem, really, is that Views and caching is such an insanely complex matter that I’m hesitant to make any larger changes, as they are almost certain to break something else. However, looking into why that preExecute() call was there in the first place, I found #2824640: Views cached results are not taking full cacheability metadata into account where it seems like we explicitly added a way for Facets to change the view’s cache contexts – even though it doesn’t seem like Facets made use of that afterwards.

So maybe that is the problem? Maybe Facets just needs to add the appropriate cache context to make this work correctly again?
Would be great if one of you could also try out the attached Facets patch and see if that works correctly (in case f is the facets query parameter).

In any case, I do think that preExecute() call is needed, even though it kinda seems like we don’t (completely) use the resulting query then. Maybe fixing this again with seanB’s suggested patch would be the right coure of action, or maybe code modifying the query would just need to also take care to modify the caching data (in case they need any external information).
Anyways, just adding the Facets data explicitly there would make it pretty obvious that we’d need to do the same for other, similar modules. And adding support code for specific other modules to our own internal code is generally not a good practice.

(Side note: The original preExecute() change was actually earlier, in #2624472-18: Make sure that cache tags of index and server are added to search result pages, where $query was removed from the $key_data array, but Joris apparently forgot to remove the (now useless) $query->preExecute() call. All very confusing.)

In summary: I’m confused, not sure what we should do and happy about any input!
(And more and more sure that we need test coverage for this!)

NW for the tests, at the very least.

Hi @drunken-monkey I've recently started to work on #2939710: Add support for "Search API (tags based)" caching in Views ticket and bummed into current issue
\Drupal\Tests\facets\Functional\IntegrationTest::testAndOrFacet()
Here are the steps how it can be reproduced in a very easy way:

1. Add server and index - ANY
2. Add content to this index - ANY
3. Add search view page for this index and enable cache plugins: search_api_cache plugin.
4. Create facet and block for it with "AND" operator.
5. Navigate to the search result view and apply facet.

After that Search API will store cached result - which is good.

6. Navigate back to the facet settings and change operator to "OR".
7. Refresh views view page.

Actual result: You wouldn't notice any changes there as it uses old query results.
Expected result: Search query will be invalidated and executed with new conjunction/operator.

This is true what you've told: facet module doesn't inject any cache or context dependencies into a query, thus it almost impossible to come up with a nice algorithm for generateResultsKey method. However I can see several ways how to fix this issue:
1. Apply patch with all query string as buildInfo e.g. work that have been done in #12.
2. As query is an instance of RefinableCacheableDependencyInterface, somehow use its cache metadata for the cache or allow to add additional cache keys to the query.
3. Facets should expire corresponding views view cache whenever values like "operator" is changed.

Unfortunately your POC doesn't work as conjunction isn't used in the URL.

After digging in second option, I found out that there is mechanism to add cache tags information to the query via alter, but it will not be used in a views results cache until view will be resaved. That seems to be wrong: because any module that implements this hook or event listener needs to add installation instructions to load all views and save it, and seems to overkill.
Is there any particular reason why query cache tag information is added in \Drupal\search_api\Plugin\views\cache\SearchApiTagCache::alterCacheMetadata() (which is executed only on when View::presave()) and not "on the fly" in \Drupal\search_api\Plugin\views\cache\SearchApiTagCache::getCacheTags() or in Drupal\search_api\Plugin\views\cache\SearchApiCachePluginTrait::cacheSet like

    $cache_tags = Cache::mergeTags(
      $this->getCacheTags(),
      $this->getQuery()->getSearchApiQuery()->getCacheTags()
    );
    $this->getCacheBackend()
      ->set($this->generateResultsKey(), $data, $expire, $cache_tags);

Now I get it, there should be only one central place to get cachebility information from a view that consists of multiple things:
- query
- render
- weather
... etc.

And by implementing alterCacheMetadata module passes query cache information back to the view object for further usage and let other module do not take care of gathering this information from different places. The only downside here is views view resaving process which isn't obvious.

Addressed second approach from #14 tests provided as well.
However it will not address initial issue. Facets module strongly recommends to disable caching, and work on this is in progress https://www.drupal.org/project/facets/issues/2939710#comment-14473247
In this patch I've addressed few minor issues regarding cacheabiity that I've found working on facet caching.

OOPS, code sniffer fixes.

One thing that is keep bothering me search_api_time plugin that doesn't let to inject cache metadata like search_api_tag does. This meta information is very important for instance it will let to vary search results depending on url, which is for the facets module is must. Can this be implemented or it was done by intention?

Hide patch with PHPCS errors.
Addressing context issue for search_api_time cache plugin from previous comment.

Thanks a lot for your research and your work, great job!

That you have to resave the view to have the change in cacheability metadata take effect seems really like a design flaw – who would think of that? I’d also hesitate to implement this automatically (that is, automatically resave the view when editing facets, for example) as I wouldn’t be confident enough that that didn’t have any unintended side effects. In a system as complex as Views, I wouldn’t count on just loading and resaving a view, without changes, always worked as it should. (Especially as we see right here that this can change the export, in some circumstances.)

So, probably we’ll have to add some additional information to users, maybe as a status message or requirement check in the Facets module, to make sure that users are aware they might need to resave views after editing their facets. Anyways, I think all of this just affects the Facets module, not this one – we might want to create a follow-up there. (Having it be part of #2939710: Add support for "Search API (tags based)" caching in Views would probably overload that issue.)

Regarding your patch, the changes seem to make sense to me (though I’m unfortunately not an expert). It’s unclear why we would want to add the query’s cacheability metadata just for the tag-based cache plugin, not the time-based one. Also, your explanations regarding the different metadata for the different displays also makes sense, and even come with test coverage, so those changes also seem reasonable.

In summary, this patch seems like a step in the right direction, even though further work will be needed in the Facets module.
I just have a few suggestions regarding code style and grammar – please see the attached patch and tell me, whether you’re OK with my changes and whether it still works for you.

Feedback from others would be very much welcome, too!

(Also, removing the duplicate issue reference – having the reference in one direction is sufficient.)

Thanks, for the review, and code refactoring, looks good to me!
Regrading Facets implementation:
- corresponding view display needs to be resaved only once when new facet has been added, to add its cacheabilty metadata into the view, which make sense because facets are altering view query.
- Right now facets module checks what kind of cache plugin is used by the view. If it is set to something different than "none" it will add a warning and change a view cache plugin to none. So view "resave" isn't something new that has to be introduced in the code, it isn't a great approach but still....

Good to hear, thanks for the feedback! (And sorry for the long delay.)
Pity that no-one else reported back whether this new approach works for them, but we’ll just have to hope for the best.
Committed. Thanks again!

If you agree that the Facets module should have a warning message or requirements check that tells you to re-save the view after changing the Facets configuration, it would be great if you could create a ticket for that. You know more about this than me, so can probably explain better.

@ others: If the committed patch doesn’t fix the problem for you (after re-saving the view), please either re-open the issue (drop me a line if you can’t) or create a new one.