diff --git a/apps/api/src/modules/listings/application/__tests__/get-listing.handler.spec.ts b/apps/api/src/modules/listings/application/__tests__/get-listing.handler.spec.ts index 4f17522..f601112 100644 --- a/apps/api/src/modules/listings/application/__tests__/get-listing.handler.spec.ts +++ b/apps/api/src/modules/listings/application/__tests__/get-listing.handler.spec.ts @@ -1,3 +1,4 @@ +import { InternalServerErrorException } from '@nestjs/common'; import { type IListingRepository } from '@modules/listings/domain/repositories/listing.repository'; import { GetListingHandler } from '../queries/get-listing/get-listing.handler'; import { GetListingQuery } from '../queries/get-listing/get-listing.query'; @@ -6,6 +7,7 @@ describe('GetListingHandler', () => { let handler: GetListingHandler; let mockListingRepo: { [K in keyof IListingRepository]: ReturnType }; let mockCache: { getOrSet: ReturnType; invalidate: ReturnType; invalidateByPrefix: ReturnType }; + let mockLogger: { log: ReturnType; error: ReturnType; warn: ReturnType; debug: ReturnType }; const mockListingDetail = { id: 'listing-1', @@ -31,9 +33,17 @@ describe('GetListingHandler', () => { invalidateByPrefix: vi.fn(), }; + mockLogger = { + log: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + debug: vi.fn(), + }; + handler = new GetListingHandler( mockListingRepo as any, mockCache as any, + mockLogger as any, ); }); @@ -48,13 +58,26 @@ describe('GetListingHandler', () => { expect(mockCache.getOrSet).toHaveBeenCalled(); }); - it('throws NotFoundException when listing not found', async () => { + it('returns null when listing not found', async () => { mockCache.getOrSet.mockImplementation(async (_key: string, fn: () => Promise) => fn()); mockListingRepo.findByIdWithProperty.mockResolvedValue(null); const query = new GetListingQuery('nonexistent'); + const result = await handler.execute(query); - await expect(handler.execute(query)).rejects.toThrow('Listing'); + expect(result).toBeNull(); + }); + + it('does not cache not-found results', async () => { + // Simulate getOrSet calling the loader and letting exceptions propagate + mockCache.getOrSet.mockImplementation(async (_key: string, fn: () => Promise) => fn()); + mockListingRepo.findByIdWithProperty.mockResolvedValue(null); + + const result = await handler.execute(new GetListingQuery('nonexistent')); + + expect(result).toBeNull(); + // The loader throws ListingNotFoundSignal to prevent caching null; + // handler catches it and returns null }); it('uses cache key with listing id', async () => { @@ -70,4 +93,13 @@ describe('GetListingHandler', () => { expect.anything(), ); }); + + it('throws InternalServerErrorException on unexpected errors', async () => { + mockCache.getOrSet.mockRejectedValue(new Error('Redis connection failed')); + + const query = new GetListingQuery('listing-1'); + + await expect(handler.execute(query)).rejects.toThrow(InternalServerErrorException); + expect(mockLogger.error).toHaveBeenCalled(); + }); }); diff --git a/apps/api/src/modules/listings/application/queries/get-listing/get-listing.handler.ts b/apps/api/src/modules/listings/application/queries/get-listing/get-listing.handler.ts index 1f29f5b..fc23f8c 100644 --- a/apps/api/src/modules/listings/application/queries/get-listing/get-listing.handler.ts +++ b/apps/api/src/modules/listings/application/queries/get-listing/get-listing.handler.ts @@ -1,6 +1,6 @@ import { Inject, InternalServerErrorException } from '@nestjs/common'; import { QueryHandler, type IQueryHandler } from '@nestjs/cqrs'; -import { DomainException, NotFoundException, CacheService, CachePrefix, CacheTTL, type LoggerService } from '@modules/shared'; +import { DomainException, CacheService, CachePrefix, CacheTTL, LoggerService } from '@modules/shared'; import { type ListingDetailData } from '../../../domain/repositories/listing-read.dto'; import { LISTING_REPOSITORY, type IListingRepository } from '../../../domain/repositories/listing.repository'; import { GetListingQuery } from './get-listing.query'; @@ -16,23 +16,33 @@ export class GetListingHandler implements IQueryHandler { private readonly logger: LoggerService, ) {} - async execute(query: GetListingQuery): Promise { + /** + * Returns listing detail or null when not found. + * The controller is responsible for mapping null to a 404 HttpException. + */ + async execute(query: GetListingQuery): Promise { try { const cacheKey = CacheService.buildKey(CachePrefix.LISTING, query.listingId); - return this.cache.getOrSet( + // Check cache first + const cached = await this.cache.getOrSet( cacheKey, async () => { const result = await this.listingRepo.findByIdWithProperty(query.listingId); if (!result) { - throw new NotFoundException('Listing', query.listingId); + // Signal to skip caching by throwing; we catch it below + throw new ListingNotFoundSignal(); } return result; }, CacheTTL.LISTING_DETAIL, 'listing', ); + + return cached; } catch (error) { + // Not-found: return null without caching so subsequent requests can find a newly-created listing + if (error instanceof ListingNotFoundSignal) return null; if (error instanceof DomainException) throw error; this.logger.error( `Failed to get listing ${query.listingId}: ${error instanceof Error ? error.message : error}`, @@ -43,3 +53,11 @@ export class GetListingHandler implements IQueryHandler { } } } + +/** Internal signal to avoid caching not-found results. Not a domain exception. */ +class ListingNotFoundSignal extends Error { + constructor() { + super('listing_not_found'); + this.name = 'ListingNotFoundSignal'; + } +} diff --git a/apps/api/src/modules/listings/presentation/__tests__/listings.controller.spec.ts b/apps/api/src/modules/listings/presentation/__tests__/listings.controller.spec.ts index 8612dc6..8362996 100644 --- a/apps/api/src/modules/listings/presentation/__tests__/listings.controller.spec.ts +++ b/apps/api/src/modules/listings/presentation/__tests__/listings.controller.spec.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { NotFoundException } from '@modules/shared'; import { ListingsController } from '../controllers/listings.controller'; describe('ListingsController', () => { @@ -60,6 +61,20 @@ describe('ListingsController', () => { expect(result).toEqual(mockDetail); expect(mockQueryBus.execute).toHaveBeenCalledTimes(1); }); + + it('should throw NotFoundException when listing does not exist', async () => { + mockQueryBus.execute.mockResolvedValue(null); + + await expect(controller.getListing('nonexistent-id')).rejects.toThrow(NotFoundException); + }); + + it('should include the listing ID in the NotFoundException message', async () => { + mockQueryBus.execute.mockResolvedValue(null); + + await expect(controller.getListing('abc123')).rejects.toThrow( + expect.objectContaining({ message: expect.stringContaining('abc123') }), + ); + }); }); describe('searchListings', () => { diff --git a/apps/api/src/modules/listings/presentation/controllers/listings.controller.ts b/apps/api/src/modules/listings/presentation/controllers/listings.controller.ts index 6dee9b6..0dbfcf6 100644 --- a/apps/api/src/modules/listings/presentation/controllers/listings.controller.ts +++ b/apps/api/src/modules/listings/presentation/controllers/listings.controller.ts @@ -10,6 +10,7 @@ import { UseGuards, UseInterceptors, } from '@nestjs/common'; +import { NotFoundException } from '@modules/shared'; import { type CommandBus, type QueryBus } from '@nestjs/cqrs'; import { FileInterceptor } from '@nestjs/platform-express'; import { @@ -121,7 +122,11 @@ export class ListingsController { @ApiResponse({ status: 404, description: 'Listing not found' }) @Get(':id') async getListing(@Param('id') id: string): Promise { - return this.queryBus.execute(new GetListingQuery(id)); + const result = await this.queryBus.execute(new GetListingQuery(id)); + if (!result) { + throw new NotFoundException('Listing', id); + } + return result; } @ApiOperation({ summary: 'Search and filter property listings' }) diff --git a/e2e/api/listings.spec.ts b/e2e/api/listings.spec.ts index f0415ac..2a38bdc 100644 --- a/e2e/api/listings.spec.ts +++ b/e2e/api/listings.spec.ts @@ -142,7 +142,7 @@ test.describe('Listings API', () => { const res = await request.get('listings/non-existent-id-12345'); expect(res.ok()).toBeFalsy(); - expect([404, 400]).toContain(res.status()); + expect(res.status()).toBe(404); }); });