Grdb.swift: Support for Polymorphic Requests

[thommw]

Hi there,

I have added a feature to GRDB that I needed and thought you might consider adding it to this excellent library.

Cheers,
Thomas Wernitz

_The Problem:_

I have an abstract base class Base and two subclasses Foo and Bar. I want to implement RowConvertible in Base and then do: try Base.fetchOne(db) and receive a Foo or Bar depending on the data in the row.

_The Solution:_

I have extended the RowConvertible protocol and added an extension to provide a default implementation:

public protocol RowConvertible {
    init(row: Row)

    static func getInstance(row: Row) -> Self
}

extension RowConvertible {

    public static func getInstance(row: Row) -> Self {
        return Self.init(row: row)
    }

}

I have also changed the next() method of the RecordCursor<Record: RowConvertible> class to use the new method instead of the initializer:

public func next() throws -> Record? {
    if done { return nil }
    switch sqlite3_step(sqliteStatement) {
        case SQLITE_DONE:
            done = true
            return nil
        case SQLITE_ROW:
            return Record.getInstance(row: row)
        case let code:
            statement.database.selectStatementDidFail(statement)
            throw DatabaseError(resultCode: code, message: statement.database.lastErrorMessage, sql: statement.sql, arguments: statement.arguments)
    }
}

In Base I can now provide this method and return the appropriate subclass:

extension Base {
    public static func getInstance(row: Row) -> Self {
        if row["foo"] == 1 {
          return unsafeDowncast(Foo(row: row), to: self)
        } else {
          return unsafeDowncast(Bar(row: row), to: self)
        }
    }
}

[groue]

Hello @thommw,

This is the first time this question is asked, and this need is expressed.

First of all, your solution looks like it works. And this is very good. You don't need any assistance :-)

RowConvertible uses an initializer, not a static method because of... some reason from its early ages (introduced in v0.35.0). Factory methods didn't play well with all situations that GRDB wants to deal with. I can't remember precisely now. Your unsafeDowncast is a hint that something is... special with factory methods, I hope you'll agree.

Please try to run all GRDB tests with your changes. If you are able to make all of them pass, I may consider replacing the row initializer with a factory method. Hurry, because GRDB 3.0 will ship in a few weeks. When shipped, API will be locked for a while! (No, I'm kidding: it's unlikely that I change this. You really have to show me that factory methods can run all tests, AND that there is not a single unsafe method in sight).

---

Meanwhile, let me describe another solution to your problem, that works with the current state of GRDB.

As you see, RowConvertible requires init(row:). And a Swift initializer can't return one subclass or another. This means that Base must not adopt RowConvertible. Plain and simple.

So what do we do?

First, let's write simple code that works, and loads some Foos and Bars:

// No RowConvertible adoption: those types don't fit the contract.
class Base {
    static func getInstance(row: Row) -> Self {
        if row["foo"] == 1 {
          return unsafeDowncast(Foo(row: row), to: self)
        } else {
          return unsafeDowncast(Bar(row: row), to: self)
        }
    }
}
class Foo: Base {
    init(row: Row) { ... }
}
class Bar: Base {
    init(row: Row) { ... }
}

And now we can load:

// Let's build a cursor of Base:
let sql = "SELECT * FROM base"
let rows = try Row.fetchCursor(db, sql)
let bases = rows.map { Base.getInstance(row: $0) }

All right. We get a cursor of bases fetched from raw SQL, which is almost the lowest level we can go until we hit hard SQLite metal. The type of the cursor is MapCursor<RowCursor, Base>. Not very interesting, but we'll use this fact below.

We can go one more level down if we replace sql with a prepared statement:

let sql = "SELECT * FROM base"
let statement = try db.makeSelectStatement(sql)
let rows = try Row.fetchCursor(statement)
let bases = rows.map { Base.getInstance(row: $0) }

Now we're quite efficient: this simple code can be used to build higher constructs.

And that's what we'll do: build everything we need from this base. What RowConvertible does for free, we'll do it by hand since Base can't adopt RowConvertible. Let's build the mother of all Base-fetching methods: fetchCursor, inspired from RowConvertible's one:

extension Base {
    static func fetchCursor(_ statement: SelectStatement, arguments: StatementArguments? = nil, adapter: RowAdapter? = nil) throws -> MapCursor<RowCursor, Base> {
        return try Row
            .fetchCursor(statement, arguments: arguments, adapter: adapter)
            .map { Base.getInstance(row: $0) }
    }
}

// Gee!
let sql = "SELECT * FROM base"
let statement = try db.makeSelectStatement(sql)
let bases = try Base.fetchCursor(statement) // cursor of Base

From fetchCursor, we derive fetchAll and fetchOne:

extension Base {
    static func fetchAll(_ statement: SelectStatement, arguments: StatementArguments? = nil, adapter: RowAdapter? = nil) throws -> [Base] {
        return try Array(fetchCursor(statement, arguments: arguments, adapter: adapter))
    }

    static func fetchOne(_ statement: SelectStatement, arguments: StatementArguments? = nil, adapter: RowAdapter? = nil) throws -> Base? {
        return try fetchCursor(statement, arguments: arguments, adapter: adapter).next()
    }
}

// Wow
let sql = "SELECT * FROM base"
let statement = try db.makeSelectStatement(sql)
let bases = try Base.fetchAll(statement) // [Base]
let base = try Base.fetchOne(statement)  // Base?

But statements are a little bit low level. Let's bump to requests, then to SQL, as RowConvertible does:

extension Base {
    static func fetchCursor(_ db: Database, _ request: Request) throws -> MapCursor<RowCursor, Base> {
        let (statement, adapter) = try request.prepare(db)
        return try fetchCursor(statement, adapter: adapter)
    }

    static func fetchAll(_ db: Database, _ request: Request) throws -> [Base] {
        let (statement, adapter) = try request.prepare(db)
        return try fetchAll(statement, adapter: adapter)
    }

    static func fetchOne(_ db: Database, _ request: Request) throws -> Base? {
        let (statement, adapter) = try request.prepare(db)
        return try fetchOne(statement, adapter: adapter)
    }
}

extension Base {
    static func fetchCursor(_ db: Database, _ sql: String, arguments: StatementArguments? = nil, adapter: RowAdapter? = nil) throws -> MapCursor<RowCursor, Base> {
        return try fetchCursor(db, SQLRequest(sql, arguments: arguments, adapter: adapter))
    }

    static func fetchAll(_ db: Database, _ sql: String, arguments: StatementArguments? = nil, adapter: RowAdapter? = nil) throws -> [Base] {
        return try fetchAll(db, SQLRequest(sql, arguments: arguments, adapter: adapter))
    }

    static func fetchOne(_ db: Database, _ sql: String, arguments: StatementArguments? = nil, adapter: RowAdapter? = nil) throws -> Base? {
        return try fetchOne(db, SQLRequest(sql, arguments: arguments, adapter: adapter))
    }
}


// Woohooo raw SQL!
let sql = "SELECT * FROM base"
let bases = try Base.fetchCursor(db, sql) // Cursor of Base
let bases = try Base.fetchAll(db, sql)    // [Base]
let base = try Base.fetchOne(db, sql)     // Base?

// Welcome requests!
extension Base: TableMapping { ... }
let request = Base.filter(...).order(...)
let bases = try Base.fetchCursor(db, request) // Cursor of Base
let bases = try Base.fetchAll(db, request)    // [Base]
let base = try Base.fetchOne(db, request)     // Base?

We'd prefer to write Base.filter(...).order(...).fetchAll(db):

extension TypedRequest where RowDecoder == Base {
    func fetchCursor(_ db: Database) throws -> RecordCursor<Base> {
        return try Base.fetchCursor(db, self)
    }

    func fetchAll(_ db: Database) throws -> [Base] {
        return try Base.fetchAll(db, self)
    }

    func fetchOne(_ db: Database) throws -> Base? {
        return try Base.fetchOne(db, self)
    }
}

// Much complete now:
let request = Base.filter(...).order(...)
let bases = try request.fetchCursor(db) // Cursor of Base
let bases = try request.fetchAll(db)    // [Base]
let base = try request.fetchOne(db)     // Base?

You got it: don't use RowConvertible, since it just can't initialize the Base instances you need. Instead, just redo it from scratch. It's not funny, but GRDB won't prevent you from defining your own fetching strategy, you see? Record protocols are conveniences: you can add your own.

Final touch: define a protocol with the factory method, and attach the above methods to the protocol instead of Base:

protocol RowConvertibleFactory {
    static func getInstance(row: Row) -> Self
}

extension Base: RowConvertibleFactory { ... }

extension RowConvertibleFactory {
    static func fetchCursor(_ statement: SelectStatement, arguments: StatementArguments? = nil, adapter: RowAdapter? = nil) throws -> MapCursor<RowCursor, Self>
    ...
}

And you'll have a nice and reusable extension to GRDB that will work with all your types that need factory methods.

[thommw]

Hi Gwendal,

Thank you for taking the time to write such an exhaustive treatment of my little problem.

I have of course already considered some of your solutions, but I am a "lazy" developer and they require a lot of typing. :)

The fix that I am suggesting is tiny and it still leaves you the choice between using the factory method or the initializer. If you don't implement the factory method then the initializer is used automagically.

I have run all the tests in GRDBiOSTests and they all passed. I would have been surprised if the change had broken anything.

The change will add a nice little feature that doesn't cost anything. I am quite happy to patch my copy whenever I need to, should you decide it is better to leave the API as it is.

Lastly I would like to thank you for all the hard work you have put into this great piece of software!

Cheers,
Thomas

[thommw]

P.S.: For those that find the unsafeDowncast() a bit too scary, here is a slightly more verbose way of doing it, where the compiler is able to figure out the type:

public static func getInstance(row: Row) -> Self {
    return getInstanceHelper(row: row)
}

public static func getInstanceHelper<T>(row: Row) -> T {
    if row["foo"] == 1 {
      return Foo(row: row) as! T
    } else {
      return Bar(row: row) as! T
    }
}

[groue]

Dont be lazy: submit a pull request. We'll see how tiny this change is.

[thommw]

Ok, I have never done this, but will give it a try. :)

[groue]

Yes. I'm sorry I'm giving you some work. But I'm lazy, too. More seriously: since I did try factory methods, and eventually changed my mind and settled on an initializer, I do still fear some unwanted side-effect. Well, Swift has evolved since: maybe the old problems don't exist any longer. I'm curious.

[groue]

And remember that you can still do this today (without writing extensions to Base, and without modifying GRDB):

// cursor
let bases = try Row.fetchCursor(...).map { Base.getInstance(row: $0) }
// array
let bases = try Array(Row.fetchCursor(...).map { Base.getInstance(row: $0) })
// single
let base = try Row.fetchOne(...).map { Base.getInstance(row: $0) }

// From a query interface request
let request = Base.filter(...).order(...)
let bases = try Row.fetchCursor(db, request).map { Base.getInstance(row: $0) }
let bases = try Array(Row.fetchCursor(db, request).map { Base.getInstance(row: $0) })
let base = try Row.fetchOne(db, request).map { Base.getInstance(row: $0) }

This means that we're not in dire need of polymorphic fetches.

[thommw]

I tried to push my branch up to the repository but getting denied. Of course I would need to be configured as a contributor. Here is the diff if you don't want to go to the trouble:

diff --git a/GRDB/Record/RowConvertible.swift b/GRDB/Record/RowConvertible.swift
index 3162400e..241b6e2a 100644
--- a/GRDB/Record/RowConvertible.swift
+++ b/GRDB/Record/RowConvertible.swift
@@ -30,6 +30,15 @@ public protocol RowConvertible {
     /// iteration of a fetch query. If you want to keep the row for later use,
     /// make sure to store a copy: `self.row = row.copy()`.
     init(row: Row)
+  static func getInstance(row: Row) -> Self
+}
+
+extension RowConvertible {
+
+  public static func getInstance(row: Row) -> Self {
+    return Self.init(row: row)
+  }
+
 }

 /// A cursor of records. For example:
@@ -59,7 +68,7 @@ public final class RecordCursor<Record: RowConvertible> : Cursor {
             done = true
             return nil
         case SQLITE_ROW:
-            return Record(row: row)
+          return Record.getInstance(row: row)
         case let code:
             statement.database.selectStatementDidFail(statement)
             throw DatabaseError(resultCode: code, message: statement.database.lastErrorMessage, sql: statement.sql, arguments: statement.arguments)

[groue]

Welcome to GitHub pull requests 😉 You're supposed to fork the repository you want to contribute to. Check https://help.github.com/articles/about-pull-requests/ for more information.

But don't rush: now that I see your diff, I know that I won't accept it:

There are now two ways to derive a record from a row: the initializer, and the factory method. This can't be, oh no. There must be a single one. The single one that always does the correct job. It's like two cowboys in a far west city: only one can survive.

• If you have two possible methods, you have to explain when one should be used, or the other. Imagine you have to write documentation.
• If you have two possible methods, the two may not agree (they may return incompatible results). What do you do in this case? You weep. The only solution is to avoid the problem right from the start, and keep a single method.
• Your change allows Base.init(row:) (the parent of Foo and Bar), which is meaningless. That's not good. If RowConvertible requires Base.init(row:) to exist, then it must return something.
• You did replace one init(row:), but you didn't look for other places where init(row:) is called. Do you really want GRDB to sometimes call init(row:), and sometimes getInstance(row:)? That would be funny, don't you think? Especially when it calls the weird Base.init(row:).

I thank you very much for your interest, and idea. But I hope I had you realize that this "tiny" change, in its current shape, is not ready to be included.

My best answer so far to your question is https://github.com/groue/GRDB.swift/issues/341#issuecomment-384292650.

I'm looking forward your next contribution!

[groue]

(Issue is closed, but discussion is not, if you have more question)

[groue]

Follow up: https://github.com/groue/GRDB.swift/issues/349

[groue]

A new chapter has shipped in the GRDB 3 documentation: Customized Decoding of Database Rows.