Redefine Model.this with enable_default_scope

[drosile]

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.

@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.

[lipanski]

I'd say soft deleted objects shouldn't be updated unless recovered first.
in what way is the recover method affected by this? I was looking into the reset_instance_dataset method which might be a better option if you can go a bit more into detail regarding this issue.

[drosile]

The recover method calls save, which in turns uses _update_dataset, which calls this. I was also looking at that method, but it seemed conflicting - the documentation says:

Note that you should not use this to change the model's dataset at runtime.

I'm guessing it will result in a race condition if you do that.

[sdepold]

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 unfiltered.

[drosile]

I agree with the principle, but you have to touch it in order to recover it--and that change uses the _update_dataset

Here is the execution chain:

1. recover calls save
2. save calls _save
3. _save calls _update_columns
4. _update_columns calls _update
5. _update calls _update_without_checking
6. _update_without_checking calls _update_dataset
7. _update_dataset uses this

Unfortunately, we cannot just use this.unfiltered as the _update_dataset, because that returns the dataset of ALL of the related model, not just one (no WHERE clause). For example, SELECT * FROM models instead of SELECT * FROM models WHERE deleted_at IS NULL AND id = 123 - the unfiltered removes both the deleted_at qualifier and the primary key.

[sdepold]

Yeah sure. I totally knew that :D that's what I meant with first of all:

Model.unfiltered.your.other.stuff

[drosile]

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">

[sdepold]

So what are we going to do with this PR ? Do you think the change in this PR is sane and production ready?

[drosile]

@sdepold I will take another look in the next few days to see if there is some other way around this issue

[drosile]

I found this SO answer from Jeremy which says not to use a default scope: http://stackoverflow.com/a/11677788

The best workaround is avoiding the problem by not having a default scope. In most cases a default scope is a bad idea. If you want most of your queries to use a scope, then apply the scope manually in those queries, don't use a default scope and try to backout the scope in other queries. A default scope only makes sense if all of your queries will use that scope.

I checked the other uses of this in Sequel::Model and it appears that it would affect updating, deleting, freezing, and the exists? method. But this PR (as it currently is) is funny because we set_dataset to one thing and then change this to another. I think a more ideal solution would be to modify the _update_dataset at runtime. I'll look into doing that.

[drosile]

@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.

[sdepold]

tbh, I don't have an opinion about this PR. Maybe we can invite some others to comment on it. @reidmix ?

[sdepold]

@drosile do you want to take care of this project and take ownership over it?