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 <git@herrmehren.de>

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) {
+  async getNote(@Param('noteIdOrAlias') noteIdOrAlias: string) {
-    return this.noteService.getNoteDtoByIdOrAlias(noteIdOrAlias);
+    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);
+    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) {
+  async getNoteContent(@Param('noteIdOrAlias') noteIdOrAlias: string) {
-    return this.noteService.getNoteContent(noteIdOrAlias);
+    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) {
+  async getNoteMetadata(@Param('noteIdOrAlias') noteIdOrAlias: string) {
-    return this.noteService.getNoteMetadata(noteIdOrAlias);
+    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) {
+  async getNoteRevisions(@Param('noteIdOrAlias') noteIdOrAlias: string) {
-    return this.revisionsService.getNoteRevisionMetadatas(noteIdOrAlias);
+    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;
+    }
   }
 }

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