Don't allow an int and decimal fields to be created with keys=true (#118)

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 {

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")
 			}

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 {

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.

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

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
 		}