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

Scan after using Scopes function panicks if *gorm.DB object is used for fetching other data. #6136

Closed
peymanmortazavi opened this issue Mar 10, 2023 · 5 comments
Assignees

Comments

@peymanmortazavi
Copy link

GORM Playground Link

go-gorm/playground#577

Description

We currently use the Scopes function, admittedly to my dislike, for some common joins or preparing long raw queries. The way we do it, is we first fetch some account attributes and then use them to return a raw query:

func ScopeFunc(id string) func (*gorm.DB) *gorm.DB {
   return func (d *gorm.DB) *gorm.DB {
        someModel := Model{}
        d.Raw("...").Scan(&someModel).Error
        // check error
        ...
       return d.Raw(preparedQuery)
  }
}

With the older version, this worked but after upgrading to 1.24.5, this panics.

It might be useful to note that I noticed in finished_api.go line 512 inside the func (db *DB) Rows(*sql.Rows, error) we have this block:

	rows, ok := tx.Statement.Dest.(*sql.Rows)   <-- here we get *sql.Row instead of *sql.Rows
	if !ok && tx.DryRun && tx.Error == nil {
		tx.Error = ErrDryRunModeUnsupported
	}

We're not dry run, ok is false so nil result is returned with a nil error cause downstream methods to pass the error check and dereference a pointer that is pointing to nil.

@black-06
Copy link
Contributor

black-06 commented Mar 10, 2023

use Session to copy a db when calling finisher_api in Scopes.
I don't know why, but it works...

err = DB.Scopes(func(d *gorm.DB) *gorm.DB {
	a := Account{}
	if err := d.Session(&gorm.Session{}).Raw("select * from accounts limit 1").Scan(&a).Error; err != nil {
		return d
	}
	return d.Raw("select * from users limit 2")
}).Scan(&users).Error

@peymanmortazavi
Copy link
Author

peymanmortazavi commented Mar 10, 2023

@black-06 Yeah that definitely resolved the issue. My understanding was that the chain API always takes a copy of the db instance. Though probably some pointers are used and those are just references.

My question is, should we always opt to use Session in similar situations or is this a bug that we should resolve in future.

P.S. thanks for a swift response 😃

@a631807682
Copy link
Member

When nested Scan is executed, the rows identifier will be deleted by mistake.
We may need to use an array to store the type of each query in the nested query.

https://github.com/go-gorm/gorm/blob/master/finisher_api.go#L512
https://github.com/go-gorm/gorm/blob/master/callbacks/row.go#L15

black-06 added a commit to black-06/gorm that referenced this issue Mar 13, 2023
@black-06 black-06 mentioned this issue Mar 13, 2023
3 tasks
@black-06
Copy link
Contributor

When nested Scan is executed, the rows identifier will be deleted by mistake. We may need to use an array to store the type of each query in the nested query.

https://github.com/go-gorm/gorm/blob/master/finisher_api.go#L512 https://github.com/go-gorm/gorm/blob/master/callbacks/row.go#L15

Fix it in #6143 , but I found another problem.

OK:

var user User
if err := DB.Scopes(func(db *gorm.DB) *gorm.DB {
	var maxID int64
	if err := db.Raw("select max(id) from users").Scan(&maxID).Error; err != nil {
		return db
	}
	return db.Raw("select * from users where id = ?", maxID)
}).Scan(&user).Error; err != nil {
	t.Errorf("failed to find user, got err: %v", err)
}

Wrong:

if err := DB.Scopes(func(db *gorm.DB) *gorm.DB {
	var maxID int64
	if err := db.Model(&User{}).Select("max(id)").Scan(&maxID).Error; err != nil {
		return db
	}
	return db.Where("id = ?", maxID)
}).Scan(&user).Error; err != nil {
	// failed to find user, got err: sql: Scan error on column index 0, name "max(id)": 
	// unsupported Scan, storing driver.Value type int64 into type *tests.User
	t.Errorf("failed to find user, got err: %v", err)
}

Because Selects are not cleaned up in callbacks Execute.
Is this a bug? Or should we not have a nested Scan?

@a631807682
Copy link
Member

Because Selects are not cleaned up in callbacks Execute.
Is this a bug? Or should we not have a nested Scan?

This is ok for me, because we don't promise to reuse. We just need to ensure that its behavior is consistent inside and outside the Scope.
https://gorm.io/docs/method_chaining.html#content-inner

@jinzhu jinzhu closed this as completed Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants