Since upgrading to 2.7, our log is now full of this.

Happens everytime you use search. I have tried looking for the cause but am not getting anywhere so let me know what/where to look.

Cheers.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jsacksick’s picture

Project: Commerce Kickstart » Commerce Search API
Version: 7.x-2.7 » 7.x-1.x-dev

Is your search index overriden ? You should go there admin/config/search/search_api/index/product_display/status and check if there's a revert tab.

alexp999’s picture

It is, as in its default form it struggles to find any of our products.

I tried checking and re-saving all the options but it made no difference.

It will be a real pain if we have to revert...

jsacksick’s picture

Project: Commerce Search API » Search API

It seems to be a Search API bug, something happened between the 1.4 and the 1.5, I'm trying to find the exact cause.

jsacksick’s picture

Category: support » bug

I did a git bisect and it was working until this commit http://drupalcode.org/project/search_api.git/commitdiff/99d20f73957d5803...

jsacksick’s picture

I know understand why, the cache of the fields has been set but it doesn't contain all the fields ... The cache contains 33 fields while I have 90 fields indexed...

jsacksick’s picture

I think the cache has to be cleared more often, reacting on index_update is not enough IMO.

drunken monkey’s picture

I think the cache has to be cleared more often, reacting on index_update is not enough IMO.

When should it react, then, in your opinion? What is/could be happening that changes the fields but doesn't trigger that hook?

jsacksick’s picture

Status: Active » Needs review
FileSize
644 bytes

What we could do instead is check if the cache is complete by testing it against the $index->options['fields'] array, so if the cache doesn't contain the same amount of fields than in this array, then set it again.

Status: Needs review » Needs work

The last submitted patch, search_api-bypass-cache-if-uncomplete-1999858-8.patch, failed testing.

jsacksick’s picture

Status: Needs work » Needs review
FileSize
679 bytes
jsacksick’s picture

I tested the patch from #10 and there was a last remaining bug in Kickstart, right after the installation, it's now fixed with the attached patch.

jsacksick’s picture

Status: Needs review » Needs work

Hm... It works but the patch is incomplete because it doesn't handle all the usecases, this will only work for the getFields() only indexed call, before I continue working on this, what's your opinion on that Thomas ?

jsacksick’s picture

Status: Needs work » Needs review
FileSize
983 bytes

The attached patch partially reverts the commit mentionned, it removes the cache and it now works again as it used to at least inside Kickstart.

jsacksick’s picture

Better with the correct patch :)

drunken monkey’s picture

Status: Needs review » Needs work

Definitely not, sorry. For sites with many content types / fields and maybe some nested structures, each individual getFields() call was measured to take up to 5% of total page execution time. For static information, this is unacceptable, even if it will be faster for sites with a less complex data structure.

I'd guess the problem during installation would be that some of the fields that are set in the index don't exist yet in the entity data structure. Thus, when the first getFields() is called, some of the index's fields are sorted out, and this reduced result is then what gets cached.
Possible fixes:
- Don't enable/create the index until the underlying data structure is complete.
- Check whether some fields were left out and don't cache the result if that was the case. (Probably also log a watchdog warning, since this would be bad if happening on a normally running site.)

While the second option seems smarter at first, I think the first one would actually be preferable, or maybe using both. Having an index with fields that don't exist (yet) is generally not a good idea. E.g., #1694832-7: React properly to Field API field changes might then re-save the index with the reduced set of fields.
So, if it's possible to re-arrange things so that this situation doesn't arise, I think it would be a good idea to do that.

jsacksick’s picture

Status: Needs work » Needs review

Ok, I used the debugger this time :p and the issue seems to be caused by the Search API ranges data alteration callback which is calling the getFields() method of the index in propertyInfo(), the wrong data were cached because of this. I'm going to send a patch in the Search API ranges issue queue for that.
See #2001846: Rewrite the data alteration callback.

drunken monkey’s picture

Status: Needs review » Needs work

And, does that patch there solve the problem? If so, then this issue here might be obsolete?
Or would you still like to get a patch in to help prevent such complications also in other cases?

jsacksick’s picture

It solves the issue, it seems that calling getFields() inside propertyInfo is not a good idea.
I also noticed that the propertyInfo method of a data alteration callback is only called when the status the data alteration changes (When you enable/disable it).
The patch attached here (http://drupal.org/node/2001846#comment-7444278) has a workaround for that in configurationFormValidate()

drunken monkey’s picture

I also noticed that the propertyInfo method of a data alteration callback is only called when the status the data alteration changes (When you enable/disable it).

Not when you change the settings? This would be bad, and definitely a bug we need to fix.
Otherwise, we should perhaps still note this in the doc comments.

jsacksick’s picture

The offending line is in search_api_admin_index_workflow_submit(),

    // Check whether callback status has changed.  ==> The check below is causing the issue
    if ($values['callbacks'][$name]['status'] == empty($options['data_alter_callbacks'][$name]['status'])) { 
      if ($values['callbacks'][$name]['status']) {
        // Callback was just enabled, add its fields.
        $properties = $callback->propertyInfo();

drunken monkey’s picture

I'm pretty sure the methods will also always be called (on the enabled data alterations) when re-creating the getFields() cache. The line in your comment is only for automatically indexing all fields that were added by a data alteration that was just enabled.

jsacksick’s picture

I know but don't you think it makes sense to automatically index fields you specifically added?

drunken monkey’s picture

I think I've lost you. Of course I think that makes sense, that's why that code is there. What do you mean? What is the issue here now?
(Except that we should perhaps document that you shouldn't call $index->getFields() in getPropertyInfo().)

jsacksick’s picture

I'm just saying that if you have a data alteration already enabled (Search API Aggregation or Search API ranges for instance) and you add new fields, these are not going to be indexed right away.

drunken monkey’s picture

Title: Undefined index: search_api_aggregation_X in SearchApiViewsHandlerFilterFulltext->getFulltextFields() » Clean up API documentation for data alterations
Category: bug » task
Status: Needs work » Needs review
FileSize
5.61 KB

OK, yes, that's right. It's somewhat harder to implement that, and also less common than just for enabling data alterations.
Also, this is clearly a different issue. If you want it added, you are welcome to create a feature request and supply a patch.

Regarding the issue at hand, I'd suggest updating the documentation for data alterations – see the attached patch. I took the chance to also adapt the documentation in that file to the Drupal documentation standards, but the important change is the addition of the following paragraph to the propertyInfo() documentation:

CAUTION: Since this method is used when calling
SearchApiIndex::getFields(), calling that method from inside propertyInfo()
will lead to a recursion and should therefore be avoided.

Do you think this is clear and helpful? (And do the comments maybe still violate some Drupal standard?)

Status: Needs review » Needs work

The last submitted patch, 1999858-25--data_alteration_doc_comments.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
drunken monkey’s picture

Component: Miscellaneous » Documentation
Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

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