refactor: moves sequelize creation into DI container factory#2636
refactor: moves sequelize creation into DI container factory#2636
Conversation
📝 WalkthroughWalkthroughReplace module-scoped Sequelize with per-container DI: add CHAIN_DB/USER_DB InjectionTokens and Sequelize factories, register APP_INITIALIZER to connect DBs, update services/repositories/tests to inject or resolve CHAIN_DB, and expose initDb() for test bootstrap. No query logic changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Container as DI Container
participant SeqProv as Sequelize Provider
participant CHAIN_DB as CHAIN_DB Token
participant Service as Service / Repository
participant DB as Postgres DB
App->>Container: bootstrap (register providers & APP_INITIALIZER)
Container->>SeqProv: register CHAIN_DB / USER_DB factories
App->>Service: instantiate (constructor has `@inject`(CHAIN_DB))
Service->>Container: resolve(CHAIN_DB)
Container->>SeqProv: execute CHAIN_DB factory
SeqProv->>DB: create & authenticate Sequelize instance
DB-->>SeqProv: Sequelize instance
SeqProv-->>Container: return Sequelize instance
Container-->>Service: inject Sequelize instance
Service->>Service: store private `#chainDb`
Service->>DB: this.#chainDb.query(...)
DB-->>Service: query result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/api/src/app/console.ts`:
- Line 81: The CLI startup removed the DB authentication so commands fail;
restore DB auth by calling chainDb.authenticate() before running migratePG() and
other initializers or add an APP_INITIALIZER that calls chainDb.authenticate()
on startup. Concretely, ensure chainDb.authenticate() is invoked (or wrapped in
a promise-returning initializer registered with APP_INITIALIZER that implements
ON_APP_START) so that database authentication completes before migratePG() and
container.resolveAll(APP_INITIALIZER).map(initializer =>
initializer[ON_APP_START]()) run; reference the chainDb.authenticate() call or
create an initializer service that performs chainDb.authenticate() to guarantee
the DB is connected prior to CLI execution.
In `@apps/api/src/dashboard/services/stats/stats.service.spec.ts`:
- Around line 4-7: The test is resolving a real chainDb from the DI container
(CHAIN_DB) instead of using a mock, which can cause side effects; replace the
container.resolve(CHAIN_DB) usage in stats.service.spec.ts with a
jest-mock-extended mock (e.g., mockDeep<Type>() or mock<...>()) named
mockChainDb, and inject that mock into the StatsService under test the same way
cosmosHttpService and coinGeckoHttpService are mocked and passed in—ensure you
import mock/mocDeep from jest-mock-extended, create mockChainDb, and provide it
to the service constructor or test DI registration instead of resolving CHAIN_DB
from tsyringe.
In `@apps/api/src/provider/repositories/provider/provider.repository.spec.ts`:
- Around line 276-277: The height fallback in the createProvider helper
incorrectly uses || which treats createdHeight = 0 as falsy; update the
expression in createProvider so it uses the nullish coalescing operator (??) to
only fall back to the DB max height when overrides.createdHeight is null or
undefined, preserving a zero value; locate the createProvider function and
replace the || fallback for overrides.createdHeight with ?? when computing
height.
In `@apps/api/test/functional/provider-deployments.spec.ts`:
- Around line 112-114: The beforeEach hook is calling initDb() which
reinitializes/truncates the database and therefore destroys the seeded data
created in beforeAll; either remove the beforeEach hook that calls initDb() or
move the seeding logic from beforeAll into beforeEach so that initDb() runs
before creating providers/deployments/leases/blocks; update or remove the
beforeEach(async () => { await initDb(); }) accordingly and ensure tests
reference the seeding functions (the setup in beforeAll or the new beforeEach)
so each test sees the expected records.
🧹 Nitpick comments (4)
apps/api/test/functional/managed-api-deployment-flow.spec.ts (1)
576-579: Consider usingLoggerServiceinstead ofconsole.error.The coding guidelines specify using
LoggerServicefor logging instead ofconsole.error. While this is in a test file for debugging purposes, consider using the project's logging infrastructure for consistency.Suggested change
if (!data?.data?.leases) { - console.error("Cannot create lease", data); + // Consider injecting or importing a logger if detailed error logging is needed return undefined; }As per coding guidelines: "Use
LoggerServiceinstead ofconsole.log,console.warn, orconsole.errorfor logging"apps/api/src/provider/services/provider-dashboard/provider-dashboard.service.spec.ts (3)
12-12: UseProviderDashboardService.namefor the root suite.♻️ Proposed change
-describe("Provider Dashboard", () => { +describe(ProviderDashboardService.name, () => {As per coding guidelines, Use
<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references.
38-38: Prefer a method name or a “when …” condition for nested suites.♻️ Proposed change
-describe("GET /v1/provider-dashboard/{owner}", () => { +describe("getProviderDashboard", () => {As per coding guidelines, Use either a method name or a condition starting with 'when' for nested suite descriptions in tests.
55-57: Movesetupto the root describe bottom and accept an inline-typed param.♻️ Proposed change
- function setup() { - return new ProviderDashboardService(); - } }); + + function setup(_: {} = {}) { + return new ProviderDashboardService(); + } });As per coding guidelines, Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.
stalniy
force-pushed
the
refactor/sequelize
branch
from
4aba56a to
207018b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/billing/repositories/usage/usage.repository.spec.ts (1)
414-423:⚠️ Potential issue | 🟡 MinorUpdate
setup()signature and mocks to match test guidelines.
setup()should accept a single parameter with an inline type definition, and mocks should come fromjest-mock-extendedrather than manualjest.fn()+ casting.🔧 Suggested refactor
+import { mock } from "jest-mock-extended"; ... - function setup() { - const userWalletRepository = { - findOneByAddress: jest.fn().mockResolvedValue(undefined) - } as unknown as UserWalletRepository; - - const txManagerService = { - getDerivedWalletAddress: jest.fn().mockResolvedValue("") - } as unknown as TxManagerService; + function setup({ + userWalletRepository: userWalletRepositoryOverrides, + txManagerService: txManagerServiceOverrides + }: { + userWalletRepository?: Partial<UserWalletRepository>; + txManagerService?: Partial<TxManagerService>; + } = {}) { + const userWalletRepository = mock<UserWalletRepository>(); + userWalletRepository.findOneByAddress.mockResolvedValue(undefined); + Object.assign(userWalletRepository, userWalletRepositoryOverrides); + + const txManagerService = mock<TxManagerService>(); + txManagerService.getDerivedWalletAddress.mockResolvedValue(""); + Object.assign(txManagerService, txManagerServiceOverrides); const usageRepository = new UsageRepository(container.resolve(CHAIN_DB), userWalletRepository, txManagerService);As per coding guidelines: "Don't use
jest.mock()in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test" and "Usesetupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."
🧹 Nitpick comments (2)
apps/api/test/functional/addresses.spec.ts (1)
214-215: Consider movinginitDb()tobeforeAllfor consistency and efficiency.In
validators.spec.ts,initDb()is called once inbeforeAll. Here, it's called insidesetup()which runs for every test (~11 times). This creates an inconsistency and may cause redundant initialization overhead.♻️ Suggested refactor
Add a
beforeAllblock and removeinitDb()fromsetup():describe("Addresses API", () => { + beforeAll(async () => { + await initDb(); + }); + afterEach(async () => {Then remove from
setup:const setup = async () => { - await initDb(); - const address = createAkashAddress();apps/api/src/provider/services/provider-versions/provider-versions.service.spec.ts (1)
11-11: UseProviderVersionsService.nameinstead of a hardcoded string.The root describe block should use the class name reference for better refactoring support.
-describe("Provider Dashboard", () => { +describe(ProviderVersionsService.name, () => {As per coding guidelines: "Use
<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references."
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2636 +/- ##
==========================================
- Coverage 50.87% 50.61% -0.26%
==========================================
Files 1028 1018 -10
Lines 29083 28754 -329
Branches 6614 6573 -41
==========================================
- Hits 14796 14554 -242
+ Misses 13900 13813 -87
Partials 387 387
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
refactor/sequelize
branch
from
207018b to
57358ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/test/functional/managed-api-deployment-flow.spec.ts (1)
329-341:⚠️ Potential issue | 🟠 MajorMove
setup()to the bottom of the rootdescribeblock and add a single inline-typed parameter.The
setup()function violates two test conventions:
- It's defined at line 329 while the root
describeblock closes at line 709, with many other helper functions after it. Per guidelines,setup()must be at the bottom.- It accepts no parameters; it should accept a single parameter with inline type definition (e.g.,
setup(params: { /* type */ })or similar).Regarding
initDb()idempotency:connectUsingSequelize(aliased asinitDb) is safe to call repeatedly—Sequelize'sauthenticate()and.sync()methods are idempotent. However, sincesetup()runs per test case (and perit.eachiteration), callinginitDb()multiple times across test executions is inefficient. Moving to a singlesetup()call at the root level would also address this.
🧹 Nitpick comments (1)
apps/api/src/chain/providers/sequelize.provider.ts (1)
18-21: Module-scope DI resolution may cause side effects.
CoreConfigServiceis resolved at module scope (line 18), which happens at module import time. This can cause side effects and reduce testability.Consider moving this resolution inside the factory or the functions that need it.
Proposed fix
-const coreConfig = container.resolve(CoreConfigService); - -const logger = new PostgresLoggerService({ orm: "sequelize", useFormat: coreConfig.get("SQL_LOG_FORMAT") === "pretty" }); -const logging = (msg: string) => logger.write(msg); +const createLogging = () => { + const coreConfig = container.resolve(CoreConfigService); + const logger = new PostgresLoggerService({ orm: "sequelize", useFormat: coreConfig.get("SQL_LOG_FORMAT") === "pretty" }); + return (msg: string) => logger.write(msg); +};Then use
createLogging()within the factory registrations.Based on learnings: "In the Akash Console API (apps/api), avoid resolving DI container dependencies at module scope (module initialization time) to prevent side effects."
stalniy
force-pushed
the
refactor/sequelize
branch
from
57358ae to
2bf07d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/api/src/chain/index.ts`:
- Around line 1-3: Move the CoreConfigService resolution and
PostgresLoggerService instantiation out of module scope and into the factory
functions used in container.register for CHAIN_DB and USER_DB: inside the
instancePerContainerCachingFactory callback, call c.resolve(CoreConfigService)
and create the PostgresLoggerService (use its get("SQL_LOG_FORMAT") value to
decide useFormat), build the logging function (msg => logger.write(msg)), then
resolve ChainConfigService (or the user DB config) to get the DB URI and
construct the Sequelize instance there; ensure you replace the module-scope
logger/coreConfig variables and wire logging, logQueryParameters,
transactionType, define, and models exactly as in the current factory
configuration.
In
`@apps/api/src/provider/services/provider-versions/provider-versions.service.spec.ts`:
- Around line 90-91: Update the test helper setup function so it accepts a
single options parameter with an inline type annotation (instead of no args);
have callers pass or default the two dependencies used to construct
ProviderVersionsService (resolve CHAIN_DB and PROVIDER_CONFIG). In other words,
change setup to take one parameter (e.g., { chainDb, providerConfig }: {
chainDb?: /* inline type */; providerConfig?: /* inline type */ } = {}) and
inside the function use chainDb ?? container.resolve(CHAIN_DB) and
providerConfig ?? container.resolve(PROVIDER_CONFIG) when calling new
ProviderVersionsService(...), referencing ProviderVersionsService,
container.resolve(CHAIN_DB) and container.resolve(PROVIDER_CONFIG) so tests can
override dependencies.
🧹 Nitpick comments (2)
apps/api/test/functional/usage.spec.ts (1)
405-427: Consider addinginitDb()for test isolation.The second describe block's
setup()doesn't callinitDb(), unlike the first block. If these tests run in isolation (e.g., viajest -t "GET /v1/usage/history/stats"), the database won't be initialized, potentially causing failures.♻️ Suggested fix
describe("GET /v1/usage/history/stats", () => { - function setup() { + async function setup() { + await initDb(); const owners = [createAkashAddress(), createAkashAddress()];And update all test calls to
await setup():it("returns usage stats for a valid address with default date range", async () => { - const { owners, expectUsageStats } = setup(); + const { owners, expectUsageStats } = await setup();apps/api/src/chain/providers/sequelize.provider.ts (1)
23-54: Avoid module-scope DI resolution for DB logging config.
CoreConfigServiceis still resolved at module scope viacoreConfig/logger, which keeps a side effect on import. Consider moving that resolution inside the per-container factories so DB creation stays fully on-demand.🔧 Suggested refactor to keep DI resolution inside factories
-const coreConfig = container.resolve(CoreConfigService); - -const logger = new PostgresLoggerService({ orm: "sequelize", useFormat: coreConfig.get("SQL_LOG_FORMAT") === "pretty" }); -const logging = (msg: string) => logger.write(msg); +const createDbLogging = (coreConfig: CoreConfigService) => { + const logger = new PostgresLoggerService({ orm: "sequelize", useFormat: coreConfig.get("SQL_LOG_FORMAT") === "pretty" }); + return (msg: string) => logger.write(msg); +};container.register(CHAIN_DB, { useFactory: instancePerContainerCachingFactory(c => { const dbUri = c.resolve(ChainConfigService).get("CHAIN_INDEXER_POSTGRES_DB_URI"); + const logging = createDbLogging(c.resolve(CoreConfigService)); return new Sequelize(dbUri, { dialectModule: pg, logging,container.register(USER_DB, { useFactory: instancePerContainerCachingFactory(c => { const dbUri = c.resolve(CoreConfigService).get("POSTGRES_DB_URI"); + const logging = createDbLogging(c.resolve(CoreConfigService)); return new Sequelize(dbUri, { dialectModule: pg, logging,Based on learnings: In the Akash Console API (apps/api), avoid resolving DI container dependencies at module scope (module initialization time) to prevent side effects. Instead, resolve dependencies inside functions/methods where they are actually used, even if this means resolving on every invocation, to maintain explicit control over when side effects occur and improve testability.
apps/api/src/provider/services/provider-versions/provider-versions.service.spec.ts
Show resolved
Hide resolved
stalniy
force-pushed
the
refactor/sequelize
branch
from
ff20936 to
c2f1b75
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/api/test/services/retry-policies.ts`:
- Line 1: The code uses isRetriableError with casts but doesn't defensively
verify the presence of the code property on thrown errors; update the check
around the retriable logic (where isRetriableError is called) to first verify
"code" in err and, when inspecting nested causes, "code" in err.cause before
casting to Error & { code: unknown } and calling isRetriableError; modify the
branch in the retry-policies logic that references err and err.cause so it only
casts and calls isRetriableError after those property-existence checks.
🧹 Nitpick comments (2)
apps/api/src/chain/providers/sequelize.provider.ts (1)
27-27: Module-level side effect contradicts PR objective.
pg.defaults.parseInt8 = truemutates global state at import time, which is inconsistent with the PR goal of removing side effects from module scope. Consider moving this into the factory or theconnectUsingSequelizefunction to ensure the mutation only happens when the database is actually initialized.Based on learnings: "avoid resolving DI container dependencies at module scope (module initialization time) to prevent side effects."
♻️ Proposed fix
Move the setting into the factory:
export const CHAIN_DB = Symbol("CHAIN_DB") as InjectionToken<Sequelize>; -pg.defaults.parseInt8 = true; container.register(CHAIN_DB, { useFactory: instancePerContainerCachingFactory(c => { + pg.defaults.parseInt8 = true; const logger = c.resolve(SEQUELIZE_LOGGER);apps/api/test/services/retry-policies.ts (1)
96-103: Prefer a type-guard helper over repeated casts.
This keeps the retriable check consistent and avoids ad‑hoc assertions when unwrapping causes.♻️ Suggested refactor
+function hasErrorCode(err: unknown): err is Error & { code: string } { + return err instanceof Error && typeof (err as { code?: unknown }).code === "string"; +} + +function isRetriableHttpError(err: unknown): boolean { + if (hasErrorCode(err) && isRetriableError(err)) return true; + if (err instanceof Error && hasErrorCode(err.cause) && isRetriableError(err.cause)) return true; + return false; +} + export function isFaucetTransient(err: unknown): boolean { - if ( - err && - err instanceof Error && - (isRetriableError(err as Error & { code: unknown }) || (err.cause instanceof Error && isRetriableError(err.cause as Error & { code: unknown }))) - ) { + if (isRetriableHttpError(err)) { return true; }
| @@ -1,14 +1,6 @@ | |||
| import { isRetriableError } from "@akashnetwork/http-sdk"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l apps/api/test/services/retry-policies.tsRepository: akash-network/console
Length of output: 109
🏁 Script executed:
cat -n apps/api/test/services/retry-policies.ts | head -150Repository: akash-network/console
Length of output: 4870
🏁 Script executed:
rg "isRetriableError" -B 2 -A 5Repository: akash-network/console
Length of output: 8447
🏁 Script executed:
fd -t f "http-sdk" --type dRepository: akash-network/console
Length of output: 347
🏁 Script executed:
find . -path "*/node_modules/@akashnetwork/http-sdk*" -type f -name "*.ts" -o -name "*.d.ts" | head -20Repository: akash-network/console
Length of output: 224
Align error property checking with existing patterns in batch-signing-client services.
The isRetriableError helper expects Error & { code: unknown }. The code casts errors to this type correctly, but other services in the codebase first validate the property exists with "code" in error before casting. Apply this defensive pattern on line 100 to check for code property existence on both err and err.cause before casting to isRetriableError.
🤖 Prompt for AI Agents
In `@apps/api/test/services/retry-policies.ts` at line 1, The code uses
isRetriableError with casts but doesn't defensively verify the presence of the
code property on thrown errors; update the check around the retriable logic
(where isRetriableError is called) to first verify "code" in err and, when
inspecting nested causes, "code" in err.cause before casting to Error & { code:
unknown } and calling isRetriableError; modify the branch in the retry-policies
logic that references err and err.cause so it only casts and calls
isRetriableError after those property-existence checks.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/api/src/chain/providers/sequelize.provider.ts`:
- Around line 82-95: connectUsingSequelize currently always resolves CHAIN_DB
and USER_DB from the global container which breaks per-container isolation;
change the function signature of connectUsingSequelize to accept an optional DI
container parameter (defaulting to the existing global container) and use that
passed-in container to resolve CHAIN_DB and USER_DB (and any other
container.resolve calls inside, e.g., syncUserSchema if it resolves from
container) so child containers will resolve their scoped instances; keep the
parameter optional to remain backward-compatible and update any call sites to
pass their local container where needed.
| /** | ||
| * Initialize database schema | ||
| * Populate db | ||
| * Create backups per version | ||
| * Load from backup if exists for current version | ||
| * @deprecated use `container.resolveAll(APP_INITIALIZER)` instead | ||
| */ | ||
| export async function connectUsingSequelize(logger = createOtelLogger({ context: "DB" })): Promise<void> { | ||
| await Promise.all([authenticateDatabase(container.resolve(CHAIN_DB), logger), authenticateDatabase(container.resolve(USER_DB), logger)]); | ||
|
|
||
| logger.debug("Sync user schema..."); | ||
| await syncUserSchema(); | ||
| logger.debug("User schema synced."); | ||
| } |
There was a problem hiding this comment.
Use the calling DI container in connectUsingSequelize to preserve per-container isolation.
Line 90 resolves CHAIN_DB/USER_DB from the global container. If a child container calls this, it will connect/sync the root instances instead of the scoped ones, undermining the per-container refactor and test isolation.
✅ Suggested fix (optional container parameter, backward-compatible)
-import { container, instancePerContainerCachingFactory } from "tsyringe";
+import { container, instancePerContainerCachingFactory, type DependencyContainer } from "tsyringe";
@@
-export async function connectUsingSequelize(logger = createOtelLogger({ context: "DB" })): Promise<void> {
- await Promise.all([authenticateDatabase(container.resolve(CHAIN_DB), logger), authenticateDatabase(container.resolve(USER_DB), logger)]);
+export async function connectUsingSequelize(
+ logger = createOtelLogger({ context: "DB" }),
+ diContainer: DependencyContainer = container
+): Promise<void> {
+ await Promise.all([
+ authenticateDatabase(diContainer.resolve(CHAIN_DB), logger),
+ authenticateDatabase(diContainer.resolve(USER_DB), logger)
+ ]);
@@
- await connectUsingSequelize(c.resolve(LoggerService));
+ await connectUsingSequelize(c.resolve(LoggerService), c);#!/bin/bash
# Verify all call sites of connectUsingSequelize so container param can be passed where needed.
rg -n --type=ts -C 2 '\bconnectUsingSequelize\s*\('🤖 Prompt for AI Agents
In `@apps/api/src/chain/providers/sequelize.provider.ts` around lines 82 - 95,
connectUsingSequelize currently always resolves CHAIN_DB and USER_DB from the
global container which breaks per-container isolation; change the function
signature of connectUsingSequelize to accept an optional DI container parameter
(defaulting to the existing global container) and use that passed-in container
to resolve CHAIN_DB and USER_DB (and any other container.resolve calls inside,
e.g., syncUserSchema if it resolves from container) so child containers will
resolve their scoped instances; keep the parameter optional to remain
backward-compatible and update any call sites to pass their local container
where needed.
2 participants
Why
to get rid of side effects inside src/db/dbConnection.ts and improve testability
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.