Enhancement: Allow removing rows based on primary key

[schveiguy]

If I have a table that has a single-column primary key, simply implementing TableMapping and RowConvertible allows me to fetch items according to primary key. Nice.

What about removing items? At the moment, I am finding myself adding code many places that implements removal with an SQL statement, but GRDB should know all the info it needs to remove using the primary key. I don't want to have to implement Persistable for this, in fact, I don't want to have to load an object at all. I should be able to do:

struct SomeRecordType : RowConvertible, TableMapping
{ ... }

SomeRecordType.removeOne(db, key: id)

Can this be made to work? I should only actually need TableMapping, not RowConvertible (but of course, no sense in doing one without the other).

[groue]

Hello Steven,

Generation of DELETE queries is supported through the Persistable protocol which grants persistence methods, including delete:

struct MyRecord : Persistable { ... }
let record = MyRecord(...)
try record.delete(db)

The minimal required code is (assuming an "id" primary key in the table "myTable"):

struct MyRecord : Persistable {
    let id: Int64
    static func databaseTableName() -> String {
        return "myTable"
    }
    var persistentDictionary: [String: DatabaseValueConvertible?] {
        return ["id": id]
    }
}

let record = MyRecord(id: 1)
dbQueue.inDatabase {
    // DELETE FROM myTable WHERE id = 1
    try record.delete(db)
}

[groue]

Still, I advise you to perform the simple:

try db.execute("DELETE FROM myTable WHERE id = ?", arguments: [1])

And if you prefer a helper, a simple extension will do the trick:

extension MyRecord {
    func removeOne(db: Database, id: Int64) throws {
        try db.execute("DELETE FROM myTable WHERE id = ?", arguments: [id])
    }
}

That's my best proposal for now.

I should only actually need TableMapping.

You're right, TableMapping is enough. You could even write the removeOne function yourself today, by querying the database schema for the primary key, generate and run the DELETE query. I just don't feel comfortable adding this ad-hoc method right now. Consistency would require MyRecord.removeAll(db), MyRecord.filter(name = "foo").removeAll(db), etc.

So thanks for the idea, but it has to mature a little bit.

[schveiguy]

OK, thanks for looking at it. I'm a big fan of introspection, so as much as I can get for as little as possible is what I like :) I love that I just add RowConvertible method for instance, and I get tons of API to fetch data.

Consistency would require ...

Yes, those would all be good too :)

[groue]

Another consistency issue:

We already have fetchOne(key: 1) and fetchOne(key: ["col1": val1, "col2": val2]). So if GRDB had deleteOne(key: 1), it'd also need deleteOne(key: ["col1": val1, "col2": val2]).

The difficulty is that deleteOne(key: ["col1": val1, "col2": val2]) MUST make sure that it will delete a single row. This requires checking that there is a unique index on (col1, col2).

That's a lot of work :-)

[schveiguy]

It is OK, I can hack a deleteOne extension that just adds the SQL call, and doesn't care about whether it's correct or not. I know in my case, it is correct, but not good for a general library.

[groue]

I reopen this issue. The use case is valid, no doubt about it. It is also implemented by enough ORMs out there: I now realize it can be expected by a non-negligible fractions of users.

If removing a row based on its primary key is useful, it is a non-trivial thing to implement. Ideally, we'd support any primary key (including composite ones), while making sure that the generated SQL statements can not delete more than one row, for any user input.

The new delete APIs should be modelled after the existing fetching ones:

// Existing APIs:
Person.fetchOne(db, key: 1)
Country.fetchOne(db, key: "FR")
Citizenship.fetchOne(db, key: ["personId": 1, "countryIsoCode": "FR"])
// Regular and supported (meaningful as long as email column has a unique index):
Person.fetchOne(db, key: ["email": "[email protected]"])

The ideal deletion API is thus:

try Person.deleteOne(db, key: 1)     // MUST succeed
try Country.deleteOne(db, key: "FR") // MUST succeed
try Citizenship.deleteOne(db, key: ["personId": 1, "countryIsoCode": "FR"]) // MUST succeed
try Person.deleteOne(db, key: ["email": "[email protected]"]) // SHOULD succeed
try Person.deleteOne(db, key: ["name": "John"])              // MUST fail

Since it is difficult to guarantee the deletion of a single line in the two last examples, an alternative would be to restrict deleteOne to single-column primary keys, and introduce deleteAll for composite primary keys. This new method would not have to make any promise about the number of deleted rows:

try Person.deleteOne(db, key: 1)
try Country.deleteOne(db, key: "FR")
try Citizenship.deleteAll(db, key: ["personId": 1, "countryIsoCode": "FR"])
try Person.deleteAll(db, key: ["email": "[email protected]"])
try Person.deleteAll(db, key: ["name": "John"])

The problem with the alternative is that we don't have any Person.fetchAll(db, key: ["name": "John"]), and symmetry is broken. It is better if we can avoid that.

[groue]

Note: the alternative above misuses the argument name key in the deleteAll method.

Another alternative would be a new method deleteAll added to the query interface:

try Citizenship.filter(SQLColumn("personId") == 1 && SQLColumn("countryIsoCode") == "FR").deleteAll(db)
try Person.filter(SQLColumn("email") == "[email protected]").deleteAll(db)
try Person.filter(SQLColumn("name") == "John").deleteAll(db)

That would open a big door, since the query interface is only used for fetching today.

I really wish we could just add deleteOne, and make it work for all primary keys, while providing safeguards.

[schveiguy]

The asymmetry comes from having the name "deleteOne" imply that only one record will be deleted. With fetchOne, the query can still select multiple rows, but you just return only one of them -- no harm done. But as you correctly point out, deleteOne cannot be so cavalier about this. It has to ensure that only one row is deleted.

Another alternative is to avoid specifying how many rows will be deleted in the name. Just add deleteAll and forget about deleteOne. The number of rows deleted will be dependent on how the keys are set up. The primary key information is only used when the version without the column name is used.

You could also call it deleteByKey. I think due to the asymmetry of the operations (fetching and deleting are immutable and mutable operations, deletion is more related to insertion than fetching), it's probably OK to have the operations support different API.

[groue]

Fortunately, SQLite is a good guy: it tells about the indexes defined on a table, on which columns, unique or not. We can thus prevent Person.deleteOne(db, key: ["name": "John"]) when there is no unique index of name, and allow Person.deleteOne(db, key: ["email": "[email protected]"]) when there is a unique index on email.

Now I have to cache those indexes, and invalidate the cache when needed ;-)

[cfilipov]

If you enable SQLITE_ENABLE_UPDATE_DELETE_LIMIT you could make use of LIMIT in the DELETE statement.

Another option would be to implicitly use a transaction in deleteOne, if the number of rows returned is > 1 then rollback and assert/throw.

[groue]

Thanks for the hints, @cfilipov

Reading your comment made me raise a fatal error instead of throwing an error whenever a unique index is missing. It is indeed a programmer error that requires a change in the source code.

For consistency, fetchOne now also raises a fatal error when there is no unique index.

If you enable SQLITE_ENABLE_UPDATE_DELETE_LIMIT you could make use of LIMIT in the DELETE statement.

This option is interesting. Unless I'm mistaken, it would require a custom build of SQLite, though. If useful, and possible, it doesn't prevent us from providing guarantees with the regular stock sqlite that ships with our favorite OSes.

Another option would be to implicitly use a transaction in deleteOne, if the number of rows returned is > 1 then rollback and assert/throw.

Fortunately, we can introspect indexes, and prevent problems. Still, your idea would have been excellent at post-processing them!

[groue]

OK: version 0.74.0 has shipped with the ability to remove rows based on primary key. Thanks for both the suggestion and the support!