Merge pull request #694 from travisturner/input-definition-tweaks

refactor some of the input defintion logic to better utilize Encode()

frame.go

@@ -213,6 +213,11 @@ func (f *Frame) CacheSize() uint32 {
 // Options returns all options for this frame.
 func (f *Frame) Options() FrameOptions {
 	f.mu.Lock()
+	defer f.mu.Unlock()
+	return f.options()
+}
+
+func (f *Frame) options() FrameOptions {
 	opt := FrameOptions{
 		RowLabel:       f.rowLabel,
 		InverseEnabled: f.inverseEnabled,
@@ -221,7 +226,6 @@ func (f *Frame) Options() FrameOptions {
 		CacheSize:      f.cacheSize,
 		TimeQuantum:    f.timeQuantum,
 	}
-	f.mu.Unlock()
 	return opt
 }
 
@@ -329,14 +333,8 @@ func (f *Frame) loadMeta() error {
 // saveMeta writes meta data for the frame.
 func (f *Frame) saveMeta() error {
 	// Marshal metadata.
-	buf, err := proto.Marshal(&internal.FrameMeta{
-		RowLabel:       f.rowLabel,
-		InverseEnabled: f.inverseEnabled,
-		RangeEnabled:   f.rangeEnabled,
-		CacheType:      f.cacheType,
-		CacheSize:      f.cacheSize,
-		TimeQuantum:    string(f.timeQuantum),
-	})
+	fo := f.options()
+	buf, err := proto.Marshal(fo.Encode())
 	if err != nil {
 		return err
 	}

handler.go

@@ -1520,12 +1520,16 @@ func (h *Handler) handlePostInputDefinition(w http.ResponseWriter, r *http.Reque
 		return
 	}
 
+	// TODO: validation before/after encode?
+
 	// Encode InputDefinition to its internal representation.
-	def, err := req.Encode()
-	if err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
-		return
-	}
+	def := req.Encode()
+	/*
+		if err != nil {
+			http.Error(w, err.Error(), http.StatusInternalServerError)
+			return
+		}
+	*/
 	def.Name = inputDefName
 
 	// Validate columnLabel and duplicate primaryKey.

handler_test.go

@@ -1485,7 +1485,7 @@ func EncodeInputDef(name string, body []byte) (*internal.InputDefinition, error)
 	if err != nil {
 		return nil, err
 	}
-	def, err := req.Encode()
+	def := req.Encode()
 	def.Name = name
-	return def, err
+	return def, nil
 }

index.go

@@ -155,7 +155,7 @@ func (i *Index) Open() error {
 		return err
 	}
 
-	if err := i.openInputDefinition(); err != nil {
+	if err := i.openInputDefinitions(); err != nil {
 		return err
 	}
 
@@ -336,7 +336,7 @@ func (i *Index) SetTimeQuantum(q TimeQuantum) error {
 // FramePath returns the path to a frame in the index.
 func (i *Index) FramePath(name string) string { return filepath.Join(i.path, name) }
 
-// InputDefinitionPath returns the path to an input definition in the index.
+// InputDefinitionPath returns the path to the input definition directory for the index.
 func (i *Index) InputDefinitionPath() string {
 	return filepath.Join(i.path, InputDefinitionDir)
 }
@@ -723,8 +723,8 @@ func (i *Index) DeleteInputDefinition(name string) error {
 	return nil
 }
 
-// openInputDefinition opens and initializes the input definitions inside the index.
-func (i *Index) openInputDefinition() error {
+// openInputDefinitions opens and initializes the input definitions inside the index.
+func (i *Index) openInputDefinitions() error {
 	inputDef, err := os.Open(i.InputDefinitionPath())
 	if os.IsNotExist(err) {
 		return nil

input_definition.go

@@ -26,7 +26,7 @@ import (
 	"github.com/pilosa/pilosa/internal"
 )
 
-// Action Mapping types
+// Action types.
 const (
 	InputMapping       = "mapping"
 	InputValueToRow    = "value-to-row"
@@ -101,7 +101,7 @@ func (i *InputDefinition) LoadDefinition(pb *internal.InputDefinition) error {
 		i.frames = append(i.frames, inputFrame)
 	}
 
-	countRowID := make(map[string]uint64)
+	accountRowID := make(map[string]uint64)
 	for _, field := range pb.Fields {
 		var actions []Action
 		for _, action := range field.InputDefinitionActions {
@@ -109,11 +109,11 @@ func (i *InputDefinition) LoadDefinition(pb *internal.InputDefinition) error {
 				return err
 			}
 			if action.ValueDestination == InputSingleRowBool && action.Frame != "" {
-				val, ok := countRowID[action.Frame]
+				val, ok := accountRowID[action.Frame]
 				if ok && val == action.RowID {
 					return fmt.Errorf("duplicate rowID with other field: %v", action.RowID)
 				}
-				countRowID[action.Frame] = action.RowID
+				accountRowID[action.Frame] = action.RowID
 			}
 			actions = append(actions, Action{
 				Frame:            action.Frame,
@@ -152,40 +152,18 @@ func (i *InputDefinition) saveMeta() error {
 	if err := os.MkdirAll(i.path, 0777); err != nil {
 		return err
 	}
-	// Marshal metadata.
+
 	var frames []*internal.Frame
 	for _, fr := range i.frames {
-		frameMeta := &internal.FrameMeta{
-			RowLabel:       fr.Options.RowLabel,
-			InverseEnabled: fr.Options.InverseEnabled,
-			CacheType:      fr.Options.CacheType,
-			CacheSize:      fr.Options.CacheSize,
-			TimeQuantum:    string(fr.Options.TimeQuantum),
-		}
-		frame := &internal.Frame{Name: fr.Name, Meta: frameMeta}
-		frames = append(frames, frame)
+		frames = append(frames, fr.Encode())
 	}
 
 	var fields []*internal.InputDefinitionField
 	for _, field := range i.fields {
-		var actions []*internal.InputDefinitionAction
-		for _, action := range field.Actions {
-			actionMeta := &internal.InputDefinitionAction{
-				Frame:            action.Frame,
-				ValueDestination: action.ValueDestination,
-				ValueMap:         action.ValueMap,
-				RowID:            convert(action.RowID),
-			}
-			actions = append(actions, actionMeta)
-		}
-
-		fieldMeta := &internal.InputDefinitionField{
-			Name:                   field.Name,
-			PrimaryKey:             field.PrimaryKey,
-			InputDefinitionActions: actions,
-		}
-		fields = append(fields, fieldMeta)
+		fields = append(fields, field.Encode())
 	}
+
+	// Marshal input definition.
 	buf, err := proto.Marshal(&internal.InputDefinition{
 		Name:   i.name,
 		Frames: frames,
@@ -211,17 +189,16 @@ type InputDefinitionField struct {
 }
 
 // Encode converts InputDefinitionField into its internal representation.
-func (o *InputDefinitionField) Encode() (*internal.InputDefinitionField, error) {
-	field := internal.InputDefinitionField{Name: o.Name, PrimaryKey: o.PrimaryKey}
-
+func (o *InputDefinitionField) Encode() *internal.InputDefinitionField {
+	var actions []*internal.InputDefinitionAction
 	for _, action := range o.Actions {
-		actionEncode, err := action.Encode()
-		if err != nil {
-			return nil, err
-		}
-		field.InputDefinitionActions = append(field.InputDefinitionActions, actionEncode)
+		actions = append(actions, action.Encode())
+	}
+	return &internal.InputDefinitionField{
+		Name:                   o.Name,
+		PrimaryKey:             o.PrimaryKey,
+		InputDefinitionActions: actions,
 	}
-	return &field, nil
 }
 
 // Action describes the mapping method for the field in the InputDefinition.
@@ -233,16 +210,19 @@ type Action struct {
 }
 
 // Encode converts Action into its internal representation.
-func (o *Action) Encode() (*internal.InputDefinitionAction, error) {
-	if o.RowID == nil && o.ValueDestination == "single-row-boolean" {
-		return nil, errors.New("rowID required for single-row-boolean")
-	}
+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")
+		}
+	*/
 	return &internal.InputDefinitionAction{
 		Frame:            o.Frame,
 		ValueDestination: o.ValueDestination,
 		ValueMap:         o.ValueMap,
 		RowID:            convert(o.RowID),
-	}, nil
+	}
 }
 
 // convert pointer to uint64
@@ -259,6 +239,15 @@ 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(),
+	}
+
+}
+
 // InputDefinitionInfo represents the json message format needed to create an InputDefinition.
 type InputDefinitionInfo struct {
 	Frames []InputFrame           `json:"frames"`
@@ -266,20 +255,15 @@ type InputDefinitionInfo struct {
 }
 
 // Encode converts InputDefinitionInfo into its internal representation.
-func (i *InputDefinitionInfo) Encode() (*internal.InputDefinition, error) {
+func (i *InputDefinitionInfo) Encode() *internal.InputDefinition {
 	var def internal.InputDefinition
 	for _, f := range i.Frames {
-		def.Frames = append(def.Frames, &internal.Frame{Name: f.Name, Meta: f.Options.Encode()})
+		def.Frames = append(def.Frames, f.Encode())
 	}
 	for _, f := range i.Fields {
-		fEncode, err := f.Encode()
-		if err != nil {
-			return nil, err
-		}
-		def.Fields = append(def.Fields, fEncode)
+		def.Fields = append(def.Fields, f.Encode())
 	}
-
-	return &def, nil
+	return &def
 }
 
 // AddFrame manually add frame to input definition.

input_definition_test.go

@@ -87,10 +87,7 @@ func TestInputDefinition_Encoding(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	internalDef, err := def.Encode()
-	if err != nil {
-		t.Fatal(err)
-	}
+	internalDef := def.Encode()
 
 	if internalDef.Frames[0].Name != "event-time" {
 		t.Fatalf("unexpected frame: %v", internalDef)
@@ -145,6 +142,8 @@ func TestInputDefinition_LoadDefinition(t *testing.T) {
 	}
 }
 
+/*
+// 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()
@@ -159,6 +158,7 @@ func TestActionEncoding(t *testing.T) {
 		t.Fatalf("Expected rowID required for single-row-boolean error, actual error: %s", err)
 	}
 }
+*/
 
 func TestHandleAction(t *testing.T) {
 	var value interface{}