Skip to content

fix(content-drive): align keyword search with Content Search index query #36688 - #36767

Open
ihoffmann-dot wants to merge 5 commits into
mainfrom
issue-36688-content-drive-keyword-search
Open

fix(content-drive): align keyword search with Content Search index query #36688#36767
ihoffmann-dot wants to merge 5 commits into
mainfrom
issue-36688-content-drive-keyword-search

Conversation

@ihoffmann-dot

@ihoffmann-dot ihoffmann-dot commented Jul 28, 2026

Copy link
Copy Markdown
Member

Problem

Content Drive's toolbar keyword/title search returned inconsistent, slow, and irrelevant results (issue #36688). Tested with client data, Content Drive diverged badly from the Search portlet: results took a long time and often had nothing to do with the query.

Root cause

Content Drive built its own Elasticsearch text query (BrowserAPIImpl.buildBaseESQuery) that diverged from the Content Search portlet's query:

  • Broad leading wildcard catchall:*kw* (matches the term anywhere inside any term, across body text → unrelated matches; also a slow, non-indexed scan) instead of Content Search's selective prefix +catchall:kw*.
  • Untokenized term and a mixed +/OR boolean group.

Content Search (LuceneQueryBuilder + GlobalSearchAttributeStrategy) does not have these problems.

An earlier revision of this PR routed the keyword to the DB. Per team discussion that was rejected: ADR-0018 keeps text search on the index, and an unindexed ILIKE over contentlet_as_json on this endpoint is a DB-performance risk. This PR now keeps text on the index and instead makes Content Drive consistent with Content Search.

Fix

buildBaseESQuery now reuses GlobalSearchAttributeStrategy (the same strategy the Content Search portlet uses) to build the free-text clause — a selective +catchall:kw* prefix plus tokenized, escaped title boosts. Content Drive keyword search is now consistent with Content Search.

  • No routing change (text stays on the index, ADR-0018 compliant).
  • No DB schema, no ES mapping, no reindex → rollback-safe.
  • /v1/drive/search is @Hidden, so no openapi.yaml change.

Behavior (validated by ContentDriveKeywordSearchTest)

Seeding IMG_1004.jpeg and searching:

Term Finds the file
IMG, img, Img, IMG_1004, jpeg ✅ yes — a single, relevant result
1004, 1004.jpeg, IMG_1004.jpeg ❌ no

The reported case (IMG) now returns exactly the file (previously: unrelated results + the file missing).

Note (consistency, not a regression introduced here): matching is prefix-based, so mid-token terms (1004), boundary-spanning terms (1004.jpeg), and even the exact full filename (IMG_1004.jpeg — the . splits it into img_1004 + jpeg tokens) do not match. This is identical to the Content Search portlet using the same strategy. Worth a QA confirmation (cc perf/relevance testing).

Out of scope / follow-ups

  • Read-your-writes / index lag (a just-uploaded item not yet in ES): inherent to index-based text search (ADR-0018), long-standing; not addressed here.
  • Relevance ordering: Content Drive still re-sorts the page by sortBy; full score-ordering parity with Content Search is a separate change to the hybrid flow.
  • Performance of the hybrid DB-candidate + permissions path: separate, pre-existing concern.

Closes #36688

🤖 Generated with Claude Code

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ihoffmann-dot's task in 6m 45s —— View job


Code Review — align keyword search with Content Search index query

  • Read the current diff (BrowserAPIImpl, BrowserAPITest, ContentDriveKeywordSearchTest, MainSuite3a)
  • Re-check the prior review findings against the new approach
  • Post review

The approach has changed substantially since @fabrizzio-dotCMS's review: the DB-routing (resolveTextInDb) is gone, and buildBaseESQuery now delegates the free-text clause to the shared GlobalSearchAttributeStrategy — the same strategy the Content Search portlet uses. This directly implements the reviewer's own suggested alternative #4 ("fix the ES query, most likely the actual root cause") and keeps text search on the index (ADR-0018). That change of direction resolves the blocking findings from the prior review.

New Issues

No new bugs found in the current diff.

Verification notes:

  • Blast radius still contained. The changed branch (BrowserAPIImpl.java:1183-1194) only executes when browserQuery.filter is set, and buildBaseESQuery is only reached through the ES-filtering path gated by isUseElasticSearchForFilteringbrowserQuery.useElasticsearchFiltering (BrowserAPIImpl.java:1496), which is set only by ContentDriveHelper. So /v1/browser and the legacy JSP paths are unaffected. The field-filter-only path (no text) skips the branch entirely — identical output to before.
  • No escaping regression. The old hand-rolled string escaped nothing; the strategy escapes the trailing title: clause. Net escaping is equal-or-better.
  • Prefix/tokenization limits (1004, 1004.jpeg, exact IMG_1004.jpeg not matching; multi-word +catchall:a b* splitting) are inherent to GlobalSearchAttributeStrategy and are identical to the Content Search portlet — the explicit goal here — not introduced by this PR. Tracked for a shared fix in Global/Content search: exact-name and mid-token terms don't match (prefix-only catchall gate) #36791.

Resolved (from prior review)

  • Blocking update with latest SVN #1ContentDriveKeywordSearchTest now registered in MainSuite3a.java:76, so it runs in CI.
  • Blocking Test Branch and Commit #2 (DB full-scan perf regression) — the DB text-routing path was removed; text stays on the index.
  • Blocking Add .gitignore #3 (contentlet_as_json::text noise) — no longer applicable; the DB predicate is gone and the ES clause is now the selective +catchall:kw* prefix.
  • Medium New Issue #4 (unescaped LIKE wildcards) — DB predicate removed.
  • Minor this is added #8 — terms now driven by a @DataProvider (matchingKeywordTerms), so each keyword reports separately.
  • Minor this is added #9languageId now derived from getDefaultLanguage().getId(); @AfterClass cleanup added (site/type/temp dir); no removeContentFromIndex in the current test.
  • Minor Create SiteSearch / Publishing Framework #10test_buildBaseESQuery_delegatesToSharedGlobalSearchStrategy (BrowserAPITest.java) now covers the branch and guards against the old leading-wildcard/mixed-OR query.

Minor (non-blocking, optional)

  • 🔵 ContentDriveHelper.java:68 — class javadoc still reads "Uses Elasticsearch for text filtering while maintaining database reliability." This file isn't in the diff, but since the PR reframes how text search works, the line reads as stale (prior review testing assign #7). Out of this PR's changeset — worth a quick follow-up touch if you re-open the file.
  • 🔵 Test coverage asserts presence of the target but not exclusion of irrelevant items for the keyword path (the composition test does assert exclusion 👍). Given the new query is now provably consistent with Content Search, this is lower risk than under the old DB approach, but a negative assertion would harden the regression guard.

Recommendation

The new direction is sound and rollback-safe (no DB schema, no ES mapping, no reindex; /v1/drive/search is @Hidden). All prior blocking findings are addressed. Nothing here blocks merge.
· issue-36688-content-drive-keyword-search

🤖 Generated with Claude Code

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ihoffmann-dot's task in 7m 16s —— View job


Rollback-Safety Analysis

  • Read docs/core/ROLLBACK_UNSAFE_CATEGORIES.md
  • Diff BrowserAPIImpl.java, BrowserQuery.java, ContentDriveHelper.java, and the new integration test
  • Check against every C/H/M category
  • Verdict posted

Verdict: ✅ Safe to Rollback — no category matched.

Why:

  • No runonce migration task, no DDL (ALTER/DROP/RENAME), no PK change — rules out C-1, C-3, C-4, H-1, H-2, H-3, H-7.
  • No putMapping() call, no ESMappingAPIImpl/ESMappingUtilHelper/ESMappingConstants touch, no reindex trigger — rules out C-2.
  • No new ContentType field type, no storage-provider change, no DROP PROCEDURE/FUNCTION — rules out H-4, H-5, H-6.
  • No ViewTool implementation or object returned to VTL templates was touched — rules out H-8.
  • /v1/drive/search is @Hidden per the PR description (confirmed no openapi.yaml diff in the changeset) and the JSON response shape is untouched — only the internal data-source routing for the free-text term changed. Rules out M-3.
  • No push-publishing bundle XML or OSGi-exported interface touched — rules out M-2, M-4.

What actually changed: a new BrowserQuery.resolveTextInDb flag (default false, so /v1/browser legacy path is byte-for-byte unchanged), a routing branch in BrowserAPIImpl.selectQuery/buildBaseESQuery/isUseElasticSearchForFiltering, and ContentDriveHelper setting the new flag instead of forcing useElasticsearchFiltering. All in-memory query-building logic — no persisted data shape, index mapping, or wire contract changes.

On rollback to N-1: the toolbar keyword search simply reverts to routing through Elasticsearch again (re-introducing the original read-your-writes bug this PR fixes) — a functional regression, not data loss, startup failure, or a broken contract for a surviving consumer. N-1 boots normally and reads/writes exactly as before.

Label AI: Safe To Rollback has been applied.

@fabrizzio-dotCMS fabrizzio-dotCMS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review

Thanks for the detailed write-up and for shipping a regression test with the fix. I agree the read-your-writes gap is real, but I don't think free-text search should be resolved in the database: I measured the query this PR introduces against two production-scale datasets and the cost is significant. I also don't think the "incoherent results" half of #36688 is fixed by this change.


🔴 Blocking

1. ContentDriveKeywordSearchTest is not registered in any suite, so it never runs in CI

It lives in dotcms-integration with a *Test name, so it only runs through @SuiteClasses. Its four siblings are registered (MainSuite3a.java:73-76: ContentDriveFieldFilterTest, ContentDriveHelperContentletAPIComparisonTest, ContentDriveWorkflowArchiveStepTest, ContentDriveWorkflowFilterTest), and the diff doesn't touch MainSuite3a. The "Integration Tests - MainSuite 3a" job on this PR passed in 27 minutes without executing a single new assertion — so the regression guard for #36688 is currently inert. One-line fix: add it to MainSuite3a.

2. Measured performance regression on the keyword search path

I rebuilt the exact SQL this PR produces — selectQuery base with skipFolder(true) (Content Drive at site root, so no parent_path predicate), the appendFilterQuery predicate (BrowserAPIImpl.java:2084-2104), order by c.mod_date desc LIMIT 900 — and ran EXPLAIN (ANALYZE, BUFFERS) on two anonymized production-scale datasets:

Dataset items in site scope contentlet table pre-fix (no text predicate in SQL) post-fix, common term post-fix, no-match term
A ~391,000 1.5 GB 0.49 – 2.0 s 4.7 s 8.2 s
B ~324,000 (+129k SYSTEM_HOST) 9.2 GB 0.37 s 37.6 s

Warm cache, local SSD, a single connection, no concurrency; EXPLAIN ANALYZE doesn't transfer rows, so these numbers are, if anything, optimistic.

Two aggravating factors that aren't visible in the diff:

  • order by c.mod_date desc forces the predicate to be evaluated over the whole candidate set before the LIMIT 900 can cut. There is no early exit for rare terms — and in a toolbar the user types progressive prefixes, so most keystrokes land in exactly the rare/no-match case (the 8–37 s column).
  • The existing safety valve stops bounding the work. BROWSER_DB_MAX_SCAN_ROWS = 50_000 (BrowserAPIImpl.java:725) caps rows returned per chunk, not rows examined. Previously the ILIKE was not in the SQL, so the DB scan stopped at 50k rows and ES narrowed each chunk. Now the predicate is inside the SQL, so Postgres sweeps the entire site (390k+ rows) evaluating contentlet_as_json::text, which forces a detoast + lz4 decompression of every row's JSON (postgres.sql:779). No trigram/GIN index exists that could serve ILIKE '%x%', and postgres.sql:2389 only has an expression index on the template field.

This also reverses a deliberate decision: the ES text path was introduced for performance in #33416 / 6a024b8 ("significant performance improvements for text-based content searches"). Reversing it is fine if warranted, but it needs a measurement, and the measurement doesn't support it.

3. The "results unrelated to the typed text" half of #36688 is not fixed — the noise source just moves

appendFilterQuery matches contentlet_as_json::text, i.e. the whole serialized JSON document, not field values. That document contains (see com.dotcms.content.model.Contentlet:41-49, FieldValue's @JsonTypeInfo(property = "type"), and the ->'fields'->...->>'value' shape used in FolderIntegrityChecker:182):

  • identifier, inode, contentType ids → any hex substring (1004, e5f) matches unrelated content
  • modDate as ISO-8601 (ContentletJsonHelper:42 sets WRITE_DATES_AS_TIMESTAMPS=false) → searching 2026 returns everything modified this year
  • JSON keys and type discriminators: "fields", "value", "type":"Binary", "Image", "Text", "TextArea" → searching image returns every content that has an Image field; searching value or type returns everything in scope

None of that was ever in the ES catchall, so for these terms precision gets worse, not better. The new test only asserts presence of the target and never exclusion of irrelevant items, which is why it passes. If the DB path stays in any form, the predicate should be scoped to field values (contentlet_as_json->'fields', e.g. jsonb_path_query_array(..., '$.fields.*.value')) and the test should include a negative assertion for a term like 2026 or Text.


Suggested alternatives — close the lag on the write side, not on every read

The lag is real and it's the default: INDEX_POLICY_SINGLE_CONTENT is DEFER (IndexPolicyProvider.java:35), so a save returns before the index refreshes. But that can be closed where it originates:

  1. IndexPolicy.WAIT_FOR on Content Drive's own write operations (upload, create, publish, rename, move). The write waits one refresh (≤ refresh_interval, typically 1 s) and every subsequent search sees the item. That's ~1 s once per write instead of 4.7–37 s per search. Worth noting: the new test seeds its fixtures with setPolicy(IndexPolicy.WAIT_FOR) — the primitive that solves this is already being used, in the right place.
  2. Optimistic insert on the front end — zero backend cost. The client just created the item and holds its payload; it can prepend it to the list and reconcile on the next refresh. This is what drive-style UIs do, and it fully covers the "I just uploaded IMG_1004.jpeg and can't see it" report.
  3. Recency-bounded overlay, if a server-side guarantee is wanted: keep ES for the search and union a second DB query restricted to recent writes (and c.mod_date > now() - interval '5 minutes' plus the same scope). Same predicate as this PR, but bounded so the ILIKE evaluates over tens of rows instead of ~400k.
  4. Fix the ES query, which is most likely the actual root cause. buildBaseESQuery emits +(title:X* OR title:'X'^15 OR title_dotraw:*X*^5 OR +catchall:*X*^10). The + on the last clause makes catchall mandatory in query_string, and wildcard/prefix terms don't go through the analyzer — so an uppercase term like IMG cannot match the lowercased index terms (→ "finds nothing"), while lowercase img matches any body containing img (→ "unrelated results"). Those are precisely the two symptoms reported in #36688. Lowercasing the term, dropping the inner +, and targeting title / title_dotraw / metadata.name instead of catchall likely fixes the report without moving anything to the database. If the ES text branch is kept for external callers, this bug stays latent either way and should be fixed.

🟠 Medium

4. Unescaped LIKE wildcards (BrowserAPIImpl.java:2085, 2102-2104) — pre-existing, but this PR promotes the path to Content Drive's primary keyword search. Searching 50% becomes ILIKE '%50%%' (matches "50" followed by anything); a_b matches "axb". Cheap fix: escape %, _, \ and add ESCAPE '\'.

5. The PR description overstates the DB predicate. Only the JSON clause is tokenized. asset_name, the binary asset name and the tag sub-select all use the full filter string, so the generated SQL is (json~tok1 AND json~tok2) OR asset_name~"full string" OR binName~"full string" OR tag~"full string". IMG 1004 works only because the file name is embedded in the JSON. Worth correcting in the description and the new javadoc.

🔵 Minor

  1. Two flags now encode one decision. useElasticsearchFiltering is set only by ContentDriveHelper, and only when INDEX field criteria exist, so the text group in buildBaseESQuery / buildPureESQuery is unreachable from any in-product caller (external OSGi callers aside). Either drop it or mark it explicitly as external-caller-only — along with the +catchall bug above.
  2. ContentDriveHelper's class javadoc still says "Uses Elasticsearch for text filtering while maintaining database reliability" — stale after this change.
  3. keywordSearch_uppercaseIMG_findsFile and keywordSearch_digits1004_findsFile are strict subsets of keywordSearch_findsFileAsset_caseInsensitive_anySubstring. The class already runs on DataProviderWeldRunner, so a @DataProvider over the terms would report each one separately instead of collapsing them into one fail().
  4. Test: languageId(1) is hardcoded while the search filters by the system default language — better derived from APILocator.getLanguageAPI().getDefaultLanguage().getId(). There's also no @AfterClass cleanup (site, content type, temp dirs leak), and removeContentFromIndex leaves the index inconsistent for the rest of the suite.
  5. No unit-level coverage for the two new branches. BrowserAPITest.test_buildBaseESQuery_withDifferentFilterCombinations (already in MainSuite3a) is the natural home for two cases: resolveTextInDb=true producing no text group, and the isUseElasticSearchForFiltering truth table.

What's good

  • Blast radius is correctly contained: useElasticsearchFiltering is only ever set by ContentDriveHelper, so /v1/browser and the legacy JSP paths are byte-identical, and the false default guarantees it.
  • The read-your-writes test (de-index, then still find it) genuinely proves the behavior change — it would have failed before the fix.
  • The composition test asserts exclusion when the field filter doesn't match, not just inclusion. 👍
  • No DB schema, no ES mapping, no reindex, and /v1/drive/search is @Hidden so no openapi.yaml churn — the rollback-safe claim holds.

Recommendation

As it stands the PR fixes a write-side problem (index lag) with a read-side regression (a full site scan per keystroke), and it doesn't close the precision half of the issue. I'd suggest (4) + (1) or (2) from the alternatives above, with (3) if a server-side guarantee is required, and I wouldn't leave resolveTextInDb as Content Drive's default. Item 1 (suite registration) should be fixed regardless of which direction the fix takes.

@ihoffmann-dot ihoffmann-dot changed the title fix(content-drive): resolve keyword/title search in DB for read-your-writes #36688 fix(content-drive): align keyword search with Content Search index query #36688 Jul 28, 2026
@ihoffmann-dot

Copy link
Copy Markdown
Member Author

Thanks for the review @fabrizzio-dotCMS your alternative #4 was the right call. I dropped the DB routing; buildBaseESQuery now reuses GlobalSearchAttributeStrategy, so Content Drive and the Search portlet build the identical query.

That retires #2, #3, #5, #6, #7 (DB code that no longer exists). Fixed here: #1 (good catch, the test was inert), #8, #9, #10.

One correction: uppercase for the example 1004_IMG.jpeg Erick shared, IMG does match now, verified against a real index. That was my initial hypothesis too, and the measurement refuted it.

Mid-token (1004) and exact-name terms still don't match, same as the Search portlet, shared code, raised separately as #36791 / PR #36793.

@mergify

mergify Bot commented Jul 30, 2026

Copy link
Copy Markdown

Queued — the merge queue status continues in this comment ↓.

@ihoffmann-dot
ihoffmann-dot added this pull request to the merge queue Jul 31, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Jul 31, 2026
@ihoffmann-dot
ihoffmann-dot added this pull request to the merge queue Jul 31, 2026
@mergify

mergify Bot commented Jul 31, 2026

Copy link
Copy Markdown

Merge Queue Status

This pull request spent 1 hour 31 minutes 51 seconds in the queue, including 1 hour 30 minutes 56 seconds running CI.

Waiting for any of
  • check-neutral = Finalize / Final Status
  • check-skipped = Finalize / Final Status
  • check-success = Finalize / Final Status
All conditions

Reason

The merge conditions cannot be satisfied due to failing checks

  • Finalize / Final Status
  • Initialize / Initialize

Failing checks:

Hint

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

  • Requeue this pull request

@mergify mergify Bot added the queued label Jul 31, 2026
@mergify mergify Bot added dequeued and removed queued labels Jul 31, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Backend PR changes Java/Maven backend code dequeued Team : Scout

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[BUG] Content Drive: keyword/title search returns inconsistent or incoherent results

2 participants