fix(scan): update Scan function to reset structs to zero values for each scan

[Waldeedle]

• Do only one thing
• Non breaking API changes
• Tested

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:

    var datasetRow T
    for rows.Next() {
        err := db.ScanRows(rows, &datasetRow)

[Waldeedle]

@jinzhu any concerns with this PR?

[Waldeedle]

@a631807682 am I missing something for this to be reviewed?

[jinzhu]

Why use field.DefaultValueInterface? I don't think it is an expected behavior for most users.

[Waldeedle]

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.

[Waldeedle]

@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
}

[Waldeedle]

@a631807682 @jinzhu ? any comments on the above? This is valid usage and should lower the memory allocations needed from my understanding

[a631807682]

I didn’t understand what this PR was going to do from the test cases.

1. 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 is Ptr, 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 {

2. Can we reset all values via reflect.Zero before scanIntoStruct?

[Waldeedle]

@a631807682

1. the PR is because there is a breaking change in v1.24.6, for example if User is scanned 1 time, then the next scan if there is a Ptr field but the value from the db is null, the scan skips the field. Meaning User2 still has User1 data. This is because now for each row scan all values are not being reset.

var user User
for rows.Next() {
  // ScanRows scan a row into user
  db.ScanRows(rows, &user)

  // do something
}

2. I am not sure what you mean?

[Waldeedle]

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 ptr structs

[Waldeedle]

@a631807682 I expanded the test to also have your example, thank you for sharing that!
Here is the output that we expect for the scan when db returns nulls for all the scanned columns:

{
    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>
}

[Waldeedle]

@a631807682 @jinzhu I'll fix the failing test, but does the goal of this PR seem appropriate?

[a631807682]

@a631807682

1. the PR is because there is a breaking change in v1.24.6, for example if User is scanned 1 time, then the next scan if there is a Ptr field but the value from the db is null, the scan skips the field. Meaning User2 still has User1 data. This is because now for each row scan all values are not being reset.

var user User
for rows.Next() {
  // ScanRows scan a row into user
  db.ScanRows(rows, &user)

  // do something
}

2. I am not sure what you mean?

1. My question is why User2 is not empty time, but the default value?
2. Can we reset reflect value like this?

	...
	// reset if type struct ...
	db.Statement.ReflectValue.Set(reflect.Zero(reflectValue.Type()))
    ...
	db.scanIntoStruct(...)

[Waldeedle]

@a631807682

1. the PR is because there is a breaking change in v1.24.6, for example if User is scanned 1 time, then the next scan if there is a Ptr field but the value from the db is null, the scan skips the field. Meaning User2 still has User1 data. This is because now for each row scan all values are not being reset.

var user User
for rows.Next() {
  // ScanRows scan a row into user
  db.ScanRows(rows, &user)

  // do something
}

2. I am not sure what you mean?

1. My question is why User2 is not empty time, but the default value?
2. Can we reset reflect value like this?

	...
	// reset if type struct ...
	db.Statement.ReflectValue.Set(reflect.Zero(reflectValue.Type()))
    ...
	db.scanIntoStruct(...)

1. Maybe it would be clear if I explained my usage. I query a table for all of my user data and then I iterate through to format and save into an excel report. Before v1.24.6, I used the golang code from the gorm docs to allow me to have 1 instance of the var that is just reused instead of allocating memory per row scan. I noticed my tests breaking when updating gorm because of the behavior change. For each scan the struct should start as default and then scan in the value from the db if there is one. The User2 data was not reset for me for the time.Time fields due to the database value being NULL and the User1 having data. Please let me know if that is a bit clearer. Thanks

2. I will try that

[a631807682]

The User2 data was not reset for me for the time.Time fields due to the database value being NULL and the User1 having data

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?

[Waldeedle]

The User2 data was not reset for me for the time.Time fields due to the database value being NULL and the User1 having data

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 will take another look as to what is breaking on monday

[a631807682]

It is necessary to filter Ptr to avoid setting the original type as Interface.
The stmt.ReflectValue of pointer type is usually unwraped, with one exception, that is Interface, which will be converted to a convertible value in Scan. At this time, it cannot be set directly through reflect.Zero.
This part of the logic can be difficult to understand at the moment because it is scattered in different places and lacks comments. I think we need to optimize it, or add necessary comments in another PR.
https://github.com/go-gorm/gorm/blob/master/callbacks.go#L117
https://github.com/go-gorm/gorm/blob/master/scan.go#L188

if reflectValue.Kind() == reflect.Struct {
		db.Statement.ReflectValue.Set(reflect.Zero(reflectValue.Type()))
}

[Waldeedle]

@a631807682 I accidentally closed this PR, but wanted to grab your thoughts about the commit: f0f01b1 (before the last one). Am I correct in using: mode == ScanInitialized to ensure the update and conflict modes are not impacted? This isn't breaking any functionality according to the tests, but I would like to double check since you may be aware of more context that I am not here. The last commit was just for me to see if I broke the tests or they were already broken in the master branch for GORM. Appreciate your time!

[Waldeedle]

@a631807682 just looking for your insight on the commit f0f01b1, is that solution sound? If it is I will reopen an MR, just want to have the full context in this MR. So you can refer to the history easily. Thanks

[a631807682]

@a631807682 just looking for your insight on the commit f0f01b1, is that solution sound? If it is I will reopen an MR, just want to have the full context in this MR. So you can refer to the history easily. Thanks

LGTM