Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to handle requests that require write access? #42

Closed
cfilipov opened this issue Oct 24, 2018 · 12 comments
Closed

How to handle requests that require write access? #42

cfilipov opened this issue Oct 24, 2018 · 12 comments

Comments

@cfilipov
Copy link

This question can also apply to plain GRDB without Rx.

I have a lot of complex queries that involve multiple statements in the form of "setup", "query" then "teardown". For example, many times I find myself creating a few temp tables, adding indexes, querying, then dropping the tmp tables.

var result: [MyRecordType] = []
try db.inTransaction {
    try db.execute("""
        CREATE TEMPORARY TABLE tmpTable1 ...;
        CREATE TEMPORARY TABLE tmpTable2 ...;

        CREATE INDEX tmpTable1 ...;
        CREATE INDEX tmpTable2 ...;
        """)
    result = try MyRecordType.fetchAll(db, "SELECT * FROM tmpTable1 JOIN tmpTable2 ...;")
    try db.execute("""
        DROP TABLE IF EXISTS tmpTable1;
        DROP TABLE IF EXISTS tmpTable2;
        """)
    return .commit
}
return result

There's no way to neatly pack the whole thing into a fetch request or even a SQLQuery so I end up putting the above code into a get(_ db: Database) method somewhere. This felt clumsy but worked until I wanted to use the Rx extensions to get notified of changes.

How would I do this with RxGRDB and ensure the temp tables are created and destroyed each time the query is ran for a change?

@groue
Copy link
Collaborator

groue commented Oct 24, 2018

Hello @cfilipov,

If I understand well, there are two questions in your issue:

  1. How do I encapsulate a complex request?
  2. How do I observe such a complex request?

How do I encapsulate a complex request?

GRDB ships with a few concrete request types. Mainly:

  • QueryInterfaceRequest

    let request = Player.all()
    let request = Player.including(...).filter(...).order(...)
  • SQLRequest

    let request = SQLRequest<Player>("SELECT * FROM players WHERE id = ?", arguments: [1])

All those requests adopt FetchRequest, the protocol for "requests that run from a single select statement, and tell how fetched rows should be interpreted".

Indeed, when a FetchRequest is bound to a fetchable type (FetchableRecord, raw Row, value such as String, Int, etc.), it grants methods that rely on this single select statement:

// Consumes a single statement result, if any
try request.fetchOne(db)    // Value?

// Iterates all statement results
try request.fetchAll(db)    // [Value]

// Iterates all statement results, in a lazy fashion
try request.fetchCursor(db) // Cursor of Value

Conclusion: whenever a request does not run from a single select statement, the FetchRequest protocol is not a good fit.

Instead, just provide your custom request type, which does not adopt any specific protocol. If you want to honor GRDB naming convention for fetching methods, define a fetchAll(_:) method on it. Prefer inSavepoint instead of inTransaction so that your request can be used even if a transaction is already open. When your customized request requires write access, use it from a database access method that lets you write:

struct MyRequest {
    func fetchAll(_ db: Database) throws -> [MyRecordType] {
        var result: [MyRecordType] = []
        try db.inSavepoint { // Savepoint instead of transaction
            try db.execute("""
                CREATE TEMPORARY TABLE tmpTable1 ...;
                CREATE TEMPORARY TABLE tmpTable2 ...;
                
                CREATE INDEX tmpTable1 ...;
                CREATE INDEX tmpTable2 ...;
                """)
            result = try MyRecordType.fetchAll(db, "SELECT * FROM tmpTable1 JOIN tmpTable2 ...;")
            try db.execute("""
                DROP TABLE IF EXISTS tmpTable1;
                DROP TABLE IF EXISTS tmpTable2;
                """)
            return .commit
        }
        return result
    }
}

let request = MyRequest()
let result = try dbQueue.write { db in
    try request.fetchAll(db)
}

How do I observe such a complex request?

It's the first time someone asks for observation of a request which writes 👍

This has consequences, because all RxGRDB value observables won't work, currently, with such requests.

Without RxGRDB value observables, you won't get free notifications of your observed request results on the main queue, for example. You'll have to do it yourself, and we'll see how below.

This time, we need a change observable. Those give database connections which are able to write, and with such a write access, we can fetch the results of our custom request.

So here is our plan:

  1. Make our request adopt the DatabaseRegionConvertible protocol (and ReactiveCompatible, which adds the .rx member).
  2. Observe changes in our request.
  3. Fetch new values on each change.
  4. Dispatch the values on our target queue.
extension MyRequest: ReactiveCompatible, DatabaseRegionConvertible {
    func databaseRegion(_ db: Database) throws -> DatabaseRegion {
        return ...
    }
}

let request = MyRequest()
request.rx
    .changes(in: dbQueue)
    .map { db in try request.fetchAll(db) }
    .observeOn(/* your favourite scheduler */)
    .subscribe(onNext: { records: [MyRecordType] in
    })

@groue
Copy link
Collaborator

