Problem/Motivation

Currently an exception is thrown in the SearchApiElasticsearchBackend constructor when a cluster has not yet been configured, or the cluster config can't be loaded. This causes the search API "add server" UI to hard error even if the user isn't trying to add an elastic search server.

Steps to reproduce

Enable the elasticsearch_connector module in a clean environment. Go to "add server" in the search_api admin UI.

Proposed resolution

The client instance should be created in a lazy fashion when it's actually needed. This postpones throwing the exception until search_api actually expects to see it, as well as gives code the opportunity to rely on BackendSpecificInterface::isAvailable().

Remaining tasks

  1. Merge request - merge request !8 created by @rwohleb in #2
  2. Add tests - done by @mparker17 by #14
  3. Review and feedback - done by @sokru in #5
  4. RTBC and feedback - done by @mparker17 in #14
  5. Commit - done by @mparker17 in #15
  6. Release - released in 8.x-7.0-alpha8 by @mparker17

User interface changes

None.

API changes

Would impact anyone who is extending SearchApiElasticsearchBackend, but this is presumably rare.

Data model changes

None.

CommentFileSizeAuthor
#8 3204443-8.0.x.patch971 bytessokru
#2 elasticsearch_connector-no-exception-backend-constructor-3204443-2.patch16.82 KBrwohleb

Issue fork elasticsearch_connector-3204443

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

rwohleb created an issue. See original summary.

rwohleb’s picture

rwohleb
he/him
Primary language English
commented
StatusFileSize
new elasticsearch_connector-no-exception-backend-constructor-3204443-2.patch16.82 KB

rwohleb opened merge request !8

rwohleb’s picture

rwohleb
he/him
Primary language English
commented
Issue summary: View changes
Related issues: +#2986469: Error trying to add search API server after having added cluster, +#3127438: Display error instead of exception when elastic search connection is down., +#3187670: entity.search_api_index.canonical route will produce fatal error when backend is down, +#3153850: getCluster error in getIndexName
sokru’s picture

sokru commented
Status: Active » Needs review

Found few nits, otherwise looks good to me.

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented
Status: Needs review » Needs work

This is also relevant to 8.x where I just encountered it.

There are a bunch of unrelated changes in the MR which make it quite difficult to review and increase the chance of a patch generated from this conflicting with other patches.

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented
Issue tags: +Needs reroll

I tried to reroll this but I got stuck at the changes to \Drupal\elasticsearch_connector\Plugin\search_api\backend\SearchApiElasticsearchBackend::updateIndex since I'm not yet very familiar with this module and the 8.x branch has moved on since this issue was created.

I ended up here because I'm trying to evaluate this as a replacement for solr for various reasons and our environments already use PHP 8.

Happy to help however I can, just a bit stuck right now.

sokru’s picture

sokru commented
StatusFileSize
new 3204443-8.0.x.patch971 bytes

This is a quick patch for 8.0.x branch. I faced the exception when none of the clusters where using "Make this cluster default connection".

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented

Fantastic, thanks @sokru, it does solve the WSOD at /admin/config/search/search-api/add-server but of course still throws an error that is a bit out of context. I'll come back around if this still need work once I'm more familiar with the module.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Version: 8.x-7.x-dev » 8.0.x-dev
Status: Needs work » Needs review
Issue tags: +Needs backport

Going to add this to the 8.0.x review queue and note that it can be backported to 8.x-7.x.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Version: 8.0.x-dev » 8.x-7.x-dev
Issue tags: -Needs reroll, -Needs backport

Moving this to 8.x-7.x, and I've fixed the merge conflicts.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

For documentation / search-ability purposes, the error thrown at /admin/config/search/search-api/add-server is...

Drupal\search_api\SearchApiException: Cannot load the Elasticsearch cluster for your index. in Drupal\elasticsearch_connector\Plugin\search_api\backend\SearchApiElasticsearchBackend->__construct() (line 205 of /var/www/html/src/Plugin/search_api/backend/SearchApiElasticsearchBackend.php).
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

I've fixed @sokru's review comments: thank you very much!

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

I've reviewed the code and it looks good.

I can easily reproduce the error in manual tests on the 8.x-7.x branch (and I think it's common enough that I've bumped the Priority). I can also confirm that the code in the merge request fixes the error.

There were no automated tests in the merge request, so I've added some.

I found that the Cluster::load() call in IndexFactory::getIndexName() makes it hard to test. That code was there before, though, so I think it's out-of-scope to change that. However, I can test some of the code around it.

I also wrote some tests for the getClusterId(), getElasticClient(), getFuzziness(), and isAvailable() functions in SearchApiElasticsearchBackend. I'm assuming that ElasticsearchTest (which extends \Drupal\Tests\search_api\Kernel\BackendTestBase) tests the other functions that were modified in this merge request (i.e.: testAddFacets(), testDeleteItems(), testFieldsUpdated(), testIndexItems(), testRemoveIndex(), testSearch(), testUpdateIndex()).

Given that this seems to be a common error, all of my changes (except for removing the t() call) were to comments or tests, and I have manually tested this, I think this would be okay to merge.

  • mparker17 committed a1da632c on 8.x-7.x authored by rwohleb 
    [#3204443] fix: Exception thrown in backend constructor

By: rwohleb
By...
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Merged! Thanks everyone! I will update this issue when the change is released.

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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

a.dmitriiev’s picture

a.dmitriiev commented

@mparker17 this was merged 9 months ago, but unfortunately not released. Could you please create 8.x-7.0-alpha8 with the fix?

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

@a.dmitriiev, I apologize for the delay, both in releasing this fix and responding to your message! Thank you for the reminder, and thank you for your patience!

This issue has been released in 8.x-7.0-alpha8.

a.dmitriiev’s picture

a.dmitriiev commented

Thank you! And no problem :)