diff --git a/src/courses/entities/course.entity.ts b/src/courses/entities/course.entity.ts index dcedac73..51972cc7 100644 --- a/src/courses/entities/course.entity.ts +++ b/src/courses/entities/course.entity.ts @@ -66,6 +66,16 @@ export class Course { @Index() category?: string; + /** Difficulty level, e.g. 'beginner' | 'intermediate' | 'advanced'. Used by search filtering. */ + @Column({ nullable: true }) + @Index() + level?: string; + + /** ISO 639-1 language code the course is taught in. Used by search filtering. */ + @Column({ nullable: true }) + @Index() + language?: string; + @ManyToOne(() => User, (user) => user.courses) instructor: User; diff --git a/src/forum/entities/forum-vote.entity.ts b/src/forum/entities/forum-vote.entity.ts index e486b8ff..3aab3f53 100644 --- a/src/forum/entities/forum-vote.entity.ts +++ b/src/forum/entities/forum-vote.entity.ts @@ -1,4 +1,14 @@ -import { Entity, PrimaryGeneratedColumn, Column, CreateDateColumn, Unique, Index } from 'typeorm'; +import { + Entity, + PrimaryGeneratedColumn, + Column, + CreateDateColumn, + Unique, + Index, + ManyToOne, + JoinColumn, +} from 'typeorm'; +import { User } from '../../users/entities/user.entity'; @Entity('forum_votes') @Unique(['entityType', 'entityId', 'authorId']) @@ -13,9 +23,18 @@ export class ForumVote { @Column() entityId: string; - @Column() + /** + * Issue #990 — a synthetic 'anonymous' value could previously collide + * across every unauthenticated voter, corrupting vote tallies. The FK + * guarantees every vote is tied to a real, distinct user. + */ + @Column({ type: 'uuid' }) authorId: string; + @ManyToOne(() => User, { onDelete: 'CASCADE' }) + @JoinColumn({ name: 'authorId' }) + author: User; + @Column({ type: 'int' }) value: number; // 1 or -1 diff --git a/src/forum/forum.controller.spec.ts b/src/forum/forum.controller.spec.ts new file mode 100644 index 00000000..ba7c4171 --- /dev/null +++ b/src/forum/forum.controller.spec.ts @@ -0,0 +1,161 @@ +import { Injectable, INestApplication } from '@nestjs/common'; +import { Test, TestingModule } from '@nestjs/testing'; +import { PassportModule, PassportStrategy } from '@nestjs/passport'; +import { ExtractJwt, Strategy } from 'passport-jwt'; +import * as jwt from 'jsonwebtoken'; +import request from 'supertest'; +import { ForumController } from './forum.controller'; +import { ForumService } from './forum.service'; + +const JWT_TEST_SECRET = 'forum-controller-spec-secret'; + +/** + * Minimal stand-in for the app's real JwtStrategy — just enough for + * passport-jwt to register a strategy under the 'jwt' name so + * `AuthGuard('jwt')` has something to consult. A request with no + * Authorization header is rejected by passport-jwt before `validate()` ever + * runs, so this is sufficient to prove the guard actually rejects + * unauthenticated requests rather than to exercise real token verification. + */ +@Injectable() +class TestJwtStrategy extends PassportStrategy(Strategy, 'jwt') { + constructor() { + super({ + jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(), + secretOrKey: JWT_TEST_SECRET, + }); + } + + async validate(payload: { sub: string }) { + return { id: payload.sub }; + } +} + +/** + * Issue #990 — every forum write handler computed + * `authorId = req.user?.id || 'anonymous'` with no guard, so unauthenticated + * callers silently wrote content/votes as a shared 'anonymous' identity + * instead of being rejected. These tests exercise the real HTTP + guard + * layer (not a mocked guard) to prove that path is closed. + */ +describe('ForumController (Issue #990 — authenticated forum writes)', () => { + let app: INestApplication; + let forumService: { + createThread: jest.Mock; + addComment: jest.Mock; + vote: jest.Mock; + getThreads: jest.Mock; + getThread: jest.Mock; + }; + + beforeEach(async () => { + forumService = { + createThread: jest.fn().mockResolvedValue({ id: 'thread-1' }), + addComment: jest.fn().mockResolvedValue({ id: 'comment-1' }), + vote: jest.fn().mockResolvedValue(undefined), + getThreads: jest.fn().mockResolvedValue({ data: [], total: 0 }), + getThread: jest.fn().mockResolvedValue({ id: 'thread-1', comments: { data: [], total: 0 } }), + }; + + const module: TestingModule = await Test.createTestingModule({ + imports: [PassportModule], + controllers: [ForumController], + providers: [TestJwtStrategy, { provide: ForumService, useValue: forumService }], + }).compile(); + + app = module.createNestApplication(); + await app.init(); + }); + + afterEach(async () => { + await app.close(); + }); + + function signToken(sub: string): string { + return jwt.sign({ sub }, JWT_TEST_SECRET); + } + + describe('unauthenticated requests', () => { + it('rejects POST /forums/threads with 401 and never calls the service', async () => { + const res = await request(app.getHttpServer()) + .post('/forums/threads') + .send({ title: 't', content: 'c' }); + + expect(res.status).toBe(401); + expect(forumService.createThread).not.toHaveBeenCalled(); + }); + + it('rejects POST /forums/threads/:id/comments with 401', async () => { + const res = await request(app.getHttpServer()) + .post('/forums/threads/thread-1/comments') + .send({ content: 'c' }); + + expect(res.status).toBe(401); + expect(forumService.addComment).not.toHaveBeenCalled(); + }); + + it('rejects POST /forums/threads/:id/vote with 401 and never calls the service', async () => { + const res = await request(app.getHttpServer()) + .post('/forums/threads/thread-1/vote') + .send({ value: 1 }); + + expect(res.status).toBe(401); + expect(forumService.vote).not.toHaveBeenCalled(); + }); + + it('rejects POST /forums/comments/:id/vote with 401 and never calls the service', async () => { + const res = await request(app.getHttpServer()) + .post('/forums/comments/comment-1/vote') + .send({ value: -1 }); + + expect(res.status).toBe(401); + expect(forumService.vote).not.toHaveBeenCalled(); + }); + + it('leaves read endpoints public', async () => { + const threadsRes = await request(app.getHttpServer()).get('/forums/threads'); + const threadRes = await request(app.getHttpServer()).get('/forums/threads/thread-1'); + + expect(threadsRes.status).toBe(200); + expect(threadRes.status).toBe(200); + }); + }); + + describe('authenticated requests', () => { + it('creates a thread with the real authenticated user id, never "anonymous"', async () => { + const token = signToken('user-42'); + + const res = await request(app.getHttpServer()) + .post('/forums/threads') + .set('Authorization', `Bearer ${token}`) + .send({ title: 't', content: 'c' }); + + expect(res.status).toBeLessThan(300); + expect(forumService.createThread).toHaveBeenCalledWith('t', 'c', 'user-42'); + }); + + it('records a vote under the authenticated user id', async () => { + const token = signToken('user-99'); + + const res = await request(app.getHttpServer()) + .post('/forums/threads/thread-1/vote') + .set('Authorization', `Bearer ${token}`) + .send({ value: 1 }); + + expect(res.status).toBeLessThan(300); + expect(forumService.vote).toHaveBeenCalledWith('thread', 'thread-1', 'user-99', 1); + }); + + it('rejects a request bearing a token signed with the wrong secret', async () => { + const badToken = jwt.sign({ sub: 'user-1' }, 'wrong-secret'); + + const res = await request(app.getHttpServer()) + .post('/forums/threads') + .set('Authorization', `Bearer ${badToken}`) + .send({ title: 't', content: 'c' }); + + expect(res.status).toBe(401); + expect(forumService.createThread).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/forum/forum.controller.ts b/src/forum/forum.controller.ts index 3fb9c301..42a1b2fc 100644 --- a/src/forum/forum.controller.ts +++ b/src/forum/forum.controller.ts @@ -1,15 +1,24 @@ -import { Controller, Post, Get, Body, Param, Req, Query } from '@nestjs/common'; +import { Controller, Post, Get, Body, Param, Query, UseGuards } from '@nestjs/common'; +import { ApiBearerAuth, ApiResponse, ApiTags } from '@nestjs/swagger'; import { ForumService } from './forum.service'; import { PaginationQueryDto } from '../common/dto/pagination.dto'; +import { JwtAuthGuard } from '../auth/guards/jwt-auth.guard'; +import { CurrentUser } from '../auth/decorators/current-user.decorator'; +@ApiTags('Forum') @Controller('forums') export class ForumController { constructor(private readonly forumService: ForumService) {} @Post('threads') - createThread(@Body() body: { title: string; content: string }, @Req() req: any) { - const authorId = req.user?.id || 'anonymous'; - return this.forumService.createThread(body.title, body.content, authorId); + @UseGuards(JwtAuthGuard) + @ApiBearerAuth() + @ApiResponse({ status: 401, description: 'Authentication required' }) + createThread( + @Body() body: { title: string; content: string }, + @CurrentUser() user: { id: string }, + ) { + return this.forumService.createThread(body.title, body.content, user.id); } @Get('threads') @@ -23,24 +32,38 @@ export class ForumController { } @Post('threads/:id/comments') + @UseGuards(JwtAuthGuard) + @ApiBearerAuth() + @ApiResponse({ status: 401, description: 'Authentication required' }) addComment( @Param('id') threadId: string, @Body() body: { content: string; parentId?: string }, - @Req() req: any, + @CurrentUser() user: { id: string }, ) { - const authorId = req.user?.id || 'anonymous'; - return this.forumService.addComment(threadId, body.content, authorId, body.parentId); + return this.forumService.addComment(threadId, body.content, user.id, body.parentId); } @Post('threads/:id/vote') - voteThread(@Param('id') id: string, @Body() body: { value: number }, @Req() req: any) { - const authorId = req.user?.id || 'anonymous'; - return this.forumService.vote('thread', id, authorId, body.value); + @UseGuards(JwtAuthGuard) + @ApiBearerAuth() + @ApiResponse({ status: 401, description: 'Authentication required' }) + voteThread( + @Param('id') id: string, + @Body() body: { value: number }, + @CurrentUser() user: { id: string }, + ) { + return this.forumService.vote('thread', id, user.id, body.value); } @Post('comments/:id/vote') - voteComment(@Param('id') id: string, @Body() body: { value: number }, @Req() req: any) { - const authorId = req.user?.id || 'anonymous'; - return this.forumService.vote('comment', id, authorId, body.value); + @UseGuards(JwtAuthGuard) + @ApiBearerAuth() + @ApiResponse({ status: 401, description: 'Authentication required' }) + voteComment( + @Param('id') id: string, + @Body() body: { value: number }, + @CurrentUser() user: { id: string }, + ) { + return this.forumService.vote('comment', id, user.id, body.value); } } diff --git a/src/forum/forum.service.ts b/src/forum/forum.service.ts index b49e8088..ef77eaff 100644 --- a/src/forum/forum.service.ts +++ b/src/forum/forum.service.ts @@ -15,7 +15,7 @@ export type ForumThreadListItem = Pick< 'id' | 'title' | 'authorId' | 'createdAt' | 'upvotes' | 'downvotes' >; -export type ForumThreadDetail = ForumThread & { +export type ForumThreadDetail = Omit & { comments: OffsetPaginatedResponse; }; diff --git a/src/migrations/1791000000000-add-course-level-language.ts b/src/migrations/1791000000000-add-course-level-language.ts new file mode 100644 index 00000000..57734ed0 --- /dev/null +++ b/src/migrations/1791000000000-add-course-level-language.ts @@ -0,0 +1,27 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +/** + * Issue #995 — `SearchFilters` has declared `level` and `language` since + * search filtering was introduced, but neither was ever backed by a column + * on `course`, so `SearchService.search()` silently ignored both filters. + * Adds the columns (mirroring the nullable, indexed `category` column) so + * the query builder can filter on them. + */ +export class AddCourseLevelLanguage1791000000000 implements MigrationInterface { + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query('ALTER TABLE "course" ADD COLUMN IF NOT EXISTS "level" varchar'); + await queryRunner.query('ALTER TABLE "course" ADD COLUMN IF NOT EXISTS "language" varchar'); + + await queryRunner.query('CREATE INDEX IF NOT EXISTS "IDX_course_level" ON "course" ("level")'); + await queryRunner.query( + 'CREATE INDEX IF NOT EXISTS "IDX_course_language" ON "course" ("language")', + ); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query('DROP INDEX IF EXISTS "IDX_course_level"'); + await queryRunner.query('DROP INDEX IF EXISTS "IDX_course_language"'); + await queryRunner.query('ALTER TABLE "course" DROP COLUMN IF EXISTS "level"'); + await queryRunner.query('ALTER TABLE "course" DROP COLUMN IF EXISTS "language"'); + } +} diff --git a/src/migrations/1791000000001-fix-forum-anonymous-author.ts b/src/migrations/1791000000001-fix-forum-anonymous-author.ts new file mode 100644 index 00000000..775b7256 --- /dev/null +++ b/src/migrations/1791000000001-fix-forum-anonymous-author.ts @@ -0,0 +1,101 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +/** + * Issue #990 — every forum write handler computed + * `authorId = req.user?.id || 'anonymous'` with no guard on the controller, + * so every thread, comment, and vote was written with the literal string + * `'anonymous'` as its author. Two consequences: + * + * 1. `forum_vote` looked up existing votes by + * `(entityType, entityId, authorId)` — every anonymous voter collided on + * the single `'anonymous'` row, so vote tallies for any thread/comment + * that received more than one anonymous vote are meaningless (each new + * "anonymous" vote just flipped the same row). + * 2. `forum_thread`/`forum_comment` content became unattributable, so the + * moderation queue (keyed on `sourceId`) has no author to act against. + * + * This migration: + * - Purges `forum_votes` rows whose `authorId` isn't a real user id — the + * literal `'anonymous'` plus any other non-UUID placeholder — since their + * tallies cannot be reconstructed (we don't know how many distinct + * anonymous callers voted, only that they collapsed into one row), then + * recomputes `upvotes`/`downvotes` on the affected threads/comments from + * what remains. + * - Flags `forum_thread`/`forum_comment` rows with `authorId = 'anonymous'` + * (status -> 'flagged') instead of deleting them outright, routing them + * through the same manual-review path flagged content already uses, + * since the content itself may still have value even though it can't be + * attributed. + * - Converts `forum_votes.authorId` from `varchar` to `uuid` (matching + * `users.id`) and adds a foreign key so a synthetic value can never be + * written again. + */ +export class FixForumAnonymousAuthor1791000000001 implements MigrationInterface { + private static readonly NOT_A_USER_ID = + '"authorId" !~ \'^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$\''; + + public async up(queryRunner: QueryRunner): Promise { + // 1. Purge votes whose author isn't a real (UUID) user id and recompute + // affected tallies. This also catches any other synthetic placeholder + // that isn't a valid UUID, not just the literal 'anonymous' value — + // both would otherwise block the FK/type change below. + const affected: Array<{ entityType: string; entityId: string }> = await queryRunner.query(` + SELECT DISTINCT "entityType", "entityId" FROM forum_votes + WHERE ${FixForumAnonymousAuthor1791000000001.NOT_A_USER_ID} + `); + + await queryRunner.query( + `DELETE FROM forum_votes WHERE ${FixForumAnonymousAuthor1791000000001.NOT_A_USER_ID}`, + ); + + for (const { entityType, entityId } of affected) { + const [{ upvotes }] = await queryRunner.query( + 'SELECT COUNT(*)::int AS upvotes FROM forum_votes WHERE "entityType" = $1 AND "entityId" = $2 AND value = 1', + [entityType, entityId], + ); + const [{ downvotes }] = await queryRunner.query( + 'SELECT COUNT(*)::int AS downvotes FROM forum_votes WHERE "entityType" = $1 AND "entityId" = $2 AND value = -1', + [entityType, entityId], + ); + + const table = entityType === 'thread' ? 'forum_threads' : 'forum_comments'; + await queryRunner.query(`UPDATE ${table} SET upvotes = $1, downvotes = $2 WHERE id = $3`, [ + upvotes, + downvotes, + entityId, + ]); + } + + // 2. Route unattributable content to manual review instead of deleting it. + await queryRunner.query( + "UPDATE forum_threads SET status = 'flagged' WHERE \"authorId\" = 'anonymous' AND status = 'active'", + ); + await queryRunner.query( + "UPDATE forum_comments SET status = 'flagged' WHERE \"authorId\" = 'anonymous' AND status = 'active'", + ); + + // 3. Align the column type with users.id and prevent a synthetic author + // id from ever being written again. + await queryRunner.query(` + ALTER TABLE forum_votes ALTER COLUMN "authorId" TYPE uuid USING "authorId"::uuid + `); + await queryRunner.query(` + ALTER TABLE forum_votes + ADD CONSTRAINT "FK_forum_votes_authorId_users" + FOREIGN KEY ("authorId") REFERENCES users(id) ON DELETE CASCADE + `); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(` + ALTER TABLE forum_votes DROP CONSTRAINT IF EXISTS "FK_forum_votes_authorId_users" + `); + await queryRunner.query(` + ALTER TABLE forum_votes ALTER COLUMN "authorId" TYPE varchar USING "authorId"::varchar + `); + // Purged votes and the flagged->active status change on threads/comments + // are not reversible: we don't retain enough information to reconstruct + // how many distinct anonymous callers voted, or which flagged rows were + // flagged for this reason versus auto-moderation. + } +} diff --git a/src/monitoring/alerting/alerting.service.spec.ts b/src/monitoring/alerting/alerting.service.spec.ts index 848a35c8..d2f5e8d0 100644 --- a/src/monitoring/alerting/alerting.service.spec.ts +++ b/src/monitoring/alerting/alerting.service.spec.ts @@ -1,6 +1,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { AlertingService } from './alerting.service'; import { ConfigService } from '@nestjs/config'; +import { MetricsService } from '../../utils/masking/metrics.service'; import axios from 'axios'; jest.mock('axios'); @@ -8,32 +9,44 @@ const mockedAxios = axios as jest.Mocked; describe('AlertingService', () => { let service: AlertingService; - let configService: ConfigService; + let metricsService: { alertDeliveryFailuresCounter: { inc: jest.Mock } }; - const mockConfig = { + const mockConfig: Record = { PAGERDUTY_ROUTING_KEY: 'test-routing-key', ALERT_SLACK_WEBHOOK_URL: 'https://example.com/slack-webhook', }; beforeEach(async () => { + metricsService = { alertDeliveryFailuresCounter: { inc: jest.fn() } }; + const module: TestingModule = await Test.createTestingModule({ providers: [ AlertingService, { provide: ConfigService, useValue: { - get: jest.fn((key: string) => mockConfig[key as keyof typeof mockConfig]), + get: jest.fn((key: string, defaultValue?: unknown) => + key in mockConfig ? mockConfig[key] : defaultValue, + ), }, }, + { + provide: MetricsService, + useValue: metricsService, + }, ], }).compile(); service = module.get(AlertingService); - configService = module.get(ConfigService); mockedAxios.post.mockClear(); + (mockedAxios as any).isAxiosError = jest.fn( + (err: unknown) => !!err && typeof err === 'object' && 'isAxiosError' in (err as object), + ); }); it('should send a CRITICAL alert to PagerDuty and Slack', async () => { + mockedAxios.post.mockResolvedValue({ status: 200 }); + // Send a critical alert manually service.sendAlert('PAYMENT_FAILURE_RATE_CRITICAL', 'Payment failure rate is high', 'CRITICAL'); @@ -46,6 +59,7 @@ describe('AlertingService', () => { expect.objectContaining({ attachments: expect.arrayContaining([expect.objectContaining({ color: '#dc2626' })]), }), + expect.objectContaining({ timeout: expect.any(Number) }), ); // Verify PagerDuty @@ -59,10 +73,13 @@ describe('AlertingService', () => { source: 'teachLink_backend', }), }), + expect.objectContaining({ timeout: expect.any(Number) }), ); }); it('should NOT send a WARNING alert to PagerDuty', async () => { + mockedAxios.post.mockResolvedValue({ status: 200 }); + service.sendAlert( 'PAYMENT_FAILURE_RATE_WARNING', 'Payment failure rate is increasing', @@ -75,6 +92,7 @@ describe('AlertingService', () => { expect(mockedAxios.post).toHaveBeenCalledWith( mockConfig.ALERT_SLACK_WEBHOOK_URL, expect.any(Object), + expect.any(Object), ); // PagerDuty should NOT be called @@ -83,4 +101,77 @@ describe('AlertingService', () => { ); expect(pagerDutyCalls.length).toBe(0); }); + + it('passes the configured timeout to every outbound delivery call', async () => { + mockedAxios.post.mockResolvedValue({ status: 200 }); + + service.sendAlert('PAYMENT_FAILURE_RATE_CRITICAL', 'high', 'CRITICAL'); + await new Promise((resolve) => setTimeout(resolve, 50)); + + for (const call of mockedAxios.post.mock.calls) { + expect(call[2]).toEqual(expect.objectContaining({ timeout: expect.any(Number) })); + } + }); + + it('increments the failure counter on a failed Slack delivery and does not throw', async () => { + mockedAxios.post.mockImplementation((url: string) => { + if (url === mockConfig.ALERT_SLACK_WEBHOOK_URL) { + return Promise.reject({ + isAxiosError: true, + response: { status: 400 }, + message: 'Bad Request', + }); + } + return Promise.resolve({ status: 200 }); + }); + + expect(() => + service.sendAlert('PAYMENT_FAILURE_RATE_CRITICAL', 'high', 'CRITICAL'), + ).not.toThrow(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + expect(metricsService.alertDeliveryFailuresCounter.inc).toHaveBeenCalledWith({ + channel: 'slack', + }); + }); + + it('retries a transient 5xx PagerDuty response and eventually succeeds', async () => { + let pagerDutyCalls = 0; + mockedAxios.post.mockImplementation((url: string) => { + if (url === 'https://events.pagerduty.com/v2/enqueue') { + pagerDutyCalls += 1; + if (pagerDutyCalls < 2) { + return Promise.reject({ isAxiosError: true, response: { status: 503 } }); + } + return Promise.resolve({ status: 202 }); + } + return Promise.resolve({ status: 200 }); + }); + + service.sendAlert('PAYMENT_FAILURE_RATE_CRITICAL', 'high', 'CRITICAL'); + await new Promise((resolve) => setTimeout(resolve, 1000)); + + expect(pagerDutyCalls).toBe(2); + expect(metricsService.alertDeliveryFailuresCounter.inc).toHaveBeenCalledWith({ + channel: 'pagerduty', + }); + }, 10000); + + it('gives up after exhausting retries on a persistent 5xx response', async () => { + mockedAxios.post.mockImplementation((url: string) => { + if (url === 'https://events.pagerduty.com/v2/enqueue') { + return Promise.reject({ isAxiosError: true, response: { status: 500 } }); + } + return Promise.resolve({ status: 200 }); + }); + + service.sendAlert('PAYMENT_FAILURE_RATE_CRITICAL', 'high', 'CRITICAL'); + await new Promise((resolve) => setTimeout(resolve, 1500)); + + const pagerDutyCalls = mockedAxios.post.mock.calls.filter( + (call) => call[0] === 'https://events.pagerduty.com/v2/enqueue', + ); + // Initial attempt + 2 retries = 3 total attempts + expect(pagerDutyCalls.length).toBe(3); + }, 10000); }); diff --git a/src/monitoring/alerting/alerting.service.ts b/src/monitoring/alerting/alerting.service.ts index 921ec6f6..3f8fc137 100644 --- a/src/monitoring/alerting/alerting.service.ts +++ b/src/monitoring/alerting/alerting.service.ts @@ -2,6 +2,7 @@ import { Injectable, Logger } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import * as nodemailer from 'nodemailer'; import axios from 'axios'; +import { MetricsService } from '../../utils/masking/metrics.service'; export type AlertSeverity = 'INFO' | 'WARNING' | 'CRITICAL'; @@ -132,7 +133,18 @@ export class AlertingService { private readonly emailFrom: string; private mailerTransport: nodemailer.Transporter | null = null; - constructor(private readonly configService: ConfigService) { + /** Timeout applied to every outbound alert delivery HTTP call. */ + private readonly alertDeliveryTimeoutMs: number; + + /** Bounded retry count for transient 5xx responses (does not include the initial attempt). */ + private readonly maxDeliveryRetries = 2; + private readonly retryBaseBackoffMs = 200; + + constructor( + private readonly configService: ConfigService, + private readonly metricsService: MetricsService, + ) { + this.alertDeliveryTimeoutMs = this.configService.get('ALERT_DELIVERY_TIMEOUT_MS', 5000); this.emailFrom = this.configService.get('EMAIL_FROM', 'noreply@teachlink.io'); const recipientRaw = this.configService.get('ALERT_EMAIL_RECIPIENTS', ''); this.alertEmailRecipients = recipientRaw @@ -389,7 +401,7 @@ export class AlertingService { ], }; - await axios.post(this.slackWebhookUrl, body); + await this.postWithRetry('slack', this.slackWebhookUrl, body); } private async sendPagerDutyAlert(event: IAlertEvent): Promise { @@ -410,6 +422,46 @@ export class AlertingService { }, }; - await axios.post('https://events.pagerduty.com/v2/enqueue', payload); + await this.postWithRetry('pagerduty', 'https://events.pagerduty.com/v2/enqueue', payload); + } + + /** + * POST an alert payload with a bounded timeout. Transient 5xx responses are + * retried with jittered backoff up to `maxDeliveryRetries` times; every + * failed attempt (transient or not) increments the failure counter, and + * exhausted retries are surfaced as a warning log. + */ + private async postWithRetry( + channel: 'slack' | 'pagerduty', + url: string, + payload: unknown, + attempt = 0, + ): Promise { + try { + await axios.post(url, payload, { timeout: this.alertDeliveryTimeoutMs }); + } catch (error) { + const status = axios.isAxiosError(error) ? error.response?.status : undefined; + this.metricsService.alertDeliveryFailuresCounter.inc({ channel }); + + const isTransient5xx = typeof status === 'number' && status >= 500 && status < 600; + if (isTransient5xx && attempt < this.maxDeliveryRetries) { + const backoffMs = + this.retryBaseBackoffMs * 2 ** attempt + Math.random() * this.retryBaseBackoffMs; + this.logger.warn( + `${channel} alert delivery failed with status ${status}, retrying ` + + `(attempt ${attempt + 1}/${this.maxDeliveryRetries}) in ${Math.round(backoffMs)}ms`, + ); + await new Promise((resolve) => setTimeout(resolve, backoffMs)); + return this.postWithRetry(channel, url, payload, attempt + 1); + } + + if (isTransient5xx) { + this.logger.warn( + `${channel} alert delivery exhausted retries after ${attempt + 1} attempts`, + ); + } + + throw error; + } } } diff --git a/src/search/search.controller.ts b/src/search/search.controller.ts index c3cf4228..a262eefd 100644 --- a/src/search/search.controller.ts +++ b/src/search/search.controller.ts @@ -45,7 +45,8 @@ export class SearchController { }, }, }) - @ApiResponse({ status: 400, description: 'Invalid filters JSON' }) + @ApiResponse({ status: 400, description: 'Invalid filters JSON, or invalid filter values' }) + @ApiResponse({ status: 503, description: 'Search is temporarily unavailable' }) async search( @Query('q') query: string, @Query('filters') filters?: string, @@ -76,6 +77,7 @@ export class SearchController { description: 'Autocomplete suggestions', schema: { example: ['javascript', 'java fundamentals', 'java spring'] }, }) + @ApiResponse({ status: 503, description: 'Search is temporarily unavailable' }) async autocomplete( @Query('q') query: string, diff --git a/src/search/search.service.category-filter.integration.spec.ts b/src/search/search.service.category-filter.integration.spec.ts new file mode 100644 index 00000000..a1928878 --- /dev/null +++ b/src/search/search.service.category-filter.integration.spec.ts @@ -0,0 +1,169 @@ +import { DataSource, EntitySchema, Repository } from 'typeorm'; +import { SearchService, SearchFilters } from './search.service'; +import { Course, CourseStatus } from '../courses/entities/course.entity'; + +/** + * Issue #995 — `qb.andWhere('course.category IN (:cats)', { cats })` bound an + * array to a scalar placeholder, so category filtering either matched + * nothing or threw at the driver. The mocked-qb unit tests in + * `search.service.spec.ts` only assert the SQL string, not that Postgres + * actually accepts the bound parameter — this test exercises the real + * TypeORM query builder against Postgres, seeding courses across two + * categories and asserting filtering returns exactly the requested ones. + * + * Uses an `EntitySchema` mirroring just the columns `SearchService.search()` + * touches, backed by a throwaway table, instead of the real `Course` entity + * — that avoids dragging in the full `Course` relation graph (User, Role, + * CourseModule, Enrollment, ...) just to resolve entity metadata, and avoids + * `synchronize: true` altering the real `course` table's schema. + * + * Requires a reachable Postgres instance configured via the same DATABASE_* + * env vars the app uses (see src/config/database.config.ts). Like the other + * `*.integration.spec.ts` files in this repo, it is excluded from the + * default `npm test` run — run it explicitly against a live database. + */ +describe('SearchService category/level/language filters (Postgres integration)', () => { + const TEST_TABLE = 'course_search_filter_test'; + + interface CourseRow { + id: string; + title: string; + description: string; + category: string | null; + level: string | null; + language: string | null; + price: number; + status: string; + instructorId: string; + createdAt: Date; + updatedAt: Date; + } + + const CourseTestSchema = new EntitySchema({ + name: 'Course', + tableName: TEST_TABLE, + columns: { + id: { type: 'uuid', primary: true, generated: 'uuid' }, + title: { type: String }, + description: { type: 'text' }, + category: { type: String, nullable: true }, + level: { type: String, nullable: true }, + language: { type: String, nullable: true }, + price: { type: 'decimal', precision: 10, scale: 2, default: 0 }, + status: { type: String, default: CourseStatus.PUBLISHED }, + instructorId: { type: String }, + createdAt: { type: 'timestamp', createDate: true }, + updatedAt: { type: 'timestamp', updateDate: true }, + }, + }); + + let dataSource: DataSource; + let courseRepository: Repository; + let service: SearchService; + + const noopElasticsearch = { ping: jest.fn(), search: jest.fn() } as any; + const metricsService = { searchFallbackCounter: { inc: jest.fn() } } as any; + + async function search(filters: SearchFilters) { + return service.search('', filters); + } + + beforeAll(async () => { + dataSource = new DataSource({ + type: 'postgres', + host: process.env.DATABASE_HOST ?? 'localhost', + port: parseInt(process.env.DATABASE_PORT ?? '5432', 10), + username: process.env.DATABASE_USER ?? 'postgres', + password: process.env.DATABASE_PASSWORD ?? 'postgres', + database: process.env.DATABASE_NAME ?? 'teachlink', + entities: [CourseTestSchema], + synchronize: true, + }); + await dataSource.initialize(); + courseRepository = dataSource.getRepository('Course'); + + // SearchService is constructed directly (not via full AppModule DI) so + // this test only needs Postgres, not the rest of the app's required env. + // The EntitySchema-backed repository satisfies the same Repository + // surface SearchService relies on (createQueryBuilder / getManyAndCount). + service = new SearchService( + courseRepository as unknown as Repository, + noopElasticsearch, + metricsService, + ); + }); + + afterAll(async () => { + await dataSource.query(`DROP TABLE IF EXISTS "${TEST_TABLE}" CASCADE`); + await dataSource.destroy(); + }); + + beforeEach(async () => { + await courseRepository.query(`TRUNCATE TABLE "${TEST_TABLE}" CASCADE`); + await courseRepository.save([ + courseRepository.create({ + title: 'Intro to Design Systems', + description: 'design', + category: 'design', + level: 'beginner', + language: 'en', + status: CourseStatus.PUBLISHED, + instructorId: 'instructor-1', + }), + courseRepository.create({ + title: 'Advanced Business Strategy', + description: 'business', + category: 'business', + level: 'advanced', + language: 'en', + status: CourseStatus.PUBLISHED, + instructorId: 'instructor-1', + }), + courseRepository.create({ + title: 'Deep Learning Fundamentals', + description: 'data-science', + category: 'data-science', + level: 'intermediate', + language: 'es', + status: CourseStatus.PUBLISHED, + instructorId: 'instructor-1', + }), + ]); + }); + + it('returns courses from both requested categories and none from others', async () => { + const result = await search({ category: ['design', 'business'] }); + + expect(result.total).toBe(2); + const categories = result.results.map((c: CourseRow) => c.category).sort(); + expect(categories).toEqual(['business', 'design']); + }); + + it('returns courses from a single requested category', async () => { + const result = await search({ category: 'design' }); + + expect(result.total).toBe(1); + expect(result.results[0].category).toBe('design'); + }); + + it('honours the level filter declared in SearchFilters', async () => { + const result = await search({ level: ['beginner', 'advanced'] }); + + expect(result.total).toBe(2); + const levels = result.results.map((c: CourseRow) => c.level).sort(); + expect(levels).toEqual(['advanced', 'beginner']); + }); + + it('honours the language filter declared in SearchFilters', async () => { + const result = await search({ language: 'es' }); + + expect(result.total).toBe(1); + expect(result.results[0].language).toBe('es'); + }); + + it('combines category and level filters', async () => { + const result = await search({ category: 'design', level: 'advanced' }); + + expect(result.total).toBe(0); + }); +}); diff --git a/src/search/search.service.spec.ts b/src/search/search.service.spec.ts index 9b200943..2603b5a6 100644 --- a/src/search/search.service.spec.ts +++ b/src/search/search.service.spec.ts @@ -1,7 +1,7 @@ import { BadRequestException } from '@nestjs/common'; import { SearchService } from './search.service'; import { SearchService, SEARCH_CACHE_TTL_MS } from './search.service'; -import { Repository } from 'typeorm'; +import { Repository, QueryFailedError } from 'typeorm'; import { ElasticsearchService } from '@nestjs/elasticsearch'; /** @@ -14,7 +14,10 @@ describe('SearchService (Issue #814 full-text search)', () => { let courseRepository: jest.Mocked>; let elasticsearch: { search: jest.Mock; ping: jest.Mock }; let isolationService: { getTenantId: jest.Mock }; - let metricsService: { searchFallbackCounter: { inc: jest.Mock } }; + let metricsService: { + searchFallbackCounter: { inc: jest.Mock }; + searchQueryFailuresCounter: { inc: jest.Mock }; + }; beforeEach(() => { courseRepository = { @@ -23,7 +26,10 @@ describe('SearchService (Issue #814 full-text search)', () => { elasticsearch = { search: jest.fn(), ping: jest.fn().mockResolvedValue(true) }; isolationService = { getTenantId: jest.fn().mockReturnValue(null) }; - metricsService = { searchFallbackCounter: { inc: jest.fn() } }; + metricsService = { + searchFallbackCounter: { inc: jest.fn() }, + searchQueryFailuresCounter: { inc: jest.fn() }, + }; service = new SearchService( courseRepository as any, @@ -94,6 +100,160 @@ describe('SearchService (Issue #814 full-text search)', () => { expect(qb.orderBy).toHaveBeenCalledWith('course.createdAt', 'DESC'); }); + describe('Issue #995 — array-valued filter parameter expansion', () => { + it('uses the spread form and array param for a single-value category filter', async () => { + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeQb({ rows: [], total: 0 }); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await service.search('', { category: 'design' }); + + expect(qb.andWhere).toHaveBeenCalledWith('course.category IN (:...cats)', { + cats: ['design'], + }); + }); + + it('uses the spread form and array param for a multi-value category filter', async () => { + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeQb({ rows: [], total: 0 }); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await service.search('', { category: ['design', 'business'] }); + + expect(qb.andWhere).toHaveBeenCalledWith('course.category IN (:...cats)', { + cats: ['design', 'business'], + }); + }); + + it('never uses the unspread single-colon form for category (regression guard)', async () => { + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeQb({ rows: [], total: 0 }); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await service.search('', { category: ['design', 'business'] }); + + expect(qb.andWhere).not.toHaveBeenCalledWith('course.category IN (:cats)', expect.anything()); + }); + + it('applies the level filter with spread-form array expansion', async () => { + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeQb({ rows: [], total: 0 }); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await service.search('', { level: ['beginner', 'intermediate'] }); + + expect(qb.andWhere).toHaveBeenCalledWith('course.level IN (:...levels)', { + levels: ['beginner', 'intermediate'], + }); + }); + + it('applies the language filter with spread-form array expansion', async () => { + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeQb({ rows: [], total: 0 }); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await service.search('', { language: 'en' }); + + expect(qb.andWhere).toHaveBeenCalledWith('course.language IN (:...languages)', { + languages: ['en'], + }); + }); + + it('omits filter clauses that were not supplied', async () => { + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeQb({ rows: [], total: 0 }); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await service.search('', { category: 'design' }); + + const calledClauses = qb.andWhere.mock.calls.map((c: any[]) => c[0]); + expect(calledClauses).not.toContain(expect.stringContaining('course.level')); + expect(calledClauses).not.toContain(expect.stringContaining('course.language')); + }); + }); + + describe('Issue #997 — search failures surface as errors, not empty results', () => { + function makeFailingQb(error: unknown) { + const qb: any = { + addSelect: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + orWhere: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + orderBy: jest.fn().mockReturnThis(), + addOrderBy: jest.fn().mockReturnThis(), + skip: jest.fn().mockReturnThis(), + take: jest.fn().mockReturnThis(), + getManyAndCount: jest.fn().mockRejectedValue(error), + }; + return qb; + } + + it('rethrows as ServiceUnavailableException (503) on a repository/infrastructure failure', async () => { + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeFailingQb(new Error('connection terminated unexpectedly')); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await expect(service.search('react')).rejects.toMatchObject({ + status: 503, + }); + }); + + it('increments search_query_failures_total with failure_class="infrastructure" on a repository failure', async () => { + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeFailingQb(new Error('connection terminated unexpectedly')); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await expect(service.search('react')).rejects.toThrow(); + + expect(metricsService.searchQueryFailuresCounter.inc).toHaveBeenCalledWith({ + failure_class: 'infrastructure', + }); + }); + + it('rethrows as BadRequestException (400) when Postgres reports a data-exception (22xxx) SQLSTATE', async () => { + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const driverError = Object.assign(new QueryFailedError('SELECT 1', [], new Error('bad')), { + code: '22P02', // invalid_text_representation + }); + const qb = makeFailingQb(driverError); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await expect(service.search('react', { category: 'not-a-real-uuid' })).rejects.toMatchObject({ + status: 400, + }); + + expect(metricsService.searchQueryFailuresCounter.inc).toHaveBeenCalledWith({ + failure_class: 'caller', + }); + }); + + it('autocomplete rethrows as ServiceUnavailableException (503) instead of returning an empty array', async () => { + const qb: any = { + where: jest.fn().mockReturnThis(), + orderBy: jest.fn().mockReturnThis(), + take: jest.fn().mockReturnThis(), + select: jest.fn().mockReturnThis(), + getMany: jest.fn().mockRejectedValue(new Error('connection terminated unexpectedly')), + }; + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await expect(service.getAutoComplete('java')).rejects.toMatchObject({ status: 503 }); + expect(metricsService.searchQueryFailuresCounter.inc).toHaveBeenCalledWith({ + failure_class: 'infrastructure', + }); + }); + + it('does not swallow an Elasticsearch-then-DB failure into an empty result', async () => { + // ES throws, DB path is attempted next and also fails outright — the + // caller must see an error, not a 200 with an empty catalogue. + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeFailingQb(new Error('ECONNREFUSED')); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await expect(service.search('react')).rejects.toBeDefined(); + }); + }); + it('autocomplete uses simple tsquery with prefix operator', async () => { const qb: any = { where: jest.fn().mockReturnThis(), diff --git a/src/search/search.service.ts b/src/search/search.service.ts index a62a6278..74ca4300 100644 --- a/src/search/search.service.ts +++ b/src/search/search.service.ts @@ -179,7 +179,17 @@ export class SearchService implements OnModuleInit { if (filters?.category) { const cats = Array.isArray(filters.category) ? filters.category : [filters.category]; - qb.andWhere('course.category IN (:cats)', { cats }); + qb.andWhere('course.category IN (:...cats)', { cats }); + } + + if (filters?.level) { + const levels = Array.isArray(filters.level) ? filters.level : [filters.level]; + qb.andWhere('course.level IN (:...levels)', { levels }); + } + + if (filters?.language) { + const languages = Array.isArray(filters.language) ? filters.language : [filters.language]; + qb.andWhere('course.language IN (:...languages)', { languages }); } if (filters?.price?.gte !== undefined) @@ -209,8 +219,7 @@ export class SearchService implements OnModuleInit { if (this.cacheManager) await this.cacheManager.set(cacheKey, result, SEARCH_CACHE_TTL_MS); return result; } catch (err) { - this.logger.error(`Search failed: ${(err as Error).message}`); - return { results: [], total: 0, page, limit, query: safeQuery }; + this.handleQueryFailure(err, 'Search'); } } @@ -243,8 +252,7 @@ export class SearchService implements OnModuleInit { this.autocompleteCache.set(query, results); return results; } catch (err) { - this.logger.error(`Autocomplete failed: ${(err as Error).message}`); - return []; + this.handleQueryFailure(err, 'Autocomplete'); } } @@ -359,6 +367,41 @@ export class SearchService implements OnModuleInit { // ── Private helpers ──────────────────────────────────────────────────────── + /** + * A repository/driver failure during search must never look like a + * genuine zero-result search — that hides real outages behind a 200 and + * makes error-rate metrics blind to them (issue #997). Logs, increments + * `search_query_failures_total` labelled by failure class, and rethrows: + * malformed filter values the caller supplied (bad UUID, invalid enum + * value — Postgres data-exception class '22xxx') become a 400; anything + * else (connection errors, timeouts, unexpected driver errors) becomes a + * 503 so the global exception filter and error-rate metrics see it. + */ + private handleQueryFailure(err: unknown, context: string): never { + const isCallerError = this.isCallerFilterError(err); + const failureClass = isCallerError ? 'caller' : 'infrastructure'; + + this.metricsService.searchQueryFailuresCounter.inc({ failure_class: failureClass }); + this.logger.error(`${context} failed (${failureClass}): ${(err as Error).message}`); + + if (isCallerError) { + throw new BadRequestException('One or more search filter values are invalid'); + } + throw new ServiceUnavailableException('Search is temporarily unavailable'); + } + + /** + * Postgres reports malformed input (invalid UUID, invalid enum value, bad + * numeric literal, ...) under the '22' (data exception) SQLSTATE class. + * Those stem directly from a filter value the caller supplied, as opposed + * to a connection failure, timeout, or a bug in our own SQL. + */ + private isCallerFilterError(err: unknown): boolean { + if (!(err instanceof QueryFailedError)) return false; + const code = (err as QueryFailedError & { code?: string }).code; + return typeof code === 'string' && code.startsWith('22'); + } + /** * Build a cache key that uniquely identifies every parameter that affects * the search result: query, filters, sort, page, and limit. Filters are diff --git a/src/slack.service.spec.ts b/src/slack.service.spec.ts index 808291fa..286f66ff 100644 --- a/src/slack.service.spec.ts +++ b/src/slack.service.spec.ts @@ -1,35 +1,126 @@ import axios from 'axios'; import { SlackService } from './slack.service'; +import { MetricsService } from './utils/masking/metrics.service'; jest.mock('axios'); const mockedAxios = axios as jest.Mocked; describe('SlackService', () => { const originalWebhook = process.env.SLACK_WEBHOOK_URL; + const originalTimeout = process.env.ALERT_DELIVERY_TIMEOUT_MS; + let metricsService: { alertDeliveryFailuresCounter: { inc: jest.Mock } }; + + beforeEach(() => { + metricsService = { alertDeliveryFailuresCounter: { inc: jest.fn() } }; + (mockedAxios as any).isAxiosError = jest.fn( + (err: unknown) => !!err && typeof err === 'object' && 'isAxiosError' in (err as object), + ); + }); afterEach(() => { process.env.SLACK_WEBHOOK_URL = originalWebhook; + process.env.ALERT_DELIVERY_TIMEOUT_MS = originalTimeout; jest.clearAllMocks(); }); it('does not post when SLACK_WEBHOOK_URL is unset', async () => { delete process.env.SLACK_WEBHOOK_URL; - const service = new SlackService(); + const service = new SlackService(metricsService as unknown as MetricsService); await service.sendAlert('test message', 'low'); expect(mockedAxios.post).not.toHaveBeenCalled(); }); - it('posts formatted high-severity alerts to the webhook', async () => { + it('posts formatted high-severity alerts to the webhook with a timeout', async () => { process.env.SLACK_WEBHOOK_URL = 'https://hooks.slack.com/services/test'; mockedAxios.post.mockResolvedValue({ status: 200, data: 'ok' }); - const service = new SlackService(); + const service = new SlackService(metricsService as unknown as MetricsService); await service.sendAlert('Integration check', 'high'); - expect(mockedAxios.post).toHaveBeenCalledWith('https://hooks.slack.com/services/test', { - text: '🚨 *TeachLink Alert* (HIGH)\nIntegration check', + expect(mockedAxios.post).toHaveBeenCalledWith( + 'https://hooks.slack.com/services/test', + { text: '🚨 *TeachLink Alert* (HIGH)\nIntegration check' }, + expect.objectContaining({ timeout: expect.any(Number) }), + ); + }); + + it('respects ALERT_DELIVERY_TIMEOUT_MS', async () => { + process.env.SLACK_WEBHOOK_URL = 'https://hooks.slack.com/services/test'; + process.env.ALERT_DELIVERY_TIMEOUT_MS = '1234'; + mockedAxios.post.mockResolvedValue({ status: 200 }); + + const service = new SlackService(metricsService as unknown as MetricsService); + await service.sendAlert('test', 'low'); + + expect(mockedAxios.post).toHaveBeenCalledWith( + expect.any(String), + expect.any(Object), + expect.objectContaining({ timeout: 1234 }), + ); + }); + + it('fails within the configured timeout on a non-responsive endpoint', async () => { + process.env.SLACK_WEBHOOK_URL = 'https://hooks.slack.com/services/test'; + mockedAxios.post.mockImplementation( + () => + new Promise((_resolve, reject) => { + setTimeout(() => reject({ isAxiosError: true, code: 'ECONNABORTED' }), 20); + }), + ); + + const service = new SlackService(metricsService as unknown as MetricsService); + await service.sendAlert('test', 'low'); + + expect(metricsService.alertDeliveryFailuresCounter.inc).toHaveBeenCalledWith({ + channel: 'slack', + }); + }); + + it('logs and increments the failure counter instead of swallowing the error silently', async () => { + process.env.SLACK_WEBHOOK_URL = 'https://hooks.slack.com/services/test'; + mockedAxios.post.mockRejectedValue({ + isAxiosError: true, + response: { status: 403 }, + message: 'Forbidden', + }); + + const service = new SlackService(metricsService as unknown as MetricsService); + const errorSpy = jest.spyOn((service as any).logger, 'error'); + + await service.sendAlert('test', 'low'); + + expect(metricsService.alertDeliveryFailuresCounter.inc).toHaveBeenCalledWith({ + channel: 'slack', }); + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining('403')); + }); + + it('retries a transient 5xx response and eventually succeeds', async () => { + process.env.SLACK_WEBHOOK_URL = 'https://hooks.slack.com/services/test'; + let calls = 0; + mockedAxios.post.mockImplementation(() => { + calls += 1; + if (calls < 2) { + return Promise.reject({ isAxiosError: true, response: { status: 503 } }); + } + return Promise.resolve({ status: 200 }); + }); + + const service = new SlackService(metricsService as unknown as MetricsService); + await service.sendAlert('test', 'low'); + + expect(calls).toBe(2); + }, 10000); + + it('does not retry a non-5xx (client) error', async () => { + process.env.SLACK_WEBHOOK_URL = 'https://hooks.slack.com/services/test'; + mockedAxios.post.mockRejectedValue({ isAxiosError: true, response: { status: 404 } }); + + const service = new SlackService(metricsService as unknown as MetricsService); + await service.sendAlert('test', 'low'); + + expect(mockedAxios.post).toHaveBeenCalledTimes(1); }); }); diff --git a/src/slack.service.ts b/src/slack.service.ts index b00050f6..ceb6b0c8 100644 --- a/src/slack.service.ts +++ b/src/slack.service.ts @@ -1,14 +1,21 @@ import { Injectable, Logger } from '@nestjs/common'; import axios from 'axios'; +import { MetricsService } from './utils/masking/metrics.service'; @Injectable() export class SlackService { + private readonly logger = new Logger(SlackService.name); + // This automatically reads the secret URL you just pasted into your .env file private readonly webhookUrl = process.env.SLACK_WEBHOOK_URL; + private readonly deliveryTimeoutMs = Number(process.env.ALERT_DELIVERY_TIMEOUT_MS) || 5000; + private readonly maxDeliveryRetries = 2; + private readonly retryBaseBackoffMs = 200; + + constructor(private readonly metricsService: MetricsService) {} async sendAlert(message: string, severity: 'low' | 'medium' | 'high') { if (!this.webhookUrl) { - // console.error('Slack Webhook URL is missing in .env file!'); return; } @@ -25,13 +32,7 @@ export class SlackService { text: `${emoji} *TeachLink Alert* (${severity.toUpperCase()})\n${message}`, }; - try { - // Send the message over the internet to Slack - await axios.post(this.webhookUrl, payload); - // console.log(`Slack alert (${severity}) sent successfully!`); - } catch (_error) { - // console.error('Failed to send Slack alert:', error.message); - } + await this.postWithRetry(payload); } async sendNotification(channel: string, message: string): Promise { @@ -45,5 +46,43 @@ export class SlackService { }); } } -} + + /** + * POST the alert payload with a bounded timeout. Transient 5xx responses are + * retried with jittered backoff up to `maxDeliveryRetries` times; every + * failed attempt increments the failure counter, and a delivery that fails + * for good is logged with the response status instead of swallowed. + */ + private async postWithRetry(payload: unknown, attempt = 0): Promise { + try { + await axios.post(this.webhookUrl, payload, { timeout: this.deliveryTimeoutMs }); + } catch (error) { + const status = axios.isAxiosError(error) ? error.response?.status : undefined; + this.metricsService.alertDeliveryFailuresCounter.inc({ channel: 'slack' }); + + const isTransient5xx = typeof status === 'number' && status >= 500 && status < 600; + if (isTransient5xx && attempt < this.maxDeliveryRetries) { + const backoffMs = + this.retryBaseBackoffMs * 2 ** attempt + Math.random() * this.retryBaseBackoffMs; + this.logger.warn( + `Slack alert delivery failed with status ${status}, retrying ` + + `(attempt ${attempt + 1}/${this.maxDeliveryRetries}) in ${Math.round(backoffMs)}ms`, + ); + await new Promise((resolve) => setTimeout(resolve, backoffMs)); + return this.postWithRetry(payload, attempt + 1); + } + + if (isTransient5xx) { + this.logger.warn(`Slack alert delivery exhausted retries after ${attempt + 1} attempts`); + } + + const errorMessage = + error instanceof Error + ? error.message + : ((error as { message?: string })?.message ?? String(error)); + this.logger.error( + `Failed to send Slack alert (status: ${status ?? 'unknown'}): ${errorMessage}`, + ); + } + } } diff --git a/src/utils/masking/metrics.service.ts b/src/utils/masking/metrics.service.ts index 5d219d6b..123efa12 100644 --- a/src/utils/masking/metrics.service.ts +++ b/src/utils/masking/metrics.service.ts @@ -15,6 +15,8 @@ export class MetricsService { // Counters public readonly paymentsTotalCounter: Counter; public readonly searchFallbackCounter: Counter; + public readonly searchQueryFailuresCounter: Counter; + public readonly alertDeliveryFailuresCounter: Counter; // Histograms public readonly apiLatencyHistogram: Histogram; @@ -78,6 +80,20 @@ export class MetricsService { registers: [this.registry], }); + this.searchQueryFailuresCounter = new Counter({ + name: 'teachlink_search_query_failures_total', + help: 'Total number of search queries that failed outright (non-2xx response)', + labelNames: ['failure_class'], // 'infrastructure', 'caller' + registers: [this.registry], + }); + + this.alertDeliveryFailuresCounter = new Counter({ + name: 'teachlink_alert_delivery_failures_total', + help: 'Total number of failed outbound alert delivery attempts', + labelNames: ['channel'], // 'slack', 'pagerduty' + registers: [this.registry], + }); + // --- Initialize Histograms --- this.apiLatencyHistogram = new Histogram({ name: 'teachlink_api_latency_seconds',