groue commented Oct 24, 2018

It's the first time someone asks for observation of a request which writes 👍

Note to myself: the list of use cases for changes observables in the RxGRDB README has to be updated.

@cfilipov
Copy link
Author

How would one go about conforming to DatabaseRegionConvertible in this case? There doesn't appear to be a way.

Normally you would call databaseRegion on the result of a query interface request or from SQLRequest, but the tables we are interested in watching are defined in the first execute call where we create the temp tables. I notice there is an UpdateStatement that gets made when you call execute but that does not have databaseRegion defined so I can't use that.

DatabaseRegion does have what looks to be several useful initializers that take table name as input. I could manually specify the database region by doing DatabaseRegion(table: "foo").union(DatabaseRegion(table: "bar")) but those initializers are not public.

From what I can tell GRDB only supports creating database regions for select statements.

@groue
Copy link
Collaborator

groue commented Oct 24, 2018

Normally you would call databaseRegion on the result of a query interface request or from SQLRequest, but the tables we are interested in watching are defined in the first execute call where we create the temp tables.

I was assuming that those temporary tables were populated from the content of other database tables, and that it would be enough to observe those source tables in order to trigger a fresh refetch.

If my assumption is wrong, what database event are you interested in?

If it is true, then do observe the source tables:

[...] manually specify the database region by doing DatabaseRegion(table: "foo").union(DatabaseRegion(table: "bar")) but those initializers are not public.

Indeed, there is no such public initializer. The only public way to create DatabaseRegion is from requests, or select statements. For example:

struct Foo: TableRecord {
    static let databaseTableName = "foo"
}
struct Bar: TableRecord {
    static let databaseTableName = "bar"
}

let fooRegion = try Foo.all().databaseRegion(db)
let barRegion = try Bar.all().databaseRegion(db)
let region = fooRegion.union(barRegion)

Also valid:

let fooRegion = try SQLRequest<Row>("SELECT * FROM foo").databaseRegion(db)
let barRegion = try SQLRequest<Row>("SELECT * FROM bar").databaseRegion(db)
let region = fooRegion.union(barRegion)

This may be verbose in you particular case, but I prefer it this way. Database regions are guaranteed to describe valid regions that interact well with SQLite, they provide opportunities for optimizations, and since they totally hide their inner implementation, there is room for future features without the burden of supporting "manual regions".

@cfilipov
Copy link
Author

The only public way to create DatabaseRegion is from requests, or select statements.

That was the part I was missing! Thank you!

@cfilipov
Copy link
Author

By the way I would be interested in what you think could be a way forward for a more idiomatic use of such queries. I find myself with quite a few of these complex queries so I defined the following:

struct Statement {
    let sql: String
    let arguments: StatementArguments?
}

struct CompoundSqlRequest<T: FetchableRecord>: DatabaseRegionConvertible {

    typealias RegionFunc = (Database) throws -> DatabaseRegion
    private let regionFunc: RegionFunc
    private let query: Statement
    private let statement: (
        setup: Statement?,
        teardown: Statement?
    )

    init(
        query: Statement,
        setup: Statement? = nil,
        teardown: Statement? = nil,
        regionFunc: @escaping RegionFunc = { _ in DatabaseRegion() }
    ) {
        self.regionFunc = regionFunc
        self.query = query
        self.statement = (setup, teardown)
    }

    init(_ sql: String, arguments: StatementArguments? = nil) {
        self = CompoundSqlRequest(query: Statement(sql: sql, arguments: arguments))
    }

    func setup(_ sql: String, arguments: StatementArguments? = nil) -> CompoundSqlRequest<T> {
        return CompoundSqlRequest(
            query: query,
            setup: Statement(sql: sql, arguments: arguments),
            teardown: statement.teardown,
            regionFunc: regionFunc)
    }

    func teardown(_ sql: String, arguments: StatementArguments? = nil) -> CompoundSqlRequest<T> {
        return CompoundSqlRequest(
            query: query,
            setup: statement.setup,
            teardown: Statement(sql: sql, arguments: arguments),
            regionFunc: regionFunc)
    }

    func region(_ regionFunc: @escaping RegionFunc) -> CompoundSqlRequest<T> {
        return CompoundSqlRequest(
            query: query,
            setup: statement.setup,
            teardown: statement.teardown,
            regionFunc: regionFunc)
    }

    public var reactive: Reactive<CompoundSqlRequest> {
        return Reactive(self)
    }

    func databaseRegion(_ db: Database) throws -> DatabaseRegion {
        return try regionFunc(db)
    }

    func fetchAll(_ db: Database) throws -> [T] {
        var result: [T] = []
        try db.inSavepoint {
            if let setup = statement.setup {
                try db.execute(setup.sql, arguments: setup.arguments)
            }
            result = try T.fetchAll(db, query.sql, arguments: query.arguments)
            if let teardown = statement.teardown {
                try db.execute(teardown.sql, arguments: teardown.arguments)
            }
            return .commit
        }
        return result
    }
}

