diff --git a/reflectx/reflect.go b/reflectx/reflect.go index 74f3e6240..0fd939d9e 100644 --- a/reflectx/reflect.go +++ b/reflectx/reflect.go @@ -241,12 +241,6 @@ func methodName() string { return f.Name() } -type typeQueue struct { - t reflect.Type - fi *FieldInfo - pp string // Parent path -} - // A copying append that creates a new slice each time. func apnd(is []int, i int) []int { x := make([]int, len(is)+1) @@ -321,19 +315,48 @@ func parseOptions(tag string) map[string]string { return options } -// getMapping returns a mapping for the t type, using the tagName, mapFunc and -// tagMapFunc to determine the canonical names of fields. +type typeField struct { + t reflect.Type + fi *FieldInfo + pp string +} + +type typeQueue []typeField + +func newTypeQueue(items ...typeField) *typeQueue { + tq := make(typeQueue, 0, len(items)) + for _, item := range items { + tq = append(tq, item) + } + return &tq +} + +func (t typeQueue) len() int { + return len(t) +} + +func (t *typeQueue) pop() typeField { + item := (*t)[0] + *t = (*t)[1:] + return item +} + +func (t *typeQueue) append(tf typeField) { + *t = append(*t, tf) +} + +// getMapping returns a mapping for the t type, which is a struct or a pointer +// to a struct, using the tagName, mapFunc and tagMapFunc to determine the +// canonical names of fields. func getMapping(t reflect.Type, tagName string, mapFunc, tagMapFunc mapf) *StructMap { m := []*FieldInfo{} - root := &FieldInfo{} - queue := []typeQueue{} - queue = append(queue, typeQueue{Deref(t), root, ""}) + queue := newTypeQueue(typeField{Deref(t), root, ""}) - for len(queue) != 0 { + for queue.len() != 0 { // pop the first item off of the queue - tq := queue[0] - queue = queue[1:] + tq := queue.pop() + nChildren := 0 if tq.t.Kind() == reflect.Struct { nChildren = tq.t.NumField() @@ -353,7 +376,7 @@ func getMapping(t reflect.Type, tagName string, mapFunc, tagMapFunc mapf) *Struc continue } - fi := FieldInfo{ + fi := &FieldInfo{ Field: f, Name: name, Zero: reflect.New(f.Type).Elem(), @@ -381,33 +404,43 @@ func getMapping(t reflect.Type, tagName string, mapFunc, tagMapFunc mapf) *Struc fi.Embedded = true fi.Index = apnd(tq.fi.Index, fieldPos) - nChildren := 0 - ft := Deref(f.Type) - if ft.Kind() == reflect.Struct { - nChildren = ft.NumField() + + if ft := Deref(f.Type); ft.Kind() == reflect.Struct { + fi.Children = make([]*FieldInfo, ft.NumField()) } - fi.Children = make([]*FieldInfo, nChildren) - queue = append(queue, typeQueue{Deref(f.Type), &fi, pp}) - } else if fi.Zero.Kind() == reflect.Struct || (fi.Zero.Kind() == reflect.Ptr && fi.Zero.Type().Elem().Kind() == reflect.Struct) { + queue.append(typeField{Deref(f.Type), fi, pp}) + } else if fi.Zero.Kind() == reflect.Struct || Deref(fi.Zero.Type()).Kind() == reflect.Struct { + // this code is responsible for non-embedded structs also being traversed fi.Index = apnd(tq.fi.Index, fieldPos) fi.Children = make([]*FieldInfo, Deref(f.Type).NumField()) - queue = append(queue, typeQueue{Deref(f.Type), &fi, fi.Path}) + queue.append(typeField{Deref(f.Type), fi, fi.Path}) } fi.Index = apnd(tq.fi.Index, fieldPos) fi.Parent = tq.fi - tq.fi.Children[fieldPos] = &fi - m = append(m, &fi) + tq.fi.Children[fieldPos] = fi + m = append(m, fi) } } - flds := &StructMap{Index: m, Tree: root, Paths: map[string]*FieldInfo{}, Names: map[string]*FieldInfo{}} - for _, fi := range flds.Index { - flds.Paths[fi.Path] = fi - if fi.Name != "" && !fi.Embedded { - flds.Names[fi.Path] = fi + sm := &StructMap{ + Index: m, + Tree: root, + Paths: map[string]*FieldInfo{}, + Names: map[string]*FieldInfo{}, + } + + for _, fi := range sm.Index { + // NOTE: very important here, these paths are in traversed order, and we did + // a breadth-first search, so any destination paths that already exist in the + // map are "shallower" and should therefore win. + if _, ok := sm.Paths[fi.Path]; !ok { + sm.Paths[fi.Path] = fi + if fi.Name != "" && !fi.Embedded { + sm.Names[fi.Path] = fi + } } } - return flds + return sm } diff --git a/reflectx/reflect_test.go b/reflectx/reflect_test.go index 799d4e8a8..f427b9903 100644 --- a/reflectx/reflect_test.go +++ b/reflectx/reflect_test.go @@ -136,13 +136,22 @@ func TestBasicEmbeddedWithTags(t *testing.T) { t.Errorf("Expecting 5 fields") } - // for _, fi := range fields.index { - // log.Println(fi) - // } - v := m.FieldByName(zv, "a") - if ival(v) != z.Bar.Foo.A { // the dominant field - t.Errorf("Expecting %d, got %d", z.Bar.Foo.A, ival(v)) + // NOTE: sqlx explains in its documentation that it uses the golang attribute + // access rules for embedded attributes. These are: + // + // For a value x of type T or *T where T is not a pointer or interface type, + // x.f denotes the field or method at the shallowest depth in T where there + // is such an f. If there is not exactly one f with shallowest depth, the + // selector expression is illegal. + // -- https://golang.org/ref/spec#Selectors + // + // As such, z.A should be "dominant" over z.Bar.Foo.A since it is at a shallower + // depth. This test previously confirmed the wrong behaviour and has been + // changed to confirm the right one. More testing of this is done in the + // TestIssue230 test below. + if ival(v) != z.A { + t.Errorf("Expecting %d, got %d", z.A, ival(v)) } v = m.FieldByName(zv, "b") if ival(v) != z.B { @@ -814,7 +823,50 @@ func TestMustBe(t *testing.T) { typ = reflect.TypeOf("string") mustBe(typ, reflect.Struct) - t.Error("got here, didn't expect to") + t.Fatal("got here, didn't expect to") +} + +// tests https://github.com/jmoiron/sqlx/issues/230 +func TestIssue230(t *testing.T) { + + // here we have two types, a "Stack" type that has some + // simple metadata.. + type Stack struct { + ID string `json:"id"` + Name string `json:"name"` + Status int `json:"status"` + CreatedAt int `json:"created_at"` + UpdatedAt int `json:"updated_at"` + } + + // and a "stack" type, which embeds the Stack, but defines its own + // Status field, which _should_ take precedence over the Stack's Status + type stack struct { + Stack + Status string + } + + s := stack{} + m := NewMapperFunc("json", strings.ToLower) + + // the names we will pull out via type map, and the _types_ we expect + // crucially, we expect that "status" will be the string in `stack` + // and *not* the int in `Stack` + names := []string{"id", "name", "status", "created_at"} + typeNames := []string{"string", "string", "string", "int"} + + values := m.FieldsByName(reflect.ValueOf(s), names) + + if len(values) != len(names) { + t.Errorf("expected %d values but got %d", len(names), len(values)) + } + for i, v := range values { + if v.Type().Name() != typeNames[i] { + t.Errorf("expected name %s to have type %s, but has type %s\n", + names[i], typeNames[i], v.Type().Name()) + } + } + } type E1 struct {