diff --git a/field.go b/field.go index 618a3815d..e040b4441 100644 --- a/field.go +++ b/field.go @@ -1709,6 +1709,30 @@ type FieldOptions struct { ForeignIndex string `json:"foreignIndex"` } +// newFieldOptions returns a new instance of FieldOptions +// with applied and validated functional options. +func newFieldOptions(opts ...FieldOption) (*FieldOptions, error) { + fo := FieldOptions{} + for _, opt := range opts { + err := opt(&fo) + if err != nil { + return nil, err + } + } + + if fo.Keys { + switch fo.Type { + case FieldTypeInt: + return nil, ErrIntFieldWithKeys + + case FieldTypeDecimal: + return nil, ErrDecimalFieldWithKeys + } + } + + return &fo, nil +} + // applyDefaultOptions updates FieldOptions with the default // values if o does not contain a valid type. func applyDefaultOptions(o *FieldOptions) *FieldOptions { diff --git a/holder.go b/holder.go index 4fa554f1a..e7577f8ed 100644 --- a/holder.go +++ b/holder.go @@ -392,7 +392,7 @@ func (h *Holder) applySchema(schema *Schema) error { } // Create fields that don't exist. for _, f := range index.Fields { - field, err := idx.createFieldIfNotExists(f.Name, f.Options) + field, err := idx.createFieldIfNotExists(f.Name, &f.Options) if err != nil { return errors.Wrap(err, "creating field") } diff --git a/index.go b/index.go index 51e062823..b3d97a61f 100644 --- a/index.go +++ b/index.go @@ -250,7 +250,7 @@ fileLoop: // openExistenceField gets or creates the existence field and associates it to the index. func (i *Index) openExistenceField() error { - f, err := i.createFieldIfNotExists(existenceFieldName, FieldOptions{CacheType: CacheTypeNone, CacheSize: 0}) + f, err := i.createFieldIfNotExists(existenceFieldName, &FieldOptions{CacheType: CacheTypeNone, CacheSize: 0}) if err != nil { return errors.Wrap(err, "creating existence field") } @@ -401,13 +401,10 @@ func (i *Index) CreateField(name string, opts ...FieldOption) (*Field, error) { return nil, newConflictError(ErrFieldExists) } - // Apply functional options. - fo := FieldOptions{} - for _, opt := range opts { - err := opt(&fo) - if err != nil { - return nil, errors.Wrap(err, "applying option") - } + // Apply and validate functional options. + fo, err := newFieldOptions(opts...) + if err != nil { + return nil, errors.Wrap(err, "applying option") } return i.createField(name, fo) @@ -428,19 +425,16 @@ func (i *Index) CreateFieldIfNotExists(name string, opts ...FieldOption) (*Field return f, nil } - // Apply functional options. - fo := FieldOptions{} - for _, opt := range opts { - err := opt(&fo) - if err != nil { - return nil, errors.Wrap(err, "applying option") - } + // Apply and validate functional options. + fo, err := newFieldOptions(opts...) + if err != nil { + return nil, errors.Wrap(err, "applying option") } return i.createField(name, fo) } -func (i *Index) createFieldIfNotExists(name string, opt FieldOptions) (*Field, error) { +func (i *Index) createFieldIfNotExists(name string, opt *FieldOptions) (*Field, error) { i.mu.Lock() defer i.mu.Unlock() @@ -452,7 +446,7 @@ func (i *Index) createFieldIfNotExists(name string, opt FieldOptions) (*Field, e return i.createField(name, opt) } -func (i *Index) createField(name string, opt FieldOptions) (*Field, error) { +func (i *Index) createField(name string, opt *FieldOptions) (*Field, error) { if name == "" { return nil, errors.New("field name required") } else if opt.CacheType != "" && !isValidCacheType(opt.CacheType) { @@ -469,7 +463,7 @@ func (i *Index) createField(name string, opt FieldOptions) (*Field, error) { // up a foreign index. f.holder = i.holder - f.setOptions(&opt) + f.setOptions(opt) // Open field. if err := f.Open(); err != nil { diff --git a/index_test.go b/index_test.go index 77736331c..6a5811c03 100644 --- a/index_test.go +++ b/index_test.go @@ -185,6 +185,30 @@ func TestIndex_CreateField(t *testing.T) { }) */ }) + + t.Run("WithKeys", func(t *testing.T) { + // Don't allow an int field to be created with keys=true + t.Run("IntField", func(t *testing.T) { + index := test.MustOpenIndex() + defer index.Close() + + _, err := index.CreateField("f", pilosa.OptFieldTypeInt(-1, 1), pilosa.OptFieldKeys()) + if errors.Cause(err) != pilosa.ErrIntFieldWithKeys { + t.Fatal("int field cannot be created with keys=true") + } + }) + + // Don't allow a decimal field to be created with keys=true + t.Run("DecimalField", func(t *testing.T) { + index := test.MustOpenIndex() + defer index.Close() + + _, err := index.CreateField("f", pilosa.OptFieldTypeDecimal(1, -1, 1), pilosa.OptFieldKeys()) + if errors.Cause(err) != pilosa.ErrDecimalFieldWithKeys { + t.Fatal("decimal field cannot be created with keys=true") + } + }) + }) } // Ensure index can delete a field. diff --git a/pilosa.go b/pilosa.go index 3f1e8545b..369b72998 100644 --- a/pilosa.go +++ b/pilosa.go @@ -71,6 +71,9 @@ var ( ErrNotImplemented = errors.New("not implemented") ErrFieldsArgumentRequired = errors.New("fields argument required") ErrExpectedFieldListArgument = errors.New("expected field list argument") + + ErrIntFieldWithKeys = errors.New("int field cannot be created with 'keys=true' option") + ErrDecimalFieldWithKeys = errors.New("decimal field cannot be created with 'keys=true' option") ) // apiMethodNotAllowedError wraps an error value indicating that a particular diff --git a/server.go b/server.go index 19e20b074..455e4dd59 100644 --- a/server.go +++ b/server.go @@ -682,7 +682,7 @@ func (s *Server) receiveMessage(m Message) error { return fmt.Errorf("local index not found: %s", obj.Index) } opt := obj.Meta - _, err := idx.createFieldIfNotExists(obj.Field, *opt) + _, err := idx.createFieldIfNotExists(obj.Field, opt) if err != nil { return err }