From 03dfabddbbee8bfeec3a6746bd99974e077d70d6 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 8 Jul 2026 21:17:31 +0200 Subject: [PATCH 01/25] feat(search): check the index schema on startup and refuse breaking changes 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. https://github.com/opencloud-eu/opencloud/issues/3092 --- .../change-search-index-schema-handling.md | 22 ++ services/search/pkg/bleve/index.go | 149 ++++++++++++- services/search/pkg/bleve/index_test.go | 170 ++++++++++++++ services/search/pkg/command/server.go | 17 +- services/search/pkg/mapping/bleve.go | 6 +- services/search/pkg/mapping/classify.go | 169 ++++++++++++++ services/search/pkg/mapping/classify_test.go | 129 +++++++++++ services/search/pkg/opensearch/backend.go | 9 +- .../search/pkg/opensearch/backend_test.go | 5 +- services/search/pkg/opensearch/index.go | 210 +++++++++--------- services/search/pkg/opensearch/index_test.go | 98 +++++++- 11 files changed, 860 insertions(+), 124 deletions(-) create mode 100644 changelog/unreleased/change-search-index-schema-handling.md create mode 100644 services/search/pkg/mapping/classify.go create mode 100644 services/search/pkg/mapping/classify_test.go diff --git a/changelog/unreleased/change-search-index-schema-handling.md b/changelog/unreleased/change-search-index-schema-handling.md new file mode 100644 index 0000000000..1177da09ca --- /dev/null +++ b/changelog/unreleased/change-search-index-schema-handling.md @@ -0,0 +1,22 @@ +Change: Check the search index schema on startup (existing indexes need a rebuild) + +The search service now compares the schema of an existing search index with +the schema expected by the code at startup, for both the bleve and the +OpenSearch engine. A purely additive change (new fields that have never been +indexed) is applied in place and the service starts; documents indexed before +the upgrade do not contain the new fields until they are re-indexed. Any other +difference (changed field definitions, changed analyzers, removed or renamed +fields, or new fields that already contain data of unknown form) makes the +service refuse to start instead of silently returning wrong or incomplete +search results. + +Upgrading to this version is a breaking change for BOTH engines: every search +index built by a previous version differs from the new schema and the service +will refuse to start. To rebuild: stop the service, delete the search index +(the bleve directory or the OpenSearch index), start the service (an empty +index with the new schema is created) and run +"opencloud search index --all-spaces" to re-index all files. To bring an +instance up without search until a maintenance window, set +OC_EXCLUDE_RUN_SERVICES=search. + +https://github.com/opencloud-eu/opencloud/issues/3092 diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index 9dd7d290ed..1c226e5ea4 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -1,11 +1,15 @@ package bleve import ( + "encoding/json" "errors" "fmt" + "maps" "math" "path/filepath" "reflect" + "slices" + "strings" "github.com/blevesearch/bleve/v2" "github.com/blevesearch/bleve/v2/analysis/analyzer/custom" @@ -20,23 +24,156 @@ import ( "github.com/opencloud-eu/opencloud/services/search/pkg/search" ) -func NewIndex(root string) (bleve.Index, error) { +// bolt_timeout makes a second process on the same datapath fail after 5s +// instead of blocking forever on the file lock. +var openRuntimeConfig = map[string]interface{}{"bolt_timeout": "5s"} + +// NewIndex opens (or creates) the bleve index at root and classifies the +// stored schema against the one generated from code. On a breaking change it +// refuses with ErrManualActionRequired. On an additive one the code schema is +// persisted into the index (the bleve analogue of an OpenSearch PUT _mapping), +// so the new fields are properly typed from now on and later startups classify +// equal; the caller must still warn that documents indexed before the upgrade +// lack the Classification.NewFields until they are re-indexed. +func NewIndex(root string) (bleve.Index, searchmapping.Classification, error) { destination := filepath.Join(root, fmt.Sprintf("bleve-v%d", search.SchemaVersion)) - index, err := bleve.Open(destination) + index, err := bleve.OpenUsing(destination, openRuntimeConfig) if errors.Is(err, bleve.ErrorIndexPathDoesNotExist) { indexMapping, err := NewMapping() if err != nil { - return nil, err + return nil, searchmapping.Classification{}, err } index, err = bleve.New(destination, indexMapping) if err != nil { - return nil, err + return nil, searchmapping.Classification{}, err } - return index, nil + return index, searchmapping.Classification{Verdict: searchmapping.VerdictEqual}, nil + } + if err != nil { + return nil, searchmapping.Classification{}, err } - return index, err + classification, codeB, err := classifyStoredMapping(index) + if err != nil { + _ = index.Close() + return nil, searchmapping.Classification{}, err + } + + switch classification.Verdict { + case searchmapping.VerdictBreaking: + _ = index.Close() + return nil, classification, searchmapping.ManualActionRequiredError(destination, classification.Reasons) + case searchmapping.VerdictAdditive: + // The classifier guarantees everything else is identical and the new + // fields hold no data yet, so storing the code mapping only adds + // fields. Reopen so the live mapping picks it up: without this the + // new fields would be indexed dynamically and the data-aware rule + // would turn them breaking on the next startup. + if err := index.SetInternal([]byte("_mapping"), codeB); err != nil { + _ = index.Close() + return nil, searchmapping.Classification{}, fmt.Errorf("failed to store the updated index mapping: %w", err) + } + if err := index.Close(); err != nil { + return nil, searchmapping.Classification{}, err + } + index, err = bleve.OpenUsing(destination, openRuntimeConfig) + if err != nil { + return nil, searchmapping.Classification{}, err + } + } + + return index, classification, nil +} + +// classifyStoredMapping diffs the mapping stored in the index against +// NewMapping() and also returns the marshaled code mapping. Fields that are +// new in the code schema but already have data in the index (previously +// indexed dynamically) are breaking, their de-facto form is unknown. The JSON +// compare is stable within one bleve version; if a bleve upgrade changes +// marshaling defaults it fails towards breaking, normalize the affected key +// here if that ever fires. +func classifyStoredMapping(index bleve.Index) (searchmapping.Classification, []byte, error) { + storedB, err := index.GetInternal([]byte("_mapping")) + if err != nil { + return searchmapping.Classification{}, nil, fmt.Errorf("failed to read the stored index mapping: %w", err) + } + codeMapping, err := NewMapping() + if err != nil { + return searchmapping.Classification{}, nil, err + } + codeB, err := json.Marshal(codeMapping) + if err != nil { + return searchmapping.Classification{}, nil, err + } + + var stored, code map[string]any + if err := json.Unmarshal(storedB, &stored); err != nil { + return searchmapping.Classification{}, nil, fmt.Errorf("failed to parse the stored index mapping: %w", err) + } + if err := json.Unmarshal(codeB, &code); err != nil { + return searchmapping.Classification{}, nil, err + } + + fields, err := index.Fields() + if err != nil { + return searchmapping.Classification{}, nil, fmt.Errorf("failed to list the indexed fields: %w", err) + } + indexedFields := make(map[string]struct{}, len(fields)) + for _, f := range fields { + if !strings.HasPrefix(f, "_") { // skip bleve-internal fields like _all + indexedFields[f] = struct{}{} + } + } + + storedDM, _ := stored["default_mapping"].(map[string]any) + codeDM, _ := code["default_mapping"].(map[string]any) + storedProps, _ := storedDM["properties"].(map[string]any) + codeProps, _ := codeDM["properties"].(map[string]any) + + classification := searchmapping.Classify(storedProps, codeProps, func(path string) bool { + if _, ok := indexedFields[path]; ok { + return true + } + nested := path + "." + for f := range indexedFields { + if strings.HasPrefix(f, nested) { + return true + } + } + return false + }) + + // everything outside default_mapping.properties (analyzer definitions, + // default analyzer, dynamic flags, ...) must match exactly + var reasons []string + compareKeysExcept(stored, code, "default_mapping", "", &reasons) + compareKeysExcept(storedDM, codeDM, "properties", "default_mapping.", &reasons) + if len(reasons) > 0 { + classification.Verdict = searchmapping.VerdictBreaking + classification.Reasons = append(reasons, classification.Reasons...) + } + + return classification, codeB, nil +} + +// compareKeysExcept deep-compares all keys present on either side except skip. +func compareKeysExcept(stored, code map[string]any, skip, prefix string, reasons *[]string) { + keys := slices.Collect(maps.Keys(stored)) + for k := range code { + if _, ok := stored[k]; !ok { + keys = append(keys, k) + } + } + slices.Sort(keys) + for _, k := range keys { + if k == skip { + continue + } + if !reflect.DeepEqual(stored[k], code[k]) { + *reasons = append(*reasons, fmt.Sprintf("%s%s changed", prefix, k)) + } + } } func NewMapping() (mapping.IndexMapping, error) { diff --git a/services/search/pkg/bleve/index_test.go b/services/search/pkg/bleve/index_test.go index b09d2da12a..6629b416af 100644 --- a/services/search/pkg/bleve/index_test.go +++ b/services/search/pkg/bleve/index_test.go @@ -4,10 +4,16 @@ import ( "fmt" "path/filepath" + bleveSearch "github.com/blevesearch/bleve/v2" + "github.com/blevesearch/bleve/v2/analysis/analyzer/custom" + "github.com/blevesearch/bleve/v2/analysis/token/lowercase" + "github.com/blevesearch/bleve/v2/analysis/tokenizer/unicode" + bleveMapping "github.com/blevesearch/bleve/v2/mapping" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/opencloud-eu/opencloud/services/search/pkg/bleve" + searchmapping "github.com/opencloud-eu/opencloud/services/search/pkg/mapping" "github.com/opencloud-eu/opencloud/services/search/pkg/search" ) @@ -39,3 +45,167 @@ var _ = Describe("Index", func() { }) }) }) + +var _ = Describe("NewIndex", func() { + var root string + + BeforeEach(func() { + root = GinkgoT().TempDir() + }) + + codeMapping := func() *bleveMapping.IndexMappingImpl { + m, err := bleve.NewMapping() + Expect(err).ToNot(HaveOccurred()) + impl, ok := m.(*bleveMapping.IndexMappingImpl) + Expect(ok).To(BeTrue()) + return impl + } + + // buildIndex simulates an index left behind by an older release + buildIndex := func(m bleveMapping.IndexMapping, docs map[string]map[string]any) { + idx, err := bleveSearch.New(filepath.Join(root, fmt.Sprintf("bleve-v%d", search.SchemaVersion)), m) + Expect(err).ToNot(HaveOccurred()) + for id, doc := range docs { + Expect(idx.Index(id, doc)).To(Succeed()) + } + Expect(idx.Close()).To(Succeed()) + } + + It("creates a fresh index", func() { + idx, classification, err := bleve.NewIndex(root) + Expect(err).ToNot(HaveOccurred()) + Expect(classification.Verdict).To(Equal(searchmapping.VerdictEqual)) + Expect(idx.Close()).To(Succeed()) + }) + + It("opens an index with an identical schema", func() { + buildIndex(codeMapping(), nil) + + idx, classification, err := bleve.NewIndex(root) + Expect(err).ToNot(HaveOccurred()) + Expect(classification.Verdict).To(Equal(searchmapping.VerdictEqual)) + Expect(classification.NewFields).To(BeEmpty()) + Expect(idx.Close()).To(Succeed()) + }) + + It("treats a genuinely new field as additive", func() { + old := codeMapping() + Expect(old.DefaultMapping.Properties).To(HaveKey("Title")) + delete(old.DefaultMapping.Properties, "Title") + buildIndex(old, nil) + + idx, classification, err := bleve.NewIndex(root) + Expect(err).ToNot(HaveOccurred()) + Expect(classification.Verdict).To(Equal(searchmapping.VerdictAdditive)) + Expect(classification.NewFields).To(ConsistOf("Title")) + Expect(idx.Index("1", map[string]any{"Title": "hello"})).To(Succeed()) + Expect(idx.Close()).To(Succeed()) + }) + + It("treats a new nested field as additive", func() { + old := codeMapping() + photo := old.DefaultMapping.Properties["photo"] + Expect(photo).ToNot(BeNil()) + Expect(photo.Properties).To(HaveKey("cameraMake")) + delete(photo.Properties, "cameraMake") + buildIndex(old, nil) + + idx, classification, err := bleve.NewIndex(root) + Expect(err).ToNot(HaveOccurred()) + Expect(classification.Verdict).To(Equal(searchmapping.VerdictAdditive)) + Expect(classification.NewFields).To(ConsistOf("photo.cameraMake")) + Expect(idx.Close()).To(Succeed()) + }) + + It("persists an additive schema change so later startups classify it as equal", func() { + old := codeMapping() + delete(old.DefaultMapping.Properties, "Title") + buildIndex(old, nil) + + idx, classification, err := bleve.NewIndex(root) + Expect(err).ToNot(HaveOccurred()) + Expect(classification.Verdict).To(Equal(searchmapping.VerdictAdditive)) + Expect(idx.Index("1", map[string]any{"Title": "hello"})).To(Succeed()) + Expect(idx.Close()).To(Succeed()) + + idx, classification, err = bleve.NewIndex(root) + Expect(err).ToNot(HaveOccurred()) + Expect(classification.Verdict).To(Equal(searchmapping.VerdictEqual)) + Expect(idx.Close()).To(Succeed()) + }) + + It("refuses when a new field already has data in the index", func() { + old := codeMapping() + Expect(old.DefaultMapping.Properties).To(HaveKey("Mtime")) + delete(old.DefaultMapping.Properties, "Mtime") + buildIndex(old, map[string]map[string]any{"1": {"Mtime": "2026-01-02T03:04:05Z"}}) + + idx, _, err := bleve.NewIndex(root) + Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) + Expect(idx).To(BeNil()) + }) + + It("refuses when a new object field already has nested data in the index", func() { + old := codeMapping() + Expect(old.DefaultMapping.Properties).To(HaveKey("photo")) + delete(old.DefaultMapping.Properties, "photo") + buildIndex(old, map[string]map[string]any{"1": {"photo": map[string]any{"cameraMake": "ACME"}}}) + + _, _, err := bleve.NewIndex(root) + Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) + }) + + It("refuses on a changed field definition", func() { + old := codeMapping() + name := old.DefaultMapping.Properties["Name"] + Expect(name).ToNot(BeNil()) + Expect(name.Fields).ToNot(BeEmpty()) + name.Fields[0].Analyzer = "fulltext" + buildIndex(old, nil) + + _, _, err := bleve.NewIndex(root) + Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) + }) + + It("refuses when a stored field was removed from the code schema", func() { + old := codeMapping() + old.DefaultMapping.AddFieldMappingsAt("Legacy", bleveSearch.NewTextFieldMapping()) + buildIndex(old, nil) + + _, _, err := bleve.NewIndex(root) + Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) + }) + + It("refuses when a default_mapping attribute changed", func() { + old := codeMapping() + old.DefaultMapping.Dynamic = false + buildIndex(old, nil) + + _, _, err := bleve.NewIndex(root) + Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) + }) + + It("refuses on a changed analyzer definition", func() { + old := codeMapping() + Expect(old.CustomAnalysis.Analyzers).To(HaveKey("fulltext")) + old.CustomAnalysis.Analyzers["fulltext"] = map[string]any{ + "type": custom.Name, + "tokenizer": unicode.Name, + "token_filters": []string{lowercase.Name}, + } + buildIndex(old, nil) + + _, _, err := bleve.NewIndex(root) + Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) + }) +}) + +var _ = Describe("NewMapping", func() { + It("only references registered analyzers", func() { + m, err := bleve.NewMapping() + Expect(err).ToNot(HaveOccurred()) + impl, ok := m.(*bleveMapping.IndexMappingImpl) + Expect(ok).To(BeTrue()) + Expect(impl.Validate()).To(Succeed()) + }) +}) diff --git a/services/search/pkg/command/server.go b/services/search/pkg/command/server.go index a93338f79c..e494c177fe 100644 --- a/services/search/pkg/command/server.go +++ b/services/search/pkg/command/server.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "os/signal" + "time" "github.com/opencloud-eu/opencloud/pkg/config/configlog" "github.com/opencloud-eu/opencloud/pkg/generators" @@ -20,6 +21,7 @@ import ( "github.com/opencloud-eu/opencloud/services/search/pkg/config" "github.com/opencloud-eu/opencloud/services/search/pkg/config/parser" "github.com/opencloud-eu/opencloud/services/search/pkg/content" + searchmapping "github.com/opencloud-eu/opencloud/services/search/pkg/mapping" "github.com/opencloud-eu/opencloud/services/search/pkg/metrics" "github.com/opencloud-eu/opencloud/services/search/pkg/opensearch" bleveQuery "github.com/opencloud-eu/opencloud/services/search/pkg/query/bleve" @@ -71,11 +73,17 @@ func Server(cfg *config.Config) *cobra.Command { var eng search.Engine switch cfg.Engine.Type { case "bleve": - idx, err := bleve.NewIndex(cfg.Engine.Bleve.Datapath) + idx, classification, err := bleve.NewIndex(cfg.Engine.Bleve.Datapath) if err != nil { return err } + if classification.Verdict == searchmapping.VerdictAdditive { + logger.Warn(). + Strs("fields", classification.NewFields). + Msgf("the bleve index at %s was built with an older schema; the new fields were added to the index schema, but documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces", cfg.Engine.Bleve.Datapath) + } + defer func() { if err = idx.Close(); err != nil { logger.Error().Err(err).Msg("could not close bleve index") @@ -119,8 +127,11 @@ func Server(cfg *config.Config) *cobra.Command { return fmt.Errorf("failed to create OpenSearch client: %w", err) } - indexName := opensearch.VersionedIndexName(cfg.Engine.OpenSearch.ResourceIndex.Name) - openSearchBackend, err := opensearch.NewBackend(indexName, client) + // bound the startup schema check so a hung cluster fails the + // start instead of blocking forever + startupCtx, cancelStartup := context.WithTimeout(ctx, time.Minute) + openSearchBackend, err := opensearch.NewBackend(startupCtx, cfg.Engine.OpenSearch.ResourceIndex.Name, client, logger) + cancelStartup() if err != nil { return fmt.Errorf("failed to create OpenSearch backend: %w", err) } diff --git a/services/search/pkg/mapping/bleve.go b/services/search/pkg/mapping/bleve.go index 9ee2d9cb82..48fc9424e3 100644 --- a/services/search/pkg/mapping/bleve.go +++ b/services/search/pkg/mapping/bleve.go @@ -12,8 +12,10 @@ import ( // struct via reflection. Field names come from json tags; overrides are // keyed by those names (or dotted paths for nested fields). // -// The returned mapping references the words analyzer for Fulltext fields; -// the caller registers it on the enclosing IndexMapping. +// The returned mapping references analyzer names (Analyzer on the FieldOpts, +// plus the words analyzer for the Fulltext type); the caller registers every +// referenced analyzer on the enclosing IndexMapping (IndexMapping.Validate +// catches missing ones). func BleveBuildMapping(t reflect.Type, overrides map[string]FieldOpts) (*bleveMapping.DocumentMapping, error) { return buildBleveDocMapping(t, overrides, "") } diff --git a/services/search/pkg/mapping/classify.go b/services/search/pkg/mapping/classify.go new file mode 100644 index 0000000000..9fa357ef7c --- /dev/null +++ b/services/search/pkg/mapping/classify.go @@ -0,0 +1,169 @@ +package mapping + +import ( + "encoding/json" + "errors" + "fmt" + "maps" + "reflect" + "slices" + "strings" +) + +// ErrManualActionRequired marks schema changes that cannot be applied in place. +var ErrManualActionRequired = errors.New("manual action required") + +// ManualActionRequiredError builds the operator-facing error for a breaking +// schema change. index is the index name (OpenSearch) or path (bleve). +func ManualActionRequiredError(index string, reasons []string) error { + 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", + ErrManualActionRequired, index, strings.Join(reasons, "; "), index, + ) +} + +type Verdict string + +const ( + VerdictEqual Verdict = "equal" + VerdictAdditive Verdict = "additive" + VerdictBreaking Verdict = "breaking" +) + +// Classification is the outcome of diffing a stored index schema against the +// schema generated from code. +type Classification struct { + Verdict Verdict + // NewFields are dotted paths of fields that only exist in the code schema. + NewFields []string + // Reasons are human-readable breaking differences. + Reasons []string +} + +// Classify recursively compares a stored `properties` tree against the one +// generated from code. Both sides must be generic JSON-decoded values +// (map[string]any, []any, float64), not marshaled Go structs, so values +// compare structurally. +// +// dataFields reports whether the index holds data at or below a dotted field +// path even though it is absent from the stored schema (bleve indexes dynamic +// fields without a schema trace). Engines that record dynamic fields in the +// live schema (OpenSearch) pass nil. +func Classify(stored, code map[string]any, dataFields func(path string) bool) Classification { + c := Classification{Verdict: VerdictEqual} + classifyProperties(stored, code, dataFields, "", &c) + if c.Verdict == VerdictEqual && len(c.NewFields) > 0 { + c.Verdict = VerdictAdditive + } + return c +} + +func classifyProperties(stored, code map[string]any, dataFields func(string) bool, prefix string, c *Classification) { + for _, k := range slices.Sorted(maps.Keys(stored)) { + path := joinPath(prefix, k) + codeNode, ok := code[k] + if !ok { + c.breaking(fmt.Sprintf("field %s exists in the index but not in the code schema (removed or renamed)", path)) + continue + } + classifyNode(stored[k], codeNode, dataFields, path, c) + } + + for _, k := range slices.Sorted(maps.Keys(code)) { + if _, ok := stored[k]; ok { + continue + } + path := joinPath(prefix, k) + if dataFields != nil && dataFields(path) { + c.breaking(fmt.Sprintf("field %s is new in the code schema but the index already contains data for it (previously indexed dynamically)", path)) + continue + } + c.NewFields = append(c.NewFields, leafPaths(code[k], path)...) + } +} + +func classifyNode(stored, code any, dataFields func(string) bool, path string, c *Classification) { + storedMap, sOK := stored.(map[string]any) + codeMap, cOK := code.(map[string]any) + if !sOK || !cOK { + if !reflect.DeepEqual(stored, code) { + c.breaking(fmt.Sprintf("field %s changed: index %s, code %s", path, compactJSON(stored), compactJSON(code))) + } + return + } + + for _, k := range sortedUnionKeys(storedMap, codeMap) { + if k == "properties" { + continue + } + sv, sHas := storedMap[k] + cv, cHas := codeMap[k] + if sHas && cHas && reflect.DeepEqual(sv, cv) { + continue + } + c.breaking(fmt.Sprintf("field %s: %s changed: index %s, code %s", path, k, optJSON(sv, sHas), optJSON(cv, cHas))) + } + + storedProps, _ := storedMap["properties"].(map[string]any) + codeProps, _ := codeMap["properties"].(map[string]any) + if len(storedProps) > 0 || len(codeProps) > 0 { + classifyProperties(storedProps, codeProps, dataFields, path, c) + } +} + +// leafPaths lists the dotted paths of all leaf fields at or below node. +func leafPaths(node any, path string) []string { + if nodeMap, ok := node.(map[string]any); ok { + if props, ok := nodeMap["properties"].(map[string]any); ok && len(props) > 0 { + var leaves []string + for _, k := range slices.Sorted(maps.Keys(props)) { + leaves = append(leaves, leafPaths(props[k], path+"."+k)...) + } + return leaves + } + } + return []string{path} +} + +func (c *Classification) breaking(reason string) { + c.Verdict = VerdictBreaking + c.Reasons = append(c.Reasons, reason) +} + +func joinPath(prefix, k string) string { + if prefix == "" { + return k + } + return prefix + "." + k +} + +func sortedUnionKeys(a, b map[string]any) []string { + keys := slices.Collect(maps.Keys(a)) + for k := range b { + if _, ok := a[k]; !ok { + keys = append(keys, k) + } + } + slices.Sort(keys) + return keys +} + +func optJSON(v any, present bool) string { + if !present { + return "(unset)" + } + return compactJSON(v) +} + +func compactJSON(v any) string { + b, err := json.Marshal(v) + if err != nil { + return fmt.Sprintf("%v", v) + } + return string(b) +} diff --git a/services/search/pkg/mapping/classify_test.go b/services/search/pkg/mapping/classify_test.go new file mode 100644 index 0000000000..37ea5fbd1b --- /dev/null +++ b/services/search/pkg/mapping/classify_test.go @@ -0,0 +1,129 @@ +package mapping + +import ( + "encoding/json" + "slices" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Classify", func() { + code := `{ + "Name": {"type": "keyword"}, + "Size": {"type": "long"}, + "photo": {"properties": {"cameraMake": {"type": "keyword"}, "cameraModel": {"type": "keyword"}}} + }` + + parse := func(s string) map[string]any { + var m map[string]any + Expect(json.Unmarshal([]byte(s), &m)).To(Succeed()) + return m + } + + hasData := func(fields ...string) func(string) bool { + return func(path string) bool { return slices.Contains(fields, path) } + } + + It("classifies identical schemas as equal", func() { + c := Classify(parse(code), parse(code), nil) + Expect(c.Verdict).To(Equal(VerdictEqual)) + Expect(c.NewFields).To(BeEmpty()) + Expect(c.Reasons).To(BeEmpty()) + }) + + It("classifies a new top-level field as additive", func() { + stored := parse(code) + delete(stored, "Size") + + c := Classify(stored, parse(code), nil) + Expect(c.Verdict).To(Equal(VerdictAdditive)) + Expect(c.NewFields).To(ConsistOf("Size")) + Expect(c.Reasons).To(BeEmpty()) + }) + + It("classifies a new nested field as additive", func() { + stored := parse(code) + delete(stored["photo"].(map[string]any)["properties"].(map[string]any), "cameraMake") + + c := Classify(stored, parse(code), nil) + Expect(c.Verdict).To(Equal(VerdictAdditive)) + Expect(c.NewFields).To(ConsistOf("photo.cameraMake")) + }) + + It("lists every leaf of a new subtree", func() { + stored := parse(code) + delete(stored, "photo") + + c := Classify(stored, parse(code), nil) + Expect(c.Verdict).To(Equal(VerdictAdditive)) + Expect(c.NewFields).To(ConsistOf("photo.cameraMake", "photo.cameraModel")) + }) + + It("breaks when a new field already has data in the index", func() { + stored := parse(code) + delete(stored, "Size") + + c := Classify(stored, parse(code), hasData("Size")) + Expect(c.Verdict).To(Equal(VerdictBreaking)) + Expect(c.Reasons).To(ConsistOf(ContainSubstring("Size"))) + }) + + It("breaks when a new nested field already has data in the index", func() { + stored := parse(code) + delete(stored["photo"].(map[string]any)["properties"].(map[string]any), "cameraMake") + + c := Classify(stored, parse(code), hasData("photo.cameraMake")) + Expect(c.Verdict).To(Equal(VerdictBreaking)) + Expect(c.Reasons).To(ConsistOf(ContainSubstring("photo.cameraMake"))) + }) + + It("breaks when a new subtree already has data below it", func() { + stored := parse(code) + delete(stored, "photo") + + // the callback is consulted with the subtree root; reporting data at + // or below it is the caller's job + c := Classify(stored, parse(code), hasData("photo")) + Expect(c.Verdict).To(Equal(VerdictBreaking)) + Expect(c.Reasons).To(ConsistOf(ContainSubstring("photo"))) + }) + + It("breaks on a changed field definition", func() { + stored := parse(code) + stored["Size"].(map[string]any)["type"] = "keyword" + + c := Classify(stored, parse(code), nil) + Expect(c.Verdict).To(Equal(VerdictBreaking)) + Expect(c.Reasons).To(ConsistOf(ContainSubstring("Size"))) + }) + + It("breaks on a field that was removed from the code schema", func() { + reduced := parse(code) + delete(reduced, "Size") + + c := Classify(parse(code), reduced, nil) + Expect(c.Verdict).To(Equal(VerdictBreaking)) + Expect(c.Reasons).To(ConsistOf(ContainSubstring("removed or renamed"))) + }) + + It("breaks on a changed object attribute", func() { + stored := parse(code) + stored["photo"].(map[string]any)["dynamic"] = true + + c := Classify(stored, parse(code), nil) + Expect(c.Verdict).To(Equal(VerdictBreaking)) + Expect(c.Reasons).To(ConsistOf(ContainSubstring("dynamic"))) + }) + + It("lets breaking win over additive", func() { + stored := parse(code) + delete(stored, "Size") + stored["Name"].(map[string]any)["type"] = "text" + + c := Classify(stored, parse(code), nil) + Expect(c.Verdict).To(Equal(VerdictBreaking)) + Expect(c.NewFields).To(ConsistOf("Size")) + Expect(c.Reasons).To(ConsistOf(ContainSubstring("Name"))) + }) +}) diff --git a/services/search/pkg/opensearch/backend.go b/services/search/pkg/opensearch/backend.go index 49cf67fc54..330dcd577d 100644 --- a/services/search/pkg/opensearch/backend.go +++ b/services/search/pkg/opensearch/backend.go @@ -14,6 +14,7 @@ import ( "github.com/opencloud-eu/opencloud/pkg/conversions" "github.com/opencloud-eu/opencloud/pkg/kql" + "github.com/opencloud-eu/opencloud/pkg/log" searchMessage "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/messages/search/v0" searchService "github.com/opencloud-eu/opencloud/protogen/gen/opencloud/services/search/v0" "github.com/opencloud-eu/opencloud/services/search/pkg/opensearch/internal/convert" @@ -33,10 +34,10 @@ type Backend struct { } // NewBackend creates a backend on the versioned generation of the named index. -func NewBackend(name string, client *opensearchgoAPI.Client) (*Backend, error) { +func NewBackend(ctx context.Context, name string, client *opensearchgoAPI.Client, logger log.Logger) (*Backend, error) { index := VersionedIndexName(name) - pingResp, err := client.Ping(context.TODO(), &opensearchgoAPI.PingReq{}) + pingResp, err := client.Ping(ctx, &opensearchgoAPI.PingReq{}) switch { case err != nil: return nil, fmt.Errorf("%w, failed to ping opensearch: %w", ErrUnhealthyCluster, err) @@ -45,13 +46,13 @@ func NewBackend(name string, client *opensearchgoAPI.Client) (*Backend, error) { } // apply the index template - if err := IndexManagerLatest.Apply(context.TODO(), index, client); err != nil { + if err := IndexManagerLatest.Apply(ctx, index, client, logger); err != nil { return nil, fmt.Errorf("failed to apply index template: %w", err) } // first check if the cluster is healthy - resp, err := client.Cluster.Health(context.TODO(), &opensearchgoAPI.ClusterHealthReq{ + resp, err := client.Cluster.Health(ctx, &opensearchgoAPI.ClusterHealthReq{ Indices: []string{index}, Params: opensearchgoAPI.ClusterHealthParams{ Local: opensearchgoAPI.ToPointer(true), diff --git a/services/search/pkg/opensearch/backend_test.go b/services/search/pkg/opensearch/backend_test.go index dc83e057b8..94f9e31555 100644 --- a/services/search/pkg/opensearch/backend_test.go +++ b/services/search/pkg/opensearch/backend_test.go @@ -9,6 +9,7 @@ import ( opensearchgo "github.com/opensearch-project/opensearch-go/v4" opensearchgoAPI "github.com/opensearch-project/opensearch-go/v4/opensearchapi" + "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/search/internal/opensearchtest" "github.com/opencloud-eu/opencloud/services/search/pkg/opensearch" ) @@ -34,7 +35,7 @@ var _ = Describe("Backend", func() { }) Expect(err).ToNot(HaveOccurred(), "failed to create OpenSearch client") - backend, err := opensearch.NewBackend("test-engine-new-engine", client) + backend, err := opensearch.NewBackend(context.Background(), "test-engine-new-engine", client, log.NopLogger()) Expect(backend).To(BeNil()) Expect(err).To(MatchError(opensearch.ErrUnhealthyCluster)) }) @@ -57,7 +58,7 @@ var _ = Describe("Backend", func() { deleteIndexOnCleanup(tc, physical) var err error - backend, err = opensearch.NewBackend(indexName, tc.Client()) + backend, err = opensearch.NewBackend(context.Background(), indexName, tc.Client(), log.NopLogger()) Expect(err).ToNot(HaveOccurred()) }) diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index bbc38640f4..81b291da9b 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -3,21 +3,25 @@ package opensearch import ( "bytes" "context" + "encoding/json" "errors" "fmt" "maps" "reflect" + "strings" - "github.com/go-jose/go-jose/v3/json" + opensearchgo "github.com/opensearch-project/opensearch-go/v4" opensearchgoAPI "github.com/opensearch-project/opensearch-go/v4/opensearchapi" "github.com/tidwall/gjson" + "github.com/opencloud-eu/opencloud/pkg/log" searchmapping "github.com/opencloud-eu/opencloud/services/search/pkg/mapping" "github.com/opencloud-eu/opencloud/services/search/pkg/search" ) var ( - ErrManualActionRequired = errors.New("manual action required") + // ErrManualActionRequired is the shared sentinel, see the mapping package. + ErrManualActionRequired = searchmapping.ErrManualActionRequired // IndexManagerLatest identifies the current resource mapping; its version is // derived from search.SchemaVersion so it never drifts from the index name. @@ -110,120 +114,118 @@ func buildResourceMapping() ([]byte, error) { return json.Marshal(index) } -func coveredAt(declared, index gjson.Result, declaredPath, indexPath string) (string, string, bool) { - declaredRaw := declared.Get(declaredPath).Raw - indexRaw := index.Get(indexPath).Raw - - var declaredValue, indexValue any - if err := json.Unmarshal([]byte(declaredRaw), &declaredValue); err != nil { - return declaredRaw, indexRaw, false - } - - if err := json.Unmarshal([]byte(indexRaw), &indexValue); err != nil { - return declaredRaw, indexRaw, false - } - - return declaredRaw, indexRaw, covered(declaredValue, indexValue) -} - -func covered(declared, index any) bool { - declaredMap, ok := declared.(map[string]any) - if !ok { - return reflect.DeepEqual(declared, index) - } - - indexMap, ok := index.(map[string]any) - if !ok { - return false - } - - for key, declaredValue := range declaredMap { - indexValue, ok := indexMap[key] - if !ok || !covered(declaredValue, indexValue) { - return false - } - } - - return true -} - -func (m IndexManager) Apply(ctx context.Context, name string, client *opensearchgoAPI.Client) error { +// Apply ensures the index exists and matches the schema generated from code. +// A missing index is created, an additive schema change is applied in place +// via PUT _mapping, a breaking one returns ErrManualActionRequired. The +// classifier decides what is additive; PUT _mapping is only the mechanism to +// apply it (its merge semantics cannot detect removals or renames). +func (m IndexManager) Apply(ctx context.Context, name string, client *opensearchgoAPI.Client, logger log.Logger) error { localIndexB, err := m.MarshalJSON() if err != nil { return fmt.Errorf("failed to marshal index %s: %w", name, err) } - indicesExistsResp, err := client.Indices.Exists(ctx, opensearchgoAPI.IndicesExistsReq{ - Indices: []string{name}, - }) - switch { - case indicesExistsResp != nil && indicesExistsResp.StatusCode == 404: - break - case err != nil: - return fmt.Errorf("failed to check if index %s exists: %w", name, err) - case indicesExistsResp == nil: - return fmt.Errorf("indicesExistsResp is nil for index %s", name) - } - - if indicesExistsResp.StatusCode == 200 { - resp, err := client.Indices.Get(ctx, opensearchgoAPI.IndicesGetReq{ - Indices: []string{name}, - }) - if err != nil { - return fmt.Errorf("failed to get index %s: %w", name, err) - } - - remoteIndex, ok := (*resp.IndicesGetRespData)[name] - if !ok { - return fmt.Errorf("index %s not found in response", name) - } - remoteIndexB, err := json.Marshal(remoteIndex) - if err != nil { - return fmt.Errorf("failed to marshal index %s: %w", name, err) - } - - localIndexJson := gjson.ParseBytes(localIndexB) - remoteIndexJson := gjson.ParseBytes(remoteIndexB) - - var errs []error - - for k := range localIndexJson.Get("settings").Map() { - if lv, rv, ok := coveredAt(localIndexJson, remoteIndexJson, "settings."+k, "settings.index."+k); !ok { - errs = append(errs, fmt.Errorf("settings.%s local %s, remote %s", k, lv, rv)) - } - } - - for k := range localIndexJson.Get("mappings.properties").Map() { - if _, _, ok := coveredAt(localIndexJson, remoteIndexJson, "mappings.properties."+k, "mappings.properties."+k); !ok { - errs = append(errs, fmt.Errorf("mappings.properties.%s", k)) - } - } - - if errs != nil { - return fmt.Errorf( - "index %s already exists with a different mapping than the requested version. "+ - "There is no in-place migration today: drop the index in OpenSearch (DELETE /%s) "+ - "and restart the search service. The index will be recreated with the new mapping. "+ - "%w: %w", - name, name, - ErrManualActionRequired, - errors.Join(errs...), - ) - } - - return nil // Index is already up to date, no action needed - } - createResp, err := client.Indices.Create(ctx, opensearchgoAPI.IndicesCreateReq{ Index: name, Body: bytes.NewReader(localIndexB), }) + var createErr *opensearchgo.StructError switch { - case err != nil: - return fmt.Errorf("failed to create index %s: %w", name, err) - case !createResp.Acknowledged: + case err == nil && createResp.Acknowledged: + return nil + case err == nil: return fmt.Errorf("failed to create index %s: not acknowledged", name) + case !errors.As(err, &createErr) || createErr.Err.Type != "resource_already_exists_exception": + // transport errors, disk-full etc. stay plain fatal, the restart policy retries + return fmt.Errorf("failed to create index %s: %w", name, err) } + // the index already exists: compare settings and classify the mapping diff + resp, err := client.Indices.Get(ctx, opensearchgoAPI.IndicesGetReq{ + Indices: []string{name}, + }) + if err != nil { + return fmt.Errorf("failed to get index %s: %w", name, err) + } + + remoteIndex, ok := (*resp.IndicesGetRespData)[name] + if !ok { + return fmt.Errorf("index %s not found in response", name) + } + remoteIndexB, err := json.Marshal(remoteIndex) + if err != nil { + return fmt.Errorf("failed to marshal index %s: %w", name, err) + } + + localIndexJson := gjson.ParseBytes(localIndexB) + remoteIndexJson := gjson.ParseBytes(remoteIndexB) + + var reasons []string + for k := range localIndexJson.Get("settings").Map() { + lv := localIndexJson.Get("settings." + k).Raw + rv := remoteIndexJson.Get("settings.index." + k).Raw + if !jsonEqual(lv, rv) { + reasons = append(reasons, fmt.Sprintf("settings.%s changed: index %s, code %s", k, rawOrUnset(rv), rawOrUnset(lv))) + } + } + + classification := searchmapping.Classify( + propertiesMap(remoteIndexJson.Get("mappings.properties").Raw), + propertiesMap(localIndexJson.Get("mappings.properties").Raw), + nil, + ) + reasons = append(reasons, classification.Reasons...) + if len(reasons) > 0 { + return searchmapping.ManualActionRequiredError(name, reasons) + } + if len(classification.NewFields) == 0 { + return nil // schema is up to date + } + + // additive: the classifier guarantees every existing field matches the + // remote state, so putting the full code properties can only add fields + putResp, err := client.Indices.Mapping.Put(ctx, opensearchgoAPI.MappingPutReq{ + Indices: []string{name}, + Body: strings.NewReader(localIndexJson.Get("mappings").Raw), + }) + var putErr *opensearchgo.StructError + switch { + case err != nil && errors.As(err, &putErr) && putErr.Err.Type == "illegal_argument_exception" && + (strings.Contains(putErr.Err.Reason, "cannot be changed") || strings.Contains(putErr.Err.Reason, "Cannot update parameter")): + // backstop, should be unreachable after the classification above + return searchmapping.ManualActionRequiredError(name, []string{putErr.Err.Reason}) + case err != nil: + return fmt.Errorf("failed to update mapping of index %s: %w", name, err) + case !putResp.Acknowledged: + 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 nil } + +func jsonEqual(a, b string) bool { + var av, bv any + if err := json.Unmarshal([]byte(a), &av); err != nil { + return false + } + if err := json.Unmarshal([]byte(b), &bv); err != nil { + return false + } + return reflect.DeepEqual(av, bv) +} + +// 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) + return props +} + +func rawOrUnset(raw string) string { + if raw == "" { + return "(unset)" + } + return raw +} diff --git a/services/search/pkg/opensearch/index_test.go b/services/search/pkg/opensearch/index_test.go index 5837189e82..06c4f3a972 100644 --- a/services/search/pkg/opensearch/index_test.go +++ b/services/search/pkg/opensearch/index_test.go @@ -5,9 +5,13 @@ import ( "strings" "testing" + opensearchgo "github.com/opensearch-project/opensearch-go/v4" + opensearchgoAPI "github.com/opensearch-project/opensearch-go/v4/opensearchapi" "github.com/stretchr/testify/require" + "github.com/tidwall/gjson" "github.com/tidwall/sjson" + "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/search/internal/opensearchtest" "github.com/opencloud-eu/opencloud/services/search/pkg/opensearch" "github.com/opencloud-eu/opencloud/services/search/pkg/search" @@ -46,7 +50,7 @@ func TestIndexManager(t *testing.T) { require.NotEmpty(t, body) require.NotEmpty(t, test.Got.String()) require.JSONEq(t, test.Got.String(), string(body)) - require.NoError(t, test.Got.Apply(t.Context(), indexName, tc.Client())) + require.NoError(t, test.Got.Apply(t.Context(), indexName, tc.Client(), log.NopLogger())) }) } }) @@ -59,7 +63,7 @@ func TestIndexManager(t *testing.T) { tc.Require.IndicesReset([]string{indexName}) tc.Require.IndicesCreate(indexName, strings.NewReader(indexManager.String())) - require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client())) + require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger())) }) t.Run("accepts an index that carries more than the definition declares", func(t *testing.T) { @@ -101,6 +105,94 @@ func TestIndexManager(t *testing.T) { require.NoError(t, err) tc.Require.IndicesCreate(indexName, strings.NewReader(body)) - require.ErrorIs(t, indexManager.Apply(t.Context(), indexName, tc.Client()), opensearch.ErrManualActionRequired) + require.ErrorIs(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger()), opensearch.ErrManualActionRequired) + }) + + t.Run("is idempotent", func(t *testing.T) { + indexManager := opensearch.IndexManagerLatest + indexName := "opencloud-test-resource" + + tc := opensearchtest.NewDefaultTestClient(t, defaultConfig.Engine.OpenSearch.Client) + tc.Require.IndicesReset([]string{indexName}) + + require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger())) + require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger())) + }) + + t.Run("adds a new field to an existing index in place", func(t *testing.T) { + indexManager := opensearch.IndexManagerLatest + indexName := "opencloud-test-resource" + + tc := opensearchtest.NewDefaultTestClient(t, defaultConfig.Engine.OpenSearch.Client) + tc.Require.IndicesReset([]string{indexName}) + + body, err := sjson.Delete(indexManager.String(), "mappings.properties.Title") + require.NoError(t, err) + tc.Require.IndicesCreate(indexName, strings.NewReader(body)) + + require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger())) + + resp, err := tc.Client().Indices.Mapping.Get(t.Context(), &opensearchgoAPI.MappingGetReq{Indices: []string{indexName}}) + require.NoError(t, err) + require.True(t, gjson.GetBytes(resp.Indices[indexName].Mappings, "properties.Title").Exists()) + }) + + t.Run("adds a new nested field to an existing index in place", func(t *testing.T) { + indexManager := opensearch.IndexManagerLatest + indexName := "opencloud-test-resource" + + tc := opensearchtest.NewDefaultTestClient(t, defaultConfig.Engine.OpenSearch.Client) + tc.Require.IndicesReset([]string{indexName}) + + body, err := sjson.Delete(indexManager.String(), "mappings.properties.photo.properties.cameraMake") + require.NoError(t, err) + tc.Require.IndicesCreate(indexName, strings.NewReader(body)) + + require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger())) + + resp, err := tc.Client().Indices.Mapping.Get(t.Context(), &opensearchgoAPI.MappingGetReq{Indices: []string{indexName}}) + require.NoError(t, err) + require.True(t, gjson.GetBytes(resp.Indices[indexName].Mappings, "properties.photo.properties.cameraMake").Exists()) + }) + + t.Run("fails when an existing field changed its definition", func(t *testing.T) { + indexManager := opensearch.IndexManagerLatest + indexName := "opencloud-test-resource" + + tc := opensearchtest.NewDefaultTestClient(t, defaultConfig.Engine.OpenSearch.Client) + tc.Require.IndicesReset([]string{indexName}) + + body, err := sjson.Set(indexManager.String(), "mappings.properties.Deleted.type", "keyword") + require.NoError(t, err) + tc.Require.IndicesCreate(indexName, strings.NewReader(body)) + + require.ErrorIs(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger()), opensearch.ErrManualActionRequired) + }) + + t.Run("fails when the index contains a field the code schema does not know", func(t *testing.T) { + indexManager := opensearch.IndexManagerLatest + indexName := "opencloud-test-resource" + + tc := opensearchtest.NewDefaultTestClient(t, defaultConfig.Engine.OpenSearch.Client) + tc.Require.IndicesReset([]string{indexName}) + + body, err := sjson.Set(indexManager.String(), "mappings.properties.legacyField.type", "keyword") + require.NoError(t, err) + tc.Require.IndicesCreate(indexName, strings.NewReader(body)) + + require.ErrorIs(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger()), opensearch.ErrManualActionRequired) + }) + + t.Run("transport errors do not demand manual action", func(t *testing.T) { + client, err := opensearchgoAPI.NewClient(opensearchgoAPI.Config{ + Client: opensearchgo.Config{ + Addresses: []string{"http://localhost:1025"}, + }, + }) + require.NoError(t, err) + + err = opensearch.IndexManagerLatest.Apply(t.Context(), "opencloud-test-resource", client, log.NopLogger()) + require.Error(t, err) + require.NotErrorIs(t, err, opensearch.ErrManualActionRequired) }) } From 1c130c448c15ba2496122e4a39dcb49177c6bbbb Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 8 Jul 2026 21:31:11 +0200 Subject: [PATCH 02/25] chore(search): drop the changelog entry --- .../change-search-index-schema-handling.md | 22 ------------------- 1 file changed, 22 deletions(-) delete mode 100644 changelog/unreleased/change-search-index-schema-handling.md diff --git a/changelog/unreleased/change-search-index-schema-handling.md b/changelog/unreleased/change-search-index-schema-handling.md deleted file mode 100644 index 1177da09ca..0000000000 --- a/changelog/unreleased/change-search-index-schema-handling.md +++ /dev/null @@ -1,22 +0,0 @@ -Change: Check the search index schema on startup (existing indexes need a rebuild) - -The search service now compares the schema of an existing search index with -the schema expected by the code at startup, for both the bleve and the -OpenSearch engine. A purely additive change (new fields that have never been -indexed) is applied in place and the service starts; documents indexed before -the upgrade do not contain the new fields until they are re-indexed. Any other -difference (changed field definitions, changed analyzers, removed or renamed -fields, or new fields that already contain data of unknown form) makes the -service refuse to start instead of silently returning wrong or incomplete -search results. - -Upgrading to this version is a breaking change for BOTH engines: every search -index built by a previous version differs from the new schema and the service -will refuse to start. To rebuild: stop the service, delete the search index -(the bleve directory or the OpenSearch index), start the service (an empty -index with the new schema is created) and run -"opencloud search index --all-spaces" to re-index all files. To bring an -instance up without search until a maintenance window, set -OC_EXCLUDE_RUN_SERVICES=search. - -https://github.com/opencloud-eu/opencloud/issues/3092 From 09ea7e5f15667bf6e2630b4b3594a68a13996c83 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 8 Jul 2026 21:34:59 +0200 Subject: [PATCH 03/25] chore(search): mention the impact of disabling search in the refuse message --- services/search/pkg/mapping/classify.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/services/search/pkg/mapping/classify.go b/services/search/pkg/mapping/classify.go index 9fa357ef7c..f87d30282e 100644 --- a/services/search/pkg/mapping/classify.go +++ b/services/search/pkg/mapping/classify.go @@ -14,7 +14,8 @@ import ( var ErrManualActionRequired = errors.New("manual action required") // ManualActionRequiredError builds the operator-facing error for a breaking -// schema change. index is the index name (OpenSearch) or path (bleve). +// schema change, shared by both engines. index is the index name (OpenSearch) +// or path (bleve). func ManualActionRequiredError(index string, reasons []string) error { return fmt.Errorf( "%w: search index %s was built with a different schema (%s). "+ @@ -22,7 +23,9 @@ func ManualActionRequiredError(index string, reasons []string) error { "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", + "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, ) } From b67811ab3c2aa1d398e078db463c25a624fda352 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 8 Jul 2026 21:44:09 +0200 Subject: [PATCH 04/25] chore(search): warn on additive opensearch changes and name the exact 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 / vs removing the bleve directory). --- services/search/pkg/bleve/index.go | 2 +- services/search/pkg/mapping/classify.go | 10 ++++++---- services/search/pkg/opensearch/index.go | 6 +++--- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index 1c226e5ea4..8a3dc4a384 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -63,7 +63,7 @@ func NewIndex(root string) (bleve.Index, searchmapping.Classification, error) { switch classification.Verdict { case searchmapping.VerdictBreaking: _ = index.Close() - return nil, classification, searchmapping.ManualActionRequiredError(destination, classification.Reasons) + return nil, classification, searchmapping.ManualActionRequiredError(destination, "delete the index directory "+destination, classification.Reasons) case searchmapping.VerdictAdditive: // The classifier guarantees everything else is identical and the new // fields hold no data yet, so storing the code mapping only adds diff --git a/services/search/pkg/mapping/classify.go b/services/search/pkg/mapping/classify.go index f87d30282e..dfaac5ebdc 100644 --- a/services/search/pkg/mapping/classify.go +++ b/services/search/pkg/mapping/classify.go @@ -15,18 +15,20 @@ var ErrManualActionRequired = errors.New("manual action required") // ManualActionRequiredError builds the operator-facing error for a breaking // schema change, shared by both engines. index is the index name (OpenSearch) -// or path (bleve). -func ManualActionRequiredError(index string, reasons []string) error { +// or path (bleve); deleteStep is the engine-specific instruction to remove the +// index, e.g. "delete the index (DELETE /name)" or "delete the index +// directory /path". +func ManualActionRequiredError(index, deleteStep string, reasons []string) error { 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, "+ + "There is no in-place migration: stop the service, %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, + ErrManualActionRequired, index, strings.Join(reasons, "; "), deleteStep, ) } diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index 81b291da9b..c50f809876 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -176,7 +176,7 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch ) reasons = append(reasons, classification.Reasons...) if len(reasons) > 0 { - return searchmapping.ManualActionRequiredError(name, reasons) + return searchmapping.ManualActionRequiredError(name, fmt.Sprintf("delete the index (DELETE /%s)", name), reasons) } if len(classification.NewFields) == 0 { return nil // schema is up to date @@ -193,14 +193,14 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch case err != nil && errors.As(err, &putErr) && putErr.Err.Type == "illegal_argument_exception" && (strings.Contains(putErr.Err.Reason, "cannot be changed") || strings.Contains(putErr.Err.Reason, "Cannot update parameter")): // backstop, should be unreachable after the classification above - return searchmapping.ManualActionRequiredError(name, []string{putErr.Err.Reason}) + return searchmapping.ManualActionRequiredError(name, fmt.Sprintf("delete the index (DELETE /%s)", name), []string{putErr.Err.Reason}) case err != nil: return fmt.Errorf("failed to update mapping of index %s: %w", name, err) case !putResp.Acknowledged: 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") + logger.Warn().Strs("fields", classification.NewFields).Str("index", name).Msg("extended the search index mapping with new fields; documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces") return nil } From 451e893d7d968ee2834e907d7ce64bf3063448a4 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 8 Jul 2026 21:49:43 +0200 Subject: [PATCH 05/25] chore(search): tighten doc comments --- services/search/pkg/bleve/index.go | 30 ++++++++------------ services/search/pkg/command/server.go | 3 +- services/search/pkg/mapping/classify.go | 18 ++++-------- services/search/pkg/mapping/classify_test.go | 3 +- services/search/pkg/opensearch/index.go | 9 +++--- 5 files changed, 24 insertions(+), 39 deletions(-) diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index 8a3dc4a384..9478bc4573 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -29,12 +29,10 @@ import ( var openRuntimeConfig = map[string]interface{}{"bolt_timeout": "5s"} // NewIndex opens (or creates) the bleve index at root and classifies the -// stored schema against the one generated from code. On a breaking change it -// refuses with ErrManualActionRequired. On an additive one the code schema is -// persisted into the index (the bleve analogue of an OpenSearch PUT _mapping), -// so the new fields are properly typed from now on and later startups classify -// equal; the caller must still warn that documents indexed before the upgrade -// lack the Classification.NewFields until they are re-indexed. +// stored schema against NewMapping(). Breaking changes refuse with +// ErrManualActionRequired, additive ones are persisted into the index (the +// bleve analogue of PUT _mapping); the caller must warn that pre-upgrade +// documents lack the Classification.NewFields until re-indexed. func NewIndex(root string) (bleve.Index, searchmapping.Classification, error) { destination := filepath.Join(root, fmt.Sprintf("bleve-v%d", search.SchemaVersion)) index, err := bleve.OpenUsing(destination, openRuntimeConfig) @@ -65,11 +63,9 @@ func NewIndex(root string) (bleve.Index, searchmapping.Classification, error) { _ = index.Close() return nil, classification, searchmapping.ManualActionRequiredError(destination, "delete the index directory "+destination, classification.Reasons) case searchmapping.VerdictAdditive: - // The classifier guarantees everything else is identical and the new - // fields hold no data yet, so storing the code mapping only adds - // fields. Reopen so the live mapping picks it up: without this the - // new fields would be indexed dynamically and the data-aware rule - // would turn them breaking on the next startup. + // Safe: everything else is identical and the new fields hold no data. + // Reopen so the live mapping picks the change up; otherwise the fields + // get indexed dynamically and flip to breaking on the next start. if err := index.SetInternal([]byte("_mapping"), codeB); err != nil { _ = index.Close() return nil, searchmapping.Classification{}, fmt.Errorf("failed to store the updated index mapping: %w", err) @@ -86,13 +82,11 @@ func NewIndex(root string) (bleve.Index, searchmapping.Classification, error) { return index, classification, nil } -// classifyStoredMapping diffs the mapping stored in the index against -// NewMapping() and also returns the marshaled code mapping. Fields that are -// new in the code schema but already have data in the index (previously -// indexed dynamically) are breaking, their de-facto form is unknown. The JSON -// compare is stable within one bleve version; if a bleve upgrade changes -// marshaling defaults it fails towards breaking, normalize the affected key -// here if that ever fires. +// classifyStoredMapping diffs the stored mapping against NewMapping() and +// returns the marshaled code mapping. New-in-code fields that already hold +// data (previously indexed dynamically) are breaking. The compare is only +// stable within one bleve version: a changed marshaling default fails towards +// breaking, normalize the affected key here if that ever fires. func classifyStoredMapping(index bleve.Index) (searchmapping.Classification, []byte, error) { storedB, err := index.GetInternal([]byte("_mapping")) if err != nil { diff --git a/services/search/pkg/command/server.go b/services/search/pkg/command/server.go index e494c177fe..7be68c2346 100644 --- a/services/search/pkg/command/server.go +++ b/services/search/pkg/command/server.go @@ -127,8 +127,7 @@ func Server(cfg *config.Config) *cobra.Command { return fmt.Errorf("failed to create OpenSearch client: %w", err) } - // bound the startup schema check so a hung cluster fails the - // start instead of blocking forever + // a hung cluster must fail the start, not block it forever startupCtx, cancelStartup := context.WithTimeout(ctx, time.Minute) openSearchBackend, err := opensearch.NewBackend(startupCtx, cfg.Engine.OpenSearch.ResourceIndex.Name, client, logger) cancelStartup() diff --git a/services/search/pkg/mapping/classify.go b/services/search/pkg/mapping/classify.go index dfaac5ebdc..7e824a06e9 100644 --- a/services/search/pkg/mapping/classify.go +++ b/services/search/pkg/mapping/classify.go @@ -14,10 +14,8 @@ import ( var ErrManualActionRequired = errors.New("manual action required") // ManualActionRequiredError builds the operator-facing error for a breaking -// schema change, shared by both engines. index is the index name (OpenSearch) -// or path (bleve); deleteStep is the engine-specific instruction to remove the -// index, e.g. "delete the index (DELETE /name)" or "delete the index -// directory /path". +// schema change. index names the index, deleteStep is the engine-specific +// instruction to remove it. func ManualActionRequiredError(index, deleteStep string, reasons []string) error { return fmt.Errorf( "%w: search index %s was built with a different schema (%s). "+ @@ -51,14 +49,10 @@ type Classification struct { } // Classify recursively compares a stored `properties` tree against the one -// generated from code. Both sides must be generic JSON-decoded values -// (map[string]any, []any, float64), not marshaled Go structs, so values -// compare structurally. -// -// dataFields reports whether the index holds data at or below a dotted field -// path even though it is absent from the stored schema (bleve indexes dynamic -// fields without a schema trace). Engines that record dynamic fields in the -// live schema (OpenSearch) pass nil. +// generated from code. Both sides must be generic JSON-decoded values, not +// marshaled Go structs. dataFields reports whether the index holds data at or +// below a dotted field path absent from the stored schema (bleve dynamic +// fields); engines without that blind spot pass nil. func Classify(stored, code map[string]any, dataFields func(path string) bool) Classification { c := Classification{Verdict: VerdictEqual} classifyProperties(stored, code, dataFields, "", &c) diff --git a/services/search/pkg/mapping/classify_test.go b/services/search/pkg/mapping/classify_test.go index 37ea5fbd1b..c4b4d35817 100644 --- a/services/search/pkg/mapping/classify_test.go +++ b/services/search/pkg/mapping/classify_test.go @@ -82,8 +82,7 @@ var _ = Describe("Classify", func() { stored := parse(code) delete(stored, "photo") - // the callback is consulted with the subtree root; reporting data at - // or below it is the caller's job + // the callback is consulted with the subtree root c := Classify(stored, parse(code), hasData("photo")) Expect(c.Verdict).To(Equal(VerdictBreaking)) Expect(c.Reasons).To(ConsistOf(ContainSubstring("photo"))) diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index c50f809876..c6f8c07e00 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -114,11 +114,10 @@ func buildResourceMapping() ([]byte, error) { return json.Marshal(index) } -// Apply ensures the index exists and matches the schema generated from code. -// A missing index is created, an additive schema change is applied in place -// via PUT _mapping, a breaking one returns ErrManualActionRequired. The -// classifier decides what is additive; PUT _mapping is only the mechanism to -// apply it (its merge semantics cannot detect removals or renames). +// Apply ensures the index exists and matches the schema generated from code: +// created if missing, additive changes applied via PUT _mapping, breaking ones +// refused with ErrManualActionRequired. The classifier judges, PUT _mapping +// only applies (its merge semantics hide removals and renames). func (m IndexManager) Apply(ctx context.Context, name string, client *opensearchgoAPI.Client, logger log.Logger) error { localIndexB, err := m.MarshalJSON() if err != nil { From 33299afbaa880b9a32dcf6e38f02037f73397875 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Thu, 9 Jul 2026 01:27:26 +0200 Subject: [PATCH 06/25] fix(search): address max-review findings - 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 --- services/search/pkg/bleve/index.go | 9 +- services/search/pkg/bleve/index_test.go | 19 + .../pkg/bleve/testdata/mapping.golden.json | 720 ++++++++++++++++++ services/search/pkg/command/server.go | 11 +- services/search/pkg/opensearch/index.go | 41 +- services/search/pkg/opensearch/index_test.go | 14 + 6 files changed, 793 insertions(+), 21 deletions(-) create mode 100644 services/search/pkg/bleve/testdata/mapping.golden.json diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index 9478bc4573..526c43ac1c 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -66,16 +66,19 @@ func NewIndex(root string) (bleve.Index, searchmapping.Classification, error) { // Safe: everything else is identical and the new fields hold no data. // Reopen so the live mapping picks the change up; otherwise the fields // get indexed dynamically and flip to breaking on the next start. + // return the classification even on errors: the mapping may already be + // persisted, so the next start classifies equal and the caller's + // warning is the only chance to surface the new fields if err := index.SetInternal([]byte("_mapping"), codeB); err != nil { _ = index.Close() - return nil, searchmapping.Classification{}, fmt.Errorf("failed to store the updated index mapping: %w", err) + return nil, classification, fmt.Errorf("failed to store the updated index mapping: %w", err) } if err := index.Close(); err != nil { - return nil, searchmapping.Classification{}, err + return nil, classification, err } index, err = bleve.OpenUsing(destination, openRuntimeConfig) if err != nil { - return nil, searchmapping.Classification{}, err + return nil, classification, err } } diff --git a/services/search/pkg/bleve/index_test.go b/services/search/pkg/bleve/index_test.go index 6629b416af..c69c4ce896 100644 --- a/services/search/pkg/bleve/index_test.go +++ b/services/search/pkg/bleve/index_test.go @@ -1,7 +1,9 @@ package bleve_test import ( + "encoding/json" "fmt" + "os" "path/filepath" bleveSearch "github.com/blevesearch/bleve/v2" @@ -208,4 +210,21 @@ var _ = Describe("NewMapping", func() { Expect(ok).To(BeTrue()) Expect(impl.Validate()).To(Succeed()) }) + + // A diff here means existing indexes will classify as breaking (schema or + // bleve marshaling changed); update the golden file only deliberately. + It("matches the committed golden mapping", func() { + m, err := bleve.NewMapping() + Expect(err).ToNot(HaveOccurred()) + b, err := json.Marshal(m) + Expect(err).ToNot(HaveOccurred()) + var got, golden map[string]any + Expect(json.Unmarshal(b, &got)).To(Succeed()) + + goldenB, err := os.ReadFile("testdata/mapping.golden.json") + Expect(err).ToNot(HaveOccurred()) + Expect(json.Unmarshal(goldenB, &golden)).To(Succeed()) + + Expect(got).To(Equal(golden)) + }) }) diff --git a/services/search/pkg/bleve/testdata/mapping.golden.json b/services/search/pkg/bleve/testdata/mapping.golden.json new file mode 100644 index 0000000000..8973f666e5 --- /dev/null +++ b/services/search/pkg/bleve/testdata/mapping.golden.json @@ -0,0 +1,720 @@ +{ + "default_mapping": { + "enabled": true, + "dynamic": true, + "properties": { + "Content": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "fulltext", + "store": true, + "index": true, + "include_term_vectors": true, + "docvalues": true + } + ] + }, + "Deleted": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "boolean", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Favorites": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "docvalues": true + } + ] + }, + "Favorites_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "Hidden": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "boolean", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "ID": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "MimeType": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Mtime": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "datetime", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Name": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Name_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "ParentID": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Path": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "docvalues": true + } + ] + }, + "RootID": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Size": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Tags": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "docvalues": true + } + ] + }, + "Tags_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "Title": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Type": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "audio": { + "enabled": true, + "dynamic": true, + "properties": { + "album": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "albumArtist": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "artist": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "bitrate": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "composers": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "copyright": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "disc": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "discCount": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "duration": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "genre": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "hasDrm": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "boolean", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "isVariableBitrate": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "boolean", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "title": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "track": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "trackCount": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "year": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + } + } + }, + "image": { + "enabled": true, + "dynamic": true, + "properties": { + "height": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "width": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + } + } + }, + "location": { + "enabled": true, + "dynamic": true, + "properties": { + "altitude": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "latitude": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "longitude": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + } + } + }, + "location_geopoint": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "geopoint", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "photo": { + "enabled": true, + "dynamic": true, + "properties": { + "cameraMake": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "cameraModel": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "exposureDenominator": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "exposureNumerator": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "fNumber": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "focalLength": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "iso": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "orientation": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "takenDateTime": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "datetime", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + } + } + } + } + }, + "type_field": "_type", + "default_type": "_default", + "default_analyzer": "keyword", + "default_datetime_parser": "dateTimeOptional", + "default_field": "_all", + "store_dynamic": true, + "index_dynamic": true, + "docvalues_dynamic": true, + "analysis": { + "analyzers": { + "fulltext": { + "token_filters": [ + "to_lower", + "stemmer_porter" + ], + "tokenizer": "unicode", + "type": "custom" + } + } + } +} diff --git a/services/search/pkg/command/server.go b/services/search/pkg/command/server.go index 7be68c2346..9413642d7f 100644 --- a/services/search/pkg/command/server.go +++ b/services/search/pkg/command/server.go @@ -74,14 +74,15 @@ func Server(cfg *config.Config) *cobra.Command { switch cfg.Engine.Type { case "bleve": idx, classification, err := bleve.NewIndex(cfg.Engine.Bleve.Datapath) - if err != nil { - return err - } - + // warn before the error check: the new mapping may already be + // persisted, then later startups classify equal and stay silent if classification.Verdict == searchmapping.VerdictAdditive { logger.Warn(). Strs("fields", classification.NewFields). - Msgf("the bleve index at %s was built with an older schema; the new fields were added to the index schema, but documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces", cfg.Engine.Bleve.Datapath) + Msgf("the bleve index at %s was built with an older schema; the new fields were added to the index schema, but documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces --force-rescan", cfg.Engine.Bleve.Datapath) + } + if err != nil { + return err } defer func() { diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index c6f8c07e00..2f5b651f86 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -124,22 +124,34 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch return fmt.Errorf("failed to marshal index %s: %w", name, err) } - createResp, err := client.Indices.Create(ctx, opensearchgoAPI.IndicesCreateReq{ - Index: name, - Body: bytes.NewReader(localIndexB), + // Exists first: a pre-provisioned index must not require create privileges + indicesExistsResp, err := client.Indices.Exists(ctx, opensearchgoAPI.IndicesExistsReq{ + Indices: []string{name}, }) - var createErr *opensearchgo.StructError switch { - case err == nil && createResp.Acknowledged: - return nil - case err == nil: - return fmt.Errorf("failed to create index %s: not acknowledged", name) - case !errors.As(err, &createErr) || createErr.Err.Type != "resource_already_exists_exception": - // transport errors, disk-full etc. stay plain fatal, the restart policy retries - return fmt.Errorf("failed to create index %s: %w", name, err) + case indicesExistsResp != nil && indicesExistsResp.StatusCode == 404: + createResp, createErr := client.Indices.Create(ctx, opensearchgoAPI.IndicesCreateReq{ + Index: name, + Body: bytes.NewReader(localIndexB), + }) + var structErr *opensearchgo.StructError + switch { + case createErr == nil && createResp.Acknowledged: + return nil + case createErr == nil: + return fmt.Errorf("failed to create index %s: not acknowledged", name) + case !errors.As(createErr, &structErr) || structErr.Err.Type != "resource_already_exists_exception": + // transport errors, disk-full etc. stay plain fatal, the restart policy retries + return fmt.Errorf("failed to create index %s: %w", name, createErr) + } + // lost the creation race to another instance, compare against its index + case err != nil: + return fmt.Errorf("failed to check if index %s exists: %w", name, err) + case indicesExistsResp == nil: + return fmt.Errorf("indicesExistsResp is nil for index %s", name) } - // the index already exists: compare settings and classify the mapping diff + // the index exists: compare settings and classify the mapping diff resp, err := client.Indices.Get(ctx, opensearchgoAPI.IndicesGetReq{ Indices: []string{name}, }) @@ -161,6 +173,9 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch var reasons []string for k := range localIndexJson.Get("settings").Map() { + if k == "number_of_replicas" { + continue // runtime-tunable via PUT _settings, drift needs no rebuild + } lv := localIndexJson.Get("settings." + k).Raw rv := remoteIndexJson.Get("settings.index." + k).Raw if !jsonEqual(lv, rv) { @@ -199,7 +214,7 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch return fmt.Errorf("failed to update mapping of index %s: not acknowledged", name) } - logger.Warn().Strs("fields", classification.NewFields).Str("index", name).Msg("extended the search index mapping with new fields; documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces") + logger.Warn().Strs("fields", classification.NewFields).Str("index", name).Msg("extended the search index mapping with new fields; documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces --force-rescan") return nil } diff --git a/services/search/pkg/opensearch/index_test.go b/services/search/pkg/opensearch/index_test.go index 06c4f3a972..61c0b98ec3 100644 --- a/services/search/pkg/opensearch/index_test.go +++ b/services/search/pkg/opensearch/index_test.go @@ -108,6 +108,20 @@ func TestIndexManager(t *testing.T) { require.ErrorIs(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger()), opensearch.ErrManualActionRequired) }) + t.Run("tolerates replica drift", func(t *testing.T) { + indexManager := opensearch.IndexManagerLatest + indexName := "opencloud-test-resource" + + tc := opensearchtest.NewDefaultTestClient(t, defaultConfig.Engine.OpenSearch.Client) + tc.Require.IndicesReset([]string{indexName}) + + body, err := sjson.Set(indexManager.String(), "settings.number_of_replicas", "2") + require.NoError(t, err) + tc.Require.IndicesCreate(indexName, strings.NewReader(body)) + + require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger())) + }) + t.Run("is idempotent", func(t *testing.T) { indexManager := opensearch.IndexManagerLatest indexName := "opencloud-test-resource" From e9347998b71ffcb9d86eb72f66733b81b5eee9dc Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 15 Jul 2026 17:37:08 +0200 Subject: [PATCH 07/25] fix(search): name the service to stop in the schema mismatch error --- services/search/pkg/mapping/classify.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/search/pkg/mapping/classify.go b/services/search/pkg/mapping/classify.go index 7e824a06e9..594e6d7650 100644 --- a/services/search/pkg/mapping/classify.go +++ b/services/search/pkg/mapping/classify.go @@ -19,8 +19,8 @@ var ErrManualActionRequired = errors.New("manual action required") func ManualActionRequiredError(index, deleteStep string, reasons []string) error { return fmt.Errorf( "%w: search index %s was built with a different schema (%s). "+ - "There is no in-place migration: stop the service, %s, "+ - "start the service (an empty index with the new schema is created), "+ + "There is no in-place migration: with the OpenCloud search service stopped, %s, "+ + "then start it again (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 "+ From 5abe2ccb26cdb58269e7a00eba0e83f0ac16a537 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 15 Jul 2026 18:22:36 +0200 Subject: [PATCH 08/25] fix(search): list the schema mismatch reasons on separate lines --- services/search/pkg/mapping/classify.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/search/pkg/mapping/classify.go b/services/search/pkg/mapping/classify.go index 594e6d7650..696980e0fc 100644 --- a/services/search/pkg/mapping/classify.go +++ b/services/search/pkg/mapping/classify.go @@ -18,7 +18,7 @@ var ErrManualActionRequired = errors.New("manual action required") // instruction to remove it. func ManualActionRequiredError(index, deleteStep string, reasons []string) error { return fmt.Errorf( - "%w: search index %s was built with a different schema (%s). "+ + "%w: search index %s was built with a different schema:\n - %s\n"+ "There is no in-place migration: with the OpenCloud search service stopped, %s, "+ "then start it again (an empty index with the new schema is created), "+ "then rebuild the content by running: opencloud search index --all-spaces. "+ @@ -26,7 +26,7 @@ func ManualActionRequiredError(index, deleteStep string, reasons []string) error "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, "; "), deleteStep, + ErrManualActionRequired, index, strings.Join(reasons, "\n - "), deleteStep, ) } @@ -79,7 +79,7 @@ func classifyProperties(stored, code map[string]any, dataFields func(string) boo } path := joinPath(prefix, k) if dataFields != nil && dataFields(path) { - c.breaking(fmt.Sprintf("field %s is new in the code schema but the index already contains data for it (previously indexed dynamically)", path)) + c.breaking(fmt.Sprintf("field %s is explicitly mapped now, but the index already holds data that was indexed dynamically for it, of an unknown type", path)) continue } c.NewFields = append(c.NewFields, leafPaths(code[k], path)...) From a6bfb61935b4bdd5608db90d474c90539ee2d966 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 29 Jul 2026 19:55:52 +0200 Subject: [PATCH 09/25] refactor(search): make the breaking-schema error developer-facing --- services/search/pkg/bleve/index.go | 2 +- services/search/pkg/mapping/classify.go | 22 +++++++++------------- services/search/pkg/opensearch/index.go | 4 ++-- 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index 526c43ac1c..d4c5d57de2 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -61,7 +61,7 @@ func NewIndex(root string) (bleve.Index, searchmapping.Classification, error) { switch classification.Verdict { case searchmapping.VerdictBreaking: _ = index.Close() - return nil, classification, searchmapping.ManualActionRequiredError(destination, "delete the index directory "+destination, classification.Reasons) + return nil, classification, searchmapping.ManualActionRequiredError(destination, classification.Reasons) case searchmapping.VerdictAdditive: // Safe: everything else is identical and the new fields hold no data. // Reopen so the live mapping picks the change up; otherwise the fields diff --git a/services/search/pkg/mapping/classify.go b/services/search/pkg/mapping/classify.go index 696980e0fc..2e0e7e100a 100644 --- a/services/search/pkg/mapping/classify.go +++ b/services/search/pkg/mapping/classify.go @@ -13,20 +13,16 @@ import ( // ErrManualActionRequired marks schema changes that cannot be applied in place. var ErrManualActionRequired = errors.New("manual action required") -// ManualActionRequiredError builds the operator-facing error for a breaking -// schema change. index names the index, deleteStep is the engine-specific -// instruction to remove it. -func ManualActionRequiredError(index, deleteStep string, reasons []string) error { +// ManualActionRequiredError reports a breaking schema change. Because the index +// is versioned by search.SchemaVersion, a released instance never hits this: a +// version bump builds a fresh index. It fires in development when the mapping is +// changed in a breaking way without bumping search.SchemaVersion, so the fix is +// to bump it (or revert the change). +func ManualActionRequiredError(index string, reasons []string) error { return fmt.Errorf( - "%w: search index %s was built with a different schema:\n - %s\n"+ - "There is no in-place migration: with the OpenCloud search service stopped, %s, "+ - "then start it again (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, "\n - "), deleteStep, + "%w: the search mapping in code differs from index %s in a breaking way:\n - %s\n"+ + "bump search.SchemaVersion to build a fresh index, or revert the mapping change", + ErrManualActionRequired, index, strings.Join(reasons, "\n - "), ) } diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index 2f5b651f86..f9e3a015bf 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -190,7 +190,7 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch ) reasons = append(reasons, classification.Reasons...) if len(reasons) > 0 { - return searchmapping.ManualActionRequiredError(name, fmt.Sprintf("delete the index (DELETE /%s)", name), reasons) + return searchmapping.ManualActionRequiredError(name, reasons) } if len(classification.NewFields) == 0 { return nil // schema is up to date @@ -207,7 +207,7 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch case err != nil && errors.As(err, &putErr) && putErr.Err.Type == "illegal_argument_exception" && (strings.Contains(putErr.Err.Reason, "cannot be changed") || strings.Contains(putErr.Err.Reason, "Cannot update parameter")): // backstop, should be unreachable after the classification above - return searchmapping.ManualActionRequiredError(name, fmt.Sprintf("delete the index (DELETE /%s)", name), []string{putErr.Err.Reason}) + return searchmapping.ManualActionRequiredError(name, []string{putErr.Err.Reason}) case err != nil: return fmt.Errorf("failed to update mapping of index %s: %w", name, err) case !putResp.Acknowledged: From 37eed7c321117081674ed35bfc6341ef8dc3aadc Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 29 Jul 2026 20:11:25 +0200 Subject: [PATCH 10/25] refactor(search): only enforce analysis settings, tolerate operational drift --- services/search/pkg/bleve/index_test.go | 6 +-- services/search/pkg/opensearch/index.go | 17 ++++---- services/search/pkg/opensearch/index_test.go | 46 +++++++------------- 3 files changed, 27 insertions(+), 42 deletions(-) diff --git a/services/search/pkg/bleve/index_test.go b/services/search/pkg/bleve/index_test.go index c69c4ce896..40c28c36a0 100644 --- a/services/search/pkg/bleve/index_test.go +++ b/services/search/pkg/bleve/index_test.go @@ -24,7 +24,7 @@ var _ = Describe("Index", func() { It("puts the index into a directory of its own generation", func() { root := GinkgoT().TempDir() - index, err := bleve.NewIndex(root) + index, _, err := bleve.NewIndex(root) Expect(err).ToNot(HaveOccurred()) DeferCleanup(index.Close) @@ -35,11 +35,11 @@ var _ = Describe("Index", func() { It("opens the index that is already there", func() { root := GinkgoT().TempDir() - index, err := bleve.NewIndex(root) + index, _, err := bleve.NewIndex(root) Expect(err).ToNot(HaveOccurred()) Expect(index.Close()).To(Succeed()) - reopened, err := bleve.NewIndex(root) + reopened, _, err := bleve.NewIndex(root) Expect(err).ToNot(HaveOccurred()) DeferCleanup(reopened.Close) diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index f9e3a015bf..cca2caf59a 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -171,16 +171,15 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch localIndexJson := gjson.ParseBytes(localIndexB) remoteIndexJson := gjson.ParseBytes(remoteIndexB) + // Only the analysis settings (analyzers/tokenizers/filters) affect how data + // is indexed and queried; a drift there yields wrong results. Shard/replica + // counts and other operational knobs are the operator's to tune (and a + // pre-provisioned index's to own), so they are not compared. var reasons []string - for k := range localIndexJson.Get("settings").Map() { - if k == "number_of_replicas" { - continue // runtime-tunable via PUT _settings, drift needs no rebuild - } - lv := localIndexJson.Get("settings." + k).Raw - rv := remoteIndexJson.Get("settings.index." + k).Raw - if !jsonEqual(lv, rv) { - reasons = append(reasons, fmt.Sprintf("settings.%s changed: index %s, code %s", k, rawOrUnset(rv), rawOrUnset(lv))) - } + lv := localIndexJson.Get("settings.analysis").Raw + rv := remoteIndexJson.Get("settings.index.analysis").Raw + if !jsonEqual(lv, rv) { + reasons = append(reasons, fmt.Sprintf("settings.analysis changed: index %s, code %s", rawOrUnset(rv), rawOrUnset(lv))) } classification := searchmapping.Classify( diff --git a/services/search/pkg/opensearch/index_test.go b/services/search/pkg/opensearch/index_test.go index 61c0b98ec3..48885f1a3e 100644 --- a/services/search/pkg/opensearch/index_test.go +++ b/services/search/pkg/opensearch/index_test.go @@ -66,42 +66,14 @@ func TestIndexManager(t *testing.T) { require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger())) }) - t.Run("accepts an index that carries more than the definition declares", func(t *testing.T) { + t.Run("fails when the analysis settings drift", func(t *testing.T) { indexManager := opensearch.IndexManagerLatest indexName := "opencloud-test-resource" tc := opensearchtest.NewDefaultTestClient(t, defaultConfig.Engine.OpenSearch.Client) tc.Require.IndicesReset([]string{indexName}) - body, err := sjson.Set(indexManager.String(), "mappings.properties.Path.fields.raw.type", "keyword") - require.NoError(t, err) - tc.Require.IndicesCreate(indexName, strings.NewReader(body)) - - require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client())) - }) - - t.Run("fails when the index misses something the definition declares", func(t *testing.T) { - indexManager := opensearch.IndexManagerLatest - indexName := "opencloud-test-resource" - - tc := opensearchtest.NewDefaultTestClient(t, defaultConfig.Engine.OpenSearch.Client) - tc.Require.IndicesReset([]string{indexName}) - - body, err := sjson.Delete(indexManager.String(), "mappings.properties.Path.analyzer") - require.NoError(t, err) - tc.Require.IndicesCreate(indexName, strings.NewReader(body)) - - require.ErrorIs(t, indexManager.Apply(t.Context(), indexName, tc.Client()), opensearch.ErrManualActionRequired) - }) - - t.Run("fails to create index if it already exists but is not up to date", func(t *testing.T) { - indexManager := opensearch.IndexManagerLatest - indexName := "opencloud-test-resource" - - tc := opensearchtest.NewDefaultTestClient(t, defaultConfig.Engine.OpenSearch.Client) - tc.Require.IndicesReset([]string{indexName}) - - body, err := sjson.Set(indexManager.String(), "settings.number_of_shards", "2") + body, err := sjson.Set(indexManager.String(), "settings.analysis.analyzer.lowercaseKeyword.tokenizer", "standard") require.NoError(t, err) tc.Require.IndicesCreate(indexName, strings.NewReader(body)) @@ -122,6 +94,20 @@ func TestIndexManager(t *testing.T) { require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger())) }) + t.Run("tolerates shard drift", func(t *testing.T) { + indexManager := opensearch.IndexManagerLatest + indexName := "opencloud-test-resource" + + tc := opensearchtest.NewDefaultTestClient(t, defaultConfig.Engine.OpenSearch.Client) + tc.Require.IndicesReset([]string{indexName}) + + body, err := sjson.Set(indexManager.String(), "settings.number_of_shards", "2") + require.NoError(t, err) + tc.Require.IndicesCreate(indexName, strings.NewReader(body)) + + require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger())) + }) + t.Run("is idempotent", func(t *testing.T) { indexManager := opensearch.IndexManagerLatest indexName := "opencloud-test-resource" From 9755728d430d782c70f36871f006164e72f9f6b3 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 29 Jul 2026 20:23:30 +0200 Subject: [PATCH 11/25] refactor(search): harden the index-diff helpers against unset input --- services/search/pkg/opensearch/index.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index cca2caf59a..3b49393b5b 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -217,7 +217,14 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch return nil } +// jsonEqual reports whether two raw JSON values are deeply equal. gjson yields +// an empty string for a path that does not exist; two such unset values are +// equal, an unset value never equals a present one, and a value that fails to +// parse counts as unequal. func jsonEqual(a, b string) bool { + if a == "" || b == "" { + return a == b + } var av, bv any if err := json.Unmarshal([]byte(a), &av); err != nil { return false @@ -228,11 +235,14 @@ func jsonEqual(a, b string) bool { return reflect.DeepEqual(av, bv) } -// propertiesMap parses a raw mappings.properties object; missing or empty -// input yields an empty map, which classifies as purely additive. +// propertiesMap parses a raw mappings.properties object into a map. Missing, +// empty, null or malformed input yields an empty (non-nil) map, which +// classifies as purely additive. func propertiesMap(raw string) map[string]any { props := map[string]any{} - _ = json.Unmarshal([]byte(raw), &props) + if err := json.Unmarshal([]byte(raw), &props); err != nil || props == nil { + return map[string]any{} + } return props } From 04c37d12d2311a6750610d16a05c20338cc85000 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 29 Jul 2026 20:36:58 +0200 Subject: [PATCH 12/25] refactor(search): extract opensearch.NewClient out of server startup --- services/search/pkg/command/server.go | 39 +----------------- services/search/pkg/opensearch/client.go | 52 ++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 37 deletions(-) create mode 100644 services/search/pkg/opensearch/client.go diff --git a/services/search/pkg/command/server.go b/services/search/pkg/command/server.go index 9413642d7f..1d6c89b5db 100644 --- a/services/search/pkg/command/server.go +++ b/services/search/pkg/command/server.go @@ -2,10 +2,7 @@ package command import ( "context" - "crypto/tls" "fmt" - "net/http" - "os" "os/signal" "time" @@ -32,8 +29,6 @@ import ( "github.com/opencloud-eu/reva/v2/pkg/events/raw" "github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool" - opensearchgo "github.com/opensearch-project/opensearch-go/v4" - opensearchgoAPI "github.com/opensearch-project/opensearch-go/v4/opensearchapi" "github.com/spf13/cobra" ) @@ -93,39 +88,9 @@ func Server(cfg *config.Config) *cobra.Command { eng = bleve.NewBackend(idx, bleveQuery.DefaultCreator, logger) case "open-search": - clientConfig := opensearchgo.Config{ - Addresses: cfg.Engine.OpenSearch.Client.Addresses, - Username: cfg.Engine.OpenSearch.Client.Username, - Password: cfg.Engine.OpenSearch.Client.Password, - Header: cfg.Engine.OpenSearch.Client.Header, - RetryOnStatus: cfg.Engine.OpenSearch.Client.RetryOnStatus, - DisableRetry: cfg.Engine.OpenSearch.Client.DisableRetry, - EnableRetryOnTimeout: cfg.Engine.OpenSearch.Client.EnableRetryOnTimeout, - MaxRetries: cfg.Engine.OpenSearch.Client.MaxRetries, - CompressRequestBody: cfg.Engine.OpenSearch.Client.CompressRequestBody, - DiscoverNodesOnStart: &cfg.Engine.OpenSearch.Client.DiscoverNodesOnStart, - DiscoverNodesInterval: cfg.Engine.OpenSearch.Client.DiscoverNodesInterval, - EnableMetrics: cfg.Engine.OpenSearch.Client.EnableMetrics, - EnableDebugLogger: cfg.Engine.OpenSearch.Client.EnableDebugLogger, - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - MinVersion: tls.VersionTLS12, - InsecureSkipVerify: cfg.Engine.OpenSearch.Client.Insecure, - }, - }, - } - - if cfg.Engine.OpenSearch.Client.CACert != "" { - certBytes, err := os.ReadFile(cfg.Engine.OpenSearch.Client.CACert) - if err != nil { - return fmt.Errorf("failed to read CA cert: %w", err) - } - clientConfig.CACert = certBytes - } - - client, err := opensearchgoAPI.NewClient(opensearchgoAPI.Config{Client: clientConfig}) + client, err := opensearch.NewClient(cfg.Engine.OpenSearch.Client) if err != nil { - return fmt.Errorf("failed to create OpenSearch client: %w", err) + return err } // a hung cluster must fail the start, not block it forever diff --git a/services/search/pkg/opensearch/client.go b/services/search/pkg/opensearch/client.go new file mode 100644 index 0000000000..34972d6625 --- /dev/null +++ b/services/search/pkg/opensearch/client.go @@ -0,0 +1,52 @@ +package opensearch + +import ( + "crypto/tls" + "fmt" + "net/http" + "os" + + opensearchgo "github.com/opensearch-project/opensearch-go/v4" + opensearchgoAPI "github.com/opensearch-project/opensearch-go/v4/opensearchapi" + + "github.com/opencloud-eu/opencloud/services/search/pkg/config" +) + +// NewClient builds an OpenSearch API client from the engine client config. +func NewClient(cfg config.EngineOpenSearchClient) (*opensearchgoAPI.Client, error) { + clientConfig := opensearchgo.Config{ + Addresses: cfg.Addresses, + Username: cfg.Username, + Password: cfg.Password, + Header: cfg.Header, + RetryOnStatus: cfg.RetryOnStatus, + DisableRetry: cfg.DisableRetry, + EnableRetryOnTimeout: cfg.EnableRetryOnTimeout, + MaxRetries: cfg.MaxRetries, + CompressRequestBody: cfg.CompressRequestBody, + DiscoverNodesOnStart: &cfg.DiscoverNodesOnStart, + DiscoverNodesInterval: cfg.DiscoverNodesInterval, + EnableMetrics: cfg.EnableMetrics, + EnableDebugLogger: cfg.EnableDebugLogger, + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, + InsecureSkipVerify: cfg.Insecure, + }, + }, + } + + if cfg.CACert != "" { + certBytes, err := os.ReadFile(cfg.CACert) + if err != nil { + return nil, fmt.Errorf("failed to read CA cert: %w", err) + } + clientConfig.CACert = certBytes + } + + client, err := opensearchgoAPI.NewClient(opensearchgoAPI.Config{Client: clientConfig}) + if err != nil { + return nil, fmt.Errorf("failed to create OpenSearch client: %w", err) + } + return client, nil +} From e8c93359b18610d20d40a647ecc4b2883ad1cd96 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 29 Jul 2026 21:27:51 +0200 Subject: [PATCH 13/25] refactor(search): route schema verdict handling through a shared mapping.Reconcile --- services/search/pkg/bleve/index.go | 79 ++++++++++++++---------- services/search/pkg/bleve/index_test.go | 25 ++++---- services/search/pkg/command/server.go | 10 +-- services/search/pkg/mapping/reconcile.go | 37 +++++++++++ services/search/pkg/opensearch/index.go | 68 ++++++++++++-------- 5 files changed, 140 insertions(+), 79 deletions(-) create mode 100644 services/search/pkg/mapping/reconcile.go diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index d4c5d57de2..d699c8f234 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -20,6 +20,7 @@ import ( "github.com/blevesearch/bleve/v2/mapping" storageProvider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/opencloud-eu/opencloud/pkg/log" searchmapping "github.com/opencloud-eu/opencloud/services/search/pkg/mapping" "github.com/opencloud-eu/opencloud/services/search/pkg/search" ) @@ -28,12 +29,10 @@ import ( // instead of blocking forever on the file lock. var openRuntimeConfig = map[string]interface{}{"bolt_timeout": "5s"} -// NewIndex opens (or creates) the bleve index at root and classifies the -// stored schema against NewMapping(). Breaking changes refuse with -// ErrManualActionRequired, additive ones are persisted into the index (the -// bleve analogue of PUT _mapping); the caller must warn that pre-upgrade -// documents lack the Classification.NewFields until re-indexed. -func NewIndex(root string) (bleve.Index, searchmapping.Classification, error) { +// NewIndex opens (or creates) the bleve index at root and reconciles the stored +// schema against NewMapping() via searchmapping.Reconcile: a breaking change +// refuses to start, an additive one is persisted into the index and warned about. +func NewIndex(root string, logger log.Logger) (bleve.Index, searchmapping.Classification, error) { destination := filepath.Join(root, fmt.Sprintf("bleve-v%d", search.SchemaVersion)) index, err := bleve.OpenUsing(destination, openRuntimeConfig) if errors.Is(err, bleve.ErrorIndexPathDoesNotExist) { @@ -52,37 +51,51 @@ func NewIndex(root string) (bleve.Index, searchmapping.Classification, error) { return nil, searchmapping.Classification{}, err } - classification, codeB, err := classifyStoredMapping(index) + r := &bleveReconciler{index: index, destination: destination} + classification, err := searchmapping.Reconcile(destination, r, logger) if err != nil { - _ = index.Close() - return nil, searchmapping.Classification{}, err + if r.index != nil { + _ = r.index.Close() + } + return nil, classification, err } - switch classification.Verdict { - case searchmapping.VerdictBreaking: - _ = index.Close() - return nil, classification, searchmapping.ManualActionRequiredError(destination, classification.Reasons) - case searchmapping.VerdictAdditive: - // Safe: everything else is identical and the new fields hold no data. - // Reopen so the live mapping picks the change up; otherwise the fields - // get indexed dynamically and flip to breaking on the next start. - // return the classification even on errors: the mapping may already be - // persisted, so the next start classifies equal and the caller's - // warning is the only chance to surface the new fields - if err := index.SetInternal([]byte("_mapping"), codeB); err != nil { - _ = index.Close() - return nil, classification, fmt.Errorf("failed to store the updated index mapping: %w", err) - } - if err := index.Close(); err != nil { - return nil, classification, err - } - index, err = bleve.OpenUsing(destination, openRuntimeConfig) - if err != nil { - return nil, classification, err - } - } + return r.index, classification, nil +} - return index, classification, nil +// bleveReconciler adapts a bleve index to searchmapping.SchemaReconciler. +type bleveReconciler struct { + index bleve.Index + destination string + codeB []byte // marshaled code mapping, produced by Classify, used by ApplyAdditive +} + +func (r *bleveReconciler) Classify() (searchmapping.Classification, error) { + classification, codeB, err := classifyStoredMapping(r.index) + r.codeB = codeB + return classification, err +} + +// ApplyAdditive persists the code mapping and reopens so the live mapping picks +// it up; otherwise the new fields get indexed dynamically and flip to breaking +// on the next start. On failure it closes the index and clears the handle. +func (r *bleveReconciler) ApplyAdditive() error { + if err := r.index.SetInternal([]byte("_mapping"), r.codeB); err != nil { + _ = r.index.Close() + r.index = nil + return fmt.Errorf("failed to store the updated index mapping: %w", err) + } + if err := r.index.Close(); err != nil { + r.index = nil + return err + } + index, err := bleve.OpenUsing(r.destination, openRuntimeConfig) + if err != nil { + r.index = nil + return err + } + r.index = index + return nil } // classifyStoredMapping diffs the stored mapping against NewMapping() and diff --git a/services/search/pkg/bleve/index_test.go b/services/search/pkg/bleve/index_test.go index 40c28c36a0..de0ecdbb01 100644 --- a/services/search/pkg/bleve/index_test.go +++ b/services/search/pkg/bleve/index_test.go @@ -14,6 +14,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/search/pkg/bleve" searchmapping "github.com/opencloud-eu/opencloud/services/search/pkg/mapping" "github.com/opencloud-eu/opencloud/services/search/pkg/search" @@ -74,7 +75,7 @@ var _ = Describe("NewIndex", func() { } It("creates a fresh index", func() { - idx, classification, err := bleve.NewIndex(root) + idx, classification, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).ToNot(HaveOccurred()) Expect(classification.Verdict).To(Equal(searchmapping.VerdictEqual)) Expect(idx.Close()).To(Succeed()) @@ -83,7 +84,7 @@ var _ = Describe("NewIndex", func() { It("opens an index with an identical schema", func() { buildIndex(codeMapping(), nil) - idx, classification, err := bleve.NewIndex(root) + idx, classification, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).ToNot(HaveOccurred()) Expect(classification.Verdict).To(Equal(searchmapping.VerdictEqual)) Expect(classification.NewFields).To(BeEmpty()) @@ -96,7 +97,7 @@ var _ = Describe("NewIndex", func() { delete(old.DefaultMapping.Properties, "Title") buildIndex(old, nil) - idx, classification, err := bleve.NewIndex(root) + idx, classification, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).ToNot(HaveOccurred()) Expect(classification.Verdict).To(Equal(searchmapping.VerdictAdditive)) Expect(classification.NewFields).To(ConsistOf("Title")) @@ -112,7 +113,7 @@ var _ = Describe("NewIndex", func() { delete(photo.Properties, "cameraMake") buildIndex(old, nil) - idx, classification, err := bleve.NewIndex(root) + idx, classification, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).ToNot(HaveOccurred()) Expect(classification.Verdict).To(Equal(searchmapping.VerdictAdditive)) Expect(classification.NewFields).To(ConsistOf("photo.cameraMake")) @@ -124,13 +125,13 @@ var _ = Describe("NewIndex", func() { delete(old.DefaultMapping.Properties, "Title") buildIndex(old, nil) - idx, classification, err := bleve.NewIndex(root) + idx, classification, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).ToNot(HaveOccurred()) Expect(classification.Verdict).To(Equal(searchmapping.VerdictAdditive)) Expect(idx.Index("1", map[string]any{"Title": "hello"})).To(Succeed()) Expect(idx.Close()).To(Succeed()) - idx, classification, err = bleve.NewIndex(root) + idx, classification, err = bleve.NewIndex(root, log.NopLogger()) Expect(err).ToNot(HaveOccurred()) Expect(classification.Verdict).To(Equal(searchmapping.VerdictEqual)) Expect(idx.Close()).To(Succeed()) @@ -142,7 +143,7 @@ var _ = Describe("NewIndex", func() { delete(old.DefaultMapping.Properties, "Mtime") buildIndex(old, map[string]map[string]any{"1": {"Mtime": "2026-01-02T03:04:05Z"}}) - idx, _, err := bleve.NewIndex(root) + idx, _, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) Expect(idx).To(BeNil()) }) @@ -153,7 +154,7 @@ var _ = Describe("NewIndex", func() { delete(old.DefaultMapping.Properties, "photo") buildIndex(old, map[string]map[string]any{"1": {"photo": map[string]any{"cameraMake": "ACME"}}}) - _, _, err := bleve.NewIndex(root) + _, _, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) }) @@ -165,7 +166,7 @@ var _ = Describe("NewIndex", func() { name.Fields[0].Analyzer = "fulltext" buildIndex(old, nil) - _, _, err := bleve.NewIndex(root) + _, _, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) }) @@ -174,7 +175,7 @@ var _ = Describe("NewIndex", func() { old.DefaultMapping.AddFieldMappingsAt("Legacy", bleveSearch.NewTextFieldMapping()) buildIndex(old, nil) - _, _, err := bleve.NewIndex(root) + _, _, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) }) @@ -183,7 +184,7 @@ var _ = Describe("NewIndex", func() { old.DefaultMapping.Dynamic = false buildIndex(old, nil) - _, _, err := bleve.NewIndex(root) + _, _, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) }) @@ -197,7 +198,7 @@ var _ = Describe("NewIndex", func() { } buildIndex(old, nil) - _, _, err := bleve.NewIndex(root) + _, _, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).To(MatchError(searchmapping.ErrManualActionRequired)) }) }) diff --git a/services/search/pkg/command/server.go b/services/search/pkg/command/server.go index 1d6c89b5db..dbfe28af9b 100644 --- a/services/search/pkg/command/server.go +++ b/services/search/pkg/command/server.go @@ -18,7 +18,6 @@ import ( "github.com/opencloud-eu/opencloud/services/search/pkg/config" "github.com/opencloud-eu/opencloud/services/search/pkg/config/parser" "github.com/opencloud-eu/opencloud/services/search/pkg/content" - searchmapping "github.com/opencloud-eu/opencloud/services/search/pkg/mapping" "github.com/opencloud-eu/opencloud/services/search/pkg/metrics" "github.com/opencloud-eu/opencloud/services/search/pkg/opensearch" bleveQuery "github.com/opencloud-eu/opencloud/services/search/pkg/query/bleve" @@ -68,14 +67,7 @@ func Server(cfg *config.Config) *cobra.Command { var eng search.Engine switch cfg.Engine.Type { case "bleve": - idx, classification, err := bleve.NewIndex(cfg.Engine.Bleve.Datapath) - // warn before the error check: the new mapping may already be - // persisted, then later startups classify equal and stay silent - if classification.Verdict == searchmapping.VerdictAdditive { - logger.Warn(). - Strs("fields", classification.NewFields). - Msgf("the bleve index at %s was built with an older schema; the new fields were added to the index schema, but documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces --force-rescan", cfg.Engine.Bleve.Datapath) - } + idx, _, err := bleve.NewIndex(cfg.Engine.Bleve.Datapath, logger) if err != nil { return err } diff --git a/services/search/pkg/mapping/reconcile.go b/services/search/pkg/mapping/reconcile.go new file mode 100644 index 0000000000..8f02604d8c --- /dev/null +++ b/services/search/pkg/mapping/reconcile.go @@ -0,0 +1,37 @@ +package mapping + +import ( + "github.com/opencloud-eu/opencloud/pkg/log" +) + +// SchemaReconciler is the engine-specific half of the startup schema check. +// Classify reads the stored and code schema and returns the verdict (with any +// engine-specific extras, e.g. analyzer/settings drift, already folded in); +// ApplyAdditive applies an additive change to the live index. Reconcile drives +// them so the verdict-to-action mapping lives in one place for every backend. +type SchemaReconciler interface { + Classify() (Classification, error) + ApplyAdditive() error +} + +// Reconcile runs the shared schema-verdict flow: an equal schema starts +// silently, a breaking one refuses with ManualActionRequiredError, an additive +// one is applied and warned about. index names the index in the messages. +func Reconcile(index string, r SchemaReconciler, logger log.Logger) (Classification, error) { + classification, err := r.Classify() + if err != nil { + return classification, err + } + + switch classification.Verdict { + case VerdictBreaking: + return classification, ManualActionRequiredError(index, classification.Reasons) + case VerdictAdditive: + if err := r.ApplyAdditive(); err != nil { + return classification, err + } + logger.Warn().Strs("fields", classification.NewFields).Str("index", index).Msg("extended the search index mapping with new fields; documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces --force-rescan") + } + + return classification, nil +} diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index 3b49393b5b..550f7a8ff6 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -114,10 +114,11 @@ func buildResourceMapping() ([]byte, error) { return json.Marshal(index) } -// Apply ensures the index exists and matches the schema generated from code: -// created if missing, additive changes applied via PUT _mapping, breaking ones -// refused with ErrManualActionRequired. The classifier judges, PUT _mapping -// only applies (its merge semantics hide removals and renames). +// Apply ensures the index exists and matches the schema generated from code: it +// is created if missing, otherwise its schema is reconciled via +// searchmapping.Reconcile (see osReconciler). PUT _mapping only applies the +// change; the classifier judges, because its merge semantics hide removals and +// renames. func (m IndexManager) Apply(ctx context.Context, name string, client *opensearchgoAPI.Client, logger log.Logger) error { localIndexB, err := m.MarshalJSON() if err != nil { @@ -151,7 +152,7 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch return fmt.Errorf("indicesExistsResp is nil for index %s", name) } - // the index exists: compare settings and classify the mapping diff + // the index exists: reconcile its schema through the shared verdict flow resp, err := client.Indices.Get(ctx, opensearchgoAPI.IndicesGetReq{ Indices: []string{name}, }) @@ -168,52 +169,69 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch return fmt.Errorf("failed to marshal index %s: %w", name, err) } - localIndexJson := gjson.ParseBytes(localIndexB) - remoteIndexJson := gjson.ParseBytes(remoteIndexB) + r := &osReconciler{ + ctx: ctx, + name: name, + client: client, + local: gjson.ParseBytes(localIndexB), + remote: gjson.ParseBytes(remoteIndexB), + } + _, err = searchmapping.Reconcile(name, r, logger) + return err +} +// osReconciler adapts an existing OpenSearch index to searchmapping.SchemaReconciler. +type osReconciler struct { + ctx context.Context + name string + client *opensearchgoAPI.Client + local gjson.Result + remote gjson.Result +} + +func (r *osReconciler) Classify() (searchmapping.Classification, error) { // Only the analysis settings (analyzers/tokenizers/filters) affect how data // is indexed and queried; a drift there yields wrong results. Shard/replica // counts and other operational knobs are the operator's to tune (and a // pre-provisioned index's to own), so they are not compared. var reasons []string - lv := localIndexJson.Get("settings.analysis").Raw - rv := remoteIndexJson.Get("settings.index.analysis").Raw + lv := r.local.Get("settings.analysis").Raw + rv := r.remote.Get("settings.index.analysis").Raw if !jsonEqual(lv, rv) { reasons = append(reasons, fmt.Sprintf("settings.analysis changed: index %s, code %s", rawOrUnset(rv), rawOrUnset(lv))) } classification := searchmapping.Classify( - propertiesMap(remoteIndexJson.Get("mappings.properties").Raw), - propertiesMap(localIndexJson.Get("mappings.properties").Raw), + propertiesMap(r.remote.Get("mappings.properties").Raw), + propertiesMap(r.local.Get("mappings.properties").Raw), nil, ) reasons = append(reasons, classification.Reasons...) if len(reasons) > 0 { - return searchmapping.ManualActionRequiredError(name, reasons) - } - if len(classification.NewFields) == 0 { - return nil // schema is up to date + classification.Verdict = searchmapping.VerdictBreaking + classification.Reasons = reasons } + return classification, nil +} - // additive: the classifier guarantees every existing field matches the - // remote state, so putting the full code properties can only add fields - putResp, err := client.Indices.Mapping.Put(ctx, opensearchgoAPI.MappingPutReq{ - Indices: []string{name}, - Body: strings.NewReader(localIndexJson.Get("mappings").Raw), +// ApplyAdditive puts the full code properties; the classifier guarantees every +// existing field already matches the remote state, so this can only add fields. +func (r *osReconciler) ApplyAdditive() error { + putResp, err := r.client.Indices.Mapping.Put(r.ctx, opensearchgoAPI.MappingPutReq{ + Indices: []string{r.name}, + Body: strings.NewReader(r.local.Get("mappings").Raw), }) var putErr *opensearchgo.StructError switch { case err != nil && errors.As(err, &putErr) && putErr.Err.Type == "illegal_argument_exception" && (strings.Contains(putErr.Err.Reason, "cannot be changed") || strings.Contains(putErr.Err.Reason, "Cannot update parameter")): // backstop, should be unreachable after the classification above - return searchmapping.ManualActionRequiredError(name, []string{putErr.Err.Reason}) + return searchmapping.ManualActionRequiredError(r.name, []string{putErr.Err.Reason}) case err != nil: - return fmt.Errorf("failed to update mapping of index %s: %w", name, err) + return fmt.Errorf("failed to update mapping of index %s: %w", r.name, err) case !putResp.Acknowledged: - return fmt.Errorf("failed to update mapping of index %s: not acknowledged", name) + return fmt.Errorf("failed to update mapping of index %s: not acknowledged", r.name) } - - logger.Warn().Strs("fields", classification.NewFields).Str("index", name).Msg("extended the search index mapping with new fields; documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces --force-rescan") return nil } From fec7536617573aac833df8d197c05156954f7ede Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 29 Jul 2026 21:32:38 +0200 Subject: [PATCH 14/25] refactor(search): warn on a persisted additive change even when the reopen fails --- services/search/pkg/bleve/index.go | 13 +++++++------ services/search/pkg/mapping/reconcile.go | 14 +++++++++++--- services/search/pkg/opensearch/index.go | 11 ++++++----- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index d699c8f234..c84767e685 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -78,24 +78,25 @@ func (r *bleveReconciler) Classify() (searchmapping.Classification, error) { // ApplyAdditive persists the code mapping and reopens so the live mapping picks // it up; otherwise the new fields get indexed dynamically and flip to breaking -// on the next start. On failure it closes the index and clears the handle. -func (r *bleveReconciler) ApplyAdditive() error { +// on the next start. Once SetInternal succeeds it reports persisted=true even if +// the reopen then fails. On failure it closes the index and clears the handle. +func (r *bleveReconciler) ApplyAdditive() (bool, error) { if err := r.index.SetInternal([]byte("_mapping"), r.codeB); err != nil { _ = r.index.Close() r.index = nil - return fmt.Errorf("failed to store the updated index mapping: %w", err) + return false, fmt.Errorf("failed to store the updated index mapping: %w", err) } if err := r.index.Close(); err != nil { r.index = nil - return err + return true, err } index, err := bleve.OpenUsing(r.destination, openRuntimeConfig) if err != nil { r.index = nil - return err + return true, err } r.index = index - return nil + return true, nil } // classifyStoredMapping diffs the stored mapping against NewMapping() and diff --git a/services/search/pkg/mapping/reconcile.go b/services/search/pkg/mapping/reconcile.go index 8f02604d8c..a812128c2b 100644 --- a/services/search/pkg/mapping/reconcile.go +++ b/services/search/pkg/mapping/reconcile.go @@ -11,7 +11,12 @@ import ( // them so the verdict-to-action mapping lives in one place for every backend. type SchemaReconciler interface { Classify() (Classification, error) - ApplyAdditive() error + // ApplyAdditive applies an additive change to the live index and reports + // whether the schema was persisted. It returns persisted=true even when a + // later step then fails (e.g. a bleve reopen), so Reconcile can still warn: + // the change is on disk, a subsequent start classifies equal and stays + // silent, so this is the only chance to surface the new fields. + ApplyAdditive() (persisted bool, err error) } // Reconcile runs the shared schema-verdict flow: an equal schema starts @@ -27,10 +32,13 @@ func Reconcile(index string, r SchemaReconciler, logger log.Logger) (Classificat case VerdictBreaking: return classification, ManualActionRequiredError(index, classification.Reasons) case VerdictAdditive: - if err := r.ApplyAdditive(); err != nil { + persisted, err := r.ApplyAdditive() + if persisted { + logger.Warn().Strs("fields", classification.NewFields).Str("index", index).Msg("extended the search index mapping with new fields; documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces --force-rescan") + } + if err != nil { return classification, err } - logger.Warn().Strs("fields", classification.NewFields).Str("index", index).Msg("extended the search index mapping with new fields; documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces --force-rescan") } return classification, nil diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index 550f7a8ff6..5870c8aa9d 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -216,7 +216,8 @@ func (r *osReconciler) Classify() (searchmapping.Classification, error) { // ApplyAdditive puts the full code properties; the classifier guarantees every // existing field already matches the remote state, so this can only add fields. -func (r *osReconciler) ApplyAdditive() error { +// The PUT is atomic, so persisted is true only on success. +func (r *osReconciler) ApplyAdditive() (bool, error) { putResp, err := r.client.Indices.Mapping.Put(r.ctx, opensearchgoAPI.MappingPutReq{ Indices: []string{r.name}, Body: strings.NewReader(r.local.Get("mappings").Raw), @@ -226,13 +227,13 @@ func (r *osReconciler) ApplyAdditive() error { case err != nil && errors.As(err, &putErr) && putErr.Err.Type == "illegal_argument_exception" && (strings.Contains(putErr.Err.Reason, "cannot be changed") || strings.Contains(putErr.Err.Reason, "Cannot update parameter")): // backstop, should be unreachable after the classification above - return searchmapping.ManualActionRequiredError(r.name, []string{putErr.Err.Reason}) + return false, searchmapping.ManualActionRequiredError(r.name, []string{putErr.Err.Reason}) case err != nil: - return fmt.Errorf("failed to update mapping of index %s: %w", r.name, err) + return false, fmt.Errorf("failed to update mapping of index %s: %w", r.name, err) case !putResp.Acknowledged: - return fmt.Errorf("failed to update mapping of index %s: not acknowledged", r.name) + return false, fmt.Errorf("failed to update mapping of index %s: not acknowledged", r.name) } - return nil + return true, nil } // jsonEqual reports whether two raw JSON values are deeply equal. gjson yields From a8ffe91c79075b666d73da90125e8fb20b903a18 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 29 Jul 2026 21:33:59 +0200 Subject: [PATCH 15/25] feat(search): log a reindex hint when a fresh search index is created --- services/search/pkg/bleve/index.go | 1 + services/search/pkg/opensearch/index.go | 1 + 2 files changed, 2 insertions(+) diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index c84767e685..44ef9ceb15 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -45,6 +45,7 @@ func NewIndex(root string, logger log.Logger) (bleve.Index, searchmapping.Classi return nil, searchmapping.Classification{}, err } + logger.Info().Str("index", destination).Msg("created a new empty search index; if this OpenCloud instance already held files, they are not in it yet, index them by running: opencloud search index --all-spaces --force-rescan") return index, searchmapping.Classification{Verdict: searchmapping.VerdictEqual}, nil } if err != nil { diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index 5870c8aa9d..6e9befc2d6 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -138,6 +138,7 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch var structErr *opensearchgo.StructError switch { case createErr == nil && createResp.Acknowledged: + logger.Info().Str("index", name).Msg("created a new empty search index; if this OpenCloud instance already held files, they are not in it yet, index them by running: opencloud search index --all-spaces --force-rescan") return nil case createErr == nil: return fmt.Errorf("failed to create index %s: not acknowledged", name) From dc863c64c69032955c836297dcedefcc80846772 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 29 Jul 2026 22:07:10 +0200 Subject: [PATCH 16/25] refactor(search): single-source the new-index log message --- services/search/pkg/bleve/index.go | 2 +- services/search/pkg/mapping/reconcile.go | 7 +++++++ services/search/pkg/opensearch/index.go | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index 44ef9ceb15..cae0338c7b 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -45,7 +45,7 @@ func NewIndex(root string, logger log.Logger) (bleve.Index, searchmapping.Classi return nil, searchmapping.Classification{}, err } - logger.Info().Str("index", destination).Msg("created a new empty search index; if this OpenCloud instance already held files, they are not in it yet, index them by running: opencloud search index --all-spaces --force-rescan") + searchmapping.LogNewIndexCreated(logger, destination) return index, searchmapping.Classification{Verdict: searchmapping.VerdictEqual}, nil } if err != nil { diff --git a/services/search/pkg/mapping/reconcile.go b/services/search/pkg/mapping/reconcile.go index a812128c2b..460218f8f0 100644 --- a/services/search/pkg/mapping/reconcile.go +++ b/services/search/pkg/mapping/reconcile.go @@ -43,3 +43,10 @@ func Reconcile(index string, r SchemaReconciler, logger log.Logger) (Classificat return classification, nil } + +// LogNewIndexCreated logs that a fresh, empty index was created and how to +// backfill it. Both backends call it after creating their index (that path does +// not run through Reconcile); an existing, up-to-date index stays silent. +func LogNewIndexCreated(logger log.Logger, index string) { + logger.Info().Str("index", index).Msg("created a new empty search index; if this OpenCloud instance already held files, they are not in it yet, index them by running: opencloud search index --all-spaces --force-rescan") +} diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index 6e9befc2d6..e7146822f9 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -138,7 +138,7 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch var structErr *opensearchgo.StructError switch { case createErr == nil && createResp.Acknowledged: - logger.Info().Str("index", name).Msg("created a new empty search index; if this OpenCloud instance already held files, they are not in it yet, index them by running: opencloud search index --all-spaces --force-rescan") + searchmapping.LogNewIndexCreated(logger, name) return nil case createErr == nil: return fmt.Errorf("failed to create index %s: not acknowledged", name) From 6b07a0d9dccb2817b5dd34c96343aaa4802ab840 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 29 Jul 2026 22:19:49 +0200 Subject: [PATCH 17/25] chore(search): tighten the schema-reconcile comments --- services/search/pkg/bleve/index.go | 10 ++++------ services/search/pkg/mapping/reconcile.go | 21 +++++++++------------ services/search/pkg/opensearch/index.go | 15 +++++---------- 3 files changed, 18 insertions(+), 28 deletions(-) diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index cae0338c7b..ec7bd0274e 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -29,9 +29,8 @@ import ( // instead of blocking forever on the file lock. var openRuntimeConfig = map[string]interface{}{"bolt_timeout": "5s"} -// NewIndex opens (or creates) the bleve index at root and reconciles the stored -// schema against NewMapping() via searchmapping.Reconcile: a breaking change -// refuses to start, an additive one is persisted into the index and warned about. +// NewIndex opens (or creates) the bleve index at root and reconciles its schema +// against NewMapping() via searchmapping.Reconcile. func NewIndex(root string, logger log.Logger) (bleve.Index, searchmapping.Classification, error) { destination := filepath.Join(root, fmt.Sprintf("bleve-v%d", search.SchemaVersion)) index, err := bleve.OpenUsing(destination, openRuntimeConfig) @@ -78,9 +77,8 @@ func (r *bleveReconciler) Classify() (searchmapping.Classification, error) { } // ApplyAdditive persists the code mapping and reopens so the live mapping picks -// it up; otherwise the new fields get indexed dynamically and flip to breaking -// on the next start. Once SetInternal succeeds it reports persisted=true even if -// the reopen then fails. On failure it closes the index and clears the handle. +// it up. persisted=true once SetInternal succeeds, even if the reopen then +// fails; on error it closes the index and clears the handle. func (r *bleveReconciler) ApplyAdditive() (bool, error) { if err := r.index.SetInternal([]byte("_mapping"), r.codeB); err != nil { _ = r.index.Close() diff --git a/services/search/pkg/mapping/reconcile.go b/services/search/pkg/mapping/reconcile.go index 460218f8f0..6f8c056127 100644 --- a/services/search/pkg/mapping/reconcile.go +++ b/services/search/pkg/mapping/reconcile.go @@ -4,18 +4,16 @@ import ( "github.com/opencloud-eu/opencloud/pkg/log" ) -// SchemaReconciler is the engine-specific half of the startup schema check. -// Classify reads the stored and code schema and returns the verdict (with any -// engine-specific extras, e.g. analyzer/settings drift, already folded in); -// ApplyAdditive applies an additive change to the live index. Reconcile drives -// them so the verdict-to-action mapping lives in one place for every backend. +// SchemaReconciler is the engine-specific half of the startup schema check: +// Classify reads stored vs code schema (extras like analyzer drift folded in), +// ApplyAdditive applies an additive change. Reconcile drives them so the +// verdict-to-action policy lives in one place for every backend. type SchemaReconciler interface { Classify() (Classification, error) - // ApplyAdditive applies an additive change to the live index and reports - // whether the schema was persisted. It returns persisted=true even when a - // later step then fails (e.g. a bleve reopen), so Reconcile can still warn: - // the change is on disk, a subsequent start classifies equal and stays - // silent, so this is the only chance to surface the new fields. + // ApplyAdditive applies an additive change and reports whether the schema + // was persisted. persisted=true even if a later step fails (e.g. a bleve + // reopen), so Reconcile still warns: it is on disk, the next start + // classifies equal and stays silent. ApplyAdditive() (persisted bool, err error) } @@ -45,8 +43,7 @@ func Reconcile(index string, r SchemaReconciler, logger log.Logger) (Classificat } // LogNewIndexCreated logs that a fresh, empty index was created and how to -// backfill it. Both backends call it after creating their index (that path does -// not run through Reconcile); an existing, up-to-date index stays silent. +// backfill it. The create path does not run through Reconcile. func LogNewIndexCreated(logger log.Logger, index string) { logger.Info().Str("index", index).Msg("created a new empty search index; if this OpenCloud instance already held files, they are not in it yet, index them by running: opencloud search index --all-spaces --force-rescan") } diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index e7146822f9..3ba873b6f6 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -114,11 +114,8 @@ func buildResourceMapping() ([]byte, error) { return json.Marshal(index) } -// Apply ensures the index exists and matches the schema generated from code: it -// is created if missing, otherwise its schema is reconciled via -// searchmapping.Reconcile (see osReconciler). PUT _mapping only applies the -// change; the classifier judges, because its merge semantics hide removals and -// renames. +// Apply ensures the index exists and matches the code schema: created if +// missing, otherwise reconciled via searchmapping.Reconcile (see osReconciler). func (m IndexManager) Apply(ctx context.Context, name string, client *opensearchgoAPI.Client, logger log.Logger) error { localIndexB, err := m.MarshalJSON() if err != nil { @@ -191,8 +188,7 @@ type osReconciler struct { } func (r *osReconciler) Classify() (searchmapping.Classification, error) { - // Only the analysis settings (analyzers/tokenizers/filters) affect how data - // is indexed and queried; a drift there yields wrong results. Shard/replica + // Only the analysis settings affect indexing correctness; shard/replica // counts and other operational knobs are the operator's to tune (and a // pre-provisioned index's to own), so they are not compared. var reasons []string @@ -215,9 +211,8 @@ func (r *osReconciler) Classify() (searchmapping.Classification, error) { return classification, nil } -// ApplyAdditive puts the full code properties; the classifier guarantees every -// existing field already matches the remote state, so this can only add fields. -// The PUT is atomic, so persisted is true only on success. +// ApplyAdditive puts the full code properties (only additions, per the +// classifier). The PUT is atomic, so persisted is true only on success. func (r *osReconciler) ApplyAdditive() (bool, error) { putResp, err := r.client.Indices.Mapping.Put(r.ctx, opensearchgoAPI.MappingPutReq{ Indices: []string{r.name}, From a0521f0260646f82d332eb7cd3d561b9da767ef2 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 19 Aug 2026 10:07:15 +0200 Subject: [PATCH 18/25] review: trim verbose comments, cover Reconcile, table-driven Classify tests - shorten the multi-line doc comments flagged as too verbose - add reconcile_test.go: direct unit tests for Reconcile incl. the persisted-but-errored and classify-error branches (previously only reached indirectly through the engine integration tests) - convert the 11 near-identical Classify It blocks to a DescribeTable --- services/search/pkg/bleve/index.go | 7 +- services/search/pkg/mapping/bleve.go | 6 +- services/search/pkg/mapping/classify.go | 17 +- services/search/pkg/mapping/classify_test.go | 183 +++++++++--------- services/search/pkg/mapping/reconcile.go | 17 +- services/search/pkg/mapping/reconcile_test.go | 78 ++++++++ services/search/pkg/opensearch/index.go | 6 +- 7 files changed, 194 insertions(+), 120 deletions(-) create mode 100644 services/search/pkg/mapping/reconcile_test.go diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index ec7bd0274e..e7431f3b72 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -99,10 +99,9 @@ func (r *bleveReconciler) ApplyAdditive() (bool, error) { } // classifyStoredMapping diffs the stored mapping against NewMapping() and -// returns the marshaled code mapping. New-in-code fields that already hold -// data (previously indexed dynamically) are breaking. The compare is only -// stable within one bleve version: a changed marshaling default fails towards -// breaking, normalize the affected key here if that ever fires. +// returns the marshaled code mapping. New-in-code fields that already hold data +// (previously indexed dynamically) are breaking. The compare assumes stable +// bleve marshaling; the golden test guards against a marshaling-default drift. func classifyStoredMapping(index bleve.Index) (searchmapping.Classification, []byte, error) { storedB, err := index.GetInternal([]byte("_mapping")) if err != nil { diff --git a/services/search/pkg/mapping/bleve.go b/services/search/pkg/mapping/bleve.go index 48fc9424e3..0a3a55c45b 100644 --- a/services/search/pkg/mapping/bleve.go +++ b/services/search/pkg/mapping/bleve.go @@ -12,10 +12,8 @@ import ( // struct via reflection. Field names come from json tags; overrides are // keyed by those names (or dotted paths for nested fields). // -// The returned mapping references analyzer names (Analyzer on the FieldOpts, -// plus the words analyzer for the Fulltext type); the caller registers every -// referenced analyzer on the enclosing IndexMapping (IndexMapping.Validate -// catches missing ones). +// The returned mapping references analyzer names that the caller must register +// on the enclosing IndexMapping (IndexMapping.Validate catches missing ones). func BleveBuildMapping(t reflect.Type, overrides map[string]FieldOpts) (*bleveMapping.DocumentMapping, error) { return buildBleveDocMapping(t, overrides, "") } diff --git a/services/search/pkg/mapping/classify.go b/services/search/pkg/mapping/classify.go index 2e0e7e100a..3d76761fa8 100644 --- a/services/search/pkg/mapping/classify.go +++ b/services/search/pkg/mapping/classify.go @@ -13,11 +13,9 @@ import ( // ErrManualActionRequired marks schema changes that cannot be applied in place. var ErrManualActionRequired = errors.New("manual action required") -// ManualActionRequiredError reports a breaking schema change. Because the index -// is versioned by search.SchemaVersion, a released instance never hits this: a -// version bump builds a fresh index. It fires in development when the mapping is -// changed in a breaking way without bumping search.SchemaVersion, so the fix is -// to bump it (or revert the change). +// ManualActionRequiredError reports a breaking schema change. Only reachable in +// development: a released instance versions the index by search.SchemaVersion, +// so a bump builds a fresh index instead. func ManualActionRequiredError(index string, reasons []string) error { return fmt.Errorf( "%w: the search mapping in code differs from index %s in a breaking way:\n - %s\n"+ @@ -44,11 +42,10 @@ type Classification struct { Reasons []string } -// Classify recursively compares a stored `properties` tree against the one -// generated from code. Both sides must be generic JSON-decoded values, not -// marshaled Go structs. dataFields reports whether the index holds data at or -// below a dotted field path absent from the stored schema (bleve dynamic -// fields); engines without that blind spot pass nil. +// Classify recursively compares a stored `properties` tree against the one from +// code (both generic JSON-decoded, not marshaled structs). dataFields reports +// whether a code-only field already holds data in the index (bleve dynamic +// fields make a new field breaking); engines without that blind spot pass nil. func Classify(stored, code map[string]any, dataFields func(path string) bool) Classification { c := Classification{Verdict: VerdictEqual} classifyProperties(stored, code, dataFields, "", &c) diff --git a/services/search/pkg/mapping/classify_test.go b/services/search/pkg/mapping/classify_test.go index c4b4d35817..e76cdb1cfd 100644 --- a/services/search/pkg/mapping/classify_test.go +++ b/services/search/pkg/mapping/classify_test.go @@ -25,104 +25,113 @@ var _ = Describe("Classify", func() { return func(path string) bool { return slices.Contains(fields, path) } } - It("classifies identical schemas as equal", func() { - c := Classify(parse(code), parse(code), nil) - Expect(c.Verdict).To(Equal(VerdictEqual)) - Expect(c.NewFields).To(BeEmpty()) - Expect(c.Reasons).To(BeEmpty()) - }) + // setup produces the (stored, code, dataFields) arguments for one case. + type setup func() (stored, codeSchema map[string]any, dataFields func(string) bool) - It("classifies a new top-level field as additive", func() { - stored := parse(code) - delete(stored, "Size") + substrings := func(ss []string) []any { + out := make([]any, len(ss)) + for i, s := range ss { + out[i] = ContainSubstring(s) + } + return out + } + fields := func(ss []string) []any { + out := make([]any, len(ss)) + for i, s := range ss { + out[i] = s + } + return out + } - c := Classify(stored, parse(code), nil) - Expect(c.Verdict).To(Equal(VerdictAdditive)) - Expect(c.NewFields).To(ConsistOf("Size")) - Expect(c.Reasons).To(BeEmpty()) - }) + // newFields/reasons are asserted only when non-nil; an empty slice asserts + // "none". + DescribeTable("verdict", + func(s setup, verdict Verdict, newFields, reasons []string) { + stored, codeSchema, dataFields := s() - It("classifies a new nested field as additive", func() { - stored := parse(code) - delete(stored["photo"].(map[string]any)["properties"].(map[string]any), "cameraMake") + c := Classify(stored, codeSchema, dataFields) + Expect(c.Verdict).To(Equal(verdict)) + if newFields != nil { + Expect(c.NewFields).To(ConsistOf(fields(newFields)...)) + } + if reasons != nil { + Expect(c.Reasons).To(ConsistOf(substrings(reasons)...)) + } + }, + Entry("identical schemas are equal", + setup(func() (map[string]any, map[string]any, func(string) bool) { + return parse(code), parse(code), nil + }), VerdictEqual, []string{}, []string{}), - c := Classify(stored, parse(code), nil) - Expect(c.Verdict).To(Equal(VerdictAdditive)) - Expect(c.NewFields).To(ConsistOf("photo.cameraMake")) - }) + Entry("a new top-level field is additive", + setup(func() (map[string]any, map[string]any, func(string) bool) { + stored := parse(code) + delete(stored, "Size") + return stored, parse(code), nil + }), VerdictAdditive, []string{"Size"}, nil), - It("lists every leaf of a new subtree", func() { - stored := parse(code) - delete(stored, "photo") + Entry("a new nested field is additive", + setup(func() (map[string]any, map[string]any, func(string) bool) { + stored := parse(code) + delete(stored["photo"].(map[string]any)["properties"].(map[string]any), "cameraMake") + return stored, parse(code), nil + }), VerdictAdditive, []string{"photo.cameraMake"}, nil), - c := Classify(stored, parse(code), nil) - Expect(c.Verdict).To(Equal(VerdictAdditive)) - Expect(c.NewFields).To(ConsistOf("photo.cameraMake", "photo.cameraModel")) - }) + Entry("a new subtree lists every leaf", + setup(func() (map[string]any, map[string]any, func(string) bool) { + stored := parse(code) + delete(stored, "photo") + return stored, parse(code), nil + }), VerdictAdditive, []string{"photo.cameraMake", "photo.cameraModel"}, nil), - It("breaks when a new field already has data in the index", func() { - stored := parse(code) - delete(stored, "Size") + Entry("a new field that already holds data is breaking", + setup(func() (map[string]any, map[string]any, func(string) bool) { + stored := parse(code) + delete(stored, "Size") + return stored, parse(code), hasData("Size") + }), VerdictBreaking, nil, []string{"Size"}), - c := Classify(stored, parse(code), hasData("Size")) - Expect(c.Verdict).To(Equal(VerdictBreaking)) - Expect(c.Reasons).To(ConsistOf(ContainSubstring("Size"))) - }) + Entry("a new nested field that already holds data is breaking", + setup(func() (map[string]any, map[string]any, func(string) bool) { + stored := parse(code) + delete(stored["photo"].(map[string]any)["properties"].(map[string]any), "cameraMake") + return stored, parse(code), hasData("photo.cameraMake") + }), VerdictBreaking, nil, []string{"photo.cameraMake"}), - It("breaks when a new nested field already has data in the index", func() { - stored := parse(code) - delete(stored["photo"].(map[string]any)["properties"].(map[string]any), "cameraMake") + Entry("a new subtree with data below it is breaking", + setup(func() (map[string]any, map[string]any, func(string) bool) { + stored := parse(code) + delete(stored, "photo") + return stored, parse(code), hasData("photo") + }), VerdictBreaking, nil, []string{"photo"}), - c := Classify(stored, parse(code), hasData("photo.cameraMake")) - Expect(c.Verdict).To(Equal(VerdictBreaking)) - Expect(c.Reasons).To(ConsistOf(ContainSubstring("photo.cameraMake"))) - }) + Entry("a changed field definition is breaking", + setup(func() (map[string]any, map[string]any, func(string) bool) { + stored := parse(code) + stored["Size"].(map[string]any)["type"] = "keyword" + return stored, parse(code), nil + }), VerdictBreaking, nil, []string{"Size"}), - It("breaks when a new subtree already has data below it", func() { - stored := parse(code) - delete(stored, "photo") + Entry("a field removed from the code schema is breaking", + setup(func() (map[string]any, map[string]any, func(string) bool) { + reduced := parse(code) + delete(reduced, "Size") + return parse(code), reduced, nil + }), VerdictBreaking, nil, []string{"removed or renamed"}), - // the callback is consulted with the subtree root - c := Classify(stored, parse(code), hasData("photo")) - Expect(c.Verdict).To(Equal(VerdictBreaking)) - Expect(c.Reasons).To(ConsistOf(ContainSubstring("photo"))) - }) + Entry("a changed object attribute is breaking", + setup(func() (map[string]any, map[string]any, func(string) bool) { + stored := parse(code) + stored["photo"].(map[string]any)["dynamic"] = true + return stored, parse(code), nil + }), VerdictBreaking, nil, []string{"dynamic"}), - It("breaks on a changed field definition", func() { - stored := parse(code) - stored["Size"].(map[string]any)["type"] = "keyword" - - c := Classify(stored, parse(code), nil) - Expect(c.Verdict).To(Equal(VerdictBreaking)) - Expect(c.Reasons).To(ConsistOf(ContainSubstring("Size"))) - }) - - It("breaks on a field that was removed from the code schema", func() { - reduced := parse(code) - delete(reduced, "Size") - - c := Classify(parse(code), reduced, nil) - Expect(c.Verdict).To(Equal(VerdictBreaking)) - Expect(c.Reasons).To(ConsistOf(ContainSubstring("removed or renamed"))) - }) - - It("breaks on a changed object attribute", func() { - stored := parse(code) - stored["photo"].(map[string]any)["dynamic"] = true - - c := Classify(stored, parse(code), nil) - Expect(c.Verdict).To(Equal(VerdictBreaking)) - Expect(c.Reasons).To(ConsistOf(ContainSubstring("dynamic"))) - }) - - It("lets breaking win over additive", func() { - stored := parse(code) - delete(stored, "Size") - stored["Name"].(map[string]any)["type"] = "text" - - c := Classify(stored, parse(code), nil) - Expect(c.Verdict).To(Equal(VerdictBreaking)) - Expect(c.NewFields).To(ConsistOf("Size")) - Expect(c.Reasons).To(ConsistOf(ContainSubstring("Name"))) - }) + Entry("breaking wins over additive", + setup(func() (map[string]any, map[string]any, func(string) bool) { + stored := parse(code) + delete(stored, "Size") + stored["Name"].(map[string]any)["type"] = "text" + return stored, parse(code), nil + }), VerdictBreaking, []string{"Size"}, []string{"Name"}), + ) }) diff --git a/services/search/pkg/mapping/reconcile.go b/services/search/pkg/mapping/reconcile.go index 6f8c056127..82cc46feb6 100644 --- a/services/search/pkg/mapping/reconcile.go +++ b/services/search/pkg/mapping/reconcile.go @@ -4,22 +4,17 @@ import ( "github.com/opencloud-eu/opencloud/pkg/log" ) -// SchemaReconciler is the engine-specific half of the startup schema check: -// Classify reads stored vs code schema (extras like analyzer drift folded in), -// ApplyAdditive applies an additive change. Reconcile drives them so the -// verdict-to-action policy lives in one place for every backend. +// SchemaReconciler is the engine-specific half of the startup schema check that +// Reconcile drives, keeping the verdict-to-action policy in one place. type SchemaReconciler interface { Classify() (Classification, error) - // ApplyAdditive applies an additive change and reports whether the schema - // was persisted. persisted=true even if a later step fails (e.g. a bleve - // reopen), so Reconcile still warns: it is on disk, the next start - // classifies equal and stays silent. + // ApplyAdditive applies an additive change. persisted is true once the + // schema is on disk, even if a later step (e.g. a bleve reopen) then fails. ApplyAdditive() (persisted bool, err error) } -// Reconcile runs the shared schema-verdict flow: an equal schema starts -// silently, a breaking one refuses with ManualActionRequiredError, an additive -// one is applied and warned about. index names the index in the messages. +// Reconcile applies the shared verdict policy: equal is silent, breaking refuses +// with ManualActionRequiredError, additive is applied and warned about. func Reconcile(index string, r SchemaReconciler, logger log.Logger) (Classification, error) { classification, err := r.Classify() if err != nil { diff --git a/services/search/pkg/mapping/reconcile_test.go b/services/search/pkg/mapping/reconcile_test.go new file mode 100644 index 0000000000..2baa32c17f --- /dev/null +++ b/services/search/pkg/mapping/reconcile_test.go @@ -0,0 +1,78 @@ +package mapping + +import ( + "errors" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/opencloud-eu/opencloud/pkg/log" +) + +type fakeReconciler struct { + classification Classification + classifyErr error + persisted bool + applyErr error + applyCalls int +} + +func (f *fakeReconciler) Classify() (Classification, error) { + return f.classification, f.classifyErr +} + +func (f *fakeReconciler) ApplyAdditive() (bool, error) { + f.applyCalls++ + return f.persisted, f.applyErr +} + +var _ = Describe("Reconcile", func() { + logger := log.NopLogger() + + It("does nothing on an equal schema", func() { + r := &fakeReconciler{classification: Classification{Verdict: VerdictEqual}} + + c, err := Reconcile("idx", r, logger) + Expect(err).ToNot(HaveOccurred()) + Expect(c.Verdict).To(Equal(VerdictEqual)) + Expect(r.applyCalls).To(BeZero()) + }) + + It("refuses a breaking schema without applying it", func() { + r := &fakeReconciler{classification: Classification{Verdict: VerdictBreaking, Reasons: []string{"Name changed"}}} + + _, err := Reconcile("idx", r, logger) + Expect(err).To(MatchError(ErrManualActionRequired)) + Expect(err.Error()).To(ContainSubstring("Name changed")) + Expect(r.applyCalls).To(BeZero()) + }) + + It("applies an additive schema", func() { + r := &fakeReconciler{classification: Classification{Verdict: VerdictAdditive, NewFields: []string{"Size"}}, persisted: true} + + c, err := Reconcile("idx", r, logger) + Expect(err).ToNot(HaveOccurred()) + Expect(c.Verdict).To(Equal(VerdictAdditive)) + Expect(r.applyCalls).To(Equal(1)) + }) + + It("surfaces an apply error even when the schema was persisted", func() { + r := &fakeReconciler{ + classification: Classification{Verdict: VerdictAdditive, NewFields: []string{"Size"}}, + persisted: true, + applyErr: errors.New("reopen failed"), + } + + _, err := Reconcile("idx", r, logger) + Expect(err).To(MatchError(ContainSubstring("reopen failed"))) + Expect(r.applyCalls).To(Equal(1)) + }) + + It("propagates a classify error", func() { + r := &fakeReconciler{classifyErr: errors.New("read schema failed")} + + _, err := Reconcile("idx", r, logger) + Expect(err).To(MatchError(ContainSubstring("read schema failed"))) + Expect(r.applyCalls).To(BeZero()) + }) +}) diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index 3ba873b6f6..263311c8da 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -232,10 +232,8 @@ func (r *osReconciler) ApplyAdditive() (bool, error) { return true, nil } -// jsonEqual reports whether two raw JSON values are deeply equal. gjson yields -// an empty string for a path that does not exist; two such unset values are -// equal, an unset value never equals a present one, and a value that fails to -// parse counts as unequal. +// jsonEqual reports whether two raw JSON values are deeply equal. A missing +// gjson path is an empty string, so two unset values compare equal. func jsonEqual(a, b string) bool { if a == "" || b == "" { return a == b From 7ff2643d232e2143cf8b82df87e30088a3fb0fd6 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 19 Aug 2026 10:13:41 +0200 Subject: [PATCH 19/25] refactor(search): share SortedUnionKeys + Classification.AddBreaking - export mapping.SortedUnionKeys and reuse it in bleve.compareKeysExcept instead of a copied union-of-keys block - add Classification.AddBreaking to fold engine-specific breaking reasons and force the verdict, replacing the identical block in the bleve and opensearch Classify paths --- services/search/pkg/bleve/index.go | 16 ++-------------- services/search/pkg/mapping/classify.go | 16 ++++++++++++++-- services/search/pkg/opensearch/index.go | 6 +----- 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index e7431f3b72..22d55772b8 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -4,11 +4,9 @@ import ( "encoding/json" "errors" "fmt" - "maps" "math" "path/filepath" "reflect" - "slices" "strings" "github.com/blevesearch/bleve/v2" @@ -158,24 +156,14 @@ func classifyStoredMapping(index bleve.Index) (searchmapping.Classification, []b var reasons []string compareKeysExcept(stored, code, "default_mapping", "", &reasons) compareKeysExcept(storedDM, codeDM, "properties", "default_mapping.", &reasons) - if len(reasons) > 0 { - classification.Verdict = searchmapping.VerdictBreaking - classification.Reasons = append(reasons, classification.Reasons...) - } + classification.AddBreaking(reasons...) return classification, codeB, nil } // compareKeysExcept deep-compares all keys present on either side except skip. func compareKeysExcept(stored, code map[string]any, skip, prefix string, reasons *[]string) { - keys := slices.Collect(maps.Keys(stored)) - for k := range code { - if _, ok := stored[k]; !ok { - keys = append(keys, k) - } - } - slices.Sort(keys) - for _, k := range keys { + for _, k := range searchmapping.SortedUnionKeys(stored, code) { if k == skip { continue } diff --git a/services/search/pkg/mapping/classify.go b/services/search/pkg/mapping/classify.go index 3d76761fa8..481f4e478a 100644 --- a/services/search/pkg/mapping/classify.go +++ b/services/search/pkg/mapping/classify.go @@ -42,6 +42,17 @@ type Classification struct { Reasons []string } +// AddBreaking records engine-specific breaking reasons (e.g. analyzer drift) +// found outside the properties tree and forces the verdict to breaking. It is a +// no-op when reasons is empty, so callers can pass their findings unconditionally. +func (c *Classification) AddBreaking(reasons ...string) { + if len(reasons) == 0 { + return + } + c.Verdict = VerdictBreaking + c.Reasons = append(reasons, c.Reasons...) +} + // Classify recursively compares a stored `properties` tree against the one from // code (both generic JSON-decoded, not marshaled structs). dataFields reports // whether a code-only field already holds data in the index (bleve dynamic @@ -89,7 +100,7 @@ func classifyNode(stored, code any, dataFields func(string) bool, path string, c return } - for _, k := range sortedUnionKeys(storedMap, codeMap) { + for _, k := range SortedUnionKeys(storedMap, codeMap) { if k == "properties" { continue } @@ -134,7 +145,8 @@ func joinPath(prefix, k string) string { return prefix + "." + k } -func sortedUnionKeys(a, b map[string]any) []string { +// SortedUnionKeys returns the sorted union of the keys of a and b. +func SortedUnionKeys(a, b map[string]any) []string { keys := slices.Collect(maps.Keys(a)) for k := range b { if _, ok := a[k]; !ok { diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index 263311c8da..e79e053aa0 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -203,11 +203,7 @@ func (r *osReconciler) Classify() (searchmapping.Classification, error) { propertiesMap(r.local.Get("mappings.properties").Raw), nil, ) - reasons = append(reasons, classification.Reasons...) - if len(reasons) > 0 { - classification.Verdict = searchmapping.VerdictBreaking - classification.Reasons = reasons - } + classification.AddBreaking(reasons...) return classification, nil } From 414247d6c471bd65443bd228f2d117b6da508281 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 19 Aug 2026 10:40:00 +0200 Subject: [PATCH 20/25] review: use any over interface{}, clone slice in AddBreaking --- services/search/pkg/bleve/index.go | 2 +- services/search/pkg/mapping/classify.go | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index 22d55772b8..fd4d50e0d6 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -25,7 +25,7 @@ import ( // bolt_timeout makes a second process on the same datapath fail after 5s // instead of blocking forever on the file lock. -var openRuntimeConfig = map[string]interface{}{"bolt_timeout": "5s"} +var openRuntimeConfig = map[string]any{"bolt_timeout": "5s"} // NewIndex opens (or creates) the bleve index at root and reconciles its schema // against NewMapping() via searchmapping.Reconcile. diff --git a/services/search/pkg/mapping/classify.go b/services/search/pkg/mapping/classify.go index 481f4e478a..edc64563cf 100644 --- a/services/search/pkg/mapping/classify.go +++ b/services/search/pkg/mapping/classify.go @@ -50,7 +50,8 @@ func (c *Classification) AddBreaking(reasons ...string) { return } c.Verdict = VerdictBreaking - c.Reasons = append(reasons, c.Reasons...) + // clone so we never append into the caller's variadic slice + c.Reasons = append(slices.Clone(reasons), c.Reasons...) } // Classify recursively compares a stored `properties` tree against the one from From 519dd112eae0d908e4338edb09f10d902420d509 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 19 Aug 2026 10:41:45 +0200 Subject: [PATCH 21/25] review: note SchemaVersion bump on bleve marshaling drift in golden test --- services/search/pkg/bleve/index_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/search/pkg/bleve/index_test.go b/services/search/pkg/bleve/index_test.go index de0ecdbb01..9e16a19f53 100644 --- a/services/search/pkg/bleve/index_test.go +++ b/services/search/pkg/bleve/index_test.go @@ -213,7 +213,8 @@ var _ = Describe("NewMapping", func() { }) // A diff here means existing indexes will classify as breaking (schema or - // bleve marshaling changed); update the golden file only deliberately. + // bleve marshaling changed). Update the golden deliberately; on a marshaling + // change, bump search.SchemaVersion too or existing indexes refuse to start. It("matches the committed golden mapping", func() { m, err := bleve.NewMapping() Expect(err).ToNot(HaveOccurred()) From 53fa66f12048b6f1262e36c1d837d2cd976755c1 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Sat, 29 Aug 2026 09:52:51 +0200 Subject: [PATCH 22/25] chore(search): follow the rebased base The opensearch-go bump renamed the mapping-get accessor, the schema check takes a context and a logger now, and the golden bleve mapping carries the word-broken Name and Title. --- .../pkg/bleve/testdata/mapping.golden.json | 281 ++++++++++++++++++ services/search/pkg/opensearch/index_test.go | 4 +- 2 files changed, 283 insertions(+), 2 deletions(-) diff --git a/services/search/pkg/bleve/testdata/mapping.golden.json b/services/search/pkg/bleve/testdata/mapping.golden.json index 8973f666e5..7be606a522 100644 --- a/services/search/pkg/bleve/testdata/mapping.golden.json +++ b/services/search/pkg/bleve/testdata/mapping.golden.json @@ -139,6 +139,18 @@ } ] }, + "Name_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, "ParentID": { "enabled": true, "dynamic": true, @@ -168,6 +180,18 @@ } ] }, + "Path_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, "RootID": { "enabled": true, "dynamic": true, @@ -237,6 +261,30 @@ } ] }, + "Title_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "Title_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, "Type": { "enabled": true, "dynamic": true, @@ -284,6 +332,54 @@ } ] }, + "albumArtist_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "albumArtist_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, + "album_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "album_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, "artist": { "enabled": true, "dynamic": true, @@ -299,6 +395,30 @@ } ] }, + "artist_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "artist_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, "bitrate": { "enabled": true, "dynamic": true, @@ -327,6 +447,30 @@ } ] }, + "composers_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "composers_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, "copyright": { "enabled": true, "dynamic": true, @@ -342,6 +486,30 @@ } ] }, + "copyright_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "copyright_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, "disc": { "enabled": true, "dynamic": true, @@ -396,6 +564,30 @@ } ] }, + "genre_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "genre_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, "hasDrm": { "enabled": true, "dynamic": true, @@ -437,6 +629,30 @@ } ] }, + "title_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "title_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, "track": { "enabled": true, "dynamic": true, @@ -587,6 +803,30 @@ } ] }, + "cameraMake_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "cameraMake_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, "cameraModel": { "enabled": true, "dynamic": true, @@ -602,6 +842,30 @@ } ] }, + "cameraModel_lowercase": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "keyword", + "index": true, + "include_term_vectors": true + } + ] + }, + "cameraModel_words": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "words", + "index": true, + "include_term_vectors": true + } + ] + }, "exposureDenominator": { "enabled": true, "dynamic": true, @@ -706,6 +970,13 @@ "index_dynamic": true, "docvalues_dynamic": true, "analysis": { + "char_filters": { + "dot_to_space": { + "regexp": "\\.", + "replace": " ", + "type": "regexp" + } + }, "analyzers": { "fulltext": { "token_filters": [ @@ -714,6 +985,16 @@ ], "tokenizer": "unicode", "type": "custom" + }, + "words": { + "char_filters": [ + "dot_to_space" + ], + "token_filters": [ + "to_lower" + ], + "tokenizer": "unicode", + "type": "custom" } } } diff --git a/services/search/pkg/opensearch/index_test.go b/services/search/pkg/opensearch/index_test.go index 48885f1a3e..332b1e36c5 100644 --- a/services/search/pkg/opensearch/index_test.go +++ b/services/search/pkg/opensearch/index_test.go @@ -134,7 +134,7 @@ func TestIndexManager(t *testing.T) { resp, err := tc.Client().Indices.Mapping.Get(t.Context(), &opensearchgoAPI.MappingGetReq{Indices: []string{indexName}}) require.NoError(t, err) - require.True(t, gjson.GetBytes(resp.Indices[indexName].Mappings, "properties.Title").Exists()) + require.True(t, gjson.GetBytes(resp.GetIndices()[indexName].Mappings, "properties.Title").Exists()) }) t.Run("adds a new nested field to an existing index in place", func(t *testing.T) { @@ -152,7 +152,7 @@ func TestIndexManager(t *testing.T) { resp, err := tc.Client().Indices.Mapping.Get(t.Context(), &opensearchgoAPI.MappingGetReq{Indices: []string{indexName}}) require.NoError(t, err) - require.True(t, gjson.GetBytes(resp.Indices[indexName].Mappings, "properties.photo.properties.cameraMake").Exists()) + require.True(t, gjson.GetBytes(resp.GetIndices()[indexName].Mappings, "properties.photo.properties.cameraMake").Exists()) }) t.Run("fails when an existing field changed its definition", func(t *testing.T) { From 7493f189a045f486cd9de373a351ab556cad1ed3 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Mon, 31 Aug 2026 14:34:13 +0200 Subject: [PATCH 23/25] chore(search): adapt tests and golden to the v4 base The refuse specs use registered analyzers (fulltext is gone), the golden regenerates via UPDATE_GOLDEN, MappingGetResp grew an accessor, and the parity suite passes the new NewBackend signature. --- services/search/pkg/bleve/index_test.go | 18 ++++++---- .../pkg/bleve/testdata/mapping.golden.json | 34 +------------------ services/search/pkg/parity/engines_test.go | 2 +- 3 files changed, 14 insertions(+), 40 deletions(-) diff --git a/services/search/pkg/bleve/index_test.go b/services/search/pkg/bleve/index_test.go index 9e16a19f53..f7650212fa 100644 --- a/services/search/pkg/bleve/index_test.go +++ b/services/search/pkg/bleve/index_test.go @@ -25,7 +25,7 @@ var _ = Describe("Index", func() { It("puts the index into a directory of its own generation", func() { root := GinkgoT().TempDir() - index, _, err := bleve.NewIndex(root) + index, _, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).ToNot(HaveOccurred()) DeferCleanup(index.Close) @@ -36,11 +36,11 @@ var _ = Describe("Index", func() { It("opens the index that is already there", func() { root := GinkgoT().TempDir() - index, _, err := bleve.NewIndex(root) + index, _, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).ToNot(HaveOccurred()) Expect(index.Close()).To(Succeed()) - reopened, _, err := bleve.NewIndex(root) + reopened, _, err := bleve.NewIndex(root, log.NopLogger()) Expect(err).ToNot(HaveOccurred()) DeferCleanup(reopened.Close) @@ -163,7 +163,7 @@ var _ = Describe("NewIndex", func() { name := old.DefaultMapping.Properties["Name"] Expect(name).ToNot(BeNil()) Expect(name.Fields).ToNot(BeEmpty()) - name.Fields[0].Analyzer = "fulltext" + name.Fields[0].Analyzer = "standard" buildIndex(old, nil) _, _, err := bleve.NewIndex(root, log.NopLogger()) @@ -190,8 +190,8 @@ var _ = Describe("NewIndex", func() { It("refuses on a changed analyzer definition", func() { old := codeMapping() - Expect(old.CustomAnalysis.Analyzers).To(HaveKey("fulltext")) - old.CustomAnalysis.Analyzers["fulltext"] = map[string]any{ + Expect(old.CustomAnalysis.Analyzers).To(HaveKey(searchmapping.WordsAnalyzer)) + old.CustomAnalysis.Analyzers[searchmapping.WordsAnalyzer] = map[string]any{ "type": custom.Name, "tokenizer": unicode.Name, "token_filters": []string{lowercase.Name}, @@ -223,6 +223,12 @@ var _ = Describe("NewMapping", func() { var got, golden map[string]any Expect(json.Unmarshal(b, &got)).To(Succeed()) + if os.Getenv("UPDATE_GOLDEN") != "" { + pretty, err := json.MarshalIndent(m, "", " ") + Expect(err).ToNot(HaveOccurred()) + Expect(os.WriteFile("testdata/mapping.golden.json", append(pretty, '\n'), 0o644)).To(Succeed()) + } + goldenB, err := os.ReadFile("testdata/mapping.golden.json") Expect(err).ToNot(HaveOccurred()) Expect(json.Unmarshal(goldenB, &golden)).To(Succeed()) diff --git a/services/search/pkg/bleve/testdata/mapping.golden.json b/services/search/pkg/bleve/testdata/mapping.golden.json index 7be606a522..09f274b7f0 100644 --- a/services/search/pkg/bleve/testdata/mapping.golden.json +++ b/services/search/pkg/bleve/testdata/mapping.golden.json @@ -9,7 +9,7 @@ "fields": [ { "type": "text", - "analyzer": "fulltext", + "analyzer": "words", "store": true, "index": true, "include_term_vectors": true, @@ -44,18 +44,6 @@ } ] }, - "Favorites_lowercase": { - "enabled": true, - "dynamic": true, - "fields": [ - { - "type": "text", - "analyzer": "keyword", - "index": true, - "include_term_vectors": true - } - ] - }, "Hidden": { "enabled": true, "dynamic": true, @@ -180,18 +168,6 @@ } ] }, - "Path_words": { - "enabled": true, - "dynamic": true, - "fields": [ - { - "type": "text", - "analyzer": "words", - "index": true, - "include_term_vectors": true - } - ] - }, "RootID": { "enabled": true, "dynamic": true, @@ -978,14 +954,6 @@ } }, "analyzers": { - "fulltext": { - "token_filters": [ - "to_lower", - "stemmer_porter" - ], - "tokenizer": "unicode", - "type": "custom" - }, "words": { "char_filters": [ "dot_to_space" diff --git a/services/search/pkg/parity/engines_test.go b/services/search/pkg/parity/engines_test.go index 7568dc7409..b49e0a928b 100644 --- a/services/search/pkg/parity/engines_test.go +++ b/services/search/pkg/parity/engines_test.go @@ -98,7 +98,7 @@ func newOpenSearch(name string, fixtures []search.Resource) testEngine { return testEngine{name: "opensearch", unavailable: err.Error()} } - backend, err := opensearch.NewBackend(name, tc.Client()) + backend, err := opensearch.NewBackend(context.Background(), name, tc.Client(), log.NopLogger()) if err != nil { return testEngine{name: "opensearch", unavailable: err.Error()} } From e11ce95d95f492da558c4d17d27945b8782389f7 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Mon, 31 Aug 2026 15:08:44 +0200 Subject: [PATCH 24/25] test(search): golden for the generated OpenSearch index definition Pins the shipped schema as a reviewable diff; regenerate with UPDATE_GOLDEN=1. --- services/search/pkg/opensearch/index_test.go | 25 ++ .../opensearch/testdata/resource.golden.json | 291 ++++++++++++++++++ 2 files changed, 316 insertions(+) create mode 100644 services/search/pkg/opensearch/testdata/resource.golden.json diff --git a/services/search/pkg/opensearch/index_test.go b/services/search/pkg/opensearch/index_test.go index 332b1e36c5..5b1ff3aaa1 100644 --- a/services/search/pkg/opensearch/index_test.go +++ b/services/search/pkg/opensearch/index_test.go @@ -1,7 +1,10 @@ package opensearch_test import ( + "bytes" + "encoding/json" "fmt" + "os" "strings" "testing" @@ -30,6 +33,28 @@ func TestVersionedIndexName(t *testing.T) { ) } +// A diff here means the generated OpenSearch index definition changed: new +// indexes get the new shape, existing ones answer to the startup classifier. +// Update the golden deliberately (UPDATE_GOLDEN=1); a breaking change needs a +// search.SchemaVersion bump. +func TestGoldenMapping(t *testing.T) { + var pretty bytes.Buffer + require.NoError(t, json.Indent(&pretty, []byte(opensearch.IndexManagerLatest.String()), "", " ")) + pretty.WriteByte('\n') + + if os.Getenv("UPDATE_GOLDEN") != "" { + require.NoError(t, os.WriteFile("testdata/resource.golden.json", pretty.Bytes(), 0o644)) + } + + goldenB, err := os.ReadFile("testdata/resource.golden.json") + require.NoError(t, err) + + var got, golden map[string]any + require.NoError(t, json.Unmarshal(pretty.Bytes(), &got)) + require.NoError(t, json.Unmarshal(goldenB, &golden)) + require.Equal(t, golden, got) +} + func TestIndexManager(t *testing.T) { t.Run("index plausibility", func(t *testing.T) { tests := []opensearchtest.TableTest[opensearch.IndexManager, struct{}]{ diff --git a/services/search/pkg/opensearch/testdata/resource.golden.json b/services/search/pkg/opensearch/testdata/resource.golden.json new file mode 100644 index 0000000000..7486322034 --- /dev/null +++ b/services/search/pkg/opensearch/testdata/resource.golden.json @@ -0,0 +1,291 @@ +{ + "mappings": { + "properties": { + "Content": { + "analyzer": "words", + "term_vector": "with_positions_offsets", + "type": "text" + }, + "Deleted": { + "type": "boolean" + }, + "Favorites": { + "type": "keyword" + }, + "Hidden": { + "type": "boolean" + }, + "ID": { + "type": "keyword" + }, + "MimeType": { + "doc_values": false, + "type": "wildcard" + }, + "Mtime": { + "type": "date" + }, + "Name": { + "type": "keyword" + }, + "Name_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "Name_words": { + "analyzer": "words", + "type": "text" + }, + "ParentID": { + "type": "keyword" + }, + "Path": { + "analyzer": "path_hierarchy", + "type": "text" + }, + "RootID": { + "type": "keyword" + }, + "Size": { + "type": "long" + }, + "Tags": { + "type": "keyword" + }, + "Tags_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "Title": { + "type": "keyword" + }, + "Title_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "Title_words": { + "analyzer": "words", + "type": "text" + }, + "Type": { + "type": "long" + }, + "audio": { + "properties": { + "album": { + "type": "keyword" + }, + "albumArtist": { + "type": "keyword" + }, + "albumArtist_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "albumArtist_words": { + "analyzer": "words", + "type": "text" + }, + "album_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "album_words": { + "analyzer": "words", + "type": "text" + }, + "artist": { + "type": "keyword" + }, + "artist_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "artist_words": { + "analyzer": "words", + "type": "text" + }, + "bitrate": { + "type": "long" + }, + "composers": { + "type": "keyword" + }, + "composers_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "composers_words": { + "analyzer": "words", + "type": "text" + }, + "copyright": { + "type": "keyword" + }, + "copyright_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "copyright_words": { + "analyzer": "words", + "type": "text" + }, + "disc": { + "type": "integer" + }, + "discCount": { + "type": "integer" + }, + "duration": { + "type": "long" + }, + "genre": { + "type": "keyword" + }, + "genre_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "genre_words": { + "analyzer": "words", + "type": "text" + }, + "hasDrm": { + "type": "boolean" + }, + "isVariableBitrate": { + "type": "boolean" + }, + "title": { + "type": "keyword" + }, + "title_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "title_words": { + "analyzer": "words", + "type": "text" + }, + "track": { + "type": "integer" + }, + "trackCount": { + "type": "integer" + }, + "year": { + "type": "integer" + } + } + }, + "image": { + "properties": { + "height": { + "type": "integer" + }, + "width": { + "type": "integer" + } + } + }, + "location": { + "properties": { + "altitude": { + "type": "double" + }, + "latitude": { + "type": "double" + }, + "longitude": { + "type": "double" + } + } + }, + "location_geopoint": { + "type": "geo_point" + }, + "photo": { + "properties": { + "cameraMake": { + "type": "keyword" + }, + "cameraMake_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "cameraMake_words": { + "analyzer": "words", + "type": "text" + }, + "cameraModel": { + "type": "keyword" + }, + "cameraModel_lowercase": { + "doc_values": false, + "type": "keyword" + }, + "cameraModel_words": { + "analyzer": "words", + "type": "text" + }, + "exposureDenominator": { + "type": "double" + }, + "exposureNumerator": { + "type": "double" + }, + "fNumber": { + "type": "double" + }, + "focalLength": { + "type": "double" + }, + "iso": { + "type": "integer" + }, + "orientation": { + "type": "integer" + }, + "takenDateTime": { + "type": "date" + } + } + } + } + }, + "settings": { + "analysis": { + "analyzer": { + "path_hierarchy": { + "tokenizer": "path_hierarchy", + "type": "custom" + }, + "words": { + "char_filter": [ + "dot_to_space" + ], + "filter": [ + "lowercase" + ], + "tokenizer": "standard", + "type": "custom" + } + }, + "char_filter": { + "dot_to_space": { + "mappings": [ + ". =\u003e \\u0020" + ], + "type": "mapping" + } + }, + "tokenizer": { + "path_hierarchy": { + "type": "path_hierarchy" + } + } + }, + "number_of_replicas": "1", + "number_of_shards": "1" + } +} From 0e2e2876f482b3af2f4c5f15a6e9a66ccd773709 Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Mon, 31 Aug 2026 15:14:39 +0200 Subject: [PATCH 25/25] test(search): a golden diff says whether it needs a SchemaVersion bump The failure runs the classifier on golden vs generated: additive means regenerate only, breaking means bump too. --- services/search/pkg/bleve/index_test.go | 26 ++++++++++++++++++- services/search/pkg/opensearch/index_test.go | 27 +++++++++++++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/services/search/pkg/bleve/index_test.go b/services/search/pkg/bleve/index_test.go index f7650212fa..e6e4499347 100644 --- a/services/search/pkg/bleve/index_test.go +++ b/services/search/pkg/bleve/index_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "reflect" bleveSearch "github.com/blevesearch/bleve/v2" "github.com/blevesearch/bleve/v2/analysis/analyzer/custom" @@ -233,6 +234,29 @@ var _ = Describe("NewMapping", func() { Expect(err).ToNot(HaveOccurred()) Expect(json.Unmarshal(goldenB, &golden)).To(Succeed()) - Expect(got).To(Equal(golden)) + Expect(got).To(Equal(golden), goldenAdvice(golden, got)) }) }) + +// goldenAdvice classifies a golden diff so the failure says whether the change +// is additive (regenerate with UPDATE_GOLDEN=1) or breaking (bump +// search.SchemaVersion too). +func goldenAdvice(golden, got map[string]any) string { + dig := func(m map[string]any, path ...string) map[string]any { + for _, k := range path { + m, _ = m[k].(map[string]any) + } + return m + } + c := searchmapping.Classify(dig(golden, "default_mapping", "properties"), dig(got, "default_mapping", "properties"), nil) + if !reflect.DeepEqual(dig(golden, "analysis"), dig(got, "analysis")) { + c.AddBreaking("the analysis definitions changed") + } + switch c.Verdict { + case searchmapping.VerdictAdditive: + return fmt.Sprintf("additive schema change (new fields: %v): regenerate the golden with UPDATE_GOLDEN=1, no SchemaVersion bump needed", c.NewFields) + case searchmapping.VerdictBreaking: + return fmt.Sprintf("breaking schema change (%v): regenerate the golden with UPDATE_GOLDEN=1 and bump search.SchemaVersion", c.Reasons) + } + return "the mapping changed outside the classified tree (bleve marshaling drift?): regenerate the golden with UPDATE_GOLDEN=1 and see the SchemaVersion note above" +} diff --git a/services/search/pkg/opensearch/index_test.go b/services/search/pkg/opensearch/index_test.go index 5b1ff3aaa1..3dc8b02fec 100644 --- a/services/search/pkg/opensearch/index_test.go +++ b/services/search/pkg/opensearch/index_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "os" + "reflect" "strings" "testing" @@ -16,6 +17,7 @@ import ( "github.com/opencloud-eu/opencloud/pkg/log" "github.com/opencloud-eu/opencloud/services/search/internal/opensearchtest" + searchmapping "github.com/opencloud-eu/opencloud/services/search/pkg/mapping" "github.com/opencloud-eu/opencloud/services/search/pkg/opensearch" "github.com/opencloud-eu/opencloud/services/search/pkg/search" ) @@ -52,7 +54,30 @@ func TestGoldenMapping(t *testing.T) { var got, golden map[string]any require.NoError(t, json.Unmarshal(pretty.Bytes(), &got)) require.NoError(t, json.Unmarshal(goldenB, &golden)) - require.Equal(t, golden, got) + require.Equal(t, golden, got, goldenAdvice(golden, got, "mappings.properties", "settings.analysis")) +} + +// goldenAdvice classifies a golden diff so the failure says whether the change +// is additive (regenerate with UPDATE_GOLDEN=1) or breaking (bump +// search.SchemaVersion too). +func goldenAdvice(golden, got map[string]any, propsPath, analysisPath string) string { + dig := func(m map[string]any, path string) map[string]any { + for _, k := range strings.Split(path, ".") { + m, _ = m[k].(map[string]any) + } + return m + } + c := searchmapping.Classify(dig(golden, propsPath), dig(got, propsPath), nil) + if !reflect.DeepEqual(dig(golden, analysisPath), dig(got, analysisPath)) { + c.AddBreaking("the analysis settings changed") + } + switch c.Verdict { + case searchmapping.VerdictAdditive: + return fmt.Sprintf("additive schema change (new fields: %v): regenerate the golden with UPDATE_GOLDEN=1, no SchemaVersion bump needed", c.NewFields) + case searchmapping.VerdictBreaking: + return fmt.Sprintf("breaking schema change (%v): regenerate the golden with UPDATE_GOLDEN=1 and bump search.SchemaVersion", c.Reasons) + } + return "the schema changed outside the classified tree: regenerate the golden with UPDATE_GOLDEN=1" } func TestIndexManager(t *testing.T) {