Skip to content

task(ESMappingApiImpl integration) Refs:34933#35275

Merged
fabrizzio-dotCMS merged 4 commits intomainfrom
issue-34933-integration-1
Apr 10, 2026
Merged

task(ESMappingApiImpl integration) Refs:34933#35275
fabrizzio-dotCMS merged 4 commits intomainfrom
issue-34933-integration-1

Conversation

@fabrizzio-dotCMS
Copy link
Copy Markdown
Member

@fabrizzio-dotCMS fabrizzio-dotCMS commented Apr 9, 2026

Summary

This PR is a subset of a larger ES → OpenSearch migration initiative (issue #34933). The changes have been split into multiple focused PRs; a follow-up PR with additional migration logic is coming after this one.

Introduces the foundational infrastructure for the ES → OpenSearch (OS) migration: startup validation, IndexTag-based targeted routing, documentation, and a Docker Compose stack for running both backends side-by-side.


Changes

Infrastructure & Tooling

  • New Docker Compose (docker/docker-compose-examples/single-node-os-migration/) — runs ES 7.9.1 and OS 3.4.0 side-by-side with dotCMS, Kibana, and OS Dashboards; designed to validate dual-write before promoting OS as primary

Documentation

  • OPENSEARCH_MIGRATION.md — migration phases, shadow index strategy, index architecture, write pipeline, dispatch model, known gotchas, rollback risks
  • OPENSEARCH_CLIENT_CONFIGURATION.md — full reference for all OS_* config properties with ES fallback mapping, defaults, and examples

Backend — Routing & Mapping

  • ESMappingAPIImpl — added putMapping(List, String, IndexTag) targeted overload; routes to a single provider (ES or OS) identified by IndexTag, bypassing phase fan-out; preserves original phase-dispatch overload unchanged
  • ESMappingUtilHelper — implements new MappingHelper interface; made fields final; adds IndexTag-overloaded addCustomMapping variants for targeted catchup operations
  • MappingHelper — new interface extracted from ESMappingUtilHelper
  • IndexTag — new enum/utility class that identifies the target vendor (ES vs OS) for targeted routing operations
  • PhaseRouter — minor additions to expose osImpl() and esImpl() accessor methods

Backend — Startup & Init

  • IndexStartupValidator — new class; validates OS version (3.x) and endpoint separation from ES at startup; invoked from ContentletIndexAPIImpl.checkAndInitialiazeIndex() whenever the migration phase is active
  • ContentletIndexAPIImpl — calls IndexStartupValidator.validateIndexingConfig() during index init when migration is started, reads are enabled, or migration is complete
  • DotCMSInitDb — minor additions (16 lines) related to migration startup wiring
  • ESMigrationUtil — removed 82 lines of code replaced by the new infrastructure
  • PermissionBitFactoryImpl — removed 10 lines of obsolete migration-related code
  • ESIndexAPI — minor code style improvement in getIndices() (no behavioral change)

Testing

  • No new automated tests in this PR — integration test coverage for the IndexTag dispatch and IndexStartupValidator paths is tracked in issue [TASK] Migrate ESMappingAPIImpl #34933 and expected in the follow-up PR
  • Manual validation: spin up the new single-node-os-migration Docker Compose stack and verify that dotCMS starts cleanly with both backends running

Breaking Changes

None. All changes are additive or internal. Existing ES behavior is preserved; the new putMapping overload with IndexTag is a new method, not a replacement.


Note: This PR represents a split subset of a larger changeset for issue #34933. A follow-up PR with additional migration logic is already in progress.

This PR fixes: #34933


Note

Medium Risk
Touches search-index initialization and mapping write paths; the new startup validator can fail fast on misconfiguration and the new targeted mapping routing changes how mappings are applied during migration scenarios.

Overview
Adds foundational ES→OpenSearch migration scaffolding: a new IndexTag utility and IndexTag-based overloads to target mapping updates at a single backend (vs phase fan-out), plus PhaseRouter accessors to support that routing.

Introduces IndexStartupValidator and wires it into index initialization (ContentletIndexAPIImpl.checkAndInitialiazeIndex() and DB bootstrap via DotCMSInitDb) to validate OpenSearch version and ensure ES/OS endpoints don’t overlap before startup proceeds.

Cleans up legacy/obsolete code (removes ESMigrationUtil and ES bulk-indexing remnants in PermissionBitFactoryImpl), adds a single-node docker-compose stack to run ES+OS side-by-side, and adds documentation for OpenSearch config and the migration/dispatch model.

Reviewed by Cursor Bugbot for commit 615ec62. Bugbot is set up for automated code reviews on this repo. Configure here.

This PR fixes: #34933

@github-actions github-actions bot added Area : Backend PR changes Java/Maven backend code Area : Documentation PR changes documentation files labels Apr 9, 2026
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 9, 2026

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


PR Review: issue-34933-integration-1

  • Gather context
  • Read key changed files
  • Analyze and post findings

Findings

[CRITICAL] Validator exception swallowed — ContentletIndexAPIImpl.java:174-207

IndexStartupValidator.validateIndexingConfig() throws DotRuntimeException, which extends RuntimeException, which extends Exception. The existing catch (Exception e) at line 204 catches it, logs FATAL, then silently continues. The fail-fast intent of the validator is completely defeated here — dotCMS will start with a misconfigured search backend exactly as if the check had never run. The DotCMSInitDb path correctly propagates the exception via Try.run(...).getOrElseThrow(...) — this path does not.

Fix: catch and re-throw before the generic handler:

} catch (DotRuntimeException e) {
    throw e;  // validator fail-fast must propagate
} catch (Exception e) {
    Logger.fatal(this.getClass(), "Failed to create new indexes:" + e.getMessage(), e);
}

