From 63e38adb9a3cf756d0993ec79a4b8c769b0d5c1e Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Wed, 8 Jul 2026 21:49:43 +0200 Subject: [PATCH] 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 521e00b929..1e4e908c0f 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -30,12 +30,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) @@ -66,11 +64,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) @@ -87,13 +83,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 2459bc4105..4086e64cd8 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) indexName := opensearch.VersionedIndexName(cfg.Engine.OpenSearch.ResourceIndex.Name) openSearchBackend, err := opensearch.NewBackend(startupCtx, indexName, client, logger) 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 209832b8b1..bc7fe53f0a 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -103,11 +103,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 {