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
This commit is contained in:
Dominik Schmidt
2026-07-09 01:27:26 +02:00
parent 63e38adb9a
commit f14455550a
6 changed files with 750 additions and 21 deletions

View File

@@ -67,16 +67,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
}
}

View File

@@ -1,7 +1,9 @@
package bleve_test
import (
"encoding/json"
"fmt"
"os"
"path/filepath"
bleveSearch "github.com/blevesearch/bleve/v2"
@@ -179,4 +181,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))
})
})

View File

@@ -0,0 +1,677 @@
{
"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": "lowercaseKeyword",
"store": true,
"index": true,
"include_term_vectors": true,
"docvalues": 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",
"store": true,
"index": true,
"include_term_vectors": true,
"include_in_all": true,
"docvalues": true
}
]
},
"MimeType": {
"enabled": true,
"dynamic": true,
"fields": [
{
"type": "text",
"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": "lowercaseKeyword",
"store": true,
"index": true,
"include_term_vectors": true,
"include_in_all": true,
"docvalues": true
}
]
},
"ParentID": {
"enabled": true,
"dynamic": true,
"fields": [
{
"type": "text",
"store": true,
"index": true,
"include_term_vectors": true,
"include_in_all": true,
"docvalues": true
}
]
},
"Path": {
"enabled": true,
"dynamic": true,
"fields": [
{
"type": "text",
"store": true,
"index": true,
"include_term_vectors": true,
"include_in_all": true,
"docvalues": true
}
]
},
"RootID": {
"enabled": true,
"dynamic": true,
"fields": [
{
"type": "text",
"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": "lowercaseKeyword",
"store": true,
"index": true,
"include_term_vectors": true,
"docvalues": true
}
]
},
"Title": {
"enabled": true,
"dynamic": true,
"fields": [
{
"type": "text",
"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",
"store": true,
"index": true,
"include_term_vectors": true,
"include_in_all": true,
"docvalues": true
}
]
},
"albumArtist": {
"enabled": true,
"dynamic": true,
"fields": [
{
"type": "text",
"store": true,
"index": true,
"include_term_vectors": true,
"include_in_all": true,
"docvalues": true
}
]
},
"artist": {
"enabled": true,
"dynamic": true,
"fields": [
{
"type": "text",
"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",
"store": true,
"index": true,
"include_term_vectors": true,
"include_in_all": true,
"docvalues": true
}
]
},
"copyright": {
"enabled": true,
"dynamic": true,
"fields": [
{
"type": "text",
"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",
"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",
"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",
"store": true,
"index": true,
"include_term_vectors": true,
"include_in_all": true,
"docvalues": true
}
]
},
"cameraModel": {
"enabled": true,
"dynamic": true,
"fields": [
{
"type": "text",
"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"
},
"lowercaseKeyword": {
"token_filters": [
"to_lower"
],
"tokenizer": "single",
"type": "custom"
}
}
}
}

View File

@@ -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() {

View File

@@ -113,22 +113,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},
})
@@ -150,6 +162,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) {
@@ -188,7 +203,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
}

View File

@@ -80,6 +80,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"