From 6f421dad6920a9bd140233de3c07385ae8e5d686 Mon Sep 17 00:00:00 2001 From: Nia Weiss Date: Wed, 17 Mar 2021 11:57:06 -0400 Subject: [PATCH] handle translation errors when using keys against an unkeyed index This also adds tests for our error outputs. --- cluster.go | 10 +++++--- executor_test.go | 17 ++---------- translator_test.go | 64 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 19 deletions(-) diff --git a/cluster.go b/cluster.go index 94bcd7776..ca95cbe67 100644 --- a/cluster.go +++ b/cluster.go @@ -1356,9 +1356,8 @@ func (c *cluster) findFieldKeys(ctx context.Context, field *Field, keys ...strin // Therefore, the field keys are actually column keys on a different index. return c.findIndexKeys(ctx, idx, keys...) } - if !field.Keys() { - return nil, errors.Wrap(ErrTranslatingKeyNotFound, "field is not keyed") + return nil, errors.Errorf("cannot find keys on unkeyed field %q", field.Name()) } // Attempt to find the keys locally. @@ -1421,7 +1420,7 @@ func (c *cluster) createFieldKeys(ctx context.Context, field *Field, keys ...str } if !field.Keys() { - return nil, errors.Wrap(ErrTranslatingKeyNotFound, "field is not keyed") + return nil, errors.Errorf("cannot create keys on unkeyed field %q", field.Name()) } // The primary is the only node that can create field keys, since it owns the authoritative copy. @@ -1621,6 +1620,9 @@ func (c *cluster) findIndexKeys(ctx context.Context, indexName string, keys ...s if idx == nil { return nil, ErrIndexNotFound } + if !idx.Keys() { + return nil, errors.Errorf("cannot find keys on unkeyed index %q", indexName) + } // Create a snapshot of the cluster to use for node/partition calculations. snap := topology.NewClusterSnapshot(c.noder, c.Hasher, c.ReplicaN) @@ -1728,7 +1730,7 @@ func (c *cluster) createIndexKeys(ctx context.Context, indexName string, keys .. } if !idx.keys { - return nil, errors.Errorf("can't create index keys on unkeyed index %s", indexName) + return nil, errors.Errorf("cannot create keys on unkeyed index %q", indexName) } // Create a snapshot of the cluster to use for node/partition calculations. diff --git a/executor_test.go b/executor_test.go index 8c160b5a6..3f33c8415 100644 --- a/executor_test.go +++ b/executor_test.go @@ -587,7 +587,7 @@ func TestExecutor_Execute_Set(t *testing.T) { }) t.Run("ErrInvalidRowValueType", func(t *testing.T) { - if _, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(2, f="bar")`}); err == nil || !hasCause(err, pilosa.ErrTranslatingKeyNotFound) || !strings.Contains(err.Error(), "field is not keyed") { + if _, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(2, f="bar")`}); err == nil || !strings.Contains(err.Error(), "cannot create keys on unkeyed field") { t.Fatal(err) } }) @@ -990,26 +990,13 @@ func TestExecutor_Execute_SetValue(t *testing.T) { }) t.Run("InvalidBSIGroupValueType", func(t *testing.T) { - if _, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(10, f="hello")`}); err == nil || !hasCause(err, pilosa.ErrTranslatingKeyNotFound) || !strings.Contains(err.Error(), "field is not keyed") { + if _, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(10, f="hello")`}); err == nil || !strings.Contains(err.Error(), "cannot create keys on unkeyed field") { t.Fatalf("unexpected error: %s", err) } }) }) } -func hasCause(err, cause error) bool { - for err != cause { - innerErr := errors.Cause(err) - if innerErr == err { - // This is the innermost accessible error, and it does not have that cause. - return false - } - err = innerErr - } - - return true -} - // Ensure a SetRowAttrs() query can be executed. func TestExecutor_Execute_SetRowAttrs(t *testing.T) { c := test.MustRunCluster(t, 1) diff --git a/translator_test.go b/translator_test.go index ab26faf06..ffe41d71a 100644 --- a/translator_test.go +++ b/translator_test.go @@ -879,6 +879,70 @@ func TestTranslation_Cluster_CreateFind(t *testing.T) { }) } +func TestTranslation_Cluster_CreateFindUnkeyed(t *testing.T) { + c := test.MustRunCluster(t, 3) + defer c.Close() + + c.CreateField(t, "i", pilosa.IndexOptions{}, "f") + + t.Run("Index", func(t *testing.T) { + t.Run("Create", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + _, err := c.GetNonPrimary().API.CreateIndexKeys(ctx, "i", "foo") + if err == nil { + t.Fatal("unexpected success") + } + expect := `cannot create keys on unkeyed index "i"` + if got := err.Error(); got != expect { + t.Fatalf("expected error %q but got %q", expect, got) + } + }) + t.Run("Find", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + _, err := c.GetNonPrimary().API.FindIndexKeys(ctx, "i", "foo") + if err == nil { + t.Fatal("unexpected success") + } + expect := `cannot find keys on unkeyed index "i"` + if got := err.Error(); got != expect { + t.Fatalf("expected error %q but got %q", expect, got) + } + }) + }) + t.Run("Field", func(t *testing.T) { + t.Run("Create", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + _, err := c.GetNonPrimary().API.CreateFieldKeys(ctx, "i", "f", "foo") + if err == nil { + t.Fatal("unexpected success") + } + expect := `cannot create keys on unkeyed field "f"` + if got := err.Error(); got != expect { + t.Fatalf("expected error %q but got %q", expect, got) + } + }) + t.Run("Find", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + _, err := c.GetNonPrimary().API.FindFieldKeys(ctx, "i", "f", "foo") + if err == nil { + t.Fatal("unexpected success") + } + expect := `cannot find keys on unkeyed field "f"` + if got := err.Error(); got != expect { + t.Fatalf("expected error %q but got %q", expect, got) + } + }) + }) +} + func compareTranslations(expected, got map[string]uint64) error { for key, id := range got { if realID, ok := expected[key]; !ok {