set up Drift database with local data models#48
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@M4dhav is this correct ? or it requires some changes ? |
…th upsert functions and removed wallet from the db
|
@M4dhav i removed the insert functions as from as well i added upsert which basically does the same thing is that ok? and i did other changes as well |
Mayank4352
left a comment
There was a problem hiding this comment.
Kindly make the requested changes, also if have any questions or suggestions feel free to comment
| const _uuid = Uuid(); | ||
|
|
||
| @DriftDatabase( | ||
| tables: [UsersTable, TransactionsTable, BalancesTable, GroupsTable], |
There was a problem hiding this comment.
Scope (issue #47) lists 5 tables it's missing WalletDetailsTable
There was a problem hiding this comment.
actually i discussed with the mentors and turns out we no longer need that table and i also modified the initial issue template
| tables: [UsersTable, TransactionsTable, BalancesTable, GroupsTable], | ||
| daos: [TransactionsDao, GroupsDao, UsersDao, BalancesDao], | ||
| ) | ||
| class AppDatabase extends _$AppDatabase { |
There was a problem hiding this comment.
SQLite doesn't enforce foreign keys by default, so the .references() in transactions and balances are currently non-functional
| class GroupsTable extends Table { | ||
| TextColumn get id => text().clientDefault(() => _uuid.v4())(); | ||
| TextColumn get name => text()(); | ||
| // storing member public keys as comma separated string |
There was a problem hiding this comment.
Storing member keys as a comma-separated string bypasses many of the benefits of a relational database. It removes foreign key enforcement (deleted users can leave dangling references)
| TextColumn get description => text().nullable()(); | ||
|
|
||
| @override | ||
| String get tableName => 'groups_table'; |
There was a problem hiding this comment.
Tiny consistency note: this one is 'groups_table' while the others are users, transactions, balances
|
|
||
| const _uuid = Uuid(); | ||
|
|
||
| class GroupsTable extends Table { |
There was a problem hiding this comment.
Maintain a reference for transaction table to access the transactions in groups, also using a simple FK here will not be a good approach since one group can have multiple transactions
| return into(balancesTable).insertOnConflictUpdate(balance); | ||
| } | ||
|
|
||
| Future<int> deletebalaces(String userPublicKey) { |
There was a problem hiding this comment.
there is a typo also use lower camel case
| return query.getSingleOrNull(); | ||
| } | ||
|
|
||
| Future<void> upsert(UsersTableCompanion user) { |
There was a problem hiding this comment.
upsert is already in lower camel case could you clarify what you'd like changed here
| return into(usersTable).insertOnConflictUpdate(user); | ||
| } | ||
|
|
||
| Future<int> deleteuser(String publicKey) { |
| TextColumn get fromUserPublicKey => | ||
| text().references(UsersTable, #publicKey)(); | ||
| TextColumn get toUserPublicKey => text().references(UsersTable, #publicKey)(); | ||
| IntColumn get amount => integer()(); |
There was a problem hiding this comment.
can use int64, keeping large crypto values in mind
|
@4555jan i have reviewed your pr all things pointed by mayank already, so just fix that and overall it's LGTM |
| return into(groupsTable).insertOnConflictUpdate(group); | ||
| } | ||
|
|
||
| Future<int> deletegroup(String id) { |
|
|
||
| class GroupsTable extends Table { | ||
| TextColumn get id => text().clientDefault(() => _uuid.v4())(); | ||
| TextColumn get name => text()(); |
There was a problem hiding this comment.
it's still just a string relabeled as set, so no dupe protection ("abc,def,abc" is allowed), no FK integrity (foreign keys can't reach inside a CSV, so the PRAGMA foreign_keys = ON doesn't protect membership), and "which groups is user X in?" are still unindexed and gets false-matches on substrings like (alice matches alice99).
Standard fix is a join table, one row per membership, but you can look for better solution as well
| ) | ||
| class AppDatabase extends _$AppDatabase { | ||
| AppDatabase() : super(_openConnection()); | ||
| AppDatabase.forTesting(QueryExecutor executor) : super(executor); |
There was a problem hiding this comment.
This can be AppDatabase.forTesting(super.executor); instead of AppDatabase.forTesting(QueryExecutor executor) : super(executor);
it'll raise analyser problem
… changed the db version cause of those changes
|
@M4dhav please review it does everything seems fine ? |
M4dhav
left a comment
There was a problem hiding this comment.
Make sure the generated files go into a separate folder to keep the working directory clean. Check @Mayank4352 's build.yaml in Resonate.
|
@M4dhav i did requested changes |
Addressed Issues:
Fixes #47
Screenshots/Recordings:
No UI changes in this PR — this is a database layer setup (Drift table definitions and AppDatabase class). No before/after screenshots applicable.
Additional Notes:
This PR sets up the Drift local database foundation for Zplit. It includes the AppDatabase class and five table definitions: UsersTable, GroupsTable, TransactionsTable, BalancesTable, and WalletDetailsTable. database initialization will be wired up in a follow-up PR.
The generated file app_database.g.dart is a build_runner output and is excluded from this PR.
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist