refactor: moves sequelize creation into DI container factory #2636
Conversation
📝 WalkthroughWalkthroughThis pull request migrates the codebase from direct module-scoped Changes
Sequence DiagramsequenceDiagram
participant App as Application Startup
participant Container as DI Container
participant SeqProvider as Sequelize Provider
participant ChainDB as CHAIN_DB Token
participant Service as Service/Repository
participant DB as Database
App->>Container: Initialize container
App->>SeqProvider: Register CHAIN_DB factory
SeqProvider->>Container: Register factory with CHAIN_DB token
App->>Service: Instantiate service with `@inject`(CHAIN_DB)
Service->>Container: Resolve CHAIN_DB token
Container->>SeqProvider: Execute CHAIN_DB factory
SeqProvider->>DB: Create Sequelize instance
DB-->>SeqProvider: Instance created
SeqProvider-->>Container: Return Sequelize instance
Container-->>Service: Inject Sequelize instance
Service->>Service: Store in private `#chainDb` field
Service->>DB: Execute query via this.#chainDb
DB-->>Service: Query result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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.91% 50.16% -0.75%
==========================================
Files 1055 1020 -35
Lines 29781 28965 -816
Branches 6713 6599 -114
==========================================
- Hits 15162 14531 -631
+ Misses 14335 14034 -301
- Partials 284 400 +116
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
Why
to get rid of side effects inside src/db/dbConnection.ts and improve testability
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.