implementing deleted_at

[dmalan]

Hi Tim, all,

We're aspiring to implement soft deletions, whereby we set a field in our (MySQL) table (whose default value is otherwise NULL) to a DATETIME when it's "deleted." At the moment, we're doing so with

models.Entity
.forge()
.query({where: ['uuid', '=', req.params.uuid]})
.save({deleted_at: new Date()})
.then(function(model) {
    // success
}, function(err) {
    // failure
});

which results in SQL of:

{ command: 3,
  sql: 'update `submissions` set `deleted_at` = \'2013-08-23 01:57:11\', `updated_at` = \'2013-08-23 01:57:11\' where `uuid` = \'110ec58a-a0f2-4ac4-8393-c866d813b8d1\'' }

Is that likely the cleanest approach, though? Or might we be better off overriding destroy for all entities? Separately, it looks like, per Bookshelf.Model's default implementation of timestamp, there's no way to prevent updated_at from being updated in this scenario (which is a minor goal so that the final value of updated_at reflects the last time its actual data was modified, as opposed to its metadata)?

On the flip side, though, it was less clear to us how to go about tweaking fetch so that SELECTs include a WHERE deleted_at IS NULL. For instance, adding

this.on('fetching', function(model, columns, options) {
    model.query({where: ['deleted_at', null]});
});

got us most of the way there but actually yielded a query with WHERE deleted_at = NULL as opposed to WHERE deleted_at IS NULL. Is there perhaps some other way to achieve the latter, perhaps by dropping down into Knex?

Many thanks!

[tgriesser]

Interesting, I had been considering soft deletes, but hadn't quite decided what the cleanest way of implementing them would be yet - maybe this will help.

Personally, I would probably override the sync's del for all entities where you'd want this to be the case, and use the Model's destroy method as you normally would, probably by doing something like this:

var SoftDeleteModel = Bookshelf.Model.extend({
   sync: function(options) {
      var sync = Bookshelf.Model.prototype.sync.apply(this, arguments);
      // This way, you can add a hardDelete flag to the options and it will 
      // actually delete the model.
      if (!options.hardDelete) {
        sync.del = function(options) {
          return sync.update.call(sync, {deleted_at: new Date()});
        };
      }
      return sync;
   }
});

That way, you'll still get the destroying and destroyed events, and the concept of soft-deletion is abstracted away from the application layer (and feels like less of a hack). Also, because you're bypassing save entirely, you shouldn't have the issue of the timestamp for updated_at being modified.

For the second question, you should be able to use:

model.query({whereNull: 'deleted_at'})...;

...which will use the whereNull clause of the query builder, though I should look into automatically using whereNull if the value for the where condition is null.

Again, I'd also probably also do that with a little customization of the sync as described above (this time with select rather than del), so that you don't need to pass the whereNull each time, and you could also come up with a flag in the options that would bypass setting the whereNull query clause if you wanted to select everything including soft-deleted.

Let me know if that sounds like a good enough solution... or if there are any issues as I haven't tried it but I believe it should all work as I described.

Edit: Actually, the event driven fetching handler you mentioned above should also work just fine (don't need to modify sync for that), and you can still use the options there to check for a custom flag if you wanted to be able to bypass the whereNull clause.

[dmalan]

Thanks so much for the thoughts, Tim. Just a few follow-ups, if you don't mind!

Unrelated to soft deletes, we noticed that code like

models.Entity
.forge()
.query(function(qb) {
    qb.where('uuid', '=', '110ec58a-a0f2-4ac4-8393-c866d813b8d1');
})
.then(function() {
    console.log('success');
}, function(err) {
    console.log('failure');
});

outputs failure, which seems to be the result of del at line 998 in bookshelf.js, which doesn't seem to like it if wheres is undefined (as happens when syncing.id is null):

return this.query.where(wheres).del();

If we change line 980 from

var wheres, syncing = this.syncing.resetQuery();

to

var wheres = {}, syncing = this.syncing.resetQuery();

we do get success and

--> ComQueryPacket
{ command: 3,
  sql: 'delete from `dropboxes` where `uuid` = \'110ec58a-a0f2-4ac4-8393-c866d813b8d1\'' }

as expected.

Meanwhile, if we do override sync to implement soft deletes with

sync: function(options) {
    var sync = Bookshelf.Model.prototype.sync.apply(this, arguments);
    if (!options.hardDelete)  {
        sync.del = function(options) {
            return sync.update.call(sync, {deleted_at: new Date()});
        };
    }
    return sync;
},

we actually get a query of

--> ComQueryPacket
{ command: 3,
  sql: 'update `dropboxes` set  where `uuid` = \'110ec58a-a0f2-4ac4-8393-c866d813b8d1\'' }

which isn't syntactically valid (because of the missing key/value pair after set). Should we be passing in deleted_at in some other way perhaps?

Many thanks!

[tgriesser]

Thanks for pointing that out - rather than using the attrs passed into update, I was instead using the attributes directly on the model, which wouldn't be updated when called directly via sync.update, should be fixed now. I also cleaned up the del method to eliminate the wheres object entirely, and instead check the length of the wheres array on the query builder chain, as is done in the update method.

Please let me know if there are any more issues with getting the soft delete to work correctly as it was previously described.

Thanks!

[dmalan]

Thanks, Tim, working nicely now!

For selects, meanwhile, we're currently using

this.on('fetching', function(model, columns, options) {
    model.query({whereNull: 'deleted_at'});
});

which works well. I was curious to ask, though, what you had in mind for soft delete-aware selects using sync? Whereas

sync.del = function(options) {
    return sync.update.call(sync, {deleted_at: new Date()});
};

works well for overriding del, doing the same for select, as with

sync.select = function(options) {
    return sync.select.call(sync, {deleted_at: null});
};

results, we realized, in infinite recursion. Just curious if you had a somewhat different approach in mind for injecting the deleted_at clause into sync.select?

[tgriesser]

Great, and yes, since you're calling the same method you're looking to overwrite, you'd need to grab a local reference to the original select function before it's overwritten, so you'd do something like this:

sync: function(options) {
  var sync = Bookshelf.Model.prototype.sync.apply(this, arguments);
  var originalSelect = sync.select;
  sync.select = function() {
    sync.query.whereNull('deleted_at');
    return originalSelect.call(sync);
  };
  return sync;
}

Then that should give you what you're looking for.