Add FTS pattern sanitizer

[CSchwang]

An invalid FTS match query leads to a crash. E.g. trying to execute X.match("hello (something") gives:

fatal error: 'try!' expression unexpectedly raised an error: malformed MATCH expression: [hello (adsfasdf]: file /Library/Caches/com.apple.xbs/Sources/swiftlang/swiftlang-700.1.101.15/src/swift/stdlib/public/core/ErrorType.swift, line 50

Sample playground showing the behavior:
https://gist.github.com/CSchwang/937f9ae9fcbf8e6e0152

It would be nice to be able to handle the error instead of crashing

[ryanholden8]

Merging PR #290 would address this..

[mikemee]

Thanks for the gist, that makes it so much easier!

This still fails even after PR #290, due to this code in Connection.swift:

    public func pluck(query: QueryType) -> Row? {
        return try! prepare(query.limit(1, query.clauses.limit?.offset)).generate().next()
    }

Now that db.prepare can throw, I suspect pluck should be able to also. However perhaps this is a philosophical issue for @stephencelis to weigh in on.

---

And, fyi, in case the gist disappears, here is the repro code, slightly tweaked to allow for #290 change:

import SQLite

let db = try! Connection()

db.trace { print($0) }

let emails = VirtualTable("emails")

let subject = Expression<String?>("subject")
let body = Expression<String?>("body")

try! db.run(emails.create(.FTS4(subject, body)))

try! db.run(emails.insert(
    subject <- "Hello, world!",
    body <- "This is a hello world message."
    ))

let workingRow = db.pluck(emails.match("hello"))

// crashes here
let failingRow = db.pluck(emails.match("hello (adsfasdf"))

[stephencelis]

@mikemee is right in wondering if there is a philosophical reason for pluck causing a fatal error and prepare throwing. From the discussion:

1. On the one hand, recovering from errors is important when you're dealing with validation and other recoverable failures.
2. On the other hand, SQL statements that read from the database generally shouldn't fail unless the statement is invalid (e.g., a syntax error, which is usually a developer issue).
   Because of this, I'm wondering if we should only promote prepare to be throws, but let the others remain fatal. With this change, developers that need the flexibility of error recovery can use db.prepare, but the rest can generally use the simpler, non-throwing APIs (scalar, pluck, etc.).

I'm open to discussion on these points, but I'd like to minimize the need for explicit error handling (and noise) wherever possible. At the moment I'm inclined to not consider this a bug (but it might require further documentation/reasoning).

[mikemee]

Not to be contrary, but I can easily imagine how the argument to match could be a user generated string, say. If so, how can this be tested beforehand to avoid the crash? I guess we're saying everyone should use prepare first, just in case?

Darn error handling!

[mikemee]

Note that #259 is also having a problem with db.pluck crashing (though seemingly for multi-threading and/or database contention problems).

It does really suck when errors can't be caught ... I fear there is little choice here but to eventually remove all the try! invocations and allow the errors to propagate. Philosophically, the strong typing already takes care of many errors, but imnsho database code can always fail for strange reasons...

[tomtaylor]

We just got caught by this. Our queries are user generated. For now, we're stripping non-alphanumerics, but it'd be great to match on other characters. Can we fix it at least for the match method, or does it require deeper plumbing?

[trongcuong1710]

I just get caught by this, too. If I put a parenthesis into the query, it would crash. I can work around by not allowing user to enter this character, but this not seem to be a proper solution

[groue]

You can look at the way GRDB.swift handles search pattern sanitization (FTS3/4, FTS5).