-
-
Notifications
You must be signed in to change notification settings - Fork 727
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
Allow Database Connection Configuration #508
Allow Database Connection Configuration #508
Conversation
bb2513f
to
c1206b1
Compare
Hello @michaelkirk-signal, Thanks for this PR! I have just closed #481, which was stuck. I may have to ship GRDB 4 with CocoaPods-only support for SQLCipher. And that's just OK: it can be improved in the future. Before we move forward, May I ask you for your opinion on #497? I think @Marus had a lot of good ideas there. And I also provided some guidance in #497 (comment). We'd better synchronize our efforts. |
Thanks for the link - I didn't realize that the custom cipher params were so recently added. Agreed that we should synchronize!
I think that's directionally correct. We can either (1) specify the old defaults or (2) convert the database to use the new defaults. As for (2) converting the the database to the new defaults, I'd recommend an approach based on In terms of (1) how to specify the old defaults, as @Marus mentioned in #497, we'd need to specify a couple of other parameters. This can get a bit hairy:
Alternatively, what this PR leverages, you can call: SQLCipher 3
SQLCipher 2
SQLCipher 1
So in Swift, that's something like:
vs.
Even if you do choose to merge this cipher_compatibility feature, note that I kept the work @Marus did in case users want to fine tune their cipher parameters. I did juggle things around a bit so that it's intentionally not possible to specify both cipher compatibility mode and custom cipher params. In my opinion the user will almost always want one or the other, so hopefully this helps them make the right decision. As for the upgrade documentation, once there's a "Migrating to GRDB4" doc, I'd recommend including something like: GRDB4 Migration Guide[ other stuff here] SQLCipher 4GRDB now supports SQLCipher 4. If you have existing databases, there are some special considerations. In all cases, the configuration syntax to use SQLCipher has changed slightly. Previously, to use the default cipher configuration, you might have something like this:
Now that would be written like this:
Dealing with existing SQLCipher 3 databases SQLCipher 4 introduced some changes to their default cipher configurations. This means that a database created with SQLCipher 3 defaults will not be readable using SQLCipher 4 unless you explicitly tell it to use the SQLCipher 3 defaults:
If you were using non-default params, you'll need to explicitly specify them like this:
|
Hi again @michaelkirk-signal, Thanks for joining the discussion! (In all I'll be writing below, remember that I'm no SQLCipher expert, and not even an SQLCipher user. I just ship a library that is supposed to provide the necessary tools for people who are actual experts or users of SQLCipher. I mean that I may be wrong sometimes due to my lack of experience.) First, I'm happy that we all agree about the three main use cases!
I'm glad you have mentioned SQLCipher 1 and 2, even if GRDB has never explicitly supported them, never documented anything about them, and no user has ever asked about them. I suppose the existing support for raw SQL pragmas have provided the necessary support. Generally speaking, we don't have to provide high-level APIs when existing low-level ones are sufficient. This is especially true for our two last use cases: if a user asks Google, Stack Overflow, or the SQLCipher forums how to deal with his SQLCipher 3 database, he will be told to set a few options and run a few pragmas. Now he just expects GRDB to allow exactly that. He won't look for a high-level API that may, or not, apply the recipe he wants to run. I admit I have somewhat over-simplified things in the last two paragraphs: @Marus has shown in #497 that database pools need special care. They create several database connections, and each one of them need to be configured before it can access an SQLCipher database that is not configured with default options. @Marus has exposed some necessary pragmas as But please follow me in your comment:
Well, is it really hairy? If you're an expert, you know what it means. And if you're not an expert, this just means that you are applying some recipe found somewhere on the internet. In both cases, you are glad and relieved when you find those options. I have turned very confident about this way of thinking about expert options since #498, an eye-opener for me. A GRDB API which was "swifty" and well typed... completely missed its target because it did not look like the raw option the user was looking for. Let's keep on:
Yes, I see your public enum Parameters {
case defaultParameters
case compatibility(version: UInt)
case custom(cipherPageSize: Int?, kdfIteration: Int?)
} I totally understand what you want to do, and what you want to prevent. Yes, the compatibility pragmas don't mix well with individual options. This means your PR is correct. This is the best quality a PR can aim for! 👍 But I also worry we are missing the point 😓 Why? Because it is difficult for experts to find their way in a complex hierarchy of GRDB types and values that more or less look like their raw SQLCipher counterpart. And non-expert users will suffer a double punishment: first they'll fight Google in order to find a solution for their problem, and then they'll fight the GRDB API and documentation until it does what they hope it should do. What about the following:
This is a somewhat drastic change. I hope it doesn't sound too bold, and above all that it allows what we want to do! |
Thanks for thoughtful followup @groue. I think the approach you've outlined sounds great! Let me know how I can help. |
I'm glad I could make sense, @michaelkirk-signal :-) I'm looking right now at the detailed database opening code. It is currently in two steps:
And because those pragmas run before the Database Swift object is fully initialized, @Marus could not use regular Only later happens further setup and configuration such as format validation, trace, foreign keys, busy mode, observation setup, and generally everything GRDB users take for granted. Consequence: it looks like my suggestion, the one which lets the user give a Does this look like something you would like to do? Tests added by #497 will be a perfect validation for this refactoring. |
But... you'll need SQLCipher 4, and #481 has been discarded. How did you use SQLCipher 4 in this PR? |
The answer is in the SQLCipher-4 branch. Any SQLCipher-related PR should be submitted against this branch, which will eventually be merged in GRDB-4. The SQLCipher-4 branch is at a very preliminary phase. I did the bare minimum until you could run the GRDBCipher tests for OSX and iOS. A few tests fail, you can ignore them. I lack the Xcode skills that would let me keep on the work started in #481, so I have integrated SQLCipher as a CocoaPods. After you clone this branch, run |
c1206b1
to
3ea41d5
Compare
3ea41d5
to
99065a4
Compare
I've updated my PR (force-pushed) in an attempt to reflect this approach. If it looks directionally correct, I'll follow up with some documentation updates. The tests are passing and this works for my cipher_compatibility needs.
I simply bumped the Podspec SQLCipher constraint and was able to build against SQLCipher4. I'll have a look at rebasing on the SQLCipher-4 branch you've set up. |
No, no, don't rebase. I rushed this message. As a matter of fact, your PR is now just OK with SQLCipher 3 too! Our new approach has removed any dependency on the SQLCipher version :-)
This looks great. I'll have a detailed look and answer your questions shortly 👍 |
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.
Thank you very much @michaelkirk-signal! Please look at the few requests for changes, and we'll be good to go!
…later, convert static->instance methods where possible
This reverts commit 26b8682.
OK @groue, thanks for the feedback so far. I've taken another pass at this. Can you please take another look? |
👍 Thanks @michaelkirk-signal ! I'll look at your changes on the beginning of the next week. |
Because it willl be easy to forget the context of groue#508 when we read this code in the future
They were failing because database initialization now uses high level methods which throw detailed errors.
…ility # Conflicts: # CHANGELOG.md
Thank you very much @michaelkirk-signal, the PR is now ready, and I'm very happy of it 👏 Let's wait for Travis tests, and we'll merge this ASAP. Welcome to the GRDB contributors :-) |
Merged 🎉 |
@michaelkirk I forgot to propose you a push access to the repository. Just drop me a line if you are interested! |
SQLCipher4.0.1 added a new pragma
cipher_compatibility
which can be helpful for folks upgrading from an older version of SQLCipher.I'm aware that SQLCipher4 support is somewhat in-motion (#481) so it might not make sense to consider this PR yet, but I wanted to get it on your radar since it would be a breaking change, so it might make more sense to merge before the GRDB4 release.
from: https://www.zetetic.net/sqlcipher/sqlcipher-api/#cipher_compatibility
This is an easier/safer way to set pageSize/kdfIterations in a way consistent with previous major versions.
Because it doesn't make sense to use
cipher_compatibility
while also specifying custompageSize/kdfIterations
I've reworked the Configuration process to ensure that only one or the other can be specified, pulling out a CipherConfiguration struct in the process.Pull Request Checklist
development
branch.As this is a breaking change, I targeted the GRDB-4.0 branch, but let me know if you'd prefer otherwise.
SQLCipher4 tests aren't yet passing - I believe that's part of #481. If there's something I can do to help there, LMK. /cc @darrenclark