create database migration for the gateway #1768
Conversation
WalkthroughThe updates encompass enhancements to the wallet extension tool, focusing on database integration and migration management. Modifications include the addition of a Dockerfile setup for gateway and migration files, SQL script adjustments for database and user setup, creation of tables with constraints in MariaDB, and enabling foreign keys in SQLite. A new migration mechanism is introduced for applying SQL migrations, alongside refactoring of database-related code to accommodate specific database implementations (MariaDB and SQLite), streamlining the process of database schema management. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- .github/workflows/manual-deploy-obscuro-gateway-database.yml (1 hunks)
- tools/walletextension/storage/database/mariadb/001_init.sql (1 hunks)
- tools/walletextension/storage/database/mariadb/mariadb.go (2 hunks)
- tools/walletextension/storage/database/migration.go (1 hunks)
- tools/walletextension/storage/database/sqlite/001_init.sql (1 hunks)
- tools/walletextension/storage/database/sqlite/sqlite.go (7 hunks)
- tools/walletextension/storage/storage.go (2 hunks)
Additional comments: 12
tools/walletextension/storage/database/sqlite/001_init.sql (3)
- 1-2: Enabling foreign keys is a good practice for relational integrity. However, ensure that all related operations in your application are designed to handle cascading effects properly.
- 5-8: The
userstable definition looks correct. Usingbinary(20)foruser_idandbinary(32)forprivate_keyis appropriate for fixed-size data, ensuring efficient storage.- 11-15: The
accountstable correctly establishes a foreign key relationship with theuserstable, which is good for maintaining data integrity. Ensure that theON DELETE CASCADEoption aligns with your application's data management policies.tools/walletextension/storage/database/mariadb/001_init.sql (4)
- 1-3: The comment provides clear context for the purpose of this migration file. It's good practice to include such comments for maintainability.
- 6-6: Granting specific permissions to a user is a secure practice. However, ensure that the 'obscurouser' has the minimum necessary permissions for its role to adhere to the principle of least privilege.
- 11-15: The
userstable definition in MariaDB usesvarbinaryforuser_idandprivate_key, which is consistent with the SQLite version. This consistency is good for maintaining similar data types across different database systems.- 17-23: The
accountstable structure is consistent with the SQLite version, including the foreign key relationship. This consistency is beneficial for cross-database compatibility.tools/walletextension/storage/storage.go (2)
- 6-7: The imports have been correctly updated to reflect the new package structure for
mariadbandsqlite. This change aligns with the PR's objective to use specific database implementations.- 24-26: The
Newfunction has been updated to use the new constructorsmariadb.NewMariaDBandsqlite.NewSqliteDatabase. This change correctly reflects the updated package structure and function signatures.tools/walletextension/storage/database/mariadb/mariadb.go (1)
- 26-36: The logic to determine the migrations directory and apply migrations is correctly implemented. However, ensure that the migrations directory path is correctly resolved in all deployment environments.
tools/walletextension/storage/database/sqlite/sqlite.go (1)
- 36-44: The approach to apply migrations in the SQLite database implementation is consistent with the MariaDB implementation. This consistency is good, but again, ensure the migrations directory path is correctly resolved across environments.
.github/workflows/manual-deploy-obscuro-gateway-database.yml (1)
- 109-114: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of the volume mount for the
001_init.sqlfile in the GitHub Actions workflow aligns with the PR's objective to manage database migrations through application startup rather than at the database container initialization. This change supports the new migration strategy.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- tools/walletextension/storage/database/mariadb/mariadb.go (2 hunks)
- tools/walletextension/storage/database/migration.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- tools/walletextension/storage/database/mariadb/mariadb.go
- tools/walletextension/storage/database/migration.go
zkokelj
force-pushed
the
ziga/gateway_database_migration
branch
from
770e701 to
5f03dc0
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/walletextension/Dockerfile (1 hunks)
Additional comments: 2
tools/walletextension/Dockerfile (2)
- 58-58: The command to copy the gateway executable is correctly placed and uses the
--from=build-walletdirective to ensure that the executable is copied from the appropriate build stage. This is a good practice as it ensures that only the necessary files are included in the final image, keeping it lightweight.- 62-62: Copying the
.sqlmigration files from thebuild-walletstage to the final image is essential for applying database migrations at the gateway's startup. This change is aligned with the PR's objectives. However, ensure that the directory structure in the final image (/home/obscuro/go-obscuro/tools/walletextension/storage/database) is correctly set up to be accessed by the application, and that permissions issues are considered, especially if the application runs as a non-root user.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/walletextension/storage/database/migration.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tools/walletextension/storage/database/migration.go
zkokelj
force-pushed
the
ziga/gateway_database_migration
branch
from
8ee83c4 to
3e9ddc0
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (8)
- tools/walletextension/Dockerfile (1 hunks)
- tools/walletextension/storage/database/001_init.sql (1 hunks)
- tools/walletextension/storage/database/mariadb/001_init.sql (1 hunks)
- tools/walletextension/storage/database/mariadb/mariadb.go (2 hunks)
- tools/walletextension/storage/database/migration.go (1 hunks)
- tools/walletextension/storage/database/sqlite/001_init.sql (1 hunks)
- tools/walletextension/storage/database/sqlite/sqlite.go (7 hunks)
- tools/walletextension/storage/storage.go (2 hunks)
Files skipped from review as they are similar to previous changes (7)
- tools/walletextension/Dockerfile
- tools/walletextension/storage/database/mariadb/001_init.sql
- tools/walletextension/storage/database/mariadb/mariadb.go
- tools/walletextension/storage/database/migration.go
- tools/walletextension/storage/database/sqlite/001_init.sql
- tools/walletextension/storage/database/sqlite/sqlite.go
- tools/walletextension/storage/storage.go
Additional comments: 3
tools/walletextension/storage/database/001_init.sql (3)
- 1-3: The addition of a comment block explaining the purpose of the SQL script is a good practice. It enhances the maintainability and readability of the code by providing context to future developers or maintainers.
- 5-5: The
CREATE DATABASE ogdb;command is straightforward and correctly formatted. However, it's important to ensure that this script is executed in a controlled environment, as it assumes the databaseogdbdoes not already exist. Attempting to create a database that already exists without checking can lead to errors.Consider adding a conditional check to ensure the database does not already exist or handling the potential error in a way that does not disrupt the migration process.
- 5-5: Using
USE ogdb;immediately after creating the database is correct for setting the context for subsequent operations within the same script. This ensures that the permissions are granted to the newly created database.
|
|
||
| CREATE DATABASE ogdb; |
There was a problem hiding this comment.
The GRANT SELECT, INSERT, UPDATE, DELETE ON ogdb.* TO 'obscurouser'; command correctly assigns the necessary permissions for the specified user on the ogdb database. However, it's crucial to ensure that 'obscurouser' is a placeholder or a generic username for demonstration purposes. In production environments, using more descriptive and secure usernames is recommended to avoid potential security risks.
Consider replacing 'obscurouser' with a more descriptive and secure username, especially if this script is intended for use in production environments.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- tools/walletextension/storage/database/001_init.sql (1 hunks)
- tools/walletextension/storage/database/mariadb/001_init.sql (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- tools/walletextension/storage/database/001_init.sql
- tools/walletextension/storage/database/mariadb/001_init.sql
zkokelj
force-pushed
the
ziga/gateway_database_migration
branch
from
63d544f to
3c4fa0b
Compare
zkokelj
force-pushed
the
ziga/gateway_database_migration
branch
from
a84f003 to
4e632a9
Compare
zkokelj
force-pushed
the
ziga/gateway_database_migration
branch
6 times, most recently
from
a4e611d to
693721d
Compare
zkokelj
force-pushed
the
ziga/gateway_database_migration
branch
from
245ffc4 to
90073c8
Compare
Why this change is needed
In the next task I need to add additional tables to the database.
Currently the database logic for creating desired schema for MariaDB is in github action which restarts the database. Restarting will cause us to lose all stored userIDs and viewingkeys.
In order to prevent that I created this PR which takes the logic from the database and runs
.sqlmigration files on gateway start.What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks