apply proper go attribute access rules in reflectx

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
 }

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 {