groue / GRDB.swift

A toolkit for SQLite databases, with a focus on application development
MIT License
6.84k stars 705 forks source link

Implementing soft delete #572

Closed Querschlag closed 5 years ago

Querschlag commented 5 years ago

My app features a sync mechanism that requires my to keep "deleted" records around and not actually delete them from the database. I therefore added the field deleted to all of my records to track their status. Next I added a few helper functions for querying deleted and non-deleted records. I also overrode the all() method to return just the non-deleted records. Is this safe to do so, or will it cause problems with GRDB internals that might use all()?

struct MyRecord: Codable, MutablePersistableRecord, FetchableRecord {

    var id: Int64?
    var created: Date
    var deleted: Bool = false
    ...
}

extension MyRecord {

    private static func defaultOrder() -> QueryInterfaceRequest<Self> {
        return self.order(Columns.created.desc)
    }

    static func allIncludingDeleted() -> QueryInterfaceRequest<Self> {
        return self.defaultOrder()
    }

    static func all() -> QueryInterfaceRequest<Self> {
        return self.softDeleted(false)
    }

    static func softDeleted(_ deleted: Bool) -> QueryInterfaceRequest<Self> {
        return self.defaultOrder().filter(Columns.deleted == deleted)
    }
}
groue commented 5 years ago

Hello @Querschlag,

Thanks for the interesting question and the detailed context.

Is this safe to do so, or will it cause problems with GRDB internals that might use all()?

To be short, yes this is somewhat dangerous.

  1. Your override of all won't be used in any method that uses all is a generic context, because this method is not a customization point (i.e. a requirement of the TableRecord protocol).

    For example, the shortcut MyRecord.fetchAll(db) won't call your override. MyRecord.order(...) won't call your override. You actually use this property in order to define softDeleted(_:).

  2. GRDB filters are always additive. This means that you can't remove the default filtering from the overridden all: someRequest.includingDeletedBecauseIChangedMyMind() can not be expressed.

  3. Your override of all is not applied to associations (belongsTo, hasMany...) of MyRecord, and this may also reveal a surprising inconsistency.

This does not mean that your code is bad, or will break. As long as your application code performs explicit calls to MyRecord.all(), you are safe. But I'm afraid you or your coworkers will eventually grow doubts and not always be sure whether deleted records are included or not, as more requests that involve MyRecord are used by your Application.


Since the beginning, GRDB has always avoided any API that allows to define any default ordering or filtering on record types.

I've been using a lot an ORM that provides this feature, Ruby's ActiveRecord, and I have grown a bad opinion of it over the years. The problem with default filtering/ordering is that applications evolve and change, and that what appears to be an obvious default at first often becomes a hindrance whenever the app discovers new database needs. Eventually we end up with a weird situation where some parts of the app are privileged over other parts (bad), for no good reason but support for legacy (bad as well). I wanted GRDB to prevent this situation from happening.

In this context, your allIncludingDeleted method is an example of currently recommended practice. And symmetry begs for an allButDeleted method, not an override to all.


I have to admit that soft delete is a strong use case for default filtering.

Last time I had to implement soft delete, also in a context of synchronization, I used a distinct table for soft-deleted records. I remember that one reason for this technical choice is that I did not want to fight this very default filtering battle we are discussing. So in the end, it is GRDB that had me pick the distinct table solution, and has you fight with all. This means that the library gets in the way, when it should not.

So there is room for improvements here. Maybe we need default filtering after all. Thanks for opening the discussion!

Querschlag commented 5 years ago

You made some really good points there. I also come from a background where this technique is common. Even though I haven't had any bad experience I can imagine the problems that can arise with it. Looking at what GRDB currently is, a comprehensive SQLite wrapper and not and ORM, i guess the answer would be to build the soft delete on top of the library with a custom Database Manager class. Therefore the original methods like all() and delete(_:) would still be predictable for every record type and the project would stay flexible for future requirement changes. The only downside being that it requires a custom implementation and a bit more setup code to get there.

I must admit that i was a bit deterred at first when I learned that GRDB lacks a lot of features I took for granted in libraries like Realm. After a good period of using it I must say that I really appreciate the predictability from working as closely to SQLite as possible. For me the key is in the documentation, which is already really good👍. Having a clear guide for this use case will retain simplicity and predictability of the library while also providing a safe and supported way of dealing with this. I suggest to add it as an example in the Database Manager section.

groue commented 5 years ago

You made some really good points there. I also come from a background where this technique is common. Even though I haven't had any bad experience I can imagine the problems that can arise with it.

Sometimes, a good and convincing use case is the opportunity to question previous design decisions. Preventing misuses is important, but letting seasoned developers do their job with as little friction as possible is sometimes more important - especially when the misuse is somewhat subjective, as in our case.

Looking at what GRDB currently is, a comprehensive SQLite wrapper and not and ORM, i guess the answer would be to build the soft delete on top of the library with a custom Database Manager class.

You are right, Database Managers are the preferred GRDB technique for dealing with soft delete at the moment. Managers are able to hide the DatabaseQueue/Pool, and can thus prevent the rest of the application from messing with records directly. However, this is more heavy-weight. Some apps are really simple, and don't need all the layers of the latest trendy application architecture. And database managers are not always easy to design "properly".

Besides, the meaning of "ORM" is slowly shifting, with the advent of Swift and Rust (see Diesel). It is possible to build a pretty workable layer on top of a database, without auto-updating, lazy loading, and record uniquing, but with proper record types, sql builder, composable requests, and associations, all features that hang on the tool belt of traditional ORMs. I no longer describe the maturing GRDB as "not an ORM". And as a heavy user of the lib, I'm very dedicated to its description: "a toolkit for SQLite databases, with a focus on application development". I want to be able to write my apps with it :-)

So dealing with "classic" use cases with little fuss is important.

I must admit that i was a bit deterred at first when I learned that GRDB lacks a lot of features I took for granted in libraries like Realm.

Thanks for your patience, then :-) I would be happy if you would give a glimpse on the list of those features, you know :-) Maybe some of them could find a home here.

For example, one user recently requested support for automatic updates of "creationDate" and "updateDate" columns. It is a totally valid request, and it can not quite easily be done with GRDB today.

After a good period of using it I must say that I really appreciate the predictability from working as closely to SQLite as possible. For me the key is in the documentation, which is already really good👍. Having a clear guide for this use case will retain simplicity and predictability of the library while also providing a safe and supported way of dealing with this. I suggest to add it as an example in the Database Manager section.

Thanks again!

This is a good idea. More "Good Practices" guides always help. And they are often updated after a good issue like this one :-)

But I don't know how to write it yet 😅. Meanwhile, your initial feature request for default filtering/ordering is now slowly crawling its way. It should not be too difficult to lift the three issues listed in the initial answer, in a reliable and consistent way. And it is not a bad answer to the soft delete use case!

groue commented 5 years ago

For example, one user recently requested support for automatic updates of "creationDate" and "updateDate" columns.

Oh my. It was you 😆. It looks like you are good at spotting weaknesses. Please keep on questioning GRDB, please!

groue commented 5 years ago

An interesting perspective on the challenges created by soft delete and default filtering, in the context of ActiveRecord: https://medium.com/galvanize/soft-deletion-is-actually-pretty-hard-cb434e24825c

groue commented 5 years ago

@Querschlag, I'm not sure what is the purpose of keeping this issue open. If I were to sum it up:

Meanwhile, our discussion remains interesting and useful for other GRDB users who will stumble upon it. Closing this issue won't prevent Github search and Google from finding it.