Views cached results are not taking full cacheability metadata into account

I guess this makes sense.

Oh god, and another reason using caching for search views is an aweful idea …
Good catch, thanks a lot for reporting!

However, are you sure normal access checks added to the query (e.g., by the "Content access" processor) will be accurately reflected in the cache key?
To me, it seems the cache key only uses $view->build_info, which (unless I'm mistaken) only contains the string representation of the query before preprocessing has run (and, thus, before conditions from processors have been added).

If so, if we really have to vary the cache by user, I guess we can forget about the whole thing for good, should just remove the plugin and try to document that caching and search views don't mix.

How do "normal" views deal with this? They should have the same problem, right?

Oh god, and another reason using caching for search views is an aweful idea

I don't agree, depending on what you are searching for it might be very useful to have caching. I think probably the only thing where caching is not useful is for doing like a full text search across highly dynamic content.

We are using search views for things like indexes of content which is known to only change very occasionally because they can only be changed by administrators.

I do not add arguments to the subject but I do have one issue. When trying to install some preset configuration files in an installation profile that among others had views configs with null query property, I received the following notice:

Trying to get property of non-object in Drupal\search_api\Plugin\views\cache\SearchApiCache->alterCacheMetadata() (line 194 of                                                      [notice]
modules/contrib/search_api/src/Plugin/views/cache/SearchApiCache.php). Drupal\search_api\Plugin\views\cache\SearchApiCache->alterCacheMetadata(Object) (Line: 2295)
Drupal\views\Plugin\views\display\DisplayPluginBase->calculateCacheMetadata() (Line: 326)
Drupal\views\Entity\View->addCacheMetadata() (Line: 300)

While this was just a notice in the installation, it's a blocker in, let's say, a BrowserTest. So I am adding one more check for the query before checking the query's property.

This also still needs a test.

Uploading a re-rolled patch cause the code related to the patch was moved to a trait.

Hi,

I don't understand why using the cache context user and not user.node_grants:view like the views that are not from search API? I attached a patch just in case but is it possible to explain?

In order to had tests, I'll be glad to help but I think I'll need guidance.

Also I have an idea (maybe a bad one I don't know) why not add a textarea to let the admin chose cache contexts? Do you prefer me to open a new issue for this? I propose this because I didn't succeed to change the cache context for Search API views.

I don't understand why using the cache context user and not user.node_grants:view like the views that are not from search API? I attached a patch just in case but is it possible to explain?

Yes: Since the searches don't necessarily only show nodes, we can't know if it's really only about node grants.
However, if the index has only node-based datasources enabled, then maybe we could make that improvement, yes. I don't feel I understand this well enough to decide whether that's really the case, though.

In order to had tests, I'll be glad to help but I think I'll need guidance.

We already have \Drupal\Tests\search_api\Kernel\ViewsDisplayCachingTest, so probably we'd just have to add some test data for access checks there and try with different users to see whether they receive results according to their permissions or whether the data is incorrectly cached.
But first we'd need to determine what the current and the correct behaviors even are.

Also I have an idea (maybe a bad one I don't know) why not add a textarea to let the admin chose cache contexts? Do you prefer me to open a new issue for this? I propose this because I didn't succeed to change the cache context for Search API views.

I don't think so, no. Ideally, we'd specify the correct cache context ourselves anyways. That's not happening at the moment (or in the near future), as far as I can see, true: but it's still too much of a niche and expert-level functionality for a UI option. Anyone knowing enough to know which cache contexts a view should have should also be competent enough to know, or find out, how to change them from some alter hook. (I, for example, know neither.)

Could we have this under attention?

Could we have this under attention?

I'm still waiting for answers to my question in #4:

However, are you sure normal access checks added to the query (e.g., by the "Content access" processor) will be accurately reflected in the cache key?
To me, it seems the cache key only uses $view->build_info, which (unless I'm mistaken) only contains the string representation of the query before preprocessing has run (and, thus, before conditions from processors have been added).

Also, we should have test coverage for this – which would also go a long way for explaining why this is needed and to make sure it's now working correctly.

Finally:

+++ b/src/Plugin/views/cache/SearchApiCachePluginTrait.php
@@ -228,4 +229,16 @@ trait SearchApiCachePluginTrait {
+    if (isset($query_options['options']['skip_access']) && $query_options['options']['skip_access'] === FALSE) {

Since this option defaults to FALSE, I think you can just use empty() here.

Ran into this for a project at work. Let's start by trying to figure out an answer for those questions before we get to the a possible patch.

Are you sure normal access checks added to the query (e.g., by the "Content access" processor) will be accurately reflected in the cache key?
To me, it seems the cache key only uses $view->build_info, which (unless I'm mistaken) only contains the string representation of the query before preprocessing has run (and, thus, before conditions from processors have been added).

It looks like they aren't and that is seems to be the underlying problem here. I haven't checked this yet (and I seem to be really busy

If so, if we really have to vary the cache by user, I guess we can forget about the whole thing for good, should just remove the plugin and try to document that caching and search views don't mix.

Search API based views are very often used to display lists of data from different sources, it might be a non-intendend usecase of Search API, but it does happen (a lot). So we should try to get the caching to be as good as possible (and preferably, get it right).

Also, we should probably use user.permissions instead of user as a cache context?

I discussed this on slack with @nick_vh and at work with @svdhout, we figure that a definition of done of caching would be something like this:

When a view based on Search API has a fulltext search:
* Warn the user that caching will not be very useful and that it should use none instead (message on view save?)

When the view is not using entity rendering:
* Warn the user that caching will not be very useful and that it should use none instead (message on view save?).

In other cases:
* Add entity listing tags to the view
* Add user:permission context

I think this is a good summary of my discussion with Nick. I added this to the summary, so we know how to fix this issue.

It looks like they aren't and that is seems to be the underlying problem here. I haven't checked this yet (and I seem to be really busy

Obviously, yeah, if you couldn’t even finish the sentence. ;P

Search API based views are very often used to display lists of data from different sources, it might be a non-intendend usecase of Search API, but it does happen (a lot). So we should try to get the caching to be as good as possible (and preferably, get it right).

It is an intended use case, I’m quite glad people consider it useful enough for this as well. But when you need to cache everything per-user, I still don’t know how useful this is. (On the other hand, I guess normal views have exactly the same problem?) Having user.permissions instead would be much better, but I agree with sardara and don’t think this will be enough.

Anyways, a look at how normal views do this migth really help.

Regarding the message on saving the view: Maybe just have it as the description for the cache mode radios? We alter that anyways (and I think have some bug there), so might as well just add it there, imo.

I agree that
* cache context: user.permissions
* cache tag: entity list
is the most sensible default.

I have had lots of examples where this not correct or optimal though and suggest to point people to https://www.drupal.org/project/views_custom_cache_tag (and possibly extending it for custom cache context) for manual finetuning.

Alternatively, we can incorporate custom tags and contexts (and provide sensible defaults) in our cache plugin.

The Drupal cache API provides mechanisms to inform us exactly which cache contexts are used to determine if a user has access to an entity. We don't need to guess.

The main problem is that this information is presented by core but not leveraged internally in Search API. We are considering access to be either TRUE or FALSE, like it was in D7.

We should change the return type of DatasourceInterface::checkItemAccess() from a boolean to an AccessResult object. This object contains all the info we need.

@sardara is correct in his comment from #2824640-20: Views cached results are not taking full cacheability metadata into account:

user.permissions won't cover all cases I'm afraid, like additional checks done through hook_entity_access() that are not related to global permissions (og for example) or I think not even when you have "view own unpublished content" permission.

All of the cache contexts used by these modules are passed in AccessResult objects:

• hook_entity_access() implementations return an AccessResult object, and this should include the required cache contexts.
• Organic Groups has its own permission system (ref OgAccess), and it puts all contexts that are used in determining access on the AccessResult object.
• Something like the view own unpublished content indeed varies per user, and we can see that this cache context is added to the AccessResult set in NodeAccessControlHandler::checkAccess().

So I am thinking we should not just blindly add some contexts here, let's just ask core which ones are actually needed and add those. For this to happen we need to:

• Change the signature of DatasourceInterface::checkItemAccess() to return either a boolean or an AccessResultInterface object, depending on the (new) parameter $return_as_object = FALSE. I would already deprecate the usage of returning a boolean, and emit a deprecation notice if it is used. Moving forward we should not encourage the use of access results as a boolean anymore.
• Add some code to SearchApiQuery::addResults() to merge all access results from the access checks, and apply this to the result. We should take care to also apply the cacheability metadata from all the results that are excluded from the result set.
• Make sure the access cacheability metadata is included in the result.

You’re right, I guess that really is the root of the problem. We didn’t properly adopt the new D8 logic of things (that is: always preserve cache metadata!) and so are running into these kinds of things.
Thanks a lot for your comment and thorough analysis!
Unfortunately, just adding a $return_as_object = FALSE parameter will not work, as it can’t be left out by child classes, so would be an illegal API break. We’ll have to add new methods and deprecate the old ones – see the attached patch. (If we agree that this is the way forward, I’d change the issue metadata accordingly and write the change record to be able to link it.)

However, as I’m such a noob regarding caching, I’m not quite sure how this should now be added? (That is, how do we “apply this to the result”?)

1. In the patch above, I just added the cache contexts to the view (as the method was already there), but I don’t think those actually make it into the results cache key?
2. The query plugin has methods for getting cache metadata – do we just override those and add the accumulated cache metadata from the results? (Would have to be stored in a property in between.)
3. Or do we provide a seperate method on the query plugin for getting this cache metadata and use it in a new SearchApiCachePluginTrait::alterCacheMetadata() method, as proposed in the existing patches?
4. I guess we also want to preserve cache tags and max age? Would that work with the second option, too, or do we need the third one for that anyways? (It just seems more “proper” to me to have that metadata available somewhere and not just alter it onto the final thing.)

It would, in any event, be really, really great if someone a bit more versed in this could write a (failing) test for this so we have a way to verify that a) there’s a problem (though I believe all of you, of course) and b) we fixed it (the crucial part).
The test should probably just add some unusual cache contexts, tags and max age to the access results and then verify they all make it to … wherever they should end up. 🤔 (The cache object saved by the cache plugin, I guess? (But how would we verify cache context there, they aren’t preserved?))

That's good! First some notes on BC:
* Even core sometimes adds an optional parameter with a CR.
* Adding methods may wake other dragons (extend the interface or implement a second interface).
* Not respecting cache metadata will be a problem for any contrib component.

So i tend to call the API change a bugfix and add a CR. Pick your poison.

Then some notes on cacheability:
* Yes, in an ideal world cacheability is part of every data item. But even core-views does not do that, and looking into \Drupal\views\Plugin\views\query\Sql, cacheability metadata is calculated in the getCacheFoo methods implementing CacheableDependencyInterface. So we will have to decide if we implement CacheableDependencyInterface in ResultSet, or do the work in the SearchAPI Query or our Views query.
* In an ideal world we use helper methods to move and merge the full triple (Contexts, Tags, MaxAge). In the real world we may need some delegating boilerplate code in the getCacheFoo methods.
* (Scanning the views sql query code, i suspect that some unusual metadata like entity contexts have been forgotten, but not sure.)
* If we need a property to collect metadata, use BubbleableMetadata, which also has helper methods.
* We must not forget $entityType->getListCacheFoo() (think: adding a new node invalidates node lists).
* We should as long as possible carry the row cacheability with the row, and do the bubbling as late as possible. This way, doing drupal-side filtering will "just work".

My math prof always said "Care about history"... How more important this is for software.
FTR: See #2379741: Add Renderer::getCacheableRenderArray() to encapsulate which data is needed for caching a render array and have views use it and #2378789: Views output cache is broken for the origins of D8 views caching.

Hmm, further looking into search_api code, i'd prefer access checking and cacheability handling "lower". Curerently it is in \Drupal\search_api\Plugin\views\query\SearchApiQuery, but for reusability its code should go to the datasource / result set.

Here's a first shot at this. It looks to me like we must completely rework \Drupal\search_api\Plugin\views\cache\SearchApiTagCache (my comments there are some quick notes and not a complete todo).

Ups, forgot some comments. Bear with me that i just copy-paste-save them here:

@@ -85,17 +85,26 @@ class SearchApiTagCache extends Tag {
 
   /**
    * {@inheritdoc}
    */
   public function getRowId(ResultRow $row) {
+    // @fixme Reading \Drupal\views\Plugin\views\cache\CachePluginBase::getRowId,
+    // it becomes clear that this must return a cache key that varies e.g with
+    // row data and user permisisons.
+    // Oh: Otoh as we overrode generateResultsKey, it may not be used anymore.
     return $row->search_api_id;
   }
 
   /**
    * {@inheritdoc}
    */
   public function getRowCacheTags(ResultRow $row) {
+    // @fixme We should set $row->_entity & $row->_relationship_entities
+    // (does the latter even make sense for search api?)
+    // in \Drupal\search_api\Plugin\views\ResultRow,
+    // and rely on the parent method. This way a lot of other components
+    // find what they expect.
     $tags = [];
 
     foreach ($row->_relationship_objects as $objects) {
       /** @var \Drupal\Core\TypedData\ComplexDataInterface $object */
       foreach ($objects as $object) {

* Even core sometimes adds an optional parameter with a CR.

To an interface? Are you sure?
If so, could you provide an example?

* Adding methods may wake other dragons (extend the interface or implement a second interface).

Adding methods to interfaces (especially those that have a “standard implementation” which should be extended – DatasourcePluginBase in this case) is explicitly allowed by the D8 BC policy. And that makes sense: adding a method won’t break anything for you if you extend the plugin base class. (OK, unless you have the bad luck to also define a method with the same name in your class. But apparently Core is fine with that risk.)
However, if we add an optional parameter to an existing method, you might have overridden that method in your class, and that will instantly result in a warning.

* If we need a property to collect metadata, use BubbleableMetadata, which also has helper methods.

Yes, of course.

* We must not forget $entityType->getListCacheFoo() (think: adding a new node invalidates node lists).

I don’t think that’s necessary in our case, as the complete list of nodes is not relevant for us – we depend on whether those newly added nodes are actually listed, so "search_api_list:$index_id" is the cache tag to use here, unless I’m mistaken. And we already add that.

Otherwise, this looks like a rather large API change, but otherwise reasonable, and if that’s what’s needed to make this work (or is the best way to do it), then I’m fine with it.
However, it would be really good if others, especially ones who know a bit about Views and caching, could weigh in here.
Also, we still need those tests, to be confident the changes at least do what they should.

> [New optional parameters] To an interface? Are you sure?

Rethinking and searching, i was in error. Only functions and final class methods got that.

> I don’t think that’s necessary in our case, as the complete list of nodes is not relevant for us – we depend on whether those newly added nodes are actually listed, so "search_api_list:$index_id" is the cache tag to use here, unless I’m mistaken. And we already add that.

Aah, so...
* instead of adding all list-cache-tags of all entitity-types in the index, (which i thought)
* we add the list-cache-tag if the *index* and invalidate that when one of the entity-type list cache tags gets invalidated (do we?)

OK i got that. Do we gain something by that indirection? (Just for interest...)

In any case, if we do that indirection, we also *must* care for entity-type list cache *contexts*.
(Rule of thumb: Always do the full package cache tags/contexts/mag-age (the latter not having a list counterpart afaict).)
Think:
* Add domain (or group) access (which adds domain list context to entitytype node)
* View an empty indexed node list on domain1
* If there is no domain context on the list, it will falsly be cached and displayed on domain2.

I'm going to work on this today. I had a quick look at the patch and I would like to rename the new methods from checkItemAccessAsObject() to getItemAccessResult(). I think this is a more future proof name.

I also will try to make a start on test coverage.

Created draft change record: https://www.drupal.org/node/3051902

• Changed instances of checkItemAccessAsObject() to getItemAccessResult() since in future versions there won't be the distinction of checking access as an object or as a boolean.
• Linked to the draft change record in deprecation messages.
• Changed instances of getItemCacheability() to getItemCacheableMetadata() because this is consistent with how similar methods are named in core.
• Completed method documentation.

+1! Thanks a lot for cleaning up (espacially of my lazynesses and the phpstorm inheritdoc strangenesses) and adjusting naming!

• Try to get the tests to run without fatal errors.
• Fix code sniffer warnings.

Making some progress on the test failures.

Thanks a lot for working on this. You’re right, your proposed method name sounds better than mine. (If at some point we will only have access results as objects, we can also deprecate this new method and rename it back to checkItemAccess() if we want. In that case, whether the new method name is future-proof wouldn’t matter too much. Still, makes sense.)

Adding a new plugin method and then throwing an (undeclared) exception in the base plugin class’ implementation is of course a no-go. Just returning blank metadata seems the best we can do there – shouldn’t really decrease cache correctness compared to what we currently have.

Also, I’m still sceptical about including access result cacheability with the other cacheability metadata:

• I don’t see the pattern of passing an account to getCacheableMetadata() methods anywhere else in Core. It also seems a rather strange construction, conceptually.
• We don’t even always check access on the result items in Views, let alone can predict what other modules will do. So, just always including access cacheability in the result set’s cacheability doesn’t seem correct.
• We already get the access result once, but then throw it away again to compute cacheability again further down. Yes, we could statically cache the access results, as suggested in the @todo comment – but why not just use it when we have it the first time?
  If we deem it worthwhile to have the getCacheableMetadata() method on the result set as a helper for other modules as well, we can just get around adding the metadata for excluded items (whose access check failed) by calling setResultItems() with the modified list after the access checks.

Finally, this still seems a bit piecemeal, adding cacheability metadata for just the result items to the framework, but not for the actual search query. It will also not quite solve the case of results data returned by Solr.
Still, I guess it also won’t hurt future efforts in that direction, so better small steps than nothing.

Anyways, thanks again for your work!
We’re making progress …

(Still (desperately) needs tests, has TODOs and needs some discussion.)

I've started on the test coverage but hit a conondrum: we are currently running the access checks on the result items to get the list cache contexts that are used to determine access, but this is problematic from the point of caching. It means we now only get the correct list cache tags _after_ doing a search query, but since the cache tags are used to generate the cache ID, we would need to do a search query in order to get the cache ID. This doesn't make a lot of sense.

I think we should probably store the information about the cacheability metadata on the search index. We can get the list cache contexts from the entity type manager. I had at look how Views is doing this and here is the code responsible (from QueryPluginBase):

  public function getCacheContexts() {
    $contexts = [];
    if (($views_data = Views::viewsData()->get($this->view->storage->get('base_table'))) && !empty($views_data['table']['entity type'])) {
      $entity_type_id = $views_data['table']['entity type'];
      $entity_type = \Drupal::entityManager()->getDefinition($entity_type_id);
      $contexts = $entity_type->getListCacheContexts();
    }
    return $contexts;
  }

We could do something similar - loop over the entity types that are returned by IndexInterface::getEntityTypes() and retrieve the list cache contexts from them.

I am stopping for today but I will leave this assigned to me, I intend to work again on this after the weekend. I have some test code written but it needs some more work before I can upload a patch.

> I've started on the test coverage but hit a conondrum: we are currently running the access checks on the result items to get the list cache contexts that are used to determine access, but this is problematic from the point of caching. It means we now only get the correct list cache tags _after_ doing a search query, but since the cache tags are used to generate the cache ID, we would need to do a search query in order to get the cache ID. This doesn't make a lot of sense.

Gotcha! So we'll add list tags and contexts to the result set, but need the list contexts beforehand to compute the cache key (contrary to the tags). I see the asymmetry now.
Can we then - given we add list tags and contexts to the result - spare us the individual item's tags and contexts? (I suppose so as they are "weaker" in both cases.)
I looked into that method you cited, too, and it should be fairly easy to code our analog.

I think we should definitely ignore the cache contexts of individual items. Since the cache contexts are used to determine the cache ID any deviation from them would render the cache useless.

The cache tags of the individual items should also be excluded for the reason that Thomas explained above: the search index is not necessarily in sync with the entities themselves. If a node is invalidated now, but the index only updated 5 minutes later we have a risk that the next search result will still return the outdated node data and be cached as such.

So I think we could go with this:

• Add the list cache contexts of the entity types being indexed.
• Add the list cache tags of the search index.

I've been trying to imagine use cases where changing the entity type list cache contexts now and invalidating the index list cache tags at a later point would be giving problems but I cannot come up with anything. I think at the worst case the user sees older indexed data, but this is no different than when we are serving data directly from the search index.

I have one doubt right now: nodes potentially vary by the user cache context because a user might have the view own unpublished content permission. However the entity definition only specifies user.node_grants:view as a list cache context. As far as I can see in NodeAccessGrantsCacheContext this doesn't include any checks on view own unpublished content. So I am not sure if this particular case will work.

I accidentally changed the status in cross posting above. Keeping this assigned to me, I am going to continue working on the test coverage today.

I wasn't able to progress much, but I'm posting what I have up to now.

@pfrenssen: Great you stick on this!

And what you write in #45 totally makes sense and is in sync with what my mind brought up for this.

> I think we should definitely ignore the cache contexts of individual items. Since the cache contexts are used to determine the cache ID any deviation from them would render the cache useless.

Yes. Any cache context of an individual item that is not a cache context of the entity type would be a bug in the first place.

> The cache tags of the individual items should also be excluded for the reason that Thomas explained above: the search index is not necessarily in sync with the entities themselves. If a node is invalidated now, but the index only updated 5 minutes later we have a risk that the next search result will still return the outdated node data and be cached as such.

Yes. Which answers my #32 question why the index needs its tag. (Facepalm!)

> So I think we could go with this:
> Add the list cache contexts of the entity types being indexed.
> Add the list cache tags of the search index.

Yes, that's it.

> I've been trying to imagine use cases where changing the entity type list cache contexts now and invalidating the index list cache tags at a later point would be giving problems but I cannot come up with anything. I think at the worst case the user sees older indexed data, but this is no different than when we are serving data directly from the search index.

I guess any change in entity type cache context should trigger invalidation (didn't RTFS though).

> I have one doubt right now: nodes potentially vary by the user cache context because a user might have the view own unpublished content permission. However the entity definition only specifies user.node_grants:view as a list cache context. As far as I can see in NodeAccessGrantsCacheContext this doesn't include any checks on view own unpublished content. So I am not sure if this particular case will work.

I agree that this would be a bug.
(Maybe everyone is using entity.module which overode this ;-)

I worked a bit more on the test coverage. I'm also incorporating the use case from #3034996: SearchApiQuery does not implement getCacheTags() which seems closely related to this issue.

I have some trouble getting results back from the view in the test. I was not able during my last iteration to create a test view in the UI - I couldn't figure out how to select SearchApiQuery as the basis for the view - so I ended up taking the config of an existing view and modifying it, but this doesn't seem to have been successful. The view always returns 0 results for the moment.

Completed the test coverage. It still needs some cleanups but it is now failing on the expected functionality that is not yet working.

Mostly work on the tests. I was stuck for some time on not getting the expected results but it turns out they were being affected by a hook_node_grants() implementation in the search_api_test module which was active by default. One of our test users also had user ID 1 which was making it act like the root user.

I'm seeing the cache tags of the indexed items appear on the result even though we are no longer passing them. It appears that Views itself will add these cache tags when it is rendering fields. This is happening in StylePluginBase::renderFields().

I'm not sure if it is possible (or rather, feasible) to prevent this from happening without writing a custom style plugin.

> I'm seeing the cache tags of the indexed items appear on the result even though we are no longer passing them. It appears that Views itself will add these cache tags when it is rendering fields. This is happening in StylePluginBase::renderFields().

Interesting. That source is about render caching and that should carry the item cache tags to leverage caching of the respective item render arrays. The view result otoh should carry the list cache tags. Or not?

Yes you are right. The test in its current form checks the render cache, but there is the underlying results cache. If the render cache would get invalidated, the results cache would probably still be intact.

In Views this is tested in \Drupal\Tests\views\Kernel\Plugin\CacheTest. They are simply executing the view and checking the results, as well as inspecting the cache entries directly.

From reading the comments in #3034996: SearchApiQuery does not implement getCacheTags() it also seems that the people reporting the cache tags not bubbling are all using entity view mode based rendering and not field rendering as I am doing.

I am currently working on some other tasks but I am keeping this assigned to me for now. I will be available to continue work on this soon.

> If the render cache would get invalidated, the results cache would probably still be intact.

So let's say our view contains node:17, node:18, node:19. If i update node:17, then the node:17 item tag AND the node_list tag will be invalidated. the node:17 tag will invalidate that part of the render cache, not the other ones.

AND: the node_list cache tag will invalidate the views result (if we do the list tags right ;-).

I dug into \Drupal\Tests\views\Kernel\Plugin\CacheTest::testTimeResultCaching. Indeed i do not grok this comment:

// Note
// that views_test_data records don't have associated cache tags, and hence
// the results cache items aren't invalidated.

AND: the node_list cache tag will invalidate the views result (if we do the list tags right ;-).

We should avoid this because of the asynchronous search indexing. We should vary by the list cache contexts of nodes, but not by their list cache tags. The difference is:

• If the list cache tag is invalidated, all the lists that contain nodes will be invalidated. This happens when e.g. a new node is created. This is not desireable in our case since the cache will be invalidated too early, it should only happen when the search index is updated.
• If the list cache context changes, all the lists that contain nodes will be invalidated. This happens when e.g. a user is granted a new permission that changes their node access grants. This is desireable in our case since the change affects the user and not the existing nodes. Even with an outdated search index, the user will be able to immediately see the new results that their permissions allow them to see.

> it should only happen when the search index is updated.

yes i was too lax writing this. yes of course, the node update will trigger indexing, and indexing will invalidate the search index's cache tag.

as of cache context, yes, this will make the cache key vary (no invalidation, but different cache keys).

(i wonder what happens when the cache context changes, e.g. if we install a new access module. i guess this triggers a cache rebuild, but dunno if we delete indexes than. Or is it they are not invalid then?)

(i wonder what happens when the cache context changes, e.g. if we install a new access module. i guess this triggers a cache rebuild, but dunno if we delete indexes than. Or is it they are not invalid then?)

I think this case should work out of the box. If an entity type has a cache context that is pluggable and a new module provides a plugin for it then the cache context value should change to include this. At least if it is implemented correctly.

Expanded the test even more. I added test cases to validate that the cache gets invalidated correctly if changes are made to the configuration of the view or the search index. I also added test cases now for both a search index that is set to index immediately on entity save, and a search index that is indexing asynchronously.

Following the advice from comment #53 I dropped the testing of the rendered view since we are not concerned with this in the end, our code is dealing with the search result so I am now testing the result cache directly.

Views will invalidate the render cache immediately when indexed content is changed. This will happen regardless of whether we are indexing directly or asynchronously. It will have the side effect that node content will be updated immediately even though the index might be updated at a later point. I think in some edge cases this might cause unexpected side effects if we use asynchronous indexing. An example: if we are searching for the word "alpaca" and get 4 results back, then one of the results is edited and the word "alpaca" is replaced by "llama", then the search index will still return the result until it is reindexed, but in the rendered result the word "alpaca" will not be visible. This is a problem though of the index synchronisation and not of caching. I.e. the problem would be exactly the same whether we cache or not, so there is nothing to be done about it in this issue. But it is something to be aware of, and I think fixable by being smart about showing the right revisions that were active at the time of indexing.

I have also updated a part of the test because I was not aware that if an entity is deleted it is always removed immediately from the index, even if the index is configured to be indexed asynchronously. I understand why this is done, otherwise the search results might include items which do not exist any more and this might cause all kinds of problems. I have updated the test accordingly.

Now the main problem that we are trying to solve is fully tested and green locally.

What is not yet tested is dedicated tests for the refactoring of the code to return access results instead of booleans. This is next up.

Very nice to see it green :) We are getting there :)

My next up plans are to do a review of the entire patch, and write the tests for the returning of the AccessResult objects.

Wow, amazing work! Thanks a lot!

Some remarks:

1. +++ b/src/Plugin/views/query/SearchApiQuery.php
   @@ -119,6 +121,33 @@ class SearchApiQuery extends QueryPluginBase {
   +  /**
   +   * The cacheability metadata of the result.
   +   *
   +   * Set in ::addResults, so only available after ::execute.
   +   *
   +   * Used in the ::getCache*() methods.
   +   *
   +   * @var \Drupal\Core\Cache\RefinableCacheableDependencyInterface
   +   */
   +  protected $cacheableMetadata;
   

   It seems this property, as well as the whole results item cache metadata computation code, are not used anymore? Does this mean they can be removed again, or is that what the @todo tags in this class are for?

2. +++ b/src/Plugin/views/query/SearchApiQuery.php
   @@ -685,8 +738,64 @@ class SearchApiQuery extends QueryPluginBase {
   +    foreach ($this->getIndex()->getEntityTypes() as $entity_type_id) {
   +      $entity_type_definition = $this->entityTypeManager->getDefinition($entity_type_id);
   +      $contexts = Cache::mergeContexts($entity_type_definition->getListCacheContexts(), $contexts);
   +    }
   

   As always, I’d prefer not making this entity-specific but adding a getListCacheContexts() method to datasources to call here. That way, non-entity datasources could also provide list cache contexts.

3. +++ b/src/Plugin/views/query/SearchApiQuery.php
   @@ -685,8 +738,64 @@ class SearchApiQuery extends QueryPluginBase {
   +    if ($this->getIndex()->getOption('index_directly')) {
   +      // @todo If the entities are indexed directly we can add the cache tags
   +      //   of the entities themselves.
   +    }
   +    else {
   +      // @todo If we are indexing asynchronously the best we can do is
   +      //   invalidate the result when the index changes.
   +      // $this->getIndex()->getEntityType()->getListCacheTags();
   +    }
   

   I’m not sure changing tags based on this setting is worthwhile. For one, it makes things even more complicated. (Especially for backends like Solr, where indexing still doesn’t mean results will change right away.)
   But mainly I think that the cache should be invalidated even when other items are indexed, as that might still change the results to be displayed.
   (But then again, I don’t really understand which cache metadata should be returned here – for the results, for rendering, …? What will actually be invalidated based on the cache tags returned here?)

Also, I’m not 100% convinced that we should just always add the list cache contexts from the entity type(s). I understand that we can’t get them from the results, that’s clear. However, as access checks can be executed or circumvented in various ways in the Search API, I think that this will lead to us varying on too many variables in certain cases. (Especially if access checks are completely disabled for some reason.)
The ideal would, of course, still be that all filters, processors, hooks, etc. add their relevant cache metadata to the search query and we use that. (Plus the entity list cache contexts, unless “Skip entity access checks” is enabled.) However, that would probably inflate the complexity of this issue yet more (and by a lot), so I don’t think that path is viable.
So, I can’t really think of a better alternative, either. Probably your current approach does make the most sense. Still, it’s something we should keep in mind.

In general, I still don’t quite understand all the changes you made, but I’m prepared to trust you on this, especially if backed up by good test coverage.

(OK, not sure how smart it was to post a new revision when you’re obviously still working on this. If the interdiff doesn’t apply for you, feel free to ignore part or all of this – my fault for ignoring the “Assigned” field.)

1. It seems this property, as well as the whole results item cache metadata computation code, are not used anymore? Does this mean they can be removed again, or is that what the @todo tags in this class are for?

   Yes it can be removed again.

2. As always, I’d prefer not making this entity-specific but adding a getListCacheContexts() method to datasources to call here. That way, non-entity datasources could also provide list cache contexts.

   Great idea!

3. I’m not sure changing tags based on this setting is worthwhile. For one, it makes things even more complicated. (Especially for backends like Solr, where indexing still doesn’t mean results will change right away.)

   You're right, this doesn't appear to be needed. The test being green while this entire section of code is commented out is proof of this. It can be removed.

In general, I still don’t quite understand all the changes you made, but I’m prepared to trust you on this, especially if backed up by good test coverage.

Yes this caching stuff can be quite difficult to understand. This is why I went with a test-first approach, and then looked into which changes were required to make the test pass.

I think the test covers the use cases outlined here and in #3034996: SearchApiQuery does not implement getCacheTags() pretty well. I took care to outline when the caches are expected to be invalidated, but also when they are expected to be left intact. It's of course possible that some edge cases were missed, and we are only testing the result cache and not the render cache (since this is not controlled by us but by Views). Also since we are now getting the list cache contexts and tags from the entity type definition developers will be able to fix any caching problems they might have by using `hook_entity_type_alter()` to alter in a custom list cache tag or context so they can control invalidation themselves.

I think that we can best split this patch in two to make this easier to review. I will split off the changes that turn the return values from booleans into AccessResult objects into a separate issue. This is still valuable but not strictly required to make the tests pass.

Reduced the patch to only what is strictly necessary to make it work. This will make it easier to grok and review.

I will make a followup issue that contains the original refactoring that returns AccessResult objects instead of booleans. There is still value in having this, but it is not required to make the cache invalidation work.

Followup for returning AccessResult objects: #3058300: Return AccessResult objects instead of booleans when checking access.

Wow, awesome work. I begin to love the test-first approach.

> I think that we can best split this patch in two to make this easier to review.
What about this:
Making maintainer-friendly "git am" single and multi-patches | Drupal.org

#60: Excellent writeup.

Let me muse a bit on the item cache tags, as a reality check. Let's assume index item 17 refers to node 42. I see these options:

a) In the index view we show "Rendered entity". Which means, we filter based on the (maybe outdated) index, but render the current entity. This rendered item must carry the node:42 cache tag.

b) In the index view we show index fields. Which means, everything displayed is based on the (maybe outdated) index. In this case the rendered items must notcarry the node:42 cache tag (as it is invalidated independently from the index), but the search_index:foo cache tag.

#62:
> Also, I’m not 100% convinced that we should just always add the list cache contexts from the entity type(s).

Yes we must not. (I suppose i added that and now see that was wrong.)

> I understand that we can’t get them from the results, that’s clear. However, as access checks can be executed or circumvented in various ways in the Search API, I think that this will lead to us varying on too many variables in certain cases.
> The ideal would, of course, still be that all filters, processors, hooks, etc. add their relevant cache metadata to the search query and we use that. (Plus the entity list cache contexts, unless “Skip entity access checks” is enabled.) However, that would probably inflate the complexity of this issue yet more (and by a lot), so I don’t think that path is viable.

Let's add the API for that and add max-age=0 for no-caching with some TODO for all components for now. This means we are no worse than today but anyone can chime in and scratch some caching itch. How about that?

I wrote the above before i saw #65. Even wore wow.✌(but unfortunately can't look into this now.)

I've been trying out this patch in my project on the various Search API Views that we have, to see how it works in practice. One of the use cases we have needs the cache contexts to be altered, but it cannot be done in a sensible way with the current patch. I was mistaken in my comment #2824640-64: Views cached results are not taking full cacheability metadata into account when I thought that altering the entity types (i.e. the data sources) would be sufficient:

Also since we are now getting the list cache contexts and tags from the entity type definition developers will be able to fix any caching problems they might have by using `hook_entity_type_alter()` to alter in a custom list cache tag or context so they can control invalidation themselves.

Here is the use case we have:

1. The view is showing a list of groups, provided by the Organic Groups module.
2. We are providing a custom facet "My Groups" that allows the user to filter the results to show only the groups they are the administrator of.
3. Only a couple dozen of our users are group administrators, but we have tens of thousands of users. For us it doesn't make sense to vary this view by user, since for 99% of the users it will show the same results. To make this possible we have a custom cache context 'is_group_owner'.

So we basically have a view that shows items from the group datasource, but varies by an aspect of the user. It feels wrong to alter this in the group entity type.

I will look into what would be a good way to make this possible. I think the general use case is that "Modules (such as Facets) should be able to pass arbitrary cacheability metadata through hook_search_api_query_alter()".

In this hook the Search API Query object is passed. Would it be a good idea to allow modules to set cacheability metadata on the query? I.e. add the RefinableCacheableDependencyTrait to the Search API Query object?

> ...[vary by custom cache context]
> Would it be a good idea to allow modules to set cacheability metadata on the query?

Interesting use case. I'll dig into this these days.

Here is an implementation of my idea from #69.

Now SearchApiQuery will take the cacheability metadata from Query and pass it on to Views. Query is responsible to get the cacheability metadata from the index and data sources.

I quite like how this looks from an architectural standpoint, since it is in the end SearchApiQuery is just a wrapper that integrates Query into Views. It should not be concerned with the specifics of how Query works, but just pass on its data.

I still need to try this out inside of my Facets use case, but this will be for after the weekend. In the meantime reviews are more than welcome so I leave this unassigned for now :)

Overall this is looking very good already! I don't have anything to say about this patch really. +1

1. +++ b/src/Datasource/DatasourceInterface.php
   @@ -238,4 +238,15 @@ interface DatasourceInterface extends IndexPluginInterface {
   +   * Allows lists that contain items from this data source to be varied as
   +   * necessary, typically to ensure users of role A see other items listed than
   +   * users of role B.
   

   This documentation is very good, but the sentence doesn't feel right.

2. +++ b/src/Plugin/views/query/SearchApiQuery.php
   @@ -688,6 +690,32 @@ class SearchApiQuery extends QueryPluginBase {
   +    if ($query instanceof CacheableDependencyInterface) {
   +      return $query->getCacheContexts();
   +    }
   
   +++ b/src/Query/Query.php
   @@ -15,9 +18,10 @@ use Drupal\search_api\Utility\QueryHelperInterface;
   +class Query implements QueryInterface, RefinableCacheableDependencyInterface {
   

   It took a little bit of searching to find that RefinableCacheableDependencyInterface extends CacheableDependencyInterface because we implement the refinable and check for the cacheable dependency.

   This is good and not a problem at all, just a hint for the following review :)

Now SearchApiQuery will take the cacheability metadata from Query and pass it on to Views. Query is responsible to get the cacheability metadata from the index and data sources.
I quite like how this looks from an architectural standpoint, since it is in the end SearchApiQuery is just a wrapper that integrates Query into Views. It should not be concerned with the specifics of how Query works, but just pass on its data.

Thought a bit about that and still like it too ;-).

Thanks for comments and reviews! Assigning to address the comments. Time permitting I will also try to work on a proof of concept to integrate this with Facets in my project. I will report back on how well it works in real life.

Addressed comment from #72. I hope this is clearer.

Awesome work here. Thank you.

I only have some nits before RTBC:

1. +++ b/tests/src/Kernel/Views/ViewsCacheInvalidationTest.php
   @@ -0,0 +1,458 @@
   +  public static $modules = [
   

   s/public/protected

2. +++ b/tests/src/Kernel/Views/ViewsCacheInvalidationTest.php
   @@ -0,0 +1,458 @@
   +    $this->createNode('Angua', FALSE);
   ...
   +  protected function createNode($title, $status) {
   

   We have NodeCreationTrait. let's not reinvent the wheel.

3. +++ b/tests/src/Kernel/Views/ViewsCacheInvalidationTest.php
   @@ -0,0 +1,458 @@
   +    $this->users['no-access']->save();
   +
   +    // The user should now be able to see all 4 items.
   

   Shouldn't we check just after adding the role that the cache has been cleared for npo-access user?

4. +++ b/tests/src/Kernel/Views/ViewsCacheInvalidationTest.php
   @@ -0,0 +1,458 @@
   +    $this->assertViewsResult('has-access', ['Angua', 'Cheery', 'Carrot', 'Detritus']);
   +    $this->assertCached('has-access');
   ...
   +    $this->assertNotCached('has-access');
   ...
   +    $this->assertViewsResult('has-access', ['Angua', 'Cheery', 'Detritus']);
   +    $this->assertCached('has-access');
   

   Last time, when no-access has visited, he/she was able to see the same sequence. Shouldn't we test also with no-access?

Thanks for the review! I wanted to address the remarks today but unfortunately I was blocked for a long time on what turned out to be a small bug in SearchApiQuery::init() - it is ignoring the passed in $display object, instead always taking the display ID that is currently active on the view.

This works fine when rendering a view, but not when saving a view through the UI - in this case View::addCacheMetadata() loops over all displays without setting them as active, and I was always getting back the cacheability metadata for the default display.

I have fixed this, but still need to add a test for it.

The other change I made is needed to allow third party modules (such as Facets API) to provide cacheability metadata on the query. If we run the preExecute() method when compiling the cacheability metadata, this will make sure the Search API processors and the alter hook are invoked if they have not yet been. The use case for which this is needed is when a new View is being saved in the Views UI: at this point it compiles the cache contexts so they can be stored in the view config entity.

My time is up for today but I will try to address the remarks from #76 as soon as possible.

Addressed the remarks from #76:

1. Fixed.
2. I consciously didn't use NodeCreationTrait. Using this trait requires us to create a field named "body" which we do not need in this test. I designed the test so that it works with just the base table of the Node entity, and avoids the added complexity of having to create node field data tables which we don't need.
3. This is a very good suggestion but it doesn't work in the way that you would expect. Changing a user's roles does not invalidate any caches. Cache invalidation works using cache tags, but we are not tagging our objects with all the cache tags of all possible users. Instead changing a user's roles affects only the cache _context_ of this user. So the original cached entry will not be invalidated. In fact it is great that it still exist, because it will benefit all other users that still have the roles that our user had before it was changed.

   What I will do is add a test that proves that changing a user does _not_ invalidate these caches, and add a clear explanation of how this process works.

4. OK good suggestion. I was continuing this part of the test with one user since both the users have identical permissions after the role to 'bypass node access' was granted to the 'no-access' user. This is indeed misleading and confusing. I have fixed it by removing the role again, and continuing the test for both users. This will make it clearer what is going on.

Next up is writing the tests for the fixes made in #77.

Added the test for the altering of cacheability metadata through hook_search_api_query_alter() and that the metadata is correct when saving a Views config entity with multiple displays.

This still needs some serious cleanup.

Did some serious cleanup.

I have taken some extra time to add detailed documentation to the test because this caching stuff can be quite difficult to grasp. I am hoping this might help someone in the future when they are trying to figure out caching issues in their search results.

I think this near completion now. I will keep this assigned to me for now because I intend to try this out on our "big" project which has a very extensive test suite. This way I can see how well it performs in real life.

I'm seeing failures in my project with this patch applied. It happens when a custom module that defines a Search API view that has facets is enabled. During the enabling of the module I am getting the error "No facet source defined for facet."

From the backtrace I can see that Views is recalculating the cacheability metadata when the view is saved from config. This causes the problem because at this time the facet does not yet exist. The facet depends on the search API data source and not otherwise.

The problem does not occur when importing single config, or when syncing config. It only happens when enabling a module that contains the view. I did some searching and there are other reports of similar issues (eg #2170579: Race conditions on "default config import", #2805645: Config can be created twice during module install.).

It looks to me like this a bug in Views. Views should not recalculate the cacheability metadata when saving a view from config. Only when it is saved through the UI or API, as explained in \Drupal\views\Entity\View::addCacheMetadata().

I will investigate this further and see if I can come up with a solution.

Here is the backtrace:

./web/modules/contrib/facets/src/Entity/Facet.php:450
./web/modules/contrib/facets/src/FacetManager/DefaultFacetManager.php:115
./web/modules/contrib/facets/facets.module:89
./web/core/lib/Drupal/Core/Extension/ModuleHandler.php:539
./web/modules/contrib/search_api/src/Query/Query.php:555
./web/modules/contrib/search_api/src/Query/Query.php:709
./web/modules/contrib/search_api/src/Plugin/views/query/SearchApiQuery.php:699
./web/core/lib/Drupal/Core/Cache/CacheableMetadata.php:171
./web/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php:2289
./web/core/modules/views/src/Entity/View.php:379
./web/core/modules/views/src/Entity/View.php:312
./web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:490
./web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:445
./web/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:263
./web/core/lib/Drupal/Core/Entity/EntityBase.php:394
./web/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:613
./web/core/lib/Drupal/Core/Config/ConfigInstaller.php:364
./web/core/lib/Drupal/Core/Config/ConfigInstaller.php:136
./web/modules/contrib/config_provider/src/ProxyClass/ConfigProviderConfigInstaller.php:75
./web/core/lib/Drupal/Core/Extension/ModuleInstaller.php:268

Yep it is a bug in Views. This fixes it. I will make an issue there.

diff --git a/core/modules/views/src/Entity/View.php b/core/modules/views/src/Entity/View.php
index 11a0938449..b2e04ab250 100644
--- a/core/modules/views/src/Entity/View.php
+++ b/core/modules/views/src/Entity/View.php
@@ -307,8 +307,7 @@ public function preSave(EntityStorageInterface $storage) {
     ksort($displays);
     $this->set('display', ['default' => $displays['default']] + $displays);
 
-    // @todo Check whether isSyncing is needed.
-    if (!$this->isSyncing()) {
+    if (!$this->isSyncing() && !$this->hasTrustedData()) {
       $this->addCacheMetadata();
     }
   }

I fixed the Views bug and provided a test in #3060081: Recalculation of display cacheability metadata during module install causing error.

It is now working great in my big project which has an test suite of 11000+ Behat steps and 1800+ assertions in PHPUnit. Our Search API views are now cacheable! Here is the PR that shows it working: https://github.com/ec-europa/joinup-dev/pull/1660

I'm unassigning so this can be reviewed.

I do think that we should probably wait until #3060081: Recalculation of display cacheability metadata during module install causing error is in before merging this in Search API. Even though it is possibly a rare edge case for projects to enable a custom module that contains a Search API view with Facets, it does cause a fatal error, and considering the large user base of Search API this might affect some projects.

Wow.

Stumbled upon #2876258: Add a way to pass Access Cacheability of EntityQuery. I think it does not hit us as we do the very expensive access check per individual entity (but which should break pagers), which someday we will want to do away with.

It is now working great in my big project which has an test suite of 11000+ Behat steps and 1800+ assertions in PHPUnit. Our Search API views are now cacheable! Here is the PR that shows it working: https://github.com/ec-europa/joinup-dev/pull/1660

This is really good to see that this works in a real project. Thanks for that @pfrenssen.

I've already reviewed this in #72 and have kept up with the interdiffs in the meanwhile. This looks really good. I really like the cleanup in #80. This now has really good documentation overall and otherwise also looks really good.

I'm going to review this again at ddd but I'd also love to get a review in by @drunken monkey before this goes to rtbc.

Wow. Amazing work, thank you so much! By now, I probably not only owe you a drink but really a whole meal …
And especially great that you even provide a “Drupal Caching for Dummies” course as a side offer, so that I have a chance of understanding what’s going on. Really appreciated, and I do think this has made things a bit clearer for me again!

I do think that we should probably wait until #3060081: Recalculation of display cacheability metadata during module install causing error is in before merging this in Search API. Even though it is possibly a rare edge case for projects to enable a custom module that contains a Search API view with Facets, it does cause a fatal error, and considering the large user base of Search API this might affect some projects.

Hm, OK, that’s bad news. To be absolutely sure that no-one runs into this problem, we’d not only need to wait until this is committed to Core (which can take long enough, from my experience) but until we depend on the version this was fixed in (8.8 at the earliest, so almost a year from now). And you’re right, even if the chance is small (and I’m not even sure it is – I’m even currently working on a site that would apparently be affected), a fatal error is bad enough that it should be avoided for even relatively few affected people.

Is there any feasible way to work around this problem in Search API for now, until we depend on 8.8 (or whatever)?

1. +++ b/src/Query/Query.php
   @@ -696,6 +700,52 @@ class Query implements QueryInterface {
   +  public function getCacheContexts() {
   +    // Call the pre-execute method to ensure that processors and modules have
   +    // had the chance to alter the query and modify the cacheability metadata.
   +    $this->preExecute();
   

   This seems a bit problematic as preExecute() can (according to its docs) throw a SearchApiException – which, of course, the getCache*() methods’ contract doesn’t allow.

   This would be a bit tricky to resolve, I guess – however, on inspection, it luckily turns out that the preExecute() docs were just wrong, there shouldn’t be any chance of an exception (except if a processor or hook implementation ignores its contract).

   So, I just corrected the preExecute() docs, removing the @throws tag. (Do we need a change record for that? As it’s pretty unlikely anyone else implements the interface, probably not. On the other hand, it might be appropriate to warn the processor/hook implementors again that they shouldn’t throw exceptions.)

2. +++ b/tests/src/Kernel/Views/ViewsCacheabilityMetadataExportTest.php
   @@ -0,0 +1,245 @@
   +  /**
   +   * The display IDs used in the test.
   +   */
   +  const TEST_VIEW_DISPLAY_IDS = ['default', 'page_1'];
   

   Unfortunately, we don’t depend on PHP 7 yet, so this is not legal. (See #3044782: Increase PHP version requirement to 7.0.)

3. +++ b/tests/src/Kernel/Views/ViewsCacheabilityMetadataExportTest.php
   @@ -0,0 +1,245 @@
   +    $cache_contexts_manager = $this->prophesize(CacheContextsManager::class);
   +    $cache_contexts_manager->assertValidTokens(Argument::any())->willReturn(TRUE);
   +    $container->set('cache_contexts_manager', $cache_contexts_manager->reveal());
   

   To be consistent within the module, I’d prefer using a mock here. We don’t really use prophecies anywhere else in the module.

4. +++ b/tests/src/Kernel/Views/ViewsCacheabilityMetadataExportTest.php
   @@ -0,0 +1,245 @@
   +      'tags' => [
   +        // Our test view depends on the search index, so whenever the index
   +        // configuration changes the cached results should be invalidated.
   +        // @see \Drupal\search_api\Query\Query::getCacheTags()
   +        'config:search_api.index.test_node_index',
   +      ],
   

   Shouldn’t they also be invalidated when new items get indexed? I.e., shouldn’t there also be the search_api_list:test_node_index tag?
   Based on the other test, this does seem to work correctly already, but I don’t understand why the tag is missing here.

5. +++ b/src/Query/Query.php
   @@ -696,6 +700,52 @@ class Query implements QueryInterface {
   +    foreach ($this->getIndex()->getDatasources() as $datasource) {
   +      $contexts = Cache::mergeContexts($datasource->getListCacheContexts(), $contexts);
   +    }
   

   Hm. In an ideal world, we’d probably include only those datasources that can be part of the results. I.e., we’d go through all filters/conditions placed on the query and look for one restricting the datasources of returned items.

   Shouldn’t be too hard, actually, and pretty reliable. Do you think it’s worth it to add that. (I’d implement that myself, don’t want to pile more work on you.)

Attached patch should fix all this (except where I had questions). Otherwise, this looks pretty much RTBC to me.
It would, however, be great, of course, to get sign-off from someone from #3034996: SearchApiQuery does not implement getCacheTags(), too. Then again, if we need to wait for the Views patch to at least be committed, there’s plenty of time anyways.

Oh, and +1 for the Pratchett references, of course! 👍 (Much better than Madlib. ;P)

So, I just corrected the preExecute() docs, removing the @throws tag. (Do we need a change record for that? As it’s pretty unlikely anyone else implements the interface, probably not. On the other hand, it might be appropriate to warn the processor/hook implementors again that they shouldn’t throw exceptions.)

I don't think we should bother with that.

I do think that we should probably wait until #3060081: Recalculation of display cacheability metadata during module install causing error is in before merging this in Search API. Even though it is possibly a rare edge case for projects to enable a custom module that contains a Search API view with Facets, it does cause a fatal error, and considering the large user base of Search API this might affect some projects.

Hm, OK, that’s bad news. To be absolutely sure that no-one runs into this problem, we’d not only need to wait until this is committed to Core (which can take long enough, from my experience) but until we depend on the version this was fixed in (8.8 at the earliest, so almost a year from now). And you’re right, even if the chance is small (and I’m not even sure it is – I’m even currently working on a site that would apparently be affected), a fatal error is bad enough that it should be avoided for even relatively few affected people.

I agree that is a bad time, but it is the right path forward we can try to push that view issue to also land in 8.7 but I doubt it will happen.
Especially since @pfrenssen asked me to take a look at this again during dev days, I'm sorry but I don't think we can commit this as is right now.

I really tried to find something in the code that wasn't up to our standards or not clear, but I can't find anything like that.

I also disagree with #87.5 about doing more work to find the correct datasources. usually those configured in the index are also the ones being indexed/searched for, and adding more complexity and possibly some slowdown on non-cached results doesn't seem great.

And I disagree super strong that Pratchett > Madlib 🤓.

I'm going to mark this as postponed, in reality it is actually RTBC as soon as we depend on the release this is in.

All right, thanks a lot for thoroughly reviewing again, and your detailed feedback.
Then yes, I guess we have to just hope the patch gets committed to 8.7 still and then wait. Just tagging now so we don’t forget. If it doesn’t land in time for 8.8.0, then we’ll just have to change the tag.

And I disagree super strong that Pratchett > Madlib 🤓.

Then you’re super wrong! 😛
I’m gonna go right now and prepare a patch replacing all the module’s Madlib songs with Pratchett novels! (Or not.)

This issue was also committed to 8.7.x, changing dependency tag.

Final small change now that #3060081: Recalculation of display cacheability metadata during module install causing error has been committed and we know the minimum required version of Drupal that this patch depends on.

Setting this back to Needs Review for this change, but this should be put back to Postponed after review. Search API still has to support Drupal 8.6.x until it reaches EOL in December 2019, so we can only merge this patch at the end of the year.

Hm, not really sure whether we want to start depending on patch versions. Up to now, we’ve always just required specific minor versions – meaning this would still have to wait for 8.8.
However, I guess the case for people who are still on Drupal 8.7.3 or below in half a year is a pretty weak one. Especially if there will be any security releases until then. So, let’s go with this.

However, as explained elsewhere, I’m against adding requirements in more than one place. Composer information is automatically generated, as far as I could see, so adding it explicitly just makes it more likely that the requirements in the two places will drift apart in the future.

I have been investigating an elusive random test failure in my project and found a small thing that is missing from this patch.

We are doing a Search query directly (not through Views) and the search_api_list:{index_id} cache tag is not being returned from Query::getCacheTags(). I had a look in the code and indeed, this cache tag is currently hardcoded in the View but is not returned by the search index entity.

This is a straight reroll. The patch was conflicting with the changes introduced in #3023704: Convert hooks to events.

The failures are caused by #2718045: Selecting the 'Rendered entity' display style when creating a view causes a crash. In that issue the Views cache plugins that are provided by Search API are altered out of the available plugins when there are no search indexes defined yet. However this now makes it impossible to install the test module that is using these plugins, since at installation time there are no indexes yet.

It looks like this fatal error will also occur when installing distributions (and normal install profiles) that use Search API.

I have proposed a solution for the bug introduced by #2718045: Selecting the 'Rendered entity' display style when creating a view causes a crash in #3071591: Search_api plugin not exists after updating 8.1.13 to 8.1.14. Here is a patch that includes the fix, to see if this turns the tests green again.

Here is a fix for the hardcoding of the list cache tag inside Views. I moved it inside Index::getCacheTagsToInvalidate.

On a side note, core already provides a list cache tag for us to use (it is set in ConfigEntityType::__construct() and has the format 'config:' . $definition['id'] . '_list', so we don't need to invent our own list cache tag. However I would suggest to do this in a separate issue since it might break B/C.

Updated tests to assure that the list cache tags for the search indexes are always present.

Indeed that seems to be one of our long lasted random failure. I am giving an RTBC for the interdiff and putting it back in Postponed with the disclaimer that if the failure occurs again, I will report back here.

We are doing a Search query directly (not through Views) and the search_api_list:{index_id} cache tag is not being returned from Query::getCacheTags(). I had a look in the code and indeed, this cache tag is currently hardcoded in the View but is not returned by the search index entity.

I don’t think there even is a Query::getCacheTags()?
There probably should be, if we supported caching completely, but as we’re not there yet, I don’t think this complaint makes sense. Yes, when doing a search query in custom code, you’ll have to either disable caching altogether or at least include that cache tag to get it, hopefully, decently working. (Could always make problems with commit gap in Solr/Elasticsearch, though, which is the main problem we face with supporting caching completely.)

On a side note, core already provides a list cache tag for us to use (it is set in ConfigEntityType::__construct() and has the format 'config:' . $definition['id'] . '_list', so we don't need to invent our own list cache tag. However I would suggest to do this in a separate issue since it might break B/C.

I’m aware of that, but these are two completely different things. Our own search_api_list:INDEX_ID tag is for lists of search results from that index, not a list of indexes.
Therefore, I don’t think your latest changes make sense. Even when an index is saved, search results will stay the same until content is re-indexed (or the index cleared).

@ idimopoulos: I don’t think I understand? Why “Postponed”?

Sorry I am not checking my pc much these days as I am in vacation.

@drunken monkey as you have noted in #93, this still has to wait for 8.8. This is why I set it "back to postponed".

Yeah I agree that we can restore this to the previous state. This patch is already complex enough. Let's move the last change to a separate issue so we can address properly the difference between synced and non-synced storage engines. We could add a method on the index interface that returns TRUE if the index is synced, and only return the list cache tags in this case.

Alright, then! I just committed #3071591: Search_api plugin not exists after updating 8.1.13 to 8.1.14 so pfrenssen’s #96 reroll should now work again.
Reposting.

@ idimopoulos: Ah, of course. Sorry!

Committed. (Yay.)
Thanks a lot again, everyone – first of all pfrenssen, of course!

(And I checked: 8.7.5 was a security release back in July, so everyone not running it yet has bigger problems than not being able to use the latest Search API version.)

Apparently this caused a regression, input welcome in #3136277: Issue with modifying conditionGroup via preExecute event/hook

Re-opening this issue temporarily to make everyone here (especially pfrenssen) aware of #3136277: Issue with modifying conditionGroup via preExecute event/hook.
The basic question is: Is it really necessary to call preExecute() in the new method implementations?

Thanks for the ping, I will subscribe to #3136277: Issue with modifying conditionGroup via preExecute event/hook and negotiate with my employer to get some time to investigate this.

IIRC this call to preExecute() was needed to make this work but it is certainly possible that there is a better way.