refactor(databases): Creation of the databases folder to keep the logic for sqlite and postgres #1811
Conversation
| let execRes = db.query(statement, NoopRowHandler) | ||
| if execRes.isErr(): | ||
| error "failed to execute migration statement", statement=statement, error=execRes.error | ||
| return err("failed to execute migration statement") |
There was a problem hiding this comment.
Question: what happen if it fail in the middle of a migration does it need to be rolled back or no?
There was a problem hiding this comment.
Good point. We are assuming that either all the query works or all the query fails but we are not considering the case where the query partially worked in which case we should make sure the database is rolled back to the initial state. However, I'd consider that optimization for further PRs. I'd like to keep this PR with minimal changes, i.e. only rename files or delete the if statement that you suggested :)
There was a problem hiding this comment.
Btw, I just said "just rename files" but precisely at this point, I'm adding content from "waku/common/sqlite/migrations.nim", which I removed. If you are not happy with that I can restore back the 'migrations.nim'.
There was a problem hiding this comment.
Totally fine to not consider this edge case for now. We just need to not forget about it.
…pool.nim & dbconn.nim
…eady started pg tests in another file
There was a problem hiding this comment.
Refactor seems reasonable to me, though my preference is for comment underneath to be addressed before merging - i.e. creating barrel imports for db_sqlite and db_postgresql which exports a common module in order to have explicit Database error handling (in this case an explicit DatabaseResult).
| @@ -23,7 +22,7 @@ template projectRoot: string = currentSourcePath.rsplit(DirSep, 1)[0] / ".." / " | |||
| const PeerStoreMigrationPath: string = projectRoot / "migrations" / "peer_store" | |||
|
|
|||
|
|
|||
| proc migrate*(db: SqliteDatabase, targetVersion = SchemaVersion): DatabaseResult[void] = | |||
| proc migrate*(db: SqliteDatabase, targetVersion = SchemaVersion): Result[void, string] = | |||
There was a problem hiding this comment.
(here and elsewhere)
I think we should keep the explicit DatabaseResult. One way to do it is to define this in a new db_common module (under databases) and then create barrel imports for both sqlite and postgres that imports and exports both the underlying db modules (i.e. either db_sqlite or db_postgres modules) and the new common module. That way modules making use of a specific database would only import db_sqlite or db_postgresql, but get access to the common symbols as well (which includes DatabaseResult.
Ivansete-status
force-pushed
the
refactor-databases
branch
from
b175b91
to
1346641
Compare
…ic for sqlite and postgres (#1811) * Refactoring in sqlite and postgres. Creation of the databases folder.
Description
Simple refactoring to move all the database logic in one place (common/databases)