From 6028b50b067874a2e117cf708fcc2f5ad1e2eedd Mon Sep 17 00:00:00 2001 From: Michael Baird Date: Wed, 28 Jun 2017 16:33:42 -0500 Subject: [PATCH] Validate incoming input definitions through the InputDefinitionInfo Validate rather than through Encode and piecemeal throughout the code. --- handler.go | 28 ++------ handler_test.go | 2 +- input_definition.go | 140 +++++++++++++++++++++++++-------------- input_definition_test.go | 110 ++++++++++++++++++------------ pilosa.go | 23 ++++--- 5 files changed, 174 insertions(+), 129 deletions(-) diff --git a/handler.go b/handler.go index 7e1ce9066..ed34f19d0 100644 --- a/handler.go +++ b/handler.go @@ -1520,34 +1520,16 @@ func (h *Handler) handlePostInputDefinition(w http.ResponseWriter, r *http.Reque return } - // TODO: validation before/after encode? + // validation definition before/after encode? + if err := req.Validate(index.ColumnLabel()); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } // Encode InputDefinition to its internal representation. def := req.Encode() - /* - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - */ def.Name = inputDefName - // Validate columnLabel and duplicate primaryKey. - numPrimaryKey := 0 - for _, field := range def.Fields { - if field.PrimaryKey { - numPrimaryKey += 1 - if field.Name != index.columnLabel { - http.Error(w, ErrInputDefinitionColumnLabel.Error(), http.StatusBadRequest) - return - } - } - } - if numPrimaryKey > 1 { - http.Error(w, ErrInputDefinitionPrimaryKey.Error(), http.StatusBadRequest) - return - } - // Create InputDefinition. _, err = index.CreateInputDefinition(def) if err == ErrInputDefinitionExists { diff --git a/handler_test.go b/handler_test.go index b5cb4b661..e47e04cc0 100644 --- a/handler_test.go +++ b/handler_test.go @@ -1159,7 +1159,7 @@ func TestHandler_DuplicatePrimaryKey(t *testing.T) { h.ServeHTTP(w, MustNewHTTPRequest("POST", "/index/i0/input-definition/input2", bytes.NewBuffer(invalidPrimaryKey))) if w.Code != http.StatusBadRequest { t.Fatalf("unexpected status code: %d", w.Code) - } else if body := w.Body.String(); body != pilosa.ErrInputDefinitionPrimaryKey.Error()+"\n" { + } else if body := w.Body.String(); body != pilosa.ErrInputDefinitionDupePrimaryKey.Error()+"\n" { t.Fatalf("unexpected body: %s", body) } diff --git a/input_definition.go b/input_definition.go index c27b2911d..32e61effa 100644 --- a/input_definition.go +++ b/input_definition.go @@ -15,13 +15,12 @@ package pilosa import ( + "errors" "fmt" "io/ioutil" "os" "path/filepath" - "errors" - "github.com/gogo/protobuf/proto" "github.com/pilosa/pilosa/internal" ) @@ -101,20 +100,9 @@ func (i *InputDefinition) LoadDefinition(pb *internal.InputDefinition) error { i.frames = append(i.frames, inputFrame) } - accountRowID := make(map[string]uint64) for _, field := range pb.Fields { var actions []Action for _, action := range field.InputDefinitionActions { - if err := i.ValidateAction(action); err != nil { - return err - } - if action.ValueDestination == InputSingleRowBool && action.Frame != "" { - val, ok := accountRowID[action.Frame] - if ok && val == action.RowID { - return fmt.Errorf("duplicate rowID with other field: %v", action.RowID) - } - accountRowID[action.Frame] = action.RowID - } actions = append(actions, Action{ Frame: action.Frame, ValueDestination: action.ValueDestination, @@ -209,19 +197,34 @@ type Action struct { RowID *uint64 `json:"rowID,omitempty"` } -// Encode converts Action into its internal representation. -func (o *Action) Encode() *internal.InputDefinitionAction { - // TODO: this check needs to happen somewhere other than Encode() - /* - if o.RowID == nil && o.ValueDestination == InputSingleRowBool { - return nil, errors.New("rowID required for single-row-boolean") +// Validate ensures the input definition action conforms to our specification. +func (a *Action) Validate() error { + if a.Frame == "" { + return ErrFrameRequired + } + validValues := make(map[string]bool) + for _, val := range validValueDestination { + validValues[val] = true + } + if _, ok := validValues[a.ValueDestination]; !ok { + return fmt.Errorf("invalid ValueDestination: %s", a.ValueDestination) + } + switch a.ValueDestination { + case InputMapping: + if len(a.ValueMap) == 0 { + return errors.New("valueMap required for map") } - */ + } + return nil +} + +// Encode converts Action into its internal representation. +func (a *Action) Encode() *internal.InputDefinitionAction { return &internal.InputDefinitionAction{ - Frame: o.Frame, - ValueDestination: o.ValueDestination, - ValueMap: o.ValueMap, - RowID: convert(o.RowID), + Frame: a.Frame, + ValueDestination: a.ValueDestination, + ValueMap: a.ValueMap, + RowID: convert(a.RowID), } } @@ -239,13 +242,21 @@ type InputFrame struct { Options FrameOptions `json:"options,omitempty"` } -// Encode converts InputFrame into its internal representation. -func (f *InputFrame) Encode() *internal.Frame { - return &internal.Frame{ - Name: f.Name, - Meta: f.Options.Encode(), +// Validate the InputFrame data +func (i *InputFrame) Validate() error { + if err := ValidateName(i.Name); err != nil { + return err } + return nil +} + +// Encode converts InputFrame into its internal representation. +func (i *InputFrame) Encode() *internal.Frame { + return &internal.Frame{ + Name: i.Name, + Meta: i.Options.Encode(), + } } // InputDefinitionInfo represents the json message format needed to create an InputDefinition. @@ -254,6 +265,56 @@ type InputDefinitionInfo struct { Fields []InputDefinitionField `json:"fields"` } +// Validate the InputDefinitionInfo data +func (i *InputDefinitionInfo) Validate(columnLabel string) error { + numPrimaryKey := 0 + accountRowID := make(map[string]uint64) + + if len(i.Frames) == 0 { + return fmt.Errorf("At least one frame is required per Input Definition") + } + + for _, frame := range i.Frames { + if err := frame.Validate(); err != nil { + return err + } + // TODO frame option validation + } + + // Validate columnLabel and duplicate primaryKey. + for _, field := range i.Fields { + if field.PrimaryKey { + numPrimaryKey++ + if field.Name != columnLabel { + return ErrInputDefinitionColumnLabel + } + } + for _, action := range field.Actions { + if err := action.Validate(); err != nil { + return err + } + if action.ValueDestination == InputSingleRowBool && action.Frame != "" { + if action.RowID == nil { + return fmt.Errorf("rowID required for single-row-boolean Field %s", field.Name) + } + val, ok := accountRowID[action.Frame] + if ok && val == convert(action.RowID) { + return fmt.Errorf("duplicate rowID with other field: %v", action.RowID) + } + accountRowID[action.Frame] = convert(action.RowID) + } + } + } + if len(i.Fields) > 0 && numPrimaryKey == 0 { + return ErrInputDefinitionHasPrimaryKey + } + if numPrimaryKey > 1 { + return ErrInputDefinitionDupePrimaryKey + } + + return nil +} + // Encode converts InputDefinitionInfo into its internal representation. func (i *InputDefinitionInfo) Encode() *internal.InputDefinition { var def internal.InputDefinition @@ -275,27 +336,6 @@ func (i *InputDefinition) AddFrame(frame InputFrame) error { return nil } -// ValidateAction ensures the input definition action conforms to our specification. -func (i *InputDefinition) ValidateAction(action *internal.InputDefinitionAction) error { - if action.Frame == "" { - return ErrFrameRequired - } - validValues := make(map[string]bool) - for _, val := range validValueDestination { - validValues[val] = true - } - if _, ok := validValues[action.ValueDestination]; !ok { - return fmt.Errorf("invalid ValueDestination: %s", action.ValueDestination) - } - switch action.ValueDestination { - case InputMapping: - if len(action.ValueMap) == 0 { - return errors.New("valueMap required for map") - } - } - return nil -} - // HandleAction Process the input data with its action and return a bit to be imported later // Note: if the Bit should not be set then nil is returned with no error // From the JSON marshalling the possible types are: float64, boolean, string diff --git a/input_definition_test.go b/input_definition_test.go index 612dbcbce..6e007fc32 100644 --- a/input_definition_test.go +++ b/input_definition_test.go @@ -102,64 +102,86 @@ func TestInputDefinition_Encoding(t *testing.T) { } } -func TestInputDefinition_LoadDefinition(t *testing.T) { - index := MustOpenIndex() - defer index.Close() +// Test The Action validation cases +func TestActionValidation(t *testing.T) { + rowID := uint64(100) - // Create Input Definition. - input := pilosa.InputDefinition{} - frames := internal.Frame{Name: "f", Meta: &internal.FrameMeta{RowLabel: "row"}} - action := internal.InputDefinitionAction{Frame: "f", ValueDestination: "value-to-ROW", ValueMap: map[string]uint64{"Green": 1}} - field := internal.InputDefinitionField{Name: "id", PrimaryKey: true, InputDefinitionActions: []*internal.InputDefinitionAction{&action}} - def := &internal.InputDefinition{Name: "test", Frames: []*internal.Frame{&frames}, Fields: []*internal.InputDefinitionField{&field}} - err := input.LoadDefinition(def) + action := pilosa.Action{Frame: "f", ValueDestination: pilosa.InputSingleRowBool, ValueMap: map[string]uint64{"Green": 1}} + field := pilosa.InputDefinitionField{Name: "id", PrimaryKey: false, Actions: []pilosa.Action{action}} + info := pilosa.InputDefinitionInfo{Fields: []pilosa.InputDefinitionField{field}} + err := info.Validate("id") + if !strings.Contains(err.Error(), "one frame is required per Input Definition") { + t.Fatalf("Expected frame required error, actual error: %s", err) + } + + frame := pilosa.InputFrame{Name: "f", Options: pilosa.FrameOptions{RowLabel: "row"}} + info = pilosa.InputDefinitionInfo{Frames: []pilosa.InputFrame{frame}, Fields: []pilosa.InputDefinitionField{field}} + err = info.Validate("id") + if !strings.Contains(err.Error(), "rowID required for single-row-boolean") { + t.Fatalf("Expected rowID required for single-row-boolean error, actual error: %s", err) + } + + frame = pilosa.InputFrame{Name: "^", Options: pilosa.FrameOptions{RowLabel: "row"}} + action = pilosa.Action{Frame: "f", ValueDestination: pilosa.InputSingleRowBool, RowID: &rowID} + field = pilosa.InputDefinitionField{Name: "id", PrimaryKey: true, Actions: []pilosa.Action{action}} + info = pilosa.InputDefinitionInfo{Frames: []pilosa.InputFrame{frame}, Fields: []pilosa.InputDefinitionField{field}} + err = info.Validate("id") + if !strings.Contains(err.Error(), "invalid index or frame's name") { + t.Fatalf("Expected iinvalid index or frame's name error, actual error: %s", err) + } + + frame = pilosa.InputFrame{Name: "f", Options: pilosa.FrameOptions{RowLabel: "row"}} + action = pilosa.Action{ValueDestination: pilosa.InputSingleRowBool, RowID: &rowID} + field = pilosa.InputDefinitionField{Name: "id", PrimaryKey: true, Actions: []pilosa.Action{action}} + info = pilosa.InputDefinitionInfo{Frames: []pilosa.InputFrame{frame}, Fields: []pilosa.InputDefinitionField{field}} + err = info.Validate("id") + if !strings.Contains(err.Error(), "frame required") { + t.Fatalf("Expected frame required error, actual error: %s", err) + } + + action = pilosa.Action{Frame: "f", ValueDestination: pilosa.InputSingleRowBool, RowID: &rowID} + field = pilosa.InputDefinitionField{Name: "id", PrimaryKey: true, Actions: []pilosa.Action{action}} + info = pilosa.InputDefinitionInfo{Frames: []pilosa.InputFrame{frame}, Fields: []pilosa.InputDefinitionField{field}} + err = info.Validate("test") + if !strings.Contains(err.Error(), "PrimaryKey field name does not match columnLabel") { + t.Fatalf("Expected PrimaryKey field name does not match columnLabel error, actual error: %s", err) + } + + action = pilosa.Action{Frame: "f", ValueDestination: pilosa.InputSingleRowBool, RowID: &rowID} + field = pilosa.InputDefinitionField{Name: "x", PrimaryKey: false, Actions: []pilosa.Action{action}} + info = pilosa.InputDefinitionInfo{Frames: []pilosa.InputFrame{frame}, Fields: []pilosa.InputDefinitionField{field}} + err = info.Validate("id") + if !strings.Contains(err.Error(), "input-definition must contain one PrimaryKey") { + t.Fatalf("Expected input-definition must contain one PrimaryKey error, actual error: %s", err) + } + + action = pilosa.Action{Frame: "f", ValueDestination: "value-to-ROW", ValueMap: map[string]uint64{"Green": 1}} + field = pilosa.InputDefinitionField{Name: "id", PrimaryKey: true, Actions: []pilosa.Action{action}} + info = pilosa.InputDefinitionInfo{Frames: []pilosa.InputFrame{frame}, Fields: []pilosa.InputDefinitionField{field}} + err = info.Validate("id") if !strings.Contains(err.Error(), "invalid ValueDestination") { t.Fatalf("Expected invalid ValueDestination error, actual error: %s", err) } - action = internal.InputDefinitionAction{Frame: "f", ValueDestination: pilosa.InputMapping, RowID: 100} - field = internal.InputDefinitionField{Name: "id", PrimaryKey: true, InputDefinitionActions: []*internal.InputDefinitionAction{&action}} - def = &internal.InputDefinition{Name: "test", Frames: []*internal.Frame{&frames}, Fields: []*internal.InputDefinitionField{&field}} - err = input.LoadDefinition(def) + action = pilosa.Action{Frame: "f", ValueDestination: pilosa.InputMapping, RowID: &rowID} + field = pilosa.InputDefinitionField{Name: "id", PrimaryKey: true, Actions: []pilosa.Action{action}} + info = pilosa.InputDefinitionInfo{Frames: []pilosa.InputFrame{frame}, Fields: []pilosa.InputDefinitionField{field}} + err = info.Validate("id") if !strings.Contains(err.Error(), "valueMap required for map") { t.Fatalf("Expected valueMap required for map error, actual error: %s", err) } - action = internal.InputDefinitionAction{Frame: "f", ValueDestination: pilosa.InputSingleRowBool, RowID: 100} - action1 := internal.InputDefinitionAction{Frame: "f", ValueDestination: pilosa.InputSingleRowBool, RowID: 100} - field1 := internal.InputDefinitionField{Name: "id", PrimaryKey: true, InputDefinitionActions: []*internal.InputDefinitionAction{&action1}} - def = &internal.InputDefinition{Name: "test", Frames: []*internal.Frame{&frames}, Fields: []*internal.InputDefinitionField{&field, &field1}} - err = input.LoadDefinition(def) + action = pilosa.Action{Frame: "f", ValueDestination: pilosa.InputSingleRowBool, RowID: &rowID} + field = pilosa.InputDefinitionField{Name: "test", PrimaryKey: false, Actions: []pilosa.Action{action}} + action1 := pilosa.Action{Frame: "f", ValueDestination: pilosa.InputSingleRowBool, RowID: &rowID} + field1 := pilosa.InputDefinitionField{Name: "id", PrimaryKey: true, Actions: []pilosa.Action{action1}} + info = pilosa.InputDefinitionInfo{Frames: []pilosa.InputFrame{frame}, Fields: []pilosa.InputDefinitionField{field, field1}} + err = info.Validate("id") if !strings.Contains(err.Error(), "duplicate rowID with other field") { t.Fatalf("Expected duplicate rowID with other field error, actual error: %s", err) } - - action = internal.InputDefinitionAction{ValueDestination: pilosa.InputSingleRowBool, RowID: 100} - def = &internal.InputDefinition{Name: "test", Frames: []*internal.Frame{&frames}, Fields: []*internal.InputDefinitionField{&field}} - err = input.LoadDefinition(def) - if !strings.Contains(err.Error(), "frame required") { - t.Fatalf("Expected frame required error, actual error: %s", err) - } } -/* -// TODO: handle validation outside of the Encode() -func TestActionEncoding(t *testing.T) { - action := pilosa.Action{Frame: "f", ValueDestination: pilosa.InputSingleRowBool, ValueMap: map[string]uint64{"Green": 1}} - _, err := action.Encode() - if !strings.Contains(err.Error(), "rowID required for single-row-boolean") { - t.Fatalf("Expected rowID required for single-row-boolean error, actual error: %s", err) - } - - field := pilosa.InputDefinitionField{Name: "id", PrimaryKey: false, Actions: []pilosa.Action{action}} - info := pilosa.InputDefinitionInfo{Fields: []pilosa.InputDefinitionField{field}} - _, err = info.Encode() - if !strings.Contains(err.Error(), "rowID required for single-row-boolean") { - t.Fatalf("Expected rowID required for single-row-boolean error, actual error: %s", err) - } -} -*/ - func TestHandleAction(t *testing.T) { var value interface{} colID := uint64(0) diff --git a/pilosa.go b/pilosa.go index bde8cc2c7..cb650e02d 100644 --- a/pilosa.go +++ b/pilosa.go @@ -36,18 +36,19 @@ var ( ErrFrameInverseDisabled = errors.New("frame inverse disabled") ErrColumnRowLabelEqual = errors.New("column and row labels cannot be equal") - ErrInputDefinitionExists = errors.New("input-definition already exists") - ErrInputDefinitionPrimaryKey = errors.New("input-definition can only contain one PrimaryKey") - ErrInputDefinitionColumnLabel = errors.New("PrimaryKey field name does not match columnLabel") - ErrInputDefinitionNameRequired = errors.New("input-definition name required") - ErrInputDefinitionAttrsRequired = errors.New("frames and fields are required") + ErrInputDefinitionExists = errors.New("input-definition already exists") + ErrInputDefinitionHasPrimaryKey = errors.New("input-definition must contain one PrimaryKey") + ErrInputDefinitionDupePrimaryKey = errors.New("input-definition can only contain one PrimaryKey") + ErrInputDefinitionColumnLabel = errors.New("PrimaryKey field name does not match columnLabel") + ErrInputDefinitionNameRequired = errors.New("input-definition name required") + ErrInputDefinitionAttrsRequired = errors.New("frames and fields are required") - ErrFieldNameRequired = errors.New("field name required") - ErrInvalidFieldType = errors.New("invalid field type") - ErrInvalidFieldRange = errors.New("invalid field range") - ErrInverseRangeNotAllowed = errors.New("inverse range not allowed") - ErrRangeCacheNotAllowed = errors.New("range cache not allowed") - ErrFrameFieldsNotAllowed = errors.New("frame fields not allowed") + ErrFieldNameRequired = errors.New("field name required") + ErrInvalidFieldType = errors.New("invalid field type") + ErrInvalidFieldRange = errors.New("invalid field range") + ErrInverseRangeNotAllowed = errors.New("inverse range not allowed") + ErrRangeCacheNotAllowed = errors.New("range cache not allowed") + ErrFrameFieldsNotAllowed = errors.New("frame fields not allowed") ErrInputDefinitionNotFound = errors.New("input-definition not found") ErrInvalidView = errors.New("invalid view")