fix(listings): return 404 instead of 500 for non-existent listing detail

Move not-found handling from the query handler to the controller layer
following DDD conventions: the handler now returns null when a listing
is not found, and the controller maps that to NotFoundException (404).

Key changes:
- Handler returns ListingDetailData | null instead of throwing
- Use ListingNotFoundSignal to prevent caching null results
- Add `return await` to properly catch errors in try/catch
- Controller throws NotFoundException with listing ID in message
- Strengthen E2E test to assert exactly 404 (was [404, 400])
- Add unit tests: not-found returns null, unexpected error → 500
- Fix missing LoggerService mock in handler test

Co-Authored-By: Paperclip <noreply@paperclip.ing>

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<typeof vi.fn> };
   let mockCache: { getOrSet: ReturnType<typeof vi.fn>; invalidate: ReturnType<typeof vi.fn>; invalidateByPrefix: ReturnType<typeof vi.fn> };
+  let mockLogger: { log: ReturnType<typeof vi.fn>; error: ReturnType<typeof vi.fn>; warn: ReturnType<typeof vi.fn>; debug: ReturnType<typeof vi.fn> };
 
   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<unknown>) => 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<unknown>) => 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();
+  });
 });

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<GetListingQuery> {
     private readonly logger: LoggerService,
   ) {}
 
-  async execute(query: GetListingQuery): Promise<ListingDetailData> {
+  /**
+   * Returns listing detail or null when not found.
+   * The controller is responsible for mapping null to a 404 HttpException.
+   */
+  async execute(query: GetListingQuery): Promise<ListingDetailData | null> {
     try {
       const cacheKey = CacheService.buildKey(CachePrefix.LISTING, query.listingId);
 
-      return this.cache.getOrSet(
+      // Check cache first
+      const cached = await this.cache.getOrSet<ListingDetailData | null>(
         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<GetListingQuery> {
     }
   }
 }
+
+/** Internal signal to avoid caching not-found results. Not a domain exception. */
+class ListingNotFoundSignal extends Error {
+  constructor() {
+    super('listing_not_found');
+    this.name = 'ListingNotFoundSignal';
+  }
+}

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', () => {

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<ListingDetailData> {
-    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' })

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);
     });
   });