fix(search): page the chunk arm behind the metadata arm and count each owner once (WOO-577) - #3856
WilcoLouwerse wants to merge 4 commits into
Conversation
…h owner once (WOO-577) With `_content_search` the result is one combined list: the metadata rows first, the chunk-only owners behind them. Two things broke that. The chunk arm had no offset, so every page past the metadata rows re-served the same chunk-only owners and a client walking `_page` never reached the end. And the overlap between the arms was computed against THIS PAGE's metadata rows, so an owner the metadata arm serves on page 2 was counted as chunk-only on page 1 and dropped on page 2 — `total` moved from page to page. Measured 2026-09-17 on a NC 32 rig with OpenCatalogi 2.1.0 in front: `_search=Klimaatakkoord&_content=true&_limit=1` answered total 4, 5, 4, 5, ... and page 4 onward returned the same document indefinitely. The chunk arm now starts at `offset - metadataTotal` and is sliced from there, so paging ends. The overlap is asked of the metadata arm itself: the caller's own query — same term, scope and guards — restricted to the resolved chunk owners via the `ids:` argument and stripped of paging, bounded by CHUNK_CANDIDATE_LIMIT ids. Its answer is a property of the query, not of the page, so `total` is the same on every page and equals the number of distinct objects a client collects by walking to the end. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| test-l10n-parity | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 170/170 | |||
| npm | ✅ | ✅ 654/654 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-17 13:57 UTC
Download the full PDF report from the workflow artifacts.
…to helpers phpmd flagged the grown augmentWithChunkMatches() and metadataArmOverlap(): method length, class complexity, cyclomatic and NPath. The reasoning moves into the docblock where it reads better anyway, and the body splits into pageChunkArm(), groupOwnersByTable() and probeQuery(). No behaviour change: 36 unit tests unchanged and green, and the rig still answers total 3 on every page with 3 distinct objects and an empty page 4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| test-l10n-parity | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 170/170 | |||
| npm | ✅ | ✅ 654/654 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-17 14:18 UTC
Download the full PDF report from the workflow artifacts.
The anchor trusts the metadata arm's `total`, which comes from a different query than its rows. When those disagree the chunk arm either repeats its first owner or hides rows past the stated total, and this method cannot tell — it sees one page, not the arm. Written down where the next reader will be standing, with the note that the fix belongs where the disagreement is rather than in a per-page correction. Raised by the review pass on this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| test-l10n-parity | ✅ | ||||
| format | ❌ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 170/170 | |||
| npm | ✅ | ✅ 654/654 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-18 10:13 UTC
Download the full PDF report from the workflow artifacts.
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Verdict: APPROVE (Thorough) — self-review posted as COMMENT (GitHub blocks self-APPROVE)
The paging arithmetic and the probe hold up. One concern, documented rather than fixed, because the fix does not belong in this method.
🟡 The chunk-arm anchor trusts a count that can disagree with its own rows
pageChunkArm() anchors on max(0, $offset - $metadataTotal). $metadataTotal comes from a separate COUNT query than the one that produced the rows, so the two can disagree — a write landing between the round-trips, or a count and a fetch built by different code paths.
Traced with the exact formula, metadataTotal claiming 5 against 3 real rows, limit=1:
offset=3 → C0
offset=4 → C0 duplicate
offset=5 → C0 duplicate
offset=6 → C1
And the inverse (claiming 2 against 5 real rows): total reports 4, so a client paging on total stops early and never reaches two real metadata rows or either chunk-only row.
Before this PR a count/row mismatch was bounded pagination drift of roughly a row; the anchor spreads it across the chunk arm. This method cannot detect it — it sees one page, not the arm. Correcting it here would mean guessing per page, which is how the bug this PR fixes was written in the first place. The honest fix is the count and the fetch agreeing, which is a different change in MagicMapper. Written into the docblock in efa4b51a so the next reader is standing in the right place, and worth its own ticket.
Verified clean, with the checks that matter
- The reverts prove the tests. Reverting
max(0, $offset - $metadataTotal)to0failstestTheChunkArmIsPagedByTheOffsetPastTheMetadataArmAndEnds; reverting the overlap to this page's rows fails bothtestAnOwnerTheMetadataArmMatchesOnAnotherPageIsNotCountedAsChunkOnlyandtestOneProbePerTableTheOwnersLiveIn. Both restored; 24 tests green at head. - The id-lookup shortcut is genuinely avoided. This was the load-bearing claim. In
MagicMapper, the single register/schema branch returns before the_ids-drivenfindMultipleAcrossAllMagicTablespath, so setting_register+_schemaroutes the probe onto the single-table path, where_idsis ANDed with the search term viaapplyIdFilters. That is why the second attempt at this probe (passingids:on the multi-schema path) silently reported the first two metadata rows as overlap. - The strip list is complete.
_registers/_schemas/register/schema/_idsare unset; the plural non-underscoreregisters/schemasare harmless becauseFilterParams::OBJECT_CONTEXT_PARAMSexcludes them from field-filter treatment._relations_contains,_order/_sortand_includeDeletedare deliberately kept — they mirror the caller's own narrowing, which is what a probe asking "would the caller's query match this owner" needs. - Cost degenerates correctly. No groups means no probe, so a query whose candidates are all scope-filtered pays nothing.
_limit = count($uuids)is an exact upper bound per group. limit=0never reachespageChunkArm—augmentWithChunkMatchesreturns early. Its$room = nullbranch is dead defensive code, andarray_slice($arr, $offset, null, true)does mean "no length limit", so the intent matches the semantics.- One live caller.
grep -rn augmentWithChunkMatches lib/finds onlyQueryHandler, so the two new defaulted parameters cannot silently change another caller.$offsetthere is the same combined-list offset the metadata arm pages on, and$activeOrgUuidis null exactly when multitenancy is off.
Pre-existing, noted not fixed
augmentWithChunkMatches never receives the ids/uses parameters searchObjectsPaginatedDatabase accepts, so a caller restricting by those alongside _content_search has the chunk fan-out ignore the restriction. That predates this PR — the signature lacked them on development too.
CI
Green at the time of writing apart from runs still in flight.
🤖 Generated with Claude Code
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| test-l10n-parity | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 170/170 | |||
| npm | ✅ | ✅ 654/654 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-18 15:11 UTC
Download the full PDF report from the workflow artifacts.
Why
With
_content_searcha search result is one combined list: the metadata-matched rows first, the chunk-only owners behind them. Two things broke that inContentSearchHandler::augmentWithChunkMatches():_pagenever reached the end.totalmoved from page to page.Measured 2026-09-17 on a NC 32 rig with OpenCatalogi 2.1.0 in front of
development:_search=Klimaatakkoord&_content=true&_limit=1answeredtotal4, 5, 4, 5, … and page 4 onward returned the same document indefinitely. Same rig after this change:total3 on every page, 3 distinct objects collected, page 4 empty.Jira: WOO-577 (sub-task of WOO-572). OpenCatalogi side (paging guards + tests) is ConductionNL/opencatalogi#1526.
What
offset - metadataTotaland is sliced from there, so paging ends._ids, stripped of paging, one probe per (register, schema) the owners live in. Its answer is a property of the query, not of the page, sototalis stable across pages and equals the number of distinct objects a client collects by walking to the end. Bounded byCHUNK_CANDIDATE_LIMITids in total.QueryHandlerpassesoffsetand the active organisation through.An observation for a follow-up (not fixed here)
MagicMapper::searchObjectsPaginated()accepts anids:argument and both its single-schema and multi-schema paths ignore it; the multi-schema UNION path also ignores a_idsquery key (a probe over three tables came back asLIMIT 2without a uuid predicate, which is how this PR's first attempt reported the first two metadata rows as the overlap). The per-table probe is the one path on which the restriction is honoured. Worth a ticket.Tests
ContentSearchHandlerTest: existing tests kept (the metadata-dedup test now feeds the probe); new tests for offset paging that ends, for an owner the metadata arm matches on another page, for the probe's shape (same query,_ids, no paging, per-table) and for the no-candidates/no-probe case. 36 tests green; phpstan and phpcs clean.🤖 Generated with Claude Code