Elastica icon indicating copy to clipboard operation
Elastica copied to clipboard
verified publisher icon ruflin

BC Break from 7.3.1 to 7.3.2

Open Tsounabe opened this issue 1 year ago • 23 comments

When migrating from 7.3.1 to 7.3.2 a blocking error is thrown while executing any operations (populate, ...):

In Client.php line 407:                                                  
  Call to a member function hasConnection() on null

Everything works in 7.3.1.

Tsounabe avatar Jun 27 '24 15:06 Tsounabe

Here is the change from 7.3.1 to 7.3.2: https://github.com/ruflin/Elastica/compare/7.3.1...7.3.2 My assumption is the problem comes from this change here: https://github.com/ruflin/Elastica/pull/2184 @csabavirag

I assume you are using addDocument or addScript? We should likely check that $this->_client is set but I also wonder why it is not set yet in your code? Are you adding the client later on? Can you share a bit more around your code that is causing this?

ruflin avatar Jun 28 '24 07:06 ruflin

Seems to affect MediaWiki too, at least in tests. But could be something related to mocking (for example Connection::getClient()) and such though.

https://gerrit.wikimedia.org/r/c/mediawiki/vendor/+/1125546 https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Elastica/+/1125547

https://github.com/wikimedia/mediawiki-extensions-Elastica https://github.com/wikimedia/mediawiki-extensions-CirrusSearch

13:50:20 Report from `split_group1`:
13:50:20 
13:50:20 There were 4 errors:
13:50:20 
13:50:20 1) CirrusSearch\DataSenderTest::testSendDeletesRequest with data set "sendDeletes-request-defaults" (array('myfixturewiki', '1ms'), 'content', array('1', '2', '3'), 'dataSender/sendDeletes-reques...pected')
13:50:20 Error: Call to a member function hasConnection() on null
13:50:20 
13:50:20 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
13:50:20 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:134
13:50:20 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:151
13:50:20 /workspace/src/vendor/ruflin/elastica/src/Client.php:353
13:50:20 /workspace/src/vendor/ruflin/elastica/src/Index.php:415
13:50:20 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:394
13:50:20 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:305
13:50:20 
13:50:20 2) CirrusSearch\DataSenderTest::testSendOtherIndexUpdatesRequest with data set "sendOtherIndexUpdates-request-defaults" (array('myfixturewiki', '1ms'), 'myfixturesourceindex', 'myfixturetargetindex', 2, array(array(1, 'Foo', 0), array(2, 'Bar_baz', 3), array(3, 'Not_Inspired', 2)), 'dataSender/sendOtherIndexUpda...pected')
13:50:20 Error: Call to a member function hasConnection() on null
13:50:20 
13:50:20 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
13:50:20 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:162
13:50:20 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:448
13:50:20 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:338
13:50:20 
13:50:20 3) CirrusSearch\DataSenderTest::testSendWeightedTagsUpdate with data set "sendWeightedTagsUpdate-request-defaults" (array('myfixturewiki', '1ms'), 'myidxsuffix', 2, 'my_custom_prefix', array(array(null), array(null), array(null)), 'dataSender/sendWeightedTagsUp...pected', null)
13:50:20 Error: Call to a member function hasConnection() on null
13:50:20 
13:50:20 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
13:50:20 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:162
13:50:20 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:115
13:50:20 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:395
13:50:20 
13:50:20 4) CirrusSearch\DataSenderTest::testSendWeightedTagsUpdate with data set "sendWeightedTagsUpdate-request-normal" (array('myfixturewiki', '1ms'), 'myidxsuffix', 2, 'my_custom_prefix', array(array(750, 800), array(850, 900), array(950, 1000)), 'dataSender/sendWeightedTagsUp...pected', null)
13:50:20 Error: Call to a member function hasConnection() on null
13:50:20 
13:50:20 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
13:50:20 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:162
13:50:20 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:115
13:50:20 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:395

reedy avatar Jul 24 '25 13:07 reedy

@reedy Do you have an option to quickly check if an additional check if $this->_client is set would fix it. Basically use a modified Elastica version and run the test against it?

ruflin avatar Jul 24 '25 13:07 ruflin

Yeah, we can modify https://gerrit.wikimedia.org/r/c/mediawiki/vendor/+/1125546 basically however we want and it'll be run through CI.

Can either modify it directly, or set the composer.json version to a specific hash/similar if you want.

Can try replacing (in two places?)

if (!$document->hasRetryOnConflict() && $this->_client->hasConnection() && $this->_client->getConnection()->hasParam('retryOnConflict') && ($retry = $this->_client->getConnection()->getParam('retryOnConflict')) > 0) {

with something like

if (!$document->hasRetryOnConflict() && $this->_client && $this->_client->hasConnection() && $this->_client->getConnection()->hasParam('retryOnConflict') && ($retry = $this->_client->getConnection()->getParam('retryOnConflict')) > 0) {

reedy avatar Jul 24 '25 13:07 reedy

https://gerrit.wikimedia.org/r/c/mediawiki/vendor/+/1125546/4..5/ruflin/elastica/src/Bulk.php

reedy avatar Jul 24 '25 13:07 reedy

https://gerrit.wikimedia.org/r/c/mediawiki/vendor/+/1125546/4..5/ruflin/elastica/src/Bulk.php

Me paying attention to the wrong thing...

reedy avatar Jul 24 '25 14:07 reedy

Line 407 of Client.php

    public function hasConnection()
    {
        return $this->_connectionPool->hasConnection();
    }

reedy avatar Jul 24 '25 14:07 reedy

Mocking Client, and therefore _initConnections not being called... But wasn't an issue before obviously.

https://github.com/wikimedia/mediawiki-extensions-CirrusSearch/blob/master/tests/phpunit/integration/DataSenderTest.php#L253 being one of the tests

reedy avatar Jul 24 '25 14:07 reedy

I did ask AI to do a quick pull request in case this helps with testing? https://github.com/ruflin/Elastica/pull/2269

@reedy Did you run the tests on your end to see if it fixes the issue?

ruflin avatar Jul 28 '25 06:07 ruflin

I can try this later tonight! Thanks

reedy avatar Jul 28 '25 14:07 reedy

https://gerrit.wikimedia.org/r/c/mediawiki/vendor/+/1125546/6..7/ruflin/elastica/src/Bulk.php would be reapplying the changes from https://github.com/ruflin/Elastica/commit/fd0ec5d01fc5cb0626df79bf3db7d64c3dfaba8d.patch to src/Bulk.php...

Running through CI now

reedy avatar Jul 28 '25 16:07 reedy

https://integration.wikimedia.org/ci/job/wmf-quibble-core-vendor-mysql-php81/9864/console

Looks to be the same result

17:57:10 There were 4 errors:
17:57:10 
17:57:10 1) CirrusSearch\DataSenderTest::testSendDeletesRequest with data set "sendDeletes-request-defaults" (array('myfixturewiki', '1ms'), 'content', array('1', '2', '3'), 'dataSender/sendDeletes-reques...pected')
17:57:10 Error: Call to a member function hasConnection() on null
17:57:10 
17:57:10 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
17:57:10 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:134
17:57:10 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:151
17:57:10 /workspace/src/vendor/ruflin/elastica/src/Client.php:353
17:57:10 /workspace/src/vendor/ruflin/elastica/src/Index.php:415
17:57:10 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:394
17:57:10 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:305
17:57:10 
17:57:10 2) CirrusSearch\DataSenderTest::testSendOtherIndexUpdatesRequest with data set "sendOtherIndexUpdates-request-defaults" (array('myfixturewiki', '1ms'), 'myfixturesourceindex', 'myfixturetargetindex', 2, array(array(1, 'Foo', 0), array(2, 'Bar_baz', 3), array(3, 'Not_Inspired', 2)), 'dataSender/sendOtherIndexUpda...pected')
17:57:10 Error: Call to a member function hasConnection() on null
17:57:10 
17:57:10 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
17:57:10 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:162
17:57:10 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:448
17:57:10 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:338
17:57:10 
17:57:10 3) CirrusSearch\DataSenderTest::testSendWeightedTagsUpdate with data set "sendWeightedTagsUpdate-request-defaults" (array('myfixturewiki', '1ms'), 'myidxsuffix', 2, 'my_custom_prefix', array(array(null), array(null), array(null)), 'dataSender/sendWeightedTagsUp...pected', null)
17:57:10 Error: Call to a member function hasConnection() on null
17:57:10 
17:57:10 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
17:57:10 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:162
17:57:10 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:115
17:57:10 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:395
17:57:10 
17:57:10 4) CirrusSearch\DataSenderTest::testSendWeightedTagsUpdate with data set "sendWeightedTagsUpdate-request-normal" (array('myfixturewiki', '1ms'), 'myidxsuffix', 2, 'my_custom_prefix', array(array(750, 800), array(850, 900), array(950, 1000)), 'dataSender/sendWeightedTagsUp...pected', null)
17:57:10 Error: Call to a member function hasConnection() on null
17:57:10 
17:57:10 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
17:57:10 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:162
17:57:10 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:115
17:57:10 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:395

reedy avatar Jul 28 '25 17:07 reedy

I just looked again at the PR. What I realised is that null check is the second condition instead of the first one. Let me push another commit and invert it.

ruflin avatar Aug 18 '25 06:08 ruflin

I pushed another commit to https://github.com/ruflin/Elastica/pull/2269 A bit late on my end, but could you check again?

ruflin avatar Aug 18 '25 06:08 ruflin

Still seems to be broken :(

https://gerrit.wikimedia.org/r/c/mediawiki/vendor/+/1125546/8/ruflin/elastica/src/Bulk.php

https://integration.wikimedia.org/ci/job/wmf-quibble-core-vendor-mysql-php81/11132/console https://integration.wikimedia.org/ci/job/wmf-quibble-vendor-mysql-php81/23875/console

13:14:28 There were 4 errors:
13:14:28 
13:14:28 1) CirrusSearch\DataSenderTest::testSendDeletesRequest with data set "sendDeletes-request-defaults" (array('myfixturewiki', '1ms'), 'content', array('1', '2', '3'), 'dataSender/sendDeletes-reques...pected')
13:14:28 Error: Call to a member function hasConnection() on null
13:14:28 
13:14:28 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
13:14:28 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:134
13:14:28 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:151
13:14:28 /workspace/src/vendor/ruflin/elastica/src/Client.php:353
13:14:28 /workspace/src/vendor/ruflin/elastica/src/Index.php:415
13:14:28 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:394
13:14:28 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:305
13:14:28 
13:14:28 2) CirrusSearch\DataSenderTest::testSendOtherIndexUpdatesRequest with data set "sendOtherIndexUpdates-request-defaults" (array('myfixturewiki', '1ms'), 'myfixturesourceindex', 'myfixturetargetindex', 2, array(array(1, 'Foo', 0), array(2, 'Bar_baz', 3), array(3, 'Not_Inspired', 2)), 'dataSender/sendOtherIndexUpda...pected')
13:14:28 Error: Call to a member function hasConnection() on null
13:14:28 
13:14:28 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
13:14:28 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:162
13:14:28 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:448
13:14:28 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:338
13:14:28 
13:14:28 3) CirrusSearch\DataSenderTest::testSendWeightedTagsUpdate with data set "sendWeightedTagsUpdate-request-defaults" (array('myfixturewiki', '1ms'), 'myidxsuffix', 2, 'my_custom_prefix', array(array(null), array(null), array(null)), 'dataSender/sendWeightedTagsUp...pected', null)
13:14:28 Error: Call to a member function hasConnection() on null
13:14:28 
13:14:28 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
13:14:28 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:162
13:14:28 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:115
13:14:28 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:395
13:14:28 
13:14:28 4) CirrusSearch\DataSenderTest::testSendWeightedTagsUpdate with data set "sendWeightedTagsUpdate-request-normal" (array('myfixturewiki', '1ms'), 'myidxsuffix', 2, 'my_custom_prefix', array(array(750, 800), array(850, 900), array(950, 1000)), 'dataSender/sendWeightedTagsUp...pected', null)
13:14:28 Error: Call to a member function hasConnection() on null
13:14:28 
13:14:28 /workspace/src/vendor/ruflin/elastica/src/Client.php:407
13:14:28 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:162
13:14:28 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:115
13:14:28 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:395

reedy avatar Aug 18 '25 12:08 reedy

It's not the client being null, it's the client being called, having a null connection pool

https://github.com/wikimedia/mediawiki-vendor/blob/a4d4ba6/ruflin/elastica/src/Client.php#L407

    /**
     * Determines whether a valid connection is available for use.
     *
     * @return bool
     */
    public function hasConnection()
    {
        return $this->_connectionPool->hasConnection();
    }

reedy avatar Aug 18 '25 12:08 reedy

Any more ideas? :)

reedy avatar Sep 22 '25 11:09 reedy

I'll have a look again. Lets get this fixed once and for all.

ruflin avatar Sep 29 '25 15:09 ruflin

Doing some more tests here: https://github.com/ruflin/Elastica/pull/2287 I think the core problem is that we use the connectionPool to get this info.

ruflin avatar Sep 29 '25 15:09 ruflin

Can you check https://github.com/ruflin/Elastica/pull/2287 and see if works for your use case? My goal is to have a test that fails without the change eventually.

The PR got a bit messy as some of the CI stuff didn't work anymore. I changed how the retry is set and move it directly to the client. I have not validated yet that if a user sets it on the connection Pool, if it still works. But that is a follow up problem.

ruflin avatar Sep 29 '25 17:09 ruflin

https://gerrit.wikimedia.org/r/c/mediawiki/vendor/+/1125546/11/ruflin/elastica/src/Bulk.php

Will see what our CI has to say

reedy avatar Sep 30 '25 01:09 reedy

A different error now - https://integration.wikimedia.org/ci/job/quibble-with-gated-extensions-vendor-mysql-php81/2753/console

02:04:52 
02:04:52 1) CirrusSearch\DataSenderTest::testSendOtherIndexUpdatesRequest with data set "sendOtherIndexUpdates-request-defaults" (array('myfixturewiki', '1ms'), 'myfixturesourceindex', 'myfixturetargetindex', 2, array(array(1, 'Foo', 0), array(2, 'Bar_baz', 3), array(3, 'Not_Inspired', 2)), 'dataSender/sendOtherIndexUpda...pected')
02:04:52 Error: Call to a member function getAll() on null
02:04:52 
02:04:52 /workspace/src/vendor/ruflin/elastica/src/Client.php:150
02:04:52 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:167
02:04:52 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:448
02:04:52 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:336
02:04:52 
02:04:52 2) CirrusSearch\DataSenderTest::testSendWeightedTagsUpdate with data set "sendWeightedTagsUpdate-request-defaults" (array('myfixturewiki', '1ms'), 'myidxsuffix', 2, 'my_custom_prefix', array(array(null), array(null), array(null)), 'dataSender/sendWeightedTagsUp...pected', null)
02:04:52 Error: Call to a member function getAll() on null
02:04:52 
02:04:52 /workspace/src/vendor/ruflin/elastica/src/Client.php:150
02:04:52 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:167
02:04:52 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:115
02:04:52 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:393
02:04:52 
02:04:52 3) CirrusSearch\DataSenderTest::testSendWeightedTagsUpdate with data set "sendWeightedTagsUpdate-request-normal" (array('myfixturewiki', '1ms'), 'myidxsuffix', 2, 'my_custom_prefix', array(array(750, 800), array(850, 900), array(950, 1000)), 'dataSender/sendWeightedTagsUp...pected', null)
02:04:52 Error: Call to a member function getAll() on null
02:04:52 
02:04:52 /workspace/src/vendor/ruflin/elastica/src/Client.php:150
02:04:52 /workspace/src/vendor/ruflin/elastica/src/Bulk.php:167
02:04:52 /workspace/src/extensions/CirrusSearch/includes/DataSender.php:115
02:04:52 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/DataSenderTest.php:393

https://github.com/ruflin/Elastica/blob/7.x/src/Client.php#L150

reedy avatar Sep 30 '25 01:09 reedy

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CirrusSearch/+/1193372/1/tests/phpunit/integration/DataSenderTest.php actually helps fix the issue for us...

reedy avatar Oct 03 '25 10:10 reedy