fix(spanner): assign value to correct index if columns are out of order in SelectAll #13316
Conversation
rahul2393
changed the title
Summary of ChangesHello @rahul2393, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a bug in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where SelectAll would fail to decode data into a struct if the column order in the result set did not match the struct's field order. The change introduces a mapping from column index to struct field index, which resolves the primary problem for flat structs, and a good test case has been added to verify this.
However, I've found a couple of issues with the implementation that could lead to incorrect behavior or panics in more complex scenarios, specifically with structs that have embedded fields, and in lenient mode. My review comments provide more details on these issues.
| } else { | ||
| if !lenient { | ||
| return nil, errNoOrDupGoField(sliceItem, colName.GetName()) | ||
| return nil, nil, errNoOrDupGoField(sliceItem, colName.GetName()) | ||
| } | ||
| fieldVal = sliceItem.FieldByName(colName.GetName()) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 fieldType.Anonymous { | ||
| sliceItemOfAnonymous := sliceItem.Field(i) | ||
| initFieldTag(sliceItemOfAnonymous, fieldTagMap) | ||
| initFieldTag(sliceItemOfAnonymous, fieldTagMap, fieldIndexes) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Add some additional test cases for SelectAll() to verify that the behavior remains the same if #13316 is merged.
Add some additional test cases for SelectAll() to verify that the behavior remains the same if #13316 is merged.
|
@olavloite Should we go ahead with this one #13301, if yes then can you please help approve that |
2 participants
Fix: #13299