From a523eadec2db7a42c69236a9bc3fe896b69892e9 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Sat, 9 Jan 2021 22:38:10 +0100 Subject: [PATCH] NotesController: Do not crash on nonexistent notes This commit adds proper error handling and returns 404 when a note does not exist. Previously, we leaked the `NotInDBError` and sent a 500 status code. Signed-off-by: David Mehren --- src/api/public/notes/notes.controller.ts | 95 ++++++++++++++++++++---- test/public-api/notes.e2e-spec.ts | 45 ++++++++++- 2 files changed, 123 insertions(+), 17 deletions(-) diff --git a/src/api/public/notes/notes.controller.ts b/src/api/public/notes/notes.controller.ts index 02e337af3..e50437867 100644 --- a/src/api/public/notes/notes.controller.ts +++ b/src/api/public/notes/notes.controller.ts @@ -10,12 +10,13 @@ import { Delete, Get, Header, + NotFoundException, Param, Post, Put, } from '@nestjs/common'; +import { NotInDBError } from '../../../errors/errors'; import { ConsoleLoggerService } from '../../../logger/console-logger.service'; -import { NoteMetadataUpdateDto } from '../../../notes/note-metadata.dto'; import { NotePermissionsUpdateDto } from '../../../notes/note-permissions.dto'; import { NotesService } from '../../../notes/notes.service'; import { RevisionsService } from '../../../revisions/revisions.service'; @@ -38,8 +39,15 @@ export class NotesController { } @Get(':noteIdOrAlias') - getNote(@Param('noteIdOrAlias') noteIdOrAlias: string) { - return this.noteService.getNoteDtoByIdOrAlias(noteIdOrAlias); + async getNote(@Param('noteIdOrAlias') noteIdOrAlias: string) { + try { + return await this.noteService.getNoteDtoByIdOrAlias(noteIdOrAlias); + } catch (e) { + if (e instanceof NotInDBError) { + throw new NotFoundException(e.message); + } + throw e; + } } @Post(':noteAlias') @@ -54,7 +62,14 @@ export class NotesController { @Delete(':noteIdOrAlias') async deleteNote(@Param('noteIdOrAlias') noteIdOrAlias: string) { this.logger.debug('Deleting note: ' + noteIdOrAlias); - await this.noteService.deleteNoteByIdOrAlias(noteIdOrAlias); + try { + await this.noteService.deleteNoteByIdOrAlias(noteIdOrAlias); + } catch (e) { + if (e instanceof NotInDBError) { + throw new NotFoundException(e.message); + } + throw e; + } this.logger.debug('Successfully deleted ' + noteIdOrAlias); return; } @@ -65,38 +80,88 @@ export class NotesController { @MarkdownBody() text: string, ) { this.logger.debug('Got raw markdown:\n' + text); - return this.noteService.updateNoteByIdOrAlias(noteIdOrAlias, text); + try { + return await this.noteService.updateNoteByIdOrAlias(noteIdOrAlias, text); + } catch (e) { + if (e instanceof NotInDBError) { + throw new NotFoundException(e.message); + } + throw e; + } } @Get(':noteIdOrAlias/content') @Header('content-type', 'text/markdown') - getNoteContent(@Param('noteIdOrAlias') noteIdOrAlias: string) { - return this.noteService.getNoteContent(noteIdOrAlias); + async getNoteContent(@Param('noteIdOrAlias') noteIdOrAlias: string) { + try { + return await this.noteService.getNoteContent(noteIdOrAlias); + } catch (e) { + if (e instanceof NotInDBError) { + throw new NotFoundException(e.message); + } + throw e; + } } @Get(':noteIdOrAlias/metadata') - getNoteMetadata(@Param('noteIdOrAlias') noteIdOrAlias: string) { - return this.noteService.getNoteMetadata(noteIdOrAlias); + async getNoteMetadata(@Param('noteIdOrAlias') noteIdOrAlias: string) { + try { + return await this.noteService.getNoteMetadata(noteIdOrAlias); + } catch (e) { + if (e instanceof NotInDBError) { + throw new NotFoundException(e.message); + } + throw e; + } } @Put(':noteIdOrAlias/permissions') - updateNotePermissions( + async updateNotePermissions( @Param('noteIdOrAlias') noteIdOrAlias: string, @Body() updateDto: NotePermissionsUpdateDto, ) { - return this.noteService.updateNotePermissions(noteIdOrAlias, updateDto); + try { + return await this.noteService.updateNotePermissions( + noteIdOrAlias, + updateDto, + ); + } catch (e) { + if (e instanceof NotInDBError) { + throw new NotFoundException(e.message); + } + throw e; + } } @Get(':noteIdOrAlias/revisions') - getNoteRevisions(@Param('noteIdOrAlias') noteIdOrAlias: string) { - return this.revisionsService.getNoteRevisionMetadatas(noteIdOrAlias); + async getNoteRevisions(@Param('noteIdOrAlias') noteIdOrAlias: string) { + try { + return await this.revisionsService.getNoteRevisionMetadatas( + noteIdOrAlias, + ); + } catch (e) { + if (e instanceof NotInDBError) { + throw new NotFoundException(e.message); + } + throw e; + } } @Get(':noteIdOrAlias/revisions/:revisionId') - getNoteRevision( + async getNoteRevision( @Param('noteIdOrAlias') noteIdOrAlias: string, @Param('revisionId') revisionId: number, ) { - return this.revisionsService.getNoteRevision(noteIdOrAlias, revisionId); + try { + return await this.revisionsService.getNoteRevision( + noteIdOrAlias, + revisionId, + ); + } catch (e) { + if (e instanceof NotInDBError) { + throw new NotFoundException(e.message); + } + throw e; + } } } diff --git a/test/public-api/notes.e2e-spec.ts b/test/public-api/notes.e2e-spec.ts index e308fccbb..2fcf1fe70 100644 --- a/test/public-api/notes.e2e-spec.ts +++ b/test/public-api/notes.e2e-spec.ts @@ -59,12 +59,19 @@ describe('Notes', () => { }); it(`GET /notes/{note}`, async () => { + // check if we can succefully get a note that exists await notesService.createNote('This is a test note.', 'test1'); const response = await request(app.getHttpServer()) .get('/notes/test1') .expect('Content-Type', /json/) .expect(200); expect(response.body.content).toEqual('This is a test note.'); + + // check if a missing note correctly returns 404 + await request(app.getHttpServer()) + .get('/notes/i_dont_exist') + .expect('Content-Type', /json/) + .expect(404); }); it(`POST /notes/{note}`, async () => { @@ -85,9 +92,13 @@ describe('Notes', () => { it(`DELETE /notes/{note}`, async () => { await notesService.createNote('This is a test note.', 'test3'); await request(app.getHttpServer()).delete('/notes/test3').expect(200); - return expect(notesService.getNoteByIdOrAlias('test3')).rejects.toEqual( + await expect(notesService.getNoteByIdOrAlias('test3')).rejects.toEqual( new NotInDBError("Note with id/alias 'test3' not found."), ); + // check if a missing note correctly returns 404 + await request(app.getHttpServer()) + .delete('/notes/i_dont_exist') + .expect(404); }); it(`PUT /notes/{note}`, async () => { @@ -97,9 +108,16 @@ describe('Notes', () => { .set('Content-Type', 'text/markdown') .send('New note text') .expect(200); - return expect( + await expect( (await notesService.getNoteDtoByIdOrAlias('test4')).content, ).toEqual('New note text'); + + // check if a missing note correctly returns 404 + await request(app.getHttpServer()) + .put('/notes/i_dont_exist') + .set('Content-Type', 'text/markdown') + .expect('Content-Type', /json/) + .expect(404); }); it(`GET /notes/{note}/metadata`, async () => { @@ -124,6 +142,12 @@ describe('Notes', () => { expect(typeof metadata.body.updateUser.photo).toEqual('string'); expect(typeof metadata.body.viewCount).toEqual('number'); expect(metadata.body.editedBy).toEqual([]); + + // check if a missing note correctly returns 404 + await request(app.getHttpServer()) + .get('/notes/i_dont_exist/metadata') + .expect('Content-Type', /json/) + .expect(404); }); it(`GET /notes/{note}/revisions`, async () => { @@ -133,6 +157,12 @@ describe('Notes', () => { .expect('Content-Type', /json/) .expect(200); expect(response.body).toHaveLength(1); + + // check if a missing note correctly returns 404 + await request(app.getHttpServer()) + .get('/notes/i_dont_exist/revisions') + .expect('Content-Type', /json/) + .expect(404); }); it(`GET /notes/{note}/revisions/{revision-id}`, async () => { @@ -143,6 +173,12 @@ describe('Notes', () => { .expect('Content-Type', /json/) .expect(200); expect(response.body.content).toEqual('This is a test note.'); + + // check if a missing note correctly returns 404 + await request(app.getHttpServer()) + .get('/notes/i_dont_exist/revisions/1') + .expect('Content-Type', /json/) + .expect(404); }); it(`GET /notes/{note}/content`, async () => { @@ -151,6 +187,11 @@ describe('Notes', () => { .get('/notes/test9/content') .expect(200); expect(response.text).toEqual('This is a test note.'); + + // check if a missing note correctly returns 404 + await request(app.getHttpServer()) + .get('/notes/i_dont_exist/content') + .expect(404); }); afterAll(async () => {