Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mobile/openapi/README.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions mobile/openapi/lib/api.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion mobile/openapi/lib/api/notifications_api.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions mobile/openapi/lib/api_client.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

99 changes: 99 additions & 0 deletions mobile/openapi/lib/model/test_email_response_dto.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions open-api/immich-openapi-specs.json
Original file line number Diff line number Diff line change
Expand Up @@ -3491,6 +3491,13 @@
},
"responses": {
"200": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/TestEmailResponseDto"
}
}
},
Comment on lines +3494 to +3500
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously no default response type.

"description": ""
}
},
Expand Down Expand Up @@ -12348,6 +12355,17 @@
},
"type": "object"
},
"TestEmailResponseDto": {
"properties": {
"messageId": {
"type": "string"
}
},
"required": [
"messageId"
],
"type": "object"
},
"TimeBucketResponseDto": {
"properties": {
"count": {
Expand Down
8 changes: 7 additions & 1 deletion open-api/typescript-sdk/src/fetch-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,9 @@ export type SystemConfigSmtpDto = {
replyTo: string;
transport: SystemConfigSmtpTransportDto;
};
export type TestEmailResponseDto = {
messageId: string;
};
export type OAuthConfigDto = {
redirectUri: string;
};
Expand Down Expand Up @@ -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({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed from fetchText to fetchJson

return oazapfts.ok(oazapfts.fetchJson<{
status: 200;
data: TestEmailResponseDto;
}>("/notifications/test-email", oazapfts.json({
...opts,
method: "POST",
body: systemConfigSmtpDto
Expand Down
3 changes: 2 additions & 1 deletion server/src/controllers/notification.controller.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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);
}
}
3 changes: 3 additions & 0 deletions server/src/dtos/notification.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export class TestEmailResponseDto {
messageId!: string;
}
5 changes: 0 additions & 5 deletions server/src/services/notification.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: '' });
Expand Down
12 changes: 5 additions & 7 deletions server/src/services/notification.service.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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 });
Expand All @@ -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,
Expand All @@ -161,6 +161,8 @@ export class NotificationService {
replyTo: dto.replyTo || dto.from,
smtp: dto.transport,
});

return { messageId };
}

async handleUserSignup({ id, tempPassword }: INotifySignupJob) {
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion server/test/repositories/notification.repository.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
};
};
16 changes: 14 additions & 2 deletions web/src/lib/utils/handle-error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading