fix(migration): Site Search highlights, pointer preservation and phase-neutral index tests (#36360)

[fabrizzio-dotCMS]

Summary

The weekly OpenSearch Phase Sweep (Sunday 2026-08-02, main @ ce9fd437) failed 6 of 18 jobs. Triaging those failures turned up three real product defects in the ES→OpenSearch migration — lost Site Search highlights, an opaque activation failure, and content-index writes silently deactivating the Site Search index — while the rest were tests asserting Elasticsearch-shaped facts rather than actual regressions. This fixes the three defects and makes the affected assertions meaningful in every migration phase instead of skipping them.

Issue #36360.

Changes

Backend — Site Search highlights (SearchHit, OSSiteSearchAPI)

• Search-result snippets lost their <em> emphasis wherever OpenSearch serves reads. The OpenSearch path already asked the engine to highlight the content field, but the neutral SearchHit had nowhere to carry the fragments, so they were dropped with an explicit TODO.
• SearchHit gains a getHighlights component (field → fragments) plus a highlightsFor(field) accessor that never returns null. Both adapters populate it; Elasticsearch's HighlightField is flattened from its Text[] inside the adapter so the vendor type does not leak.

Backend — activation diagnostics (ContentletIndexAPIImpl.activateIndex)

• A name matching no content slot used to fall through every branch: a silent no-op in phases 0/1/2, and in Phase 3 it saved an empty VersionedIndices, so the caller got At least one index must be specified when saving versioned indices for version: 3.X from deep inside the store — naming neither the input nor the problem.
• Rejected up front with a message stating what was passed and what is accepted. ESIndexResource already maps DotStateException to a 400, and both production callers route site-search names to SiteSearchAPI before reaching here.

Backend — Site Search pointer preservation (ContentletIndexAPIImpl)

• The siteSearch slot shares the OpenSearch version row with the content pointers, and saveIndices() is a delete-by-version followed by a re-insert. Six of the ten places that rebuild a VersionedIndices for content slots omitted siteSearch, which does not leave the slot alone — it erases it. So a reindex start, switchover, abort, activate or deactivate of a content index silently deactivated the active Site Search index.
• Adds preserveSiteSearch(builder, existing) and calls it from every rebuild that was missing the carry-over. The four that already carried the slot are unchanged.

Tests — phase-neutral assertions (5 integration classes)

• Alias lookups go through SiteSearchAPI.getAliasToIndexMap(). The content-index router keys OpenSearch entries by their .os-tagged physical name and deliberately kept that contract in fix(sitesearch): resolve aliases & index stats .os-aware in OpenSearch read phases (#36360) #36797, which rerouted the six production callers but left the tests behind.
• Default-index checks use SiteSearchAPI.isDefaultIndex(); content pointer checks use the phase-aware getActiveIndexName(). IndiciesAPI.loadIndicies() only ever reads the legacy non-versioned rows, so it reports nothing once activation writes the versioned store.
• Name comparisons strip the vendor tag (IndexTag.strip) rather than comparing raw — reporting .os from Phase 3 is the intended display contract, and endsWith(logicalName) can never match a .os-suffixed name.
• SiteSearchJobImplTest clears indices through SiteSearchAPI.deleteIndex() (idempotent per engine) instead of the content router. This was the actual cause of the "Site Search returns 0 results in Phase 3" failures: OSIndexAPIImpl.delete() applies the cluster prefix but not the .os tag and treats index-not-found as a successful no-op, so the old loop deleted nothing while reporting success, and the test then searched a stale leftover index.
• optimize receives the physical names the engine holds, matching production callers that feed it names from getIndices() — it dispatches by vendor tag.
• ESIndexAPITest.testDeleteOldIndices normalizes the retention listing to logical names. Retention itself is correct: each provider preserves its active set from its own store.
• test_createSiteSearchIndex_shouldBePossibleToAddMoreThan100 now deletes its 115 indices; leaving them behind polluted every later alias assertion in the same JVM. The two reindex tests build their index name with the _ separator that production and every other test use.
• New unit coverage in SearchHitTest for the highlight mapping and the empty-map default.

Testing

Verified against a live stack (PostgreSQL + Elasticsearch 7.10 + OpenSearch 3.4) — the five affected classes, 61 tests, under each migration phase:

	Phase 1	Phase 2	Phase 3
Failures	0	0	0
Skipped	0	0	5

The 5 skipped in Phase 3 are the ES-retention tests the class itself skips there (Elasticsearch is decommissioned); they all run and pass in Phase 2, so that fix is covered.

To reproduce:

just test-integration-phase 3 ContentletIndexAPIImplTest,ESIndexAPITest,ESSiteSearchAPITest,SiteSearchJobImplTest,ContentTypesPaginatorTest

Unit tests: ./mvnw test -pl :dotcms-core -Dtest=SearchHitTest → 5/5.

One pre-existing flake, unrelated to these classes, reproduced in phases 2 and 3: ContentletIndexAPIImplTest.test_that_live_and_working_content_makes_it_into_the_index times out after 10s waiting for a document to become visible and passes on retry. Possibly a refresh/visibility gap on the OpenSearch write path — not investigated here.

Breaking Changes

None. SearchHit gains a record component, so its canonical constructor arity changes; the only construction sites are its own builder and the two vendor adapters, all in this repo. activateIndex now throws DotStateException for a name that maps to no content slot, where it previously did nothing — the REST layer already renders that as a 400, and no production caller passes such a name.

Deliberately out of scope

• A bare-name delete in Phase 3 reports success without deleting anything (OSIndexAPIImpl.delete omits the .os tag). That silence is worse than the Phase 1 exception, and it is the same bug family as the open fix(sitesearch): reconcile ES/OS index mirrors on the write path (#36360) #36825 — changing it in parallel would conflict.
• removeClusterIdFromName(null) returns "", turning "no index" into an index named the empty string. This is what made the activation failure reachable; changing the contract touches every caller and cannot be validated without the full battery. The new guard makes the symptom loud instead of silent.
• A site-search index name without the _ separator never resolves its alias — the index-name timestamp parsing keys off it, and that same parsing feeds retention. Production always generates sitesearch_<timestamp>, so this is only reachable with an arbitrary name.

🤖 Generated with Claude Code

[claude]

Claude finished @fabrizzio-dotCMS's task in 8m 0s —— View job

---

Review

Reviewed the diff against origin/main (6 commits). The three product fixes are sound and the test changes correctly move from ES-shaped assertions to phase-neutral ones. No blocking bugs found.

Response to @wezell's review comment (performance of eager highlights on all search results)

The concern is valid to raise, but the eager population does not add cost to the content-search read path:

• Content search never requests highlighting. Neither ESContentFactoryImpl nor ContentFactoryIndexOperations set a highlighter on the query, so esSearchHit.getHighlightFields() / osHit.highlight() come back empty for content reads. SearchHit.fromEsHighlightFields() short-circuits on null/empty and returns Map.of() (SearchHit.java:123); the OS path assigns the empty map through the builder's null-guard (SearchHit.java:239-242). The only per-hit work for content results is one empty-check — no allocation, no Text[] walk.
• Only Site Search asks for the content highlight (OSSiteSearchAPI.java:345), and that path is exactly where the fragments are wanted. So the map is built precisely when it's consumed.

So the highlight map is effectively already pay-for-what-you-use — it's empty and free for content, populated only for site search. Lazy-loading on-demand wouldn't reduce work on the content path (the empty map is a shared singleton via Map.of()), and it can't be truly lazy anyway: SearchHit is an immutable record built once from a vendor hit that isn't retained, so there's no live handle to defer the extraction to. wezell's broader point about separating content-search DTOs from heavier site-search DTOs is a reasonable future refactor, but it's an architectural change beyond this bug-fix PR's scope.

New Issues

None blocking.

• 🟡 Medium: dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ContentletIndexAPIImpl.java:3304 — the new guard rejects any name that doesn't startsWith("working"|"live"), including a cluster-prefixed physical name (cluster_<id>.working_...). The Javadoc at line 3289-3290 says it accepts names "with or without the cluster prefix", but IndexType.WORKING.is() is a plain startsWith on the bare prefix — a prefixed name fails the check and throws. In practice both production callers pass a stripped name (ESIndexResource strips via removeClusterIdFromName, IndexAjaxAction routes site-search names away and the content name comes stripped), and the pre-existing branch logic below already only matched bare working_/live_, so this isn't a regression. But the Javadoc overstates what's accepted — either tighten the doc to say "cluster-prefix-stripped" or make the guard prefix-tolerant to match its own contract.

Notes (non-blocking, no action needed)

• preserveSiteSearch is applied consistently to all six previously-missing rebuild sites, and the four that already carried the slot are untouched. The deactivateIndex → mirrorDeactivateToOsStore path (:3481) correctly preserves site-search before the hasAnyIndex() emptiness check, so a content deactivation with an active site-search index keeps the version row rather than removing it — the intended fix.
• activateIndex throwing DotStateException is mapped to a 400 by DotStateExceptionMapper; the REST/AJAX callers route site-search names elsewhere first, so the new throw is only reachable for genuinely invalid input. Breaking-change note in the PR body is accurate.
• Test migrations look correct: IndexTag.strip comparisons, SiteSearchAPI.getAliasToIndexMap()/isDefaultIndex() over IndiciesAPI.loadIndicies(), physicalNamesOf() for tag-dispatched optimize, and the UtilMethods.isSet guards around restore calls (now that activateIndex rejects "") are all consistent with the phase-neutral goal.
• SearchHitTest covers both the populated-highlight mapping and the empty/absent-field default (the NPE guard for toArray). Good coverage for the new behavior.
• Only construction site of the domain SearchHit is its own builder + the two adapters; the record-arity change is contained. (ESQueryCacheTest's new SearchHit(...) is the unrelated org.elasticsearch.search.SearchHit.)

Overall: LGTM once the Javadoc/guard mismatch above is reconciled (non-blocking).
· issue-36360-phase-sweep-fixes

[wezell]

See my comment.

[wezell]

For what it’s worth, I think we could separate the content search results from the site search results. Content search results are super important and should be small, performant, and fast, almost records. Site search results are different and can be heavier. Really two different functions

[fabrizzio-dotCMS]

Thanks — you're right about the direction, so I built it: #36899 separates the two shapes, stacked on this branch. Details below, plus what the field actually costs today and why the lazy variant would have gone the wrong way.

The separation (#36899)

SearchHit becomes a sealed interface over two records:

public sealed interface SearchHit permits ContentSearchHit, SiteSearchHit {
    // ... the six neutral accessors ...
    default List<String> highlightsFor(String field) { return List.of(); }
}

• ContentSearchHit — six components, no highlight state at all. What every content query produces.
• SiteSearchHit — the same six plus the fragments.

The default is what makes it cheap: the concern is declared once on the contract, costs zero bytes per instance, and a caller holding a plain SearchHit never has to know or ask which shape it got.

Callers don't choose the shape either — Builder.build() picks it from the data, returning the lean record unless the engine actually returned fragments. That detail matters more than it looks: Site Search and content search reach the neutral layer through the same factory (ContentSearchResponse.from, since Site Search reads via rawSearch after #36398), so a "do I want highlights" flag would have had to be threaded through rawSearch → ContentSearchResponse.from → SearchHits.from → the hit adapter. Deciding from the response keeps it in one place.

Blast radius turned out to be nil outside the domain package: compile -pl :dotcms-core passes with zero changes anywhere else, and the enterprise consumer really does go through the contract (invokeinterface SearchHit.highlightsFor in the bytecode). ContentSearchToolTest — the VTL guard that evaluates customer templates through the real Velocity engine — is 11/11 green against it.

What the field costs today

Worth recording since it shaped how I scoped this: on the content path the shared field was already free rather than a regression. Highlighting only produces data if the query asks for it, and the only three places that ask are all Site Search (ESSiteSearchAPI:251, :257, OSSiteSearchAPI:344). With no highlight section in the response the adapter returns the shared Map.of() singleton, so a content hit carried one reference field and one isEmpty() check — no allocation, nothing extra parsed or on the wire. It also never reached the query cache: ESQueryCache stores Elasticsearch's own SearchHits, not the neutral type.

So #36899 is worth doing for the reason you gave second — cohesion, content results not advertising a Site Search concern — rather than to recover measurable overhead. Side benefit: ContentSearchHit no longer serializes an empty "highlights":{}.

Why not lazy

The fragments only exist inside the vendor hit (org.elasticsearch.search.SearchHit / OpenSearch's Hit). Deferring the read means holding a reference to that object inside the neutral record, which leaks the vendor type the neutral layer exists to hide and keeps the engine's whole response graph reachable — more retained heap, not less. And there's no second round-trip to defer: highlights arrive in the same response or not at all.

Two things I'd flag on the split

• Jackson. SearchHit/SearchHits are deserialized and back caches/REST paths, and an interface can't be instantiated. I used @JsonDeserialize(as = ContentSearchHit.class), which keeps readValue(json, SearchHit.class) working; the tradeoff is that a serialized Site Search hit would round-trip back as a content hit and lose its fragments. Nothing serializes one today. The alternative, polymorphic @JsonTypeInfo, would add a type discriminator to a JSON shape those caches depend on, which seemed the worse trade.
• SearchHit goes from a record to an interface, so anything compiled against the concrete type needs recompiling. Nothing in the repo does, and the type is internal to the neutral search layer — but flagging it in case you know of plugin code that shouldn't be.

Also worth noting on your framing: the split is already half-there in the direction you describe. The heavy carrier is SiteSearchResult (a HashMap-backed bag) and content results never pass through it — what was shared is only the engine adapter, which is what #36899 splits.

Happy either way on sequencing: land this PR and #36899 behind it, or if you'd rather the field never land at all, I can pull the highlights commit out of here and ship the two pointer fixes first. The tradeoff on that second path is that the Site Search phase-2/3 test stays red until the split merges.

[wezell]

Looks better with the change