Skip to content

Hydrate value collections instead of returning them empty - #17

Merged
Spamercz merged 1 commit into
v2.0from
fix/value-collection-hydration
Aug 17, 2026
Merged

Hydrate value collections instead of returning them empty#17
Spamercz merged 1 commit into
v2.0from
fix/value-collection-hydration

Conversation

@Spamercz

Copy link
Copy Markdown
Contributor

The bug

PrepareEntityArray has always known how to write a ValueCollectionInterface: it walks the collection and stores value() for each member, leaving a flat list of scalars in the document.

EntityFactory::resolveProperties() had no matching branch to read one back. The property fell through to the generic "instantiate from nested properties" tail, which looks for the collection's own properties under field.*. A document holding genres: ["Action", "Drama"] has nothing there, so every read produced an empty collection.

Silently. No exception, no warning. The data sits in Elasticsearch, correctly indexed and searchable, and simply never reaches anything that holds an entity.

How it was found

On spameri.cz, 627,507 of 1,581,579 titles carry genres in the index. The public API answered "genres": [] for every one of them, including titles whose stored document plainly reads ["Comedy"]. Every downstream consumer saw a catalogue with no genres at all.

The fix

The read side now mirrors the write side: one value object per stored scalar, and nothing else — 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 with a mapping attribute, the way ElasticCollection already names one:

#[\Spameri\Elastic\Mapping\ValueCollection(class: Genre::class)]
public GenresCollection $genres,

Collections without the attribute keep their current behaviour, so this adds a capability rather than changing one.

Tests

tests/SpameriTests/Elastic/Factory/EntityFactory/ValueCollectionTest.phpt — four cases: the round trip through prepare(), an empty list, an absent field, and null/empty members being skipped rather than turned into values. Three of the four fail on v2.0 without this change.

Verification

  • vendor/bin/tester tests/SpameriTests/Elastic/Factory — 54 passed
  • make phpstan (level 6) — no errors
  • phpcs --standard=ruleset.xml on the touched files — clean
  • Full suite, no local Elasticsearch: 679 tests / 172 failures with the change, 675 / 175 on unmodified v2.0. The failures are the ES-dependent tests CI provides a server for; the count does not rise.

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.
@Spamercz

Copy link
Copy Markdown
Contributor Author

CI comparison against the v2.0 baseline

v2.0 is already red, so here is the like-for-like on the PHP 8.2 - standard job, which runs the suite against a real Elasticsearch:

tests failures
v2.0 (run 29584300236) 675 2
this branch (run 31986265806) 679 1

The four added tests are the four new ones in ValueCollectionTest.phpt, and they pass.

Failures before: EntityManager/ClearTest::testClearRemovesChangeSetTracking and Model/DumpIndex/Execute::testProcess.
Failure after: EntityManager/ClearTest::testClearRemovesChangeSetTracking only — the DumpIndex one passed this run, so it looks flaky rather than fixed. Neither is touched by this change.

The remaining red jobs (PHPStan (lowest deps), the lowest matrix legs) fail identically on v2.0 and are unrelated to this change; make phpstan at level 6 against the pinned dependencies is clean.

@Spamercz
Spamercz merged commit 02ab4ba into v2.0 Aug 17, 2026
1 of 6 checks passed
@Spamercz
Spamercz deleted the fix/value-collection-hydration branch August 17, 2026 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant