From 77521a33d3976725130c35bd89d7d915dead4bcf Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Mon, 24 Nov 2014 20:06:43 -0800 Subject: [PATCH 1/3] Add stack of previous values to converter --- pkg/conversion/converter.go | 56 ++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/pkg/conversion/converter.go b/pkg/conversion/converter.go index 920bf3e7afd..a3a3ab7238c 100644 --- a/pkg/conversion/converter.go +++ b/pkg/conversion/converter.go @@ -100,32 +100,34 @@ type Meta struct { // scope contains information about an ongoing conversion. type scope struct { - converter *Converter - meta *Meta - flags FieldMatchingFlags - srcTagStack []reflect.StructTag - destTagStack []reflect.StructTag + converter *Converter + meta *Meta + flags FieldMatchingFlags + + // srcStack & destStack are separate because they may not have a 1:1 + // relationship. + srcStack scopeStack + destStack scopeStack } -// push adds a level to the src/dest tag stacks. -func (s *scope) push() { - s.srcTagStack = append(s.srcTagStack, "") - s.destTagStack = append(s.destTagStack, "") +type scopeStackElem struct { + tag reflect.StructTag + value reflect.Value } -// pop removes a level to the src/dest tag stacks. -func (s *scope) pop() { - n := len(s.srcTagStack) - s.srcTagStack = s.srcTagStack[:n-1] - s.destTagStack = s.destTagStack[:n-1] +type scopeStack []scopeStackElem + +func (s *scopeStack) pop() { + n := len(*s) + *s = (*s)[:n-1] } -func (s *scope) setSrcTag(tag reflect.StructTag) { - s.srcTagStack[len(s.srcTagStack)-1] = tag +func (s *scopeStack) push(e scopeStackElem) { + *s = append(*s, e) } -func (s *scope) setDestTag(tag reflect.StructTag) { - s.destTagStack[len(s.destTagStack)-1] = tag +func (s *scopeStack) top() *scopeStackElem { + return &(*s)[len(*s)-1] } // Convert continues a conversion. @@ -135,12 +137,12 @@ func (s *scope) Convert(src, dest interface{}, flags FieldMatchingFlags) error { // SrcTag returns the tag of the struct containing the current source item, if any. func (s *scope) SrcTag() reflect.StructTag { - return s.srcTagStack[len(s.srcTagStack)-1] + return s.srcStack.top().tag } // DestTag returns the tag of the struct containing the current dest item, if any. func (s *scope) DestTag() reflect.StructTag { - return s.destTagStack[len(s.destTagStack)-1] + return s.destStack.top().tag } // Flags returns the flags with which the current conversion was started. @@ -265,7 +267,9 @@ func (c *Converter) Convert(src, dest interface{}, flags FieldMatchingFlags, met flags: flags, meta: meta, } - s.push() // Easy way to make SrcTag and DestTag never fail + // Leave something on the stack, so that calls to struct tag getters never fail. + s.srcStack.push(scopeStackElem{}) + s.destStack.push(scopeStackElem{}) return c.convert(sv, dv, s) } @@ -305,8 +309,10 @@ func (c *Converter) convert(sv, dv reflect.Value, scope *scope) error { c.Debug.Logf("Trying to convert '%v' to '%v'", st, dt) } - scope.push() - defer scope.pop() + scope.srcStack.push(scopeStackElem{value: sv}) + scope.destStack.push(scopeStackElem{value: dv}) + defer scope.srcStack.pop() + defer scope.destStack.pop() switch dv.Kind() { case reflect.Struct: @@ -375,11 +381,11 @@ func (c *Converter) convertStruct(sv, dv reflect.Value, scope *scope) error { if sf.IsValid() { // No need to check error, since we know it's valid. field, _ := st.FieldByName(f.Name) - scope.setSrcTag(field.Tag) + scope.srcStack.top().tag = field.Tag } if df.IsValid() { field, _ := dt.FieldByName(f.Name) - scope.setDestTag(field.Tag) + scope.destStack.top().tag = field.Tag } // TODO: set top level of scope.src/destTagStack with these field tags here. if !df.IsValid() || !sf.IsValid() { From 564c0870620c5a817863072967a8c92783d05131 Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Wed, 26 Nov 2014 11:00:17 -0800 Subject: [PATCH 2/3] produce more readable error messages --- pkg/conversion/converter.go | 61 ++++++++++++++++++++++++++++++++++--- 1 file changed, 57 insertions(+), 4 deletions(-) diff --git a/pkg/conversion/converter.go b/pkg/conversion/converter.go index a3a3ab7238c..9a5ea849ed9 100644 --- a/pkg/conversion/converter.go +++ b/pkg/conversion/converter.go @@ -113,6 +113,7 @@ type scope struct { type scopeStackElem struct { tag reflect.StructTag value reflect.Value + key string } type scopeStack []scopeStackElem @@ -130,6 +131,37 @@ func (s *scopeStack) top() *scopeStackElem { return &(*s)[len(*s)-1] } +func (s scopeStack) describe() string { + desc := "" + if len(s) > 1 { + desc = "(" + s[1].value.Type().String() + ")" + } + for i, v := range s { + if i < 2 { + // First layer on stack is not real; second is handled specially above. + continue + } + if v.key == "" { + desc += fmt.Sprintf(".%v", v.value.Type()) + } else { + desc += fmt.Sprintf(".%v", v.key) + } + } + return desc +} + +// Formats src & dest as indices for printing. +func (s *scope) setIndices(src, dest int) { + s.srcStack.top().key = fmt.Sprintf("[%v]", src) + s.destStack.top().key = fmt.Sprintf("[%v]", dest) +} + +// Formats src & dest as map keys for printing. +func (s *scope) setKeys(src, dest interface{}) { + s.srcStack.top().key = fmt.Sprintf(`["%v"]`, src) + s.destStack.top().key = fmt.Sprintf(`["%v"]`, dest) +} + // Convert continues a conversion. func (s *scope) Convert(src, dest interface{}, flags FieldMatchingFlags) error { return s.converter.Convert(src, dest, flags, s.meta) @@ -155,6 +187,19 @@ func (s *scope) Meta() *Meta { return s.meta } +// describe prints the path to get to the current (source, dest) values. +func (s *scope) describe() (src, dest string) { + return s.srcStack.describe(), s.destStack.describe() +} + +// error makes an error that includes information about where we were in the objects +// we were asked to convert. +func (s *scope) error(message string, args ...interface{}) error { + srcPath, destPath := s.describe() + where := fmt.Sprintf("converting %v to %v: ", srcPath, destPath) + return fmt.Errorf(where+message, args...) +} + // Register registers a conversion func with the Converter. conversionFunc must take // three parameters: a pointer to the input type, a pointer to the output type, and // a conversion.Scope (which should be used if recursive conversion calls are desired). @@ -292,7 +337,7 @@ func (c *Converter) convert(sv, dv reflect.Value, scope *scope) error { } if !scope.flags.IsSet(AllowDifferentFieldTypeNames) && c.NameFunc(dt) != c.NameFunc(st) { - return fmt.Errorf("can't convert %v to %v because type names don't match (%v, %v).", st, dt, c.NameFunc(st), c.NameFunc(dt)) + return scope.error("type names don't match (%v, %v)", c.NameFunc(st), c.NameFunc(dt)) } // This should handle all simple types. @@ -325,6 +370,7 @@ func (c *Converter) convert(sv, dv reflect.Value, scope *scope) error { } dv.Set(reflect.MakeSlice(dt, sv.Len(), sv.Cap())) for i := 0; i < sv.Len(); i++ { + scope.setIndices(i, i) if err := c.convert(sv.Index(i), dv.Index(i), scope); err != nil { return err } @@ -350,13 +396,14 @@ func (c *Converter) convert(sv, dv reflect.Value, scope *scope) error { return err } dkv := reflect.New(dt.Elem()).Elem() + scope.setKeys(sk.Interface(), dk.Interface()) if err := c.convert(sv.MapIndex(sk), dkv, scope); err != nil { return err } dv.SetMapIndex(dk, dkv) } default: - return fmt.Errorf("couldn't copy '%v' into '%v'", st, dt) + return scope.error("couldn't copy '%v' into '%v'; didn't understand types", st, dt) } return nil } @@ -393,12 +440,14 @@ func (c *Converter) convertStruct(sv, dv reflect.Value, scope *scope) error { case scope.flags.IsSet(IgnoreMissingFields): // No error. case scope.flags.IsSet(SourceToDest): - return fmt.Errorf("%v not present in dest (%v to %v)", f.Name, st, dt) + return scope.error("%v not present in dest (%v to %v)", f.Name, st, dt) default: - return fmt.Errorf("%v not present in src (%v to %v)", f.Name, st, dt) + return scope.error("%v not present in src (%v to %v)", f.Name, st, dt) } continue } + scope.srcStack.top().key = f.Name + scope.srcStack.top().key = f.Name if err := c.convert(sf, df, scope); err != nil { return err } @@ -425,6 +474,8 @@ func (c *Converter) checkStructField(fieldName string, sv, dv reflect.Value, sco } if sf.Type() == potentialSourceKey.fieldType { // Both the source's name and type matched, so copy. + scope.srcStack.top().key = potentialSourceKey.fieldName + scope.destStack.top().key = fieldName if err := c.convert(sf, df, scope); err != nil { return true, err } @@ -448,6 +499,8 @@ func (c *Converter) checkStructField(fieldName string, sv, dv reflect.Value, sco } if df.Type() == potentialDestKey.fieldType { // Both the dest's name and type matched, so copy. + scope.srcStack.top().key = fieldName + scope.destStack.top().key = potentialDestKey.fieldName if err := c.convert(sf, df, scope); err != nil { return true, err } From 3c1d51b19dca04adc231f10ee528dd73748ff787 Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Wed, 26 Nov 2014 11:13:37 -0800 Subject: [PATCH 3/3] refactor to hide structs behind an interface --- pkg/conversion/converter.go | 119 ++++++++++++++++++++++++++---------- 1 file changed, 87 insertions(+), 32 deletions(-) diff --git a/pkg/conversion/converter.go b/pkg/conversion/converter.go index 9a5ea849ed9..519ab179eda 100644 --- a/pkg/conversion/converter.go +++ b/pkg/conversion/converter.go @@ -361,7 +361,7 @@ func (c *Converter) convert(sv, dv reflect.Value, scope *scope) error { switch dv.Kind() { case reflect.Struct: - return c.convertStruct(sv, dv, scope) + return c.convertKV(toKVValue(sv), toKVValue(dv), scope) case reflect.Slice: if sv.IsNil() { // Don't make a zero-length slice. @@ -408,46 +408,99 @@ func (c *Converter) convert(sv, dv reflect.Value, scope *scope) error { return nil } -func (c *Converter) convertStruct(sv, dv reflect.Value, scope *scope) error { - dt, st := dv.Type(), sv.Type() - - listType := dt - if scope.flags.IsSet(SourceToDest) { - listType = st +func toKVValue(v reflect.Value) kvValue { + switch v.Kind() { + case reflect.Struct: + return structAdaptor(v) } - for i := 0; i < listType.NumField(); i++ { - f := listType.Field(i) - if found, err := c.checkStructField(f.Name, sv, dv, scope); found { + return nil +} + +// kvValue lets us write the same conversion logic to work with both maps +// and structs. Only maps with string keys make sense for this. +type kvValue interface { + // returns all keys, as a []string. + keys() []string + // Will just return "" for maps. + tagOf(key string) reflect.StructTag + // Will return the zero Value if the key doesn't exist. + value(key string) reflect.Value + // Maps require explict setting-- will do nothing for structs. + // Returns false on failure. + confirmSet(key string, v reflect.Value) bool +} + +type structAdaptor reflect.Value + +func (sa structAdaptor) len() int { + v := reflect.Value(sa) + return v.Type().NumField() +} + +func (sa structAdaptor) keys() []string { + v := reflect.Value(sa) + t := v.Type() + keys := make([]string, t.NumField()) + for i := range keys { + keys[i] = t.Field(i).Name + } + return keys +} + +func (sa structAdaptor) tagOf(key string) reflect.StructTag { + v := reflect.Value(sa) + field, ok := v.Type().FieldByName(key) + if ok { + return field.Tag + } + return "" +} + +func (sa structAdaptor) value(key string) reflect.Value { + v := reflect.Value(sa) + return v.FieldByName(key) +} + +func (sa structAdaptor) confirmSet(key string, v reflect.Value) bool { + return true +} + +// convertKV can convert things that consist of key/value pairs, like structs +// and some maps. +func (c *Converter) convertKV(skv, dkv kvValue, scope *scope) error { + if skv == nil || dkv == nil { + // TODO: add keys to stack to support really understandable error messages. + return fmt.Errorf("Unable to convert %#v to %#v", skv, dkv) + } + + lister := dkv + if scope.flags.IsSet(SourceToDest) { + lister = skv + } + for _, key := range lister.keys() { + if found, err := c.checkField(key, skv, dkv, scope); found { if err != nil { return err } continue } - df := dv.FieldByName(f.Name) - sf := sv.FieldByName(f.Name) - if sf.IsValid() { - // No need to check error, since we know it's valid. - field, _ := st.FieldByName(f.Name) - scope.srcStack.top().tag = field.Tag - } - if df.IsValid() { - field, _ := dt.FieldByName(f.Name) - scope.destStack.top().tag = field.Tag - } - // TODO: set top level of scope.src/destTagStack with these field tags here. + df := dkv.value(key) + sf := skv.value(key) if !df.IsValid() || !sf.IsValid() { switch { case scope.flags.IsSet(IgnoreMissingFields): // No error. case scope.flags.IsSet(SourceToDest): - return scope.error("%v not present in dest (%v to %v)", f.Name, st, dt) + return scope.error("%v not present in dest", key) default: - return scope.error("%v not present in src (%v to %v)", f.Name, st, dt) + return scope.error("%v not present in src", key) } continue } - scope.srcStack.top().key = f.Name - scope.srcStack.top().key = f.Name + scope.srcStack.top().key = key + scope.srcStack.top().tag = skv.tagOf(key) + scope.destStack.top().key = key + scope.destStack.top().tag = dkv.tagOf(key) if err := c.convert(sf, df, scope); err != nil { return err } @@ -455,12 +508,12 @@ func (c *Converter) convertStruct(sv, dv reflect.Value, scope *scope) error { return nil } -// checkStructField returns true if the field name matches any of the struct +// checkField returns true if the field name matches any of the struct // field copying rules. The error should be ignored if it returns false. -func (c *Converter) checkStructField(fieldName string, sv, dv reflect.Value, scope *scope) (bool, error) { +func (c *Converter) checkField(fieldName string, skv, dkv kvValue, scope *scope) (bool, error) { replacementMade := false if scope.flags.IsSet(DestFromSource) { - df := dv.FieldByName(fieldName) + df := dkv.value(fieldName) if !df.IsValid() { return false, nil } @@ -468,7 +521,7 @@ func (c *Converter) checkStructField(fieldName string, sv, dv reflect.Value, sco // Check each of the potential source (type, name) pairs to see if they're // present in sv. for _, potentialSourceKey := range c.structFieldSources[destKey] { - sf := sv.FieldByName(potentialSourceKey.fieldName) + sf := skv.value(potentialSourceKey.fieldName) if !sf.IsValid() { continue } @@ -479,13 +532,14 @@ func (c *Converter) checkStructField(fieldName string, sv, dv reflect.Value, sco if err := c.convert(sf, df, scope); err != nil { return true, err } + dkv.confirmSet(fieldName, df) replacementMade = true } } return replacementMade, nil } - sf := sv.FieldByName(fieldName) + sf := skv.value(fieldName) if !sf.IsValid() { return false, nil } @@ -493,7 +547,7 @@ func (c *Converter) checkStructField(fieldName string, sv, dv reflect.Value, sco // Check each of the potential dest (type, name) pairs to see if they're // present in dv. for _, potentialDestKey := range c.structFieldDests[srcKey] { - df := dv.FieldByName(potentialDestKey.fieldName) + df := dkv.value(potentialDestKey.fieldName) if !df.IsValid() { continue } @@ -504,6 +558,7 @@ func (c *Converter) checkStructField(fieldName string, sv, dv reflect.Value, sco if err := c.convert(sf, df, scope); err != nil { return true, err } + dkv.confirmSet(potentialDestKey.fieldName, df) replacementMade = true } }