Apply automatic migrations #128
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a new Fastify plugin for managing TypeORM integration and PostgreSQL migrations with SSL support, enhanced error handling, and migration logging. The existing analytics ORM plugin and related API routes are refactored to use a namespaced ORM context ( Changes
Sequence Diagram(s)sequenceDiagram
participant Fastify
participant ORMPlugin
participant TypeORM
participant MigrationDir
Fastify->>ORMPlugin: Register plugin (orm-repositories)
ORMPlugin->>TypeORM: Configure connection (with SSL, migrations)
ORMPlugin->>TypeORM: Run pending migrations
TypeORM-->>ORMPlugin: Migration results/errors
ORMPlugin->>MigrationDir: Read migration filenames
MigrationDir-->>ORMPlugin: Return filenames/errors
ORMPlugin->>Fastify: Log migration status and errors
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
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. 🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces automatic migration execution on API startup to simplify database updates across multiple repositories. Key changes include refactoring the Postgres ORM configuration to support migrations, updating API routes to use a dedicated “analytics” ORM namespace, and adding new Fastify plugins to register and run migrations.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| libs/repositories/src/datasources/orm/postgresOrm.ts | Refactored ORM configuration by separating database parameters and defining migration settings. |
| apps/api/src/app/routes/__chainId/yield/getPoolsInfo.ts | Updated repository reference to use the analytics ORM namespace and adjusted formatting. |
| apps/api/src/app/routes/__chainId/yield/getPoolsAverageApr.ts | Similar update to the repository source with minor formatting changes in the SQL query and header setup. |
| apps/api/src/app/plugins/orm-repositories.ts | Added a new plugin to register the repositories ORM, including migration execution and logging. |
| apps/api/src/app/plugins/orm-analytics.ts | Introduced a new plugin for the analytics ORM that registers entities and runs migrations on startup. |
Comments suppressed due to low confidence (4)
libs/repositories/src/datasources/orm/postgresOrm.ts:4
- The removal of the 'IndexerState' entity import may cause issues if it is expected to be part of this ORM instance. Verify that this entity is no longer needed or has been migrated to the appropriate ORM configuration.
-import { IndexerState } from '../../database/IndexerState.entity';
apps/api/src/app/routes/__chainId/yield/getPoolsInfo.ts:40
- Confirm that referencing PoolInfo from the 'analytics' namespace is intentional and that the entity is registered under this ORM instance to prevent potential runtime errors.
const poolInfoRepository = fastify.orm.analytics.getRepository(PoolInfo);
apps/api/src/app/routes/__chainId/yield/getPoolsAverageApr.ts:44
- Ensure that using the 'analytics' namespace for PoolInfo queries aligns with your overall database strategy and that the entity mapping is correct to avoid data mismatches.
const poolInfoRepository = fastify.orm.analytics.getRepository(PoolInfo);
apps/api/src/app/plugins/orm-analytics.ts:45
- [nitpick] Verify that running migrations automatically in the 'analytics' namespace during Fastify's boot does not lead to race conditions or conflicts in a multi-instance deployment scenario.
fastify.orm.analytics.runMigrations({ transaction: 'all' });
|
|
||
| fastify.register(typeORMPlugin, { | ||
| ...dbParams, | ||
| namespace: 'analytics', |
There was a problem hiding this comment.
namespacing helps to differenciate between the different typeorm instances. This one is for the analytics.
| const migrationsDir = resolve( | ||
| __dirname, | ||
| '../../../../../libs/repositories/src/migrations' | ||
| ); |
There was a problem hiding this comment.
It took me a bit to arrive here, when we run the API using nx, it depends on the build. This PR adds a build step for the repositories too. This way, we can find the migrations and use them.
| migrationsTableName: 'migrations_repositories', | ||
| entities: [], | ||
| ssl: isProduction, | ||
| extra: isProduction |
There was a problem hiding this comment.
use ssl only in prod
| import { IndexerState } from '../../database/IndexerState.entity'; | ||
|
|
||
| export function createNewPostgresOrm(): DataSource { | ||
| export function getDatabaseParams() { |
There was a problem hiding this comment.
just convenient to share the credentials to be used in the typeorm API config
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/api/src/app/plugins/orm-repositories.ts (2)
14-17: Consider making the migrations path configurable.The migrations directory path is hardcoded with multiple "../" references to navigate up the directory tree, which could be fragile if the directory structure changes in the future.
+import { config } from '@cowprotocol/config'; // Hypothetical config module - const migrationsDir = resolve( - __dirname, - '../../../../../libs/repositories/src/migrations' - ); + const migrationsDir = config.migrations?.repositoriesPath || resolve( + __dirname, + '../../../../../libs/repositories/src/migrations' + );
1-68: Overall well-implemented automatic migration plugin.This plugin successfully achieves the objective of automatically applying database migrations during API startup. The implementation is robust with proper error handling, logging, and transaction support. It aligns with the PR's goal of streamlining migration management and reducing manual intervention.
A few additional considerations:
- You might want to add a check to skip migrations in certain environments if needed.
- Consider adding more detailed logging for each migration being applied.
- Think about how this would work if multiple API instances start simultaneously.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/src/app/plugins/orm-analytics.ts(1 hunks)apps/api/src/app/plugins/orm-repositories.ts(1 hunks)apps/api/src/app/routes/__chainId/yield/getPoolsAverageApr.ts(2 hunks)apps/api/src/app/routes/__chainId/yield/getPoolsInfo.ts(2 hunks)libs/repositories/src/datasources/orm/postgresOrm.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/api/src/app/plugins/orm-repositories.ts (2)
libs/repositories/src/datasources/orm/postgresOrm.ts (1)
getDatabaseParams(6-21)libs/shared/src/logger.ts (1)
logger(3-3)
apps/api/src/app/routes/__chainId/yield/getPoolsAverageApr.ts (2)
apps/api/src/app/routes/__chainId/yield/utils.ts (1)
trimDoubleQuotes(1-11)apps/api/src/utils/cache.ts (2)
CACHE_CONTROL_HEADER(3-3)getCacheControlHeaderValue(66-71)
🔇 Additional comments (13)
libs/repositories/src/datasources/orm/postgresOrm.ts (2)
6-21: Well-structured extraction of database parameters!This is a good refactoring that follows the DRY principle by centralizing database parameter extraction into a dedicated function. This will make the code more maintainable and facilitate reuse of these parameters across different ORM contexts, which is helpful for the automatic migration functionality.
23-29: Good refactoring of DataSource creationUsing the extracted
getDatabaseParams()function with object spread simplifies the DataSource creation and improves maintainability. The function now clearly focuses on its primary responsibility of creating the ORM instance.apps/api/src/app/routes/__chainId/yield/getPoolsAverageApr.ts (2)
44-44: Appropriate update to use analytics namespaceGood update to use the new analytics namespace for the ORM. This change properly aligns with the architectural decision to separate different ORM contexts and will support the automatic migration functionality.
46-68: Good code formatting improvementsThe reformatting of the SQL query, reduce function, and cache header setting improves code readability and maintainability without changing functionality. These changes make the code easier to understand and follow established formatting conventions.
apps/api/src/app/plugins/orm-analytics.ts (2)
27-30: Excellent addition of analytics namespaceAdding the
namespace: 'analytics'property to the typeORMPlugin registration is a key architectural improvement that allows for separate ORM instances. This is crucial for the automatic migration functionality and helps avoid race conditions when multiple repositories or applications access the same database.
45-45: Correctly updated migration call to use analytics namespaceGood update to use the analytics namespace for running migrations. This ensures migrations are executed in the correct ORM context and aligns with the automatic migration functionality being implemented.
apps/api/src/app/routes/__chainId/yield/getPoolsInfo.ts (2)
40-40: Consistent use of analytics namespaceGood update to use the new analytics namespace for repository access. This change maintains consistency with the architectural decision to separate ORM contexts and will support the automatic migration functionality.
5-58: Good code formatting improvements throughoutThe reformatting of imports, query parameters, and result mapping improves code readability and maintainability. These changes establish consistent formatting patterns across the codebase, making it easier to understand and modify in the future.
apps/api/src/app/plugins/orm-repositories.ts (5)
1-10: Clean and well-structured imports.The imports section properly organizes the required dependencies for TypeORM integration and Fastify plugin development. The use of selective imports from shared libraries follows good practices.
19-35: Good database connection configuration with environment awareness.The TypeORM configuration is well-structured with:
- Appropriate namespace for the ORM connection
- Properly configured SSL settings based on environment
- Clean separation of database parameters using the shared
getDatabaseParams()function- Use of empty entities array, which is appropriate if this plugin only handles migrations
The configuration will properly connect to PostgreSQL in both development and production environments.
36-53: Well-implemented migration execution with proper error handling.The migration execution is correctly placed in the
fastify.ready()hook to ensure it runs after the Fastify app is properly initialized. The use oftransaction: 'all'ensures all migrations run atomically, which is a good practice. Error handling is comprehensive with proper logging and error propagation.
44-44: Ensure migration printing doesn't block API startup.The
printMigrationsfunction is called withoutawait, which means the API continues startup without waiting for the directory reading to complete. This is likely intentional for performance reasons but could lead to interleaved log messages.If you want to ensure migration list is printed before running migrations, consider adding
await:- printMigrations(migrationsDir); + await printMigrations(migrationsDir);
56-67: Good utility function for migration visibility.The
printMigrationsfunction is well-implemented with proper filtering for JavaScript files and error handling. This will help with debugging and monitoring of available migrations.
Automatically apply the repository migrations.
background on this PR
I was not planning to do this PR, as I just need to create a single table, and I was fine with applying the migrations to a remote db, but I don't have write permissions to production or staging
There might be many repositories using the DB, and can also be used in different apps. If I try to apply migrations in all apps boot, there could be a race condition. My first approach was using docker and apply the migration when we deploy the services, but later I realised there was a simple way by using the API.
The API is deployed each time there's a new BFF version, so now in its boot will apply any un-applied migration.
It was also a bit of not a straight line to make it work. I will try to merge this PR and test this in AWS to see if it works well there.
Changes
Namespaces for different ORM
The API had an orm of
typeormAutomatically apply migrations of the repositories
The API defines a new plugin that applies the migrations.
EDIT: It works 🎉
I merged to test it in AWS, and it applied the migration!