From 8352a6db6b195567f61c232a12fab6d9a72f4e65 Mon Sep 17 00:00:00 2001 From: Spamer Date: Wed, 2 Sep 2026 14:36:06 +0200 Subject: [PATCH 1/2] fix(get): a missing document raises DocumentNotFound Every client exception was wrapped as ElasticSearch, so a document that is not there and a cluster that cannot be reached arrived as the same type - while findOneBy() has always raised DocumentNotFound for that very situation, and AbstractBaseService::get() means to as well. Its found() check cannot be reached today: the blanket wrap turns a 404 into ElasticSearch, and its own catch rethrows that first. Four tests asserted the imprecise type. Their intent - that asking for a deleted document fails - is unchanged; they now name which failure it is. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LiMwcctmVX9zeodkjmoTnK --- src/Model/Get.php | 15 +++++++++++++++ .../Elastic/EdgeCases/ConcurrencyTest.phpt | 4 ++-- .../Elastic/Model/DeleteMultipleTest.phpt | 2 +- tests/SpameriTests/Elastic/Model/DeleteTest.phpt | 4 ++-- tests/SpameriTests/Elastic/Model/GetTest.phpt | 16 ++++++++++++---- 5 files changed, 32 insertions(+), 9 deletions(-) diff --git a/src/Model/Get.php b/src/Model/Get.php index 1154cb2b..f66f56f8 100644 --- a/src/Model/Get.php +++ b/src/Model/Get.php @@ -14,6 +14,7 @@ public function __construct( /** + * @throws \Spameri\Elastic\Exception\DocumentNotFound when the document is not there * @throws \Spameri\Elastic\Exception\ElasticSearch */ public function execute( @@ -34,6 +35,20 @@ public function execute( ) ; + } catch (\Elastic\Elasticsearch\Exception\ClientResponseException $exception) { + if ($exception->getCode() !== 404) { + throw new \Spameri\Elastic\Exception\ElasticSearch($exception->getMessage()); + } + + // The client reports a missing document as a 404 and throws. Saying so with the + // exception the library already has for it makes this path agree with every other one: + // findOneBy() raises DocumentNotFound for the same situation, and AbstractBaseService + // means to as well - its found() check cannot be reached today, because the blanket + // wrap below turns a 404 into ElasticSearch and its catch rethrows that first. + throw new \Spameri\Elastic\Exception\DocumentNotFound( + $index . ' with id ' . $id->value(), + ); + } catch (\Elastic\Elasticsearch\Exception\ElasticsearchException $exception) { throw new \Spameri\Elastic\Exception\ElasticSearch($exception->getMessage()); } diff --git a/tests/SpameriTests/Elastic/EdgeCases/ConcurrencyTest.phpt b/tests/SpameriTests/Elastic/EdgeCases/ConcurrencyTest.phpt index b873bdb5..61d97d77 100644 --- a/tests/SpameriTests/Elastic/EdgeCases/ConcurrencyTest.phpt +++ b/tests/SpameriTests/Elastic/EdgeCases/ConcurrencyTest.phpt @@ -223,7 +223,7 @@ class ConcurrencyTest extends \SpameriTests\Elastic\AbstractTestCase \usleep(300000); // Verify deletions - use a fresh Get model to bypass identity map cache - // Get model throws ElasticSearch exception for 404 + // Get model reports a document that is not there as DocumentNotFound /** @var \Spameri\Elastic\Model\Get $get */ $get = $this->container->getByType(\Spameri\Elastic\Model\Get::class); @@ -233,7 +233,7 @@ class ConcurrencyTest extends \SpameriTests\Elastic\AbstractTestCase new \Spameri\Elastic\Entity\Property\ElasticId($id), \SpameriTests\Elastic\Config::INDEX_EDGE_CASE, ), - \Spameri\Elastic\Exception\ElasticSearch::class, + \Spameri\Elastic\Exception\DocumentNotFound::class, ); } diff --git a/tests/SpameriTests/Elastic/Model/DeleteMultipleTest.phpt b/tests/SpameriTests/Elastic/Model/DeleteMultipleTest.phpt index f642d4a0..a9ee55d7 100644 --- a/tests/SpameriTests/Elastic/Model/DeleteMultipleTest.phpt +++ b/tests/SpameriTests/Elastic/Model/DeleteMultipleTest.phpt @@ -219,7 +219,7 @@ class DeleteMultipleTest extends \SpameriTests\Elastic\AbstractTestCase static function () use ($get, $id): void { $get->execute(new \Spameri\Elastic\Entity\Property\ElasticId($id), self::INDEX); }, - \Spameri\Elastic\Exception\ElasticSearch::class, + \Spameri\Elastic\Exception\DocumentNotFound::class, ); } diff --git a/tests/SpameriTests/Elastic/Model/DeleteTest.phpt b/tests/SpameriTests/Elastic/Model/DeleteTest.phpt index 6552a9bb..493d2ee3 100644 --- a/tests/SpameriTests/Elastic/Model/DeleteTest.phpt +++ b/tests/SpameriTests/Elastic/Model/DeleteTest.phpt @@ -67,7 +67,7 @@ class DeleteTest extends \SpameriTests\Elastic\AbstractTestCase // Verify it's gone \Tester\Assert::exception( static fn () => $get->execute(new \Spameri\Elastic\Entity\Property\ElasticId($id), self::INDEX), - \Spameri\Elastic\Exception\ElasticSearch::class, + \Spameri\Elastic\Exception\DocumentNotFound::class, ); } @@ -132,7 +132,7 @@ class DeleteTest extends \SpameriTests\Elastic\AbstractTestCase // Entity A should be gone \Tester\Assert::exception( static fn () => $get->execute(new \Spameri\Elastic\Entity\Property\ElasticId($idA), $indexA), - \Spameri\Elastic\Exception\ElasticSearch::class, + \Spameri\Elastic\Exception\DocumentNotFound::class, ); // Cleanup diff --git a/tests/SpameriTests/Elastic/Model/GetTest.phpt b/tests/SpameriTests/Elastic/Model/GetTest.phpt index 542b9700..72636ce7 100644 --- a/tests/SpameriTests/Elastic/Model/GetTest.phpt +++ b/tests/SpameriTests/Elastic/Model/GetTest.phpt @@ -65,7 +65,14 @@ class GetTest extends \SpameriTests\Elastic\AbstractTestCase } - public function testGetNonExistentThrowsException(): void + /** + * The exception says which situation it is now. + * + * It used to be ElasticSearch, not by design but because every client exception was wrapped the + * same way - so a missing document and an unreachable cluster arrived as the same type, while + * findOneBy() raised DocumentNotFound for that very situation. + */ + public function testGetNonExistentThrowsDocumentNotFound(): void { /** @var \Spameri\Elastic\Model\Get $get */ $get = $this->container->getByType(\Spameri\Elastic\Model\Get::class); @@ -75,7 +82,7 @@ class GetTest extends \SpameriTests\Elastic\AbstractTestCase new \Spameri\Elastic\Entity\Property\ElasticId('nonexistent-id-12345'), self::INDEX, ), - \Spameri\Elastic\Exception\ElasticSearch::class, + \Spameri\Elastic\Exception\DocumentNotFound::class, ); } @@ -126,13 +133,14 @@ class GetTest extends \SpameriTests\Elastic\AbstractTestCase ); \Tester\Assert::same($idB, $resultB->hit()->id()); - // Trying to get A's ID from B should fail + // Trying to get A's ID from B should fail - as a document that is not there, which is what + // it is, rather than as an unspecified Elasticsearch failure. \Tester\Assert::exception( static fn () => $get->execute( new \Spameri\Elastic\Entity\Property\ElasticId($idA), $indexB, ), - \Spameri\Elastic\Exception\ElasticSearch::class, + \Spameri\Elastic\Exception\DocumentNotFound::class, ); // Cleanup extra indexes From 0c1e06bf2e0df6a865df2c1bfd1e27bbebcd0461 Mon Sep 17 00:00:00 2001 From: Spamer Date: Wed, 2 Sep 2026 14:36:06 +0200 Subject: [PATCH 2/2] perf(entity-manager): find() fetches the document instead of searching for it find() built a term query on _id and ran it through findOneBy, so looking a document up by its identifier cost a full search - query parsing, scoring, a fan-out across shards - to return the one document a GET returns directly. It matters because find() is not only called by application code. EntityFactory resolves every single-entity reference through it while hydrating, one per reference, so a page hydrating twenty entities with a handful of references each pays for hundreds of searches that were only ever document lookups. Nothing needs catching for a missing document: Get now raises DocumentNotFound, which is what find() already propagated through findOneBy. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LiMwcctmVX9zeodkjmoTnK --- src/EntityManager.php | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/EntityManager.php b/src/EntityManager.php index 13a63299..13d4a05d 100644 --- a/src/EntityManager.php +++ b/src/EntityManager.php @@ -8,6 +8,7 @@ public function __construct( private \Spameri\Elastic\Model\Insert $insert, private \Spameri\Elastic\Model\GetAllBy $getAllBy, + private \Spameri\Elastic\Model\Get $get, private \Spameri\Elastic\Model\Delete $delete, private \Spameri\Elastic\Model\EntitySettingsLocator $entitySettingsLocator, private \Spameri\Elastic\EventManager $eventManager, @@ -39,15 +40,21 @@ class: $class, return $entity; } - $elasticQuery = new \Spameri\ElasticQuery\ElasticQuery(); - $elasticQuery->addMustQuery( - new \Spameri\ElasticQuery\Query\Term( - '_id', - $id, - ), - ); + $indexConfig = $this->entitySettingsLocator->locateByEntityClass($class); + + try { + $singleResult = $this->get->execute( + new \Spameri\Elastic\Entity\Property\ElasticId($id), + $indexConfig->indexName(), + ); + + } catch (\Spameri\Elastic\Exception\ElasticSearch $exception) { + \Tracy\Debugger::log($exception->getMessage(), \Tracy\ILogger::CRITICAL); + + throw $exception; + } - return $this->findOneBy($elasticQuery, $class); + return $this->entityFactory->create($singleResult->hit(), $class, $this); }