Feature/update package #158
Conversation
WalkthroughMultiple dependency upgrades including Node.js 24.10, OpenTelemetry packages, and core libraries. Configuration changes include removing Kafka services, updating container images, and converting ESLint configuration to TypeScript. Several type aliases converted to interfaces, and significant refactoring of JWT middleware, reset password flow, and tracing initialization with NodeSDK. Changes
Sequence DiagramsequenceDiagram
participant User
participant App
participant Email Service
participant Database
participant User Email
User->>App: POST /password/recover (email)
activate App
App->>Database: Find user by email
Database-->>App: User found
App->>Database: Create reset token
Database-->>App: Reset hash
App->>Email Service: Send reset email with hash
Email Service->>User Email: Email sent
deactivate App
User->>User Email: Click reset link (hash)
activate User
User->>App: POST /password/reset (newPassword, hash)
activate App
App->>Database: Find reset by hash
Database-->>App: Reset record
App->>Database: Update user password
Database-->>App: Success
App-->>User: Password updated
deactivate App
deactivate User
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/presentation/tracing.ts (1)
18-21: Remove redundantregisterInstrumentationscall.This call with an empty instrumentations array is now redundant since instrumentations are passed directly to
NodeSDKon line 33. It can be safely removed along with the import on line 4.-import { registerInstrumentations } from '@opentelemetry/instrumentation'-// Optionally register instrumentation libraries -registerInstrumentations({ - instrumentations: [], -})src/core/providers/notification.provider.ts (1)
73-88: Async strategy methods fire‑and‑forget; addawaitto avoid unhandled rejectionsThe shorthand
{ email, hash }insendEmailis fine, but both strategy helpers invoke async methods withoutawait:
sendCodeByStrategy→this.sendCodeByEmail/this.sendCodeByPhonesendMfaHashByStrategy→this.sendMfaHashByEmail/this.sendMfaHashByPhoneThis means:
- Callers of the strategy methods can resolve before Kafka sends complete.
- Any rejection inside those async methods becomes an unhandled promise rejection, which can be fatal on newer Node versions.
Recommend awaiting the inner calls so errors propagate and the contract “send completes when this promise resolves” holds:
async sendCodeByStrategy( @@ - switch (strategy) { - case Strategy.EMAIL: - this.sendCodeByEmail(userId, code) - break - case Strategy.PHONE: - this.sendCodeByPhone(userId, code) - break - default: - break - } + switch (strategy) { + case Strategy.EMAIL: + await this.sendCodeByEmail(userId, code) + break + case Strategy.PHONE: + await this.sendCodeByPhone(userId, code) + break + default: + break + } @@ async sendMfaHashByStrategy( @@ - switch (strategy) { - case Strategy.EMAIL: - this.sendMfaHashByEmail(userId, hash) - break - case Strategy.PHONE: - this.sendMfaHashByPhone(userId, hash) - break - default: - break - } + switch (strategy) { + case Strategy.EMAIL: + await this.sendMfaHashByEmail(userId, hash) + break + case Strategy.PHONE: + await this.sendMfaHashByPhone(userId, hash) + break + default: + break + }This aligns better with the rest of your async API and reduces the risk of unhandled rejections around Kafka I/O.
Also applies to: 132-147, 149-158
src/core/usecases/reset_password.usecase.ts (1)
29-36: Await reset email sending to avoid unhandled rejectionsThe refactor in
recoverto declareuserin oneconstis fine. However, inforgetyou call an async dependency without awaiting it:const hash = await this.creatingResetPassword.create(email) this.sendingResetEmail.sendEmail(email, hash)If
sendEmailrejects (Kafka/network issues, etc.), the error will not be caught by thistry/catchand can become an unhandled rejection, especially problematic after upgrading Node.Recommend awaiting it:
async forget(email: string): Promise<void> { try { const hash = await this.creatingResetPassword.create(email) - this.sendingResetEmail.sendEmail(email, hash) + await this.sendingResetEmail.sendEmail(email, hash)This way callers only see success once the reset email enqueue/send has actually succeeded, and dependency errors map cleanly to
ForgetPasswordErrorsTypes.DEPENDECY_ERROR.Also applies to: 39-44
🧹 Nitpick comments (30)
src/core/usecases/token.usecase.ts (1)
27-28: Comma-separated declarations reduce readability.While syntactically correct, merging
constdeclarations with mixed async/sync assignments on consecutive lines can be harder to read and debug. Consider keeping separate declarations:const user = await this.findingUser.findById(userId) const token = this.creatingToken.create(user)If this pattern is enforced by your updated linter configuration, feel free to disregard.
src/core/providers/mfa_code.repository.ts (1)
40-42: Same readability concern with comma-separated declarations.While correct, multi-line comma-separated declarations can be harder to scan. Consider separate declarations for clarity:
const hash = this.uuidService.generateHash() const code = this.codeService.generateRandomNumber() const content: CacheCode = { userId, code, strategy }If this is a linter-enforced pattern, feel free to disregard.
test/fixtures/generators.ts (1)
9-15: jsonGenerator refactor keeps output shape; slight change in construction styleThe concise
reduceform still produces an object whose keys come fromkeyListand whose values arecasual.sentence. The only difference is that it now creates a fresh object on each iteration with spread, which is fine for tests but does change property insertion order compared to an in‑place mutation pattern.test/fixtures/user.ts (1)
29-40: Consider splitting consolidated declarations for readability.While the consolidated comma-separated declarations are functionally correct, combining 12 lines of logic—including input defaulting, cryptographic operations, and database insertion—into a single statement reduces readability and makes debugging more difficult.
Consider breaking this into logical groups:
- const name = input?.name ?? casual.full_name, - email = input?.email ?? casual.email.toLowerCase(), - password = input?.password ?? passwordGenerator(), - salt = genSaltSync(12), - hashPw = await hashFunc(password, salt), - row: { id: string }[] = await database('user') - .insert({ - name, - email, - password_hash: hashPw, - }) - .returning('id') + const name = input?.name ?? casual.full_name + const email = input?.email ?? casual.email.toLowerCase() + const password = input?.password ?? passwordGenerator() + + const salt = genSaltSync(12) + const hashPw = await hashFunc(password, salt) + + const row: { id: string }[] = await database('user') + .insert({ + name, + email, + password_hash: hashPw, + }) + .returning('id')src/core/providers/user.repository.ts (2)
105-114: Consider splitting consolidated declarations for clarity.Consolidating hash generation, insert object creation, and the database operation into a single multi-variable declaration reduces readability and makes debugging more difficult.
Consider separating the concerns:
- const hash = await this.passwordService.generateHash(password), - insertLine = { - name, - email, - password_hash: hash, - }, - response: { id: string }[] = await this.database<UserRow>('user') - .insert(insertLine) - .returning('id') + const hash = await this.passwordService.generateHash(password) + const insertLine = { + name, + email, + password_hash: hash, + } + const response: { id: string }[] = await this.database<UserRow>('user') + .insert(insertLine) + .returning('id')
125-129: Consider splitting async operations.Combining password hash generation and the database update in a single declaration makes it harder to debug and understand the flow.
Consider:
- const hash = await this.passwordService.generateHash(password), - response = await this.database<UserRow>('user') - .update({ password_hash: hash }) - .where('id', user.id) + const hash = await this.passwordService.generateHash(password) + const response = await this.database<UserRow>('user') + .update({ password_hash: hash }) + .where('id', user.id)src/core/providers/mfa.repository.ts (1)
46-54: Awkward multi-declaration formatting reduces readability.The comma-separated
constdeclaration combininginsertLineandrespis valid but the indentation is misleading—the object properties are indented further than the opening brace, and therespdeclaration appears as if it's part of the object literal. Consider using separateconststatements for clarity:- const insertLine = { - user_id: user.id, - strategy, - //If GA, is_enable must be true. No way to validate the authenticity - is_enable: strategy === Strategy.GA, - }, - resp: { id: string }[] = await this.database(this.tableName) - .insert(insertLine) - .returning('id') + const insertLine = { + user_id: user.id, + strategy, + // If GA, is_enable must be true. No way to validate the authenticity + is_enable: strategy === Strategy.GA, + } + const resp: { id: string }[] = await this.database(this.tableName) + .insert(insertLine) + .returning('id')src/presentation/tracing.ts (1)
32-32: ConsiderBatchSpanProcessorfor production environments.
SimpleSpanProcessorexports spans synchronously, which can add latency to requests. For production use,BatchSpanProcessoris recommended as it batches and exports spans asynchronously, reducing performance impact.-import { SimpleSpanProcessor } from '@opentelemetry/sdk-trace-base' +import { BatchSpanProcessor } from '@opentelemetry/sdk-trace-base'- spanProcessors: [new SimpleSpanProcessor(exporter)], + spanProcessors: [new BatchSpanProcessor(exporter)],test/fixtures/multi_factor_authentication.ts (1)
11-28: Row typing is fine; consider safer destructuring
const row: { id: string }[] = await ...returning('id')matches Knex’s return shape, and existing logic is preserved. For slightly safer code in tests, you could destructure and guard against unexpected empty results:- const row: { id: string }[] = await database('multi_factor_authentication') + const rows: { id: string }[] = await database('multi_factor_authentication') .insert({ user_id: input?.userId, strategy: input?.strategy, is_enable: input?.isEnable ?? true, }) .returning('id') + const [row] = rows + + if (!row) { + throw new Error('insertMfaIntoDatabase: insert did not return an id') + } @@ - id: row[0].id, + id: row.id,Not required, but makes failures easier to diagnose in fixtures.
src/presentation/http/routes/reset_password.route.ts (1)
12-21: Handlers are correct but multi‑awaitconst declarations hurt readabilityThe use of
Joi.types()with destructuredobject/stringis fine, and the validation logic looks sound. The chainedconstdeclarations with multipleawaits in each route make the handlers harder to read and debug, though.Consider reverting to one
constper logical step:- try { - const { email }: ForgetPasswordInput = await schema.validateAsync(req.body), - core = await getCore(), - resp = await core.reset.forget(email) + try { + const { email }: ForgetPasswordInput = await schema.validateAsync(req.body) + const core = await getCore() + const resp = await core.reset.forget(email) @@ - try { - const { password, hash }: RecoverPasswordInput = - await schema2.validateAsync(req.body), - core = await getCore(), - resp = await core.reset.recover(password, hash) + try { + const { password, hash }: RecoverPasswordInput = + await schema2.validateAsync(req.body) + const core = await getCore() + const resp = await core.reset.recover(password, hash)Same behavior, but clearer stack traces and easier future edits.
Also applies to: 23-36, 43-46, 48-62
src/core/index.ts (1)
51-173: Consider readability implications of 123-line const block.While this consolidation aligns with the PR's formatting objectives and maintains functional equivalence, a single 123-line comma-separated const declaration may impact:
- Debugging: Setting breakpoints or adding logging mid-chain requires restructuring
- Readability: The long chain can be harder to scan than grouped declarations
- Error messages: Stack traces may be less precise for initialization failures
If this style is a deliberate project convention, consider adding inline comments to mark major sections (services, providers, usecases) for easier navigation.
src/core/services/uuid.service.ts (1)
4-4: Optional: Consider removing unusedversionfield.The
versionfield is no longer used after migrating tocrypto.randomUUID().Apply this diff to remove the unused field:
export class UuidService { - private version = 4 generateHash(): string { return randomUUID() } }src/presentation/http/routes/logout.route.ts (1)
20-21: Consolidated declarations work but consider readability.The comma-separated variable declarations are syntactically correct and follow a pattern used consistently across the route files in this PR. However, separating async operations onto individual lines can improve debuggability and readability.
If preferred, you could separate these for clarity:
- const token = retriveToken(req), - core = await getCore() + const token = retriveToken(req) + const core = await getCore()src/presentation/http/routes/organization.route.ts (1)
30-34: Consolidated declarations are consistent but consider readability.The comma-separated variable declarations follow the pattern established across route files in this PR. While syntactically correct, separating async operations can improve debugging and stacktrace readability, especially when validation and core initialization might fail.
For improved debugging, consider separating these:
try { const { name, parentId }: OrganizationInput = await schema.validateAsync( - req.body - ), - core = await getCore(), - id = await core.organization.create(name, parentId) + req.body + ) + const core = await getCore() + const id = await core.organization.create(name, parentId) res.status(200).send({ id })Also applies to: 57-59, 84-86
src/presentation/http/routes/login.route.ts (1)
32-36: Consolidated declarations follow PR pattern; consider readability.The comma-separated declarations are consistent with other route files in this PR. As with the other routes, separating async operations can improve debugging and error tracking.
For clarity in error stacktraces:
- const { email, password }: LoginInput = await schema.validateAsync( - req.body - ), - core = await getCore(), - resp = await core.login.login(email, password) + const { email, password }: LoginInput = await schema.validateAsync( + req.body + ) + const core = await getCore() + const resp = await core.login.login(email, password)Also applies to: 49-51
src/presentation/http/routes/user.route.ts (1)
32-36: Consolidated declarations are consistent; consider readability.The comma-separated variable declarations follow the established pattern throughout this PR. Separating async operations can help with debugging, particularly in the PATCH handler where multiple optional fields are processed.
For improved debugging:
- const { name, email, password }: UserInput = await schema.validateAsync( - req.body - ), - core = await getCore(), - id = await core.user.create(name, email, password) + const { name, email, password }: UserInput = await schema.validateAsync( + req.body + ) + const core = await getCore() + const id = await core.user.create(name, email, password)Also applies to: 67-76, 85-86
src/core/usecases/login.usecase.ts (1)
30-34: LGTM! Consolidated declarations follow PR pattern.The comma-separated declarations for
userandmfaListare consistent with the refactoring pattern across the codebase. As with the route files, separating these can improve debuggability.For improved debugging:
- const user = await this.findingUser.findUserByEmailAndPassword( - email, - password - ), - mfaList = await this.findingMFA.findMfaListByUserId(user.id) + const user = await this.findingUser.findUserByEmailAndPassword( + email, + password + ) + const mfaList = await this.findingMFA.findMfaListByUserId(user.id)test/unit/providers/notification.provider.test.ts (1)
22-39: Consider removing the redundantclientvariable.The
clientobject (lines 31-39) duplicates the structure already returned by thejest.spyOn(kafka, 'getKafka')mock (lines 22-30). IfNotificationProviderusesgetKafka()internally, this localclientvariable may be unnecessary.test/unit/usecases/organization.usecase.test.ts (1)
46-85: Consider extracting repetitive mock setup into a helper function.Each test case repeats the same mock instantiation pattern for
CreatingOrganization,FindingUser,AddingUserToOrganization,UpdatingOrganization, andFindingOrganization. A factory helper would reduce boilerplate and improve maintainability.Example helper pattern:
function createMocks() { const mockCreatingOrganization = mock(OrganizationRepository) const mockFindingUser = mock(UserRepository) // ... other mocks return { mockCreatingOrganization, creatingOrganization: instance(mockCreatingOrganization), // ... other instances } }src/presentation/http/middlewares/trace.ts (1)
12-15: Misplaced comment reduces readability.The comment
// Override the json functionis awkwardly positioned within the comma-separated declaration, followed by a blank line. Consider moving the comment above the const block or removing it since the code is self-explanatory.- const originalJson = res.send, - // Override the json function - - tracer = trace.getTracer(getEnv().app.name) + const originalJson = res.send + const tracer = trace.getTracer(getEnv().app.name)test/integration/http/reset_password.route.test.ts (1)
19-26: Pinned containers and mock refactor look correctExplicit Postgres/Redis image tags, the simplified
getKafkamock, and the grouped const declarations keep the reset‑password flow and its assertions intact while improving test determinism and readability. If you ever revisit this file, you might optionallyawait redis.del('*')inbeforeEachto align with the async Redis API and avoid subtle timing issues.Also applies to: 28-30, 61-69, 88-95, 99-112, 118-125
test/integration/http/organization.route.test.ts (1)
19-25: Organization integration tests: infra refactor is safeExplicit container images, consolidated test variables, and the inline Kafka mock all keep the create/update organization flows and DB assertions the same as before. Optionally, consider
await redis.del('*')inbeforeEachfor stricter cleanup in async Redis.Also applies to: 27-28, 60-68, 88-101, 107-121
src/presentation/http/middlewares/jwt.ts (2)
1-1: JWT options and exp claim look consistent; confirm 1‑hour TTL is desiredRestricting
verifytoalgorithms: ['HS256']and adding an explicitexpto the payload increateTokentighten security and make expiry explicit. However,createTokennow hard‑codes a 1‑hour lifetime (now + 60 * 60), which may differ from any previous, configurable TTL. Please confirm that this fixed duration matches your auth requirements (and any existing documentation/config); if not, consider deriving the lifetime from configuration or a parameter.Also applies to: 3-3, 8-10, 33-38, 40-53
27-31:extractPayload’sJwtbranch is likely unused with current verify options
removeJwtAttrcallsverify(token, secret, option)whereoptionis justVerifyOptions(nocomplete: true), so at runtime the result is typically a string orJwtPayload, not a fullJwtobject. That makes theisJwt/payload.payloadbranch inextractPayloadeffectively unreachable in current usage. You could either:
- simplify
extractPayloadto handle onlystring | JwtPayload, or- extend the verify options to request
complete: true(and adjust the type accordingly) if you want to rely on theJwtshape.Either way, behavior of existing callers stays clearer and the intent of this helper becomes more explicit.
Also applies to: 56-75
test/integration/http/login.route.test.ts (1)
22-27: Login/MFA integration tests stay robust after setup refactorThe consolidated variable declarations, explicit Postgres/Redis images, simplified Kafka mock, and the typed
CacheCodeparsing all keep the login + MFA (EMAIL/PHONE) and refresh‑token flows behaving as before, while making the tests clearer. As a small follow‑up, if you see any intermittent issues, consider changingredis.del('*')inbeforeEachtoawait redis.del('*')(and mirroring that in other integration tests) so cleanup is guaranteed before each test body runs.Also applies to: 29-30, 62-70, 85-95, 97-105, 107-139, 141-179, 181-203
test/unit/providers/organization.repository.test.ts (3)
123-149: Consider extracting the inline database insert for better readability.The inline database insertion within the comma-separated declaration (lines 128-135) makes the code harder to debug and understand. Breaking this into separate statements would improve clarity without changing behavior.
For example:
- const userFixture = await insertUserIntoDatabase(database), - userId = userFixture.output.id, - orgFixture = await insertOrgIntoDatabase(database), - orgId = orgFixture.output.id, - relationId: string = ( - await database('organization_user') - .insert({ - organization_id: orgId, - user_id: userId, - }) - .returning('id') - )[0].id, - orgRepository = new OrganizationRepository(database) + const userFixture = await insertUserIntoDatabase(database) + const userId = userFixture.output.id + const orgFixture = await insertOrgIntoDatabase(database) + const orgId = orgFixture.output.id + + const relationResult = await database('organization_user') + .insert({ + organization_id: orgId, + user_id: userId, + }) + .returning('id') + const relationId = relationResult[0].id + + const orgRepository = new OrganizationRepository(database)
184-211: Recommend splitting the lengthy comma-separated declaration for maintainability.This single declaration spans 22 lines with ~12 variables, mixing async fixture creation, object construction, and repository method calls. This makes debugging difficult and git diffs harder to review.
Consider breaking into separate
conststatements:- const orgFixtureC = await insertOrgIntoDatabase(database), - currentParentOrgId = orgFixtureC.output.id, - orgFixtureT = await insertOrgIntoDatabase(database), - targetParentOrgId = orgFixtureT.output.id, - orgFixture = await insertOrgIntoDatabase(database), - orgId = orgFixture.output.id, - organization: Organization = { - id: orgId, - name: orgFixture.input.name, - parentOrganizationId: currentParentOrgId, - }, - targetOrganization: Organization = { - id: targetParentOrgId, - name: orgFixtureT.input.name, - parentOrganizationId: null, - }, - orgRepository = new OrganizationRepository(database), - result = await orgRepository.checkCyclicRelationship( - organization, - targetOrganization - ) + const orgFixtureC = await insertOrgIntoDatabase(database) + const currentParentOrgId = orgFixtureC.output.id + const orgFixtureT = await insertOrgIntoDatabase(database) + const targetParentOrgId = orgFixtureT.output.id + const orgFixture = await insertOrgIntoDatabase(database) + const orgId = orgFixture.output.id + + const organization: Organization = { + id: orgId, + name: orgFixture.input.name, + parentOrganizationId: currentParentOrgId, + } + const targetOrganization: Organization = { + id: targetParentOrgId, + name: orgFixtureT.input.name, + parentOrganizationId: null, + } + + const orgRepository = new OrganizationRepository(database) + const result = await orgRepository.checkCyclicRelationship( + organization, + targetOrganization + )
212-241: Same recommendation: split lengthy declaration for better maintainability.Similar to the previous test case, this 21-line declaration with ~12 variables mixing async operations and object construction impacts readability and debugging. Consider the same refactoring approach of using separate
conststatements.test/unit/providers/mfa.repository.test.ts (1)
108-120: Consider simplifying the declaration that mixes async operations, destructuring, and mocking.Lines 109-117 combine fixture creation, object destructuring, mock setup, and repository method invocation in a single declaration. The destructuring pattern
{ id } = mfaFixture.outputwithin a comma-separated list is particularly hard to parse.Breaking this into separate statements would improve clarity:
- const mfaFixture = await insertMfaIntoDatabase(database, { - userId: mockUserId, - strategy: Strategy.EMAIL, - }), - { id } = mfaFixture.output, - mockUpdatingUser: UpdatingUser = mock(UserRepository), - updatingUser: UpdatingUser = instance(mockUpdatingUser), - mFARepository = new MFARepository(database, updatingUser), - result = await mFARepository.findMfaListByUserId(mockUserId) + const mfaFixture = await insertMfaIntoDatabase(database, { + userId: mockUserId, + strategy: Strategy.EMAIL, + }) + const { id } = mfaFixture.output + + const mockUpdatingUser: UpdatingUser = mock(UserRepository) + const updatingUser: UpdatingUser = instance(mockUpdatingUser) + const mFARepository = new MFARepository(database, updatingUser) + const result = await mFARepository.findMfaListByUserId(mockUserId)test/integration/http/user.route.test.ts (1)
86-107: Recommend separating async operations from variable declarations.Lines 87-97 embed an async HTTP request within a comma-separated declaration, and lines 99-102 mix an async database query with synchronous array access. This makes debugging harder and obscures the test flow.
Consider separating the operations:
- const employeeName = casual.full_name, - employeeEmail = casual.email.toLowerCase(), - employeePassword = passwordGenerator(), - response = await request(server) - .post('/user') - .set('Authorization', `Bearer ${token}`) - .send({ - name: employeeName, - email: employeeEmail, - password: employeePassword, - }) + const employeeName = casual.full_name + const employeeEmail = casual.email.toLowerCase() + const employeePassword = passwordGenerator() + + const response = await request(server) + .post('/user') + .set('Authorization', `Bearer ${token}`) + .send({ + name: employeeName, + email: employeeEmail, + password: employeePassword, + }) + expect(response.status).toEqual(201) - const tuples = await database('user') - .select('*') - .where('id', response.body.id), - row = tuples[0] + + const tuples = await database('user') + .select('*') + .where('id', response.body.id) + const row = tuples[0]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (75)
.nvmrc(1 hunks).prettierignore(1 hunks)coverage/lcov.info(39 hunks)docker-compose.yml(2 hunks)eslint.config.mjs(0 hunks)eslint.config.mts(1 hunks)package.json(2 hunks)src/config/enviroment_config.ts(2 hunks)src/config/metric.ts(1 hunks)src/core/config/cache.ts(1 hunks)src/core/config/kafka.ts(1 hunks)src/core/entities/credentials.ts(1 hunks)src/core/entities/mfa.ts(1 hunks)src/core/index.ts(1 hunks)src/core/providers/mfa.repository.ts(2 hunks)src/core/providers/mfa_code.repository.ts(1 hunks)src/core/providers/notification.provider.ts(1 hunks)src/core/providers/organization.repository.ts(3 hunks)src/core/providers/token.repository.ts(1 hunks)src/core/providers/user.repository.ts(6 hunks)src/core/services/password.service.ts(1 hunks)src/core/services/uuid.service.ts(1 hunks)src/core/usecases/driven/finding_user.driven.ts(1 hunks)src/core/usecases/driver/refresh_token.driver.ts(1 hunks)src/core/usecases/login.usecase.ts(1 hunks)src/core/usecases/logout.usecase.ts(0 hunks)src/core/usecases/mfa.usecase.ts(2 hunks)src/core/usecases/mfa_choose.usecase.ts(0 hunks)src/core/usecases/mfa_code.usecase.ts(1 hunks)src/core/usecases/organization.usecase.ts(0 hunks)src/core/usecases/reset_password.usecase.ts(1 hunks)src/core/usecases/token.usecase.ts(1 hunks)src/core/usecases/user.usecase.ts(2 hunks)src/presentation/http/app.ts(1 hunks)src/presentation/http/middlewares/jwt.ts(3 hunks)src/presentation/http/middlewares/metric.ts(2 hunks)src/presentation/http/middlewares/trace.ts(2 hunks)src/presentation/http/routes/login.route.ts(3 hunks)src/presentation/http/routes/logout.route.ts(2 hunks)src/presentation/http/routes/mfa.route.ts(5 hunks)src/presentation/http/routes/organization.route.ts(4 hunks)src/presentation/http/routes/reset_password.route.ts(3 hunks)src/presentation/http/routes/user.route.ts(4 hunks)src/presentation/http/server.ts(1 hunks)src/presentation/tracing.ts(2 hunks)test/fixtures/generators.ts(1 hunks)test/fixtures/multi_factor_authentication.ts(1 hunks)test/fixtures/organization.ts(1 hunks)test/fixtures/setup_migration.ts(1 hunks)test/fixtures/user.ts(2 hunks)test/fixtures/user_info.ts(1 hunks)test/integration/http/login.route.test.ts(6 hunks)test/integration/http/logout.route.test.ts(2 hunks)test/integration/http/mfa.route.test.ts(3 hunks)test/integration/http/organization.route.test.ts(3 hunks)test/integration/http/reset_password.route.test.ts(3 hunks)test/integration/http/user.route.test.ts(3 hunks)test/unit/providers/mfa.repository.test.ts(5 hunks)test/unit/providers/mfa_choose.repository.test.ts(2 hunks)test/unit/providers/mfa_code.repository.test.ts(4 hunks)test/unit/providers/notification.provider.test.ts(2 hunks)test/unit/providers/organization.repository.test.ts(10 hunks)test/unit/providers/user.repository.test.ts(16 hunks)test/unit/services/code.services.test.ts(1 hunks)test/unit/services/password.services.test.ts(1 hunks)test/unit/services/uuid.services.test.ts(1 hunks)test/unit/usecases/login.usecase.test.ts(4 hunks)test/unit/usecases/logout.usecase.test.ts(2 hunks)test/unit/usecases/mfa.usecase.test.ts(9 hunks)test/unit/usecases/mfa_choose.usecase.test.ts(5 hunks)test/unit/usecases/mfa_code.usecase.test.ts(4 hunks)test/unit/usecases/organization.usecase.test.ts(18 hunks)test/unit/usecases/reset_password.usecase.test.ts(2 hunks)test/unit/usecases/token.usecase.test.ts(2 hunks)test/unit/usecases/user.usecase.test.ts(3 hunks)
💤 Files with no reviewable changes (4)
- eslint.config.mjs
- src/core/usecases/mfa_choose.usecase.ts
- src/core/usecases/logout.usecase.ts
- src/core/usecases/organization.usecase.ts
🧰 Additional context used
🧬 Code graph analysis (33)
src/presentation/http/routes/reset_password.route.ts (1)
src/core/index.ts (1)
getCore(50-186)
test/unit/services/password.services.test.ts (2)
src/core/services/password.service.ts (1)
PasswordService(4-19)test/fixtures/generators.ts (1)
passwordGenerator(39-51)
test/unit/usecases/mfa.usecase.test.ts (5)
src/core/usecases/driven/finding_user.driven.ts (1)
FindingUser(3-8)src/core/usecases/driven/finding_mfa.driven.ts (1)
FindingMFA(3-16)src/core/usecases/driven/creating_mfa.driven.ts (2)
CreatingMFA(5-7)CreatingMFAError(14-19)src/core/usecases/driven/sending_mfa_hash.driven.ts (1)
SendingMfaHash(3-11)src/core/usecases/mfa.usecase.ts (1)
MFA(27-88)
test/integration/http/reset_password.route.test.ts (6)
src/core/config/database.ts (1)
Knex(26-26)test/fixtures/user.ts (1)
UserFixture(13-23)src/core/config/cache.ts (1)
RedisClient(6-6)test/loading/k6.js (1)
token(60-60)test/fixtures/generators.ts (1)
passwordGenerator(39-51)src/core/services/password.service.ts (1)
compare(11-13)
test/unit/usecases/token.usecase.test.ts (5)
src/core/entities/user.ts (1)
User(1-6)src/core/usecases/driven/decoding_token.driven.ts (1)
DecodingToken(1-3)src/core/usecases/driven/finding_user.driven.ts (1)
FindingUser(3-8)src/core/usecases/driven/creating_token.driven.ts (1)
CreatingToken(3-5)src/core/usecases/driven/invalidating_token.driven.ts (1)
InvalidatingToken(1-3)
test/unit/services/code.services.test.ts (1)
src/core/services/code.service.ts (1)
CodeService(3-14)
test/unit/usecases/logout.usecase.test.ts (3)
src/core/usecases/driven/invalidating_token.driven.ts (1)
InvalidatingToken(1-3)src/core/providers/token.repository.ts (1)
TokenRepository(11-35)src/core/usecases/logout.usecase.ts (1)
Logout(9-20)
test/unit/usecases/user.usecase.test.ts (4)
src/core/usecases/driven/finding_user.driven.ts (1)
FindingUser(3-8)src/core/usecases/driven/creating_user.driven.ts (2)
CreatingUser(1-3)CreatingUserErrors(9-14)src/core/usecases/driven/updating_user.driven.ts (2)
UpdatingUser(3-10)UpdatingUserErrors(16-21)src/core/usecases/user.usecase.ts (2)
UserUsecase(23-86)list(83-85)
src/presentation/http/routes/organization.route.ts (1)
src/core/index.ts (1)
getCore(50-186)
test/unit/usecases/organization.usecase.test.ts (8)
src/core/usecases/driven/creating_organization.driven.ts (1)
CreatingOrganization(1-3)src/core/providers/organization.repository.ts (1)
OrganizationRepository(37-156)src/core/usecases/driven/finding_user.driven.ts (2)
FindingUser(3-8)FindingUserErrors(16-21)src/core/providers/user.repository.ts (1)
UserRepository(35-247)src/core/usecases/driven/adding_user_to_organization.driven.ts (2)
AddingUserToOrganization(1-3)AddingUserToOrganizationErrors(10-15)src/core/usecases/driven/updating_organization.driven.ts (1)
UpdatingOrganization(3-13)src/core/usecases/driven/finding_organization.driven.ts (2)
FindingOrganization(3-5)FindingOrganizationErrors(11-16)src/core/usecases/organization.usecase.ts (1)
OrganizationUseCase(38-126)
test/integration/http/logout.route.test.ts (3)
src/core/config/cache.ts (1)
RedisClient(6-6)test/loading/k6.js (1)
token(60-60)test/fixtures/generators.ts (1)
tokenGenerator(18-29)
src/presentation/http/routes/logout.route.ts (2)
src/presentation/http/middlewares/jwt.ts (1)
retriveToken(17-25)src/core/index.ts (1)
getCore(50-186)
test/unit/providers/notification.provider.test.ts (4)
src/core/config/database.ts (1)
Knex(26-26)test/fixtures/setup_migration.ts (1)
setupDB(4-74)test/fixtures/user.ts (1)
insertUserIntoDatabase(25-52)test/fixtures/user_info.ts (1)
insertUserInfoIntoDatabase(12-35)
test/integration/http/login.route.test.ts (4)
src/core/config/database.ts (1)
Knex(26-26)src/core/config/cache.ts (1)
RedisClient(6-6)test/fixtures/user.ts (1)
UserFixture(13-23)src/core/providers/mfa_code.repository.ts (1)
CacheCode(19-23)
src/presentation/http/routes/user.route.ts (1)
src/core/index.ts (1)
getCore(50-186)
test/unit/providers/organization.repository.test.ts (4)
src/core/config/database.ts (1)
Knex(26-26)test/fixtures/organization.ts (1)
insertOrgIntoDatabase(9-29)test/fixtures/user.ts (1)
insertUserIntoDatabase(25-52)src/core/entities/organization.ts (1)
Organization(1-5)
src/presentation/http/middlewares/jwt.ts (1)
src/config/enviroment_config.ts (1)
getEnv(50-78)
test/unit/providers/mfa_choose.repository.test.ts (3)
src/core/config/cache.ts (1)
RedisClient(6-6)src/core/services/uuid.service.ts (1)
UuidService(3-8)src/core/providers/mfa_choose.repository.ts (1)
MFAChooseRepository(11-41)
test/unit/services/uuid.services.test.ts (1)
src/core/services/uuid.service.ts (1)
UuidService(3-8)
src/core/usecases/mfa.usecase.ts (1)
src/core/usecases/user.usecase.ts (1)
list(83-85)
src/core/config/cache.ts (1)
src/config/enviroment_config.ts (1)
getEnv(50-78)
test/integration/http/mfa.route.test.ts (3)
src/core/config/database.ts (1)
Knex(26-26)src/core/config/cache.ts (1)
RedisClient(6-6)test/fixtures/multi_factor_authentication.ts (1)
insertMfaIntoDatabase(11-29)
test/unit/usecases/reset_password.usecase.test.ts (6)
test/fixtures/generators.ts (1)
tokenGenerator(18-29)src/core/usecases/driven/decoding_token.driven.ts (1)
DecodingToken(1-3)src/core/usecases/driven/finding_user.driven.ts (1)
FindingUser(3-8)src/core/usecases/driven/creating_token.driven.ts (1)
CreatingToken(3-5)src/core/usecases/driven/invalidating_token.driven.ts (1)
InvalidatingToken(1-3)src/core/usecases/token.usecase.ts (1)
TokenUsecase(13-42)
src/presentation/tracing.ts (1)
src/config/enviroment_config.ts (1)
getEnv(50-78)
test/integration/http/organization.route.test.ts (5)
src/core/config/database.ts (1)
Knex(26-26)test/fixtures/user.ts (1)
UserFixture(13-23)src/core/config/cache.ts (1)
RedisClient(6-6)test/loading/k6.js (1)
token(60-60)test/fixtures/organization.ts (1)
insertOrgIntoDatabase(9-29)
test/unit/providers/mfa_code.repository.test.ts (4)
src/core/config/cache.ts (1)
RedisClient(6-6)src/core/services/uuid.service.ts (1)
UuidService(3-8)src/core/services/code.service.ts (1)
CodeService(3-14)src/core/providers/mfa_code.repository.ts (1)
MFACodeRepository(25-73)
test/unit/providers/user.repository.test.ts (3)
test/fixtures/generators.ts (1)
passwordGenerator(39-51)test/fixtures/user.ts (1)
insertUserIntoDatabase(25-52)test/fixtures/user_info.ts (1)
insertUserInfoIntoDatabase(12-35)
src/core/index.ts (6)
src/config/enviroment_config.ts (1)
getEnv(50-78)src/core/usecases/driven/invalidating_token.driven.ts (1)
InvalidatingToken(1-3)src/core/usecases/driven/creating_token.driven.ts (1)
CreatingToken(3-5)src/core/usecases/login.usecase.ts (2)
login(25-61)Login(17-62)src/core/usecases/organization.usecase.ts (1)
OrganizationUseCase(38-126)src/core/usecases/token.usecase.ts (1)
TokenUsecase(13-42)
src/presentation/http/routes/login.route.ts (1)
src/core/index.ts (1)
getCore(50-186)
test/fixtures/user.ts (1)
test/fixtures/generators.ts (1)
passwordGenerator(39-51)
src/presentation/http/middlewares/trace.ts (1)
src/config/enviroment_config.ts (1)
getEnv(50-78)
src/presentation/http/routes/mfa.route.ts (1)
src/core/index.ts (1)
getCore(50-186)
src/core/providers/organization.repository.ts (1)
src/core/entities/organization.ts (1)
Organization(1-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
| "joi": "17.13.3", | ||
| "jsonwebtoken": "9.0.2", | ||
| "dotenv": "17.2.3", | ||
| "express": "5.2.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Express.js 5 breaking changes from Express 4 migration guide
💡 Result:
Here are the main breaking changes when migrating from Express 4 → Express 5 (summary of the official migration guide):
- Removed / changed APIs
- app.del() removed — use app.delete(). [1]
- app.param(fn) and router.param(fn) signatures removed; leading-colon name in app.param(name, fn) deprecated. [1]
- req.param(name) removed. [1]
- res.json(obj, status) signature removed (use res.status(...).json(...)). [1]
- res.sendfile() renamed to res.sendFile() and uses mime-types (several Content-Type values changed). [1]
- Pluralized accept* methods removed: use req.acceptsCharsets(), req.acceptsEncodings(), req.acceptsLanguages(). [1]
- express.static.mime no longer exported; use mime-types. [1]
- Path matching / routing changes
- Wildcard * must be named (use /*splat or /{*splat}); optional ? in path strings no longer supported — use brace syntax. [1]
- Behavior / defaults changed
- express.urlencoded() now defaults extended: false. [1]
- express.static dotfiles option now defaults to "ignore" (dotfiles served by default in v4). [1]
- req.body is undefined when body not parsed (was {} in v4). [1]
- req.query is now a getter (not writable); default query parser changed to "simple". [1]
- req.host retains port now (v4 stripped port). [1]
- res.clearCookie ignores maxAge/expires options. [1]
- res.status only accepts integers 100–999 and errors on non-integers. [1]
- res.vary throws when field argument is missing (was console warning). [1]
- app.listen callback now receives an error on server 'error' events instead of throwing. [1]
- app.router exists again as reference to base router. [1]
- Platform / runtime
- Minimum Node.js requirement raised (Node 18+). [2]
Notes:
- The Express team provides codemods to automate many changes. Test your app and run the official codemods linked in the migration guide. [1]
Sources: Express official “Migrating to Express 5” and v5 release notes. [1][2]
References
[1] Express — Migrating to Express 5 (official guide).
[2] Express — v5 release announcement / requirements.
Express 5.x includes significant breaking changes from Express 4.x.
Key breaking changes to address include:
- Removed methods:
app.del()(useapp.delete()),req.param(), pluralizedaccept*methods - Modified signatures:
res.json(obj, status)→res.status(...).json(...),res.sendfile()→res.sendFile() - Routing changes: Wildcard
*must be named (e.g.,/*splat), optional?syntax no longer supported - Behavior changes:
express.urlencoded()defaults toextended: false,express.staticnow ignores dotfiles by default,req.queryis now read-only,req.bodyisundefined(not{}) when not parsed - Runtime requirement: Node.js 18+ is now required
Review all route handlers, middleware, and request/response usage for compatibility. The Express team provides codemods to automate many migrations.
🤖 Prompt for AI Agents
In package.json around line 42, Express is pinned to 5.2.1 but our codebase
likely uses Express 4.x APIs; update code for Express 5 compatibility or revert
to a 4.x release. Either: (A) change package.json to an Express 4.x compatible
version to avoid breaking changes, or (B) migrate the codebase: replace
app.del() with app.delete(), remove req.param() usages, update res.json(obj,
status) to res.status(...).json(...), change res.sendfile() to res.sendFile(),
update wildcard routes to named params (e.g., /*splat), stop using optional ?
routing syntax, explicitly set express.urlencoded({ extended: true/false }) as
needed, adjust express.static options for dotfiles if required, treat req.query
as read-only and ensure body parsers are registered so req.body is not
undefined, and ensure package.json engines or CI use Node.js 18+. Run the
Express 5 codemods to automate changes and run full tests to validate behavior.
| const grandParentRow = await this.database<OrganizationRow>( | ||
| 'organization' | ||
| ).where('id', parent.parentOrganizationId) | ||
| const grandParent: Organization = { | ||
| id: grandParentRow[0].id, | ||
| name: grandParentRow[0].name, | ||
| parentOrganizationId: grandParentRow[0].parent_organization_id, | ||
| } | ||
| 'organization' | ||
| ).where('id', parent.parentOrganizationId), | ||
| grandParent: Organization = { | ||
| id: grandParentRow[0].id, | ||
| name: grandParentRow[0].name, | ||
| parentOrganizationId: grandParentRow[0].parent_organization_id, | ||
| } |
There was a problem hiding this comment.
Missing existence check for grandParentRow before array access.
If the database query returns an empty array (e.g., due to data inconsistency or a race condition where the grandparent was deleted), accessing grandParentRow[0].id will cause a runtime error.
Apply this diff to add defensive error handling:
const grandParentRow = await this.database<OrganizationRow>(
'organization'
- ).where('id', parent.parentOrganizationId),
- grandParent: Organization = {
+ ).where('id', parent.parentOrganizationId)
+ if (grandParentRow.length === 0) {
+ throw new FindingOrganizationErrors(
+ FindingOrganizationErrorsTypes.ORGANIZATION_NOT_FOUND
+ )
+ }
+ const grandParent: Organization = {
id: grandParentRow[0].id,
name: grandParentRow[0].name,
parentOrganizationId: grandParentRow[0].parent_organization_id,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const grandParentRow = await this.database<OrganizationRow>( | |
| 'organization' | |
| ).where('id', parent.parentOrganizationId) | |
| const grandParent: Organization = { | |
| id: grandParentRow[0].id, | |
| name: grandParentRow[0].name, | |
| parentOrganizationId: grandParentRow[0].parent_organization_id, | |
| } | |
| 'organization' | |
| ).where('id', parent.parentOrganizationId), | |
| grandParent: Organization = { | |
| id: grandParentRow[0].id, | |
| name: grandParentRow[0].name, | |
| parentOrganizationId: grandParentRow[0].parent_organization_id, | |
| } | |
| const grandParentRow = await this.database<OrganizationRow>( | |
| 'organization' | |
| ).where('id', parent.parentOrganizationId) | |
| if (grandParentRow.length === 0) { | |
| throw new FindingOrganizationErrors( | |
| FindingOrganizationErrorsTypes.ORGANIZATION_NOT_FOUND | |
| ) | |
| } | |
| const grandParent: Organization = { | |
| id: grandParentRow[0].id, | |
| name: grandParentRow[0].name, | |
| parentOrganizationId: grandParentRow[0].parent_organization_id, | |
| } |
🤖 Prompt for AI Agents
In src/core/providers/organization.repository.ts around lines 146 to 153, the
code assumes grandParentRow[0] exists and will crash if the query returns an
empty array; add a defensive existence check immediately after the DB query
(e.g., if (!grandParentRow || grandParentRow.length === 0) throw a controlled
error or return null/undefined per repository convention), then map fields from
grandParentRow[0] to the grandParent object (id, name, parent_organization_id ->
parentOrganizationId) only after confirming the row exists so you avoid runtime
array-access errors.
|
|
||
| beforeAll(async () => { | ||
| redisContainer = await new RedisContainer().start() | ||
| redisContainer = await new RedisContainer('redis:7.0.5').start() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Redis versions used across test files
rg -n "RedisContainer\(['\"]redis:" --type=ts
# Also check for any other redis image references
rg -n "redis:" --type=ts | head -20
# Check package.json for redis-related dependencies
cat package.json | grep -A 5 -B 5 "redis" || echo "No redis found in package.json"
# Check if there's documentation about Redis version requirements
fd -e md . -x grep -l "redis" {} \; 2>/dev/nullRepository: auth-plus/auth-plus-authentication
Length of output: 3396
🏁 Script executed:
#!/bin/bash
# Check for documentation mentioning Redis version
fd -e md . -x grep -l "redis" {} \; 2>/dev/null
# Check docker-related files
find . -name "docker*" -o -name "*compose*" 2>/dev/null | head -20
# Check README and contribution guidelines
cat README.md 2>/dev/null | grep -i redis -A 3 -B 3 || echo "No redis info in README"
# Check if there's a .env or config file specifying Redis
fd -e env . -x grep -i redis {} \; 2>/dev/null | head -10Repository: auth-plus/auth-plus-authentication
Length of output: 122
🏁 Script executed:
cat docker-compose.ymlRepository: auth-plus/auth-plus-authentication
Length of output: 1623
Update Redis container version in tests to match docker-compose configuration.
Tests are pinned to redis:7.0.5, but docker-compose.yml specifies redis:7.4.6 for the development environment. Update all test Redis containers to redis:7.4.6 to ensure tests run against the actual development and production Redis version.
Affected test files:
- test/integration/http/logout.route.test.ts
- test/integration/http/login.route.test.ts
- test/integration/http/user.route.test.ts
- test/integration/http/organization.route.test.ts
- test/integration/http/reset_password.route.test.ts
- test/integration/http/mfa.route.test.ts
- test/unit/providers/mfa_code.repository.test.ts
- test/unit/providers/mfa_choose.repository.test.ts
🤖 Prompt for AI Agents
In test/integration/http/logout.route.test.ts around line 12, change the
RedisContainer image tag from 'redis:7.0.5' to 'redis:7.4.6' to match
docker-compose; do the same replacement in the other affected test files listed
(test/integration/http/login.route.test.ts,
test/integration/http/user.route.test.ts,
test/integration/http/organization.route.test.ts,
test/integration/http/reset_password.route.test.ts,
test/integration/http/mfa.route.test.ts,
test/unit/providers/mfa_code.repository.test.ts,
test/unit/providers/mfa_choose.repository.test.ts), and then run the test suite
to verify everything starts correctly with redis:7.4.6.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/core/usecases/reset_password.usecase.ts (1)
41-42: Consider separate declarations for clarity.The comma-separated variable declarations are valid but could be more readable with separate
conststatements:- const email = await this.findingResetPassword.findByHash(hash), - user = await this.findingUser.findByEmail(email) + const email = await this.findingResetPassword.findByHash(hash) + const user = await this.findingUser.findByEmail(email)test/unit/usecases/reset_password.usecase.test.ts (1)
16-17: Consider separate declarations for consistency and clarity.Similar to the main code, these comma-separated declarations could be split for improved readability.
- const hash = casual.uuid, - userId = casual.uuid, - user: User = { + const hash = casual.uuid + const userId = casual.uuid + const user: User = {src/core/providers/notification.provider.ts (2)
73-87: Async handling insendCodeByStrategyis improved; consider explicit GA caseAwaiting
sendCodeByEmailandsendCodeByPhoneis a good fix — callers now reliably wait for completion and see errors from the underlying produce calls.For readability and future-proofing, you might consider handling
Strategy.GAexplicitly instead of using thedefaultbranch for GA and any future strategies. That would avoid accidentally throwingGA_SENT_CODEif a new strategy is added but this switch isn’t updated.switch (strategy) { case Strategy.EMAIL: await this.sendCodeByEmail(userId, code) break case Strategy.PHONE: await this.sendCodeByPhone(userId, code) break + case Strategy.GA: + throw new SendingMfaCodeErrors(SendingMfaCodeErrorsTypes.GA_SENT_CODE) default: - throw new SendingMfaCodeErrors(SendingMfaCodeErrorsTypes.GA_SENT_CODE) + // optionally: no-op or a more generic "unknown strategy" error + break }
132-147: AlignsendMfaHashByStrategyasync behavior withsendCodeByStrategy
sendMfaHashByStrategyisasyncbut neverawaits orreturns the calls tosendMfaHashByEmail/sendMfaHashByPhone. As a result:
- The method resolves immediately, even if the underlying send fails or is still in flight.
- Errors from the inner async calls can surface as unhandled rejections.
To match the improved pattern in
sendCodeByStrategyand make the behavior less surprising for callers thatawaitthis method, consider:async sendMfaHashByStrategy( userId: string, hash: string, strategy: Strategy ): Promise<void> { switch (strategy) { case Strategy.EMAIL: - this.sendMfaHashByEmail(userId, hash) - break + await this.sendMfaHashByEmail(userId, hash) + break case Strategy.PHONE: - this.sendMfaHashByPhone(userId, hash) - break + await this.sendMfaHashByPhone(userId, hash) + break default: break } }test/unit/providers/notification.provider.test.ts (2)
10-14: Test setup updates look good; consider restoring the Kafka spyThe added imports for
StrategyandSendingMfaHashErrorsTypesalign with the new test cases, and pinningPostgreSqlContainertopostgres:15.1plus the simplifiedgetKafkaspy make the environment more deterministic and easier to reason about.One small improvement: since
jest.spyOn(kafka, 'getKafka')is set up inbeforeAll, you may want to restore it inafterAll(or usejest.restoreAllMocks()there) to avoid leaking this mock into other test files that might rely on the real implementation.Also applies to: 19-32
99-109: New GA and hash error tests provide useful coverage; consider adding strategy-hash testsThe new tests:
should fail when send code by GA(expectingSendingMfaCodeErrorsTypes.GA_SENT_CODE),should fail when send hash by email but user not found(expectingUSER_EMAIL_HASH_NOT_FOUND), andshould fail when send hash by Phone(expectingUSER_PHONE_HASH_NOT_FOUND),correctly validate the new/error paths introduced in
NotificationProviderand the hash-related enums.As a follow-up, you might also add tests around
sendMfaHashByStrategyitself (EMAIL/PHONE/GA) to mirror thesendCodeByStrategycoverage and catch regressions in that dispatcher logic.Also applies to: 111-117, 119-125
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
coverage/lcov.info(44 hunks)src/core/providers/notification.provider.ts(2 hunks)src/core/services/uuid.service.ts(1 hunks)src/core/usecases/driven/sending_mfa_code.driven.ts(1 hunks)src/core/usecases/reset_password.usecase.ts(2 hunks)test/unit/providers/notification.provider.test.ts(2 hunks)test/unit/usecases/reset_password.usecase.test.ts(1 hunks)test/unit/usecases/token.usecase.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- coverage/lcov.info
- test/unit/usecases/token.usecase.test.ts
- src/core/services/uuid.service.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/core/providers/notification.provider.ts (1)
src/core/usecases/driven/sending_mfa_code.driven.ts (1)
SendingMfaCodeErrors(19-24)
test/unit/usecases/reset_password.usecase.test.ts (8)
src/core/entities/user.ts (1)
User(1-6)src/core/usecases/driven/creating_reset_password.driven.ts (1)
CreatingResetPassword(1-3)src/core/providers/reset_password.repository.ts (1)
ResetPasswordRepository(10-35)src/core/usecases/driven/sending_reset_email.driven.ts (1)
SendingResetEmail(1-3)src/core/providers/notification.provider.ts (1)
NotificationProvider(19-169)src/core/usecases/driven/finding_reset_password.driven.ts (1)
FindingResetPassword(1-3)src/core/providers/user.repository.ts (1)
UserRepository(35-247)src/core/usecases/reset_password.usecase.ts (1)
ResetPasswordUseCase(18-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
src/core/usecases/reset_password.usecase.ts (1)
32-32: Good improvement: awaiting the email operation.Changing from fire-and-forget to awaited ensures the email-sending operation completes and any errors are properly caught by the try-catch block.
src/core/usecases/driven/sending_mfa_code.driven.ts (1)
13-17: Enum extension for GA_SENT_CODE looks consistentAdding
GA_SENT_CODEtoSendingMfaCodeErrorsTypesis type-safe and integrates cleanly withSendingMfaCodeErrors. Downstream usage inNotificationProviderand tests makes the intent clear.src/core/providers/notification.provider.ts (1)
149-155: RESET_PASSWORD payload shorthand is fineUsing object property shorthand for
{ email, hash }keepssendEmailconsistent with other producers and has no behavioral impact.test/unit/providers/notification.provider.test.ts (1)
53-60: Refactored email/phone tests remain clear and correctThe refactoring to chained
constdeclarations doesn’t change behavior, and the tests for:
- successful email send,
- email user not found,
- successful SMS send, and
- missing phone,
still correctly exercise
NotificationProvider’s email/phone code paths and the correspondingSendingMfaCodeErrorsTypesvalues.Also applies to: 65-67, 72-76, 82-87, 91-97
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/loading/k6.js (1)
47-61: GuardJSON.parsewith a status check to avoid brittle failures
JSON.parse(resLogin.body)runs before verifyingresLogin.status, so a failed or non‑JSON response will throw and abort the whole script before thecheckruns.Consider checking the status first and only parsing on success, e.g.:
- const resLogin = http.post( + const resLogin = http.post( @@ - ) - const token = JSON.parse(resLogin.body).token - check(resLogin, { 'Login status was 200': (r) => r.status == 200 }) + ) + const loginOk = check(resLogin, { + 'Login status was 200': (r) => r.status === 200, + }) + if (!loginOk) { + throw new Error(`Login failed with status ${resLogin.status}`) + } + const token = JSON.parse(resLogin.body).tokentest/integration/http/user.route.test.ts (1)
83-83:redis.del('*')does not delete keys by pattern.Same issue as other integration tests. Use
flushDb()to clear the cache.beforeEach(async () => { - redis.del('*') + await redis.flushDb() })test/integration/http/reset_password.route.test.ts (1)
84-84:redis.del('*')does not delete keys by pattern.Same issue as other integration tests. Use
flushDb()to clear the cache.beforeEach(async () => { - redis.del('*') + await redis.flushDb() })test/integration/http/organization.route.test.ts (1)
20-35: Redis cache cleanup needs correctionThe container setup and Redis client wiring look solid. However, cache cleanup in
beforeEachis incorrect:
redis.del('*')deletes only the literal key"*", not all keys. It does not clear the cache between tests.- Even if it worked, the operation is not awaited, so cleanup may not complete before the next test starts.
Use
await redis.flushDb()instead to clear the current database asynchronously:beforeEach(async () => { await database('organization').del() - redis.del('*') + await redis.flushDb() })This ensures deterministic test isolation and prevents cross-test cache interference.
Also applies to: 60-68, 82-85
♻️ Duplicate comments (2)
package.json (1)
42-42:⚠️ Express 5.2.1 requires extensive codebase verification for breaking changes.Express 5 maintains the same basic API as Express 4, but there are changes that break compatibility; applications built with Express 4 might not work if updated to Express 5. Key breaking changes include removal of
res.json(obj, status)signature (useres.status(status).json(obj)instead), changedexpress.staticdotfiles default from served to "ignore", and routing changes using [email protected] that remove sub-expression regex patterns for security.This is a duplicate of a previously flagged critical issue. Verify full test suite passes and review route handlers, middleware, and request/response usage for compatibility before merge.
test/unit/usecases/reset_password.usecase.test.ts (1)
28-101: Error scenario test coverage still missing.While test coverage for the
recovermethod has been added, the test suite still lacks error scenario tests for bothforgetandrecovermethods (e.g., when dependencies throw errors). Both methods catch errors and throw custom error types (ForgetPasswordErrorsandRecoverPasswordErrors) that should be validated.
🧹 Nitpick comments (7)
test/unit/services/password.services.test.ts (1)
8-8: Consider consistent property access style.This line uses destructuring (
const { password } = casual), while Lines 16-17 use direct property access (casual.full_name,casual.email). For consistency and readability, consider using the same approach throughout the file.Apply this diff to make the style consistent with the rest of the file:
- const { password } = casual + const password = casual.passwordAlternatively, update Lines 16-17 to also use destructuring for consistency.
test/loading/k6.js (1)
47-79: Avoid unnecessaryasync/awaitand meaninglessPromise.all
setupand the default export are markedasync, but there are no real async operations;listUseris an array of response bodies (strings), soawait Promise.all(listUser)is effectively a no‑op and can confuse readers.You can simplify and better match k6’s synchronous execution model like this:
-export async function setup() { +export function setup() { @@ - const listUser = USERS.map((u) => { - const resUser = http.post(`${BASE_URL}/user`, JSON.stringify(u), { - headers: { - 'Content-Type': 'application/json', - Authorization: `Bearer ${token}`, - }, - }) - check(resUser, { 'Create status was 200': (r) => r.status == 200 }) - return resUser.body - }) - await Promise.all(listUser) + USERS.forEach((u) => { + const resUser = http.post(`${BASE_URL}/user`, JSON.stringify(u), { + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${token}`, + }, + }) + check(resUser, { 'Create status was 200': (r) => r.status == 200 }) + }) } -export default async function () { +export default function () { const resHealth = http.get(`${BASE_URL}/health`) check(resHealth, { 'health status is 200': (r) => r.status == 200 }) sleep(1) }If you’re relying on any newer k6 features that explicitly support async/await, double‑check the current k6 docs to ensure this refactor aligns with your runtime expectations.
test/unit/providers/mfa_choose.repository.test.ts (2)
17-23: Pinned Redis image improves test determinismUsing
new RedisContainer('redis:7.0.5')is a good move for keeping the test environment stable across machines/CI. If other tests also hardcode the same image string, consider centralizing this tag in a shared test config to avoid divergence later, but not required for this PR.
30-50: MFAChooseRepository tests correctly exercise UUID and Redis behaviorBoth tests are well-structured:
createtest correctly stubsUuidService.generateHash(), verifies it's called once, and asserts the returned hash.findByHashseeds Redis directly, ensuresgenerateHash()is never called, and validates the deserialized payload.This gives good coverage of the happy paths for both methods and aligns with the updated
UuidServiceimplementation. If you want to DRY things up later, you could extract a small helper to constructMFAChooseRepository(redis)to reduce duplication across tests.src/presentation/tracing.ts (3)
19-21: Consider removing the emptyregisterInstrumentationscall.Since instrumentations are now configured directly in the NodeSDK setup (line 33), this empty registration appears redundant and can be removed for clarity.
Apply this diff to remove the dead code:
-// Optionally register instrumentation libraries -registerInstrumentations({ - instrumentations: [], -}) -
23-26: Consider sourcing the service version dynamically.The version is currently hardcoded as
'0.1.0'. For better maintainability, consider reading it frompackage.jsonor an environment variable to keep it in sync with releases.Example approach:
import packageJson from '../../package.json' const resource = resourceFromAttributes({ [ATTR_SERVICE_NAME]: getEnv().app.name, [ATTR_SERVICE_VERSION]: packageJson.version, })
36-36: Add error handling for SDK initialization.Consider adding error handling around
sdk.start()to catch and log initialization failures. This improves observability when tracing setup encounters issues.-sdk.start() +sdk + .start() + .catch((error) => { + console.error('Error initializing OpenTelemetry SDK:', error) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (50)
eslint.config.mts(1 hunks)package.json(2 hunks)src/core/config/cache.ts(1 hunks)src/core/index.ts(0 hunks)src/core/providers/mfa.repository.ts(2 hunks)src/core/providers/organization.repository.ts(2 hunks)src/core/providers/user.repository.ts(5 hunks)src/core/usecases/login.usecase.ts(1 hunks)src/core/usecases/mfa.usecase.ts(0 hunks)src/core/usecases/mfa_code.usecase.ts(0 hunks)src/core/usecases/reset_password.usecase.ts(1 hunks)src/core/usecases/token.usecase.ts(0 hunks)src/core/usecases/user.usecase.ts(2 hunks)src/presentation/http/middlewares/jwt.ts(3 hunks)src/presentation/http/middlewares/trace.ts(1 hunks)src/presentation/http/routes/login.route.ts(2 hunks)src/presentation/http/routes/logout.route.ts(1 hunks)src/presentation/http/routes/mfa.route.ts(1 hunks)src/presentation/http/routes/organization.route.ts(1 hunks)src/presentation/http/routes/reset_password.route.ts(1 hunks)src/presentation/http/routes/user.route.ts(1 hunks)src/presentation/tracing.ts(2 hunks)test/fixtures/generators.ts(1 hunks)test/fixtures/organization.ts(1 hunks)test/fixtures/user.ts(2 hunks)test/fixtures/user_info.ts(1 hunks)test/integration/http/login.route.test.ts(7 hunks)test/integration/http/logout.route.test.ts(2 hunks)test/integration/http/mfa.route.test.ts(3 hunks)test/integration/http/organization.route.test.ts(2 hunks)test/integration/http/reset_password.route.test.ts(3 hunks)test/integration/http/user.route.test.ts(2 hunks)test/loading/k6.js(1 hunks)test/unit/providers/mfa.repository.test.ts(3 hunks)test/unit/providers/mfa_choose.repository.test.ts(3 hunks)test/unit/providers/mfa_code.repository.test.ts(2 hunks)test/unit/providers/notification.provider.test.ts(2 hunks)test/unit/providers/organization.repository.test.ts(2 hunks)test/unit/providers/user.repository.test.ts(4 hunks)test/unit/services/password.services.test.ts(1 hunks)test/unit/services/uuid.services.test.ts(1 hunks)test/unit/usecases/login.usecase.test.ts(2 hunks)test/unit/usecases/logout.usecase.test.ts(1 hunks)test/unit/usecases/mfa.usecase.test.ts(2 hunks)test/unit/usecases/mfa_choose.usecase.test.ts(2 hunks)test/unit/usecases/mfa_code.usecase.test.ts(3 hunks)test/unit/usecases/organization.usecase.test.ts(2 hunks)test/unit/usecases/reset_password.usecase.test.ts(2 hunks)test/unit/usecases/token.usecase.test.ts(2 hunks)test/unit/usecases/user.usecase.test.ts(4 hunks)
💤 Files with no reviewable changes (4)
- src/core/usecases/mfa.usecase.ts
- src/core/usecases/mfa_code.usecase.ts
- src/core/index.ts
- src/core/usecases/token.usecase.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- test/unit/services/uuid.services.test.ts
- src/core/providers/mfa.repository.ts
- test/unit/usecases/logout.usecase.test.ts
- test/unit/usecases/mfa_code.usecase.test.ts
- test/fixtures/generators.ts
- src/core/providers/user.repository.ts
- test/unit/providers/mfa_code.repository.test.ts
- test/unit/usecases/token.usecase.test.ts
- src/presentation/http/middlewares/trace.ts
- test/unit/usecases/organization.usecase.test.ts
- src/core/usecases/login.usecase.ts
- eslint.config.mts
- test/integration/http/logout.route.test.ts
- src/core/usecases/user.usecase.ts
- test/unit/usecases/mfa_choose.usecase.test.ts
- test/fixtures/user.ts
- src/presentation/http/routes/login.route.ts
- src/core/providers/organization.repository.ts
- src/presentation/http/routes/organization.route.ts
- test/unit/usecases/mfa.usecase.test.ts
- test/unit/providers/mfa.repository.test.ts
🧰 Additional context used
🧬 Code graph analysis (11)
test/integration/http/user.route.test.ts (3)
test/fixtures/user.ts (1)
UserFixture(13-23)src/core/config/cache.ts (1)
RedisClient(5-5)test/loading/k6.js (1)
token(60-60)
test/integration/http/login.route.test.ts (2)
src/core/config/cache.ts (1)
RedisClient(5-5)test/fixtures/user.ts (1)
UserFixture(13-23)
test/integration/http/organization.route.test.ts (3)
test/fixtures/user.ts (1)
UserFixture(13-23)src/core/config/cache.ts (1)
RedisClient(5-5)test/loading/k6.js (1)
token(60-60)
test/unit/providers/organization.repository.test.ts (2)
src/core/entities/organization.ts (1)
Organization(1-5)src/core/providers/organization.repository.ts (1)
OrganizationRepository(37-156)
src/presentation/tracing.ts (1)
src/config/enviroment_config.ts (1)
getEnv(50-78)
test/unit/providers/notification.provider.test.ts (3)
src/core/config/database.ts (1)
Knex(26-26)test/fixtures/setup_migration.ts (1)
setupDB(4-74)src/core/providers/notification.provider.ts (1)
NotificationProvider(19-169)
test/integration/http/mfa.route.test.ts (1)
src/core/config/cache.ts (1)
RedisClient(5-5)
test/unit/usecases/reset_password.usecase.test.ts (8)
src/core/usecases/driven/creating_reset_password.driven.ts (1)
CreatingResetPassword(1-3)src/core/providers/reset_password.repository.ts (1)
ResetPasswordRepository(10-35)src/core/usecases/driven/sending_reset_email.driven.ts (1)
SendingResetEmail(1-3)src/core/providers/notification.provider.ts (1)
NotificationProvider(19-169)src/core/usecases/driven/finding_reset_password.driven.ts (1)
FindingResetPassword(1-3)src/core/usecases/driven/finding_user.driven.ts (1)
FindingUser(3-8)src/core/usecases/driven/updating_user.driven.ts (1)
UpdatingUser(3-10)src/core/usecases/reset_password.usecase.ts (1)
ResetPasswordUseCase(18-51)
src/presentation/http/middlewares/jwt.ts (2)
test/loading/k6.js (1)
token(60-60)src/config/enviroment_config.ts (1)
getEnv(50-78)
test/unit/providers/mfa_choose.repository.test.ts (2)
src/core/services/uuid.service.ts (1)
UuidService(3-7)src/core/providers/mfa_choose.repository.ts (1)
MFAChooseRepository(11-41)
test/integration/http/reset_password.route.test.ts (3)
test/fixtures/user.ts (1)
UserFixture(13-23)src/core/config/cache.ts (1)
RedisClient(5-5)test/loading/k6.js (1)
token(60-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (36)
test/unit/usecases/user.usecase.test.ts (3)
2-2: LGTM!The import reordering to alphabetical order aligns with the eslint-plugin-simple-import-sort configuration mentioned in the PR objectives.
31-31: LGTM!Using an arrow function for the
describecallback is consistent with modern JavaScript conventions and the updated ESLint configuration.
131-131: LGTM!Destructuring
{ phone }fromcasualworks correctly since the casual library uses getters for value generation. The destructuring accesses the property once and stores the generated value, maintaining the same behavior as direct property access.Also applies to: 174-174
test/unit/usecases/login.usecase.test.ts (2)
2-2: LGTM! Import ordering improvement.The ts-mockito imports are now alphabetically sorted, aligning with the project's adoption of
eslint-plugin-simple-import-sortas mentioned in the PR objectives.
29-29: LGTM! Modernized describe block syntax.The conversion to arrow function syntax is consistent with the project-wide formatting improvements. Since the test suite doesn't rely on
thiscontext, this change is safe and improves code consistency.package.json (4)
10-11: Lint scripts properly consolidated with ESLint then Prettier. ✓Aligns well with the PR objective to update lint configuration. The order (ESLint --fix first, then Prettier --write) follows best practices by allowing ESLint fixes to settle before formatting.
27-27: Node.js engine correctly updated to 24.x LTS. ✓Aligns with PR objective and current LTS versions. Ensure .nvmrc is also updated (as noted in AI summary, this appears to be done).
38-38: Verify test suite passes with major dependency version bumps.Multiple critical dependencies are being updated to major versions: bcrypt 6.0.0, jsonwebtoken 9.0.3, helmet 8.1.0, pg 8.16.3, redis 5.10.0, winston 3.19.0, jest 30.2.0, typescript 5.9.3, and typescript-eslint 8.49.0. Each major version bump can introduce breaking changes. The consolidated dependency updates are appropriate for a package refresh PR, but verification is critical.
Run the full test suite (
npm run testandnpm run stryker) to confirm all tests pass with the new dependency versions before merge. Additionally, spot-check any usage of jsonwebtoken and bcrypt APIs against the latest docs to ensure compatibility.Also applies to: 44-44, 45-45, 49-49, 51-51, 53-53, 79-79, 86-86, 92-92, 95-95, 96-96
29-55: UUID npm package has been properly removed from dependencies.The uuid npm package is confirmed absent from package.json. The UuidService references appearing in the codebase (imports in repositories and exports in core/index.ts) are for an internal custom service class, not the external npm package. The dependency removal aligns with the PR objective and is complete.
src/core/config/cache.ts (1)
1-5: LGTM on simplified import and type alias.Using
ReturnType<typeof createClient>is more maintainable—it automatically stays in sync with the redis library's return type without requiring manual updates when the library changes.src/core/usecases/reset_password.usecase.ts (1)
32-32: LGTM!Awaiting the email-sending operation ensures it completes before the method returns, improving error handling and reliability. This is a good change from fire-and-forget behavior.
test/unit/usecases/reset_password.usecase.test.ts (1)
28-62: LGTM!The test correctly validates the forget flow by ensuring that only
createandsendEmailare called, while recovery-related methods are not invoked.test/loading/k6.js (1)
1-1: Import order change is safe and purely stylisticReordering
{ sleep, check }to{ check, sleep }from'k6'doesn’t affect behavior and aligns fine with typical import-sorting rules.test/unit/providers/user.repository.test.ts (3)
8-8: LGTM! Import reordering aligns with ESLint configuration updates.The alphabetical ordering of ts-mockito imports is consistent with the eslint-plugin-simple-import-sort mentioned in the PR objectives.
213-213: LGTM! Variable declaration improves code organization.Declaring
newPhonebefore the test setup improves readability and follows good practice of defining variables before use.
292-292: LGTM! Variable declaration improves code organization.Declaring
newGAbefore the test setup improves readability and maintains consistency with similar test patterns in this file.test/unit/providers/mfa_choose.repository.test.ts (1)
1-8: Imports and explicit RedisClient typing look goodReordered
ts-mockitoimports and the explicitRedisClientimport/annotation keep things clear and consistent with the rest of the codebase. No functional concerns here.src/presentation/tracing.ts (1)
2-7: LGTM! Clean migration to NodeSDK pattern.The import changes correctly support the migration from manual TracerProvider setup to the recommended NodeSDK approach, including auto-instrumentations and streamlined resource construction.
test/fixtures/organization.ts (1)
14-27: Type annotation update forrowis correct and consistent
rowbeing typed as{ id: string }[]matches thereturning('id')usage and the later accessrow[0].id; no behavioral change, clearer typing.test/fixtures/user_info.ts (1)
17-33:valuegeneration now respectsinput.valueand fixes precedence bug
const value = input?.value ?? (type === 'phone' ? casual.phone : casual.uuid)correctly prefers a providedinput.valueand only generates a fallback when it’s nullish; combined with the{ id: string }[]typing forrow, this aligns with how the result is used (row[0].id).test/unit/providers/organization.repository.test.ts (2)
26-28: Explicit PostgreSQL image tag improves test reproducibilityUsing
new PostgreSqlContainer('postgres:15.1')pins the DB version for tests, which should make runs more deterministic, assuming this version matches what you intend to support.
229-237: RenamedgrandChildOrgvariable keeps cyclic check test behavior intactThe
grandChildOrgobject structure is unchanged and passing it tocheckCyclicRelationship(rootOrg, grandChildOrg)still exercises the intended root–child–grandchild cycle scenario; this is a naming/clarity improvement only.test/unit/providers/notification.provider.test.ts (2)
24-34: LGTM - Container image pinning and simplified mock.Pinning the PostgreSQL container to
postgres:15.1ensures reproducible test environments. The Kafka mock simplification maintains the same behavior with cleaner syntax.
100-127: Good addition of error path coverage.These new test cases properly cover:
- GA strategy rejection (which shouldn't send codes externally)
- Email hash sending when user not found
- Phone hash sending when user not found
The tests correctly verify that the provider throws the appropriate error types.
test/integration/http/mfa.route.test.ts (2)
27-35: LGTM - Container setup and Redis readiness handling.Good improvements:
- Explicit image tags ensure reproducible test environments
- Redis readiness check prevents race conditions during setup
- Extracting
userIdfrom fixture improves test readability
60-68: LGTM - Kafka mock simplification.The inline mock object is cleaner while maintaining the same mock behavior.
test/integration/http/login.route.test.ts (2)
29-36: LGTM - Container setup and Redis readiness handling.Consistent with other integration tests - explicit image tags and Redis readiness check.
141-142: LGTM - Clean destructuring.Using
const { phone } = casualis more idiomatic thancasual.phonewhen the value needs to be reused.test/integration/http/user.route.test.ts (1)
27-34: LGTM - Container setup and Redis readiness handling.Consistent pattern with other integration tests.
test/integration/http/reset_password.route.test.ts (2)
28-35: LGTM - Container setup and Redis readiness handling.Consistent pattern with other integration tests.
107-107: LGTM - ES6 shorthand property.Using shorthand
hashinstead ofhash: hashis cleaner.src/presentation/http/routes/logout.route.ts (1)
1-7: Express import reordering only; behavior unchangedThis is just a normalization of the express import order/typing. The logout route handler and its error handling remain unchanged and correct.
src/presentation/http/routes/reset_password.route.ts (1)
1-7: Import normalization; reset‑password behavior unaffectedThe express import reorder keeps the existing
/forgetand/recoverhandlers and Joi validation intact. No behavioral changes here.src/presentation/http/routes/user.route.ts (1)
1-7: Express import cleanup for user routesPulling
NextFunctionandRequestHandlerinto the primary express import keeps the file consistent with other routes; handler typing and behavior are unchanged.src/presentation/http/routes/mfa.route.ts (1)
1-7: MFA route import reordering onlyThe updated express import ordering is consistent with the rest of the routes; all MFA endpoints and Joi validation logic remain unchanged.
src/presentation/http/middlewares/jwt.ts (1)
1-15: JWT refactor looks solid; verify no legacy string-payload tokens existThe changes here are coherent:
- Constraining verification via
VerifyOptionswithalgorithms: ['HS256']aligns with the default algorithm used bysignwhen given a symmetric secret.createTokenexplicitly setsexpandextractPayloadstrips standard claims (iat,exp,iss,sub,aud,jti,nbf), so callers ofremoveJwtAttronly see your domain fields (userId,now, etc.).Important behavioral change to validate:
extractPayloadnow throws whenverifyreturns a string. In jsonwebtoken, the return type is determined by what was signed: object payloads return objects, string payloads return strings. If this service previously signed any tokens with raw string payloads (rather than object payloads), those tokens will now fail when passed throughremoveJwtAttr.Before deploying, confirm that all tokens in your system were signed with object payloads (as in
createToken). If legacy string-payload tokens exist, either migrate them, add runtime type handling, or provide a clearer error message to identify problematic tokens.
| client = await createClient({ | ||
| url: url.includes('redis') ? url : `redis://${url}`, | ||
| }) |
There was a problem hiding this comment.
Weak URL scheme detection.
url.includes('redis') matches any URL containing "redis" anywhere (e.g., my-redis-server.example.com), potentially skipping the prefix when it's actually needed. Check for the scheme prefix instead:
client = await createClient({
- url: url.includes('redis') ? url : `redis://${url}`,
+ url: url.startsWith('redis://') || url.startsWith('rediss://') ? url : `redis://${url}`,
})🤖 Prompt for AI Agents
In src/core/config/cache.ts around lines 11 to 13, the current check uses
url.includes('redis') which can falsely match hostnames and skip adding the
scheme; instead detect the actual scheme by trimming the input and checking for
known prefixes (e.g., startsWith('redis://') or startsWith('rediss://')) or by
parsing the string as a URL, and only prepend 'redis://' when no valid redis
scheme is present; update the createClient call to normalize the url
accordingly.
| .on('error', (error: Error) => { | ||
| logger.error('error on connecting:', error) | ||
| throw error | ||
| }) |
There was a problem hiding this comment.
Throwing in event handler causes unhandled exceptions.
The error event fires for both connection failures and runtime errors after connection. Throwing inside an event handler doesn't propagate to the await—it becomes an unhandled exception. For connection failures, connect() already rejects the promise.
Remove the throw and rely on connect() rejection for connection errors:
.on('error', (error: Error) => {
logger.error('error on connecting:', error)
- throw error
})Alternatively, if you need to handle post-connection errors differently, consider using process.on('unhandledRejection', ...) or a dedicated error recovery strategy.
🤖 Prompt for AI Agents
In src/core/config/cache.ts around lines 14 to 17, the 'error' event handler
currently logs the error and then throws it, which causes unhandled exceptions
because throwing inside an event handler doesn't reject the original connect()
promise; remove the throw so the handler only logs the error (or performs
recovery) and rely on connect() to reject on connection failures; if you need to
treat post-connection runtime errors differently, replace the throw with a
dedicated recovery/reconnect strategy or register a global handler (e.g.,
process.on('unhandledRejection' / custom error recovery) ) to handle those
cases.
| const sdk = new NodeSDK({ | ||
| resource, | ||
| spanProcessors: [new SimpleSpanProcessor(exporter)], | ||
| instrumentations: [getNodeAutoInstrumentations()], | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/presentation/tracing.ts | head -50Repository: auth-plus/auth-plus-authentication
Length of output: 1549
🏁 Script executed:
# Search for other tracing or SDK configurations
fd -t f "tracing\|trace\|sdk" --extension ts --extension js | head -20Repository: auth-plus/auth-plus-authentication
Length of output: 60
🏁 Script executed:
# Check if there's environment-specific configuration
grep -r "SimpleSpanProcessor\|BatchSpanProcessor" --include="*.ts" --include="*.js"Repository: auth-plus/auth-plus-authentication
Length of output: 256
🏁 Script executed:
# Look for production-related configuration
grep -r "production\|PRODUCTION\|NODE_ENV" src/presentation/tracing.tsRepository: auth-plus/auth-plus-authentication
Length of output: 60
🏁 Script executed:
# Check git history for this file
git log --oneline src/presentation/tracing.ts 2>/dev/null | head -10Repository: auth-plus/auth-plus-authentication
Length of output: 103
🏁 Script executed:
# Look for any documentation or comments about tracing strategy
grep -r "SimpleSpanProcessor\|BatchSpanProcessor\|span.*processor" --include="*.md" --include="*.txt" src/ 2>/dev/nullRepository: auth-plus/auth-plus-authentication
Length of output: 60
🌐 Web query:
OpenTelemetry SimpleSpanProcessor BatchSpanProcessor production performance best practices
💡 Result:
Brief summary + production best practices:
-
Use BatchSpanProcessor in production; SimpleSpanProcessor exports each finished span immediately and is intended for debugging (high CPU/network overhead). [1][5]
-
Tune BSP core knobs (defaults shown): max_queue_size (default 2048), max_export_batch_size (default 512), scheduled_delay_millis (default 5000 ms), export_timeout_millis (default 30000 ms). Adjust based on throughput, memory, and desired visibility latency. [1][4][5]
-
Sizing guidance:
- Increase max_queue_size for large burst tolerance (but watch memory). Decrease if you must bound memory. [1][4]
- Set max_export_batch_size to balance network payload size vs. export frequency (larger reduces calls, but increases per-export latency/CPU). [1][4]
- Reduce scheduled_delay_millis for fresher spans (higher export frequency → more traffic). [1][4]
-
Reliability and shutdown:
- Always call force_flush()/shutdown() on app exit to avoid losing buffered spans. BSP buffers are in-memory and can be lost on abrupt termination. [1][5]
-
Backpressure & failures:
- Configure export timeouts, retries/backoff, and queue limits so failures don’t cause unbounded memory growth; prefer dropping spans (load-shedding) to OOM. Use exporter helper / collector queue controls when using a Collector. [4][1]
-
Reduce volume before export:
- Apply sampling, attribute/span filtering, and aggregation to lower telemetry volume (reduces required queue size and export work). [4]
-
Use a local/sidecar Collector (or batching at exporter) to offload exporting work from app processes and enable buffering, retry, compression. Enable OTLP compression (gzip) where supported. [4][3]
-
Monitor and profile:
- Measure exporter latency, queue fill rate, dropped spans, and CPU/memory impact. If exporter is slow, BSP settings should be adjusted or move to a Collector to avoid tail latency. Coralogix/field reports show switching from Simple→Batch can drastically reduce CPU/memory and latency. [3][2]
-
When to use SimpleSpanProcessor:
- Local development, debugging, CI, or when immediate visibility per-span is required and volume is tiny. Not recommended for production. [1][5]
References
- OpenTelemetry tracing SDK (Batch & Simple processor details). [1]
- CNCF OpenTelemetry best-practices overview. [2]
- Coralogix / field experience on Collector performance and impact of switching to BSP. [3]
- OpenTelemetry config / Collector best-practices (queue, compression, timeouts). [4]
- Vendor/guide docs on Simple vs Batch and tuning knobs. [5]
SimpleSpanProcessor is not suitable for production environments.
SimpleSpanProcessor exports each span synchronously as it finishes, causing significant performance degradation under load. OpenTelemetry documentation explicitly recommends BatchSpanProcessor for production use, which batches spans and exports them asynchronously. Using SimpleSpanProcessor with ZipkinExporter (network-based) is a performance anti-pattern.
Replace with BatchSpanProcessor for production:
-import { SimpleSpanProcessor } from '@opentelemetry/sdk-trace-base'
+import { BatchSpanProcessor } from '@opentelemetry/sdk-trace-base'
const sdk = new NodeSDK({
resource,
- spanProcessors: [new SimpleSpanProcessor(exporter)],
+ spanProcessors: [new BatchSpanProcessor(exporter)],
instrumentations: [getNodeAutoInstrumentations()],
})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/presentation/tracing.ts around lines 30 to 34, the code uses
SimpleSpanProcessor which exports spans synchronously and is unsuitable for
production; replace it with BatchSpanProcessor (and import it) so spans are
batched and exported asynchronously via the ZipkinExporter, and instantiate
NodeSDK with spanProcessors: [new BatchSpanProcessor(exporter)]; optionally
configure BatchSpanProcessor parameters (scheduledDelayMillis,
maxExportBatchSize, maxQueueSize, exportTimeoutMillis) to appropriate
production-tuned values.
| beforeEach(async () => { | ||
| await database('multi_factor_authentication').del() | ||
| await database('user_info').del() | ||
| redis.del('*') |
There was a problem hiding this comment.
redis.del('*') does not delete keys by pattern.
Same issue as in mfa.route.test.ts. Use flushDb() to clear the cache between tests.
beforeEach(async () => {
await database('multi_factor_authentication').del()
await database('user_info').del()
- redis.del('*')
+ await redis.flushDb()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| redis.del('*') | |
| beforeEach(async () => { | |
| await database('multi_factor_authentication').del() | |
| await database('user_info').del() | |
| await redis.flushDb() | |
| }) |
🤖 Prompt for AI Agents
In test/integration/http/login.route.test.ts around line 82, the test calls
redis.del('*') which does not delete keys by pattern; replace this with await
redis.flushDb() (or the appropriate async flushDb call on your redis client) to
clear the test database between tests, and ensure the call is awaited so cleanup
completes before proceeding.
| beforeEach(async () => { | ||
| await database('multi_factor_authentication').del() | ||
| await database('user_info').del() | ||
| redis.del('*') |
There was a problem hiding this comment.
redis.del('*') does not delete keys by pattern.
The Redis DEL command requires explicit key names, not glob patterns. This call likely returns 0 and deletes nothing. To clear keys, use flushDb() or retrieve keys first.
beforeEach(async () => {
await database('multi_factor_authentication').del()
await database('user_info').del()
- redis.del('*')
+ await redis.flushDb()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| redis.del('*') | |
| beforeEach(async () => { | |
| await database('multi_factor_authentication').del() | |
| await database('user_info').del() | |
| await redis.flushDb() | |
| }) |
🤖 Prompt for AI Agents
In test/integration/http/mfa.route.test.ts around line 78, the call
redis.del('*') is incorrect because DEL does not accept glob patterns and will
not remove matching keys; replace it with a proper Redis clear operation —
either call redis.flushDb() to clear the test DB, or first fetch matching keys
(e.g., redis.keys or scan) and then pass the explicit key list to redis.del so
the keys are actually deleted before the test.
|
|
||
| beforeAll(async () => { | ||
| pgSqlContainer = await new PostgreSqlContainer().start() | ||
| pgSqlContainer = await new PostgreSqlContainer('postgres:15.1').start() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PostgreSQL 15.1 security vulnerabilities CVE
💡 Result:
Which CVEs affect PostgreSQL 15.1 (summary)
-
PostgreSQL’s official security tracker (filterable to major version 15) lists all CVEs and the minor release in which each was fixed — use that to see which CVEs apply to 15.1 and which minor release fixes them. [1]
-
Third‑party aggregated lists (example: CVE Details) show the CVEs reported against PostgreSQL 15.1 (22 items on that page). For a per‑CVE view you can open each entry to see description, severity and affected versions. [2]
Recommendation: upgrade 15.1 to the latest 15.x minor release for your platform (the PostgreSQL project releases security fixes only in minor updates). See the PostgreSQL security page for the authoritative, up‑to‑date CVE/fix mapping and individual CVE details. [1][2]
Sources:
[1] PostgreSQL — Security information / Known vulnerabilities (PostgreSQL 15 filter).
[2] CVE Details — PostgreSQL 15.1 vulnerability list.
🌐 Web query:
PostgreSQL 15 latest stable version 2025
💡 Result:
The latest PostgreSQL 15 stable minor release is PostgreSQL 15.15, released November 13, 2025. [1][2]
Sources:
[1] PostgreSQL 15.15 release notes (docs).
[2] PostgreSQL project release announcement (Nov 13, 2025).
Upgrade PostgreSQL container to version 15.15.
PostgreSQL 15.1 has 22 known security vulnerabilities. Upgrade to PostgreSQL 15.15 (the latest stable version in the 15.x series, released November 2025) to patch these CVEs while maintaining reproducibility.
🤖 Prompt for AI Agents
In test/unit/providers/user.repository.test.ts around line 31 the test starts a
PostgreSQL container pinned to image 'postgres:15.1' which has known CVEs;
update the image tag to 'postgres:15.15' to use the patched 15.x release. Change
the new PostgreSqlContainer(...) invocation to reference 'postgres:15.15' and
run the tests to verify compatibility; if any container startup settings or
tests rely on image-specific behavior, adjust them accordingly.
| verify(mockCreatingResetPassword.create(anything())).never() | ||
| verify(mockSendingResetEmail.sendEmail(anything(), anything())).never() | ||
| verify(mockFindingResetPassword.findByHash(anything())).never() | ||
| verify(mockFindingUser.findByEmail(anything())).never() | ||
| verify(mockUpdatingUser.updatePassword(anything(), anything())).never() |
There was a problem hiding this comment.
Critical test bug: Incorrect verifications for recover flow.
The test verifies that findByHash, findByEmail, and updatePassword were never called, but according to the recover implementation, these methods should be called once. This test will pass even if the recover method does nothing, defeating its purpose.
Apply this diff to fix the verifications:
verify(mockCreatingResetPassword.create(anything())).never()
verify(mockSendingResetEmail.sendEmail(anything(), anything())).never()
- verify(mockFindingResetPassword.findByHash(anything())).never()
- verify(mockFindingUser.findByEmail(anything())).never()
- verify(mockUpdatingUser.updatePassword(anything(), anything())).never()
+ verify(mockFindingResetPassword.findByHash(hash)).once()
+ verify(mockFindingUser.findByEmail(user.email)).once()
+ verify(mockUpdatingUser.updatePassword(user, newPassword)).once()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| verify(mockCreatingResetPassword.create(anything())).never() | |
| verify(mockSendingResetEmail.sendEmail(anything(), anything())).never() | |
| verify(mockFindingResetPassword.findByHash(anything())).never() | |
| verify(mockFindingUser.findByEmail(anything())).never() | |
| verify(mockUpdatingUser.updatePassword(anything(), anything())).never() | |
| verify(mockCreatingResetPassword.create(anything())).never() | |
| verify(mockSendingResetEmail.sendEmail(anything(), anything())).never() | |
| verify(mockFindingResetPassword.findByHash(hash)).once() | |
| verify(mockFindingUser.findByEmail(user.email)).once() | |
| verify(mockUpdatingUser.updatePassword(user, newPassword)).once() |
🤖 Prompt for AI Agents
In test/unit/usecases/reset_password.usecase.test.ts around lines 96 to 100, the
test incorrectly asserts that findByHash, findByEmail, and updatePassword are
never called; change the verifications so that
mockFindingResetPassword.findByHash(...), mockFindingUser.findByEmail(...), and
mockUpdatingUser.updatePassword(...) are verified to have been called once (use
.once()), while keeping mockCreatingResetPassword.create(...) and
mockSendingResetEmail.sendEmail(...) verified as never called.
Proposal
Links
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.