From 005528ab5ec6514e2b93a52a5dbe43481821b733 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Wed, 25 Sep 2024 12:05:03 -0400 Subject: [PATCH] fix(server): http error parsing on endpoints without a default response (#12927) --- mobile/openapi/README.md | Bin 32081 -> 32137 bytes mobile/openapi/lib/api.dart | Bin 11461 -> 11504 bytes mobile/openapi/lib/api/notifications_api.dart | Bin 1653 -> 2154 bytes mobile/openapi/lib/api_client.dart | Bin 29487 -> 29579 bytes .../lib/model/test_email_response_dto.dart | Bin 0 -> 2944 bytes open-api/immich-openapi-specs.json | 18 ++++++++++++++++++ open-api/typescript-sdk/src/fetch-client.ts | 8 +++++++- .../controllers/notification.controller.ts | 3 ++- server/src/dtos/notification.dto.ts | 3 +++ .../src/services/notification.service.spec.ts | 5 ----- server/src/services/notification.service.ts | 12 +++++------- .../notification.repository.mock.ts | 2 +- web/src/lib/utils/handle-error.ts | 16 ++++++++++++++-- 13 files changed, 50 insertions(+), 17 deletions(-) create mode 100644 mobile/openapi/lib/model/test_email_response_dto.dart create mode 100644 server/src/dtos/notification.dto.ts diff --git a/mobile/openapi/README.md b/mobile/openapi/README.md index 285514e11cd549f09303a11c9467ce1247ea5f6d..b6b0897e8f5e075202151fa3e6594def3beda763 100644 GIT binary patch delta 54 zcmccki?Q=J;|AR_<<#O5*WAR+oS@X=g8aPVRF{(cSdEnYWPSY*nABtef04~PWvc7| DYhM(a delta 14 VcmeDD&3N$_;|ATb&8cN7>;N}n23`OF diff --git a/mobile/openapi/lib/api.dart b/mobile/openapi/lib/api.dart index fc0224a8c2072997f80213d6da70f07eae2e3ccc..d08b6fc52138b309c0cd788fa084233be5017aa5 100644 GIT binary patch delta 30 mcmX>a`5|%xiyCKYaY=k?ZenK6kY5XB-Xf`uSr=`%$>AY4?e6p;`U#3DufYO~%=yoI~nV|U^uO307M9})al z_V&(Qw96I)yYKCrH*bD+zxO^zm4gTJJV=#J#^f_=Jl(Lp{u)<~c2+>Qi|3dO6da|N z37*^=Il1x784qLeVxt9e&9RU?E1r4SDb7yLJII9}t0WN`Bcjko1kFJWazweu;F!QV zrQ%EOG;8xJZ`yY-iPXyRMQDxm;|1D}tb|lB8x5>atNV92 zzQQeYc3+Mh7~8jo$#5GVr3d_F1eGhzP?_#0TJRtu$xy3Azf;dIOxRivt9a_5xor{6 bP8-T&8+2K=Da*2~o1MVSlBB?vwPo!WQf{HH delta 22 dcmaDQ@RetSA1g~)erC$%WL9}bmRhb_E&yMh2Lb>9 diff --git a/mobile/openapi/lib/api_client.dart b/mobile/openapi/lib/api_client.dart index 828c0b9ed925c95858c5c53f6cd1edd026c590be..c62d1c5b2e2b22838391733471cfad653a35c7e5 100644 GIT binary patch delta 41 qcmZ4gjIsMUk%mc delta 14 VcmeBv&ba;=Tmj$)rY7HH_5%eRrfJ z%P5;>3y?%2kN5IC&mA?Hj3yI!^T%@W^7r}O{Qk{yehoLb@8>aGFW_#mgb$1B+nc}6 z(2Oi!=SS(0E+3E&i>U#{ZUUgU+=HAJ3FDmPwn690Q6%aP5wpV6aL^ZqiE04Vu}K$@JBa zX;w0AMk5SoK^35CE?F%S{Jk2DvWgi4OK$9Z%S3h0O^p%xwN>yq0+=8O_uMvG8eky# z4pt%CH3*kx#uGdgvWUnVKnDTXQkZ1RjbR&po5#3$xWIG(K3Xm02Kp_x_*#ADS|bE# z@Ss#hMQCP~hUqj~JbdCAmS#(?FJL+Y4^_shh@!;Z&X+I#3B)%FK0kLkI!DLcFvJtt z2x(!%EtJfZzp0#KeCMPW4{1j7j4IW8;v8u#xe>;hMSYz~=fT4;thj~5D*q6@fVm>R-Hr81g+zP-HW(>a9@{J$iZyAz1~Va0#aHmG$G{$|ViXVx`A@ zi18ZjPbu}L+G)qx)53um{?D2MCUfvDWTlG0LaIV-XhyR%d|G36J6%Kp{m;b3fG_4##} zHuXlcoH{CUNyhLZMBNMIojL?7@OC6qtf6xQa6*oAm1NA=uV{+Af_%GC>ju+~Tr@K3 zS-gZTtN$cy7sYa4^Vah$B029HWFEG_m;WMak>jt?z z!O+KibltwuYR4WbY%VBRWq5q(Q1O^(Zko!HWP<@#>m#(}mXM`%;QZVvGZE%a;ry%Z zzVnM{<8X;u_iBgp&6yteJ3br{7&;U=A%(iCuz(gQ!tBWL+0-d?=$3{gm6NbM_evO? z^e|H+k)IRL+f!CiX;BB+C3H{Di)+A^k>NlHWj`Z11bOMw_3)q-LEp`mpsQ|lP~{y3 z82?EIoZZwj^ev&|FljI8OX%;@6t6VAx#@=XQ}%>ST-E!I&xGCtH(f=9s;}yKygFxz zvmLPX9exq8l|T45-_zuD=ezB#=_F^m1dnoO8IMWCv1sJ)KhfZhuC=O zP4Q$qO7R-NBlPJ*9#L8Wg^;Sk?d5br+d-|w@Vp+zz(W;15$^wJ>6tQlj^x_8H<%CK zRTS2CN_dEsgedDF90)7!Tlm<_aj&+(B*1V3kxXD&@?H=92N+7v;FZmg@&={yzyWVV qgJG#P7`*+sxT9~mRtfHk?7!aL(b>M`XO9k&!PkKMoVyy~u=y8?V!}KC literal 0 HcmV?d00001 diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index 99ea313063..1a070f126b 100644 --- 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 index d1b88afabb..f2f946f262 100644 --- 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 index 2772e93b5d..3dd72dd73a 100644 --- 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 { return this.service.sendTestEmail(auth.user.id, dto); } } diff --git a/server/src/dtos/notification.dto.ts b/server/src/dtos/notification.dto.ts new file mode 100644 index 0000000000..34b3923580 --- /dev/null +++ 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 index 9ef1310bfb..a0b9436f75 100644 --- 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: 'test@immich.app' } } }); notificationMock.sendEmail.mockResolvedValue({ messageId: '', response: '' }); diff --git a/server/src/services/notification.service.ts b/server/src/services/notification.service.ts index 4eef49c631..bdb23ce700 100644 --- 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 index 71975b429c..16862dc3d7 100644 --- 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 => { 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 index 9ca5bc8773..a7e9a4340c 100644 --- 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) {