Use Drift and SQLite for data storage #22
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I've rebased this a couple of times as the related work has progressed, and made some further revisions earlier this week. Just pushed the current version. This still needs end-to-end testing before I'd want to merge it. That's a lot closer now that #11 is done, and #55. The end-to-end testing will basically consist of implementing #34, storing login credentials. |
Just the other day we were talking about this get-server-settings property, and I explained why it's important for a mobile client to use it: https://chat.zulip.org/#narrow/stream/378-api-design/topic/uri.20to.20url.20compatibility/near/1549242 But apparently when I wrote this code a couple of weeks earlier, I'd forgotten about that, oops. Fix it. It's particularly useful that we fix this before we start keeping the list of accounts in a database, so that there's no legacy of some people's devices having a non-canonicalized URL lying around.
This removes the one place where we really use the fact that we've had Account as a subtype of Auth. We'll be giving up that subtyping as part of getting Account from a database ORM.
This frees us up to switch to an Account type that's generated to correspond to a database table.
This will let LiveGlobalStore and TestGlobalStore diverge in how the data is ultimately stored and how the account IDs are assigned, while sharing the code to maintain the in-Dart cache.
…_dev These are the dependencies called for by the Drift docs: https://drift.simonbinder.eu/docs/getting-started/#adding-the-dependency
Also add a .gitattributes file, excluding `*.g.dart` files from diffs. We didn't really need this when the only such files were the JSON serialization/deserialization files in the API bindings, because those are pretty compact; but this generated ORM code is much longer and makes it more necessary.
This will keep things simple when we start storing these in the database.
gnprice
changed the title
|
OK! This is now tested end to end, and I think it's complete. Please take a look. |
chrisbobbe
approved these changes
Apr 24, 2023
Comment on lines
+25
to
+29
| @override | ||
| List<Set<Column<Object>>> get uniqueKeys => [ | ||
| {realmUrl, userId}, | ||
| {realmUrl, email}, | ||
| ]; |
There was a problem hiding this comment.
Could add a TODO in lib/widgets/login.dart to handle exceptions thrown because of this, or maybe check GlobalStore.accountIds before sending a bad insert to the database.
There was a problem hiding this comment.
Good thought. I added one TODO(log) to log database errors (which we should do in a general way down in the Drift layer), and also a TODO(#35) in lib/widgets/login.dart to give the user feedback if the account is a duplicate.
We didn't really need to do this as a migration, because we already know we need this column and so we could have included it from the start. But this lets us exercise Drift's support for migrations, and for testing migrations.
|
Thanks for the review! Done. |
gnprice
added a commit
to gnprice/zulip-flutter
that referenced
this pull request
May 30, 2023
This happens automatically if you rerun build_runner. Seems like its omission was basically a mismerge between 901a99e, when it was merged early from zulip#84: zulip#84 (comment) and 780b092 / zulip#22, which added this file. Chalk it up as another occasion where zulip#60 would have helped keep things clean.
gnprice
added a commit
to gnprice/zulip-flutter
that referenced
this pull request
May 31, 2023
This happens automatically if you rerun build_runner. Seems like its omission was basically a mismerge between 901a99e, when it was merged early from zulip#84: zulip#84 (comment) and 780b092 / zulip#22, which added this file. Chalk it up as another occasion where zulip#60 would have helped keep things clean.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #13.
Fixes #34.
originally:
Toward #13.
This installs Drift as a dependency, sets up a schema for an accounts table, and performs a migration on that table. It also adds tests for the database schema and the migration, to let us see how those look.
I won't want to merge something like this until I've tested it more end-to-end, which will require some other work including #20 and #21. But I think the branch is otherwise ready, apart from a couple of polish issues.