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: behavior when getting removed callback #6910

Closed
wants to merge 2 commits into from

Conversation

snackmgmg
Copy link
Contributor

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

What did this pull request do?

Resolved the issue where Get() retrieves a callback that has been removed by Remove().
This was due to the search continuing even when a callback with v.removed: true was found, eventually reaching another callback of the same name with v.removed: false.

User Case Description

If a callback named cb1 is registered, it is expected that calling Get("cb1") after Remove("cb1") should return nil. However, a callback function registered as cb1 is being returned instead.

db, _ := gorm.Open(tests.DummyDialector{}, nil)
createCallback := db.Callback().Create()

_ = createCallback.Register("cb1", func(*gorm.DB){})
_ = createCallback.Remove("cb1")

cb := createCallback.Get("cb1") // not nil

tests/callbacks_test.go Outdated Show resolved Hide resolved
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.

Obtaining key values based on order does not solve this problem, because the deleted element may be at the front or at the back.

Failed case

- err := createCallback.Register("plugin_1_fn1", c1)
+ err := createCallback.Before("*").Register("plugin_1_fn1", c1)

I don't know the history of this part of the code, and I don't understand why the deleted callback is not removed when compiling.

@jinzhu
Copy link
Member

jinzhu commented Mar 19, 2024

previously, the callbacks system allowed a new callback to overwrite a previously deleted one, which is no longer supported now~

@jinzhu
Copy link
Member

jinzhu commented Mar 19, 2024

maybe we can remove the callback from processor.callbacks when call the Remove method

@snackmgmg
Copy link
Contributor Author

Thank you for your review. You are right, I overlooked that part.
Additionally, after reviewing your comments, I realized that the Remove() is not working properly either.
For instance, it seems that callbacks are not being removed in certain cases.

func TestRemoveNotWorking(t *testing.T) {
	db, _ := gorm.Open(nil, nil)
	rawCallback := db.Callback().Raw()

	rawCallback.Before("*").Register("cb1", func(*gorm.DB) { panic("Remove does not work") })
	rawCallback.Remove("cb1")

	db.Exec("SELECT 1") // panic
}

// In this case, it works correctly.
func TestRemoveWorking(t *testing.T) {
	db, _ := gorm.Open(nil, nil)
	rawCallback := db.Callback().Raw()

	rawCallback.Before("*").Register("cb1", func(*gorm.DB) { panic("Remove does not work") })
	rawCallback.Before("*").Remove("cb1")

	db.Exec("SELECT 1") // not panic
}

I think that adopting the method of removing callbacks that have been marked for removal is not only a solution for the Get() issue but also addresses the problem with Remove().
I will try this approach. Thank you!

@snackmgmg
Copy link
Contributor Author

Sorry, I forgot to close this.

@snackmgmg snackmgmg closed this Mar 21, 2024
@snackmgmg snackmgmg deleted the fix_callbacks_get branch March 21, 2024 10:40
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

4 participants