From b20fde12b1f05dd798c28d5bafeef451f23bd291 Mon Sep 17 00:00:00 2001 From: Andrew Stone Date: Wed, 17 Oct 2018 12:25:34 -0700 Subject: [PATCH 1/4] Add go.mod --- go.mod | 1 + 1 file changed, 1 insertion(+) create mode 100644 go.mod diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..f1b3ebd --- /dev/null +++ b/go.mod @@ -0,0 +1 @@ +module github.com/goji/param From 2a809f292b9bfbc41d440ae98546fc7a670cc045 Mon Sep 17 00:00:00 2001 From: Andrew Stone Date: Wed, 17 Oct 2018 13:20:51 -0700 Subject: [PATCH 2/4] Use conventional error handling --- crazy_test.go | 56 --------------- error_helpers.go | 25 ------- errors.go | 17 +++++ param.go | 32 +++++---- param_test.go | 141 +++++++++++++++++++++++++++++++++--- parse.go | 182 ++++++++++++++++++++++++++++++----------------- pebkac_test.go | 58 --------------- struct.go | 56 ++++++++------- struct_test.go | 10 ++- 9 files changed, 322 insertions(+), 255 deletions(-) delete mode 100644 crazy_test.go delete mode 100644 error_helpers.go delete mode 100644 pebkac_test.go diff --git a/crazy_test.go b/crazy_test.go deleted file mode 100644 index 46538cd..0000000 --- a/crazy_test.go +++ /dev/null @@ -1,56 +0,0 @@ -package param - -import ( - "net/url" - "testing" -) - -type Crazy struct { - A *Crazy - B *Crazy - Value int - Slice []int - Map map[string]Crazy -} - -func TestCrazy(t *testing.T) { - t.Parallel() - - c := Crazy{} - err := Parse(url.Values{ - "A[B][B][A][Value]": {"1"}, - "B[A][A][Slice][]": {"3", "1", "4"}, - "B[Map][hello][A][Value]": {"8"}, - "A[Value]": {"2"}, - "A[Slice][]": {"9", "1", "1"}, - "Value": {"42"}, - }, &c) - if err != nil { - t.Error("Error parsing craziness: ", err) - } - - // Exhaustively checking everything here is going to be a huge pain, so - // let's just hope for the best, pretend NPEs don't exist, and hope that - // this test covers enough stuff that it's actually useful. - assertEqual(t, "c.A.B.B.A.Value", 1, c.A.B.B.A.Value) - assertEqual(t, "c.A.Value", 2, c.A.Value) - assertEqual(t, "c.Value", 42, c.Value) - assertEqual(t, `c.B.Map["hello"].A.Value`, 8, c.B.Map["hello"].A.Value) - - assertEqual(t, "c.A.B.B.B", (*Crazy)(nil), c.A.B.B.B) - assertEqual(t, "c.A.B.A", (*Crazy)(nil), c.A.B.A) - assertEqual(t, "c.A.A", (*Crazy)(nil), c.A.A) - - if c.Slice != nil || c.Map != nil { - t.Error("Map and Slice should not be set") - } - - sl := c.B.A.A.Slice - if len(sl) != 3 || sl[0] != 3 || sl[1] != 1 || sl[2] != 4 { - t.Error("Something is wrong with c.B.A.A.Slice") - } - sl = c.A.Slice - if len(sl) != 3 || sl[0] != 9 || sl[1] != 1 || sl[2] != 1 { - t.Error("Something is wrong with c.A.Slice") - } -} diff --git a/error_helpers.go b/error_helpers.go deleted file mode 100644 index 9477d3a..0000000 --- a/error_helpers.go +++ /dev/null @@ -1,25 +0,0 @@ -package param - -import ( - "errors" - "fmt" - "log" -) - -// Testing log.Fatal in tests is... not a thing. Allow tests to stub it out. -var pebkacTesting bool - -const errPrefix = "param/parse: " -const yourFault = " This is a bug in your use of the param library." - -// Problem exists between keyboard and chair. This function is used in cases of -// programmer error, i.e. an inappropriate use of the param library, to -// immediately force the program to halt with a hopefully helpful error message. -func pebkac(format string, a ...interface{}) { - err := errors.New(errPrefix + fmt.Sprintf(format, a...) + yourFault) - if pebkacTesting { - panic(err) - } else { - log.Fatal(err) - } -} diff --git a/errors.go b/errors.go index e6b9de6..e91dfec 100644 --- a/errors.go +++ b/errors.go @@ -110,3 +110,20 @@ func (k KeyError) Error() string { return fmt.Sprintf("param: error parsing key %q: unknown field %q on "+ "struct %q of type %v", k.FullKey, k.Field, k.Key, k.Type) } + +// InvalidParseError describes an invalid argument passed to Parse. It is always +// the result of programmer error. +type InvalidParseError struct { + Type reflect.Type + Hint string +} + +func (err InvalidParseError) Error() string { + msg := fmt.Sprintf("param/parse: unsupported type %v", err.Type) + + if err.Hint != "" { + msg += ": " + err.Hint + } + + return msg +} diff --git a/param.go b/param.go index 685df90..1c55f6d 100644 --- a/param.go +++ b/param.go @@ -26,34 +26,38 @@ import ( ) // Parse the given arguments into the the given pointer to a struct object. -func Parse(params url.Values, target interface{}) (err error) { +func Parse(params url.Values, target interface{}) error { v := reflect.ValueOf(target) - defer func() { - if r := recover(); r != nil { - var ok bool - err, ok = r.(error) - if !ok { - panic(err) - } + if v.Kind() != reflect.Ptr || v.Elem().Kind() != reflect.Struct { + hint := "target must be a pointer to a struct" + if v.Kind() == reflect.Ptr && v.IsNil() { + hint = "target may not be a nil pointer" } - }() - if v.Kind() != reflect.Ptr || v.Elem().Kind() != reflect.Struct { - pebkac("Target of param.Parse must be a pointer to a struct. "+ - "We instead were passed a %v", v.Type()) + return InvalidParseError{ + Type: v.Type(), + Hint: hint, + } } el := v.Elem() t := el.Type() - cache := cacheStruct(t) + cache, err := cacheStruct(t) + if err != nil { + return err + } for key, values := range params { sk, keytail := key, "" if i := strings.IndexRune(key, '['); i != -1 { sk, keytail = sk[:i], sk[i:] } - parseStructField(cache, key, sk, keytail, values, el) + + err := parseStructField(cache, key, sk, keytail, values, el) + if err != nil { + return err + } } return nil diff --git a/param_test.go b/param_test.go index 7ee81d3..1437bf0 100644 --- a/param_test.go +++ b/param_test.go @@ -71,7 +71,7 @@ func init() { testTime, _ = time.Parse(time.RFC3339, testTimeString) } -func singletonErrors(t *testing.T, field, valid, invalid string) { +func singletonErrorsWithInvalid(t *testing.T, field, valid, invalid string) { e := Everything{} err := Parse(url.Values{field: {invalid}}, &e) @@ -79,17 +79,23 @@ func singletonErrors(t *testing.T, field, valid, invalid string) { t.Errorf("Expected error parsing %q as %s", invalid, field) } - err = Parse(url.Values{field + "[]": {valid}}, &e) + singletonErrors(t, field, valid) +} + +func singletonErrors(t *testing.T, field, val string) { + e := Everything{} + + err := Parse(url.Values{field + "[]": {val}}, &e) if err == nil { t.Errorf("Expected error parsing nested %s", field) } - err = Parse(url.Values{field + "[nested]": {valid}}, &e) + err = Parse(url.Values{field + "[nested]": {val}}, &e) if err == nil { t.Errorf("Expected error parsing nested %s", field) } - err = Parse(url.Values{field: {valid, valid}}, &e) + err = Parse(url.Values{field: {val, val}}, &e) if err == nil { t.Errorf("Expected error passing %s twice", field) } @@ -122,7 +128,7 @@ func TestBoolTyped(t *testing.T) { func TestBoolErrors(t *testing.T) { t.Parallel() - singletonErrors(t, "Bool", "true", "llama") + singletonErrorsWithInvalid(t, "Bool", "true", "llama") } var intAnswers = map[string]int{ @@ -158,7 +164,7 @@ func TestIntTyped(t *testing.T) { func TestIntErrors(t *testing.T) { t.Parallel() - singletonErrors(t, "Int", "1", "llama") + singletonErrorsWithInvalid(t, "Int", "1", "llama") e := Everything{} err := Parse(url.Values{"Int": {"4.2"}}, &e) @@ -199,7 +205,7 @@ func TestUintTyped(t *testing.T) { func TestUintErrors(t *testing.T) { t.Parallel() - singletonErrors(t, "Uint", "1", "llama") + singletonErrorsWithInvalid(t, "Uint", "1", "llama") e := Everything{} err := Parse(url.Values{"Uint": {"4.2"}}, &e) @@ -249,7 +255,7 @@ func TestFloatTyped(t *testing.T) { func TestFloatErrors(t *testing.T) { t.Parallel() - singletonErrors(t, "Float", "1.0", "llama") + singletonErrorsWithInvalid(t, "Float", "1.0", "llama") } func TestMap(t *testing.T) { @@ -434,6 +440,17 @@ func TestStringTyped(t *testing.T) { assertEqual(t, "e.AString", MyString("llama"), e.AString) } +func TestStringErrors(t *testing.T) { + t.Parallel() + singletonErrors(t, "String", "str") + + e := Everything{} + err := Parse(url.Values{"Int": {"4.2"}}, &e) + if err == nil { + t.Error("Expected error parsing float as int") + } +} + func TestStruct(t *testing.T) { t.Parallel() e := Everything{} @@ -500,10 +517,112 @@ func TestTextUnmarshaler(t *testing.T) { func TestTextUnmarshalerError(t *testing.T) { t.Parallel() - e := Everything{} - err := Parse(url.Values{"Time": {"llama"}}, &e) + now := time.Now().Format(time.RFC3339) + singletonErrorsWithInvalid(t, "Time", now, "llama") +} + +type Crazy struct { + A *Crazy + B *Crazy + Value int + Slice []int + Map map[string]Crazy +} + +func TestCrazy(t *testing.T) { + t.Parallel() + + c := Crazy{} + err := Parse(url.Values{ + "A[B][B][A][Value]": {"1"}, + "B[A][A][Slice][]": {"3", "1", "4"}, + "B[Map][hello][A][Value]": {"8"}, + "A[Value]": {"2"}, + "A[Slice][]": {"9", "1", "1"}, + "Value": {"42"}, + }, &c) + if err != nil { + t.Error("Error parsing craziness: ", err) + } + + // Exhaustively checking everything here is going to be a huge pain, so + // let's just hope for the best, pretend NPEs don't exist, and hope that + // this test covers enough stuff that it's actually useful. + assertEqual(t, "c.A.B.B.A.Value", 1, c.A.B.B.A.Value) + assertEqual(t, "c.A.Value", 2, c.A.Value) + assertEqual(t, "c.Value", 42, c.Value) + assertEqual(t, `c.B.Map["hello"].A.Value`, 8, c.B.Map["hello"].A.Value) + + assertEqual(t, "c.A.B.B.B", (*Crazy)(nil), c.A.B.B.B) + assertEqual(t, "c.A.B.A", (*Crazy)(nil), c.A.B.A) + assertEqual(t, "c.A.A", (*Crazy)(nil), c.A.A) + + if c.Slice != nil || c.Map != nil { + t.Error("Map and Slice should not be set") + } + + sl := c.B.A.A.Slice + if len(sl) != 3 || sl[0] != 3 || sl[1] != 1 || sl[2] != 4 { + t.Error("Something is wrong with c.B.A.A.Slice") + } + sl = c.A.Slice + if len(sl) != 3 || sl[0] != 9 || sl[1] != 1 || sl[2] != 1 { + t.Error("Something is wrong with c.A.Slice") + } +} + +func TestParseNilPtr(t *testing.T) { + var s *struct{} + + err := Parse(url.Values{"A": {"123"}}, s) + if err == nil { + t.Errorf("Expected error parsing %T", s) + } +} + +func TestUnsupportedStructKind(t *testing.T) { + var s struct { + Bad chan string + } + + err := Parse(url.Values{"Bad": {"123"}}, &s) + if err == nil { + t.Errorf("Expected error parsing %T", s) + } +} + +func TestUnsupportedNestedStruct(t *testing.T) { + var s struct { + Bad struct { + A chan string + } + } + + err := Parse(url.Values{"Bad[A]": {"123"}}, &s) + if err == nil { + t.Errorf("Expected error parsing %T", s) + } +} + +func TestUnsupportedParseKind(t *testing.T) { + var s struct { + Bad []chan string + } + + err := Parse(url.Values{"Bad[]": {"123"}}, &s) + if err == nil { + t.Errorf("Expected error parsing %T", s) + } +} + +func TestUnsupportedMapKey(t *testing.T) { + var s struct { + Bad map[int]int + } + + err := Parse(url.Values{"Bad[123]": {"123"}}, &s) if err == nil { - t.Error("expected error parsing llama as time") + t.Errorf("Expected error parsing %T", s) } } diff --git a/parse.go b/parse.go index 6386da7..8e25719 100644 --- a/parse.go +++ b/parse.go @@ -16,36 +16,35 @@ var textUnmarshalerType = reflect.TypeOf((*encoding.TextUnmarshaler)(nil)).Elem( // parser is responsible for, for instance "[bar][]". `values` is the list of // values assigned to this key, and `target` is where the resulting typed value // should be Set() to. -func parse(key, keytail string, values []string, target reflect.Value) { +func parse(key, keytail string, values []string, target reflect.Value) error { t := target.Type() if reflect.PtrTo(t).Implements(textUnmarshalerType) { - parseTextUnmarshaler(key, keytail, values, target) - return + return parseTextUnmarshaler(key, keytail, values, target) } switch k := target.Kind(); k { case reflect.Bool: - parseBool(key, keytail, values, target) + return parseBool(key, keytail, values, target) case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - parseInt(key, keytail, values, target) + return parseInt(key, keytail, values, target) case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - parseUint(key, keytail, values, target) + return parseUint(key, keytail, values, target) case reflect.Float32, reflect.Float64: - parseFloat(key, keytail, values, target) + return parseFloat(key, keytail, values, target) case reflect.Map: - parseMap(key, keytail, values, target) + return parseMap(key, keytail, values, target) case reflect.Ptr: - parsePtr(key, keytail, values, target) + return parsePtr(key, keytail, values, target) case reflect.Slice: - parseSlice(key, keytail, values, target) + return parseSlice(key, keytail, values, target) case reflect.String: - parseString(key, keytail, values, target) + return parseString(key, keytail, values, target) case reflect.Struct: - parseStruct(key, keytail, values, target) - + return parseStruct(key, keytail, values, target) default: - pebkac("unsupported object of type %v and kind %v.", - target.Type(), k) + return InvalidParseError{ + Type: target.Type(), + } } } @@ -59,137 +58,168 @@ func kpath(key, keytail string) string { // Helper for validating that a value has been passed exactly once, and that the // user is not attempting to nest on the key. -func primitive(key, keytail string, tipe reflect.Type, values []string) { +func primitive(key, keytail string, tipe reflect.Type, values []string) error { if keytail != "" { - panic(NestingError{ + return NestingError{ Key: kpath(key, keytail), Type: tipe, Nesting: keytail, - }) + } } + if len(values) != 1 { - panic(SingletonError{ + return SingletonError{ Key: kpath(key, keytail), Type: tipe, Values: values, - }) + } } + + return nil } -func keyed(tipe reflect.Type, key, keytail string) (string, string) { +func keyed(tipe reflect.Type, key, keytail string) (string, string, error) { if keytail == "" || keytail[0] != '[' { - panic(SyntaxError{ + return "", "", SyntaxError{ Key: kpath(key, keytail), Subtype: MissingOpeningBracket, ErrorPart: keytail, - }) + } } idx := strings.IndexRune(keytail, ']') if idx == -1 { - panic(SyntaxError{ + return "", "", SyntaxError{ Key: kpath(key, keytail), Subtype: MissingClosingBracket, ErrorPart: keytail[1:], - }) + } } - return keytail[1:idx], keytail[idx+1:] + return keytail[1:idx], keytail[idx+1:], nil } -func parseTextUnmarshaler(key, keytail string, values []string, target reflect.Value) { - primitive(key, keytail, target.Type(), values) +func parseTextUnmarshaler(key, keytail string, values []string, target reflect.Value) error { + err := primitive(key, keytail, target.Type(), values) + if err != nil { + return err + } tu := target.Addr().Interface().(encoding.TextUnmarshaler) - err := tu.UnmarshalText([]byte(values[0])) + err = tu.UnmarshalText([]byte(values[0])) if err != nil { - panic(TypeError{ + return TypeError{ Key: kpath(key, keytail), Type: target.Type(), Err: err, - }) + } } + + return nil } -func parseBool(key, keytail string, values []string, target reflect.Value) { - primitive(key, keytail, target.Type(), values) +func parseBool(key, keytail string, values []string, target reflect.Value) error { + err := primitive(key, keytail, target.Type(), values) + if err != nil { + return err + } switch values[0] { case "true", "1", "on": target.SetBool(true) + return nil case "false", "0", "": target.SetBool(false) + return nil default: - panic(TypeError{ + return TypeError{ Key: kpath(key, keytail), Type: target.Type(), - }) + } } } -func parseInt(key, keytail string, values []string, target reflect.Value) { +func parseInt(key, keytail string, values []string, target reflect.Value) error { t := target.Type() - primitive(key, keytail, t, values) + err := primitive(key, keytail, t, values) + if err != nil { + return err + } i, err := strconv.ParseInt(values[0], 10, t.Bits()) if err != nil { - panic(TypeError{ + return TypeError{ Key: kpath(key, keytail), Type: t, Err: err, - }) + } } + target.SetInt(i) + return nil } -func parseUint(key, keytail string, values []string, target reflect.Value) { +func parseUint(key, keytail string, values []string, target reflect.Value) error { t := target.Type() - primitive(key, keytail, t, values) + err := primitive(key, keytail, t, values) + if err != nil { + return err + } i, err := strconv.ParseUint(values[0], 10, t.Bits()) if err != nil { - panic(TypeError{ + return TypeError{ Key: kpath(key, keytail), Type: t, Err: err, - }) + } } + target.SetUint(i) + return nil } -func parseFloat(key, keytail string, values []string, target reflect.Value) { +func parseFloat(key, keytail string, values []string, target reflect.Value) error { t := target.Type() - primitive(key, keytail, t, values) + err := primitive(key, keytail, t, values) + if err != nil { + return err + } f, err := strconv.ParseFloat(values[0], t.Bits()) if err != nil { - panic(TypeError{ + return TypeError{ Key: kpath(key, keytail), Type: t, Err: err, - }) + } } target.SetFloat(f) + return nil } -func parseString(key, keytail string, values []string, target reflect.Value) { - primitive(key, keytail, target.Type(), values) +func parseString(key, keytail string, values []string, target reflect.Value) error { + err := primitive(key, keytail, target.Type(), values) + if err != nil { + return err + } target.SetString(values[0]) + return nil } -func parseSlice(key, keytail string, values []string, target reflect.Value) { +func parseSlice(key, keytail string, values []string, target reflect.Value) error { t := target.Type() // BUG(carl): We currently do not handle slices of nested types. If // support is needed, the implementation probably could be fleshed out. if keytail != "[]" { - panic(NestingError{ + return NestingError{ Key: kpath(key, keytail), Type: t, Nesting: keytail, - }) + } } slice := reflect.MakeSlice(t, len(values), len(values)) @@ -198,14 +228,22 @@ func parseSlice(key, keytail string, values []string, target reflect.Value) { // We actually cheat a little bit and modify the key so we can // generate better debugging messages later key := fmt.Sprintf("%s[%d]", kp, i) - parse(key, "", values[i:i+1], slice.Index(i)) + err := parse(key, "", values[i:i+1], slice.Index(i)) + if err != nil { + return err + } } + target.Set(slice) + return nil } -func parseMap(key, keytail string, values []string, target reflect.Value) { +func parseMap(key, keytail string, values []string, target reflect.Value) error { t := target.Type() - mapkey, maptail := keyed(t, key, keytail) + mapkey, maptail, err := keyed(t, key, keytail) + if err != nil { + return err + } // BUG(carl): We don't support any map keys except strings, although // there's no reason we shouldn't be able to throw the value through our @@ -214,7 +252,10 @@ func parseMap(key, keytail string, values []string, target reflect.Value) { if t.Key().Kind() == reflect.String { mk = reflect.ValueOf(mapkey).Convert(t.Key()) } else { - pebkac("key for map %v isn't a string (it's a %v).", t, t.Key()) + return InvalidParseError{ + Type: t, + Hint: fmt.Sprintf("map keys must be strings, not %v", t.Key()), + } } if target.IsNil() { @@ -227,23 +268,36 @@ func parseMap(key, keytail string, values []string, target reflect.Value) { // MapIndex isn't Set()table if the key exists. val = reflect.New(t.Elem()).Elem() } - parse(key, maptail, values, val) + + err = parse(key, maptail, values, val) + if err != nil { + return err + } + target.SetMapIndex(mk, val) + return nil } -func parseStruct(key, keytail string, values []string, target reflect.Value) { +func parseStruct(key, keytail string, values []string, target reflect.Value) error { t := target.Type() - sk, skt := keyed(t, key, keytail) - cache := cacheStruct(t) + sk, skt, err := keyed(t, key, keytail) + if err != nil { + return err + } + + cache, err := cacheStruct(t) + if err != nil { + return err + } - parseStructField(cache, key, sk, skt, values, target) + return parseStructField(cache, key, sk, skt, values, target) } -func parsePtr(key, keytail string, values []string, target reflect.Value) { +func parsePtr(key, keytail string, values []string, target reflect.Value) error { t := target.Type() - if target.IsNil() { target.Set(reflect.New(t.Elem())) } - parse(key, keytail, values, target.Elem()) + + return parse(key, keytail, values, target.Elem()) } diff --git a/pebkac_test.go b/pebkac_test.go deleted file mode 100644 index 71d64eb..0000000 --- a/pebkac_test.go +++ /dev/null @@ -1,58 +0,0 @@ -package param - -import ( - "net/url" - "strings" - "testing" -) - -type Bad struct { - Unknown interface{} -} - -type Bad2 struct { - Unknown *interface{} -} - -type Bad3 struct { - BadMap map[int]int -} - -// These tests are not parallel so we can frob pebkac behavior in an isolated -// way - -func assertPebkac(t *testing.T, err error) { - if err == nil { - t.Error("Expected PEBKAC error message") - } else if !strings.HasSuffix(err.Error(), yourFault) { - t.Errorf("Expected PEBKAC error, but got: %v", err) - } -} - -func TestBadInputs(t *testing.T) { - pebkacTesting = true - - err := Parse(url.Values{"Unknown": {"4"}}, Bad{}) - assertPebkac(t, err) - - b := &Bad{} - err = Parse(url.Values{"Unknown": {"4"}}, &b) - assertPebkac(t, err) - - pebkacTesting = false -} - -func TestBadTypes(t *testing.T) { - pebkacTesting = true - - err := Parse(url.Values{"Unknown": {"4"}}, &Bad{}) - assertPebkac(t, err) - - err = Parse(url.Values{"Unknown": {"4"}}, &Bad2{}) - assertPebkac(t, err) - - err = Parse(url.Values{"BadMap[llama]": {"4"}}, &Bad3{}) - assertPebkac(t, err) - - pebkacTesting = false -} diff --git a/struct.go b/struct.go index 56094ba..c1d0af8 100644 --- a/struct.go +++ b/struct.go @@ -1,6 +1,7 @@ package param import ( + "fmt" "reflect" "strings" "sync" @@ -16,19 +17,19 @@ import ( type structCache map[string]cacheLine type cacheLine struct { offset int - parse func(string, string, []string, reflect.Value) + parse func(string, string, []string, reflect.Value) error } var cacheLock sync.RWMutex var cache = make(map[reflect.Type]structCache) -func cacheStruct(t reflect.Type) structCache { +func cacheStruct(t reflect.Type) (structCache, error) { cacheLock.RLock() sc, ok := cache[t] cacheLock.RUnlock() if ok { - return sc + return sc, nil } // It's okay if two people build struct caches simultaneously @@ -42,7 +43,12 @@ func cacheStruct(t reflect.Type) structCache { } name := extractName(sf) if name != "-" { - sc[name] = cacheLine{i, extractHandler(t, sf)} + h, err := extractHandler(t, sf) + if err != nil { + return nil, err + } + + sc[name] = cacheLine{i, h} } } @@ -50,7 +56,7 @@ func cacheStruct(t reflect.Type) structCache { cache[t] = sc cacheLock.Unlock() - return sc + return sc, nil } // Extract the name of the given struct field, looking at struct tags as @@ -71,51 +77,51 @@ func extractName(sf reflect.StructField) string { return name } -func extractHandler(s reflect.Type, sf reflect.StructField) func(string, string, []string, reflect.Value) { +func extractHandler(s reflect.Type, sf reflect.StructField) (func(string, string, []string, reflect.Value) error, error) { if reflect.PtrTo(sf.Type).Implements(textUnmarshalerType) { - return parseTextUnmarshaler + return parseTextUnmarshaler, nil } switch sf.Type.Kind() { case reflect.Bool: - return parseBool + return parseBool, nil case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - return parseInt + return parseInt, nil case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - return parseUint + return parseUint, nil case reflect.Float32, reflect.Float64: - return parseFloat + return parseFloat, nil case reflect.Map: - return parseMap + return parseMap, nil case reflect.Ptr: - return parsePtr + return parsePtr, nil case reflect.Slice: - return parseSlice + return parseSlice, nil case reflect.String: - return parseString + return parseString, nil case reflect.Struct: - return parseStruct - + return parseStruct, nil default: - pebkac("struct %v has illegal field %q (type %v, kind %v).", - s, sf.Name, sf.Type, sf.Type.Kind()) - return nil + return nil, InvalidParseError{ + Type: s, + Hint: fmt.Sprintf("field %q in struct %v", sf.Name, s), + } } } // We have to parse two types of structs: ones at the top level, whose keys // don't have square brackets around them, and nested structs, which do. -func parseStructField(cache structCache, key, sk, keytail string, values []string, target reflect.Value) { +func parseStructField(cache structCache, key, sk, keytail string, values []string, target reflect.Value) error { l, ok := cache[sk] if !ok { - panic(KeyError{ + return KeyError{ FullKey: key, Key: kpath(key, keytail), Type: target.Type(), Field: sk, - }) + } } - f := target.Field(l.offset) - l.parse(key, keytail, values, f) + f := target.Field(l.offset) + return l.parse(key, keytail, values, f) } diff --git a/struct_test.go b/struct_test.go index ecba3e2..a37271c 100644 --- a/struct_test.go +++ b/struct_test.go @@ -68,7 +68,10 @@ func TestNames(t *testing.T) { func TestCacheStruct(t *testing.T) { t.Parallel() - sc := cacheStruct(fruityType) + sc, err := cacheStruct(fruityType) + if err != nil { + t.Errorf("Failed to cache struct: %v", err) + } if len(sc) != len(fruityCache) { t.Errorf("Cache has %d keys, but expected %d", len(sc), @@ -99,7 +102,10 @@ func TestCacheStruct(t *testing.T) { func TestPrivate(t *testing.T) { t.Parallel() - sc := cacheStruct(privateType) + sc, err := cacheStruct(privateType) + if err != nil { + t.Errorf("Failed to cache struct: %v", err) + } if len(sc) != 1 { t.Error("Expected Private{} to have one cachable field") } From 9a639f394364242398b58301e78c9d93b65752ea Mon Sep 17 00:00:00 2001 From: Andrew Stone Date: Wed, 17 Oct 2018 13:30:14 -0700 Subject: [PATCH 3/4] parse{num}: Return the error from strconv.NumError Returning the full error just adds extra noise to the error since it's already clear that there was an error with the number. This now says exactly what is wrong with the number. --- parse.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/parse.go b/parse.go index 8e25719..3050c48 100644 --- a/parse.go +++ b/parse.go @@ -151,7 +151,7 @@ func parseInt(key, keytail string, values []string, target reflect.Value) error return TypeError{ Key: kpath(key, keytail), Type: t, - Err: err, + Err: err.(*strconv.NumError).Err, } } @@ -171,7 +171,7 @@ func parseUint(key, keytail string, values []string, target reflect.Value) error return TypeError{ Key: kpath(key, keytail), Type: t, - Err: err, + Err: err.(*strconv.NumError).Err, } } @@ -191,7 +191,7 @@ func parseFloat(key, keytail string, values []string, target reflect.Value) erro return TypeError{ Key: kpath(key, keytail), Type: t, - Err: err, + Err: err.(*strconv.NumError).Err, } } From bfb29fd46d8c89f255ca0475c53d74ef81822e19 Mon Sep 17 00:00:00 2001 From: Andrew Stone Date: Wed, 17 Oct 2018 13:33:11 -0700 Subject: [PATCH 4/4] Rename TypeError to ValueError A TypeError typically refers to the go type of something. This error is used to indicate that the value cannot be parsed correctly. --- errors.go | 12 ++++++------ parse.go | 10 +++++----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/errors.go b/errors.go index e91dfec..8c9bc0e 100644 --- a/errors.go +++ b/errors.go @@ -5,9 +5,9 @@ import ( "reflect" ) -// TypeError is an error type returned when param has difficulty deserializing a -// parameter value. -type TypeError struct { +// ValueError is an error type returned when param has difficulty deserializing +// a parameter value. +type ValueError struct { // The key that was in error. Key string // The type that was expected. @@ -17,9 +17,9 @@ type TypeError struct { Err error } -func (t TypeError) Error() string { - return fmt.Sprintf("param: error parsing key %q as %v: %v", t.Key, t.Type, - t.Err) +func (v ValueError) Error() string { + return fmt.Sprintf("param: error parsing key %q as %v: %v", + v.Key, v.Type, v.Err) } // SingletonError is an error type returned when a parameter is passed multiple diff --git a/parse.go b/parse.go index 3050c48..960c873 100644 --- a/parse.go +++ b/parse.go @@ -108,7 +108,7 @@ func parseTextUnmarshaler(key, keytail string, values []string, target reflect.V tu := target.Addr().Interface().(encoding.TextUnmarshaler) err = tu.UnmarshalText([]byte(values[0])) if err != nil { - return TypeError{ + return ValueError{ Key: kpath(key, keytail), Type: target.Type(), Err: err, @@ -132,7 +132,7 @@ func parseBool(key, keytail string, values []string, target reflect.Value) error target.SetBool(false) return nil default: - return TypeError{ + return ValueError{ Key: kpath(key, keytail), Type: target.Type(), } @@ -148,7 +148,7 @@ func parseInt(key, keytail string, values []string, target reflect.Value) error i, err := strconv.ParseInt(values[0], 10, t.Bits()) if err != nil { - return TypeError{ + return ValueError{ Key: kpath(key, keytail), Type: t, Err: err.(*strconv.NumError).Err, @@ -168,7 +168,7 @@ func parseUint(key, keytail string, values []string, target reflect.Value) error i, err := strconv.ParseUint(values[0], 10, t.Bits()) if err != nil { - return TypeError{ + return ValueError{ Key: kpath(key, keytail), Type: t, Err: err.(*strconv.NumError).Err, @@ -188,7 +188,7 @@ func parseFloat(key, keytail string, values []string, target reflect.Value) erro f, err := strconv.ParseFloat(values[0], t.Bits()) if err != nil { - return TypeError{ + return ValueError{ Key: kpath(key, keytail), Type: t, Err: err.(*strconv.NumError).Err,