Attach and detach

[macu]

Reading the documentation for this project I thought, "This is what Swift should be." I think it merits attach and detach methods (returning Bool or NSError). Some of the quirks to consider...

Working with FMDB, db.executeUpdate("ATTACH ? AS ?", withArgumentsInArray: [attachFilepath, attachAs]) is always returning true in my environment, perhaps everywhere. To determine success, I have to verify that the attached database is actually listed in the database_list pragma:

// Verify that the database is listed in the database_list pragma.
// http://www.sqlite.org/pragma.html#pragma_database_list
if let r = db.executeQuery("pragma database_list", withArgumentsInArray: []) {
    while r.next() {
        if let dbName = r.stringForColumnIndex(1) {
            if dbName == attachAs {
                return true
            }
        }
    }
}
return false

But even this wasn't sufficient. A zero-byte or corrupt database file could be attached, and would be listed in the database_list pragma, so I then test whether a query can be made against the expected tables:

/// Returns whether a query over the given table and columns may produce results.
func testQuery(#table: String, columns: [String]? = nil) -> Bool {
    let cols = (columns == nil) ? "*" : ",".join(columns!)
    let query = "SELECT \(cols) FROM \(table) LIMIT 1"
    // Result set will be nil if the query is invalid.
    return self.executeQuery(query, withArgumentsInArray: []) != nil
}

There may be a better way to determine the success of attach. If someone decides to implement attach and detach, I hope these notes will be useful. 👍

[stephencelis]

I've never had the chance to use ATTACH/DETACH, but am open to a pull request.

SQLite.swift prefers to assert developer error early, and if your app is unable to attach to another database that it expects to exist, that's probably a hard error that you want to fail immediately in DEBUG mode (rather than a soft Bool that's easier to ignore and more complicated/cyclomatic to maintain).

[stephencelis]

(One thing to note: there is currently no way to disambiguate Query tables by database/namespace. Support for this would be needed to properly handle ATTACHed databases with ambiguous table names.)

[jberkel]

@macu I think the 0-byte files are a result of lazy creation - immediately after attaching the db has size 0, then after the first DDL statements it grows.

@stephencelis for simple cases it would be enough to do something like db["mydb.mytable"]. however insert / create statements always quote the table name which leads to the creation of the table mydb.mytable in the main namespace / database.

[stephencelis]

@jberkel Yep. For now, everything's quoted where it can be, and because of this, relying and detecting dots is problematic because you could, in theory, have a table name with a period in it.

Alternatively, we could add a subscript with another parameter:

let table = db["database", "table"]
// or
let table = db["table", namespace: "database"]

I've actually been envisioning a bit of a refactor, however, where Query objects are for the most independent from database connections, but are passed to the database connections when you want to run a SELECT/UPDATE/DELETE, etc. This would allow for Query objects to be more flexible and not be coupled to a specific database connection. E.g.,

let users = Query("users")
for user in db.select(users) { /* … */ }

db.insert(into: users, values: values)

db.update(users, set: values)

db.delete(from: users)

db.scalar(users.count)

I'm not sure about the interface yet, and it creates a bit more noise, but I think it'll be a step in the right direction.

[jberkel]

Agreed on decoupling Query objects from the db, I feels weird to have them mixed up. I like the db["table", namespace: "database"] approach, however I don't think namespaces/multi-dbs are a widely used sqlite feature, so I would not put more parameters it in the subscript. How about

db.database("database")["table"]

or

db.scope(database: "database")["table"]

The database returned by these calls would wrap the existing db + automatically scope all operations on it. But with your refactoring in mind it would probably be nicer just to say

let users = Query("users", database: "foo")
db.select(users)

With multi-db support it might make sense to rename Database to Connection to avoid more confusion.

[stephencelis]

@macu See https://www.sqlite.org/uri.html for more information on what formats can be passed to ATTACH. I found that if you use the file:// protocol and the mode=rw query parameter it will return an error if the database doesn't already exist. If you pass a path (as in your snippet above), the default mode is rwc (read-write-create) and it will create the database automatically if it doesn't already exist, only returning an error if it doesn't have permission to do so at the given path. So I believe your FMDB calls actually weren't failing at all. They were, however, creating and attaching new databases when you expected to attach to an existing database.

[macu]

@stephencelis Yes that must have been it, which was the reason for all the hassle checking if queries could be performed. Great find about the URI mode parameter, and thank you for the link; I will clean up my projects tomorrow.

[mikemee]

Excuse my ignorance, but I've read this a few times now and aren't sure what if anything should be done with the current codebase (which has changed quite a bit since March 2015). It feels like there's a request lingering in here, but I'm not sure exactly what it should be.

Closing this out, but please re-open if I'm missing something or #298 doesn't already cover it. Thanks!

[jberkel]

#298 is unrelated, I only referenced this issue for context.

Just checked the current codebase and it is indeed now possible to specify a database when creating a reference to a table, thus avoiding the quoting problem mentioned above:

let table = Table("foo", database: "attached")

https://github.com/stephencelis/SQLite.swift/blob/master/SQLite/Typed/Query.swift#L782

What still needs to be done is to encapsulate ATTACH DATABASE / DETACH DATABASE as methods, add tests + documentation.

[stephencelis]

Yep, sorry for the confusion @mikemee! This is still something that would be helpful to have implemented.