Skip to content

refactor: throw typed Elastica exceptions for unsupported features #2303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ruflin wants to merge 1 commit into
base: 9.x
Choose a base branch
from
Open

refactor: throw typed Elastica exceptions for unsupported features #2303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* Added support for "search after" based pagination [#1645](https://github.com/ruflin/Elastica/issues/1645)
* Added support for the `seq_no_primary_term` search option and the `if_seq_no` / `if_primary_term` index options to enable optimistic concurrency control [#2284](https://github.com/ruflin/Elastica/pull/2284)
### Changed
* `Elastica\Client::setAsync()`, `getAsync()`, `setResponseException()`, `getResponseException()` and `setServerless()` now throw `Elastica\Exception\NotImplementedException` (a `BadMethodCallException` implementing `Elastica\Exception\ExceptionInterface`) instead of a generic `\Exception` when called. Callers can now catch a typed exception.
* `Elastica\Task::cancel()` now throws `Elastica\Exception\InvalidException` (instead of a generic `\Exception`) when no task id is set.
### Deprecated
### Removed
### Fixed
Expand Down
11 changes: 6 additions & 5 deletions src/Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
use Elastica\Exception\Bulk\ResponseException as BulkResponseException;
use Elastica\Exception\ClientException;
use Elastica\Exception\InvalidException;
use Elastica\Exception\NotImplementedException;
use Elastica\Script\AbstractScript;
use Psr\Http\Client\ClientInterface as HttpClientInterface;
use Psr\Http\Message\RequestInterface;
Expand Down Expand Up @@ -80,12 +81,12 @@ public function getTransport(): Transport

public function setAsync(bool $async): self
{
throw new \Exception('Not supported');
throw new NotImplementedException('Async mode is not supported by Elastica.');
}

public function getAsync(): bool
{
throw new \Exception('Not supported');
throw new NotImplementedException('Async mode is not supported by Elastica.');
}

public function setElasticMetaHeader(bool $active): self
Expand All @@ -102,17 +103,17 @@ public function getElasticMetaHeader(): bool

public function setResponseException(bool $active): self
{
throw new \Exception('Not supported');
throw new NotImplementedException('Toggling the response-exception behaviour is not supported by Elastica.');
}

public function getResponseException(): bool
{
throw new \Exception('Not supported');
throw new NotImplementedException('Toggling the response-exception behaviour is not supported by Elastica.');
}

public function setServerless(bool $value): ClientInterface
{
throw new \Exception('Not supported');
throw new NotImplementedException('Serverless mode is not supported by Elastica.');
}

public function getServerless(): bool
Expand Down
8 changes: 6 additions & 2 deletions src/Task.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Elastic\Elasticsearch\Exception\ServerResponseException;
use Elastic\Transport\Exception\NoNodeAvailableException;
use Elastica\Exception\ClientException;
use Elastica\Exception\InvalidException;

/**
* Represents elasticsearch task.
Expand Down Expand Up @@ -107,12 +108,15 @@ public function isCompleted(): bool
}

/**
* @throws \Exception
* @throws InvalidException if no task id is set
* @throws ClientResponseException if the status code of response is 4xx
* @throws ServerResponseException if the status code of response is 5xx
* @throws ClientException
*/
public function cancel(): Response
{
if ('' === $this->_id) {
throw new \Exception('No task id given');
throw new InvalidException('No task id given');
}

return $this->_client->toElasticaResponse(
Expand Down
38 changes: 38 additions & 0 deletions tests/ClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@

use Elastica\Client;
use Elastica\ClientConfiguration;
use Elastica\Exception\ExceptionInterface;
use Elastica\Exception\InvalidException;
use Elastica\Exception\NotImplementedException;
use Elastica\Test\Base as BaseTest;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\Group;

/**
Expand Down Expand Up @@ -251,4 +254,39 @@
$transport = $client->getTransport();
$this->assertEquals(0, $transport->getRetries());
}

/**
* @return iterable<string, array{0: callable(Client): mixed}>
*/
public static function unsupportedClientFeaturesProvider(): iterable
{
yield 'setAsync' => [static fn (Client $client) => $client->setAsync(true)];
yield 'getAsync' => [static fn (Client $client) => $client->getAsync()];
yield 'setResponseException' => [static fn (Client $client) => $client->setResponseException(true)];
yield 'getResponseException' => [static fn (Client $client) => $client->getResponseException()];
yield 'setServerless' => [static fn (Client $client) => $client->setServerless(true)];
}

#[DataProvider('unsupportedClientFeaturesProvider')]
public function testUnsupportedFeaturesThrowNotImplementedException(callable $invocation): void
{
$client = new Client();

$this->expectException(NotImplementedException::class);
Comment on lines +259 to +275

$invocation($client);
}
Comment on lines +257 to +278

@coderabbitai coderabbitai Bot Apr 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Consolidate old unsupported-feature tests to the new typed-exception expectations.

This new provider-based test is good, but the legacy tests at Line 103, Line 120, Line 129, and Line 138 still assert the old message (Not supported). With the new messages from Client, those checks are now inconsistent and can fail CI. Please update/remove those legacy expectations and keep this provider-driven test as the single source of truth.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/ClientTest.php` around lines 257 - 278, The legacy single-case tests
that still assert the old message "Not supported" must be removed or updated to
match the new provider-based expectation: replace the message-based assertions
with a typed exception check (expectException(NotImplementedException::class))
or delete those legacy tests so the provider-driven test
(unsupportedClientFeaturesProvider +
testUnsupportedFeaturesThrowNotImplementedException) is the single source of
truth; ensure references to Client methods (setAsync, getAsync,
setResponseException, getResponseException, setServerless) remain covered by the
provider test and that no remaining tests assert the exact "Not supported"
message.


public function testNotImplementedExceptionIsCatchableAsElasticaException(): void
{
$client = new Client();

try {
$client->setAsync(true);
$this->fail('Expected NotImplementedException was not thrown.');
} catch (ExceptionInterface $e) {
$this->assertInstanceOf(NotImplementedException::class, $e);
$this->assertInstanceOf(\BadMethodCallException::class, $e);

Check failure on line 289 in tests/ClientTest.php

View workflow job for this annotation

GitHub Actions / PHPStan 

Call to method PHPUnit\Framework\Assert::assertInstanceOf() with 'BadMethodCallException' and Elastica\Exception\NotImplementedException will always evaluate to true.

Check failure on line 289 in tests/ClientTest.php

View workflow job for this annotation

GitHub Actions / PHPStan 

Call to method PHPUnit\Framework\Assert::assertInstanceOf() with 'BadMethodCallException' and Elastica\Exception\NotImplementedException will always evaluate to true.
}
Comment on lines +287 to +290

@coderabbitai coderabbitai Bot Apr 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove the always-true assertion flagged by PHPStan.

After asserting NotImplementedException, the \BadMethodCallException assertion is redundant and reported as always true. Rework the catch/assert pattern to avoid tautological checks.

Suggested adjustment 
-        } catch (ExceptionInterface $e) {
-            $this->assertInstanceOf(NotImplementedException::class, $e);
-            $this->assertInstanceOf(\BadMethodCallException::class, $e);
+        } catch (\BadMethodCallException $e) {
+            $this->assertInstanceOf(ExceptionInterface::class, $e);
+            $this->assertInstanceOf(NotImplementedException::class, $e);
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} catch (ExceptionInterface $e) {
$this->assertInstanceOf(NotImplementedException::class, $e);
$this->assertInstanceOf(\BadMethodCallException::class, $e);
}
} catch (\BadMethodCallException $e) {
$this->assertInstanceOf(ExceptionInterface::class, $e);
$this->assertInstanceOf(NotImplementedException::class, $e);
}
🧰 Tools
🪛 GitHub Check: PHPStan

[failure] 289-289:
Call to method PHPUnit\Framework\Assert::assertInstanceOf() with 'BadMethodCallException' and Elastica\Exception\NotImplementedException will always evaluate to true.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/ClientTest.php` around lines 287 - 290, The test currently catches
ExceptionInterface and asserts it's a NotImplementedException then redundantly
asserts \BadMethodCallException (which PHPStan flags as always-true). Fix by
making the catch specific (catch NotImplementedException $e) or keep the generic
catch but remove the redundant $this->assertInstanceOf(\BadMethodCallException,
$e);—ensure only a single meaningful assertion remains referencing
NotImplementedException (and retain ExceptionInterface only if you need to
assert the interface explicitly).

}
}
3 changes: 2 additions & 1 deletion tests/TaskTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
namespace Elastica\Test;

use Elastica\Document;
use Elastica\Exception\InvalidException;
use Elastica\Index;
use Elastica\Task;
use PHPUnit\Framework\Attributes\Group;
Expand Down Expand Up @@ -61,7 +62,7 @@ public function testRefreshWithOptionsContainingOnWaitForResponseTrue(): void
#[Group('unit')]
public function testCancelThrowsExceptionWithEmptyTaskId(): void
{
$this->expectException(\Exception::class);
$this->expectException(InvalidException::class);
$this->expectExceptionMessage('No task id given');

$task = new Task($this->_getClient(), '');
Expand Down
Loading