Fix this →


[MEDIUM] Hardcoded OS 3.x version check — IndexStartupValidator.java:39,92

REQUIRED_OS_MAJOR = "3." checked via startsWith. OS 4.0 is already in active development. Any customer upgrading to OS 4.x will hit a hard startup failure with no config escape hatch. The check should compare the numeric major version (>= 3) or be driven by a config property like OS_REQUIRED_MAJOR_VERSION.


[MEDIUM] putMapping fan-out doesn't respect shadow-failure semantics — ESMappingAPIImpl.java:344-354

The manual nested loop in putMapping(List, String) lets any provider failure propagate immediately — result &= ops.putMapping(...) will throw if putMapping throws IOException. In dual-write phase 1, OS is a shadow: an OS mapping failure should be logged and swallowed, not abort the ES mapping write. This is inconsistent with what PhaseRouter.writeChecked does for all other operations, and means a flaky OS instance in phase 1 can break content-type mapping for all ES writes. The same issue exists in the new putMapping(List, String, IndexTag) for completeness (single-provider path, less critical).


[MEDIUM] resolveOSEndpoints() is an instance method but doesn't use thisIndexStartupValidator.java:174

It never references osClientProvider. All calls are to static IndexConfigHelper methods. Should be private static like resolveESEndpoints(). As written it's misleading — looks like it depends on injected state.


[LOW] Redundant migration phase condition — ContentletIndexAPIImpl.java:174

isMigrationStarted() is defined as !isMigrationNotStarted() — it's true for all phases except Phase 0. isReadEnabled() and isMigrationComplete() are both subsets of that. isReadEnabled() || isMigrationComplete() is dead code in this condition. This either reflects a misunderstanding of the phase model or left-over defensive code from a prior revision. Either way it should be isMigrationStarted() only, so the intent is clear.


[LOW] putMapping(List, String, IndexTag) is not on any interface — ESMappingAPIImpl.java:368

This new overload is added to the concrete class only, not to IndexMappingRestOperations or any other interface. The PutMappingFn that wraps it in ESMappingUtilHelper is package-private. Callers or tests that operate on the interface type can't reach this. Whether that matters depends on how the migration is wired, but it constrains testability and future abstraction.


[INFO] Endpoint overlap check is documented as best-effort but the gap is wider than the docs suggest

normalizeEndpoint strips the scheme and returns host:port. But resolveESEndpoints() uses Config.getStringProperty("ES_HOSTNAME", "localhost") and resolveOSEndpoints() uses IndexConfigHelper.getString(OSIndexProperty.HOSTNAME, "localhost"). If OSIndexProperty.HOSTNAME falls back through to ES_HOSTNAME (which the doc says it can), both sides produce the same value — overlap IS caught. But if they fall back to the same IP via different names (localhost vs 127.0.0.1), the check silently passes. This is documented as "best-effort" but the failure mode (misconfigured dual-write) is serious enough to warrant a clearer warning in the FATAL log when the overlap check itself is running on fallback defaults.


The %n in JSON strings that Cursor flagged is not present in the current diff — either it was fixed before this review or Cursor was looking at an intermediate commit.

@fabrizzio-dotCMS fabrizzio-dotCMS marked this pull request as ready for review April 9, 2026 20:40
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Fix All in Cursor

Bugbot Autofix is kicking off a free cloud agent to fix these issues. This run is complimentary, but you can enable autofix for all future PRs in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 615ec62. Configure here.

@cursor
Copy link
Copy Markdown

cursor bot commented Apr 9, 2026

You have used all of your free Bugbot PR reviews.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 9, 2026

Rollback analysis complete - label added.

@cursor
Copy link
Copy Markdown

cursor bot commented Apr 9, 2026

You have used all of your free Bugbot PR reviews.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 9, 2026

Rollback analysis complete - label added.

@fabrizzio-dotCMS fabrizzio-dotCMS added this pull request to the merge queue Apr 10, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 10, 2026
@fabrizzio-dotCMS fabrizzio-dotCMS added this pull request to the merge queue Apr 10, 2026
Merged via the queue into main with commit 054c44c Apr 10, 2026
49 checks passed
@fabrizzio-dotCMS fabrizzio-dotCMS deleted the issue-34933-integration-1 branch April 10, 2026 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code Area : Documentation PR changes documentation files

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[TASK] Migrate ESMappingAPIImpl

3 participants