Skip to content

Commit 158a3bd

Browse files
cursoragentruflin
cursoragent
and
ruflin
committed
Fix: Add null checks for _client in Bulk class
Co-authored-by: ruflin <ruflin@elastic.co>
1 parent bc9eeaf commit 158a3bd

File tree

3 files changed

+197
-4
lines changed

3 files changed

+197
-4
lines changed

‎ISSUE-2219-INVESTIGATION.md‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
# Issue #2219 Investigation Report
2+
3+
## Problem Summary
4+
5+
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.
6+
7+
## Root Cause
8+
9+
The `Bulk` class has three locations where `$this->_client->getConfigValue()` is called without checking if `_client` is null first:
10+
11+
1. **Line 136** in `addDocument()` method - checks for `retryOnConflict` config
12+
2. **Line 168** in `addScript()` method - checks for `retryOnConflict` config
13+
3. **Line 347** in `_processResponse()` method - checks for `autoPopulate` config
14+
15+
## Solution Applied
16+
17+
Added null checks before calling `getConfigValue()` on `_client` in all three locations:
18+
19+
### 1. `addDocument()` method (line 135)
20+
```php
21+
// Before:
22+
if (!$document->hasRetryOnConflict()) {
23+
$retry = $this->_client->getConfigValue('retryOnConflict', 0);
24+
// ...
25+
}
26+
27+
// After:
28+
if (!$document->hasRetryOnConflict() && null !== $this->_client) {
29+
$retry = $this->_client->getConfigValue('retryOnConflict', 0);
30+
// ...
31+
}
32+
```
33+
34+
### 2. `addScript()` method (line 167)
35+
```php
36+
// Before:
37+
if (!$script->hasRetryOnConflict()) {
38+
$retry = $this->_client->getConfigValue('retryOnConflict', 0);
39+
// ...
40+
}
41+
42+
// After:
43+
if (!$script->hasRetryOnConflict() && null !== $this->_client) {
44+
$retry = $this->_client->getConfigValue('retryOnConflict', 0);
45+
// ...
46+
}
47+
```
48+
49+
### 3. `_processResponse()` method (line 346-347)
50+
```php
51+
// Before:
52+
if ($data instanceof Document && $data->isAutoPopulate()
53+
|| $this->_client->getConfigValue(['document', 'autoPopulate'], false)
54+
) {
55+
// ...
56+
}
57+
58+
// After:
59+
if ($data instanceof Document && ($data->isAutoPopulate()
60+
|| (null !== $this->_client && $this->_client->getConfigValue(['document', 'autoPopulate'], false)))
61+
) {
62+
// ...
63+
}
64+
```
65+
66+
## Tests Added
67+
68+
Three comprehensive unit tests were added to `tests/BulkTest.php`:
69+
70+
1. **`testAddDocumentWithNullClientDoesNotThrowError()`** - Verifies that `addDocument()` handles null client gracefully
71+
2. **`testAddScriptWithNullClientDoesNotThrowError()`** - Verifies that `addScript()` handles null client gracefully
72+
3. **`testProcessResponseWithNullClientAndAutoPopulateDoesNotThrowError()`** - Verifies that `_processResponse()` handles null client when checking autoPopulate config
73+
74+
All tests use reflection to simulate the null client scenario and verify that no errors are thrown.
75+
76+
## Branch Status
77+
78+
### 9.x Branch (Current)
79+
- ✅ **Fixed** - All three locations now have null checks
80+
- ✅ **Tests Added** - Comprehensive unit tests verify the fix
81+
82+
### 8.x Branch
83+
- ❌ **Issue Still Exists** - The following lines still lack null checks:
84+
- Line 139: `addDocument()` method
85+
- Line 171: `addScript()` method
86+
- Line 350: `_processResponse()` method for autoPopulate
87+
88+
### 7.x Branch (fix-2219)
89+
- ✅ **Fixed** - A fix was previously applied to this branch, but it uses an older codebase structure
90+
91+
## Recommendations
92+
93+
1. **Apply the same fix to 8.x branch** - The issue exists there and should be fixed for consistency
94+
2. **Backport tests** - The unit tests should also be added to 8.x branch
95+
3. **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
96+
97+
## Files Modified
98+
99+
- `src/Bulk.php` - Added null checks in three methods
100+
- `tests/BulkTest.php` - Added three unit tests for null client scenarios
101+
102+
## Related Commits
103+
104+
- Original fix attempt: `fd0ec5d0` (Fix null client error in Bulk operations #2219)
105+
- Branch: `remotes/origin/issue-2219` (has the fix applied)
106+
- Branch: `remotes/origin/fix-2219` (older version with fix)

‎src/Bulk.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ public function getActions(): array
132132
*/
133133
public function addDocument(Document $document, ?string $opType = null): self
134134
{
135-
if (!$document->hasRetryOnConflict()) {
135+
if (!$document->hasRetryOnConflict() && null !== $this->_client) {
136136
$retry = $this->_client->getConfigValue('retryOnConflict', 0);
137137

138138
if ($retry > 0) {
@@ -164,7 +164,7 @@ public function addDocuments(array $documents, ?string $opType = null): self
164164
*/
165165
public function addScript(AbstractScript $script, ?string $opType = null): self
166166
{
167-
if (!$script->hasRetryOnConflict()) {
167+
if (!$script->hasRetryOnConflict() && null !== $this->_client) {
168168
$retry = $this->_client->getConfigValue('retryOnConflict', 0);
169169

170170
if ($retry > 0) {
@@ -343,8 +343,8 @@ protected function _processResponse(Response $response): ResponseSet
343343

344344
if ($action instanceof AbstractDocumentAction) {
345345
$data = $action->getData();
346-
if ($data instanceof Document && $data->isAutoPopulate()
347-
|| $this->_client->getConfigValue(['document', 'autoPopulate'], false)
346+
if ($data instanceof Document && ($data->isAutoPopulate()
347+
|| (null !== $this->_client && $this->_client->getConfigValue(['document', 'autoPopulate'], false)))
348348
) {
349349
if (!$data->hasId() && isset($bulkResponseData['_id'])) {
350350
$data->setId($bulkResponseData['_id']);

‎tests/BulkTest.php‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -782,4 +782,91 @@ public function testSendRequestEntityTooLargeExceptionIf413Response(): void
782782
$this->expectException(RequestEntityTooLargeException::class);
783783
$bulk->send();
784784
}
785+
786+
#[Group('unit')]
787+
public function testAddDocumentWithNullClientDoesNotThrowError(): void
788+
{
789+
$clientMock = $this->createMock(Client::class);
790+
$bulk = new Bulk($clientMock);
791+
792+
// Use reflection to simulate the scenario where _client might be null
793+
$reflection = new \ReflectionClass($bulk);
794+
$clientProperty = $reflection->getProperty('_client');
795+
$clientProperty->setAccessible(true);
796+
$clientProperty->setValue($bulk, null);
797+
798+
$document = new Document('1', ['name' => 'Test Document']);
799+
800+
// This should not throw an error even when _client is null
801+
$bulk->addDocument($document);
802+
803+
$actions = $bulk->getActions();
804+
$this->assertCount(1, $actions);
805+
$this->assertInstanceOf(AbstractDocument::class, $actions[0]);
806+
}
807+
808+
#[Group('unit')]
809+
public function testAddScriptWithNullClientDoesNotThrowError(): void
810+
{
811+
$clientMock = $this->createMock(Client::class);
812+
$bulk = new Bulk($clientMock);
813+
814+
// Use reflection to simulate the scenario where _client might be null
815+
$reflection = new \ReflectionClass($bulk);
816+
$clientProperty = $reflection->getProperty('_client');
817+
$clientProperty->setAccessible(true);
818+
$clientProperty->setValue($bulk, null);
819+
820+
$script = new Script('ctx._source.name = params.name', ['name' => 'Test Script'], 'painless');
821+
822+
// This should not throw an error even when _client is null
823+
$bulk->addScript($script);
824+
825+
$actions = $bulk->getActions();
826+
$this->assertCount(1, $actions);
827+
$this->assertInstanceOf(AbstractDocument::class, $actions[0]);
828+
}
829+
830+
#[Group('unit')]
831+
public function testProcessResponseWithNullClientAndAutoPopulateDoesNotThrowError(): void
832+
{
833+
$clientMock = $this->createMock(Client::class);
834+
$bulk = new Bulk($clientMock);
835+
836+
// Use reflection to simulate the scenario where _client might be null
837+
$reflection = new \ReflectionClass($bulk);
838+
$clientProperty = $reflection->getProperty('_client');
839+
$clientProperty->setAccessible(true);
840+
$clientProperty->setValue($bulk, null);
841+
842+
$document = new Document(null, ['name' => 'Test Document']);
843+
$bulk->addDocument($document);
844+
845+
// Create a mock response that would trigger autoPopulate logic
846+
$responseData = [
847+
'items' => [
848+
[
849+
'index' => [
850+
'_id' => 'test-id-123',
851+
'_version' => 1,
852+
'status' => 201,
853+
],
854+
],
855+
],
856+
];
857+
858+
$response = new ElasticaResponse('', 200);
859+
$response->setData($responseData);
860+
861+
// Use reflection to access the protected _processResponse method
862+
$processResponseMethod = $reflection->getMethod('_processResponse');
863+
$processResponseMethod->setAccessible(true);
864+
865+
// This should not throw an error even when _client is null
866+
// The autoPopulate check should handle null client gracefully
867+
$responseSet = $processResponseMethod->invoke($bulk, $response);
868+
869+
$this->assertInstanceOf(\Elastica\Bulk\ResponseSet::class, $responseSet);
870+
$this->assertFalse($responseSet->hasError());
871+
}
785872
}

0 commit comments

Comments
 (0)