feat(search): check the index schema on startup and refuse breaking changes - #3098
Conversation
Not up to standards ⛔🟢 Coverage
|
| Metric | Results |
|---|---|
| Coverage variation | ✅ +0.16% coverage variation (-1.00%) |
| Diff coverage | ✅ 62.86% diff coverage |
Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (1b697e4) 82737 19353 23.39% Head commit (a1b0535) 82984 (+247) 19541 (+188) 23.55% (+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) 350 220 62.86% 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") |
| 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, | ||
| ) |
…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
… 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
00341ce to
2bac094
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
services/search/pkg/opensearch/index.go:224
propertiesMapclaims to return an empty map on missing/empty input, but unmarshalling"null"into a map sets it tonil. Returning a nil map is easy to mishandle later and contradicts the function comment. Normalize empty/nullto an empty map and handle unmarshal errors explicitly.
// propertiesMap parses a raw mappings.properties object; missing or empty
// input yields an empty map, which classifies as purely additive.
func propertiesMap(raw string) map[string]any {
props := map[string]any{}
_ = json.Unmarshal([]byte(raw), &props)
services/search/pkg/opensearch/index.go:218
jsonEqualtreats missing JSON (empty string fromgjson.Result.Rawwhen a path doesn’t exist) as unequal becausejson.Unmarshal([]byte(""), …)fails. This can falsely classify settings as drifting (and refuse startup) when both sides are unset. Normalize empty inputs (and optionallynull) before unmarshalling.
This issue also appears on line 220 of the same file.
func jsonEqual(a, b string) bool {
var av, bv any
if err := json.Unmarshal([]byte(a), &av); err != nil {
return false
}
|
Reopened from the upstream repo as #3197 (identical content and target). The head branch now lives in the upstream repo instead of my fork so follow-up PRs can natively stack on it. Continuing there. |
Superseded by #3197. Reopened from the upstream repo with identical content and target so it can serve as a native-stack parent.
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.Two complementary mechanisms keep the index schema and the code in sync.
Versioned indices (from the base #2659). The index name/path carries the schema version from
search.SchemaVersion(OpenSearchopencloud-resource-v3, blevebleve-v3). An intended breaking schema change is made by bumping the constant: the service targets a fresh, empty index and leaves the old one in place. No migration, no index-to-index reindex; content lives only in the index, the files are the source of truth, so a rescan (opencloud search index --all-spaces) rebuilds everything into the new index.Startup schema check (this PR). The version bump is manual, so this is the safety net for the schema changing without one. On startup both engines diff the existing (current-version) 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.SchemaVersionand gets a fresh index. It fires in development when the mapping is changed in a breaking way without bumping, so the error is developer-facing (bumpsearch.SchemaVersion, or revert the change) and lists the diff that triggered it.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). A field that is explicitly mapped now but already holds dynamically indexed data of unknown shape is breaking rather than a source of silently wrong results, no search beats wrong search.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.