From c034293967e0c472a45a02541a07d5cfe11b30d0 Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Sun, 2 Apr 2017 22:26:36 -0500 Subject: [PATCH 1/8] validate db and frame --- db.go | 2 +- handler.go | 89 +++++++++++++++++++++++++++++++++++++++++++++++-- handler_test.go | 86 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 173 insertions(+), 4 deletions(-) diff --git a/db.go b/db.go index 36fb38a94..d278cb2ef 100644 --- a/db.go +++ b/db.go @@ -478,7 +478,7 @@ func MergeSchemas(a, b []*DBInfo) []*DBInfo { // DBOptions represents options to set when initializing a db. type DBOptions struct { - ColumnLabel string `json:"columnLabel,omitempty"` + ColumnLabel string `json:"columnLabel,omitempty" valid:"required,string"` } // hasTime returns true if a contains a non-nil time. diff --git a/handler.go b/handler.go index 400d9c48d..eba6c51af 100644 --- a/handler.go +++ b/handler.go @@ -17,6 +17,7 @@ import ( "strings" "time" + "bytes" "github.com/gogo/protobuf/proto" "github.com/pilosa/pilosa/internal" "github.com/pilosa/pilosa/pql" @@ -321,6 +322,16 @@ type sliceMaxResponse struct { // handlePostDB handles POST /db request. func (h *Handler) handlePostDB(w http.ResponseWriter, r *http.Request) { + var err error + // Copy request body for validation + buf, err := ioutil.ReadAll(r.Body) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + rdr2 := ioutil.NopCloser(bytes.NewBuffer(buf)) + r.Body = rdr2 + // Decode request. var req postDBRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { @@ -328,8 +339,15 @@ func (h *Handler) handlePostDB(w http.ResponseWriter, r *http.Request) { return } + // Validate request + err = h.validateRequest(buf, r.URL.Path) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + // Create database. - _, err := h.Index.CreateDB(req.DB, req.Options) + _, err = h.Index.CreateDB(req.DB, req.Options) if err == ErrDatabaseExists { http.Error(w, err.Error(), http.StatusConflict) return @@ -344,9 +362,57 @@ func (h *Handler) handlePostDB(w http.ResponseWriter, r *http.Request) { } } +// Validate request body for db/frame creation +func (h *Handler) validateRequest(r []byte, path string) error { + var data map[string]interface{} + + err := json.Unmarshal(r, &data) + if err != nil { + return err + } + + option, ok := data["options"] + if len(data) >= 2 && !ok { + return errors.New("options needs to be provided") + } else if ok { + err = h.validateOptions(path, option.(map[string]interface{})) + if err != nil { + return fmt.Errorf("invalid options: %s", option.(map[string]interface{})) + } + } + + return nil + +} + +// Validate options in request body for db/frame creation, make sure key and value for columnLabel/rowLable is correct +func (h Handler) validateOptions(path string, options map[string]interface{}) error { + switch path { + case "/db": + if _, ok := options["columnLabel"]; !ok && len(options) > 0 { + return errors.New("columnLabel is not provided") + } else if ok { + err := ValidateName(options["columnLabel"].(string)) + if err != nil { + return err + } + } + case "/frame": + if _, ok := options["rowLabel"]; !ok && len(options) > 0 { + return errors.New("rowLabel is not provided") + } else if ok { + err := ValidateName(options["rowLabel"].(string)) + if err != nil { + return err + } + } + } + return nil +} + type postDBRequest struct { DB string `json:"db"` - Options DBOptions `json:"options"` + Options DBOptions `json:"options" valid:"json"` } type postDBResponse struct{} @@ -478,6 +544,23 @@ type postDBAttrDiffResponse struct { // handlePostFrame handles POST /frame request. func (h *Handler) handlePostFrame(w http.ResponseWriter, r *http.Request) { + var err error + // Copy request body for validation + buf, err := ioutil.ReadAll(r.Body) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + rdr2 := ioutil.NopCloser(bytes.NewBuffer(buf)) + r.Body = rdr2 + + // Validate request + err = h.validateRequest(buf, r.URL.Path) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + // Decode request. var req postFrameRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { @@ -493,7 +576,7 @@ func (h *Handler) handlePostFrame(w http.ResponseWriter, r *http.Request) { } // Create frame. - _, err := db.CreateFrame(req.Frame, req.Options) + _, err = db.CreateFrame(req.Frame, req.Options) if err == ErrFrameExists { http.Error(w, err.Error(), http.StatusConflict) return diff --git a/handler_test.go b/handler_test.go index 20f76de05..c6d181f44 100644 --- a/handler_test.go +++ b/handler_test.go @@ -14,6 +14,7 @@ import ( "strings" "testing" + "fmt" "github.com/gogo/protobuf/proto" "github.com/pilosa/pilosa" "github.com/pilosa/pilosa/internal" @@ -868,3 +869,88 @@ func MustReadAll(r io.Reader) []byte { } return buf } + +// Ensure that options needs to be provided to set columnLabel when create DB +func TestHandler_DB_Options(t *testing.T) { + idx := MustOpenIndex() + defer idx.Close() + + s := NewServer() + s.Handler.Index = idx.Index + defer s.Close() + + resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", s.URL+"/db", strings.NewReader(`{"db": "sample-db", "columnLabel": "location"}`))) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + + // Verify body response. + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("unexpected status: %d", resp.StatusCode) + } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { + t.Fatal(err) + } else if string(buf) != "options needs to be provided"+"\n" { + fmt.Println(string(buf) == "options needs to be provided") + t.Fatalf("unexpected response body: %s", buf) + } + +} + +// Ensure that rowLabel is provided as an options when create frame +func TestHandler_Frame_Options(t *testing.T) { + idx := MustOpenIndex() + defer idx.Close() + + s := NewServer() + s.Handler.Index = idx.Index + defer s.Close() + + // Create database. + if _, err := idx.CreateDBIfNotExists("sample-db", pilosa.DBOptions{}); err != nil { + t.Fatal(err) + } + resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", s.URL+"/frame", strings.NewReader(`{"db": "sample-db", "frame": "test", "options": {"columnLabel": "location"}}`))) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + + // Verify body response. + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("unexpected status: %d", resp.StatusCode) + } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { + t.Fatal(err) + } else if string(buf) != "invalid options: map[columnLabel:location]"+"\n" { + t.Fatalf("unexpected response body: %s", buf) + } +} + +// Ensure that rowLabel is provided as an options when create frame +func TestHandler_OptionsValue(t *testing.T) { + idx := MustOpenIndex() + defer idx.Close() + + s := NewServer() + s.Handler.Index = idx.Index + defer s.Close() + + // Create database. + if _, err := idx.CreateDBIfNotExists("sample-db", pilosa.DBOptions{}); err != nil { + t.Fatal(err) + } + resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", s.URL+"/frame", strings.NewReader(`{"db": "sample-db", "options": {"rowLabel": "///"}}`))) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + + // Verify body response. + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("unexpected status: %d", resp.StatusCode) + } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { + t.Fatal(err) + } else if string(buf) != "invalid options: map[rowLabel:///]"+"\n" { + t.Fatalf("unexpected response body: %s", buf) + } +} From abd696ca566973e41a9ada8f7bc08f18268cbee5 Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Mon, 3 Apr 2017 10:50:19 -0500 Subject: [PATCH 2/8] rm govalidation phase --- db.go | 2 +- handler.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/db.go b/db.go index d278cb2ef..36fb38a94 100644 --- a/db.go +++ b/db.go @@ -478,7 +478,7 @@ func MergeSchemas(a, b []*DBInfo) []*DBInfo { // DBOptions represents options to set when initializing a db. type DBOptions struct { - ColumnLabel string `json:"columnLabel,omitempty" valid:"required,string"` + ColumnLabel string `json:"columnLabel,omitempty"` } // hasTime returns true if a contains a non-nil time. diff --git a/handler.go b/handler.go index eba6c51af..5ee1a03ce 100644 --- a/handler.go +++ b/handler.go @@ -412,7 +412,7 @@ func (h Handler) validateOptions(path string, options map[string]interface{}) er type postDBRequest struct { DB string `json:"db"` - Options DBOptions `json:"options" valid:"json"` + Options DBOptions `json:"options"` } type postDBResponse struct{} From 1be7a42d148d0e79e1fdf9bbfc4fca8a4ffb0948 Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Tue, 4 Apr 2017 14:50:43 -0500 Subject: [PATCH 3/8] custom Unmarshal JSON --- handler.go | 129 ++++++++++++++++++++++-------------------------- handler_test.go | 10 ++-- 2 files changed, 62 insertions(+), 77 deletions(-) diff --git a/handler.go b/handler.go index 5ee1a03ce..14ad78dd4 100644 --- a/handler.go +++ b/handler.go @@ -17,7 +17,6 @@ import ( "strings" "time" - "bytes" "github.com/gogo/protobuf/proto" "github.com/pilosa/pilosa/internal" "github.com/pilosa/pilosa/pql" @@ -322,16 +321,6 @@ type sliceMaxResponse struct { // handlePostDB handles POST /db request. func (h *Handler) handlePostDB(w http.ResponseWriter, r *http.Request) { - var err error - // Copy request body for validation - buf, err := ioutil.ReadAll(r.Body) - if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } - rdr2 := ioutil.NopCloser(bytes.NewBuffer(buf)) - r.Body = rdr2 - // Decode request. var req postDBRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { @@ -339,15 +328,8 @@ func (h *Handler) handlePostDB(w http.ResponseWriter, r *http.Request) { return } - // Validate request - err = h.validateRequest(buf, r.URL.Path) - if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } - // Create database. - _, err = h.Index.CreateDB(req.DB, req.Options) + _, err := h.Index.CreateDB(req.DB, req.Options) if err == ErrDatabaseExists { http.Error(w, err.Error(), http.StatusConflict) return @@ -362,50 +344,34 @@ func (h *Handler) handlePostDB(w http.ResponseWriter, r *http.Request) { } } -// Validate request body for db/frame creation -func (h *Handler) validateRequest(r []byte, path string) error { +// Custom Unmarshal JSON to validate request body when creating a new database +func (p *postDBRequest) UnmarshalJSON(b []byte) error { var data map[string]interface{} - - err := json.Unmarshal(r, &data) - if err != nil { + if err := json.Unmarshal(b, &data); err != nil { return err } - - option, ok := data["options"] - if len(data) >= 2 && !ok { - return errors.New("options needs to be provided") - } else if ok { - err = h.validateOptions(path, option.(map[string]interface{})) - if err != nil { - return fmt.Errorf("invalid options: %s", option.(map[string]interface{})) - } + f := func(key string, m map[string]interface{}) bool { _, ok := m[key]; return ok } + if !f("db", data) { + return errors.New("db required") } + p.DB = data["db"].(string) - return nil - -} - -// Validate options in request body for db/frame creation, make sure key and value for columnLabel/rowLable is correct -func (h Handler) validateOptions(path string, options map[string]interface{}) error { - switch path { - case "/db": - if _, ok := options["columnLabel"]; !ok && len(options) > 0 { - return errors.New("columnLabel is not provided") - } else if ok { + if f("options", data) { + options := data["options"].(map[string]interface{}) + if len(options) == 0 { + return nil + } else if f("columnLabel", options) { err := ValidateName(options["columnLabel"].(string)) if err != nil { - return err - } - } - case "/frame": - if _, ok := options["rowLabel"]; !ok && len(options) > 0 { - return errors.New("rowLabel is not provided") - } else if ok { - err := ValidateName(options["rowLabel"].(string)) - if err != nil { - return err + return errors.New("invalid columnLabel") } + p.Options = DBOptions{ColumnLabel: options["columnLabel"].(string)} + } else { + return errors.New("columnLabel required") } + + } else if len(data) > 1 { + return errors.New("options required") } return nil } @@ -544,22 +510,6 @@ type postDBAttrDiffResponse struct { // handlePostFrame handles POST /frame request. func (h *Handler) handlePostFrame(w http.ResponseWriter, r *http.Request) { - var err error - // Copy request body for validation - buf, err := ioutil.ReadAll(r.Body) - if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } - rdr2 := ioutil.NopCloser(bytes.NewBuffer(buf)) - r.Body = rdr2 - - // Validate request - err = h.validateRequest(buf, r.URL.Path) - if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } // Decode request. var req postFrameRequest @@ -576,7 +526,7 @@ func (h *Handler) handlePostFrame(w http.ResponseWriter, r *http.Request) { } // Create frame. - _, err = db.CreateFrame(req.Frame, req.Options) + _, err := db.CreateFrame(req.Frame, req.Options) if err == ErrFrameExists { http.Error(w, err.Error(), http.StatusConflict) return @@ -591,6 +541,43 @@ func (h *Handler) handlePostFrame(w http.ResponseWriter, r *http.Request) { } } +// Custom Unmarshal JSON to validate request body when creating a new frame +func (p *postFrameRequest) UnmarshalJSON(b []byte) error { + var data map[string]interface{} + if err := json.Unmarshal(b, &data); err != nil { + return err + } + f := func(key string, m map[string]interface{}) bool { _, ok := m[key]; return ok } + if !f("db", data) { + return errors.New("db required") + } + p.DB = data["db"].(string) + + if !f("frame", data) { + return errors.New("frame required") + } + p.Frame = data["frame"].(string) + + if f("options", data) { + options := data["options"].(map[string]interface{}) + if len(options) == 0 { + return nil + } else if f("rowLabel", options) { + err := ValidateName(options["rowLabel"].(string)) + if err != nil { + return errors.New("invalid rowLabel") + } + p.Options = FrameOptions{RowLabel: options["rowLabel"].(string)} + } else { + return errors.New("rowLabel required") + } + + } else if len(data) > 2 { + return errors.New("options required") + } + return nil +} + type postFrameRequest struct { DB string `json:"db"` Frame string `json:"frame"` diff --git a/handler_test.go b/handler_test.go index c6d181f44..007bb639a 100644 --- a/handler_test.go +++ b/handler_test.go @@ -14,7 +14,6 @@ import ( "strings" "testing" - "fmt" "github.com/gogo/protobuf/proto" "github.com/pilosa/pilosa" "github.com/pilosa/pilosa/internal" @@ -890,8 +889,7 @@ func TestHandler_DB_Options(t *testing.T) { t.Fatalf("unexpected status: %d", resp.StatusCode) } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { t.Fatal(err) - } else if string(buf) != "options needs to be provided"+"\n" { - fmt.Println(string(buf) == "options needs to be provided") + } else if string(buf) != "options required"+"\n" { t.Fatalf("unexpected response body: %s", buf) } @@ -921,7 +919,7 @@ func TestHandler_Frame_Options(t *testing.T) { t.Fatalf("unexpected status: %d", resp.StatusCode) } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { t.Fatal(err) - } else if string(buf) != "invalid options: map[columnLabel:location]"+"\n" { + } else if string(buf) != "rowLabel required"+"\n" { t.Fatalf("unexpected response body: %s", buf) } } @@ -939,7 +937,7 @@ func TestHandler_OptionsValue(t *testing.T) { if _, err := idx.CreateDBIfNotExists("sample-db", pilosa.DBOptions{}); err != nil { t.Fatal(err) } - resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", s.URL+"/frame", strings.NewReader(`{"db": "sample-db", "options": {"rowLabel": "///"}}`))) + resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", s.URL+"/frame", strings.NewReader(`{"db": "sample-db", "frame": "test", "options": {"rowLabel": "///"}}`))) if err != nil { t.Fatal(err) } @@ -950,7 +948,7 @@ func TestHandler_OptionsValue(t *testing.T) { t.Fatalf("unexpected status: %d", resp.StatusCode) } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { t.Fatal(err) - } else if string(buf) != "invalid options: map[rowLabel:///]"+"\n" { + } else if string(buf) != "invalid rowLabel"+"\n" { t.Fatalf("unexpected response body: %s", buf) } } From 22475df035379fe0e4d22c8f727fcb44eee05cf1 Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Wed, 5 Apr 2017 10:50:56 -0500 Subject: [PATCH 4/8] revise unmarshalJSON --- handler.go | 116 ++++++++++++++++++++++++++++++------------------ handler_test.go | 6 +-- 2 files changed, 75 insertions(+), 47 deletions(-) diff --git a/handler.go b/handler.go index 14ad78dd4..ca243f991 100644 --- a/handler.go +++ b/handler.go @@ -350,28 +350,50 @@ func (p *postDBRequest) UnmarshalJSON(b []byte) error { if err := json.Unmarshal(b, &data); err != nil { return err } - f := func(key string, m map[string]interface{}) bool { _, ok := m[key]; return ok } - if !f("db", data) { - return errors.New("db required") - } - p.DB = data["db"].(string) - - if f("options", data) { - options := data["options"].(map[string]interface{}) - if len(options) == 0 { - return nil - } else if f("columnLabel", options) { - err := ValidateName(options["columnLabel"].(string)) - if err != nil { - return errors.New("invalid columnLabel") + for key, value := range data { + switch key { + case "db": + if val, ok := data["db"].(string); !ok { + return errors.New("db required and must be a string") + } else { + p.DB = val } - p.Options = DBOptions{ColumnLabel: options["columnLabel"].(string)} - } else { - return errors.New("columnLabel required") + case "options": + options, ok := data["options"].(map[string]interface{}) + if !ok { + return errors.New("options is not map[string]interface{}") + } + if len(options) == 0 { + return nil + } + 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) } + } + return nil +} - } else if len(data) > 1 { - return errors.New("options required") +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) + } + } + default: + return fmt.Errorf("invalid key for options {%v:%v}", k, v) + } } return nil } @@ -547,35 +569,41 @@ func (p *postFrameRequest) UnmarshalJSON(b []byte) error { if err := json.Unmarshal(b, &data); err != nil { return err } - f := func(key string, m map[string]interface{}) bool { _, ok := m[key]; return ok } - if !f("db", data) { - return errors.New("db required") - } - p.DB = data["db"].(string) - - if !f("frame", data) { - return errors.New("frame required") - } - p.Frame = data["frame"].(string) - - if f("options", data) { - options := data["options"].(map[string]interface{}) - if len(options) == 0 { - return nil - } else if f("rowLabel", options) { - err := ValidateName(options["rowLabel"].(string)) - if err != nil { - return errors.New("invalid rowLabel") + for key, value := range data { + switch key { + case "db": + if val, ok := data["db"].(string); !ok { + return errors.New("db required and must be a string") + } else { + p.DB = val + } + case "frame": + if val, ok := data["frame"].(string); !ok { + return errors.New("frame required and must be a string") + } else { + p.Frame = val } - p.Options = FrameOptions{RowLabel: options["rowLabel"].(string)} - } else { - return errors.New("rowLabel required") - } - } else if len(data) > 2 { - return errors.New("options required") + case "options": + options, ok := data["options"].(map[string]interface{}) + if !ok { + return errors.New("options is not map[string]interface{}") + } + if len(options) == 0 { + return nil + } + err := validateOptions(options, "rowLabel") + if err != nil { + return err + } else { + p.Options = FrameOptions{RowLabel: options["rowLabel"].(string)} + } + default: + return fmt.Errorf("Unknown key: {%v:%v}", key, value) + } } return nil + } type postFrameRequest struct { diff --git a/handler_test.go b/handler_test.go index 007bb639a..7a40c0a0d 100644 --- a/handler_test.go +++ b/handler_test.go @@ -889,7 +889,7 @@ func TestHandler_DB_Options(t *testing.T) { t.Fatalf("unexpected status: %d", resp.StatusCode) } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { t.Fatal(err) - } else if string(buf) != "options required"+"\n" { + } else if string(buf) != "Unknown key: columnLabel:location"+"\n" { t.Fatalf("unexpected response body: %s", buf) } @@ -919,7 +919,7 @@ func TestHandler_Frame_Options(t *testing.T) { t.Fatalf("unexpected status: %d", resp.StatusCode) } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { t.Fatal(err) - } else if string(buf) != "rowLabel required"+"\n" { + } else if string(buf) != "invalid key for options {columnLabel:location}"+"\n" { t.Fatalf("unexpected response body: %s", buf) } } @@ -948,7 +948,7 @@ func TestHandler_OptionsValue(t *testing.T) { t.Fatalf("unexpected status: %d", resp.StatusCode) } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { t.Fatal(err) - } else if string(buf) != "invalid rowLabel"+"\n" { + } else if string(buf) != "invalid rowLabel value: ///"+"\n" { t.Fatalf("unexpected response body: %s", buf) } } From 07e689ed586ff9b629a3820e6e6527bb9b9ca26c Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Wed, 5 Apr 2017 15:40:02 -0500 Subject: [PATCH 5/8] custom UnmarshalJSON unit test --- handle_internal_test.go | 71 ++++++++++++++++++++++++++++++++++ handler.go | 35 +++++++++-------- handler_test.go | 84 ----------------------------------------- 3 files changed, 91 insertions(+), 99 deletions(-) create mode 100644 handle_internal_test.go diff --git a/handle_internal_test.go b/handle_internal_test.go new file mode 100644 index 000000000..2f79a9b1b --- /dev/null +++ b/handle_internal_test.go @@ -0,0 +1,71 @@ +package pilosa + +import ( + "encoding/json" + "reflect" + "testing" +) + +// Test custom UnmarshalJSON for postDBRequest object +func TestPostDBRequestUnmarshalJSON(t *testing.T) { + tests := []struct { + json string + expected postDBRequest + err string + }{ + {json: `{"db": "d", "options": {}}`, expected: postDBRequest{DB: "d", Options: DBOptions{}}}, + {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"}}}, + {json: `{"db": "d", "options": {"columnLabl": "test"}}`, err: "invalid key for options {columnLabl:test}"}, + {json: `{"db": "d", "options": {"columnLabel": "////"}}`, err: "invalid columnLabel value: ////"}, + } + 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) + } + } + + if test.err == "" { + if !reflect.DeepEqual(*actual, test.expected) { + t.Errorf("expected: %v, but got: %v", test.expected, *actual) + } + } + + } +} + +// Test custom UnmarshalJSON for postFrameRequest object +func TestPostFrameRequestUnmarshalJSON(t *testing.T) { + tests := []struct { + json string + expected postFrameRequest + err string + }{ + {json: `{"db": "d", "frame":"f", "options": {}}`, expected: postFrameRequest{DB: "d", Frame: "f", Options: FrameOptions{}}}, + {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"}}}, + {json: `{"db": "d", "frame":"f", "options": {"rowLabl": "test"}}`, err: "invalid key for options {rowLabl:test}"}, + {json: `{"db": "d", "frame":"f", "options": {"rowLabel": "////"}}`, err: "invalid rowLabel value: ////"}, + } + for _, test := range tests { + actual := &postFrameRequest{} + 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) + } + } + + if test.err == "" { + if !reflect.DeepEqual(*actual, test.expected) { + t.Errorf("expected: %v, but got: %v", test.expected, *actual) + } + } + + } +} diff --git a/handler.go b/handler.go index ca243f991..b3e5f9c22 100644 --- a/handler.go +++ b/handler.go @@ -353,25 +353,28 @@ func (p *postDBRequest) 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 "options": options, ok := data["options"].(map[string]interface{}) if !ok { return errors.New("options is not map[string]interface{}") } + if len(options) == 0 { - return nil - } - err := validateOptions(options, "columnLabel") - if err != nil { - return err + p.Options = DBOptions{} } else { - p.Options = DBOptions{ColumnLabel: options["columnLabel"].(string)} + 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) } @@ -590,14 +593,16 @@ func (p *postFrameRequest) UnmarshalJSON(b []byte) error { return errors.New("options is not map[string]interface{}") } if len(options) == 0 { - return nil - } - err := validateOptions(options, "rowLabel") - if err != nil { - return err + p.Options = FrameOptions{} } else { - p.Options = FrameOptions{RowLabel: options["rowLabel"].(string)} + err := validateOptions(options, "rowLabel") + if err != nil { + return err + } else { + p.Options = FrameOptions{RowLabel: options["rowLabel"].(string)} + } } + default: return fmt.Errorf("Unknown key: {%v:%v}", key, value) } diff --git a/handler_test.go b/handler_test.go index 7a40c0a0d..20f76de05 100644 --- a/handler_test.go +++ b/handler_test.go @@ -868,87 +868,3 @@ func MustReadAll(r io.Reader) []byte { } return buf } - -// Ensure that options needs to be provided to set columnLabel when create DB -func TestHandler_DB_Options(t *testing.T) { - idx := MustOpenIndex() - defer idx.Close() - - s := NewServer() - s.Handler.Index = idx.Index - defer s.Close() - - resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", s.URL+"/db", strings.NewReader(`{"db": "sample-db", "columnLabel": "location"}`))) - if err != nil { - t.Fatal(err) - } - defer resp.Body.Close() - - // Verify body response. - if resp.StatusCode != http.StatusBadRequest { - t.Fatalf("unexpected status: %d", resp.StatusCode) - } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { - t.Fatal(err) - } else if string(buf) != "Unknown key: columnLabel:location"+"\n" { - t.Fatalf("unexpected response body: %s", buf) - } - -} - -// Ensure that rowLabel is provided as an options when create frame -func TestHandler_Frame_Options(t *testing.T) { - idx := MustOpenIndex() - defer idx.Close() - - s := NewServer() - s.Handler.Index = idx.Index - defer s.Close() - - // Create database. - if _, err := idx.CreateDBIfNotExists("sample-db", pilosa.DBOptions{}); err != nil { - t.Fatal(err) - } - resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", s.URL+"/frame", strings.NewReader(`{"db": "sample-db", "frame": "test", "options": {"columnLabel": "location"}}`))) - if err != nil { - t.Fatal(err) - } - defer resp.Body.Close() - - // Verify body response. - if resp.StatusCode != http.StatusBadRequest { - t.Fatalf("unexpected status: %d", resp.StatusCode) - } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { - t.Fatal(err) - } else if string(buf) != "invalid key for options {columnLabel:location}"+"\n" { - t.Fatalf("unexpected response body: %s", buf) - } -} - -// Ensure that rowLabel is provided as an options when create frame -func TestHandler_OptionsValue(t *testing.T) { - idx := MustOpenIndex() - defer idx.Close() - - s := NewServer() - s.Handler.Index = idx.Index - defer s.Close() - - // Create database. - if _, err := idx.CreateDBIfNotExists("sample-db", pilosa.DBOptions{}); err != nil { - t.Fatal(err) - } - resp, err := http.DefaultClient.Do(MustNewHTTPRequest("POST", s.URL+"/frame", strings.NewReader(`{"db": "sample-db", "frame": "test", "options": {"rowLabel": "///"}}`))) - if err != nil { - t.Fatal(err) - } - defer resp.Body.Close() - - // Verify body response. - if resp.StatusCode != http.StatusBadRequest { - t.Fatalf("unexpected status: %d", resp.StatusCode) - } else if buf, err := ioutil.ReadAll(resp.Body); err != nil { - t.Fatal(err) - } else if string(buf) != "invalid rowLabel value: ///"+"\n" { - t.Fatalf("unexpected response body: %s", buf) - } -} From 37ffcd4ed9ee485d792df5bf08ee88ea874e53b2 Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Thu, 6 Apr 2017 16:03:35 -0500 Subject: [PATCH 6/8] 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: From b2db3397417c4b9147356f3605c3f786b44d18eb Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Thu, 6 Apr 2017 16:20:08 -0500 Subject: [PATCH 7/8] remove comment code --- handler.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/handler.go b/handler.go index 2ab6ff1cb..19b2a7607 100644 --- a/handler.go +++ b/handler.go @@ -373,17 +373,6 @@ func (p *postDBRequest) UnmarshalJSON(b []byte) error { 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) } From 9abdb29269df85e705364898193ac2dc5c987c8d Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Thu, 6 Apr 2017 23:28:16 -0500 Subject: [PATCH 8/8] remove duplicate code, move ValidateName out of validateOptions --- db.go | 11 +++++++++++ frame.go | 5 +++++ handle_internal_test.go | 4 +--- handler.go | 25 ++++++++----------------- 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/db.go b/db.go index 36fb38a94..965da17fa 100644 --- a/db.go +++ b/db.go @@ -86,6 +86,11 @@ func (db *DB) SetColumnLabel(v string) error { db.mu.Lock() defer db.mu.Unlock() + // Make sure columnLabel is valid name + err := ValidateName(v) + if err != nil { + return err + } // Ignore if no change occurred. if v == "" || db.columnLabel == v { return nil @@ -369,6 +374,12 @@ func (db *DB) createFrame(name string, opt FrameOptions) (*Frame, error) { } // Update options. + if opt.RowLabel != "" { + err := ValidateName(opt.RowLabel) + if err != nil { + return nil, err + } + } f.SetRowLabel(opt.RowLabel) // Add to database's frame lookup. diff --git a/frame.go b/frame.go index 9d0fccf50..21db2c24d 100644 --- a/frame.go +++ b/frame.go @@ -110,6 +110,11 @@ func (f *Frame) SetRowLabel(v string) error { f.mu.Lock() defer f.mu.Unlock() + // Make sure rowLabel is valid name + err := ValidateName(v) + if err != nil { + return err + } // Ignore if no change occurred. if v == "" || f.rowLabel == v { return nil diff --git a/handle_internal_test.go b/handle_internal_test.go index 61790a950..b5ce8c161 100644 --- a/handle_internal_test.go +++ b/handle_internal_test.go @@ -19,7 +19,6 @@ func TestPostDBRequestUnmarshalJSON(t *testing.T) { {json: `{"db": "d", "option": {}}`, err: "Unknown key: option:map[]"}, {json: `{"db": "d", "options": {"columnLabel": "test"}}`, expected: postDBRequest{DB: "d", Options: DBOptions{ColumnLabel: "test"}}}, {json: `{"db": "d", "options": {"columnLabl": "test"}}`, err: "invalid key for options {columnLabl:test}"}, - {json: `{"db": "d", "options": {"columnLabel": "////"}}`, err: "invalid columnLabel value: ////"}, } for _, test := range tests { actual := &postDBRequest{} @@ -52,12 +51,11 @@ 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", "options": {}}`, err: "frame 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"}}}, {json: `{"db": "d", "frame":"f", "options": {"rowLabl": "test"}}`, err: "invalid key for options {rowLabl:test}"}, - {json: `{"db": "d", "frame":"f", "options": {"rowLabel": "////"}}`, err: "invalid rowLabel value: ////"}, } for _, test := range tests { actual := &postFrameRequest{} diff --git a/handler.go b/handler.go index 19b2a7607..fc14fc22f 100644 --- a/handler.go +++ b/handler.go @@ -359,11 +359,7 @@ func (p *postDBRequest) UnmarshalJSON(b []byte) error { } p.DB = val case "options": - options, ok := data["options"].(map[string]interface{}) - if !ok { - return errors.New("options is not map[string]interface{}") - } - value, err := validateOptions(options, "columnLabel") + value, err := validateOptions(data, "columnLabel") if err != nil { return err } @@ -380,7 +376,11 @@ func (p *postDBRequest) UnmarshalJSON(b []byte) error { return nil } -func validateOptions(options map[string]interface{}, field string) (string, error) { +func validateOptions(data map[string]interface{}, field string) (string, error) { + options, ok := data["options"].(map[string]interface{}) + if !ok { + return "", errors.New("options is not map[string]interface{}") + } var optionValue string if len(options) == 0 { optionValue = "" @@ -392,12 +392,7 @@ func validateOptions(options map[string]interface{}, field string) (string, erro 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) + optionValue = val default: return "", fmt.Errorf("invalid key for options {%v:%v}", k, v) } @@ -593,11 +588,7 @@ func (p *postFrameRequest) UnmarshalJSON(b []byte) error { p.Frame = val case "options": - options, ok := data["options"].(map[string]interface{}) - if !ok { - return errors.New("options is not map[string]interface{}") - } - value, err := validateOptions(options, "rowLabel") + value, err := validateOptions(data, "rowLabel") if err != nil { return err }