Elastica issue 2219 investigation

ISSUE-2219-INVESTIGATION.md

@@ -0,0 +1,112 @@
+# Issue #2219 Investigation Report
+
+## Problem Summary
+
+Issue #2219 reports a "Call to a member function on null" error in the `Bulk` class when `_client` is null. This can occur in mocking scenarios or edge cases where the client might be null.
+
+## Root Cause
+
+The `Bulk` class has three locations where `$this->_client->getConfigValue()` is called without checking if `_client` is null first:
+
+1. **Line 136** in `addDocument()` method - checks for `retryOnConflict` config
+2. **Line 168** in `addScript()` method - checks for `retryOnConflict` config  
+3. **Line 347** in `_processResponse()` method - checks for `autoPopulate` config
+
+## Solution Applied
+
+Added null checks before calling `getConfigValue()` on `_client` in all three locations:
+
+### 1. `addDocument()` method (line 135)
+```php
+// Before:
+if (!$document->hasRetryOnConflict()) {
+    $retry = $this->_client->getConfigValue('retryOnConflict', 0);
+    // ...
+}
+
+// After:
+if (!$document->hasRetryOnConflict() && null !== $this->_client) {
+    $retry = $this->_client->getConfigValue('retryOnConflict', 0);
+    // ...
+}
+```
+
+### 2. `addScript()` method (line 167)
+```php
+// Before:
+if (!$script->hasRetryOnConflict()) {
+    $retry = $this->_client->getConfigValue('retryOnConflict', 0);
+    // ...
+}
+
+// After:
+if (!$script->hasRetryOnConflict() && null !== $this->_client) {
+    $retry = $this->_client->getConfigValue('retryOnConflict', 0);
+    // ...
+}
+```
+
+### 3. `_processResponse()` method (line 346-347)
+```php
+// Before:
+if ($data instanceof Document && $data->isAutoPopulate()
+    || $this->_client->getConfigValue(['document', 'autoPopulate'], false)
+) {
+    // ...
+}
+
+// After:
+if ($data instanceof Document && ($data->isAutoPopulate()
+    || (null !== $this->_client && $this->_client->getConfigValue(['document', 'autoPopulate'], false)))
+) {
+    // ...
+}
+```
+
+## Tests Added
+
+Three comprehensive unit tests were added to `tests/BulkTest.php`:
+
+1. **`testAddDocumentWithNullClientDoesNotThrowError()`** - Verifies that `addDocument()` handles null client gracefully
+2. **`testAddScriptWithNullClientDoesNotThrowError()`** - Verifies that `addScript()` handles null client gracefully
+3. **`testProcessResponseWithNullClientAndAutoPopulateDoesNotThrowError()`** - Verifies that `_processResponse()` handles null client when checking autoPopulate config
+
+All tests use reflection to simulate the null client scenario and verify that no errors are thrown.
+
+## Branch Status
+
+### 9.x Branch (Current)
+- ✅ **Fixed** - All three locations now have null checks
+- ✅ **Tests Added** - Comprehensive unit tests verify the fix
+
+### 8.x Branch
+- ❌ **Issue Still Exists** - The following lines still lack null checks:
+  - Line 139: `addDocument()` method - calls `$this->_client->getConfigValue()` without null check
+  - Line 171: `addScript()` method - calls `$this->_client->getConfigValue()` without null check
+  - Line 350: `_processResponse()` method - calls `$this->_client->getConfigValue()` without null check for autoPopulate
+
+### 7.x Branch
+- ❌ **Issue Still Exists** - The issue exists but in a different form due to older codebase structure:
+  - Line 133: `addDocument()` method - calls `$this->_client->hasConnection()` without null check
+  - Line 161: `addScript()` method - calls `$this->_client->hasConnection()` without null check
+  - Line 341: `_processResponse()` method - calls `$this->_client->getConfigValue()` without null check for autoPopulate
+
+**Note:** The 7.x branch uses a different API structure (Connection-based) for `addDocument()` and `addScript()`, but still has the same null client vulnerability. The fix would need to be adapted to check for null before calling `hasConnection()`.
+
+## Recommendations
+
+1. **Apply the same fix to 8.x branch** - The issue exists there and should be fixed for consistency
+2. **Apply fix to 7.x branch** - The issue exists there but requires adaptation due to different API structure (Connection-based vs ConfigValue-based)
+3. **Backport tests** - The unit tests should also be added to 8.x and 7.x branches (adapted for their respective code structures)
+4. **Consider making `_client` nullable** - If null is a valid state, consider updating the type hint to `?Client` and adding proper null handling throughout the class
+
+## Files Modified
+
+- `src/Bulk.php` - Added null checks in three methods
+- `tests/BulkTest.php` - Added three unit tests for null client scenarios
+
+## Related Commits
+
+- Original fix attempt: `fd0ec5d0` (Fix null client error in Bulk operations #2219)
+- Branch: `remotes/origin/issue-2219` (has the fix applied)
+- Branch: `remotes/origin/fix-2219` (older version with fix)

src/Bulk.php

@@ -132,7 +132,7 @@
      */
     public function addDocument(Document $document, ?string $opType = null): self
     {
-        if (!$document->hasRetryOnConflict()) {
+        if (!$document->hasRetryOnConflict() && null !== $this->_client) {
             $retry = $this->_client->getConfigValue('retryOnConflict', 0);
 
             if ($retry > 0) {
@@ -164,7 +164,7 @@
      */
     public function addScript(AbstractScript $script, ?string $opType = null): self
     {
-        if (!$script->hasRetryOnConflict()) {
+        if (!$script->hasRetryOnConflict() && null !== $this->_client) {
             $retry = $this->_client->getConfigValue('retryOnConflict', 0);
 
             if ($retry > 0) {
@@ -343,8 +343,8 @@
 
                 if ($action instanceof AbstractDocumentAction) {
                     $data = $action->getData();
-                    if ($data instanceof Document && $data->isAutoPopulate()
-                        || $this->_client->getConfigValue(['document', 'autoPopulate'], false)
+                    if ($data instanceof Document && ($data->isAutoPopulate()
+                        || (null !== $this->_client && $this->_client->getConfigValue(['document', 'autoPopulate'], false)))
                     ) {
                         if (!$data->hasId() && isset($bulkResponseData['_id'])) {
                             $data->setId($bulkResponseData['_id']);

tests/BulkTest.php

@@ -782,4 +782,91 @@
         $this->expectException(RequestEntityTooLargeException::class);
         $bulk->send();
     }
+
+    #[Group('unit')]
+    public function testAddDocumentWithNullClientDoesNotThrowError(): void
+    {
+        $clientMock = $this->createMock(Client::class);
+        $bulk = new Bulk($clientMock);
+
+        // Use reflection to simulate the scenario where _client might be null
+        $reflection = new \ReflectionClass($bulk);
+        $clientProperty = $reflection->getProperty('_client');
+        $clientProperty->setAccessible(true);
+        $clientProperty->setValue($bulk, null);
+
+        $document = new Document('1', ['name' => 'Test Document']);
+
+        // This should not throw an error even when _client is null
+        $bulk->addDocument($document);
+
+        $actions = $bulk->getActions();
+        $this->assertCount(1, $actions);
+        $this->assertInstanceOf(AbstractDocument::class, $actions[0]);
+    }
+
+    #[Group('unit')]
+    public function testAddScriptWithNullClientDoesNotThrowError(): void
+    {
+        $clientMock = $this->createMock(Client::class);
+        $bulk = new Bulk($clientMock);
+
+        // Use reflection to simulate the scenario where _client might be null
+        $reflection = new \ReflectionClass($bulk);
+        $clientProperty = $reflection->getProperty('_client');
+        $clientProperty->setAccessible(true);
+        $clientProperty->setValue($bulk, null);
+
+        $script = new Script('ctx._source.name = params.name', ['name' => 'Test Script'], 'painless');
+
+        // This should not throw an error even when _client is null
+        $bulk->addScript($script);
+
+        $actions = $bulk->getActions();
+        $this->assertCount(1, $actions);
+        $this->assertInstanceOf(AbstractDocument::class, $actions[0]);
+    }
+
+    #[Group('unit')]
+    public function testProcessResponseWithNullClientAndAutoPopulateDoesNotThrowError(): void
+    {
+        $clientMock = $this->createMock(Client::class);
+        $bulk = new Bulk($clientMock);
+
+        // Use reflection to simulate the scenario where _client might be null
+        $reflection = new \ReflectionClass($bulk);
+        $clientProperty = $reflection->getProperty('_client');
+        $clientProperty->setAccessible(true);
+        $clientProperty->setValue($bulk, null);
+
+        $document = new Document(null, ['name' => 'Test Document']);
+        $bulk->addDocument($document);
+
+        // Create a mock response that would trigger autoPopulate logic
+        $responseData = [
+            'items' => [
+                [
+                    'index' => [
+                        '_id' => 'test-id-123',
+                        '_version' => 1,
+                        'status' => 201,
+                    ],
+                ],
+            ],
+        ];
+
+        $response = new ElasticaResponse('', 200);
+        $response->setData($responseData);
+
+        // Use reflection to access the protected _processResponse method
+        $processResponseMethod = $reflection->getMethod('_processResponse');
+        $processResponseMethod->setAccessible(true);
+
+        // This should not throw an error even when _client is null
+        // The autoPopulate check should handle null client gracefully
+        $responseSet = $processResponseMethod->invoke($bulk, $response);
+
+        $this->assertInstanceOf(\Elastica\Bulk\ResponseSet::class, $responseSet);
+        $this->assertFalse($responseSet->hasError());
+    }
 }