feat(search): check the index schema on startup and refuse breaking changes#3098
Draft
dschmidt wants to merge 8 commits into
Draft
Conversation
…hanges Both engines now diff the stored/live index schema against the schema generated from code when the service starts. A shared recursive classifier in the mapping package is the single oracle: - equal: start normally. - additive (new fields without any indexed data): applied in place. OpenSearch gets a PUT _mapping with the full code properties, bleve persists the code mapping into the index (SetInternal + reopen) so the new fields are properly typed immediately and later startups classify equal. A startup warning lists the new fields because documents indexed before the upgrade lack them until re-indexed. - breaking (changed definitions or analyzers, removed or renamed fields, or new fields that already contain data of unknown form): refuse to start with an error describing the rebuild procedure (delete the index, start, run "opencloud search index --all-spaces") and the OC_EXCLUDE_RUN_SERVICES=search escape hatch. PUT _mapping is deliberately only the apply mechanism, never the judge: its merge semantics cannot see removals or renames and it accepts in-place updatable param changes with an ack. bleve additionally checks idx.Fields() so previously dynamically indexed data (which leaves no schema trace in bleve) is caught, matching by exact name and by path prefix. While at it: the OpenSearch startup check runs with a real, minute-bounded context instead of context.TODO(), bleve indexes are opened with a 5s bolt_timeout so a second process fails fast instead of hanging on the file lock, and the reversed errors.Is arguments in bleve.NewIndex were fixed. opencloud-eu#3092
Not up to standards ⛔🟢 Coverage
|
| Metric | Results |
|---|---|
| Coverage variation | ✅ +0.16% coverage variation (-1.00%) |
| Diff coverage | ✅ 70.93% diff coverage |
Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (599022e) 82816 19399 23.42% Head commit (00341ce) 83036 (+220) 19584 (+185) 23.58% (+0.16%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3098) 289 205 70.93% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull request overview
This PR adds a shared, recursive schema-diff classifier and uses it at startup for both OpenSearch and bleve to (1) start normally when schemas match, (2) apply additive mapping changes in-place while warning about missing fields on pre-upgrade documents, and (3) refuse to start on breaking changes with an operator-facing rebuild procedure.
Changes:
- Introduces
services/search/pkg/mapping.Classify+ sharedErrManualActionRequired/error builder used by both engines. - Updates OpenSearch startup to create-or-compare index schema, apply additive changes via
PUT _mapping, and fail hard on breaking diffs. - Updates bleve startup to compare stored vs code schema (including “data exists for previously-dynamic fields”), persist additive schema changes into the index, and fail hard on breaking diffs; adds targeted tests for both engines.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| services/search/pkg/opensearch/index.go | Create-or-compare index on startup; classify mapping diff; apply additive changes via PUT _mapping; return shared manual-action error on breaking diffs. |
| services/search/pkg/opensearch/index_test.go | Expands OpenSearch index-manager tests for idempotency, additive updates, breaking diffs, and transport-error behavior. |
| services/search/pkg/opensearch/backend.go | Plumbs context + logger through backend creation and schema application/health checks. |
| services/search/pkg/opensearch/backend_test.go | Updates OpenSearch backend tests to the new NewBackend(ctx, ..., logger) signature. |
| services/search/pkg/mapping/classify.go | New shared classifier and shared ErrManualActionRequired + operator-facing error message builder. |
| services/search/pkg/mapping/classify_test.go | Unit tests covering equal/additive/breaking classification cases (including data-aware callback). |
| services/search/pkg/mapping/bleve.go | Tightens comment to require registering all referenced analyzers (validated by IndexMapping.Validate). |
| services/search/pkg/command/server.go | Uses bleve classification to warn on additive schema changes; bounds OpenSearch startup checks with a timeout context. |
| services/search/pkg/bleve/index.go | New bleve open/create logic with schema classification, additive persistence via SetInternal("_mapping"), and breaking-change refusal. |
| services/search/pkg/bleve/index_test.go | New tests validating bleve’s equal/additive/breaking behaviors plus analyzer-registration validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return fmt.Errorf("failed to update mapping of index %s: not acknowledged", name) | ||
| } | ||
|
|
||
| logger.Info().Strs("fields", classification.NewFields).Str("index", name).Msg("extended the search index mapping with new fields") |
Comment on lines
+20
to
+30
| return fmt.Errorf( | ||
| "%w: search index %s was built with a different schema (%s). "+ | ||
| "There is no in-place migration: stop the service, delete %s, "+ | ||
| "start the service (an empty index with the new schema is created), "+ | ||
| "then rebuild the content by running: opencloud search index --all-spaces. "+ | ||
| "To bring the instance up without search until a maintenance window, "+ | ||
| "set OC_EXCLUDE_RUN_SERVICES=search; until the service is back, search "+ | ||
| "and features built on it (e.g. the search bar and the tag list) are "+ | ||
| "unavailable", | ||
| ErrManualActionRequired, index, strings.Join(reasons, "; "), index, | ||
| ) |
… delete step Addresses the two Copilot review comments on the PR: the additive opensearch log now matches the bleve warning (level and re-index hint), and the refuse message spells out how to delete the index per engine (DELETE /<name> vs removing the bleve directory).
- the additive warnings advertise --all-spaces --force-rescan; a plain walk skips unchanged documents and never backfills the new fields - Apply checks index existence first again, so a pre-provisioned index needs no create privilege and odd create-error shapes (string error bodies, cluster blocks) cannot fail a healthy startup; Create on 404 keeps the typed already-exists swallow as the creation-race backstop - number_of_replicas drift is not breaking, it is runtime-tunable and needs no rebuild - bleve returns the classification alongside post-persist errors and the server warns before the error check, so the one-time additive warning is not lost when close or reopen fails - a golden fixture pins the marshaled bleve mapping so a dependency bump that changes marshaling fails in CI instead of refusing every installation in the field
This was referenced Jul 15, 2026
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.
Stacked on #2659. The base
tmp/refactor-search-mappingis a temporary upstream snapshot of the #2659 branch so this PR shows only its own commits; it gets retargeted tomain(and the temp branch deleted) once #2659 is merged. Both PRs need to ship in the same release. Refs #3092.On startup, both engines diff the existing index schema against the schema generated from code, using one shared recursive classifier:
PUT _mappingwith the full code properties. bleve: the code mapping is persisted into the index (SetInternal+ reopen), so new fields are typed correctly right away and the next start classifies equal. A startup warning lists the new fields, documents indexed before the upgrade lack them until re-indexed.opencloud search index --all-spaces) and points toOC_EXCLUDE_RUN_SERVICES=searchfor running without search until a maintenance window.Why this shape:
PUT _mappingis just the apply mechanism: its merge semantics hide removals/renames and it acks in-place updatable param changes, so it must not judge.idx.Fields()(exact name + path prefix). This deliberately makes the current upgrade breaking on bleve too: starting against mistyped Mtime/geo data would silently return wrong results, and no search beats wrong search.Breaking change: every existing installation has to rebuild the search index once (procedure above).
Known and accepted, no code changes:
bolt_timeout) instead of hanging forever.Deliberate consequences of the classifier rules (not bugs):
PUT _mapping/ persisted bleve mapping), the previous release sees a stored-only field and refuses to start. Roll forward or rebuild; refuse-in-both-directions is what keeps mismatched schemas from ever serving queries.