From e8a3126b0d493ae9e1c43e9f005a8bc4e031b374 Mon Sep 17 00:00:00 2001 From: Spamer Date: Mon, 17 Aug 2026 03:42:16 +0200 Subject: [PATCH] fix(factory): a value collection comes back with its values in it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PrepareEntityArray has always known how to write one: it walks a ValueCollectionInterface and stores value() for each member, leaving a flat list of scalars in the document. EntityFactory had no matching branch, so the property fell through to the generic "instantiate from nested properties" tail — which looks for the collection's own properties under `field.*`, finds nothing there, and hands back an empty collection. Every read. Silently. The data was in Elasticsearch, indexed and searchable the whole time, and simply never reached anything holding an entity. It was found in spameri.cz, where 627k of 1.58m titles carry genres in the index and the API answered `"genres": []` for all of them. The read side now mirrors the write side: one value object per stored scalar, and nothing else, because there are no nested properties to resolve. Absent and empty fields both hydrate as an empty collection rather than throwing, so a document written before the field existed still reads. The value class cannot be recovered from ["Action", "Drama"], so it is named the way ElasticCollection already names one, with a mapping attribute. Collections without it keep their current behaviour. --- src/Factory/EntityFactory.php | 28 ++++ src/Mapping/ValueCollection.php | 24 +++ .../Data/Entity/EntityWithValueCollection.php | 28 ++++ .../EntityFactory/ValueCollectionTest.phpt | 146 ++++++++++++++++++ 4 files changed, 226 insertions(+) create mode 100644 src/Mapping/ValueCollection.php create mode 100644 tests/SpameriTests/Elastic/Data/Entity/EntityWithValueCollection.php create mode 100644 tests/SpameriTests/Elastic/Factory/EntityFactory/ValueCollectionTest.phpt diff --git a/src/Factory/EntityFactory.php b/src/Factory/EntityFactory.php index 3fd35739..97d76fd5 100644 --- a/src/Factory/EntityFactory.php +++ b/src/Factory/EntityFactory.php @@ -183,6 +183,34 @@ class: $entity[\Spameri\Elastic\Model\Insert\PrepareEntityArray::ENTITY_CLASS], ); } + } elseif ( + $attribute->getName() === \Spameri\Elastic\Mapping\ValueCollection::class + ) { + /** @var array{class: class-string} $arguments */ + $arguments = $attribute->getArguments(); + + // The mirror of PrepareEntityArray's ValueCollectionInterface + // branch, which writes value() for each member and so leaves a + // flat list of scalars in the document. Rebuilding one member + // per scalar is the whole of it; there are no nested properties + // to resolve, and looking for them under `field.*` is what this + // used to do by falling through to the tail below - producing an + // empty collection, silently, on every single read. + $propertyValue = new $propertyTypeName(); + + if (\is_array($value)) { + foreach ($value as $item) { + if ($item === null || $item === '') { + continue; + } + + $collectionValue = new $arguments['class']($item); + $propertyValue->add($collectionValue); + + $this->changeSet->markExisting($collectionValue); + } + } + } elseif ( $attribute->getName() === \Spameri\Elastic\Mapping\STIEntity::class ) { diff --git a/src/Mapping/ValueCollection.php b/src/Mapping/ValueCollection.php new file mode 100644 index 00000000..e3aed959 --- /dev/null +++ b/src/Mapping/ValueCollection.php @@ -0,0 +1,24 @@ + $genres + */ + public function __construct( + #[\Spameri\Elastic\Mapping\Entity(class: \Spameri\Elastic\Entity\Property\ElasticId::class)] + public \Spameri\Elastic\Entity\Property\ElasticIdInterface $id, + #[\Spameri\Elastic\Mapping\ValueCollection(class: \SpameriTests\Elastic\Data\Entity\Video\Details\Genre::class)] + public \SpameriTests\Elastic\Data\Entity\Video\Details\GenreCollection $genres, + ) + { + parent::__construct($id); + } + +} diff --git a/tests/SpameriTests/Elastic/Factory/EntityFactory/ValueCollectionTest.phpt b/tests/SpameriTests/Elastic/Factory/EntityFactory/ValueCollectionTest.phpt new file mode 100644 index 00000000..06d5f6c7 --- /dev/null +++ b/tests/SpameriTests/Elastic/Factory/EntityFactory/ValueCollectionTest.phpt @@ -0,0 +1,146 @@ +container->getByType(\Spameri\Elastic\EntityManager::class); + /** @var \Spameri\Elastic\Factory\EntityFactory $entityFactory */ + $entityFactory = $this->container->getByType(\Spameri\Elastic\Factory\EntityFactory::class); + /** @var \Spameri\Elastic\Model\Insert\PrepareEntityArray $prepareEntityArray */ + $prepareEntityArray = $this->container->getByType(\Spameri\Elastic\Model\Insert\PrepareEntityArray::class); + + $entity = new \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection( + new \Spameri\Elastic\Entity\Property\ElasticId('vc-1'), + new \SpameriTests\Elastic\Data\Entity\Video\Details\GenreCollection( + new \SpameriTests\Elastic\Data\Entity\Video\Details\Genre('Action'), + new \SpameriTests\Elastic\Data\Entity\Video\Details\Genre('Science Fiction'), + ), + ); + + $source = $prepareEntityArray->prepare($entity); + + // What the write side puts in the document: a flat list of scalars. + \Tester\Assert::same(['Action', 'Science Fiction'], $source['genres']); + + $hit = new \Spameri\ElasticQuery\Response\Result\Hit( + source: $source, + position: 0, index: '', type: '', id: 'vc-1', score: 0.0, version: 0, + ); + + /** @var \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection $hydrated */ + $hydrated = $entityFactory->create( + $hit, + \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection::class, + $entityManager, + ); + + $genres = []; + foreach ($hydrated->genres as $genre) { + $genres[] = $genre->value(); + } + + \Tester\Assert::same(['Action', 'Science Fiction'], $genres); + } + + + public function testAnEmptyValueCollectionStaysEmpty(): void + { + /** @var \Spameri\Elastic\EntityManager $entityManager */ + $entityManager = $this->container->getByType(\Spameri\Elastic\EntityManager::class); + /** @var \Spameri\Elastic\Factory\EntityFactory $entityFactory */ + $entityFactory = $this->container->getByType(\Spameri\Elastic\Factory\EntityFactory::class); + + $hit = new \Spameri\ElasticQuery\Response\Result\Hit( + source: ['genres' => []], + position: 0, index: '', type: '', id: 'vc-2', score: 0.0, version: 0, + ); + + /** @var \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection $hydrated */ + $hydrated = $entityFactory->create( + $hit, + \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection::class, + $entityManager, + ); + + \Tester\Assert::same(0, \iterator_count($hydrated->genres->getIterator())); + } + + + public function testAnAbsentValueCollectionIsNotAnError(): void + { + /** @var \Spameri\Elastic\EntityManager $entityManager */ + $entityManager = $this->container->getByType(\Spameri\Elastic\EntityManager::class); + /** @var \Spameri\Elastic\Factory\EntityFactory $entityFactory */ + $entityFactory = $this->container->getByType(\Spameri\Elastic\Factory\EntityFactory::class); + + // A document written before the field existed. It has to read as empty + // rather than throw, or one old document takes down a whole index. + $hit = new \Spameri\ElasticQuery\Response\Result\Hit( + source: [], + position: 0, index: '', type: '', id: 'vc-3', score: 0.0, version: 0, + ); + + /** @var \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection $hydrated */ + $hydrated = $entityFactory->create( + $hit, + \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection::class, + $entityManager, + ); + + \Tester\Assert::same(0, \iterator_count($hydrated->genres->getIterator())); + } + + + public function testNullMembersAreNotTurnedIntoValues(): void + { + /** @var \Spameri\Elastic\EntityManager $entityManager */ + $entityManager = $this->container->getByType(\Spameri\Elastic\EntityManager::class); + /** @var \Spameri\Elastic\Factory\EntityFactory $entityFactory */ + $entityFactory = $this->container->getByType(\Spameri\Elastic\Factory\EntityFactory::class); + + $hit = new \Spameri\ElasticQuery\Response\Result\Hit( + source: ['genres' => ['Action', NULL, '', 'Drama']], + position: 0, index: '', type: '', id: 'vc-4', score: 0.0, version: 0, + ); + + /** @var \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection $hydrated */ + $hydrated = $entityFactory->create( + $hit, + \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection::class, + $entityManager, + ); + + $genres = []; + foreach ($hydrated->genres as $genre) { + $genres[] = $genre->value(); + } + + \Tester\Assert::same(['Action', 'Drama'], $genres); + } + +} + +(new ValueCollectionTest())->run();