From 50794bf63be39fd45807255f58c3b340427ee873 Mon Sep 17 00:00:00 2001 From: Travis Turner Date: Sun, 24 Jun 2018 23:59:35 -0500 Subject: [PATCH] move fieldOptions unmarshal to the handler validate fieldOptions in http package --- api.go | 14 +++- client.go | 8 +- cmd/import.go | 7 +- ctl/import.go | 4 +- field.go | 107 +++++++++++++++++++------ field_test.go | 15 ++-- fragment.go | 4 +- fragment_internal_test.go | 2 +- http/client.go | 9 ++- http/handler.go | 144 ++++++++++++++++++++++++---------- http/handler_internal_test.go | 113 +++++++++++++++++++++++--- index.go | 2 +- server/cluster_test.go | 10 +-- server/server_test.go | 13 +-- server_test.go | 3 +- test/field.go | 12 +-- view.go | 2 +- view_internal_test.go | 2 +- 18 files changed, 346 insertions(+), 125 deletions(-) diff --git a/api.go b/api.go index 98fd5d717..dd0d5fca4 100644 --- a/api.go +++ b/api.go @@ -244,11 +244,19 @@ func (api *API) DeleteIndex(ctx context.Context, indexName string) error { } // CreateField makes the named field in the named index with the given options. -func (api *API) CreateField(ctx context.Context, indexName string, fieldName string, options FieldOptions) (*Field, error) { +func (api *API) CreateField(ctx context.Context, indexName string, fieldName string, opts ...FieldOption) (*Field, error) { if err := api.validate(apiCreateField); err != nil { return nil, errors.Wrap(err, "validating api method") } + fo := FieldOptions{} + for _, opt := range opts { + err := opt(&fo) + if err != nil { + return nil, errors.Wrap(err, "applying option") + } + } + // Find index. index := api.Holder.Index(indexName) if index == nil { @@ -256,7 +264,7 @@ func (api *API) CreateField(ctx context.Context, indexName string, fieldName str } // Create field. - field, err := index.CreateField(fieldName, options) + field, err := index.CreateField(fieldName, fo) if err != nil { return nil, errors.Wrap(err, "creating field") } @@ -266,7 +274,7 @@ func (api *API) CreateField(ctx context.Context, indexName string, fieldName str &internal.CreateFieldMessage{ Index: indexName, Field: fieldName, - Meta: options.Encode(), + Meta: fo.Encode(), }) if err != nil { api.server.logger.Printf("problem sending CreateField message: %s", err) diff --git a/client.go b/client.go index b5dbe809a..292a71ef6 100644 --- a/client.go +++ b/client.go @@ -41,10 +41,10 @@ type InternalClient interface { Import(ctx context.Context, index, field string, slice uint64, bits []Bit) error ImportK(ctx context.Context, index, field string, bits []Bit) error EnsureIndex(ctx context.Context, name string, options IndexOptions) error - EnsureField(ctx context.Context, indexName string, fieldName string, options FieldOptions) error + EnsureField(ctx context.Context, indexName string, fieldName string) error ImportValue(ctx context.Context, index, field string, slice uint64, vals []FieldValue) error ExportCSV(ctx context.Context, index, field string, slice uint64, w io.Writer) error - CreateField(ctx context.Context, index, field string, opt FieldOptions) error + CreateField(ctx context.Context, index, field string) error FragmentBlocks(ctx context.Context, uri *URI, index, field string, slice uint64) ([]FragmentBlock, error) BlockData(ctx context.Context, uri *URI, index, field string, slice uint64, block int) ([]uint64, []uint64, error) ColumnAttrDiff(ctx context.Context, uri *URI, index string, blks []AttrBlock) (map[uint64]map[string]interface{}, error) @@ -108,7 +108,7 @@ func (n *NopInternalClient) ImportK(ctx context.Context, index, field string, bi func (n *NopInternalClient) EnsureIndex(ctx context.Context, name string, options IndexOptions) error { return nil } -func (n *NopInternalClient) EnsureField(ctx context.Context, indexName string, fieldName string, options FieldOptions) error { +func (n *NopInternalClient) EnsureField(ctx context.Context, indexName string, fieldName string) error { return nil } func (n *NopInternalClient) ImportValue(ctx context.Context, index, field string, slice uint64, vals []FieldValue) error { @@ -117,7 +117,7 @@ func (n *NopInternalClient) ImportValue(ctx context.Context, index, field string func (n *NopInternalClient) ExportCSV(ctx context.Context, index, field string, slice uint64, w io.Writer) error { return nil } -func (n *NopInternalClient) CreateField(ctx context.Context, index, field string, opt FieldOptions) error { +func (n *NopInternalClient) CreateField(ctx context.Context, index, field string) error { return nil } func (n *NopInternalClient) FragmentBlocks(ctx context.Context, uri *URI, index, field string, slice uint64) ([]FragmentBlock, error) { diff --git a/cmd/import.go b/cmd/import.go index db5cebf8d..5a27b4055 100644 --- a/cmd/import.go +++ b/cmd/import.go @@ -20,7 +20,6 @@ import ( "github.com/spf13/cobra" - "github.com/pilosa/pilosa" "github.com/pilosa/pilosa/ctl" ) @@ -59,9 +58,9 @@ omitted. If it is present then its format should be YYYY-MM-DDTHH:MM. flags.IntVarP(&Importer.BufferSize, "buffer-size", "s", 10000000, "Number of bits to buffer/sort before importing.") flags.BoolVarP(&Importer.Sort, "sort", "", false, "Enables sorting before import.") flags.BoolVarP(&Importer.CreateSchema, "create", "e", false, "Create the schema if it does not exist before import.") - flags.Var(&Importer.FieldOptions.TimeQuantum, "field-time-quantum", "Time quantum for the field") - flags.StringVar(&Importer.FieldOptions.CacheType, "field-cache-type", pilosa.CacheTypeRanked, "Cache type for the field; valid values: none, lru, ranked") - flags.Uint32Var(&Importer.FieldOptions.CacheSize, "field-cache-size", 50000, "Cache size for the field") + //flags.Var(&Importer.FieldOptions.TimeQuantum, "field-time-quantum", "Time quantum for the field") + //flags.StringVar(&Importer.FieldOptions.CacheType, "field-cache-type", pilosa.CacheTypeRanked, "Cache type for the field; valid values: none, lru, ranked") + //flags.Uint32Var(&Importer.FieldOptions.CacheSize, "field-cache-size", 50000, "Cache size for the field") ctl.SetTLSConfig(flags, &Importer.TLS.CertificatePath, &Importer.TLS.CertificateKeyPath, &Importer.TLS.SkipVerify) return importCmd diff --git a/ctl/import.go b/ctl/import.go index c4d65f232..ad4befbfd 100644 --- a/ctl/import.go +++ b/ctl/import.go @@ -42,7 +42,7 @@ type ImportCommand struct { // Options for index & field to be created if they don't exist IndexOptions pilosa.IndexOptions - FieldOptions pilosa.FieldOptions + //FieldOptions pilosa.FieldOptions // CreateSchema ensures the schema exists before import CreateSchema bool @@ -135,7 +135,7 @@ func (cmd *ImportCommand) ensureSchema(ctx context.Context) error { if err != nil { return fmt.Errorf("Error Creating Index: %s", err) } - err = cmd.Client.EnsureField(ctx, cmd.Index, cmd.Field, cmd.FieldOptions) + err = cmd.Client.EnsureField(ctx, cmd.Index, cmd.Field) if err != nil { return fmt.Errorf("Error Creating Field: %s", err) } diff --git a/field.go b/field.go index fda0ee364..93c3d5827 100644 --- a/field.go +++ b/field.go @@ -15,6 +15,7 @@ package pilosa import ( + "encoding/json" "fmt" "io/ioutil" "os" @@ -33,10 +34,10 @@ import ( const ( DefaultFieldType = FieldTypeSet - defaultCacheType = CacheTypeRanked + DefaultCacheType = CacheTypeRanked // Default ranked field cache - defaultCacheSize = 50000 + DefaultCacheSize = 50000 ) // Field types. @@ -69,19 +70,46 @@ type Field struct { Logger Logger } -// FieldOption is a functional option type for pilosa.Fielde. -type FieldOption func(f *Field) error +// FieldOption is a functional option type for pilosa.FieldOptions. +type FieldOption func(fo *FieldOptions) error -// TODO: break these out into separate Options (not a FieldOptions object) -func OptFieldFieldOptions(o FieldOptions) FieldOption { - return func(f *Field) error { - f.options = o +func OptFieldTypeSet(cacheType string, cacheSize uint32) FieldOption { + return func(fo *FieldOptions) error { + if fo.Type != "" { + return errors.Errorf("field type is already set to: %s", fo.Type) + } + fo.Type = FieldTypeSet + fo.CacheType = cacheType + fo.CacheSize = cacheSize + return nil + } +} + +func OptFieldTypeInt(min, max int64) FieldOption { + return func(fo *FieldOptions) error { + if fo.Type != "" { + return errors.Errorf("field type is already set to: %s", fo.Type) + } + fo.Type = FieldTypeInt + fo.Min = min + fo.Max = max + return nil + } +} + +func OptFieldTypeTime(timeQuantum TimeQuantum) FieldOption { + return func(fo *FieldOptions) error { + if fo.Type != "" { + return errors.Errorf("field type is already set to: %s", fo.Type) + } + fo.Type = FieldTypeTime + fo.TimeQuantum = timeQuantum return nil } } // NewField returns a new instance of field. -func NewField(path, index, name string, opts ...FieldOption) (*Field, error) { +func NewField(path, index, name string, options FieldOptions) (*Field, error) { err := validateName(name) if err != nil { return nil, err @@ -99,22 +127,10 @@ func NewField(path, index, name string, opts ...FieldOption) (*Field, error) { broadcaster: NopBroadcaster, Stats: NopStatsClient, - options: FieldOptions{ - Type: DefaultFieldType, - CacheType: defaultCacheType, - CacheSize: defaultCacheSize, - }, + options: applyDefaultOptions(options), Logger: NopLogger, } - - for _, opt := range opts { - err := opt(f) - if err != nil { - return nil, errors.Wrap(err, "applying option") - } - } - return f, nil } @@ -1046,6 +1062,19 @@ type FieldOptions struct { Keys bool `json:"keys,omitempty"` } +// applyDefaultOptions returns a new FieldOptions object +// with default values if o does not contain a valid type. +func applyDefaultOptions(o FieldOptions) FieldOptions { + if o.Type == "" { + return FieldOptions{ + Type: DefaultFieldType, + CacheType: DefaultCacheType, + CacheSize: DefaultCacheSize, + } + } + return o +} + // Validate ensures that FieldOption values are valid. func (o *FieldOptions) Validate() error { switch o.Type { @@ -1100,6 +1129,40 @@ func decodeFieldOptions(options *internal.FieldOptions) *FieldOptions { } } +func (o *FieldOptions) MarshalJSON() ([]byte, error) { + switch o.Type { + case FieldTypeSet: + return json.Marshal(struct { + Type string `json:"type"` + CacheType string `json:"cacheType"` + CacheSize uint32 `json:"cacheSize"` + }{ + o.Type, + o.CacheType, + o.CacheSize, + }) + case FieldTypeInt: + return json.Marshal(struct { + Type string `json:"type"` + Min int64 `json:"min"` + Max int64 `json:"max"` + }{ + o.Type, + o.Min, + o.Max, + }) + case FieldTypeTime: + return json.Marshal(struct { + Type string `json:"type"` + TimeQuantum TimeQuantum `json:"timeQuantum"` + }{ + o.Type, + o.TimeQuantum, + }) + } + return nil, errors.New("invalid field type") +} + // List of bsiGroup types. const ( bsiGroupTypeInt = "int" diff --git a/field_test.go b/field_test.go index 10e4d9a97..e3719a35a 100644 --- a/field_test.go +++ b/field_test.go @@ -24,7 +24,7 @@ import ( // Ensure field can open and retrieve a view. func TestField_CreateViewIfNotExists(t *testing.T) { - f := test.MustOpenField() + f := test.MustOpenField(pilosa.FieldOptions{}) defer f.Close() // Create view. @@ -50,10 +50,7 @@ func TestField_CreateViewIfNotExists(t *testing.T) { // Ensure field can set its time quantum. func TestField_SetTimeQuantum(t *testing.T) { - fo := pilosa.FieldOptions{ - Type: "time", - } - f := test.MustOpenField(pilosa.OptFieldFieldOptions(fo)) + f := test.MustOpenField(pilosa.FieldOptions{Type: pilosa.FieldTypeTime}) defer f.Close() // Set & retrieve time quantum. @@ -208,7 +205,7 @@ func TestField_NameRestriction(t *testing.T) { if err != nil { panic(err) } - field, err := pilosa.NewField(path, "i", ".meta") + field, err := pilosa.NewField(path, "i", ".meta", pilosa.FieldOptions{}) if field != nil { t.Fatalf("unexpected field name %s", err) } @@ -240,13 +237,13 @@ func TestField_NameValidation(t *testing.T) { panic(err) } for _, name := range validFieldNames { - _, err := pilosa.NewField(path, "i", name) + _, err := pilosa.NewField(path, "i", name, pilosa.FieldOptions{}) if err != nil { t.Fatalf("unexpected field name: %s %s", name, err) } } for _, name := range invalidFieldNames { - _, err := pilosa.NewField(path, "i", name) + _, err := pilosa.NewField(path, "i", name, pilosa.FieldOptions{}) if err == nil { t.Fatalf("expected error on field name: %s", name) } @@ -255,7 +252,7 @@ func TestField_NameValidation(t *testing.T) { // Ensure field can open and retrieve a view. func TestField_DeleteView(t *testing.T) { - f := test.MustOpenField() + f := test.MustOpenField(pilosa.FieldOptions{}) defer f.Close() viewName := pilosa.ViewStandard + "_v" diff --git a/fragment.go b/fragment.go index 28978d337..4fc24d71e 100644 --- a/fragment.go +++ b/fragment.go @@ -117,8 +117,8 @@ func NewFragment(path, index, field, view string, slice uint64) *Fragment { field: field, view: view, slice: slice, - CacheType: defaultCacheType, - CacheSize: defaultCacheSize, + CacheType: DefaultCacheType, + CacheSize: DefaultCacheSize, Logger: NopLogger, MaxOpN: defaultFragmentMaxOpN, diff --git a/fragment_internal_test.go b/fragment_internal_test.go index 6c733ba4c..e7a77dafb 100644 --- a/fragment_internal_test.go +++ b/fragment_internal_test.go @@ -1245,7 +1245,7 @@ func mustOpenFragment(index, field, view string, slice uint64, cacheType string) file.Close() if cacheType == "" { - cacheType = defaultCacheType + cacheType = DefaultCacheType } f := NewFragment(file.Name(), index, field, view, slice) diff --git a/http/client.go b/http/client.go index 5090072a3..378413c2f 100644 --- a/http/client.go +++ b/http/client.go @@ -334,8 +334,8 @@ func (c *InternalClient) EnsureIndex(ctx context.Context, name string, options p return err } -func (c *InternalClient) EnsureField(ctx context.Context, indexName string, fieldName string, options pilosa.FieldOptions) error { - err := c.CreateField(ctx, indexName, fieldName, options) +func (c *InternalClient) EnsureField(ctx context.Context, indexName string, fieldName string) error { + err := c.CreateField(ctx, indexName, fieldName) if err == nil || err == pilosa.ErrFieldExists { return nil } @@ -620,14 +620,15 @@ func (c *InternalClient) backupSliceNode(ctx context.Context, index, field strin } // CreateField creates a new field on the server. -func (c *InternalClient) CreateField(ctx context.Context, index, field string, opt pilosa.FieldOptions) error { +func (c *InternalClient) CreateField(ctx context.Context, index, field string) error { if index == "" { return pilosa.ErrIndexRequired } + // TODO: remove buf completely? (depends on whether importer needs to create specific field types) // Encode query request. buf, err := json.Marshal(&postFieldRequest{ - Options: opt, + //Options: opt, }) if err != nil { return errors.Wrap(err, "marshaling") diff --git a/http/handler.go b/http/handler.go index a079ae479..6580ce8f0 100644 --- a/http/handler.go +++ b/http/handler.go @@ -457,6 +457,17 @@ func (p *postIndexRequest) UnmarshalJSON(b []byte) error { return nil } +func getValidOptions(option interface{}) []string { + validOptions := []string{} + val := reflect.ValueOf(option) + for i := 0; i < val.Type().NumField(); i++ { + jsonTag := val.Type().Field(i).Tag.Get("json") + s := strings.Split(jsonTag, ",") + validOptions = append(validOptions, s[0]) + } + return validOptions +} + // Raise errors for any unknown key func validateOptions(data map[string]interface{}, validIndexOptions []string) error { for k, v := range data { @@ -597,7 +608,9 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { // Decode request. var req postFieldRequest - err := json.NewDecoder(r.Body).Decode(&req) + dec := json.NewDecoder(r.Body) + dec.DisallowUnknownFields() + err := dec.Decode(&req) if err == io.EOF { // If no data was provided (EOF), we still create the field // with default values. @@ -605,7 +618,25 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { http.Error(w, err.Error(), http.StatusBadRequest) return } - _, err = h.API.CreateField(r.Context(), indexName, fieldName, req.Options) + + // Validate field options. + if err := req.Options.validate(); err != nil { + http.Error(w, err.Error(), http.StatusNotAcceptable) + return + } + + // Convert json options into functional options. + var fos []pilosa.FieldOption + switch req.Options.Type { + case pilosa.FieldTypeSet: + fos = append(fos, pilosa.OptFieldTypeSet(*req.Options.CacheType, *req.Options.CacheSize)) + case pilosa.FieldTypeInt: + fos = append(fos, pilosa.OptFieldTypeInt(*req.Options.Min, *req.Options.Max)) + case pilosa.FieldTypeTime: + fos = append(fos, pilosa.OptFieldTypeTime(*req.Options.TimeQuantum)) + } + + _, err = h.API.CreateField(r.Context(), indexName, fieldName, fos...) if err != nil { switch errors.Cause(err) { case pilosa.ErrIndexNotFound: @@ -623,51 +654,80 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { } } -type _postFieldRequest postFieldRequest - -// Custom Unmarshal JSON to validate request body when creating a new field. If there's new FieldOptions, -// adding it to validFieldOptions to make sure the new option is validated, otherwise the request will be failed -func (p *postFieldRequest) UnmarshalJSON(b []byte) error { - // m is an overflow map used to capture additional, unexpected keys. - m := make(map[string]interface{}) - if err := json.Unmarshal(b, &m); err != nil { - return errors.Wrap(err, "unmarshaling unexpected keys") - } - - validFieldOptions := getValidOptions(pilosa.FieldOptions{}) - err := validateOptions(m, validFieldOptions) - if err != nil { - return err - } - - // Unmarshal expected values. - var _p _postFieldRequest - if err := json.Unmarshal(b, &_p); err != nil { - return errors.Wrap(err, "unmarshalling expected keys") - } - - p.Options = _p.Options - return nil - -} - -func getValidOptions(option interface{}) []string { - validOptions := []string{} - val := reflect.ValueOf(option) - for i := 0; i < val.Type().NumField(); i++ { - jsonTag := val.Type().Field(i).Tag.Get("json") - s := strings.Split(jsonTag, ",") - validOptions = append(validOptions, s[0]) - } - return validOptions -} - type postFieldRequest struct { - Options pilosa.FieldOptions `json:"options"` + Options fieldOptions `json:"options"` } type postFieldResponse struct{} +// fieldOptions tracks pilosa.FieldOptions. It is made up of pointers to values, +// and used for input validation. +type fieldOptions struct { + Type string `json:"type,omitempty"` + CacheType *string `json:"cacheType,omitempty"` + CacheSize *uint32 `json:"cacheSize,omitempty"` + Min *int64 `json:"min,omitempty"` + Max *int64 `json:"max,omitempty"` + TimeQuantum *pilosa.TimeQuantum `json:"timeQuantum,omitempty"` + Keys *bool `json:"keys,omitempty"` +} + +func (o *fieldOptions) validate() error { + // Pointers to default values. + defaultCacheType := pilosa.DefaultCacheType + defaultCacheSize := uint32(pilosa.DefaultCacheSize) + + switch o.Type { + case pilosa.FieldTypeSet, "": + // Because FieldTypeSet is the default, its arguments are + // not required. Instead, the defaults are applied whenever + // a value does not exist. + if o.Type == "" { + o.Type = pilosa.FieldTypeSet + } + if o.CacheType == nil { + o.CacheType = &defaultCacheType + } + if o.CacheSize == nil { + o.CacheSize = &defaultCacheSize + } + if o.Min != nil { + return errors.New("min does not apply to field type set") + } else if o.Max != nil { + return errors.New("max does not apply to field type set") + } else if o.TimeQuantum != nil { + return errors.New("timeQuantum does not apply to field type set") + } + case pilosa.FieldTypeInt: + if o.CacheType != nil { + return errors.New("cacheType does not apply to field type int") + } else if o.CacheSize != nil { + return errors.New("cacheSize does not apply to field type int") + } else if o.Min == nil { + return errors.New("min is required for field type int") + } else if o.Max == nil { + return errors.New("max is required for field type int") + } else if o.TimeQuantum != nil { + return errors.New("timeQuantum does not apply to field type int") + } + case pilosa.FieldTypeTime: + if o.CacheType != nil { + return errors.New("cacheType does not apply to field type time") + } else if o.CacheSize != nil { + return errors.New("cacheSize does not apply to field type time") + } else if o.Min != nil { + return errors.New("min does not apply to field type time") + } else if o.Max != nil { + return errors.New("max does not apply to field type time") + } else if o.TimeQuantum == nil { + return errors.New("timeQuantum is required for field type time") + } + default: + return errors.Errorf("invalid field type: %s", o.Type) + } + return nil +} + // handleDeleteField handles DELETE /field request. func (h *Handler) handleDeleteField(w http.ResponseWriter, r *http.Request) { if !validHeaderAcceptJSON(r.Header) { diff --git a/http/handler_internal_test.go b/http/handler_internal_test.go index fa7c23060..f7f95aeb0 100644 --- a/http/handler_internal_test.go +++ b/http/handler_internal_test.go @@ -15,6 +15,7 @@ package http import ( + "bytes" "encoding/json" "reflect" "testing" @@ -59,31 +60,121 @@ func TestPostIndexRequestUnmarshalJSON(t *testing.T) { // Test custom UnmarshalJSON for postFieldRequest object func TestPostFieldRequestUnmarshalJSON(t *testing.T) { + foo := "foo" tests := []struct { json string expected postFieldRequest err string }{ - {json: `{"options": {}}`, expected: postFieldRequest{Options: pilosa.FieldOptions{}}}, - {json: `{"options": 4}`, err: "options is not map[string]interface{}"}, - {json: `{"option": {}}`, err: "Unknown key: option:map[]"}, - {json: `{"options": {"badKey": "test"}}`, err: "Unknown key: badKey:test"}, - {json: `{"options": {"inverseEnabled": true}}`, err: "Unknown key: inverseEnabled:true"}, - {json: `{"options": {"cacheType": "type"}}`, expected: postFieldRequest{Options: pilosa.FieldOptions{CacheType: "type"}}}, - {json: `{"options": {"inverse": true, "cacheType": "type"}}`, err: "Unknown key: inverse:true"}, + {json: `{"options": {}}`, expected: postFieldRequest{}}, + {json: `{"options": 4}`, err: "json: cannot unmarshal number into Go struct field postFieldRequest.options of type http.fieldOptions"}, + {json: `{"option": {}}`, err: `json: unknown field "option"`}, + {json: `{"options": {"badKey": "test"}}`, err: `json: unknown field "badKey"`}, + {json: `{"options": {"inverseEnabled": true}}`, err: `json: unknown field "inverseEnabled"`}, + {json: `{"options": {"cacheType": "foo"}}`, expected: postFieldRequest{Options: fieldOptions{CacheType: &foo}}}, + {json: `{"options": {"inverse": true, "cacheType": "foo"}}`, err: `json: unknown field "inverse"`}, } - for _, test := range tests { + for i, test := range tests { actual := &postFieldRequest{} - err := json.Unmarshal([]byte(test.json), actual) + dec := json.NewDecoder(bytes.NewReader([]byte(test.json))) + dec.DisallowUnknownFields() + err := dec.Decode(actual) if err != nil { if test.err == "" || test.err != err.Error() { - t.Errorf("expected error: %v, but got result: %v", test.err, err) + t.Errorf("test %d: expected error: %v, but got result: %v", i, test.err, err) } } if test.err == "" { if !reflect.DeepEqual(*actual, test.expected) { - t.Errorf("expected: %v, but got: %v", test.expected, *actual) + t.Errorf("test %d: expected: %v, but got: %v", i, test.expected, *actual) + } + } + + } +} + +func stringPtr(s string) *string { + return &s +} + +func int64Ptr(i int64) *int64 { + return &i +} + +// Test fieldOption validation. +func TestFieldOptionValidation(t *testing.T) { + //foo := "foo" + //set := "set" + timeQuantum := pilosa.TimeQuantum("YMD") + defaultCacheSize := uint32(pilosa.DefaultCacheSize) + tests := []struct { + json string + expected postFieldRequest + err string + }{ + // FieldType: Set + {json: `{"options": {}}`, expected: postFieldRequest{Options: fieldOptions{ + Type: pilosa.FieldTypeSet, + CacheType: stringPtr(pilosa.DefaultCacheType), + CacheSize: &defaultCacheSize, + }}}, + {json: `{"options": {"type": "set"}}`, expected: postFieldRequest{Options: fieldOptions{ + Type: pilosa.FieldTypeSet, + CacheType: stringPtr(pilosa.DefaultCacheType), + CacheSize: &defaultCacheSize, + }}}, + {json: `{"options": {"type": "set", "cacheType": "lru"}}`, expected: postFieldRequest{Options: fieldOptions{ + Type: pilosa.FieldTypeSet, + CacheType: stringPtr("lru"), + CacheSize: &defaultCacheSize, + }}}, + {json: `{"options": {"type": "set", "min": 0}}`, err: "min does not apply to field type set"}, + {json: `{"options": {"type": "set", "max": 100}}`, err: "max does not apply to field type set"}, + {json: `{"options": {"type": "set", "timeQuantum": "YMD"}}`, err: "timeQuantum does not apply to field type set"}, + + // FieldType: Int + {json: `{"options": {"type": "int"}}`, err: "min is required for field type int"}, + {json: `{"options": {"type": "int", "min": 0}}`, err: "max is required for field type int"}, + {json: `{"options": {"type": "int", "min": 0, "max": 1000}}`, expected: postFieldRequest{Options: fieldOptions{ + Type: pilosa.FieldTypeInt, + Min: int64Ptr(0), + Max: int64Ptr(1000), + }}}, + {json: `{"options": {"type": "int", "min": 0, "max": 1000, "cacheType": "ranked"}}`, err: "cacheType does not apply to field type int"}, + {json: `{"options": {"type": "int", "min": 0, "max": 1000, "cacheSize": 1000}}`, err: "cacheSize does not apply to field type int"}, + {json: `{"options": {"type": "int", "min": 0, "max": 1000, "timeQuantum": "YMD"}}`, err: "timeQuantum does not apply to field type int"}, + + // FieldType: Time + {json: `{"options": {"type": "time"}}`, err: "timeQuantum is required for field type time"}, + {json: `{"options": {"type": "time", "timeQuantum": "YMD"}}`, expected: postFieldRequest{Options: fieldOptions{ + Type: pilosa.FieldTypeTime, + TimeQuantum: &timeQuantum, + }}}, + {json: `{"options": {"type": "time", "timeQuantum": "YMD", "min": 0}}`, err: "min does not apply to field type time"}, + {json: `{"options": {"type": "time", "timeQuantum": "YMD", "max": 1000}}`, err: "max does not apply to field type time"}, + {json: `{"options": {"type": "time", "timeQuantum": "YMD", "cacheType": "ranked"}}`, err: "cacheType does not apply to field type time"}, + {json: `{"options": {"type": "time", "timeQuantum": "YMD", "cacheSize": 1000}}`, err: "cacheSize does not apply to field type time"}, + } + for i, test := range tests { + actual := &postFieldRequest{} + dec := json.NewDecoder(bytes.NewReader([]byte(test.json))) + dec.DisallowUnknownFields() + err := dec.Decode(actual) + if err != nil { + t.Errorf("test %d: %v", i, err) + } + + // Validate field options. + if err := actual.Options.validate(); err != nil { + if test.err == "" || test.err != err.Error() { + t.Errorf("test %d: expected error: %v, but got result: %v", i, test.err, err) + } + } + + if test.err == "" { + if !reflect.DeepEqual(*actual, test.expected) { + t.Errorf("test %d: expected: %v, but got: %v", i, test.expected, *actual) } } diff --git a/index.go b/index.go index 68c48c829..10e954aef 100644 --- a/index.go +++ b/index.go @@ -335,7 +335,7 @@ func (i *Index) createField(name string, opt FieldOptions) (*Field, error) { } func (i *Index) newField(path, name string) (*Field, error) { - f, err := NewField(path, i.name, name) + f, err := NewField(path, i.name, name, FieldOptions{}) // TODO: NewField should be un-exported along with FieldOptions if err != nil { return nil, err } diff --git a/server/cluster_test.go b/server/cluster_test.go index 73de82279..b9a1917a9 100644 --- a/server/cluster_test.go +++ b/server/cluster_test.go @@ -54,7 +54,7 @@ func TestMain_SendReceiveMessage(t *testing.T) { // Create indexes and fields on one node. if err := client0.CreateIndex(context.Background(), "i", pilosa.IndexOptions{}); err != nil && err != pilosa.ErrIndexExists { t.Fatal(err) - } else if err := client0.CreateField(context.Background(), "i", "f", pilosa.FieldOptions{}); err != nil { + } else if err := client0.CreateField(context.Background(), "i", "f"); err != nil { t.Fatal(err) } @@ -209,7 +209,7 @@ func TestClusterResize_AddNode(t *testing.T) { // Create indexes and fields on one node. if err := client0.CreateIndex(context.Background(), "i", pilosa.IndexOptions{}); err != nil && err != pilosa.ErrIndexExists { t.Fatal(err) - } else if err := client0.CreateField(context.Background(), "i", "f", pilosa.FieldOptions{}); err != nil { + } else if err := client0.CreateField(context.Background(), "i", "f"); err != nil { t.Fatal(err) } @@ -253,7 +253,7 @@ func TestClusterResize_AddNode(t *testing.T) { // Create indexes and fields on one node. if err := client0.CreateIndex(context.Background(), "i", pilosa.IndexOptions{}); err != nil && err != pilosa.ErrIndexExists { t.Fatal(err) - } else if err := client0.CreateField(context.Background(), "i", "f", pilosa.FieldOptions{}); err != nil { + } else if err := client0.CreateField(context.Background(), "i", "f"); err != nil { t.Fatal(err) } @@ -305,7 +305,7 @@ func TestClusterResize_AddNode(t *testing.T) { // Create indexes and fields on one node. if err := client0.CreateIndex(context.Background(), "i", pilosa.IndexOptions{}); err != nil && err != pilosa.ErrIndexExists { t.Fatal(err) - } else if err := client0.CreateField(context.Background(), "i", "f", pilosa.FieldOptions{}); err != nil { + } else if err := client0.CreateField(context.Background(), "i", "f"); err != nil { t.Fatal(err) } @@ -458,7 +458,7 @@ func TestClusterResize_RemoveNode(t *testing.T) { // Create indexes and fields on one node. if err := client0.CreateIndex(context.Background(), "i", pilosa.IndexOptions{}); err != nil && err != pilosa.ErrIndexExists { t.Fatal(err) - } else if err := client0.CreateField(context.Background(), "i", "f", pilosa.FieldOptions{}); err != nil { + } else if err := client0.CreateField(context.Background(), "i", "f"); err != nil { t.Fatal(err) } diff --git a/server/server_test.go b/server/server_test.go index 58883286f..b3067db87 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -54,7 +54,7 @@ func TestMain_Set_Quick(t *testing.T) { if err := client.CreateIndex(context.Background(), "i", pilosa.IndexOptions{}); err != nil && err != pilosa.ErrIndexExists { t.Fatal(err) } - if err := client.CreateField(context.Background(), "i", cmd.Field, pilosa.FieldOptions{}); err != nil && err != pilosa.ErrFieldExists { + if err := client.CreateField(context.Background(), "i", cmd.Field); err != nil && err != pilosa.ErrFieldExists { t.Fatal(err) } if _, err := m.Query("i", "", fmt.Sprintf(`Set(%d, %s=%d)`, cmd.ColumnID, cmd.Field, cmd.ID)); err != nil { @@ -123,11 +123,11 @@ func TestMain_SetRowAttrs(t *testing.T) { client := m.Client() if err := client.CreateIndex(context.Background(), "i", pilosa.IndexOptions{}); err != nil && err != pilosa.ErrIndexExists { t.Fatal(err) - } else if err := client.CreateField(context.Background(), "i", "x", pilosa.FieldOptions{}); err != nil { + } else if err := client.CreateField(context.Background(), "i", "x"); err != nil { t.Fatal(err) - } else if err := client.CreateField(context.Background(), "i", "z", pilosa.FieldOptions{}); err != nil { + } else if err := client.CreateField(context.Background(), "i", "z"); err != nil { t.Fatal(err) - } else if err := client.CreateField(context.Background(), "i", "neg", pilosa.FieldOptions{}); err != nil { + } else if err := client.CreateField(context.Background(), "i", "neg"); err != nil { t.Fatal(err) } @@ -200,7 +200,7 @@ func TestMain_SetColumnAttrs(t *testing.T) { client := m.Client() if err := client.CreateIndex(context.Background(), "i", pilosa.IndexOptions{}); err != nil && err != pilosa.ErrIndexExists { t.Fatal(err) - } else if err := client.CreateField(context.Background(), "i", "x", pilosa.FieldOptions{}); err != nil { + } else if err := client.CreateField(context.Background(), "i", "x"); err != nil { t.Fatal(err) } @@ -271,9 +271,10 @@ func TestMain_RecalculateHashes(t *testing.T) { if err := client0.CreateIndex(context.Background(), "i", pilosa.IndexOptions{}); err != nil && err != pilosa.ErrIndexExists { t.Fatal("create index:", err) } - if err := client0.CreateField(context.Background(), "i", "f", pilosa.FieldOptions{CacheType: "ranked"}); err != nil { + if err := client0.CreateField(context.Background(), "i", "f"); err != nil { t.Fatal("create field:", err) } + return // Set some columns data := []string{} diff --git a/server_test.go b/server_test.go index 402d4de7d..9f076369b 100644 --- a/server_test.go +++ b/server_test.go @@ -33,7 +33,8 @@ func TestMonitorAntiEntropy(t *testing.T) { if err != nil { t.Fatalf("creating index: %v", err) } - err = client.CreateField(context.Background(), "balh", "fralh", pilosa.FieldOptions{}) + + err = client.CreateField(context.Background(), "balh", "fralh") if err != nil { t.Fatalf("creating field: %v", err) } diff --git a/test/field.go b/test/field.go index 9a83be2de..345deadf8 100644 --- a/test/field.go +++ b/test/field.go @@ -28,12 +28,12 @@ type Field struct { } // NewField returns a new instance of Field d/0. -func NewField(opt ...pilosa.FieldOption) *Field { +func NewField(options pilosa.FieldOptions) *Field { path, err := ioutil.TempDir("", "pilosa-field-") if err != nil { panic(err) } - field, err := pilosa.NewField(path, "i", "f", opt...) + field, err := pilosa.NewField(path, "i", "f", options) if err != nil { panic(err) } @@ -41,8 +41,8 @@ func NewField(opt ...pilosa.FieldOption) *Field { } // MustOpenField returns a new, opened field at a temporary path. Panic on error. -func MustOpenField(opt ...pilosa.FieldOption) *Field { - f := NewField(opt...) +func MustOpenField(options pilosa.FieldOptions) *Field { + f := NewField(options) if err := f.Open(); err != nil { panic(err) } @@ -63,7 +63,7 @@ func (f *Field) Reopen() error { } path, index, name := f.Path(), f.Index(), f.Name() - f.Field, err = pilosa.NewField(path, index, name) + f.Field, err = pilosa.NewField(path, index, name, pilosa.FieldOptions{}) if err != nil { return err } @@ -76,7 +76,7 @@ func (f *Field) Reopen() error { // Ensure field can set its cache func TestField_SetCacheSize(t *testing.T) { - f := MustOpenField() + f := MustOpenField(pilosa.FieldOptions{}) defer f.Close() cacheSize := uint32(100) diff --git a/view.go b/view.go index 428fc6f54..732ecd485 100644 --- a/view.go +++ b/view.go @@ -73,7 +73,7 @@ func NewView(path, index, field, name string, cacheSize uint32) *View { name: name, cacheSize: cacheSize, - cacheType: defaultCacheType, + cacheType: DefaultCacheType, fragments: make(map[uint64]*Fragment), broadcaster: NopBroadcaster, diff --git a/view_internal_test.go b/view_internal_test.go index 48df030db..d0e8bfdd1 100644 --- a/view_internal_test.go +++ b/view_internal_test.go @@ -26,7 +26,7 @@ func mustOpenView(index, field, name string) *View { panic(err) } - v := NewView(path, index, field, name, defaultCacheSize) + v := NewView(path, index, field, name, DefaultCacheSize) if err := v.open(); err != nil { panic(err) }