From 002b65c516f6005bd1265d3b53875179e0225ea7 Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Mon, 1 Nov 2021 17:04:33 -0500 Subject: [PATCH 01/10] add checks for data presence on import req --- api.go | 7 ++++++- http/handler.go | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/api.go b/api.go index 458164f6e..b954d9268 100644 --- a/api.go +++ b/api.go @@ -1760,6 +1760,9 @@ func (api *API) ImportValueWithTx(ctx context.Context, qcx *Qcx, req *ImportValu // if we're importing into a specific shard if req.Shard != math.MaxUint64 { + if len(req.ColumnIDs) == 0 { + return errors.Wrap(err, "calculating shard, no columns in request") + } // Check that column IDs match the stated shard. shard := req.ColumnIDs[0] / ShardWidth if s2 := req.ColumnIDs[len(req.ColumnIDs)-1] / ShardWidth; (shard != s2) || (shard != req.Shard) { @@ -1798,7 +1801,9 @@ func (api *API) ImportValueWithTx(ctx context.Context, qcx *Qcx, req *ImportValu return errors.Wrap(err, "importing value") } // end if req.Shard != math.MaxUint64 - + if len(req.ColumnIDs) == 0 { + return errors.Wrap(err, "calculating shard, no columns in request") + } options.IgnoreKeyCheck = true start := 0 shard := req.ColumnIDs[0] / ShardWidth diff --git a/http/handler.go b/http/handler.go index 59a3c3162..92f374e84 100644 --- a/http/handler.go +++ b/http/handler.go @@ -2721,6 +2721,10 @@ func (h *Handler) handlePostImport(w http.ResponseWriter, r *http.Request) { qcx := h.api.Txf().NewQcx() defer qcx.Abort() + if len(req.Values) == 0 { + return + } + if err := h.api.ImportValue(r.Context(), qcx, req, opts...); err != nil { switch errors.Cause(err) { case pilosa.ErrClusterDoesNotOwnShard, pilosa.ErrPreconditionFailed: From 1de6085782d08827fe5ef2d2ebb0fa4ee3722f8f Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Mon, 1 Nov 2021 17:51:16 -0500 Subject: [PATCH 02/10] remove empty value check from handler --- http/handler.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/http/handler.go b/http/handler.go index 92f374e84..59a3c3162 100644 --- a/http/handler.go +++ b/http/handler.go @@ -2721,10 +2721,6 @@ func (h *Handler) handlePostImport(w http.ResponseWriter, r *http.Request) { qcx := h.api.Txf().NewQcx() defer qcx.Abort() - if len(req.Values) == 0 { - return - } - if err := h.api.ImportValue(r.Context(), qcx, req, opts...); err != nil { switch errors.Cause(err) { case pilosa.ErrClusterDoesNotOwnShard, pilosa.ErrPreconditionFailed: From 5206e8fa0e0c8ab8a3c714a59340857551fa852b Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Mon, 1 Nov 2021 18:19:38 -0500 Subject: [PATCH 03/10] more comprehensive value check --- http/handler.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/http/handler.go b/http/handler.go index 59a3c3162..9b65fe257 100644 --- a/http/handler.go +++ b/http/handler.go @@ -2721,6 +2721,9 @@ func (h *Handler) handlePostImport(w http.ResponseWriter, r *http.Request) { qcx := h.api.Txf().NewQcx() defer qcx.Abort() + if isImportRequestEmpty(req) { + return + } if err := h.api.ImportValue(r.Context(), qcx, req, opts...); err != nil { switch errors.Cause(err) { case pilosa.ErrClusterDoesNotOwnShard, pilosa.ErrPreconditionFailed: @@ -3463,3 +3466,8 @@ func (h *Handler) handlePostRestore(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) w.Write([]byte("OK")) //nolint:errcheck } + +func isImportRequestEmpty(req *pilosa.ImportValueRequest) bool { + totalData := len(req.Values) + len(req.FloatValues) + len(req.TimestampValues) + len(req.StringValues) + return totalData == 0 +} From 3ca91bba7f6f3a7e3de0153b38c3ab8992627f98 Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Tue, 2 Nov 2021 15:49:59 -0500 Subject: [PATCH 04/10] More specific error message on mismatch --- http/handler.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/http/handler.go b/http/handler.go index 9b65fe257..28663cc23 100644 --- a/http/handler.go +++ b/http/handler.go @@ -2722,6 +2722,9 @@ func (h *Handler) handlePostImport(w http.ResponseWriter, r *http.Request) { defer qcx.Abort() if isImportRequestEmpty(req) { + if len(req.ColumnIDs) > 0 || len(req.ColumnKeys) > 0 { + http.Error(w, "columns provided but no values", http.StatusBadRequest) + } return } if err := h.api.ImportValue(r.Context(), qcx, req, opts...); err != nil { From 2bc2640fbfa465c03387d462fc84ad3b07c4b2d7 Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Tue, 2 Nov 2021 15:50:48 -0500 Subject: [PATCH 05/10] test two scenarios --- api_test.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/api_test.go b/api_test.go index 378aee9fd..05fd3acf9 100644 --- a/api_test.go +++ b/api_test.go @@ -251,6 +251,43 @@ func TestAPI_ImportValue(t *testing.T) { } }) + t.Run("ValIntEmpty", func(t *testing.T) { + ctx := context.Background() + index := "valintempty" + field := "f" + + _, err := coord.API.CreateIndex(ctx, index, pilosa.IndexOptions{Keys: true}) + if err != nil { + t.Fatalf("creating index: %v", err) + } + _, err = coord.API.CreateField(ctx, index, field, pilosa.OptFieldTypeInt(math.MinInt64, math.MaxInt64)) + if err != nil { + t.Fatalf("creating field: %v", err) + } + + // Column keys are sharded so their order is not guaranteed. + colKeys := []string{"col2", "col1", "col3"} + + // Import without data, verify that it succeeds + req := &pilosa.ImportValueRequest{ + Index: index, + Field: field, + } + qcx1 := coord.API.Txf().NewQcx() + if err := coord.API.ImportValue(ctx, qcx1, req); err != nil { + t.Fatal(err) + } + PanicOn(qcx1.Finish()) + + // Import without data but with columnkeys, verify that it errors + req.ColumnKeys = colKeys + qcx2 := coord.API.Txf().NewQcx() + if err := coord.API.ImportValue(ctx, qcx2, req); err == nil { + t.Fatal("expected error but succeeded") + } + PanicOn(qcx2.Finish()) + }) + t.Run("ValDecimalField", func(t *testing.T) { ctx := context.Background() index := "valdec" From c0e201d638fba6711cdfd935fb12cb41bcf1725e Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Thu, 4 Nov 2021 12:19:56 -0500 Subject: [PATCH 06/10] dedup api_test --- api_test.go | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/api_test.go b/api_test.go index 05fd3acf9..1ac27ba02 100644 --- a/api_test.go +++ b/api_test.go @@ -254,16 +254,9 @@ func TestAPI_ImportValue(t *testing.T) { t.Run("ValIntEmpty", func(t *testing.T) { ctx := context.Background() index := "valintempty" - field := "f" - - _, err := coord.API.CreateIndex(ctx, index, pilosa.IndexOptions{Keys: true}) - if err != nil { - t.Fatalf("creating index: %v", err) - } - _, err = coord.API.CreateField(ctx, index, field, pilosa.OptFieldTypeInt(math.MinInt64, math.MaxInt64)) - if err != nil { - t.Fatalf("creating field: %v", err) - } + field := "fld" + createIndexForTest(index, coord, t) + createFieldForTest(index, field, coord, t) // Column keys are sharded so their order is not guaranteed. colKeys := []string{"col2", "col1", "col3"} @@ -1262,3 +1255,19 @@ func TestAPI_MutexCheck(t *testing.T) { }) } } + +func createIndexForTest(index string, coord *test.Command, t *testing.T) { + ctx := context.Background() + _, err := coord.API.CreateIndex(ctx, index, pilosa.IndexOptions{Keys: true}) + if err != nil { + t.Fatalf("creating index: %v", err) + } +} + +func createFieldForTest(index string, field string, coord *test.Command, t *testing.T) { + ctx := context.Background() + _, err := coord.API.CreateField(ctx, index, field, pilosa.OptFieldTypeInt(math.MinInt64, math.MaxInt64)) + if err != nil { + t.Fatalf("creating field: %v", err) + } +} From 67233d572096ccadd5d9be264b1ded93c8122434 Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Thu, 4 Nov 2021 15:52:37 -0500 Subject: [PATCH 07/10] move checks to api.go --- api.go | 9 +++++++++ http/handler.go | 11 ----------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/api.go b/api.go index b954d9268..5aba8d272 100644 --- a/api.go +++ b/api.go @@ -1688,6 +1688,15 @@ func (api *API) ImportValueWithTx(ctx context.Context, qcx *Qcx, req *ImportValu return errors.Wrap(err, "validating api method") } + numCols := len(req.ColumnIDs) + len(req.ColumnKeys) + numVals := len(req.Values) + len(req.FloatValues) + len(req.TimestampValues) + len(req.StringValues) + if numCols != numVals { + return errors.New(fmt.Sprintf("number of columns (%v) and number of values (%v) do not match", numCols, numVals)) + } + if numCols == 0 { + return nil + } + idx, field, err := api.indexField(req.Index, req.Field, req.Shard) if err != nil { return errors.Wrap(err, fmt.Sprintf("getting index '%v' and field '%v'; shard=%v", req.Index, req.Field, req.Shard)) diff --git a/http/handler.go b/http/handler.go index 28663cc23..59a3c3162 100644 --- a/http/handler.go +++ b/http/handler.go @@ -2721,12 +2721,6 @@ func (h *Handler) handlePostImport(w http.ResponseWriter, r *http.Request) { qcx := h.api.Txf().NewQcx() defer qcx.Abort() - if isImportRequestEmpty(req) { - if len(req.ColumnIDs) > 0 || len(req.ColumnKeys) > 0 { - http.Error(w, "columns provided but no values", http.StatusBadRequest) - } - return - } if err := h.api.ImportValue(r.Context(), qcx, req, opts...); err != nil { switch errors.Cause(err) { case pilosa.ErrClusterDoesNotOwnShard, pilosa.ErrPreconditionFailed: @@ -3469,8 +3463,3 @@ func (h *Handler) handlePostRestore(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) w.Write([]byte("OK")) //nolint:errcheck } - -func isImportRequestEmpty(req *pilosa.ImportValueRequest) bool { - totalData := len(req.Values) + len(req.FloatValues) + len(req.TimestampValues) + len(req.StringValues) - return totalData == 0 -} From cdaec2e0d2cd787ffb0178d88e4fa87a5bf4dc76 Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Thu, 4 Nov 2021 16:04:00 -0500 Subject: [PATCH 08/10] remove duplicate checks --- api.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/api.go b/api.go index 5aba8d272..42d7e3b0d 100644 --- a/api.go +++ b/api.go @@ -1769,9 +1769,6 @@ func (api *API) ImportValueWithTx(ctx context.Context, qcx *Qcx, req *ImportValu // if we're importing into a specific shard if req.Shard != math.MaxUint64 { - if len(req.ColumnIDs) == 0 { - return errors.Wrap(err, "calculating shard, no columns in request") - } // Check that column IDs match the stated shard. shard := req.ColumnIDs[0] / ShardWidth if s2 := req.ColumnIDs[len(req.ColumnIDs)-1] / ShardWidth; (shard != s2) || (shard != req.Shard) { @@ -1810,9 +1807,6 @@ func (api *API) ImportValueWithTx(ctx context.Context, qcx *Qcx, req *ImportValu return errors.Wrap(err, "importing value") } // end if req.Shard != math.MaxUint64 - if len(req.ColumnIDs) == 0 { - return errors.Wrap(err, "calculating shard, no columns in request") - } options.IgnoreKeyCheck = true start := 0 shard := req.ColumnIDs[0] / ShardWidth From 20131a697cde98a19f599c41bbc0d1db377176a3 Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Thu, 4 Nov 2021 17:07:24 -0500 Subject: [PATCH 09/10] add additional tests and defer qcx --- api_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/api_test.go b/api_test.go index 1ac27ba02..592700acb 100644 --- a/api_test.go +++ b/api_test.go @@ -260,6 +260,7 @@ func TestAPI_ImportValue(t *testing.T) { // Column keys are sharded so their order is not guaranteed. colKeys := []string{"col2", "col1", "col3"} + values := []int64{1, 2, 3, 4} // Import without data, verify that it succeeds req := &pilosa.ImportValueRequest{ @@ -267,6 +268,9 @@ func TestAPI_ImportValue(t *testing.T) { Field: field, } qcx1 := coord.API.Txf().NewQcx() + defer qcx1.Abort() + + // Import with empty request, should succeed if err := coord.API.ImportValue(ctx, qcx1, req); err != nil { t.Fatal(err) } @@ -279,6 +283,23 @@ func TestAPI_ImportValue(t *testing.T) { t.Fatal("expected error but succeeded") } PanicOn(qcx2.Finish()) + + // Import with mismatch column and value lengths + req.Values = values + qcx3 := coord.API.Txf().NewQcx() + if err := coord.API.ImportValue(ctx, qcx3, req); err == nil { + t.Fatal("expected error but succeeded") + } + PanicOn(qcx3.Finish()) + + // Import with data but no columns + req.ColumnKeys = make([]string, 0) + qcx4 := coord.API.Txf().NewQcx() + if err := coord.API.ImportValue(ctx, qcx4, req); err == nil { + t.Fatal("expected error but succeeded") + } + PanicOn(qcx4.Finish()) + }) t.Run("ValDecimalField", func(t *testing.T) { From cb6a3648909d16eedaa2ed147a0a7c11b61afaa4 Mon Sep 17 00:00:00 2001 From: Samir Patel <48686912+54mir@users.noreply.github.com> Date: Fri, 5 Nov 2021 09:12:28 -0500 Subject: [PATCH 10/10] more qcx defers --- api_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api_test.go b/api_test.go index 592700acb..08e1b49e5 100644 --- a/api_test.go +++ b/api_test.go @@ -279,6 +279,7 @@ func TestAPI_ImportValue(t *testing.T) { // Import without data but with columnkeys, verify that it errors req.ColumnKeys = colKeys qcx2 := coord.API.Txf().NewQcx() + defer qcx2.Abort() if err := coord.API.ImportValue(ctx, qcx2, req); err == nil { t.Fatal("expected error but succeeded") } @@ -287,6 +288,7 @@ func TestAPI_ImportValue(t *testing.T) { // Import with mismatch column and value lengths req.Values = values qcx3 := coord.API.Txf().NewQcx() + defer qcx3.Abort() if err := coord.API.ImportValue(ctx, qcx3, req); err == nil { t.Fatal("expected error but succeeded") } @@ -295,6 +297,7 @@ func TestAPI_ImportValue(t *testing.T) { // Import with data but no columns req.ColumnKeys = make([]string, 0) qcx4 := coord.API.Txf().NewQcx() + defer qcx4.Abort() if err := coord.API.ImportValue(ctx, qcx4, req); err == nil { t.Fatal("expected error but succeeded") }