MediaService: Refactor saveFile

The function now expects a `Note` object instead of a noteId
and a `User` instead of a username to
make it more consistent with other functions.

Signed-off-by: David Mehren <git@herrmehren.de>

src/api/private/media/media.controller.ts

@@ -24,12 +24,18 @@ import { ConsoleLoggerService } from '../../../logger/console-logger.service';
 import { MediaUploadUrlDto } from '../../../media/media-upload-url.dto';
 import { MediaService } from '../../../media/media.service';
 import { MulterFile } from '../../../media/multer-file.interface';
+import { Note } from '../../../notes/note.entity';
+import { NotesService } from '../../../notes/notes.service';
+import { User } from '../../../users/user.entity';
+import { UsersService } from '../../../users/users.service';
 
 @Controller('media')
 export class MediaController {
   constructor(
     private readonly logger: ConsoleLoggerService,
     private mediaService: MediaService,
+    private userService: UsersService,
+    private noteService: NotesService,
   ) {
     this.logger.setContext(MediaController.name);
   }
@@ -42,17 +48,15 @@ export class MediaController {
     @Headers('HedgeDoc-Note') noteId: string,
   ): Promise<MediaUploadUrlDto> {
     // ToDo: Get real userName
-    const username = 'hardcoded';
-    this.logger.debug(
-      `Recieved filename '${file.originalname}' for note '${noteId}' from user '${username}'`,
-      'uploadMedia',
-    );
+    const user: User = await this.userService.getUserByUsername('hardcoded');
     try {
-      const url = await this.mediaService.saveFile(
-        file.buffer,
-        username,
-        noteId,
+      // TODO: Move getting the Note object into a decorator
+      const note: Note = await this.noteService.getNoteByIdOrAlias(noteId);
+      this.logger.debug(
+        `Recieved filename '${file.originalname}' for note '${noteId}' from user '${user.userName}'`,
+        'uploadMedia',
       );
+      const url = await this.mediaService.saveFile(file.buffer, user, note);
       return this.mediaService.toMediaUploadUrlDto(url);
     } catch (e) {
       if (e instanceof ClientError || e instanceof NotInDBError) {

src/api/public/media/media.controller.ts

@@ -42,6 +42,8 @@ import { ConsoleLoggerService } from '../../../logger/console-logger.service';
 import { MediaUploadUrlDto } from '../../../media/media-upload-url.dto';
 import { MediaService } from '../../../media/media.service';
 import { MulterFile } from '../../../media/multer-file.interface';
+import { Note } from '../../../notes/note.entity';
+import { NotesService } from '../../../notes/notes.service';
 import { User } from '../../../users/user.entity';
 import {
   forbiddenDescription,
@@ -58,6 +60,7 @@ export class MediaController {
   constructor(
     private readonly logger: ConsoleLoggerService,
     private mediaService: MediaService,
+    private noteService: NotesService,
   ) {
     this.logger.setContext(MediaController.name);
   }
@@ -93,17 +96,14 @@ export class MediaController {
     @UploadedFile() file: MulterFile,
     @Headers('HedgeDoc-Note') noteId: string,
   ): Promise<MediaUploadUrlDto> {
-    const username = user.userName;
-    this.logger.debug(
-      `Recieved filename '${file.originalname}' for note '${noteId}' from user '${username}'`,
-      'uploadMedia',
-    );
     try {
-      const url = await this.mediaService.saveFile(
-        file.buffer,
-        username,
-        noteId,
+      // TODO: Move getting the Note object into a decorator
+      const note: Note = await this.noteService.getNoteByIdOrAlias(noteId);
+      this.logger.debug(
+        `Recieved filename '${file.originalname}' for note '${noteId}' from user '${user.userName}'`,
+        'uploadMedia',
       );
+      const url = await this.mediaService.saveFile(file.buffer, user, note);
       return this.mediaService.toMediaUploadUrlDto(url);
     } catch (e) {
       if (e instanceof ClientError || e instanceof NotInDBError) {

src/media/media.service.spec.ts

@@ -98,10 +98,12 @@ describe('MediaService', () => {
   });
 
   describe('saveFile', () => {
+    let user: User;
+    let note: Note;
     beforeEach(() => {
-      const user = User.create('hardcoded', 'Testy') as User;
+      user = User.create('hardcoded', 'Testy') as User;
       const alias = 'alias';
-      const note = Note.create(user, alias);
+      note = Note.create(user, alias);
       jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce(user);
       jest.spyOn(noteRepo, 'findOne').mockResolvedValueOnce(note);
     });
@@ -126,22 +128,22 @@ describe('MediaService', () => {
             return [fileName, null];
           },
         );
-      const url = await service.saveFile(testImage, 'hardcoded', 'test');
+      const url = await service.saveFile(testImage, user, note);
       expect(url).toEqual(fileId);
     });
 
     describe('fails:', () => {
       it('MIME type not identifiable', async () => {
         await expect(
-          service.saveFile(Buffer.alloc(1), 'hardcoded', 'test'),
+          service.saveFile(Buffer.alloc(1), user, note),
         ).rejects.toThrow(ClientError);
       });
 
       it('MIME type not supported', async () => {
         const testText = await fs.readFile('test/public-api/fixtures/test.zip');
-        await expect(
-          service.saveFile(testText, 'hardcoded', 'test'),
-        ).rejects.toThrow(ClientError);
+        await expect(service.saveFile(testText, user, note)).rejects.toThrow(
+          ClientError,
+        );
       });
     });
   });

src/media/media.service.ts

@@ -69,24 +69,18 @@ export class MediaService {
    * @async
    * Save the given buffer to the configured MediaBackend and create a MediaUploadEntity to track where the file is, who uploaded it and to which note.
    * @param {Buffer} fileBuffer - the buffer of the file to save.
-   * @param {string} username - the username of the user who uploaded this file
-   * @param {string} noteId - the id or alias of the note which will be associated with the new file.
+   * @param {User} user - the user who uploaded this file
+   * @param {Note} note - the note which will be associated with the new file.
    * @return {string} the url of the saved file
    * @throws {ClientError} the MIME type of the file is not supported.
    * @throws {NotInDBError} - the note or user is not in the database
    * @throws {MediaBackendError} - there was an error saving the file
    */
-  async saveFile(
-    fileBuffer: Buffer,
-    username: string,
-    noteId: string,
-  ): Promise<string> {
+  async saveFile(fileBuffer: Buffer, user: User, note: Note): Promise<string> {
     this.logger.debug(
-      `Saving file for note '${noteId}' and user '${username}'`,
+      `Saving file for note '${note.id}' and user '${user.userName}'`,
       'saveFile',
     );
-    const note = await this.notesService.getNoteByIdOrAlias(noteId);
-    const user = await this.usersService.getUserByUsername(username);
     const fileTypeResult = await FileType.fromBuffer(fileBuffer);
     if (!fileTypeResult) {
       throw new ClientError('Could not detect file type.');

test/private-api/me.e2e-spec.ts

@@ -111,26 +111,10 @@ describe('Me', () => {
     expect(responseBefore.body).toHaveLength(0);
 
     const testImage = await fs.readFile('test/public-api/fixtures/test.png');
-    const url0 = await mediaService.saveFile(
-      testImage,
-      'hardcoded',
-      note1.publicId,
-    );
-    const url1 = await mediaService.saveFile(
-      testImage,
-      'hardcoded',
-      note1.publicId,
-    );
-    const url2 = await mediaService.saveFile(
-      testImage,
-      'hardcoded',
-      note2.alias ?? '',
-    );
-    const url3 = await mediaService.saveFile(
-      testImage,
-      'hardcoded',
-      note2.alias ?? '',
-    );
+    const url0 = await mediaService.saveFile(testImage, user, note1);
+    const url1 = await mediaService.saveFile(testImage, user, note1);
+    const url2 = await mediaService.saveFile(testImage, user, note2);
+    const url3 = await mediaService.saveFile(testImage, user, note2);
 
     const response = await request(httpServer)
       .get('/me/media/')
@@ -163,11 +147,7 @@ describe('Me', () => {
 
   it('DELETE /me', async () => {
     const testImage = await fs.readFile('test/public-api/fixtures/test.png');
-    const url0 = await mediaService.saveFile(
-      testImage,
-      'hardcoded',
-      note1.publicId,
-    );
+    const url0 = await mediaService.saveFile(testImage, user, note1);
     const dbUser = await userService.getUserByUsername('hardcoded');
     expect(dbUser).toBeInstanceOf(User);
     const mediaUploads = await mediaService.listUploadsByUser(dbUser);

test/private-api/notes.e2e-spec.ts

@@ -157,8 +157,8 @@ describe('Notes', () => {
     describe('works', () => {
       it('with an existing alias and keepMedia false', async () => {
         const noteId = 'test3';
-        await notesService.createNote(content, noteId, user);
-        await mediaService.saveFile(testImage, user.userName, noteId);
+        const note = await notesService.createNote(content, noteId, user);
+        await mediaService.saveFile(testImage, user, note);
         await request(app.getHttpServer())
           .delete(`/notes/${noteId}`)
           .set('Content-Type', 'application/json')
@@ -174,12 +174,8 @@ describe('Notes', () => {
       });
       it('with an existing alias and keepMedia true', async () => {
         const noteId = 'test3a';
-        await notesService.createNote(content, noteId, user);
-        const url = await mediaService.saveFile(
-          testImage,
-          user.userName,
-          noteId,
-        );
+        const note = await notesService.createNote(content, noteId, user);
+        const url = await mediaService.saveFile(testImage, user, note);
         await request(app.getHttpServer())
           .delete(`/notes/${noteId}`)
           .set('Content-Type', 'application/json')
@@ -263,8 +259,8 @@ describe('Notes', () => {
     it('works', async () => {
       const alias = 'test6';
       const extraAlias = 'test7';
-      await notesService.createNote(content, alias, user);
-      await notesService.createNote(content, extraAlias, user);
+      const note1 = await notesService.createNote(content, alias, user);
+      const note2 = await notesService.createNote(content, extraAlias, user);
       const httpServer = app.getHttpServer();
       const response = await request(httpServer)
         .get(`/notes/${alias}/media/`)
@@ -273,12 +269,8 @@ describe('Notes', () => {
       expect(response.body).toHaveLength(0);
 
       const testImage = await fs.readFile('test/private-api/fixtures/test.png');
-      const url0 = await mediaService.saveFile(testImage, 'hardcoded', alias);
-      const url1 = await mediaService.saveFile(
-        testImage,
-        'hardcoded',
-        extraAlias,
-      );
+      const url0 = await mediaService.saveFile(testImage, user, note1);
+      const url1 = await mediaService.saveFile(testImage, user, note2);
 
       const responseAfter = await request(httpServer)
         .get(`/notes/${alias}/media/`)

test/public-api/me.e2e-spec.ts

@@ -241,26 +241,10 @@ describe('Me', () => {
     expect(response1.body).toHaveLength(0);
 
     const testImage = await fs.readFile('test/public-api/fixtures/test.png');
-    const url0 = await mediaService.saveFile(
-      testImage,
-      'hardcoded',
-      note1.publicId,
-    );
-    const url1 = await mediaService.saveFile(
-      testImage,
-      'hardcoded',
-      note1.publicId,
-    );
-    const url2 = await mediaService.saveFile(
-      testImage,
-      'hardcoded',
-      note2.publicId,
-    );
-    const url3 = await mediaService.saveFile(
-      testImage,
-      'hardcoded',
-      note2.publicId,
-    );
+    const url0 = await mediaService.saveFile(testImage, user, note1);
+    const url1 = await mediaService.saveFile(testImage, user, note1);
+    const url2 = await mediaService.saveFile(testImage, user, note2);
+    const url3 = await mediaService.saveFile(testImage, user, note2);
 
     const response = await request(httpServer)
       .get('/me/media/')

test/public-api/media.e2e-spec.ts

@@ -22,15 +22,20 @@ import { ConsoleLoggerService } from '../../src/logger/console-logger.service';
 import { LoggerModule } from '../../src/logger/logger.module';
 import { MediaModule } from '../../src/media/media.module';
 import { MediaService } from '../../src/media/media.service';
+import { Note } from '../../src/notes/note.entity';
 import { NotesModule } from '../../src/notes/notes.module';
 import { NotesService } from '../../src/notes/notes.service';
 import { PermissionsModule } from '../../src/permissions/permissions.module';
+import { User } from '../../src/users/user.entity';
+import { UsersService } from '../../src/users/users.service';
 import { ensureDeleted } from '../utils';
 
 describe('Media', () => {
   let app: NestExpressApplication;
   let mediaService: MediaService;
   let uploadPath: string;
+  let testNote: Note;
+  let user: User;
 
   beforeAll(async () => {
     const moduleRef = await Test.createTestingModule({
@@ -69,7 +74,12 @@ describe('Media', () => {
     logger.log('Switching logger', 'AppBootstrap');
     app.useLogger(logger);
     const notesService: NotesService = moduleRef.get(NotesService);
-    await notesService.createNote('test content', 'test_upload_media');
+    const userService = moduleRef.get(UsersService);
+    user = await userService.createUser('hardcoded', 'Testy');
+    testNote = await notesService.createNote(
+      'test content',
+      'test_upload_media',
+    );
     mediaService = moduleRef.get(MediaService);
   });
 
@@ -129,11 +139,7 @@ describe('Media', () => {
 
   it('DELETE /media/{filename}', async () => {
     const testImage = await fs.readFile('test/public-api/fixtures/test.png');
-    const url = await mediaService.saveFile(
-      testImage,
-      'hardcoded',
-      'test_upload_media',
-    );
+    const url = await mediaService.saveFile(testImage, user, testNote);
     const filename = url.split('/').pop() || '';
     await request(app.getHttpServer())
       .delete('/media/' + filename)

test/public-api/notes.e2e-spec.ts

@@ -161,8 +161,8 @@ describe('Notes', () => {
     describe('works', () => {
       it('with an existing alias and keepMedia false', async () => {
         const noteId = 'test3';
-        await notesService.createNote(content, noteId, user);
-        await mediaService.saveFile(testImage, user.userName, noteId);
+        const note = await notesService.createNote(content, noteId, user);
+        await mediaService.saveFile(testImage, user, note);
         await request(app.getHttpServer())
           .delete(`/notes/${noteId}`)
           .set('Content-Type', 'application/json')
@@ -177,12 +177,8 @@ describe('Notes', () => {
       });
       it('with an existing alias and keepMedia true', async () => {
         const noteId = 'test3a';
-        await notesService.createNote(content, noteId, user);
-        const url = await mediaService.saveFile(
-          testImage,
-          user.userName,
-          noteId,
-        );
+        const note = await notesService.createNote(content, noteId, user);
+        const url = await mediaService.saveFile(testImage, user, note);
         await request(app.getHttpServer())
           .delete(`/notes/${noteId}`)
           .set('Content-Type', 'application/json')
@@ -402,8 +398,8 @@ describe('Notes', () => {
     it('works', async () => {
       const alias = 'test9';
       const extraAlias = 'test10';
-      await notesService.createNote(content, alias, user);
-      await notesService.createNote(content, extraAlias, user);
+      const note1 = await notesService.createNote(content, alias, user);
+      const note2 = await notesService.createNote(content, extraAlias, user);
       const httpServer = app.getHttpServer();
       const response = await request(httpServer)
         .get(`/notes/${alias}/media/`)
@@ -412,12 +408,8 @@ describe('Notes', () => {
       expect(response.body).toHaveLength(0);
 
       const testImage = await fs.readFile('test/public-api/fixtures/test.png');
-      const url0 = await mediaService.saveFile(testImage, 'hardcoded', alias);
-      const url1 = await mediaService.saveFile(
-        testImage,
-        'hardcoded',
-        extraAlias,
-      );
+      const url0 = await mediaService.saveFile(testImage, user, note1);
+      const url1 = await mediaService.saveFile(testImage, user, note2);
 
       const responseAfter = await request(httpServer)
         .get(`/notes/${alias}/media/`)