-
Notifications
You must be signed in to change notification settings - Fork 61
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
feature: Add function to set journal mode on sqlite databases #443
Conversation
✅ Deploy Preview for apollo-ios-docc canceled.
|
✅ Deploy Preview for eclectic-pie-88a2ba canceled.
|
} | ||
|
||
public extension SQLiteDatabase { | ||
func setJournalMode<T>(mode: T) throws where T : RawRepresentable, T.RawValue == String { } |
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 extension ensures it's not a breaking change for projects that have written their own SQLiteDatabase
implementation.
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 LGTM! Nice work!
public enum JournalMode: String { | ||
/// The rollback journal is deleted at the conclusion of each transaction. This is the default behaviour. | ||
case delete = "DELETE" | ||
/// Commits transactions by truncating the rollback journal to zero-length instead of deleting it. | ||
case truncate = "TRUNCATE" | ||
/// Prevents the rollback journal from being deleted at the end of each transaction. Instead, the header | ||
/// of the journal is overwritten with zeros. | ||
case persist = "PERSIST" | ||
/// Stores the rollback journal in volatile RAM. This saves disk I/O but at the expense of database | ||
/// safety and integrity. | ||
case memory = "MEMORY" | ||
/// Uses a write-ahead log instead of a rollback journal to implement transactions. The WAL journaling | ||
/// mode is persistent; after being set it stays in effect across multiple database connections and after | ||
/// closing and reopening the database. | ||
case wal = "WAL" | ||
/// Disables the rollback journal completely | ||
case off = "OFF" | ||
} |
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.
Is it possible some other SQLite implementation would have different journal modes? Are these modes intrinsic to SQLite or just to the SQLite.swift
implementation?
If the are universally the journal modes of SQLite, then I don't think this should be part of our specific implementation. The setJournalMode
function in the protocol could just take in this enum instead of being generic.
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.
I suppose there might be but these come directly from sqlite.org documentation so if there is an sqlite implementation that differs enough you'll probably want to write your own SQLiteDatabase
implementation too at which point you get to define the new set of journal modes.
As for the generic parameter I simply wanted something that was able to be expressible as a String
since that's how the pragma statement gets built. Moving the enum into SQLiteDatabase
will serve the vast majority of users although I didn't like how protocols can't hold enums so we lost some namespacing there.
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.
Refactored as we discussed in 27cc399.
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.
LGTM!
70c34472 feature: Add function to set journal mode on sqlite databases (#443) 5f529536 Update ROADMAP.md git-subtree-dir: apollo-ios git-subtree-split: 70c3447284b9fdeba7bbc2292644c494243ad0fb
…de on sqlite databases git-subtree-dir: apollo-ios git-subtree-mainline: 90cabb5 git-subtree-split: 70c3447284b9fdeba7bbc2292644c494243ad0fb
Closes apollographql/apollo-ios#3399.
There are many other
PRAGMA
commands that sqlite supports so there were a few ways this could have been implemented:execute
orrun
functions public onSQLiteDatabase
but this is potentially the most destructive as it's a wide open door to executing anything against the connected database.setPragma
function that would execute thePRAGMA <statement>
command. This narrows the scope to onlyPRAGMA
statements but this is also potentially destructive as I am not familiar with all the pragma statements and their effects.setJournalMode
function that only allows the journal mode to be set. This is very narrow in scope but the safest since it solves the requested feature and it's a single statement to grok.I chose option 3, so this PR adds a protocol function to allow sqlite database implementations to set the journal mode of the connected database.
JournalMode
pragma values were taken from https://www.sqlite.org/pragma.html#pragma_journal_mode.