From 0f45d27028ba2aca0dbe7397b3ec874eacc950c2 Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Tue, 23 May 2017 14:40:58 -0500 Subject: [PATCH 1/6] #33 validate config --- cmd/root.go | 38 +++++++++++++++++++++++++++++++++++++- cmd/root_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/cmd/root.go b/cmd/root.go index d50dd05b4..caea25992 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -22,6 +22,7 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" "github.com/spf13/viper" + "reflect" ) var ( @@ -109,6 +110,11 @@ func setAllConfig(v *viper.Viper, flags *pflag.FlagSet, envPrefix string) error v.AutomaticEnv() c := v.GetString("config") + var flagErr error + validTags := make(map[string]bool) + flags.VisitAll(func(f *pflag.Flag) { + validTags[f.Name] = true + }) // add config file to viper if c != "" { @@ -118,10 +124,16 @@ func setAllConfig(v *viper.Viper, flags *pflag.FlagSet, envPrefix string) error if err != nil { return fmt.Errorf("error reading configuration file '%s': %v", c, err) } + + for _, key := range v.AllKeys() { + if _, ok := validTags[key]; !ok { + return fmt.Errorf("invalid tag: %v", key) + } + } + } // set all values from viper - var flagErr error flags.VisitAll(func(f *pflag.Flag) { if flagErr != nil { return @@ -151,3 +163,27 @@ func setAllConfig(v *viper.Viper, flags *pflag.FlagSet, envPrefix string) error }) return flagErr } + +func GetValidTags(v interface{}) map[string]bool { + validTag := make(map[string]bool) + conf := reflect.ValueOf(v) + + for i := 0; i < conf.Type().NumField(); i++ { + field := conf.Field(i) + if field.Kind() == reflect.Struct { + tag := conf.Type().Field(i).Tag.Get("toml") + tagString := strings.Split(tag, ",") + val := reflect.ValueOf(field.Interface()) + for j := 0; j < val.Type().NumField(); j++ { + subTag := val.Type().Field(j).Tag.Get("toml") + subTagString := strings.Split(subTag, ",") + validTag[fmt.Sprintf("%s.%s", tagString[0], subTagString[0])] = true + } + } else { + tomlTag := conf.Type().Field(i).Tag.Get("toml") + s := strings.Split(tomlTag, ",") + validTag[s[0]] = true + } + } + return validTag +} diff --git a/cmd/root_test.go b/cmd/root_test.go index 16bc82a8a..d3aeeab1f 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -171,3 +171,27 @@ func TestRootCommand(t *testing.T) { t.Fatalf("Expected standard usage message from RootCommand, but err: '%v', output: '%s'", err, outStr) } } + +func TestRootCommand_Config(t *testing.T) { + file, err := ioutil.TempFile("", "test.conf") + if err != nil { + panic(err) + } + config := `data-dir = "/tmp/pil5_0" +bind = "127.0.0.1:15000" + +[cluster] + poll-interval = "2m0s" + replicas = 2 + partitions = 128 + hosts = [ + "127.0.0.1:15000", + "127.0.0.1:15001", + ]` + file.Write([]byte(config)) + file.Close() + _, err = ExecNewRootCommand(t, "server", "--config", file.Name()) + if err.Error() != "invalid tag: cluster.partitions" { + t.Fatalf("Expected invalid tag, but err: '%v'", err) + } +} From 0a19e9f413a6b715b5864453f17586be849f47fb Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Tue, 23 May 2017 14:44:16 -0500 Subject: [PATCH 2/6] #33 don't need reflect for get valid field --- cmd/root.go | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index caea25992..e506f2fed 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -22,7 +22,6 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" "github.com/spf13/viper" - "reflect" ) var ( @@ -163,27 +162,3 @@ func setAllConfig(v *viper.Viper, flags *pflag.FlagSet, envPrefix string) error }) return flagErr } - -func GetValidTags(v interface{}) map[string]bool { - validTag := make(map[string]bool) - conf := reflect.ValueOf(v) - - for i := 0; i < conf.Type().NumField(); i++ { - field := conf.Field(i) - if field.Kind() == reflect.Struct { - tag := conf.Type().Field(i).Tag.Get("toml") - tagString := strings.Split(tag, ",") - val := reflect.ValueOf(field.Interface()) - for j := 0; j < val.Type().NumField(); j++ { - subTag := val.Type().Field(j).Tag.Get("toml") - subTagString := strings.Split(subTag, ",") - validTag[fmt.Sprintf("%s.%s", tagString[0], subTagString[0])] = true - } - } else { - tomlTag := conf.Type().Field(i).Tag.Get("toml") - s := strings.Split(tomlTag, ",") - validTag[s[0]] = true - } - } - return validTag -} From 15853fb448b691fc6d7bd2f167f7c7eec489cd6f Mon Sep 17 00:00:00 2001 From: Yuce Tekol Date: Thu, 25 May 2017 15:49:55 +0300 Subject: [PATCH 3/6] Fix name, label validation off by one error; added tests --- pilosa.go | 4 ++-- pilosa_test.go | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 pilosa_test.go diff --git a/pilosa.go b/pilosa.go index a192364ed..13ade19ea 100644 --- a/pilosa.go +++ b/pilosa.go @@ -49,10 +49,10 @@ var ( ) // Regular expression to validate index and frame names. -var nameRegexp = regexp.MustCompile(`^[a-z][a-z0-9_-]{0,64}$`) +var nameRegexp = regexp.MustCompile(`^[a-z][a-z0-9_-]{0,63}$`) // Regular expression to validate row and column labels. -var labelRegexp = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9_-]{0,64}$`) +var labelRegexp = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9_-]{0,63}$`) // ColumnAttrSet represents a set of attributes for a vertical column in an index. // Can have a set of attributes attached to it. diff --git a/pilosa_test.go b/pilosa_test.go new file mode 100644 index 000000000..ad5f9b762 --- /dev/null +++ b/pilosa_test.go @@ -0,0 +1,55 @@ +package pilosa_test + +import ( + "testing" + + "github.com/pilosa/pilosa" +) + +func TestValidateName(t *testing.T) { + names := []string{ + "a", "ab", "ab1", "b-c", "d_e", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + } + for _, name := range names { + if pilosa.ValidateName(name) != nil { + t.Fatalf("Should be valid index name: %s", name) + } + } +} + +func TestValidateNameInvalid(t *testing.T) { + names := []string{ + "", "'", "^", "/", "\\", "A", "*", "a:b", "valid?no", "yüce", "1", "_", "-", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1", + } + for _, name := range names { + if pilosa.ValidateName(name) == nil { + t.Fatalf("Should be invalid index name: %s", name) + } + } +} + +func TestValidateLabel(t *testing.T) { + labels := []string{ + "a", "ab", "ab1", "d_e", "A", "Bc", "B1", "aB", "b-c", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + } + for _, label := range labels { + if pilosa.ValidateLabel(label) != nil { + t.Fatalf("Should be valid label: %s", label) + } + } +} + +func TestValidateLabelInvalid(t *testing.T) { + labels := []string{ + "", "1", "_", "-", "'", "^", "/", "\\", "*", "a:b", "valid?no", "yüce", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1", + } + for _, label := range labels { + if pilosa.ValidateLabel(label) == nil { + t.Fatalf("Should be invalid label: %s", label) + } + } +} From e34e9f9789a3fbb2266413298c03362f04a0d046 Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Thu, 25 May 2017 11:48:08 -0500 Subject: [PATCH 4/6] fixed comment, update validQueryArgs --- ctl/export.go | 2 +- handler.go | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/ctl/export.go b/ctl/export.go index fa6277f16..295e10039 100644 --- a/ctl/export.go +++ b/ctl/export.go @@ -31,7 +31,7 @@ type ExportCommand struct { // Name of the index & frame to export from. Index string Frame string - View string + View string // Filename to export to. Path string diff --git a/handler.go b/handler.go index a2e8868f1..f7b956bc6 100644 --- a/handler.go +++ b/handler.go @@ -844,7 +844,7 @@ func (h *Handler) readProtobufQueryRequest(r *http.Request) (*QueryRequest, erro // readURLQueryRequest parses query parameters from URL parameters from r. func (h *Handler) readURLQueryRequest(r *http.Request) (*QueryRequest, error) { q := r.URL.Query() - validQuery := h.getValidURLQuery(r) + validQuery := validOptions(QueryRequest{}) for key, _ := range q { if _, ok := validQuery[key]; !ok { return nil, errors.New("invalid query params") @@ -882,12 +882,13 @@ func (h *Handler) readURLQueryRequest(r *http.Request) (*QueryRequest, error) { }, nil } -func (h *Handler) getValidURLQuery(r *http.Request) map[string]bool { +// validOptions return all attributes of an interface with lower first character. +func validOptions(v interface{}) map[string]bool { validQuery := make(map[string]bool) - args := reflect.ValueOf(QueryRequest{}) + argsType := reflect.ValueOf(v) - for i := 0; i < args.Type().NumField(); i++ { - fieldName := args.Type().Field(i).Name + for i := 0; i < argsType.Type().NumField(); i++ { + fieldName := argsType.Type().Field(i).Name chars := []rune(fieldName) chars[0] = unicode.ToLower(chars[0]) fieldName = string(chars) From 51db1c77848327981a0eeb412401dfbbfde0603e Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Thu, 25 May 2017 12:07:09 -0500 Subject: [PATCH 5/6] fix review --- cmd/root.go | 2 +- cmd/root_test.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index e506f2fed..a34e8b0c7 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -126,7 +126,7 @@ func setAllConfig(v *viper.Viper, flags *pflag.FlagSet, envPrefix string) error for _, key := range v.AllKeys() { if _, ok := validTags[key]; !ok { - return fmt.Errorf("invalid tag: %v", key) + return fmt.Errorf("invalid option in configuration file: %v", key) } } diff --git a/cmd/root_test.go b/cmd/root_test.go index d3aeeab1f..e1aaaf31d 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -178,20 +178,20 @@ func TestRootCommand_Config(t *testing.T) { panic(err) } config := `data-dir = "/tmp/pil5_0" -bind = "127.0.0.1:15000" +bind = "127.0.0.1:10101" [cluster] poll-interval = "2m0s" replicas = 2 partitions = 128 hosts = [ - "127.0.0.1:15000", - "127.0.0.1:15001", + "127.0.0.1:10101", + "127.0.0.1:10111", ]` file.Write([]byte(config)) file.Close() _, err = ExecNewRootCommand(t, "server", "--config", file.Name()) - if err.Error() != "invalid tag: cluster.partitions" { - t.Fatalf("Expected invalid tag, but err: '%v'", err) + if err.Error() != "invalid option in configuration file: cluster.partitions" { + t.Fatalf("Expected invalid option in configuration file, but err: '%v'", err) } } From 3706fc90222d64877a2c79f994a44b5583fff615 Mon Sep 17 00:00:00 2001 From: Linh Vo Date: Thu, 25 May 2017 13:04:38 -0500 Subject: [PATCH 6/6] changed argType --- handler.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/handler.go b/handler.go index f7b956bc6..c4a2c2a06 100644 --- a/handler.go +++ b/handler.go @@ -885,10 +885,10 @@ func (h *Handler) readURLQueryRequest(r *http.Request) (*QueryRequest, error) { // validOptions return all attributes of an interface with lower first character. func validOptions(v interface{}) map[string]bool { validQuery := make(map[string]bool) - argsType := reflect.ValueOf(v) + argsType := reflect.ValueOf(v).Type() - for i := 0; i < argsType.Type().NumField(); i++ { - fieldName := argsType.Type().Field(i).Name + for i := 0; i < argsType.NumField(); i++ { + fieldName := argsType.Field(i).Name chars := []rune(fieldName) chars[0] = unicode.ToLower(chars[0]) fieldName = string(chars)