docs(content-drive): spec for field-filter chunk multiplier (#37184) - #37188
docs(content-drive): spec for field-filter chunk multiplier (#37184)#37188ihoffmann-dot wants to merge 4 commits into
Conversation
|
Claude finished @ihoffmann-dot's task in 2m 12s —— View job Review — spec vs. round-2 feedback (commit
|
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Reviewed against main at 88af0bad55, and against ADR-0018 in platform-adrs. (Paraphrasing the ADR rather than quoting it — this repo is public and that one isn't. Section names given so an internal reader can check me.)
Structure is complete, User Story 2 and FR-005/FR-008 protect the DB-routed combinations properly, and the Dependencies & Coordination section correctly identifies the shared query with item 1. The Assumption about which field types route where is accurate — I verified ContentDriveFieldFilterResolver#routingBucketFor (:216-219) sends Tag and Relationship to DB and Text/Select/Boolean/Date/Category to INDEX.
My concern is that the spec never names which implementation it means, and the two candidates have opposite compliance profiles against ADR-0018. Its own success criterion pushes toward the non-compliant one.
The two implementations FR-002 allows
FR-002 requires the DB scan to run "at most once per request, regardless of folder size or how sparse the matches are." There are two ways to get there:
(i) Stay hybrid; fetch all candidates in one DB query instead of chunking. ADR-0018-compliant — the DB still drives the candidate set, ordering, pagination and permissions. But for a 20k folder you materialize 20k inodes and then have to ES-filter them, and the ES boolean-clause ceiling is 1024 (calculateMaxInodesPerESQuery, BrowserAPIImpl.java:797-823), so processMultipleESQueries subdivides into roughly two dozen ES round trips. You trade 4 DB scans for 1 large scan plus ~24 index queries. BROWSER_DB_MAX_SCAN_ROWS (default 50,000, :735) caps the scan.
(ii) Route to PURE_ES. This is what #37148 item 2 actually proposes — "the existing PURE_ES heuristic could serve the query directly instead." See below.
Everything in Legacy Considerations reads as if (i) is meant. Everything in the parent issue and in SC-002 points at (ii). The spec never picks, and almost every other question depends on which it is.
On the PURE_ES path, FR-003 / FR-004 / FR-007 are new work, not preservation
The spec states these three as behaviour to keep. On that path they don't exist today:
| Spec requirement | PURE_ES as implemented |
ADR-0018 routing table |
|---|---|---|
| FR-003 "MUST still be scoped to the folder … never return content from outside the requested folder" | buildPureESQuery (:606-692) emits conhost only — the site. There is no conFolder clause anywhere in it. The inline comment says "Host/folder filter"; the code filters host. The method doc says "without inode filtering". |
assigns parent folder / path to DB, as structural and authoritative |
| FR-004 permission filter "sourced from the database, after candidates are retrieved, for every request" | permissions come from contentletAPI.search(esQuery, …, user, respectFrontEndRoles) (:571-578) — index-side, not a DB filterCollection after candidate selection |
assigns the READ-permission filter to DB, applied after candidate selection |
| FR-007 "same items, same order, same pagination behavior" | ordering via browserQuery.sortBy, pagination via startRow + indexCount (:581-585) — not the DB cursor (contentCursor / generateNextContentCursor) the hybrid path uses |
assigns sorting and pagination to DB, on the grounds that only the DB sees every row |
The ADR is also explicit that PURE_ES is available behind configuration but must not become the default, because it surrenders read-your-writes for all criteria — and that the DB-resolved criteria must never be silently re-routed to the index for speed, the remedy for slowness being a DB index or query change instead. Making PURE_ES the automatic path for a whole request class, shipped default-on with no flag (FR-009), is close to the scenario that clause exists to prevent. Whether it counts as "becoming the default" is a question for the ADR's decision-makers, not one the spec can settle by assertion.
Separately, PURE_ES throws today when field criteria are present:
// :493-501
// PURE_ES bypasses the DB select entirely and doesn't build per-field clauses,
// so it can neither apply DB-routed (Tag) predicates nor index-routed field clauses.
if (!browserQuery.getFieldCriteria().isEmpty()) {
throw new DotRuntimeException("Content Drive field filters (userSearchable) "
+ "are not supported under the PURE_ES heuristic; use HYBRID_SINGLE_CHUNKED_QUERY_ES.");
}So the closing Assumption — "the existing per-field index query logic … is assumed to be reusable as-is for the single-pass case; no new field-to-index translation logic is expected to be needed" — is half right. buildFieldCriteriaESClauses is reusable. The path that would host it refuses field criteria by design and has no folder scoping. That is not a no-op.
The freshness trade-off being justified already exists
Assumptions spends its longest resolution defending "accepting a brief, index-lag-bounded delay before a just-written item appears in a field-filter-only search" as a deliberate, scoped, new trade-off.
But that is already today's behaviour. In the hybrid loop, getChunkFiltered(browserQuery, true, …) narrows each DB chunk with an ES query that already carries the index-routed field clauses (buildBaseESQuery → buildFieldCriteriaESClauses, :1238, :1262-1275). A just-written, not-yet-indexed item is already excluded from a field-filtered result. ADR-0018's "defer to the index" section mandates exactly that for searchable fields.
So there is no new concession on that axis and no product decision needed for it — good news for the change. The cost is that the spec spends its whole risk budget here and consequently never examines the three things that would change under (ii): folder scoping, permission sourcing, and ordering/pagination.
SC-002 is unreachable under (i) until item 1 lands
SC-002 asks for response time within 20% of the closest equivalent content-search operation. Per #37148 that baseline is 117 ms against Drive's 705 ms, so the target is ~140 ms.
Under (i), a single candidate scan of a 20k folder costs 450–461 ms today. SC-002 is arithmetically unreachable. It becomes reachable only once item 1's folder-first CTE brings that scan to 101–108 ms.
Dependencies & Coordination does flag item 1, but frames it as "lowers this case's exposure to item 1's problem" rather than "SC-002 is unachievable without it". As written, an implementer chasing SC-002 with item 1 unlanded is pushed straight to PURE_ES. Item 1's spec (#37230) is still open.
Smaller items
FR-009's escape hatch doesn't work for the case it guards. FR-009 declines a kill switch because BROWSE_API_HEURISTIC_TYPE "remains the escape hatch of last resort if the single-pass path needs to be disabled entirely." The only two legal values are HYBRID_SINGLE_CHUNKED_QUERY_ES and PURE_ES (:460-465), defaulting to hybrid (:697). If the single-pass lands inside the hybrid branch, switching to PURE_ES doesn't disable it — it throws on any request carrying field criteria (:496-501). There is no value that yields "hybrid, previous chunked behaviour."
"0 open clarifications" overstates where the decisions stand. Two were resolved inline on 2026-08-24, both classified as technical calls needing no product sign-off. Even granting that the freshness relaxation is narrower than claimed (above), the routing question — does a whole request class stop being DB-first — is squarely an ADR question, and ADR-0018 has named decision-makers. That belongs as an open clarification addressed to them, not a resolved one.
No test type is named. SC-001 counts candidate-scan iterations and SC-003/SC-004 assert "100% of tested combinations", but nothing says what runs them or whether any of it is reachable without the 20k-folder dataset. Worth noting ContentDriveFieldFilterTest already exists in dotcms-integration and its own header says it asserts the ADR-0018 routing contract (:66) — that is the natural home, and Principle V wants the tests written and failing first.
Stale javadoc worth fixing while nearby. FieldSearchCriteria.RoutingBucket.DB is documented as "Resolved against the database (Tag)" (:42-43), but the resolver routes Tag and Relationship to DB. The spec's Assumption gets this right; the enum comment doesn't.
Verified, no action needed
- Tag / Relationship →
DB, Text / Select / Boolean / Date / Category →INDEX(ContentDriveFieldFilterResolver.java:210-229) ✅ buildFieldCriteriaESClausesskips non-INDEXcriteria (:1268);appendFieldCriteriaDBPredicatesskips non-DB(:2210) ✅- Chunk size 900 (
BROWSER_CONTENT_CHUNK_SIZE,:547); ~3,337 candidates over 900 ≈ 4 chunks — consistent with SC-001's "four to at most one" ✅ BROWSE_API_HEURISTIC_TYPEexists, defaults toHYBRID_SINGLE_CHUNKED_QUERY_ES(:697) ✅- ADR-0018 correctly identified as binding, and the spec is right that whether structural filtering should move to the index is already settled by it and out of scope here ✅
- All four mandatory sections of the dotCMS override template present, plus Assumptions ✅
Adjacent, not in scope while PURE_ES stays off: buildPureESQuery's text clause still uses the broad leading-wildcard catchall:*<kw>* form (:658-663) that the hybrid path deliberately replaced under #36688 (see the comment at :1191-1198). It's evidence that PURE_ES is unmaintained, which matters if the fix intends to route real traffic through it.
What I'd ask for
- Name the implementation — (i) a single large DB scan inside the hybrid strategy, or (ii)
PURE_ES/ a new index-only path. The spec is currently readable both ways and most of the above depends on it. - If (ii): reopen as an ADR-0018 question for its decision-makers, and rewrite FR-003/FR-004/FR-007 as requirements to build rather than behaviour to preserve.
- If (i): state that SC-002 depends on item 1 landing first, and add an acceptance criterion on ES round-trip count — that's what (i) trades the DB scans for.
- Drop or rescope the freshness trade-off in Assumptions; it's already shipped behaviour and already mandated by the ADR.
- Fix FR-009 — either identify a real disable path or say plainly that there isn't one.
Sequencing note, same as the sibling specs: #37148 is closed and its guidance was to land item 1 first and then re-evaluate whether items 2–4 are still needed. Here the dependency isn't only sequencing — per SC-002 above it determines whether this item's own success criterion is achievable without stepping outside the ADR.
|
@fabrizzio-dotCMS fair point, it was ambiguous. Named the implementation explicitly: hybrid single-scan (FR-002 unchanged in substance), not PURE_ES — verified |
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Round 2 — re-review of b9cce8c5
Re-reviewed against main @ 340c703feb.
Thanks — ask 1 is answered properly, and I verified the answer. Naming implementation (i) hybrid single-scan, and rejecting index-only for the reason you give, collapses B1, B2 and essentially all of the ADR-0018 exposure. That was the whole point of the round-1 review and it's resolved.
Two things though. FR-009's correction replaced a wrong claim with a differently wrong one, and the new version is the direction that could bite in production. And naming (i) sharpened a cost the spec still doesn't measure rather than closing it.
✅ Resolved
Ask 1 — and your rejection rationale reproduces. buildPureESQuery (BrowserAPIImpl.java:610) emits +conhost:<hostId> (plus the SYSTEM_HOST variant) and no conFolder / parentPath clause — the comment says "Host/folder filter", the code filters host. Your reason for rejecting index-only is the real one. ✅
Ask 2 — moot under (i). FR-003/FR-004/FR-007 genuinely are preservation now. ✅
Ask 3, first half. SC-002's dependency on #37230 is stated plainly (:160-168), and the FR-002-ships-independently / SC-002-doesn't split is a useful way to put it. ✅ Worth noting #37230 is still open with only a COMMENTED review, so SC-002 is gated behind a spec that hasn't cleared its own gate.
S3 — test home named (:224-227). ✅ Partially: ContentDriveFieldFilterTest already asserts the ADR-0018 routing contract (:66) and is the natural home for the FR-005/FR-008 DB-routed parity cases. Worth naming both.
🔴 FR-009's correction is wrong, and it's the dangerous direction
FR-009 now says (:135-139) that BROWSE_API_HEURISTIC_TYPE "does not apply here — it toggles … for the free-text-filtering code path (doElasticSearchTextFiltering) only, and has no effect on getContentByChunks's repeat-scan loop, which is what FR-002 changes."
Traced on main, the config gates the FR-002 path directly:
1. isUseElasticSearchForFiltering (:1582-1589):
final boolean hasTextFilter = UtilMethods.isSet(browserQuery.filter) || UtilMethods.isSet(browserQuery.fileName);
final boolean hasIndexFieldCriteria = browserQuery.getFieldCriteria().stream()
.anyMatch(c -> c.getBucket() == FieldSearchCriteria.RoutingBucket.INDEX);
return browserQuery.useElasticsearchFiltering && (hasTextFilter || hasIndexFieldCriteria);FR-002's case — index-routed field criteria, no free-text term — is exactly hasIndexFieldCriteria == true. Returns true.
2. So it goes to doElasticSearchTextFiltering (:204). Despite the name that method isn't a free-text path — it's the heuristic dispatcher, switching on HEURISTIC_TYPE (:488-497). I'd guess the name is what caused this.
3. HYBRID_SINGLE_CHUNKED_QUERY_ES → doHybridSingleChunkedQueryES (:549-556) → getContentByChunks(…, chunkSize = BROWSER_CONTENT_CHUNK_SIZE (900), applyESFilter = true).
That is the repeat-scan loop FR-002 changes, and 900 is the chunk size SC-001's "~3,337 candidates → four chunks" arithmetic rests on. There's no other route to it for this request shape.
And the config isn't inert here — it's a landmine. Flipping to PURE_ES doesn't leave FR-002's behaviour alone, it makes every such request throw (:496-505), since field criteria are non-empty by the FR's own precondition:
if (!browserQuery.getFieldCriteria().isEmpty()) {
throw new DotRuntimeException("Content Drive field filters (userSearchable) "
+ "are not supported under the PURE_ES heuristic; use HYBRID_SINGLE_CHUNKED_QUERY_ES.");
}The comment right above it (:501-502) says "Content Drive runs the default HYBRID heuristic, so this only trips on misconfiguration." FR-009 as written tells an operator the config is irrelevant to this path — which is exactly the reading under which someone sets PURE_ES for an unrelated reason and turns every field-filtered Drive request into a 500.
Your conclusion is right (no escape hatch exists) — that was round-1 S1. It's the reason that needs replacing, roughly:
BROWSE_API_HEURISTIC_TYPEdoes gate this path, but neither value yields "hybrid with the pre-fix chunked behaviour":HYBRID_SINGLE_CHUNKED_QUERY_ESis the new single-pass, andPURE_ESthrows on any request carrying field criteria. So there's no existing escape hatch — which is an argument for considering a new flag, not a note that none is needed.
🔴 "At most one scan" deletes an early exit, and SC-001 rewards deleting it
Naming (i) makes this concrete. getContentByChunks (:250-325) isn't a fixed four-pass loop — it short-circuits:
| Break condition | Line |
|---|---|
accumulatedContent.size() >= maxRows |
:295-300 |
| partial chunk → DB exhausted | :302-307 |
dbOffset >= scanLimit (BROWSER_DB_MAX_SCAN_ROWS, default 50,000) |
:286-293, :254, :739-740 |
The ~4 chunks in SC-001 are the sparse-match worst case. When the filter matches densely — most of the folder, page size 40 — the loop breaks after chunk one today.
FR-002 requires at most one scan "regardless of folder size or how sparse the matches are." Getting there means fetching the candidate set in one query instead of 900 rows at a time — which for the dense case swaps a 900-row fetch for one of up to 50,000, plus ES filtering and getContentFilteredByRole over all of it. SC-001 rewards that unconditionally: it counts DB scans and asks for "four to at most one", with nothing saying the cases already resolved in one chunk mustn't get worse.
Suggest an FR or SC saying the single-pass must not increase work for requests the current early exit already satisfies in one chunk — or scoping FR-002 to the sparse case it was written for.
🟠 Ask 3's second half still open — and now quantifiable
Nothing bounds the ES fan-out that (i) trades the DB scans for. With numbers from the code:
calculateMaxInodesPerESQuery(:802-828): 1024 clause limit, reservemax(50, baseClauses+20), ×0.9, floor 100 → ~876 inodes per ES query typically (1024−50 = 974 × 0.9 = 876).processESDirectly(:784-794) sends one query under that, elseprocessMultipleESQueries(:904-…) splits intoceil(n/876)sub-queries, run in parallel onDotConcurrentFactory.getSubmitter(), eachorTimeout(60, SECONDS).
So today, chunk size 900 > 876 — every chunk already fans out to 2 ES queries; four chunks ≈ 8. Under (i) on a 20k folder: ceil(20000/876) = 23 parallel ES sub-queries per request, each up to 60s, on a shared pool.
That's the trade SC-001 rewards and nothing observes. The concurrency shape matters independently of latency: 23 parallel ES queries per request on a shared submitter, with several concurrent Drive users, is pool contention — a different failure mode from 4 sequential DB scans.
🟠 Ask 4 untouched, and it now contradicts the named implementation
Assumptions (:212-220) still frames the freshness delay as "a deliberate, scoped trade-off for this one case." Under (i) — now explicitly named — getChunkFiltered (:335-348) already routes every candidate chunk through processESDirectly before permission filtering, so a just-written, not-yet-indexed item is already excluded today. Collapsing four scans to one concedes nothing new. Rescope to "no change to current freshness behaviour."
Not just cosmetic: FR-009's justification rests on that trade-off being narrow-but-real. Combined with the item above, FR-009 now has two broken links — the trade-off it guards doesn't exist, and the escape hatch it cites doesn't behave as described. The answer is probably still "no flag", but for the much simpler reason that nothing about freshness changed.
🟡 Round-1 S4 still stands (agreed, PR 2)
FieldSearchCriteria.RoutingBucket.DB is still documented as "Resolved against the database (Tag) to preserve read-your-writes" (dotCMS/src/main/java/com/dotcms/browser/FieldSearchCriteria.java:41-42) while ContentDriveFieldFilterResolver#routingBucketFor (:216-219) routes Tag and Relationship. Agreed it's a PR 2 code fix — noting it so it doesn't get lost.
Line drift (minor)
buildPureESQuery is at :610, not :605; SearchHeuristicType at :465-469; BROWSER_DB_MAX_SCAN_ROWS at :739-740. Claims all reproduce, numbers a few lines off.
Suggested order
FR-009's reasoning first — it's a paragraph, and it's the one that could cause an incident if someone acts on it. Then the early-exit and ES-fan-out items together, since both are "what does the single scan actually cost" and one criterion covering scan size + ES round-trips answers both. The freshness rescope falls out of the FR-009 rewrite anyway, since the two share a justification.
Leaving this as a comment again rather than a change request — the central ambiguity really is resolved, and whether the rest gates approval is the team's call.
… ES round trips in FR-002
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Approved — the spec phase is cleared.
FR-009 is now correct: it says plainly that BROWSE_API_HEURISTIC_TYPE does gate this path, explains why (isUseElasticSearchForFiltering routes on hasIndexFieldCriteria alone, so a zero-free-text field-filter request still goes through doElasticSearchTextFiltering), and warns that PURE_ES isn't a safe no-op but throws on any request with field criteria. That's the item I most wanted fixed, and the last sentence — "corrected here so an operator doesn't reach for BROWSE_API_HEURISTIC_TYPE=PURE_ES expecting it to be a safe no-op" — is exactly the right framing.
FR-002 and SC-001 now bound DB scans and ES filtering calls as one criterion, and the sparse-vs-dense distinction is stated rather than glossed. The part I didn't ask for and that improves the spec most: SC-001 admits that physical ES round trips stay at ~20–23 because processESDirectly re-splits above ~900 inodes for the 1024-clause limit, and that bounding those would mean switching to a terms filter — a separate, larger change, explicitly out of scope. Naming the limit of your own success criterion is the thing that makes it trustworthy.
One leftover, non-blocking: the freshness paragraph in Assumptions (spec.md:256-260) still frames the index-lag delay as "a deliberate, scoped trade-off for this one case." Now that FR-002 is explicitly the hybrid path, getChunkFiltered already routes every candidate chunk through processESDirectly before permission filtering — so a not-yet-indexed item is already excluded today and this fix concedes nothing new. Worth rescoping to "no change to current freshness behaviour" during planning; it also simplifies FR-009's justification, which currently leans on that trade-off being real.
Process note, not a gate item: this head also carries Phase 1 plan artifacts (data-model.md, .specify/feature.json). Per the Spec-Kit flow PR 1 carries spec.md alone and planning runs after approval — worth keeping them separate so the thing being approved stays the spec. Flagging it here because the same pattern landed on #37189, #37190 and would apply to #37230, which still has open blockers.
2 participants
Spec-Kit PR 1 of 2. Carries the spec alone. Needs a developer approval (not a merge) before /speckit-plan runs.
Resolves the spec phase of #37184.
Proposed Changes
spec.md— 2 prioritized user stories, 9 functional requirements (2 resolved decisions recorded inline, no open clarifications remaining), 4 success criteria, edge cases, dependencies/coordination, and the dotCMS Legacy Considerations section.Summary
When a Content Drive field filter routes entirely to the search index (correct per ADR-0018 —
searchable-flagged fields are index-resolved by design, this is not a mis-routing bug), the DB-first hybrid chunk loop still re-scans the database candidate set in fixed-size chunks until enough index-narrowed matches survive. On a ~21,000-item folder this took 4 repeats and made the response ~6x slower than an equivalent search.buildPureESQueryalready builds nearly everything needed for a single-pass, index-only resolution for this specific case — it's missing only a folder-scoping clause. Routing to that path when zero DB-required criteria (Tag, Relationship), no workflow filter, and no free-text term are present should collapse the repeated scans to one, with no change to any other filter combination's behavior or freshness guarantee.Decisions recorded (no product sign-off needed, resolved as technical calls)
Coordination note
Shares the underlying candidate-scan query with #37183 (spike on that query's plan instability). Whichever direction #37183 lands on must preserve this fix's single-scan-per-request assumption.
Checklist
Additional Info
Parent epic #36814. Originally investigated as item 2 of #37148 (umbrella investigation issue).
/speckit-adr-contextconsulted ADR-0018 (database-first search for Content Drive) — this fix operates strictly inside that contract: it changes how many times the existing hybrid strategy's DB step runs for one already-correctly-routed case, not which source of truth is authoritative for any criterion. No new ADR proposed.🤖 Generated with Claude Code
This PR fixes: #37184