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: remove callback from callbacks if Remove() called #6916

Merged
merged 3 commits into from Mar 26, 2024

Conversation

snackmgmg
Copy link
Contributor

@snackmgmg snackmgmg commented Mar 19, 2024

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

What did this pull request do?

ref: #6910 (review)

Modified the Remove() method so that it not only sets the removed flag but also removes the callback from the callbacks within compile().
This resolves the following issues:

  • The problem where the Get() method could retrieve callbacks that have already been removed.
  • The problem where removed callbacks could still operate due to the Remove() method.

In addition, I have made some modifications to the existing tests.
Previously, the specified before and after for a callback continued to affect the overall order even after being Remove()d. I believe that continuing to have an effect after being removed is an unintended behavior.
What do you think? I would appreciate any advice.

User Case Description

The problem where the Get() method

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(nil, nil)
createCallback := db.Callback().Create()

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

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

The problem where the Remove() method

The expectation is that callbacks marked as removed via Remove() should not be executed. However, we encountered an issue where callbacks registered through methods like Before("*").Register() continue to operate even after being Remove()d.
This typically occurs in scenarios such as the following:

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

workaround

For your reference, using Before("*").Remove() can be mentioned as a workaround for the above issue.

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.

Maybe we can reduce the number of loops through map ?

@snackmgmg
Copy link
Contributor Author

Thanks! I have improved the part you mentioned in your comment.
f255f36

@jinzhu jinzhu merged commit 26195e6 into go-gorm:master Mar 26, 2024
34 checks passed
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