-
Notifications
You must be signed in to change notification settings - Fork 7
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
Redefine Model.this with enable_default_scope
#9
base: master
Are you sure you want to change the base?
Conversation
Since `enable_default_scope` changes the dataset for a model, every instance of that model is affected in its `this` method (which is used for e.g. updating). This means that once (soft-)deleted, an instance can no longer be updated, because its `this` method returns something like: SELECT * FROM model WHERE ((`id` = 1) AND (`deleted_at` IS NULL)); This returns 0 rows instead of 1, and any changes therefore fail. This includes the `recover` method.
efccb47
to
9a0183d
Compare
I'd say soft deleted objects shouldn't be updated unless recovered first. |
The
I'm guessing it will result in a race condition if you do that. |
IMHO you should not try to touch a deleted entry as it is deleted :D If you want to do that anyways, you should first of all disable the default scope by calling |
I agree with the principle, but you have to touch it in order to Here is the execution chain:
Unfortunately, we cannot just use |
Yeah sure. I totally knew that :D that's what I meant with
|
I don't think it works like that, since the model is instantiated separately from the dataset: [3] pry(main)> User.unfiltered
=> #<Sequel::Mysql2::Dataset: "SELECT * FROM `users` ORDER BY `id`">
[4] pry(main)> User.unfiltered[1].this
=> #<Sequel::Mysql2::Dataset: "SELECT * FROM `users` WHERE ((`users`.`deleted_at` IS NULL) AND (`id` = 1)) ORDER BY `id` LIMIT 1"> |
So what are we going to do with this PR ? Do you think the change in this PR is sane and production ready? |
@sdepold I will take another look in the next few days to see if there is some other way around this issue |
I found this SO answer from Jeremy which says not to use a default scope: http://stackoverflow.com/a/11677788
I checked the other uses of |
By returning `super` in the modified `this` method call, we can avoid using this code except in cases when it is needed for updating a deleted entity. This commit also adds a test of the recover method with the `enable_default_scope` option.
@sdepold i've just updated this PR after a long hiatus. after a lot of a review, I think the way I did it back in March was the best option. the change I made today was to only call this code when the instance is deleted, which should help reduce the scope of its changes to Sequel. |
tbh, I don't have an opinion about this PR. Maybe we can invite some others to comment on it. @reidmix ? |
@drosile do you want to take care of this project and take ownership over it? |
Since
enable_default_scope
changes the dataset for a model, everyinstance of that model is affected in its
this
method (which is usedfor e.g. updating). This means that once (soft-)deleted, an instance can
no longer be updated, because its
this
method returns something like:SELECT * FROM model WHERE ((
id
= 1) AND (deleted_at
IS NULL));This returns 0 rows instead of 1, and any changes therefore fail. This
includes the
recover
method.@lipanski @sdepold I couldn't figure out a way to implement this without
essentially rewriting Jeremy's method: https://github.com/jeremyevans/sequel/blob/master/lib/sequel/model/base.rb#L1669-1684
Would love your feedback if there is a better way to solve this issue.