-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Removing SQLite.swift Dependency #20
Conversation
- Adds .DS_Store files to gitignore - Rewrites the SQLiteStorageEngine to use the build-in SQLite C API - Removes SQLite.swift dependency - Updates SQLiteStorageEngine test file to delete store after test completion - Verified that all tests pass
Hey @brettohland, this looks great! Thank you so much for this contribution, it not only looks great (to my somewhat untrained raw SQLite eyes), but it's incredibly helpful because I've wanted to do this for quite some time. I've taken a cursory look at it and I'll be very happy to merge this in when I have time for a closer look. I want to properly set expectations and not make you think that I'm ignoring it, but I'm on a bit of a tight deadline in non-open source land, and am very close to shipping a new app. Because of that I won't be able to merge this into main for another week or two, but if you'd like to merge this onto another branch (I can make one called Would love to know how that sounds, and once again thank you for the help. 🙂 |
@@ -3,13 +3,24 @@ import XCTest | |||
|
|||
final class SQLiteStorageEngineTests: XCTestCase { | |||
|
|||
private var storage: SQLiteStorageEngine! | |||
private static var _storage: SQLiteStorageEngine! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm not familiar with this pattern but is there a reason to create this new property (_storage
) and then have a computed property below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, yeah It probably would have benefitted to have added a comment here. Through a combination of necessity, and not wanting to make massive changes to this test case, I had to get a bit creative.
The backing sqlite3
file wasn't being deleted between test runs which was an issue during development, but to fix this requires that I hold onto a reference until the static tearDown
method is called at the end of the test case. One of the delights of the SQLite C API is that the database connection pointer is needed to properly close the connection before deleting the backing file or else some API misuse warnings are thrown in the console.
So I'm essentially just creating a very rough singleton here in order to capture the reference to the store so that I can call it's internal delete command correctly while also keeping the rest of the test case from needing to reference Self
every time it interacts with the store.
Glad to help. I'm integrating
No worries on the merge being a few weeks out, the project this is getting integrated is still in its awkward toddler phase right now so the extra dependency isn't an issue. Since I'm accessing this through |
if !Self.directoryExists(atURL: directory.url) { | ||
try Self.createDirectory(url: directory.url) | ||
} | ||
sqliteFileURL = try Self.createSQLiteFile(in: directory, withFilename: filename, attributes: [.protectionKey: fileProtection]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mergesort One thing that I had noticed for both the current SQLiteStorageEngine
as well as the DiskStorageEngine
is a lack of the ability to explicitly set the URLFileProtection
level on the created files on disk.
I've added this here and the default protection level mirrors the system standard, but I was wondering if that was an explicit decision that was made during development?
From experience I know that setting it to .completeUnlessOpen
or .complete
can complicate things when it comes to interacting with files while your app is in the background, but not providing options to set the level is important for security reasons.
PR mergesort#20 essentially asks for the ability to use Bodega without using any dependencies. This PR provides a possible implementation for that. The main idea is that Bodega now exposes two possible libraries to use: 1. BodegaCore - the bulk of the existing functionality, but without the SQLiteStorageEngine 2. Bodega - the existing SQliteStorageEngine implementation For everyone who wants the simple use-case, they can continue to `import Bodega`. The Bodega target does an `@_exported import BodegaCore`, which means that it re-exposes all the types and symbols of BodegaCore, as if they came from Bodega itself. For the occasional user who wants to use Bodega *without* dependencies, they would `import BodegaCore` and then implement their own StorageEngine themselves in their own code (such as one that uses the system SQLite3 module as the basis for storage). The code changes to support this are very minimal: 1. The addition of a new target + product in Package.swift 2. Moving the bulk of the files to a new BodegaCore folder 3. Changing `CacheKey.rawValue` to be public so it can be accessed by the SQLiteStorageEngine 4. Adding a testable import of BodegaCore in a unit test file
static func createTableNamed(_ tableName: String, inDatabase database: OpaquePointer?) throws { | ||
let command = """ | ||
CREATE TABLE IF NOT EXISTS \(tableName) ( | ||
"\(Column.key.rawValue)" TEXT PRIMARY KEY NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that need proper escaping, i.e. a bound variable?
let currentDateString = Self.dateFormatter.string(from: Date()) | ||
for entry in dataAndKeys { | ||
let bytes = [UInt8](entry.data) | ||
sqlite3_bind_text(insertStatement, 1, strdup(entry.key.rawValue), -1, SQLITE_TRANSIENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is leaking the strdup
result, just bind the value w/ SQLITE_TRANSIENT. SQLite will dup and free properly.
If you want to avoid the strdup
overhead, this can also be done w/ static and a nested call to step. Given the specific focus of this thing, probably worth doing.
sqlite3_bind_text(insertStatement, 4, strdup(currentDateString), -1, SQLITE_TRANSIENT) | ||
let result = sqlite3_step(insertStatement) | ||
guard result == SQLITE_DONE else { | ||
throw SQLiteError.insertFailed(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this leak the statement on errors? Maybe a defer
for dealloc would be better.
LIMIT | ||
\(keys.count) | ||
""" | ||
var removeStatement: OpaquePointer? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this optional if the prepare throws on errors? Presumably the func should just return the statement or throw otherwise.
FROM | ||
\(tableName) | ||
WHERE | ||
\(Column.key.rawValue) IN (\(Array(repeating: "?", count: keys.count).joined(separator: ", "))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is OK, but not exactly cheap. This creates a ton of malloced objects just to concat a static String. Maybe rather make that procedural.
Also again, is the key guaranteed to be SQL safe or does it need escaping? Aka proper binding.
\(tableName) | ||
WHERE | ||
\(Column.key.rawValue) IN (\(Array(repeating: "?", count: keys.count).joined(separator: ", "))) | ||
LIMIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointless limit? Just extra SQL to be processed.
var selectStatement: OpaquePointer? | ||
try prepareDatabase(database, forCommand: command, statementPointer: &selectStatement) | ||
for (index, key) in keys.enumerated() { | ||
sqlite3_bind_text(selectStatement, Int32(index + 1), strdup(key.rawValue), -1, SQLITE_TRANSIENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't dup.
Since I closed out #20 I'm closing out this related pull request. Thank you so much for the hard work, even though I won't be integrating this I still appreciate the effort. 🙇🏻♂️ |
Hello! Here's a PR that removes the dependency on SQLite.swift ( #17 ).
The SQLite C APIs are hilariously verbose, and I was aiming to keep this code as readable and maintainable as possible. This meant keeping all of the SQLite command strings in-line with their methods.
✅ All tests pass.
From the commit: