From 9c02edc4e3d09e9d14668825ccd4e1f64c96bfaa Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Thu, 20 Apr 2017 17:28:22 -0500 Subject: [PATCH 1/5] #457 rewrite validateOptions, update frame options --- handler.go | 69 ++++++++++++++++++++++++++-------------- handler_internal_test.go | 3 ++ 2 files changed, 49 insertions(+), 23 deletions(-) diff --git a/handler.go b/handler.go index 0e25c3f2f..4f6f2e3bf 100644 --- a/handler.go +++ b/handler.go @@ -234,23 +234,25 @@ type postDBRequest struct { // Custom Unmarshal JSON to validate request body when creating a new database func (p *postDBRequest) UnmarshalJSON(b []byte) error { + validDBOptions := []string{"columnLabel"} var data map[string]interface{} if err := json.Unmarshal(b, &data); err != nil { return err } + p.Options = DBOptions{} for key, value := range data { switch key { case "options": - value, err := validateOptions(data, "columnLabel") + values, err := validateOptions(data, validDBOptions) if err != nil { return err } - if value == "" { - p.Options = DBOptions{} - } else { - p.Options = DBOptions{ColumnLabel: value} + for k, v := range values { + switch k { + case "columnLabel": + p.Options.ColumnLabel = v + } } - default: return fmt.Errorf("Unknown key: %v:%v", key, value) } @@ -258,31 +260,39 @@ func (p *postDBRequest) UnmarshalJSON(b []byte) error { return nil } -func validateOptions(data map[string]interface{}, field string) (string, error) { +func validateOptions(data map[string]interface{}, field []string) (map[string]string, error) { options, ok := data["options"].(map[string]interface{}) if !ok { - return "", errors.New("options is not map[string]interface{}") + return map[string]string{}, errors.New("options is not map[string]interface{}") } - var optionValue string + optionValue := make(map[string]string) if len(options) == 0 { - optionValue = "" + optionValue = map[string]string{} } else { for k, v := range options { - switch k { - case field: - val, ok := options[field].(string) + if foundItem(field, k) { + val, ok := options[k].(string) if !ok { - return "", fmt.Errorf("invalid option %v: {%v:%v}", field, k, v) + return map[string]string{}, fmt.Errorf("invalid option %v: {%v:%v}", field, k, v) } - optionValue = val - default: - return "", fmt.Errorf("invalid key for options {%v:%v}", k, v) + optionValue[k] = val + } else { + return map[string]string{}, fmt.Errorf("invalid key for options {%v:%v}", k, v) } } } return optionValue, nil } +func foundItem(items []string, item string) bool { + for _, i := range items { + if item == i { + return true + } + } + return false +} + type postDBResponse struct{} // handleDeleteDB handles DELETE /db request. @@ -465,24 +475,37 @@ func (h *Handler) handlePostFrame(w http.ResponseWriter, r *http.Request) { } } -// Custom Unmarshal JSON to validate request body when creating a new frame +// Custom Unmarshal JSON to validate request body when creating a new frame. If there's new FrameOptions, +// adding it to validFrameOptions to make sure the new option is validated, otherwise the request will be failed func (p *postFrameRequest) UnmarshalJSON(b []byte) error { + validFrameOptions := []string{"rowLabel", "cacheType", "inverseEnabled"} var data map[string]interface{} if err := json.Unmarshal(b, &data); err != nil { return err } + p.Options = FrameOptions{} for key, value := range data { switch key { case "options": - value, err := validateOptions(data, "rowLabel") + values, err := validateOptions(data, validFrameOptions) if err != nil { return err } - if value == "" { - p.Options = FrameOptions{} - } else { - p.Options = FrameOptions{RowLabel: value} + for k, v := range values { + switch k { + case "rowLabel": + p.Options.RowLabel = v + case "cacheType": + p.Options.CacheType = v + case "inverseEnabled": + inverse, err := strconv.ParseBool(v) + if err != nil { + continue + } + p.Options.InverseEnabled = inverse + } } + default: return fmt.Errorf("Unknown key: {%v:%v}", key, value) } diff --git a/handler_internal_test.go b/handler_internal_test.go index 8a7bc910f..71b705f0a 100644 --- a/handler_internal_test.go +++ b/handler_internal_test.go @@ -54,6 +54,9 @@ func TestPostFrameRequestUnmarshalJSON(t *testing.T) { {json: `{"option": {}}`, err: "Unknown key: {option:map[]}"}, {json: `{"options": {"rowLabel": "test"}}`, expected: postFrameRequest{Options: FrameOptions{RowLabel: "test"}}}, {json: `{"options": {"rowLabl": "test"}}`, err: "invalid key for options {rowLabl:test}"}, + {json: `{"options": {"rowLabel": "test", "inverseEnabled": "true"}}`, expected: postFrameRequest{Options: FrameOptions{RowLabel: "test", InverseEnabled: true}}}, + {json: `{"options": {"rowLabel": "test", "inverseEnabled": "true", "cacheType": "type"}}`, expected: postFrameRequest{Options: FrameOptions{RowLabel: "test", InverseEnabled: true, CacheType: "type"}}}, + {json: `{"options": {"rowLabel": "test", "inverse": "true", "cacheType": "type"}}`, err: "invalid key for options {inverse:true}"}, } for _, test := range tests { actual := &postFrameRequest{} From 9198dfab59d3cefd2eae1ca60a273b0a42db58bf Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Thu, 20 Apr 2017 22:11:01 -0500 Subject: [PATCH 2/5] inverseEnabled check for bool type --- handler.go | 41 +++++++++++++++++++++++----------------- handler_internal_test.go | 6 +++--- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/handler.go b/handler.go index 4f6f2e3bf..08ca21b31 100644 --- a/handler.go +++ b/handler.go @@ -250,7 +250,7 @@ func (p *postDBRequest) UnmarshalJSON(b []byte) error { for k, v := range values { switch k { case "columnLabel": - p.Options.ColumnLabel = v + p.Options.ColumnLabel = v.(string) } } default: @@ -260,24 +260,35 @@ func (p *postDBRequest) UnmarshalJSON(b []byte) error { return nil } -func validateOptions(data map[string]interface{}, field []string) (map[string]string, error) { +func validateOptions(data map[string]interface{}, field []string) (map[string]interface{}, error) { options, ok := data["options"].(map[string]interface{}) + optionValue := make(map[string]interface{}) if !ok { - return map[string]string{}, errors.New("options is not map[string]interface{}") + return nil, errors.New("options is not map[string]interface{}") } - optionValue := make(map[string]string) + if len(options) == 0 { - optionValue = map[string]string{} + optionValue = nil } else { for k, v := range options { if foundItem(field, k) { - val, ok := options[k].(string) - if !ok { - return map[string]string{}, fmt.Errorf("invalid option %v: {%v:%v}", field, k, v) + switch k { + case "inverseEnabled": + val, ok := options[k].(bool) + if !ok { + return nil, fmt.Errorf("invalid option type %v: {%v:%v}", field, k, v) + } + optionValue[k] = val + default: + val, ok := options[k].(string) + if !ok { + return nil, fmt.Errorf("invalid option %v: {%v:%v}", field, k, v) + } + optionValue[k] = val } - optionValue[k] = val + } else { - return map[string]string{}, fmt.Errorf("invalid key for options {%v:%v}", k, v) + return nil, fmt.Errorf("invalid key for options {%v:%v}", k, v) } } } @@ -494,15 +505,11 @@ func (p *postFrameRequest) UnmarshalJSON(b []byte) error { for k, v := range values { switch k { case "rowLabel": - p.Options.RowLabel = v + p.Options.RowLabel = v.(string) case "cacheType": - p.Options.CacheType = v + p.Options.CacheType = v.(string) case "inverseEnabled": - inverse, err := strconv.ParseBool(v) - if err != nil { - continue - } - p.Options.InverseEnabled = inverse + p.Options.InverseEnabled = v.(bool) } } diff --git a/handler_internal_test.go b/handler_internal_test.go index 71b705f0a..5942bd12c 100644 --- a/handler_internal_test.go +++ b/handler_internal_test.go @@ -54,9 +54,9 @@ func TestPostFrameRequestUnmarshalJSON(t *testing.T) { {json: `{"option": {}}`, err: "Unknown key: {option:map[]}"}, {json: `{"options": {"rowLabel": "test"}}`, expected: postFrameRequest{Options: FrameOptions{RowLabel: "test"}}}, {json: `{"options": {"rowLabl": "test"}}`, err: "invalid key for options {rowLabl:test}"}, - {json: `{"options": {"rowLabel": "test", "inverseEnabled": "true"}}`, expected: postFrameRequest{Options: FrameOptions{RowLabel: "test", InverseEnabled: true}}}, - {json: `{"options": {"rowLabel": "test", "inverseEnabled": "true", "cacheType": "type"}}`, expected: postFrameRequest{Options: FrameOptions{RowLabel: "test", InverseEnabled: true, CacheType: "type"}}}, - {json: `{"options": {"rowLabel": "test", "inverse": "true", "cacheType": "type"}}`, err: "invalid key for options {inverse:true}"}, + {json: `{"options": {"rowLabel": "test", "inverseEnabled": true}}`, expected: postFrameRequest{Options: FrameOptions{RowLabel: "test", InverseEnabled: true}}}, + {json: `{"options": {"rowLabel": "test", "inverseEnabled": true, "cacheType": "type"}}`, expected: postFrameRequest{Options: FrameOptions{RowLabel: "test", InverseEnabled: true, CacheType: "type"}}}, + {json: `{"options": {"rowLabel": "test", "inverse": true, "cacheType": "type"}}`, err: "invalid key for options {inverse:true}"}, } for _, test := range tests { actual := &postFrameRequest{} From 7e935dd90a04203817bd2449a909e8779de4da8a Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Fri, 21 Apr 2017 11:37:32 -0500 Subject: [PATCH 3/5] rewrite custom UnmarshalJSON for postDBRequest and postFrameRequest --- handler.go | 139 ++++++++++++++++++--------------------- handler_internal_test.go | 8 +-- 2 files changed, 69 insertions(+), 78 deletions(-) diff --git a/handler.go b/handler.go index 08ca21b31..adb3967e1 100644 --- a/handler.go +++ b/handler.go @@ -21,6 +21,7 @@ import ( "github.com/gorilla/mux" "github.com/pilosa/pilosa/internal" "github.com/pilosa/pilosa/pql" + "reflect" ) // Handler represents an HTTP handler. @@ -232,67 +233,53 @@ type postDBRequest struct { Options DBOptions `json:"options"` } +//_postDBRequest is necessary to avoid recursion while decoding. +type _postDBRequest postDBRequest + // Custom Unmarshal JSON to validate request body when creating a new database func (p *postDBRequest) UnmarshalJSON(b []byte) error { - validDBOptions := []string{"columnLabel"} - var data map[string]interface{} - if err := json.Unmarshal(b, &data); err != nil { + + // m is an overflow map used to capture additional, unexpected keys. + m := make(map[string]interface{}) + if err := json.Unmarshal(b, &m); err != nil { return err } - p.Options = DBOptions{} - for key, value := range data { - switch key { - case "options": - values, err := validateOptions(data, validDBOptions) - if err != nil { - return err - } - for k, v := range values { - switch k { - case "columnLabel": - p.Options.ColumnLabel = v.(string) - } - } - default: - return fmt.Errorf("Unknown key: %v:%v", key, value) - } + + validDBOptions := getValidOptions(DBOptions{}) + err := validateOptions(m, validDBOptions) + if err != nil { + return err } + // Unmarshal expected values. + var _p _postDBRequest + if err := json.Unmarshal(b, &_p); err != nil { + return err + } + + p.Options = _p.Options + return nil } -func validateOptions(data map[string]interface{}, field []string) (map[string]interface{}, error) { - options, ok := data["options"].(map[string]interface{}) - optionValue := make(map[string]interface{}) - if !ok { - return nil, errors.New("options is not map[string]interface{}") - } - - if len(options) == 0 { - optionValue = nil - } else { - for k, v := range options { - if foundItem(field, k) { - switch k { - case "inverseEnabled": - val, ok := options[k].(bool) - if !ok { - return nil, fmt.Errorf("invalid option type %v: {%v:%v}", field, k, v) - } - optionValue[k] = val - default: - val, ok := options[k].(string) - if !ok { - return nil, fmt.Errorf("invalid option %v: {%v:%v}", field, k, v) - } - optionValue[k] = val - } - - } else { - return nil, fmt.Errorf("invalid key for options {%v:%v}", k, v) +// Raise errors for any unknown key +func validateOptions(data map[string]interface{}, validDBOptions []string) error { + for k, v := range data { + switch k { + case "options": + options, ok := v.(map[string]interface{}) + if !ok { + return errors.New("options is not map[string]interface{}") } + for kk, vv := range options { + if !foundItem(validDBOptions, kk) { + return fmt.Errorf("Unknown key: %v:%v", kk, vv) + } + } + default: + return fmt.Errorf("Unknown key: %v:%v", k, v) } } - return optionValue, nil + return nil } func foundItem(items []string, item string) bool { @@ -486,41 +473,45 @@ func (h *Handler) handlePostFrame(w http.ResponseWriter, r *http.Request) { } } +type _postFrameRequest postFrameRequest + // Custom Unmarshal JSON to validate request body when creating a new frame. If there's new FrameOptions, // adding it to validFrameOptions to make sure the new option is validated, otherwise the request will be failed func (p *postFrameRequest) UnmarshalJSON(b []byte) error { - validFrameOptions := []string{"rowLabel", "cacheType", "inverseEnabled"} - var data map[string]interface{} - if err := json.Unmarshal(b, &data); err != nil { + // m is an overflow map used to capture additional, unexpected keys. + m := make(map[string]interface{}) + if err := json.Unmarshal(b, &m); err != nil { return err } - p.Options = FrameOptions{} - for key, value := range data { - switch key { - case "options": - values, err := validateOptions(data, validFrameOptions) - if err != nil { - return err - } - for k, v := range values { - switch k { - case "rowLabel": - p.Options.RowLabel = v.(string) - case "cacheType": - p.Options.CacheType = v.(string) - case "inverseEnabled": - p.Options.InverseEnabled = v.(bool) - } - } - default: - return fmt.Errorf("Unknown key: {%v:%v}", key, value) - } + validFrameOptions := getValidOptions(FrameOptions{}) + err := validateOptions(m, validFrameOptions) + if err != nil { + return err } + + // Unmarshal expected values. + var _p _postFrameRequest + if err := json.Unmarshal(b, &_p); err != nil { + return err + } + + p.Options = _p.Options return nil } +func getValidOptions(option interface{}) []string { + validFrameOptions := []string{} + val := reflect.ValueOf(option) + for i := 0; i < val.Type().NumField(); i++ { + jsonTag := val.Type().Field(i).Tag.Get("json") + s := strings.Split(jsonTag, ",") + validFrameOptions = append(validFrameOptions, s[0]) + } + return validFrameOptions +} + type postFrameRequest struct { Options FrameOptions `json:"options"` } diff --git a/handler_internal_test.go b/handler_internal_test.go index 5942bd12c..92221f1d7 100644 --- a/handler_internal_test.go +++ b/handler_internal_test.go @@ -17,7 +17,7 @@ func TestPostDBRequestUnmarshalJSON(t *testing.T) { {json: `{"options": 4}`, err: "options is not map[string]interface{}"}, {json: `{"option": {}}`, err: "Unknown key: option:map[]"}, {json: `{"options": {"columnLabel": "test"}}`, expected: postDBRequest{Options: DBOptions{ColumnLabel: "test"}}}, - {json: `{"options": {"columnLabl": "test"}}`, err: "invalid key for options {columnLabl:test}"}, + {json: `{"options": {"columnLabl": "test"}}`, err: "Unknown key: columnLabl:test"}, } for _, test := range tests { actual := &postDBRequest{} @@ -51,12 +51,12 @@ func TestPostFrameRequestUnmarshalJSON(t *testing.T) { }{ {json: `{"options": {}}`, expected: postFrameRequest{Options: FrameOptions{}}}, {json: `{"options": 4}`, err: "options is not map[string]interface{}"}, - {json: `{"option": {}}`, err: "Unknown key: {option:map[]}"}, + {json: `{"option": {}}`, err: "Unknown key: option:map[]"}, {json: `{"options": {"rowLabel": "test"}}`, expected: postFrameRequest{Options: FrameOptions{RowLabel: "test"}}}, - {json: `{"options": {"rowLabl": "test"}}`, err: "invalid key for options {rowLabl:test}"}, + {json: `{"options": {"rowLabl": "test"}}`, err: "Unknown key: rowLabl:test"}, {json: `{"options": {"rowLabel": "test", "inverseEnabled": true}}`, expected: postFrameRequest{Options: FrameOptions{RowLabel: "test", InverseEnabled: true}}}, {json: `{"options": {"rowLabel": "test", "inverseEnabled": true, "cacheType": "type"}}`, expected: postFrameRequest{Options: FrameOptions{RowLabel: "test", InverseEnabled: true, CacheType: "type"}}}, - {json: `{"options": {"rowLabel": "test", "inverse": true, "cacheType": "type"}}`, err: "invalid key for options {inverse:true}"}, + {json: `{"options": {"rowLabel": "test", "inverse": true, "cacheType": "type"}}`, err: "Unknown key: inverse:true"}, } for _, test := range tests { actual := &postFrameRequest{} From e1404cf330881d89286c42cc578e3dfba0c24a49 Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Fri, 21 Apr 2017 11:42:31 -0500 Subject: [PATCH 4/5] change variable name of getValidOptions --- handler.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/handler.go b/handler.go index adb3967e1..5af464764 100644 --- a/handler.go +++ b/handler.go @@ -502,14 +502,14 @@ func (p *postFrameRequest) UnmarshalJSON(b []byte) error { } func getValidOptions(option interface{}) []string { - validFrameOptions := []string{} + validOptions := []string{} val := reflect.ValueOf(option) for i := 0; i < val.Type().NumField(); i++ { jsonTag := val.Type().Field(i).Tag.Get("json") s := strings.Split(jsonTag, ",") - validFrameOptions = append(validFrameOptions, s[0]) + validOptions = append(validOptions, s[0]) } - return validFrameOptions + return validOptions } type postFrameRequest struct { From 93f36d0e9ab18f51f30aae4f74d52c128ee43029 Mon Sep 17 00:00:00 2001 From: Travis Date: Fri, 21 Apr 2017 13:54:39 -0500 Subject: [PATCH 5/5] adds a basic BroadcastReceiver test --- broadcast_test.go | 54 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/broadcast_test.go b/broadcast_test.go index ee2d720a9..a2842a919 100644 --- a/broadcast_test.go +++ b/broadcast_test.go @@ -35,3 +35,57 @@ func testMessageMarshal(t *testing.T, m proto.Message) { t.Fatalf("unexpected message marshalling: %s", unmarshalled) } } + +// Ensure that BroadcastReceiver can register a BroadcastHandler. +func TestBroadcast_BroadcastReceiver(t *testing.T) { + + s := pilosa.NewServer() + + sbr := NewSimpleBroadcastReceiver() + sbh := NewSimpleBroadcastHandler() + + s.BroadcastReceiver = sbr + s.BroadcastReceiver.Start(sbh) + + msg := &internal.DeleteDBMessage{ + DB: "d", + } + + s.BroadcastReceiver.(*SimpleBroadcastReceiver).Receive(msg) + + // Make sure the message received is what was sentd + if !reflect.DeepEqual(sbh.receivedMessage, msg) { + t.Fatalf("unexpected message: %s", sbh.receivedMessage) + } +} + +type SimpleBroadcastReceiver struct { + broadcastHandler pilosa.BroadcastHandler +} + +func NewSimpleBroadcastReceiver() *SimpleBroadcastReceiver { + return &SimpleBroadcastReceiver{} +} + +func (r *SimpleBroadcastReceiver) Start(h pilosa.BroadcastHandler) error { + r.broadcastHandler = h + return nil +} + +func (r *SimpleBroadcastReceiver) Receive(pb proto.Message) error { + r.broadcastHandler.ReceiveMessage(pb) + return nil +} + +type SimpleBroadcastHandler struct { + receivedMessage proto.Message +} + +func NewSimpleBroadcastHandler() *SimpleBroadcastHandler { + return &SimpleBroadcastHandler{} +} + +func (h *SimpleBroadcastHandler) ReceiveMessage(pb proto.Message) error { + h.receivedMessage = pb.(proto.Message) + return nil +}