This allows me to write the following to easily get a request:

struct MyRecord: Codable, FetchableRecord {
    var id: Int64
    ...
}

extension MyRecord {
    static func request() -> CompoundSqlRequest<MyRecord> {
        return CompoundSqlRequest("SELECT * FROM tmpTable1 JOIN tmpTable2 ...;")
            .setup("""
                CREATE TEMPORARY TABLE tmpTable1 ...;
                CREATE TEMPORARY TABLE tmpTable2 ...;
                
                CREATE INDEX tmpTable1 ...;
                CREATE INDEX tmpTable2 ...;
                """)
            .teardown("""
                DROP TABLE IF EXISTS tmpTable1;
                DROP TABLE IF EXISTS tmpTable2;
                """)
            .region(SourceTable.all().databaseRegion)
    }
}

...

let request = MyRecord.request()
return request.reactive
    .changes(in: queue)
    .map { db in try request.fetchAll(db) }

@groue
Copy link
Collaborator

groue commented Oct 25, 2018

This looks great! I like this snippet:

return CompoundSqlRequest(...)...
    .region(SourceTable.all().databaseRegion) // <3

That's a nice way to feed a (Database) -> DatabaseRegion closure, I don't think about this Swift feature often enough :-)

I also really appreciate how you define a few convenience types and methods on top of primitives. They help a lot your main code be terse and super clear: just define a request, observe it, and react to its changes 👍

@groue groue changed the title How to handle complex, multi-statement query How to handle requests that require write access? Oct 28, 2018
@groue
Copy link
Collaborator

groue commented Nov 2, 2018

@cfilipov With GRDB 3.5, observing your request can now read:

let request = MyRecord.request()
var observation = ValueObservation.tracking(request, fetch: request.fetchAll)
observation.requiresWriteAccess = true

// GRDB 3.5.0
let observer = try observation.start(in: dbQueue) { records in
    print("Records have changed")
}

With RxGRDB 0.13.0, it gives:

// RxGRDB 0.13.0
observation.rx
    .fetch(in: dbQueue)
    .subscribe(onNext: { records in
        print("Records have changed")
    }

The only thing you may want to change in CompoundSqlRequest is removing the savepoint: it is now automatically added by ValueObservation.

@cfilipov
Copy link
Author

cfilipov commented Nov 2, 2018

...
var observation = ValueObservation.tracking(request, fetch: request.fetchAll)
observation.requiresWriteAccess = true
...

Could the requiresWriteAccess be a property of DatabaseRegionConvertible? This way passing a request is all that's needed. It makes sense to me because it's the request itself that requires write access or not and the value observation should simply check that property.

@groue
Copy link
Collaborator

groue commented Nov 2, 2018

I'm not that sure yet it is a good idea. DatabaseRegionConvertivle is not just about your use case. Please develop your thoughts, and don't think only about you in order to convince me. Make it bigger than you. Thanks.

@cfilipov
Copy link
Author

cfilipov commented Nov 2, 2018

Please develop your thoughts, and don't think only about you in order to convince me. Make it bigger than you. Thanks.

I've really enjoyed out correspondence up to this point but I do not appreciate your condescending remark.

A request that requires write access is a property of the request. I was pointing out that having to manually set that as a flag on ValueObservation seemed backwards (and prone to error). I suggested DatabaseRegionConvertible because that's what is currently passed in to tracking, but as you point out this is probably not the appropriate protocol to put this on.

It seems to me that ValueObservation should take a request rather than a DatabaseRegionConvertible. Of course, FetchRequest does not cover requests that write and there is no other type that does.

@groue
Copy link
Collaborator

groue commented Nov 3, 2018

@cfilipov I'm sorry my words did not sound nice, but they could not reflect my thoughts less exactly. Yet you deserve a better answer.

I suggest the reading of The Obvious, the Easy, and the Possible blog post by Jason Fried.

Requests that write now belong to the possible realm, thanks to your suggestion. They certainly are not abvious, and not even easy. But they are made possible in the latest GRDB release, in a way that was not possible before.

You write:

It seems to me that ValueObservation should take a request rather than a DatabaseRegionConvertible.

I'm not sure at all. ValueObservation distinguishes the region it observes from the values it fetches. This is a strict separation of concerns, and a way to break the coupling between the two inputs. It happens that some types both define one region, and one fetching method. But this is a particular case.

A regular query interface request, for example, defines an observable database region, and three fetching methods: fetchCount, fetchOne, fetchAll.

Limiting the supported use cases to requests that provide a region and a single fetching method would break this. This is a total non-starter, considering that query interface requests belong to the obvious realm.

Of course, FetchRequest does not cover requests that write and there is no other type that does.

Yes! But nothing prevents you from defining an extra protocol, or an extra ValueObservation factory method (you'll choose) that does exactly what your application needs! Because, come on, it is possible and available right now! And I think that it's great :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants