Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Waldeedle
Copy link

  • 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 Waldeedle force-pushed the master branch 2 times, most recently from c1afb54 to 88330cb Compare April 3, 2024 20:58
@Waldeedle Waldeedle marked this pull request as draft April 4, 2024 01:52
@Waldeedle Waldeedle marked this pull request as ready for review April 4, 2024 11:28
@Waldeedle Waldeedle marked this pull request as draft April 4, 2024 11:33
@Waldeedle Waldeedle marked this pull request as ready for review April 4, 2024 11:44
@Waldeedle Waldeedle force-pushed the master branch 2 times, most recently from 7d156dd to d66da4e Compare April 4, 2024 18:29
@Waldeedle Waldeedle changed the title fix: update setupValuerAndSetter to use default values when pointer t… fix(scan): update setupValuerAndSetter to use default values when pointer t… Apr 4, 2024
@Waldeedle
Copy link
Author

@jinzhu any concerns with this PR?

@Waldeedle
Copy link
Author

@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))
Copy link
Member

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.

Copy link
Author

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.

Copy link
Author

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
}

Copy link
Author

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

Copy link
Member

@a631807682 a631807682 left a 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.

  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 {
  1. Can we reset all values via reflect.Zero before scanIntoStruct?

@Waldeedle
Copy link
Author

@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
}
  1. I am not sure what you mean?

@Waldeedle
Copy link
Author

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
Copy link
Author

@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
Copy link
Author

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

@a631807682
Copy link
Member

a631807682 commented May 22, 2024

@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
}
  1. 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
Copy link
Author

@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
}
  1. 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
Copy link
Member

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 Waldeedle changed the title fix(scan): update setupValuerAndSetter to use default values when pointer t… fix(scan): update Scan function to reset structs to zero values for each scan May 24, 2024
@Waldeedle
Copy link
Author

Waldeedle commented May 24, 2024

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 think the tests are broken in places outside my change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants