From 028e95d914942fbb25237f575c46e74d0ca0a517 Mon Sep 17 00:00:00 2001 From: Travis Turner Date: Tue, 26 Jun 2018 10:37:56 -0500 Subject: [PATCH] Allow a single functional option for field options. Move field type specific validation to functional options. --- api.go | 13 +++++++------ ctl/import.go | 1 - field.go | 25 ++++++------------------- http/handler.go | 10 +++++----- http/handler_internal_test.go | 2 -- index.go | 5 ----- test/holder.go | 2 +- 7 files changed, 19 insertions(+), 39 deletions(-) diff --git a/api.go b/api.go index dd0d5fca4..66add8863 100644 --- a/api.go +++ b/api.go @@ -244,17 +244,18 @@ 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, opts ...FieldOption) (*Field, error) { +// This method currently only takes a single functional option, but that may be +// changed in the future to support multiple options. +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") } + // Apply functional option. fo := FieldOptions{} - for _, opt := range opts { - err := opt(&fo) - if err != nil { - return nil, errors.Wrap(err, "applying option") - } + err := opts(&fo) + if err != nil { + return nil, errors.Wrap(err, "applying option") } // Find index. diff --git a/ctl/import.go b/ctl/import.go index ad4befbfd..05659fa27 100644 --- a/ctl/import.go +++ b/ctl/import.go @@ -42,7 +42,6 @@ type ImportCommand struct { // Options for index & field to be created if they don't exist IndexOptions pilosa.IndexOptions - //FieldOptions pilosa.FieldOptions // CreateSchema ensures the schema exists before import CreateSchema bool diff --git a/field.go b/field.go index 93c3d5827..4fd684682 100644 --- a/field.go +++ b/field.go @@ -90,6 +90,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 ErrInvalidBSIGroupRange + } fo.Type = FieldTypeInt fo.Min = min fo.Max = max @@ -102,6 +105,9 @@ func OptFieldTypeTime(timeQuantum TimeQuantum) FieldOption { if fo.Type != "" { return errors.Errorf("field type is already set to: %s", fo.Type) } + if !timeQuantum.Valid() { + return ErrInvalidTimeQuantum + } fo.Type = FieldTypeTime fo.TimeQuantum = timeQuantum return nil @@ -1075,25 +1081,6 @@ func applyDefaultOptions(o FieldOptions) FieldOptions { return o } -// Validate ensures that FieldOption values are valid. -func (o *FieldOptions) Validate() error { - switch o.Type { - case FieldTypeSet, "": - // TODO: cacheType, cacheSize validation - case FieldTypeInt: - if o.Min > o.Max { - return ErrInvalidBSIGroupRange - } - case FieldTypeTime: - if o.TimeQuantum == "" || !o.TimeQuantum.Valid() { - return ErrInvalidTimeQuantum - } - default: - return errors.New("invalid field type") - } - return nil -} - // Encode converts o into its internal representation. func (o *FieldOptions) Encode() *internal.FieldOptions { return encodeFieldOptions(o) diff --git a/http/handler.go b/http/handler.go index 6580ce8f0..bbe784f69 100644 --- a/http/handler.go +++ b/http/handler.go @@ -626,17 +626,17 @@ func (h *Handler) handlePostField(w http.ResponseWriter, r *http.Request) { } // Convert json options into functional options. - var fos []pilosa.FieldOption + var fos pilosa.FieldOption switch req.Options.Type { case pilosa.FieldTypeSet: - fos = append(fos, pilosa.OptFieldTypeSet(*req.Options.CacheType, *req.Options.CacheSize)) + fos = pilosa.OptFieldTypeSet(*req.Options.CacheType, *req.Options.CacheSize) case pilosa.FieldTypeInt: - fos = append(fos, pilosa.OptFieldTypeInt(*req.Options.Min, *req.Options.Max)) + fos = pilosa.OptFieldTypeInt(*req.Options.Min, *req.Options.Max) case pilosa.FieldTypeTime: - fos = append(fos, pilosa.OptFieldTypeTime(*req.Options.TimeQuantum)) + fos = pilosa.OptFieldTypeTime(*req.Options.TimeQuantum) } - _, err = h.API.CreateField(r.Context(), indexName, fieldName, fos...) + _, err = h.API.CreateField(r.Context(), indexName, fieldName, fos) if err != nil { switch errors.Cause(err) { case pilosa.ErrIndexNotFound: diff --git a/http/handler_internal_test.go b/http/handler_internal_test.go index f7f95aeb0..071ac1926 100644 --- a/http/handler_internal_test.go +++ b/http/handler_internal_test.go @@ -104,8 +104,6 @@ func int64Ptr(i int64) *int64 { // Test fieldOption validation. func TestFieldOptionValidation(t *testing.T) { - //foo := "foo" - //set := "set" timeQuantum := pilosa.TimeQuantum("YMD") defaultCacheSize := uint32(pilosa.DefaultCacheSize) tests := []struct { diff --git a/index.go b/index.go index 10e954aef..a118b1808 100644 --- a/index.go +++ b/index.go @@ -301,11 +301,6 @@ func (i *Index) createField(name string, opt FieldOptions) (*Field, error) { return nil, ErrInvalidCacheType } - // Validate options. - if err := opt.Validate(); err != nil { - return nil, errors.Wrap(err, "validating options") - } - // Initialize field. f, err := i.newField(i.FieldPath(name), name) if err != nil { diff --git a/test/holder.go b/test/holder.go index 7bae8afaa..648d910ed 100644 --- a/test/holder.go +++ b/test/holder.go @@ -92,7 +92,7 @@ func (h *Holder) MustCreateFieldIfNotExists(index, field string) *Field { // MustCreateRankedFragmentIfNotExists returns a given fragment with a ranked cache. Panic on error. func (h *Holder) MustCreateRankedFragmentIfNotExists(index, field, view string, slice uint64) *Fragment { idx := h.MustCreateIndexIfNotExists(index, pilosa.IndexOptions{}) - f, err := idx.CreateFieldIfNotExists(field, pilosa.FieldOptions{CacheType: pilosa.CacheTypeRanked}) + f, err := idx.CreateFieldIfNotExists(field, pilosa.FieldOptions{}) if err != nil { panic(err) }