Skip to content

fix(spanner): assign value to correct index if columns are out of order in SelectAll #13316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rahul2393 wants to merge 2 commits into from
Closed

fix(spanner): assign value to correct index if columns are out of order in SelectAll #13316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e9a0cf
fix: assign value to correct index if columns are out of order in Sel…
olavloite Nov 5, 2025
d95dea5
Merge branch 'main' into alt-fix-issue-13299
olavloite Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions spanner/row.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,7 @@ func SelectAll(rows rowIterator, destination interface{}, options ...DecodeOptio

isPrimitive := itemType.Kind() != reflect.Struct
var pointers []interface{}
var fieldIndexes []int
isFirstRow := true
var err error
return rows.Do(func(row *Row) error {
Expand All @@ -469,7 +470,7 @@ func SelectAll(rows rowIterator, destination interface{}, options ...DecodeOptio
defer func() {
isFirstRow = false
}()
if pointers, err = structPointers(sliceItem.Elem(), row.fields, s.Lenient); err != nil {
if pointers, fieldIndexes, err = structPointers(sliceItem.Elem(), row.fields, s.Lenient); err != nil {
return err
}
}
Expand Down Expand Up @@ -502,7 +503,7 @@ func SelectAll(rows rowIterator, destination interface{}, options ...DecodeOptio
if p == nil {
continue
}
e.Field(idx).Set(reflect.ValueOf(p).Elem())
e.Field(fieldIndexes[idx]).Set(reflect.ValueOf(p).Elem())
idx++
}
}
Expand All @@ -524,40 +525,45 @@ func SelectAll(rows rowIterator, destination interface{}, options ...DecodeOptio
})
}

func structPointers(sliceItem reflect.Value, cols []*sppb.StructType_Field, lenient bool) ([]interface{}, error) {
pointers := make([]interface{}, 0, len(cols))
func structPointers(sliceItem reflect.Value, cols []*sppb.StructType_Field, lenient bool) ([]interface{}, []int, error) {
pointers := make([]interface{}, len(cols))
fieldIndexes := make([]int, len(cols))
fieldTag := make(map[string]reflect.Value, len(cols))
initFieldTag(sliceItem, &fieldTag)
fieldIndexMap := make(map[string]int, len(cols))
initFieldTag(sliceItem, &fieldTag, &fieldIndexMap)

for i, colName := range cols {
if colName.Name == "" {
return nil, errColNotFound(fmt.Sprintf("column %d", i))
return nil, nil, errColNotFound(fmt.Sprintf("column %d", i))
}

var fieldVal reflect.Value
fieldIdx := i
if v, ok := fieldTag[strings.ToLower(colName.GetName())]; ok {
fieldIdx = fieldIndexMap[strings.ToLower(colName.GetName())]
fieldVal = v
} else {
if !lenient {
return nil, errNoOrDupGoField(sliceItem, colName.GetName())
return nil, nil, errNoOrDupGoField(sliceItem, colName.GetName())
}
fieldVal = sliceItem.FieldByName(colName.GetName())
}
Comment on lines 545 to 550
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This fallback logic for lenient mode has a flaw. If sliceItem.FieldByName successfully finds a field (which could happen for fields in embedded structs that initFieldTag might not handle correctly), the fieldIdx is not updated. It retains its initial value of i (the column index), which leads to fieldIndexes[i] being set to i. This bypasses the column-to-field-index mapping for this field, leading to incorrect data assignment if columns are out of order.

Given that initFieldTag is intended to populate the maps for all relevant fields, this else block should ideally handle columns that are not present in the destination struct. In lenient mode, these should be ignored. The current code does lead to them being ignored because FieldByName would likely fail and result in an invalid fieldVal. However, if FieldByName succeeds, it introduces a bug.

To make this safer, you should either ensure this path also correctly resolves the field index, or rely solely on the pre-computed maps from initFieldTag and treat any column not in the maps as a field to be ignored in lenient mode.

Copy link
Copy Markdown
Contributor

@olavloite olavloite Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Embedded structs are not supported (see the recently added test cases). So this flaw can be ignored, as it is something that was not supported in the first place.

if !fieldVal.IsValid() || !fieldVal.CanSet() {
// have to add if we found a column because Columns() requires
// len(cols) arguments or it will error. This way we can scan to
// a useless pointer
pointers = append(pointers, nil)
pointers[i] = nil
continue
}

pointers = append(pointers, fieldVal.Addr().Interface())
fieldIndexes[i] = fieldIdx
pointers[i] = fieldVal.Addr().Interface()
}
return pointers, nil
return pointers, fieldIndexes, nil
}

// Initialization the tags from struct.
func initFieldTag(sliceItem reflect.Value, fieldTagMap *map[string]reflect.Value) {
func initFieldTag(sliceItem reflect.Value, fieldTagMap *map[string]reflect.Value, fieldIndexes *map[string]int) {
typ := sliceItem.Type()

for i := 0; i < sliceItem.NumField(); i++ {
Expand All @@ -573,7 +579,7 @@ func initFieldTag(sliceItem reflect.Value, fieldTagMap *map[string]reflect.Value
// found an embedded struct
if fieldType.Anonymous {
sliceItemOfAnonymous := sliceItem.Field(i)
initFieldTag(sliceItemOfAnonymous, fieldTagMap)
initFieldTag(sliceItemOfAnonymous, fieldTagMap, fieldIndexes)
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The recursive call to initFieldTag for anonymous embedded structs will cause incorrect field indices to be stored in the fieldIndexes map. Inside the recursive call, the field index i is relative to the embedded struct (sliceItemOfAnonymous), not the top-level struct. However, the decoding in SelectAll uses e.Field(fieldIndexes[idx]), which expects an index on the top-level struct element e.

This will lead to panics or incorrect field assignments when decoding into structs with embedded fields.

A correct implementation would need to build an index path ([]int) for nested fields and use FieldByIndex for value access, which would be a more significant change throughout the decoding logic.

Copy link
Copy Markdown
Contributor

@olavloite olavloite Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Embedded structs are not supported (see the recently added test cases). So this flaw can be ignored, as it is something that was not supported in the first place.

continue
}
}
Expand All @@ -585,5 +591,6 @@ func initFieldTag(sliceItem reflect.Value, fieldTagMap *map[string]reflect.Value
name = fieldType.Name
}
(*fieldTagMap)[strings.ToLower(name)] = sliceItem.Field(i)
(*fieldIndexes)[strings.ToLower(name)] = i
}
}
31 changes: 31 additions & 0 deletions spanner/row_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2313,6 +2313,37 @@ func TestSelectAll(t *testing.T) {
wantPanic bool
want interface{}
}{
{
name: "success: using slice of structs with spanner tag annotations in different order",
args: args{
destination: &[]testStructWithTag{},
mock: newMockIterator(
&Row{
[]*sppb.StructType_Field{
{Name: "Tag2", Type: floatType()},
{Name: "Tag1", Type: intType()},
{Name: "Tag4", Type: timeType()},
{Name: "Tag3", Type: stringType()},
},
[]*proto3.Value{floatProto(1.1), intProto(1), timeProto(tm), stringProto("value")},
},
&Row{
[]*sppb.StructType_Field{
{Name: "Tag2", Type: floatType()},
{Name: "Tag1", Type: intType()},
{Name: "Tag4", Type: timeType()},
{Name: "Tag3", Type: stringType()},
},
[]*proto3.Value{floatProto(2.2), intProto(2), timeProto(tm.Add(24 * time.Hour)), stringProto("value2")},
},
iterator.Done,
),
},
want: &[]testStructWithTag{
{Col1: 1, Col2: 1.1, Col3: "value", Col4: tm},
{Col1: 2, Col2: 2.2, Col3: "value2", Col4: tm.Add(24 * time.Hour)},
},
},
{
name: "success: using slice of primitives",
args: args{
Expand Down
Loading