From 37ffcd4ed9ee485d792df5bf08ee88ea874e53b2 Mon Sep 17 00:00:00 2001
From: Linh Vo <lvo@umbel.com>
Date: Thu, 6 Apr 2017 16:03:35 -0500
Subject: [PATCH] refactor validateOptions

---
 handle_internal_test.go |  7 ++++
 handler.go              | 83 ++++++++++++++++++++++++-----------------
 2 files changed, 56 insertions(+), 34 deletions(-)

diff --git a/handle_internal_test.go b/handle_internal_test.go
index 2f79a9b1b..61790a950 100644
--- a/handle_internal_test.go
+++ b/handle_internal_test.go
@@ -14,6 +14,7 @@ func TestPostDBRequestUnmarshalJSON(t *testing.T) {
 		err      string
 	}{
 		{json: `{"db": "d", "options": {}}`, expected: postDBRequest{DB: "d", Options: DBOptions{}}},
+		{json: `{"db": 1, "options": {}}`, err: "db required and must be a string"},
 		{json: `{"db": "d", "options": 4}`, err: "options is not map[string]interface{}"},
 		{json: `{"db": "d", "option": {}}`, err: "Unknown key: option:map[]"},
 		{json: `{"db": "d", "options": {"columnLabel": "test"}}`, expected: postDBRequest{DB: "d", Options: DBOptions{ColumnLabel: "test"}}},
@@ -23,10 +24,15 @@ func TestPostDBRequestUnmarshalJSON(t *testing.T) {
 	for _, test := range tests {
 		actual := &postDBRequest{}
 		err := json.Unmarshal([]byte(test.json), actual)
+
 		if err != nil {
 			if test.err == "" || test.err != err.Error() {
 				t.Errorf("expected error: %v, but got result: %v", test.err, err)
 			}
+		} else {
+			if test.err != "" {
+				t.Errorf("expected error: %v, but got no error", test.err)
+			}
 		}
 
 		if test.err == "" {
@@ -46,6 +52,7 @@ func TestPostFrameRequestUnmarshalJSON(t *testing.T) {
 		err      string
 	}{
 		{json: `{"db": "d", "frame":"f",  "options": {}}`, expected: postFrameRequest{DB: "d", Frame: "f", Options: FrameOptions{}}},
+		{json: `{"db": "d", "options": {}}`, err: "db required and must be a string"},
 		{json: `{"db": "d", "frame":"f", "options": 4}`, err: "options is not map[string]interface{}"},
 		{json: `{"db": "d", "frame":"f", "option": {}}`, err: "Unknown key: {option:map[]}"},
 		{json: `{"db": "d", "frame":"f", "options": {"rowLabel": "test"}}`, expected: postFrameRequest{DB: "d", Frame: "f", Options: FrameOptions{RowLabel: "test"}}},
diff --git a/handler.go b/handler.go
index b3e5f9c22..2ab6ff1cb 100644
--- a/handler.go
+++ b/handler.go
@@ -363,18 +363,27 @@ func (p *postDBRequest) UnmarshalJSON(b []byte) error {
 			if !ok {
 				return errors.New("options is not map[string]interface{}")
 			}
-
-			if len(options) == 0 {
+			value, err := validateOptions(options, "columnLabel")
+			if err != nil {
+				return err
+			}
+			if value == "" {
 				p.Options = DBOptions{}
 			} else {
-				err := validateOptions(options, "columnLabel")
-				if err != nil {
-					return err
-				} else {
-					p.Options = DBOptions{ColumnLabel: options["columnLabel"].(string)}
-				}
+				p.Options = DBOptions{ColumnLabel: value}
 			}
 
+			//if len(options) == 0 {
+			//	p.Options = DBOptions{}
+			//} else {
+			//	err := validateOptions(options, "columnLabel")
+			//	if err != nil {
+			//		return err
+			//	} else {
+			//		p.Options = DBOptions{ColumnLabel: options["columnLabel"].(string)}
+			//	}
+			//}
+
 		default:
 			return fmt.Errorf("Unknown key: %v:%v", key, value)
 		}
@@ -382,23 +391,30 @@ func (p *postDBRequest) UnmarshalJSON(b []byte) error {
 	return nil
 }
 
-func validateOptions(options map[string]interface{}, field string) error {
-	for k, v := range options {
-		switch k {
-		case field:
-			if colValue, ok := options[field].(string); !ok {
-				return fmt.Errorf("invalid option %v: {%v:%v}", field, k, v)
-			} else {
-				err := ValidateName(colValue)
-				if err != nil {
-					return fmt.Errorf("invalid %v value: %v", field, v)
+func validateOptions(options map[string]interface{}, field string) (string, error) {
+	var optionValue string
+	if len(options) == 0 {
+		optionValue = ""
+	} else {
+		for k, v := range options {
+			switch k {
+			case field:
+				val, ok := options[field].(string)
+				if !ok {
+					return "", fmt.Errorf("invalid option %v: {%v:%v}", field, k, v)
 				}
+				err := ValidateName(val)
+				if err != nil {
+					return "", fmt.Errorf("invalid %v value: %v", field, v)
+
+				}
+				optionValue = options[field].(string)
+			default:
+				return "", fmt.Errorf("invalid key for options {%v:%v}", k, v)
 			}
-		default:
-			return fmt.Errorf("invalid key for options {%v:%v}", k, v)
 		}
 	}
-	return nil
+	return optionValue, nil
 }
 
 type postDBRequest struct {
@@ -575,32 +591,31 @@ func (p *postFrameRequest) UnmarshalJSON(b []byte) error {
 	for key, value := range data {
 		switch key {
 		case "db":
-			if val, ok := data["db"].(string); !ok {
+			val, ok := data["db"].(string)
+			if !ok {
 				return errors.New("db required and must be a string")
-			} else {
-				p.DB = val
 			}
+			p.DB = val
 		case "frame":
-			if val, ok := data["frame"].(string); !ok {
+			val, ok := data["frame"].(string)
+			if !ok {
 				return errors.New("frame required and must be a string")
-			} else {
-				p.Frame = val
 			}
+			p.Frame = val
 
 		case "options":
 			options, ok := data["options"].(map[string]interface{})
 			if !ok {
 				return errors.New("options is not map[string]interface{}")
 			}
-			if len(options) == 0 {
+			value, err := validateOptions(options, "rowLabel")
+			if err != nil {
+				return err
+			}
+			if value == "" {
 				p.Options = FrameOptions{}
 			} else {
-				err := validateOptions(options, "rowLabel")
-				if err != nil {
-					return err
-				} else {
-					p.Options = FrameOptions{RowLabel: options["rowLabel"].(string)}
-				}
+				p.Options = FrameOptions{RowLabel: value}
 			}
 
 		default: