Skip to content

Commit e8a3126

Browse files
SpamerczSpamercz
committed
fix(factory): a value collection comes back with its values in it
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.
1 parent c6984f4 commit e8a3126

4 files changed

Lines changed: 226 additions & 0 deletions

File tree

‎src/Factory/EntityFactory.php‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,34 @@ class: $entity[\Spameri\Elastic\Model\Insert\PrepareEntityArray::ENTITY_CLASS],
183183
);
184184
}
185185

186+
} elseif (
187+
$attribute->getName() === \Spameri\Elastic\Mapping\ValueCollection::class
188+
) {
189+
/** @var array{class: class-string} $arguments */
190+
$arguments = $attribute->getArguments();
191+
192+
// The mirror of PrepareEntityArray's ValueCollectionInterface
193+
// branch, which writes value() for each member and so leaves a
194+
// flat list of scalars in the document. Rebuilding one member
195+
// per scalar is the whole of it; there are no nested properties
196+
// to resolve, and looking for them under `field.*` is what this
197+
// used to do by falling through to the tail below - producing an
198+
// empty collection, silently, on every single read.
199+
$propertyValue = new $propertyTypeName();
200+
201+
if (\is_array($value)) {
202+
foreach ($value as $item) {
203+
if ($item === null || $item === '') {
204+
continue;
205+
}
206+
207+
$collectionValue = new $arguments['class']($item);
208+
$propertyValue->add($collectionValue);
209+
210+
$this->changeSet->markExisting($collectionValue);
211+
}
212+
}
213+
186214
} elseif (
187215
$attribute->getName() === \Spameri\Elastic\Mapping\STIEntity::class
188216
) {

‎src/Mapping/ValueCollection.php‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Spameri\Elastic\Mapping;
4+
5+
/**
6+
* A collection of value objects, stored as a flat list of scalars.
7+
*
8+
* The class is named for the same reason ElasticCollection names one: the
9+
* document holds ["Action", "Drama"] and nothing else, so the value type
10+
* cannot be recovered from what was written. Without it the collection can be
11+
* written but never read back.
12+
*/
13+
#[\Attribute(\Attribute::TARGET_PROPERTY|\Attribute::TARGET_PARAMETER)]
14+
class ValueCollection
15+
{
16+
17+
18+
public function __construct(
19+
public string $class,
20+
)
21+
{
22+
}
23+
24+
}

‎tests/SpameriTests/Elastic/Data/Entity/EntityWithValueCollection.php‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace SpameriTests\Elastic\Data\Entity;
4+
5+
/**
6+
* Entity with a collection of value objects, stored as a flat list of scalars.
7+
*
8+
* Distinct from EntityWithCollection, whose members are nested objects with
9+
* properties of their own: a value collection round-trips through
10+
* ["Action", "Drama"], not through a list of documents.
11+
*/
12+
class EntityWithValueCollection extends \Spameri\Elastic\Entity\AbstractElasticEntity
13+
{
14+
15+
/**
16+
* @param \SpameriTests\Elastic\Data\Entity\Video\Details\GenreCollection<\SpameriTests\Elastic\Data\Entity\Video\Details\Genre> $genres
17+
*/
18+
public function __construct(
19+
#[\Spameri\Elastic\Mapping\Entity(class: \Spameri\Elastic\Entity\Property\ElasticId::class)]
20+
public \Spameri\Elastic\Entity\Property\ElasticIdInterface $id,
21+
#[\Spameri\Elastic\Mapping\ValueCollection(class: \SpameriTests\Elastic\Data\Entity\Video\Details\Genre::class)]
22+
public \SpameriTests\Elastic\Data\Entity\Video\Details\GenreCollection $genres,
23+
)
24+
{
25+
parent::__construct($id);
26+
}
27+
28+
}

‎tests/SpameriTests/Elastic/Factory/EntityFactory/ValueCollectionTest.phpt‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace SpameriTests\Elastic\Factory\EntityFactory;
4+
5+
require_once __DIR__ . '/../../../../bootstrap.php';
6+
7+
/**
8+
* A value collection is written as a flat list of scalars and has to come back
9+
* as the same list of value objects.
10+
*
11+
* PrepareEntityArray has always known how: it walks a ValueCollectionInterface
12+
* and writes value() for each member. EntityFactory did not, so the property
13+
* fell through to the generic "instantiate from nested properties" tail, where
14+
* the collection's own properties are looked for under `genres.*` — nothing is
15+
* stored there, because the document holds `genres: ["Action"]` — and every
16+
* read produced an empty collection.
17+
*
18+
* Nothing reported an error. The data was in Elasticsearch, indexed and
19+
* searchable, and simply never reached anything that read an entity.
20+
*
21+
* @testCase
22+
*/
23+
class ValueCollectionTest extends \SpameriTests\Elastic\AbstractTestCase
24+
{
25+
26+
public function testValueCollectionSurvivesTheRoundTrip(): void
27+
{
28+
/** @var \Spameri\Elastic\EntityManager $entityManager */
29+
$entityManager = $this->container->getByType(\Spameri\Elastic\EntityManager::class);
30+
/** @var \Spameri\Elastic\Factory\EntityFactory $entityFactory */
31+
$entityFactory = $this->container->getByType(\Spameri\Elastic\Factory\EntityFactory::class);
32+
/** @var \Spameri\Elastic\Model\Insert\PrepareEntityArray $prepareEntityArray */
33+
$prepareEntityArray = $this->container->getByType(\Spameri\Elastic\Model\Insert\PrepareEntityArray::class);
34+
35+
$entity = new \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection(
36+
new \Spameri\Elastic\Entity\Property\ElasticId('vc-1'),
37+
new \SpameriTests\Elastic\Data\Entity\Video\Details\GenreCollection(
38+
new \SpameriTests\Elastic\Data\Entity\Video\Details\Genre('Action'),
39+
new \SpameriTests\Elastic\Data\Entity\Video\Details\Genre('Science Fiction'),
40+
),
41+
);
42+
43+
$source = $prepareEntityArray->prepare($entity);
44+
45+
// What the write side puts in the document: a flat list of scalars.
46+
\Tester\Assert::same(['Action', 'Science Fiction'], $source['genres']);
47+
48+
$hit = new \Spameri\ElasticQuery\Response\Result\Hit(
49+
source: $source,
50+
position: 0, index: '', type: '', id: 'vc-1', score: 0.0, version: 0,
51+
);
52+
53+
/** @var \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection $hydrated */
54+
$hydrated = $entityFactory->create(
55+
$hit,
56+
\SpameriTests\Elastic\Data\Entity\EntityWithValueCollection::class,
57+
$entityManager,
58+
);
59+
60+
$genres = [];
61+
foreach ($hydrated->genres as $genre) {
62+
$genres[] = $genre->value();
63+
}
64+
65+
\Tester\Assert::same(['Action', 'Science Fiction'], $genres);
66+
}
67+
68+
69+
public function testAnEmptyValueCollectionStaysEmpty(): void
70+
{
71+
/** @var \Spameri\Elastic\EntityManager $entityManager */
72+
$entityManager = $this->container->getByType(\Spameri\Elastic\EntityManager::class);
73+
/** @var \Spameri\Elastic\Factory\EntityFactory $entityFactory */
74+
$entityFactory = $this->container->getByType(\Spameri\Elastic\Factory\EntityFactory::class);
75+
76+
$hit = new \Spameri\ElasticQuery\Response\Result\Hit(
77+
source: ['genres' => []],
78+
position: 0, index: '', type: '', id: 'vc-2', score: 0.0, version: 0,
79+
);
80+
81+
/** @var \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection $hydrated */
82+
$hydrated = $entityFactory->create(
83+
$hit,
84+
\SpameriTests\Elastic\Data\Entity\EntityWithValueCollection::class,
85+
$entityManager,
86+
);
87+
88+
\Tester\Assert::same(0, \iterator_count($hydrated->genres->getIterator()));
89+
}
90+
91+
92+
public function testAnAbsentValueCollectionIsNotAnError(): void
93+
{
94+
/** @var \Spameri\Elastic\EntityManager $entityManager */
95+
$entityManager = $this->container->getByType(\Spameri\Elastic\EntityManager::class);
96+
/** @var \Spameri\Elastic\Factory\EntityFactory $entityFactory */
97+
$entityFactory = $this->container->getByType(\Spameri\Elastic\Factory\EntityFactory::class);
98+
99+
// A document written before the field existed. It has to read as empty
100+
// rather than throw, or one old document takes down a whole index.
101+
$hit = new \Spameri\ElasticQuery\Response\Result\Hit(
102+
source: [],
103+
position: 0, index: '', type: '', id: 'vc-3', score: 0.0, version: 0,
104+
);
105+
106+
/** @var \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection $hydrated */
107+
$hydrated = $entityFactory->create(
108+
$hit,
109+
\SpameriTests\Elastic\Data\Entity\EntityWithValueCollection::class,
110+
$entityManager,
111+
);
112+
113+
\Tester\Assert::same(0, \iterator_count($hydrated->genres->getIterator()));
114+
}
115+
116+
117+
public function testNullMembersAreNotTurnedIntoValues(): void
118+
{
119+
/** @var \Spameri\Elastic\EntityManager $entityManager */
120+
$entityManager = $this->container->getByType(\Spameri\Elastic\EntityManager::class);
121+
/** @var \Spameri\Elastic\Factory\EntityFactory $entityFactory */
122+
$entityFactory = $this->container->getByType(\Spameri\Elastic\Factory\EntityFactory::class);
123+
124+
$hit = new \Spameri\ElasticQuery\Response\Result\Hit(
125+
source: ['genres' => ['Action', NULL, '', 'Drama']],
126+
position: 0, index: '', type: '', id: 'vc-4', score: 0.0, version: 0,
127+
);
128+
129+
/** @var \SpameriTests\Elastic\Data\Entity\EntityWithValueCollection $hydrated */
130+
$hydrated = $entityFactory->create(
131+
$hit,
132+
\SpameriTests\Elastic\Data\Entity\EntityWithValueCollection::class,
133+
$entityManager,
134+
);
135+
136+
$genres = [];
137+
foreach ($hydrated->genres as $genre) {
138+
$genres[] = $genre->value();
139+
}
140+
141+
\Tester\Assert::same(['Action', 'Drama'], $genres);
142+
}
143+
144+
}
145+
146+
(new ValueCollectionTest())->run();

0 commit comments

Comments
 (0)