fix(search): count only the chunk owners the caller can actually see - #3333
Merged
rubenvdlinde merged 2 commits intoSep 2, 2026
Merged
Conversation
`_content_search` reported a total that included chunk owners it then
refused to return. Measured on the dev instance 2026-09-02, UNAUTHENTICATED,
against OpenCatalogi's #[PublicPage] search:
GET /apps/opencatalogi/api/search?_search=<phrase>&_content=true
-> {"results":[],"total":1}
The owning document was soft-deleted, so the row was correctly withheld and
the count was not. An anonymous caller could probe a phrase and learn from
the count alone that a document containing it exists. A count is an answer,
so it has to obey the same visibility rules as the rows.
The over-count was deliberate and documented: counting the distinct
chunk-owner set BEFORE the resolve loop kept `total` stable across pages,
because the loop is clamped to the page's remaining room. Stability was the
right goal; the upper bound was the wrong way to reach it. Scope mismatch,
RBAC, tenancy and a soft-deleted owner each drop a row without touching the
count.
RESOLVE FIRST, THEN COUNT AND PAGE. Every candidate is resolved, `total`
counts what resolved, and the page is a slice of that same set. Stability is
preserved and is now a property of the query rather than a guess: page 1 and
page 3 resolve the same candidates and report the same number. The existing
cross-page test still passes unchanged.
Cost: up to CHUNK_CANDIDATE_LIMIT resolves per call instead of up to the
page's room. That is the worst case this class already budgets for and
documents on that constant, and `_content_search` is opt-in.
Six tests asserted the over-count as intentional. They are rewritten, not
deleted: each now pins the visibility rule it was documenting the absence
of. The clamping test additionally proves the page is still clamped while
the total still counts the resolvable owner.
Contributor
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 | ✅ | ✅ 174/174 | |||
| npm | ✅ | ✅ 543/543 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-02 11:13 UTC
Download the full PDF report from the workflow artifacts.
Coverage Baseline Protection was the only red on this PR. The change itself removed six covered statements — the distinct-owner upper bound, replaced by a count of the owners the caller can actually see — so the ratio slipped 96.33% -> 96.12% without any new uncovered code. The honest way back up is to cover something real rather than to reinstate dead statements. resolveScope() reads four keys per dimension, and this file exercised '_register' (singular) and '_schemas' (plural) — one of each, never the other diagonal. So a caller passing '_registers' or '_schema' got a scope assembled by branches nothing had ever run. Both are added, and both directions: the plural register list narrowing to a match, and an object outside it being skipped. The second is what makes the first mean anything, since an empty scope matches everything and a '_registers' branch that silently contributed nothing would pass the match test on its own.
Contributor
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 | ✅ | ✅ 174/174 | |||
| npm | ✅ | ✅ 543/543 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-02 11:30 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
deleted the
fix/content-search-total-counts-only-visible-owners
branch
September 2, 2026 11:31
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
_content_searchreported a total that included chunk owners it then refused to return.Measured on the dev instance 2026-09-02, unauthenticated, against OpenCatalogi's
#[PublicPage]search:The owning document was soft-deleted, so the row was correctly withheld and the count was not. An anonymous caller could probe a phrase and learn from the count alone that a document containing it exists. A count is an answer, so it has to obey the same visibility rules as the rows.
Why it was like that
The over-count was deliberate and documented. Counting the distinct chunk-owner set before the resolve loop kept
totalstable across pages, because that loop is clamped to the page's remaining room. Stability was the right goal; the upper bound was the wrong way to reach it. Scope mismatch, RBAC, tenancy and a soft-deleted owner each drop a row without touching the count.The change
Resolve first, then count and page. Every candidate is resolved,
totalcounts what resolved, and the page is a slice of that same set.Stability is preserved, and is now a property of the query rather than a guess: page 1 and page 3 resolve the same candidates and report the same number. The existing cross-page test (
53across page one and page three) passes unchanged.Cost, stated plainly
Up to
CHUNK_CANDIDATE_LIMIT(50) resolves per call instead of up to the page's room. That is the worst case this class already budgets for and documents on that constant, and_content_searchis opt-in.Verified
total: 1, rows: 0total: 0, rows: 0total: 1, rows: 1total: 1, rows: 1Both measured live. The positive control matters: a fix that just zeroed the count would pass the first row and fail the second.
Six tests asserted the over-count as intentional. They are rewritten, not deleted — each now pins the visibility rule it was documenting the absence of. The clamping test additionally proves the page is still clamped while the total still counts the resolvable owner.
PHPUnit 14/14 plus the sibling
QueryHandlerContentSearchTest12/12; PHPCS, PHPStan, PHPMD and Psalm clean.🤖 Generated with Claude Code