From 80da129861758b5649940666ef6693eb282ec010 Mon Sep 17 00:00:00 2001 From: Travis Date: Mon, 30 Mar 2020 13:38:50 -0500 Subject: [PATCH] require scale argument for decimal fields --- http/handler.go | 56 ++++++++++++++++++++++++++++-------------- server/handler_test.go | 22 ++++++++++++++--- 2 files changed, 55 insertions(+), 23 deletions(-) diff --git a/http/handler.go b/http/handler.go index c8fed7ce0..8b5d764ad 100644 --- a/http/handler.go +++ b/http/handler.go @@ -797,7 +797,7 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { switch req.Options.Type { case pilosa.FieldTypeSet: fos = append(fos, pilosa.OptFieldTypeSet(*req.Options.CacheType, *req.Options.CacheSize)) - case pilosa.FieldTypeInt, pilosa.FieldTypeDecimal: + case pilosa.FieldTypeInt: if req.Options.Min == nil { min := pql.NewDecimal(int64(math.MinInt64), 0) req.Options.Min = &min @@ -806,24 +806,30 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { max := pql.NewDecimal(int64(math.MaxInt64), 0) req.Options.Max = &max } - if req.Options.Type == pilosa.FieldTypeDecimal { - scale := int64(0) - if req.Options.Scale != nil { - scale = *req.Options.Scale - } - var minmax []pql.Decimal - if req.Options.Min != nil { - minmax = []pql.Decimal{ - *req.Options.Min, - } - if req.Options.Max != nil { - minmax = append(minmax, *req.Options.Max) - } - } - fos = append(fos, pilosa.OptFieldTypeDecimal(scale, minmax...)) - } else { - fos = append(fos, pilosa.OptFieldTypeInt(req.Options.Min.ToInt64(0), req.Options.Max.ToInt64(0))) + fos = append(fos, pilosa.OptFieldTypeInt(req.Options.Min.ToInt64(0), req.Options.Max.ToInt64(0))) + case pilosa.FieldTypeDecimal: + scale := int64(0) + if req.Options.Scale != nil { + scale = *req.Options.Scale } + if req.Options.Min == nil { + min := pql.NewDecimal(int64(math.MinInt64), scale) + req.Options.Min = &min + } + if req.Options.Max == nil { + max := pql.NewDecimal(int64(math.MaxInt64), scale) + req.Options.Max = &max + } + var minmax []pql.Decimal + if req.Options.Min != nil { + minmax = []pql.Decimal{ + *req.Options.Min, + } + if req.Options.Max != nil { + minmax = append(minmax, *req.Options.Max) + } + } + fos = append(fos, pilosa.OptFieldTypeDecimal(scale, minmax...)) case pilosa.FieldTypeTime: fos = append(fos, pilosa.OptFieldTypeTime(*req.Options.TimeQuantum, req.Options.NoStandardView)) case pilosa.FieldTypeMutex: @@ -895,7 +901,7 @@ func (o *fieldOptions) validate() error { } else if o.ForeignIndex != nil { return pilosa.NewBadRequestError(errors.New("set field cannot be a foreign key")) } - case pilosa.FieldTypeInt, pilosa.FieldTypeDecimal: + case pilosa.FieldTypeInt: if o.CacheType != nil { return pilosa.NewBadRequestError(errors.New("cacheType does not apply to field type int")) } else if o.CacheSize != nil { @@ -905,6 +911,18 @@ func (o *fieldOptions) validate() error { } else if o.ForeignIndex != nil && o.Type == pilosa.FieldTypeDecimal { return pilosa.NewBadRequestError(errors.New("decimal field cannot be a foreign key")) } + case pilosa.FieldTypeDecimal: + if o.Scale == nil { + return pilosa.NewBadRequestError(errors.New("decimal field requires a scale argument")) + } else if o.CacheType != nil { + return pilosa.NewBadRequestError(errors.New("cacheType does not apply to field type int")) + } else if o.CacheSize != nil { + return pilosa.NewBadRequestError(errors.New("cacheSize does not apply to field type int")) + } else if o.TimeQuantum != nil { + return pilosa.NewBadRequestError(errors.New("timeQuantum does not apply to field type int")) + } else if o.ForeignIndex != nil && o.Type == pilosa.FieldTypeDecimal { + return pilosa.NewBadRequestError(errors.New("decimal field cannot be a foreign key")) + } case pilosa.FieldTypeTime: if o.CacheType != nil { return pilosa.NewBadRequestError(errors.New("cacheType does not apply to field type time")) diff --git a/server/handler_test.go b/server/handler_test.go index 8623d4403..31e084243 100644 --- a/server/handler_test.go +++ b/server/handler_test.go @@ -655,7 +655,7 @@ func TestHandler_Endpoints(t *testing.T) { w := httptest.NewRecorder() fieldName := "f-decimal-ubound" h.ServeHTTP(w, test.MustNewHTTPRequest("POST", fmt.Sprintf("/index/i0/field/%s", fieldName), - strings.NewReader(`{"options":{"type":"decimal"}}`))) + strings.NewReader(`{"options":{"type":"decimal", "scale": 0}}`))) if w.Code != gohttp.StatusOK { t.Fatalf("unexpected status code: %d", w.Code) } @@ -684,7 +684,7 @@ func TestHandler_Endpoints(t *testing.T) { w := httptest.NewRecorder() fieldName := "f-decimal-ubound-min" h.ServeHTTP(w, test.MustNewHTTPRequest("POST", fmt.Sprintf("/index/i0/field/%s", fieldName), - strings.NewReader(`{"options":{"type":"decimal", "max": 10.5}}`))) + strings.NewReader(`{"options":{"type":"decimal", "scale": 1, "max": 10.5}}`))) if w.Code != gohttp.StatusOK { fmt.Println(w.Body.String()) t.Fatalf("unexpected status code: %d", w.Code) @@ -702,14 +702,28 @@ func TestHandler_Endpoints(t *testing.T) { if field == nil { t.Fatalf("field not found: %s", fieldName) } - if !reflect.DeepEqual(pql.NewDecimal(math.MinInt64, 0), field.Options.Min) { - t.Fatalf("field min %d != %d", int64(math.MinInt64), field.Options.Min) + if !reflect.DeepEqual(pql.NewDecimal(math.MinInt64, 1), field.Options.Min) { + t.Fatalf("field min %d != %d", pql.NewDecimal(math.MinInt64, 1), field.Options.Min) } if !reflect.DeepEqual(pql.NewDecimal(105, 1), field.Options.Max) { t.Fatalf("field max %s != %d", pql.NewDecimal(105, 1), field.Options.Max) } }) + // Ensure that decimal fields error when scale is not provided. + t.Run("Query decimal field scale error", func(t *testing.T) { + w := httptest.NewRecorder() + fieldName := "f-decimal-ubound" + h.ServeHTTP(w, test.MustNewHTTPRequest("POST", fmt.Sprintf("/index/i0/field/%s", fieldName), + strings.NewReader(`{"options":{"type":"decimal"}}`))) + expErr := "decimal field requires a scale argument" + if w.Code != gohttp.StatusBadRequest { + t.Fatalf("unexpected status code: %d", w.Code) + } else if !strings.Contains(w.Body.String(), expErr) { + t.Fatalf("expected error to contain: %s, but got: %s", expErr, w.Body.String()) + } + }) + t.Run("Method not allowed", func(t *testing.T) { w := httptest.NewRecorder() h.ServeHTTP(w, test.MustNewHTTPRequest("GET", "/index/i0/query", nil))