fix(server): http error parsing on endpoints without a default response

immich-app · Sep 25, 2024 · 224d2aa · 224d2aa
1 parent 46fe606
commit 224d2aa
Show file tree

Hide file tree

Showing 13 changed files with 162 additions and 18 deletions.
diff --git a/mobile/openapi/README.md b/mobile/openapi/README.md
diff --git a/mobile/openapi/lib/api.dart b/mobile/openapi/lib/api.dart
diff --git a/mobile/openapi/lib/api/notifications_api.dart b/mobile/openapi/lib/api/notifications_api.dart
diff --git a/mobile/openapi/lib/api_client.dart b/mobile/openapi/lib/api_client.dart
diff --git a/mobile/openapi/lib/model/test_email_response_dto.dart b/mobile/openapi/lib/model/test_email_response_dto.dart
diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json
@@ -3491,6 +3491,13 @@
         },
         "responses": {
           "200": {
+            "content": {
+              "application/json": {
+                "schema": {
+                  "$ref": "#/components/schemas/TestEmailResponseDto"
+                }
+              }
+            },
             "description": ""
           }
         },
@@ -12348,6 +12355,17 @@
         },
         "type": "object"
       },
+      "TestEmailResponseDto": {
+        "properties": {
+          "messageId": {
+            "type": "string"
+          }
+        },
+        "required": [
+          "messageId"
+        ],
+        "type": "object"
+      },
       "TimeBucketResponseDto": {
         "properties": {
           "count": {

diff --git a/open-api/typescript-sdk/src/fetch-client.ts b/open-api/typescript-sdk/src/fetch-client.ts
@@ -656,6 +656,9 @@ export type SystemConfigSmtpDto = {
     replyTo: string;
     transport: SystemConfigSmtpTransportDto;
 };
+export type TestEmailResponseDto = {
+    messageId: string;
+};
 export type OAuthConfigDto = {
     redirectUri: string;
 };
@@ -2220,7 +2223,10 @@ export function addMemoryAssets({ id, bulkIdsDto }: {
 export function sendTestEmail({ systemConfigSmtpDto }: {
     systemConfigSmtpDto: SystemConfigSmtpDto;
 }, opts?: Oazapfts.RequestOpts) {
-    return oazapfts.ok(oazapfts.fetchText("/notifications/test-email", oazapfts.json({
+    return oazapfts.ok(oazapfts.fetchJson<{
+        status: 200;
+        data: TestEmailResponseDto;
+    }>("/notifications/test-email", oazapfts.json({
         ...opts,
         method: "POST",
         body: systemConfigSmtpDto

diff --git a/server/src/controllers/notification.controller.ts b/server/src/controllers/notification.controller.ts
@@ -1,6 +1,7 @@
 import { Body, Controller, HttpCode, HttpStatus, Post } from '@nestjs/common';
 import { ApiTags } from '@nestjs/swagger';
 import { AuthDto } from 'src/dtos/auth.dto';
+import { TestEmailResponseDto } from 'src/dtos/notification.dto';
 import { SystemConfigSmtpDto } from 'src/dtos/system-config.dto';
 import { Auth, Authenticated } from 'src/middleware/auth.guard';
 import { NotificationService } from 'src/services/notification.service';
@@ -13,7 +14,7 @@ export class NotificationController {
   @Post('test-email')
   @HttpCode(HttpStatus.OK)
   @Authenticated({ admin: true })
-  sendTestEmail(@Auth() auth: AuthDto, @Body() dto: SystemConfigSmtpDto) {
+  sendTestEmail(@Auth() auth: AuthDto, @Body() dto: SystemConfigSmtpDto): Promise<TestEmailResponseDto> {
     return this.service.sendTestEmail(auth.user.id, dto);
   }
 }
diff --git a/server/src/dtos/notification.dto.ts b/server/src/dtos/notification.dto.ts
@@ -0,0 +1,3 @@
+export class TestEmailResponseDto {
+  messageId!: string;
+}
diff --git a/server/src/services/notification.service.spec.ts b/server/src/services/notification.service.spec.ts
@@ -616,11 +616,6 @@ describe(NotificationService.name, () => {
       await expect(sut.handleSendEmail({ html: '', subject: '', text: '', to: '' })).resolves.toBe(JobStatus.SKIPPED);
     });
 
-    it('should fail if email could not be sent', async () => {
-      systemMock.get.mockResolvedValue({ notifications: { smtp: { enabled: true } } });
-      await expect(sut.handleSendEmail({ html: '', subject: '', text: '', to: '' })).resolves.toBe(JobStatus.FAILED);
-    });
-
     it('should send mail successfully', async () => {
       systemMock.get.mockResolvedValue({ notifications: { smtp: { enabled: true, from: '[email protected]' } } });
       notificationMock.sendEmail.mockResolvedValue({ messageId: '', response: '' });

diff --git a/server/src/services/notification.service.ts b/server/src/services/notification.service.ts
@@ -1,4 +1,4 @@
-import { HttpException, HttpStatus, Inject, Injectable } from '@nestjs/common';
+import { BadRequestException, Inject, Injectable } from '@nestjs/common';
 import { DEFAULT_EXTERNAL_DOMAIN } from 'src/constants';
 import { SystemConfigCore } from 'src/cores/system-config.core';
 import { OnEmit } from 'src/decorators';
@@ -140,7 +140,7 @@ export class NotificationService {
     try {
       await this.notificationRepository.verifySmtp(dto.transport);
     } catch (error) {
-      throw new HttpException('Failed to verify SMTP configuration', HttpStatus.BAD_REQUEST, { cause: error });
+      throw new BadRequestException('Failed to verify SMTP configuration', { cause: error });
     }
 
     const { server } = await this.configCore.getConfig({ withCache: false });
@@ -152,7 +152,7 @@ export class NotificationService {
       },
     });
 
-    await this.notificationRepository.sendEmail({
+    const { messageId } = await this.notificationRepository.sendEmail({
       to: user.email,
       subject: 'Test email from Immich',
       html,
@@ -161,6 +161,8 @@ export class NotificationService {
       replyTo: dto.replyTo || dto.from,
       smtp: dto.transport,
     });
+
+    return { messageId };
   }
 
   async handleUserSignup({ id, tempPassword }: INotifySignupJob) {
@@ -312,10 +314,6 @@ export class NotificationService {
       imageAttachments: data.imageAttachments,
     });
 
-    if (!response) {
-      return JobStatus.FAILED;
-    }
-
     this.logger.log(`Sent mail with id: ${response.messageId} status: ${response.response}`);
 
     return JobStatus.SUCCESS;

diff --git a/server/test/repositories/notification.repository.mock.ts b/server/test/repositories/notification.repository.mock.ts
@@ -4,7 +4,7 @@ import { Mocked } from 'vitest';
 export const newNotificationRepositoryMock = (): Mocked<INotificationRepository> => {
   return {
     renderEmail: vitest.fn(),
-    sendEmail: vitest.fn(),
+    sendEmail: vitest.fn().mockResolvedValue({ messageId: 'message-1' }),
     verifySmtp: vitest.fn(),
   };
 };
diff --git a/web/src/lib/utils/handle-error.ts b/web/src/lib/utils/handle-error.ts
@@ -2,9 +2,21 @@ import { isHttpError } from '@immich/sdk';
 import { notificationController, NotificationType } from '../components/shared-components/notification/notification';
 
 export function getServerErrorMessage(error: unknown) {
-  if (isHttpError(error)) {
-    return error.data?.message || error.message;
+  if (!isHttpError(error)) {
+    return;
   }
+
+  // errors for endpoints without return types aren't parsed as json
+  let data = error.data;
+  if (typeof data === 'string') {
+    try {
+      data = JSON.parse(data);
+    } catch {
+      // Not a JSON string
+    }
+  }
+
+  return data?.message || error.message;
 }
 
 export function handleError(error: unknown, message: string) {