From 62e2b16b8861cf54c24fa918879e8ce25893b486 Mon Sep 17 00:00:00 2001 From: Yuce Tekol Date: Mon, 27 May 2019 15:49:10 +0300 Subject: [PATCH 1/5] set defaults for int field min and max --- http/handler.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/http/handler.go b/http/handler.go index 0ce3ee6cd..0d4691d78 100644 --- a/http/handler.go +++ b/http/handler.go @@ -759,7 +759,13 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { case pilosa.FieldTypeSet: fos = append(fos, pilosa.OptFieldTypeSet(*req.Options.CacheType, *req.Options.CacheSize)) case pilosa.FieldTypeInt: - fos = append(fos, pilosa.OptFieldTypeInt(math.MinInt64, math.MaxInt64)) + if req.Options.Min == nil { + *req.Options.Min = math.MinInt64 + } + if req.Options.Max == nil { + *req.Options.Max = math.MaxInt64 + } + fos = append(fos, pilosa.OptFieldTypeInt(*req.Options.Min, *req.Options.Max)) case pilosa.FieldTypeTime: fos = append(fos, pilosa.OptFieldTypeTime(*req.Options.TimeQuantum, req.Options.NoStandardView)) case pilosa.FieldTypeMutex: From b5e4b90438c3406c4f2382ebd3400c52e1c3a56a Mon Sep 17 00:00:00 2001 From: Yuce Tekol Date: Mon, 27 May 2019 17:43:53 +0300 Subject: [PATCH 2/5] fixes #1977 --- field.go | 3 +++ http/handler.go | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/field.go b/field.go index 73c09f14c..3e71af589 100644 --- a/field.go +++ b/field.go @@ -135,6 +135,9 @@ func OptFieldTypeInt(min, max int64) FieldOption { if fo.Type != "" { return errors.Errorf("field type is already set to: %s", fo.Type) } + if min > max { + return errors.New("int field min cannot be greater than max") + } fo.Type = FieldTypeInt fo.Min = min fo.Max = max diff --git a/http/handler.go b/http/handler.go index 0d4691d78..1334af4d5 100644 --- a/http/handler.go +++ b/http/handler.go @@ -760,10 +760,12 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { fos = append(fos, pilosa.OptFieldTypeSet(*req.Options.CacheType, *req.Options.CacheSize)) case pilosa.FieldTypeInt: if req.Options.Min == nil { - *req.Options.Min = math.MinInt64 + min := int64(math.MinInt64) + req.Options.Min = &min } if req.Options.Max == nil { - *req.Options.Max = math.MaxInt64 + max := int64(math.MaxInt64) + req.Options.Max = &max } fos = append(fos, pilosa.OptFieldTypeInt(*req.Options.Min, *req.Options.Max)) case pilosa.FieldTypeTime: From 5f4c5d4d35a7d0eeccc27f2e5927caa0ed3f6406 Mon Sep 17 00:00:00 2001 From: Yuce Tekol Date: Tue, 28 May 2019 13:54:12 +0300 Subject: [PATCH 3/5] added test for 1977 fix --- api.go | 2 +- http/handler.go | 6 +++ server/handler_test.go | 116 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 1 deletion(-) diff --git a/api.go b/api.go index 2b96d023c..bdaa35b03 100644 --- a/api.go +++ b/api.go @@ -213,7 +213,7 @@ func (api *API) CreateField(ctx context.Context, indexName string, fieldName str for _, opt := range opts { err := opt(&fo) if err != nil { - return nil, errors.Wrap(err, "applying option") + return nil, NewBadRequestError(errors.Wrap(err, "applying option")) } } diff --git a/http/handler.go b/http/handler.go index 1334af4d5..0d52a270b 100644 --- a/http/handler.go +++ b/http/handler.go @@ -782,6 +782,12 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { } _, err = h.api.CreateField(r.Context(), indexName, fieldName, fos...) + if err != nil { + if _, ok := err.(pilosa.BadRequestError); ok { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + } resp.write(w, err) } diff --git a/server/handler_test.go b/server/handler_test.go index fc832269a..dc4f571d5 100644 --- a/server/handler_test.go +++ b/server/handler_test.go @@ -22,6 +22,7 @@ import ( "fmt" "io" "io/ioutil" + "math" gohttp "net/http" "net/http/httptest" "reflect" @@ -542,6 +543,104 @@ func TestHandler_Endpoints(t *testing.T) { } }) + t.Run("Query int field unbounded", func(t *testing.T) { + w := httptest.NewRecorder() + fieldName := "f-int-ubound" + h.ServeHTTP(w, test.MustNewHTTPRequest("POST", fmt.Sprintf("/index/i0/field/%s", fieldName), + strings.NewReader(`{"options":{"type":"int"}}`))) + if w.Code != gohttp.StatusOK { + t.Fatalf("unexpected status code: %d", w.Code) + } + w = httptest.NewRecorder() + h.ServeHTTP(w, test.MustNewHTTPRequest("GET", "/schema", strings.NewReader(""))) + if w.Code != gohttp.StatusOK { + t.Fatalf("unexpected status code: %d", w.Code) + } + rsp := getSchemaResponse{} + if err := json.Unmarshal([]byte(w.Body.String()), &rsp); err != nil { + t.Fatalf("json decode: %s", err) + } + field := rsp.findField("i0", fieldName) + if field == nil { + t.Fatalf("field not found: %s", fieldName) + } + if math.MinInt64 != field.Options.Min { + t.Fatalf("field min %d != %d", math.MinInt64, field.Options.Min) + } + if math.MaxInt64 != field.Options.Max { + t.Fatalf("field max %d != %d", math.MaxInt64, field.Options.Max) + } + }) + + t.Run("Query int field unbounded min", func(t *testing.T) { + w := httptest.NewRecorder() + fieldName := "f-int-ubound-min" + h.ServeHTTP(w, test.MustNewHTTPRequest("POST", fmt.Sprintf("/index/i0/field/%s", fieldName), + strings.NewReader(`{"options":{"type":"int", "max": 10}}`))) + if w.Code != gohttp.StatusOK { + t.Fatalf("unexpected status code: %d", w.Code) + } + w = httptest.NewRecorder() + h.ServeHTTP(w, test.MustNewHTTPRequest("GET", "/schema", strings.NewReader(""))) + if w.Code != gohttp.StatusOK { + t.Fatalf("unexpected status code: %d", w.Code) + } + rsp := getSchemaResponse{} + if err := json.Unmarshal([]byte(w.Body.String()), &rsp); err != nil { + t.Fatalf("json decode: %s", err) + } + field := rsp.findField("i0", fieldName) + if field == nil { + t.Fatalf("field not found: %s", fieldName) + } + if math.MinInt64 != field.Options.Min { + t.Fatalf("field min %d != %d", math.MinInt64, field.Options.Min) + } + if 10 != field.Options.Max { + t.Fatalf("field max %d != %d", 10, field.Options.Max) + } + }) + + t.Run("Query int field unbounded max", func(t *testing.T) { + w := httptest.NewRecorder() + fieldName := "f-int-ubound-max" + h.ServeHTTP(w, test.MustNewHTTPRequest("POST", fmt.Sprintf("/index/i0/field/%s", fieldName), + strings.NewReader(`{"options":{"type":"int", "min": -10}}`))) + if w.Code != gohttp.StatusOK { + t.Fatalf("unexpected status code: %d", w.Code) + } + w = httptest.NewRecorder() + h.ServeHTTP(w, test.MustNewHTTPRequest("GET", "/schema", strings.NewReader(""))) + if w.Code != gohttp.StatusOK { + t.Fatalf("unexpected status code: %d", w.Code) + } + rsp := getSchemaResponse{} + if err := json.Unmarshal([]byte(w.Body.String()), &rsp); err != nil { + t.Fatalf("json decode: %s", err) + } + field := rsp.findField("i0", fieldName) + if field == nil { + t.Fatalf("field not found: %s", fieldName) + } + if -10 != field.Options.Min { + t.Fatalf("field min %d != %d", 10, field.Options.Min) + } + if math.MaxInt64 != field.Options.Max { + t.Fatalf("field max %d != %d", math.MaxInt64, field.Options.Max) + } + }) + + t.Run("Query int field min > max return 400", func(t *testing.T) { + w := httptest.NewRecorder() + fieldName := "f-int-ubound-err" + h.ServeHTTP(w, test.MustNewHTTPRequest("POST", fmt.Sprintf("/index/i0/field/%s", fieldName), + strings.NewReader(`{"options":{"type":"int", "min": 10, "max": -10}}`))) + fmt.Println("body", w.Body.String()) + if w.Code != gohttp.StatusBadRequest { + t.Fatalf("unexpected status code: %d", w.Code) + } + }) + t.Run("Method not allowed", func(t *testing.T) { w := httptest.NewRecorder() h.ServeHTTP(w, test.MustNewHTTPRequest("GET", "/index/i0/query", nil)) @@ -999,3 +1098,20 @@ func mustJSONDecodeSlice(t *testing.T, r io.Reader) (ret []interface{}) { } return ret } + +type getSchemaResponse struct { + Indexes []*pilosa.IndexInfo `json:"indexes"` +} + +func (r getSchemaResponse) findField(indexName, fieldName string) *pilosa.FieldInfo { + for _, index := range r.Indexes { + if index.Name == indexName { + for _, field := range index.Fields { + if field.Name == fieldName { + return field + } + } + } + } + return nil +} From 5e102154caf5efd3395282a5cdeb1d3ac8505fa0 Mon Sep 17 00:00:00 2001 From: Yuce Tekol Date: Tue, 28 May 2019 14:12:04 +0300 Subject: [PATCH 4/5] make linter happy --- http/handler.go | 8 +++----- server/handler_test.go | 6 +++--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/http/handler.go b/http/handler.go index 0d52a270b..8e932271a 100644 --- a/http/handler.go +++ b/http/handler.go @@ -782,11 +782,9 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { } _, err = h.api.CreateField(r.Context(), indexName, fieldName, fos...) - if err != nil { - if _, ok := err.(pilosa.BadRequestError); ok { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } + if _, ok := err.(pilosa.BadRequestError); ok { + http.Error(w, err.Error(), http.StatusBadRequest) + return } resp.write(w, err) } diff --git a/server/handler_test.go b/server/handler_test.go index dc4f571d5..c830c94d7 100644 --- a/server/handler_test.go +++ b/server/handler_test.go @@ -557,7 +557,7 @@ func TestHandler_Endpoints(t *testing.T) { t.Fatalf("unexpected status code: %d", w.Code) } rsp := getSchemaResponse{} - if err := json.Unmarshal([]byte(w.Body.String()), &rsp); err != nil { + if err := json.Unmarshal(w.Body.Bytes(), &rsp); err != nil { t.Fatalf("json decode: %s", err) } field := rsp.findField("i0", fieldName) @@ -586,7 +586,7 @@ func TestHandler_Endpoints(t *testing.T) { t.Fatalf("unexpected status code: %d", w.Code) } rsp := getSchemaResponse{} - if err := json.Unmarshal([]byte(w.Body.String()), &rsp); err != nil { + if err := json.Unmarshal(w.Body.Bytes(), &rsp); err != nil { t.Fatalf("json decode: %s", err) } field := rsp.findField("i0", fieldName) @@ -615,7 +615,7 @@ func TestHandler_Endpoints(t *testing.T) { t.Fatalf("unexpected status code: %d", w.Code) } rsp := getSchemaResponse{} - if err := json.Unmarshal([]byte(w.Body.String()), &rsp); err != nil { + if err := json.Unmarshal(w.Body.Bytes(), &rsp); err != nil { t.Fatalf("json decode: %s", err) } field := rsp.findField("i0", fieldName) From c8a3dc8c185ffe3961f977e6de503b910493ec96 Mon Sep 17 00:00:00 2001 From: Yuce Tekol Date: Tue, 28 May 2019 14:25:46 +0300 Subject: [PATCH 5/5] fix int min max test for 32bit --- server/handler_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/handler_test.go b/server/handler_test.go index c830c94d7..c4e98d7f3 100644 --- a/server/handler_test.go +++ b/server/handler_test.go @@ -565,10 +565,10 @@ func TestHandler_Endpoints(t *testing.T) { t.Fatalf("field not found: %s", fieldName) } if math.MinInt64 != field.Options.Min { - t.Fatalf("field min %d != %d", math.MinInt64, field.Options.Min) + t.Fatalf("field min %d != %d", int64(math.MinInt64), field.Options.Min) } if math.MaxInt64 != field.Options.Max { - t.Fatalf("field max %d != %d", math.MaxInt64, field.Options.Max) + t.Fatalf("field max %d != %d", int64(math.MaxInt64), field.Options.Max) } }) @@ -594,7 +594,7 @@ func TestHandler_Endpoints(t *testing.T) { t.Fatalf("field not found: %s", fieldName) } if math.MinInt64 != field.Options.Min { - t.Fatalf("field min %d != %d", math.MinInt64, field.Options.Min) + t.Fatalf("field min %d != %d", int64(math.MinInt64), field.Options.Min) } if 10 != field.Options.Max { t.Fatalf("field max %d != %d", 10, field.Options.Max) @@ -626,7 +626,7 @@ func TestHandler_Endpoints(t *testing.T) { t.Fatalf("field min %d != %d", 10, field.Options.Min) } if math.MaxInt64 != field.Options.Max { - t.Fatalf("field max %d != %d", math.MaxInt64, field.Options.Max) + t.Fatalf("field max %d != %d", int64(math.MaxInt64), field.Options.Max) } })