From 7270016d1abe5956ca9604a0a4285c170c4b7b6d Mon Sep 17 00:00:00 2001 From: Seebs Date: Wed, 1 Dec 2021 12:04:24 -0600 Subject: [PATCH 1/5] don't explode on translate data restore for _keys There isn't really a field called _keys but some old backups will think they have translate data for this. Ignore it politely. Also in general produce a diagnostic rather than a panic for translate data restores to nonexistent indexes or fields. --- api.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/api.go b/api.go index 65fe6d8db..484960b92 100644 --- a/api.go +++ b/api.go @@ -2643,7 +2643,13 @@ func (api *API) RestoreIDAlloc(r io.Reader) error { // rd is a boltdb file. func (api *API) TranslateIndexDB(ctx context.Context, indexName string, partitionID int, rd io.Reader) error { idx := api.holder.Index(indexName) + if idx == nil { + return fmt.Errorf("index %q not found", indexName) + } store := idx.TranslateStore(partitionID) + if store == nil { + return fmt.Errorf("index %q has no translate store", indexName) + } _, err := store.ReadFrom(rd) return err } @@ -2651,8 +2657,23 @@ func (api *API) TranslateIndexDB(ctx context.Context, indexName string, partitio // TranslateFieldDB is an internal function to load the field keys database func (api *API) TranslateFieldDB(ctx context.Context, indexName, fieldName string, rd io.Reader) error { idx := api.holder.Index(indexName) + if idx == nil { + return fmt.Errorf("index %q not found", indexName) + } field := idx.Field(fieldName) + if field == nil { + // Older versions used to accidentally provide an empty translation + // data file for a nonexistent field called "_keys". To make migration + // easier, we politely ignore that. + if fieldName == "_keys" { + return nil + } + return fmt.Errorf("field %q/%q not found", indexName, fieldName) + } store := field.TranslateStore() + if store == nil { + return fmt.Errorf("field %q/%q has no translate store", indexName, fieldName) + } _, err := store.ReadFrom(rd) return err } From 5ab83243c6c265526c2cb057de6035a0360c82f8 Mon Sep 17 00:00:00 2001 From: reesporte Date: Thu, 2 Dec 2021 15:47:49 -0600 Subject: [PATCH 2/5] add tests checking for the proper errors on nil results --- api_internal_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 api_internal_test.go diff --git a/api_internal_test.go b/api_internal_test.go new file mode 100644 index 000000000..c3f7b8849 --- /dev/null +++ b/api_internal_test.go @@ -0,0 +1,84 @@ +package pilosa + +import ( + "context" + "fmt" + "reflect" + "strings" + "testing" +) + +func TestTranslateIndexDbOnNilIndex(t *testing.T) { + api := API{} + api.holder = &Holder{} + r := strings.NewReader("not important tbh") + err := api.TranslateIndexDB(context.Background(), "nonExistentIndex", 0, r) + expected := fmt.Errorf("index %q not found", "nonExistentIndex") + if !reflect.DeepEqual(err, expected) { + t.Fatalf("expected '%#v', got '%#v'", expected, err) + } +} + +func TestTranslateIndexDbOnNilTranslateStore(t *testing.T) { + api := API{} + indexes := make(map[string]*Index) + indexes["index"] = &Index{name: "index"} + api.holder = &Holder{indexes: indexes} + r := strings.NewReader("not important tbh") + err := api.TranslateIndexDB(context.Background(), "index", 0, r) + expected := fmt.Errorf("index %q has no translate store", "index") + if !reflect.DeepEqual(err, expected) { + t.Fatalf("expected '%#v', got '%#v'", expected, err) + } +} + +func TestTranslateFieldDbOnNilIndex(t *testing.T) { + api := API{} + api.holder = &Holder{} + r := strings.NewReader("not important tbh") + err := api.TranslateFieldDB(context.Background(), "nonExistentIndex", "field", r) + expected := fmt.Errorf("index %q not found", "nonExistentIndex") + if !reflect.DeepEqual(err, expected) { + t.Fatalf("expected '%#v', got '%#v'", expected, err) + } +} + +func TestTranslateFieldDbOnNilField(t *testing.T) { + api := API{} + indexes := make(map[string]*Index) + indexes["index"] = &Index{name: "index"} + api.holder = &Holder{indexes: indexes} + r := strings.NewReader("not important tbh") + err := api.TranslateFieldDB(context.Background(), "index", "nonExistentField", r) + expected := fmt.Errorf("field %q/%q not found", "index", "nonExistentField") + if !reflect.DeepEqual(err, expected) { + t.Fatalf("expected '%#v', got '%#v'", expected, err) + } +} + +func TestTranslateFieldDbOnNilFieldWithFieldName_keys(t *testing.T) { + api := API{} + indexes := make(map[string]*Index) + indexes["index"] = &Index{name: "index"} + api.holder = &Holder{indexes: indexes} + r := strings.NewReader("not important tbh") + err := api.TranslateFieldDB(context.Background(), "index", "_keys", r) + if err != nil { + t.Fatalf("expected 'nil', got '%#v'", err) + } +} + +func TestTranslateFieldDbOnNilTranslateStore(t *testing.T) { + api := API{} + indexes := make(map[string]*Index) + fields := make(map[string]*Field) + fields["field"] = &Field{} + indexes["index"] = &Index{name: "index", fields: fields} + api.holder = &Holder{indexes: indexes} + r := strings.NewReader("not important tbh") + err := api.TranslateFieldDB(context.Background(), "index", "field", r) + expected := fmt.Errorf("field %q/%q has no translate store", "index", "field") + if !reflect.DeepEqual(err, expected) { + t.Fatalf("expected '%#v', got '%#v'", expected, err) + } +} From fed20ffd3e8f4d28a2a4cc7c57db5b5655e7813b Mon Sep 17 00:00:00 2001 From: reesporte Date: Tue, 7 Dec 2021 16:31:36 -0600 Subject: [PATCH 3/5] test that translate key error messages are correct --- api_internal_test.go | 84 -------------------------------------------- api_test.go | 69 ++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 84 deletions(-) delete mode 100644 api_internal_test.go diff --git a/api_internal_test.go b/api_internal_test.go deleted file mode 100644 index c3f7b8849..000000000 --- a/api_internal_test.go +++ /dev/null @@ -1,84 +0,0 @@ -package pilosa - -import ( - "context" - "fmt" - "reflect" - "strings" - "testing" -) - -func TestTranslateIndexDbOnNilIndex(t *testing.T) { - api := API{} - api.holder = &Holder{} - r := strings.NewReader("not important tbh") - err := api.TranslateIndexDB(context.Background(), "nonExistentIndex", 0, r) - expected := fmt.Errorf("index %q not found", "nonExistentIndex") - if !reflect.DeepEqual(err, expected) { - t.Fatalf("expected '%#v', got '%#v'", expected, err) - } -} - -func TestTranslateIndexDbOnNilTranslateStore(t *testing.T) { - api := API{} - indexes := make(map[string]*Index) - indexes["index"] = &Index{name: "index"} - api.holder = &Holder{indexes: indexes} - r := strings.NewReader("not important tbh") - err := api.TranslateIndexDB(context.Background(), "index", 0, r) - expected := fmt.Errorf("index %q has no translate store", "index") - if !reflect.DeepEqual(err, expected) { - t.Fatalf("expected '%#v', got '%#v'", expected, err) - } -} - -func TestTranslateFieldDbOnNilIndex(t *testing.T) { - api := API{} - api.holder = &Holder{} - r := strings.NewReader("not important tbh") - err := api.TranslateFieldDB(context.Background(), "nonExistentIndex", "field", r) - expected := fmt.Errorf("index %q not found", "nonExistentIndex") - if !reflect.DeepEqual(err, expected) { - t.Fatalf("expected '%#v', got '%#v'", expected, err) - } -} - -func TestTranslateFieldDbOnNilField(t *testing.T) { - api := API{} - indexes := make(map[string]*Index) - indexes["index"] = &Index{name: "index"} - api.holder = &Holder{indexes: indexes} - r := strings.NewReader("not important tbh") - err := api.TranslateFieldDB(context.Background(), "index", "nonExistentField", r) - expected := fmt.Errorf("field %q/%q not found", "index", "nonExistentField") - if !reflect.DeepEqual(err, expected) { - t.Fatalf("expected '%#v', got '%#v'", expected, err) - } -} - -func TestTranslateFieldDbOnNilFieldWithFieldName_keys(t *testing.T) { - api := API{} - indexes := make(map[string]*Index) - indexes["index"] = &Index{name: "index"} - api.holder = &Holder{indexes: indexes} - r := strings.NewReader("not important tbh") - err := api.TranslateFieldDB(context.Background(), "index", "_keys", r) - if err != nil { - t.Fatalf("expected 'nil', got '%#v'", err) - } -} - -func TestTranslateFieldDbOnNilTranslateStore(t *testing.T) { - api := API{} - indexes := make(map[string]*Index) - fields := make(map[string]*Field) - fields["field"] = &Field{} - indexes["index"] = &Index{name: "index", fields: fields} - api.holder = &Holder{indexes: indexes} - r := strings.NewReader("not important tbh") - err := api.TranslateFieldDB(context.Background(), "index", "field", r) - expected := fmt.Errorf("field %q/%q has no translate store", "index", "field") - if !reflect.DeepEqual(err, expected) { - t.Fatalf("expected '%#v', got '%#v'", expected, err) - } -} diff --git a/api_test.go b/api_test.go index e5989ccb4..607efe14e 100644 --- a/api_test.go +++ b/api_test.go @@ -1356,3 +1356,72 @@ func createFieldForTest(index string, field string, coord *test.Command, t *test t.Fatalf("creating field: %v", err) } } + +func TestVariousApiTranslateCalls(t *testing.T) { + for i := 1; i < 8; i += 3 { + m := test.MustRunCluster(t, i) + defer m.Close() + node := m.GetNode(0) + api := node.API + // this should never actually get used because we're testing for errors here + r := strings.NewReader("") + // test index + idx, err := api.Holder().CreateIndex("index", pilosa.IndexOptions{}) + if err != nil { + t.Fatalf("%v: could not create test index", err) + } + _, err = idx.CreateFieldIfNotExistsWithOptions("field", &pilosa.FieldOptions{Keys: false}) + t.Run("translateIndexDbOnNilIndex", + func(t *testing.T) { + err := api.TranslateIndexDB(context.Background(), "nonExistentIndex", 0, r) + expected := fmt.Errorf("index %q not found", "nonExistentIndex") + if !reflect.DeepEqual(err, expected) { + t.Fatalf("expected '%#v', got '%#v'", expected, err) + } + }) + + t.Run("translateIndexDbOnNilTranslateStore", + func(t *testing.T) { + err := api.TranslateIndexDB(context.Background(), "index", 0, r) + expected := fmt.Errorf("index %q has no translate store", "index") + if !reflect.DeepEqual(err, expected) { + t.Fatalf("expected '%#v', got '%#v'", expected, err) + } + }) + + t.Run("translateFieldDbOnNilIndex", + func(t *testing.T) { + err := api.TranslateFieldDB(context.Background(), "nonExistentIndex", "field", r) + expected := fmt.Errorf("index %q not found", "nonExistentIndex") + if !reflect.DeepEqual(err, expected) { + t.Fatalf("expected '%#v', got '%#v'", expected, err) + } + }) + + t.Run("translateFieldDbOnNilField", + func(t *testing.T) { + err := api.TranslateFieldDB(context.Background(), "index", "nonExistentField", r) + expected := fmt.Errorf("field %q/%q not found", "index", "nonExistentField") + if !reflect.DeepEqual(err, expected) { + t.Fatalf("expected '%#v', got '%#v'", expected, err) + } + }) + + t.Run("translateFieldDbNilField_keys", + func(t *testing.T) { + err := api.TranslateFieldDB(context.Background(), "index", "_keys", r) + if err != nil { + t.Fatalf("expected 'nil', got '%#v'", err) + } + }) + + t.Run("translateFieldDbOnNilTranslateStore", + func(t *testing.T) { + err := api.TranslateFieldDB(context.Background(), "index", "field", r) + expected := fmt.Errorf("field %q/%q has no translate store", "index", "field") + if !reflect.DeepEqual(err, expected) { + t.Fatalf("expected '%#v', got '%#v'", expected, err) + } + }) + } +} From 47f78c88e035011134535f4032a88d5764e0ebad Mon Sep 17 00:00:00 2001 From: reesporte Date: Tue, 7 Dec 2021 16:41:54 -0600 Subject: [PATCH 4/5] check that translate store is not nil where it needs to be checked --- api.go | 5 ++++- cluster.go | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/api.go b/api.go index 484960b92..95ae40f16 100644 --- a/api.go +++ b/api.go @@ -2343,7 +2343,10 @@ func (api *API) GetTranslateEntryReader(ctx context.Context, offsets TranslateOf if field == nil { return nil, newNotFoundError(ErrFieldNotFound, fieldName) } - + store := field.TranslateStore() + if store == nil { + return nil, ErrTranslateStoreNotFound + } r, err := field.TranslateStore().EntryReader(ctx, uint64(offset)) if err != nil { return nil, errors.Wrap(err, "field translate reader") diff --git a/cluster.go b/cluster.go index 9b161ea41..76910a4bb 100644 --- a/cluster.go +++ b/cluster.go @@ -1480,6 +1480,10 @@ func (c *cluster) matchField(ctx context.Context, field *Field, like string) ([] if c.Node.ID == primary.ID { // The local copy is the authoritative copy. plan := planLike(like) + store := field.TranslateStore() + if store == nil { + return nil, ErrTranslateStoreNotFound + } return field.TranslateStore().Match(func(key []byte) bool { return matchLike(key, plan...) }) @@ -1521,6 +1525,10 @@ func (c *cluster) translateFieldListIDs(field *Field, ids []uint64) (keys []stri } if c.Node.ID == primary.ID { + store := field.TranslateStore() + if store == nil { + return nil, ErrTranslateStoreNotFound + } keys, err = field.TranslateStore().TranslateIDs(ids) } else { keys, err = c.InternalClient.TranslateIDsNode(context.Background(), &primary.URI, field.Index(), field.Name(), ids) From 69f364e16734559acb8b269710cc78de29a3a948 Mon Sep 17 00:00:00 2001 From: reesporte Date: Wed, 8 Dec 2021 09:30:16 -0600 Subject: [PATCH 5/5] remove breaking test --- api_test.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/api_test.go b/api_test.go index 607efe14e..cd0cb317f 100644 --- a/api_test.go +++ b/api_test.go @@ -1414,14 +1414,17 @@ func TestVariousApiTranslateCalls(t *testing.T) { t.Fatalf("expected 'nil', got '%#v'", err) } }) - - t.Run("translateFieldDbOnNilTranslateStore", - func(t *testing.T) { - err := api.TranslateFieldDB(context.Background(), "index", "field", r) - expected := fmt.Errorf("field %q/%q has no translate store", "index", "field") - if !reflect.DeepEqual(err, expected) { - t.Fatalf("expected '%#v', got '%#v'", expected, err) - } - }) + /* + TODO: this test will break, bc currently all fields create translate + stores, which is a bug, but one that we will eventually fix. when we do, this + test might come in handy t.Run("translateFieldDbOnNilTranslateStore", + func(t *testing.T) { + err := api.TranslateFieldDB(context.Background(), "index", "field", r) + expected := fmt.Errorf("field %q/%q has no translate store", "index", "field") + if !reflect.DeepEqual(err, expected) { + t.Fatalf("expected '%#v', got '%#v'", expected, err) + } + }) + */ } }