In case of an exception thrown by the backend, Search API Cache plugins always cache an empty result.

The exception gets caught and the view is executed again with "abort" set to handle the errors: 

    catch (\Exception $e) {
      $this->abort($e->getMessage());
      // Recursion to get the same error behaviour as above.
      $this->execute($view);
    }

That triggers the Search API cache plugin selected for this view. This plugin just uses the cache configuration from the view and ignores the cache metadata set on the query.
According to #3133997: Avoid query caching in Search API when a Search API Solr Exception is thrown the backend should set max-age to 0 on the query before throwing an exception. But the current implementation. ignores that.

In general the cache plugin should respect the max-age any 3rd party module might have set on the query if that max-age is shorter than the configuration values.

CommentFileSizeAuthor
#9 3296006-9--exception_caching_test.patch2.66 KBdrunken monkey
#9 3296006-9--exception_caching_test--with_fix_reverted.patch5.27 KBdrunken monkey
#5 3296006-simplified.patch2.18 KBmkalkbrenner
#3 3296006.patch1.26 KBmkalkbrenner
#2 expire.patch1021 bytesmkalkbrenner

Comments

mkalkbrenner created an issue. See original summary.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Status: Active » Needs review
StatusFileSize
new expire.patch1021 bytes
mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
StatusFileSize
new 3296006.patch1.26 KB

Sorry, wrong patch. Here's the right one.

BTW if max-age is 0 no cache entry should be written at all.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

The approach here looks good, but I have some trouble understanding everything correctly.

  1. +++ b/src/Plugin/views/cache/SearchApiCachePluginTrait.php
@@ -124,14 +124,21 @@ public function cacheSet($type) {
+    if ($query_max_age !== Cache::PERMANENT && ($expire > $query_max_age || $expire === Cache::PERMANENT)){
+      $expire = $query_max_age;
     }

    this makes it really difficult to understand. Can we rewrite this? to make it clearer? 

  2. +++ b/src/Plugin/views/cache/SearchApiCachePluginTrait.php
@@ -124,14 +124,21 @@ public function cacheSet($type) {
+    if ($expire !== 0) {

    By reversing this, it might make it easier to read what is going on?

    if ($expire === 0) {
    return;
    }

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
StatusFileSize
new 3296006-simplified.patch2.18 KB

Here's a more readable and commented version.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

Looks a lot better.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Priority: Critical » Normal
Status: Reviewed & tested by the community » Active
Issue tags: +Needs tests

Looks great, thanks a lot, both of you!
Fixed a typo and committed.

As we already have a Views caching test (ViewsDisplayCachingTest), it would also be great to get test coverage for this fix. Would you mind writing one, Markus? (I chose to commit it without a test as you maintain the issue is urgent, but normally it should have test coverage. While at it, we could also add a test for #3295564: Views caching is broken with facets 2.0.4.)

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Active » Needs review
Issue tags: -Needs tests
StatusFileSize
new 3296006-9--exception_caching_test--with_fix_reverted.patch5.27 KB
new 3296006-9--exception_caching_test.patch2.66 KB

Finally got round to writing the test for this.

The last submitted patch, 9: 3296006-9--exception_caching_test--with_fix_reverted.patch, failed testing. View results

  • drunken monkey committed 30914cd0 on 8.x-1.x 
    Follow-up to #3296006 by drunken monkey: Added test for caching of...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Merged.
Thanks again, everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.