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.
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
fix(scan): update Scan function to reset structs to zero values for each scan #6938
base: master
Are you sure you want to change the base?
Conversation
c1afb54
to
88330cb
Compare
7d156dd
to
d66da4e
Compare
@jinzhu any concerns with this PR? |
@a631807682 am I missing something for this to be reviewed? |
scan.go
Outdated
fieldIsEmbeddedPointerTypeStruct := len(field.BindNames) > 1 && len(field.StructField.Index) > 0 && field.StructField.Index[0] < 0 | ||
fieldValue := reflect.ValueOf(values[idx]).Elem() | ||
if !fieldIsEmbeddedPointerTypeStruct && fieldValue.Kind() == reflect.Ptr && fieldValue.IsNil() { | ||
db.AddError(field.Set(db.Statement.Context, reflectValue, field.DefaultValueInterface)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use field.DefaultValueInterface? I don't think it is an expected behavior for most users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
field.DefaultValueInterface
ensures that I return the struct back with default/zero values for each field. i.e. nil for pointers and 0 for ints. It helps to reset the struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jinzhu
I think it is expected usage based on your docs: https://gorm.io/docs/sql_builder.html#Scan-sql-Rows-into-struct
var user User
for rows.Next() {
// ScanRows scan a row into user
db.ScanRows(rows, &user)
// do something
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@a631807682 @jinzhu ? any comments on the above? This is valid usage and should lower the memory allocations needed from my understanding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn’t understand what this PR was going to do from the test cases.
- Why do we need to reset the value to the default value? Its means there a default value when the type is
Struct
, but not when the type isPtr
, and this is a breaking change.
type NestedEmbeddedStruct struct {
- NestedEmbeddedIntField int
+ NestedEmbeddedIntField int `gorm:"default:2"`
}
+ result = Result{}
for rows.Next() {
if err := DB.ScanRows(rows, &result); err != nil {
- Can we reset all values via
reflect.Zero
beforescanIntoStruct
?
var user User
for rows.Next() {
// ScanRows scan a row into user
db.ScanRows(rows, &user)
// do something
}
|
I cannot reset an embedded ptr struct to nil because THAT would be a breaking change for gorm, we instead want to have only the values reset not the embedded |
@a631807682 I expanded the test to also have your example, thank you for sharing that! {
BoolField:false
IntField:0
Int8Field:0
Int16Field:0
Int32Field:0
Int64Field:0
UIntField:0
UInt8Field:0
UInt16Field:0
UInt32Field:0
UInt64Field:0
Float32Field:0
Float64Field:0
StringField:""
TimeField:0001-01-01 00:00:00 +0000 UTC
TimePtrField:<nil>
EmbeddedStruct:{EmbeddedIntField:0 NestedEmbeddedStruct:{NestedEmbeddedIntField:0 NestedEmbeddedIntFieldWithDefault:2}}
EmbeddedPtrStruct:<nil>
} |
@a631807682 @jinzhu I'll fix the failing test, but does the goal of this PR seem appropriate? |
|
|
I understand that the User2 time field needs to be reset, I am sure this is a bug, my question is why do we need to reset to defalut value instead of zero value? |
@a631807682 Ah just got what you meant now, updated the logic to reset to the zero values. I originally had that but was not aware of the default value annotation. So when I found DefaultValueInterface, I thought it was how gorm was referring to zero values and used it to ensure that any special logic was consistent for my change. I applied your solution and my test passes locally I think the tests are broken in places outside my change? |
What did this pull request do?
Addresses #6819
Adding the else condition to use the base type when data is nil ensures that the struct field is always set by the scanIntoStructMethod
User Case Description
It fixes the example from the docs where you initialize the var once: