From 5c7a787d7e78b483255e36b0fec3abf19b602220 Mon Sep 17 00:00:00 2001 From: David Mehren Date: Sun, 29 Aug 2021 22:28:21 +0200 Subject: [PATCH] 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 --- src/api/private/media/media.controller.ts | 22 ++++++++++------- src/api/public/media/media.controller.ts | 18 +++++++------- src/media/media.service.spec.ts | 16 ++++++------ src/media/media.service.ts | 14 +++-------- test/private-api/me.e2e-spec.ts | 30 ++++------------------- test/private-api/notes.e2e-spec.ts | 24 ++++++------------ test/public-api/me.e2e-spec.ts | 24 +++--------------- test/public-api/media.e2e-spec.ts | 18 +++++++++----- test/public-api/notes.e2e-spec.ts | 24 ++++++------------ 9 files changed, 72 insertions(+), 118 deletions(-) diff --git a/src/api/private/media/media.controller.ts b/src/api/private/media/media.controller.ts index c815cc455..8db4e26f4 100644 --- a/src/api/private/media/media.controller.ts +++ b/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 { // 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) { diff --git a/src/api/public/media/media.controller.ts b/src/api/public/media/media.controller.ts index e9d5545c6..0401be2a5 100644 --- a/src/api/public/media/media.controller.ts +++ b/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 { - 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) { diff --git a/src/media/media.service.spec.ts b/src/media/media.service.spec.ts index 75682436f..4dec3d89a 100644 --- a/src/media/media.service.spec.ts +++ b/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, + ); }); }); }); diff --git a/src/media/media.service.ts b/src/media/media.service.ts index fd8cdbc83..1c523051a 100644 --- a/src/media/media.service.ts +++ b/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 { + async saveFile(fileBuffer: Buffer, user: User, note: Note): Promise { 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.'); diff --git a/test/private-api/me.e2e-spec.ts b/test/private-api/me.e2e-spec.ts index 3c7a2ca7e..d0d35499e 100644 --- a/test/private-api/me.e2e-spec.ts +++ b/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); diff --git a/test/private-api/notes.e2e-spec.ts b/test/private-api/notes.e2e-spec.ts index 65ced873e..d04347a56 100644 --- a/test/private-api/notes.e2e-spec.ts +++ b/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/`) diff --git a/test/public-api/me.e2e-spec.ts b/test/public-api/me.e2e-spec.ts index 341ae6b16..18b3a1a28 100644 --- a/test/public-api/me.e2e-spec.ts +++ b/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/') diff --git a/test/public-api/media.e2e-spec.ts b/test/public-api/media.e2e-spec.ts index 04a984f33..ec21f6ead 100644 --- a/test/public-api/media.e2e-spec.ts +++ b/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) diff --git a/test/public-api/notes.e2e-spec.ts b/test/public-api/notes.e2e-spec.ts index 0fb44fab5..f72b0d258 100644 --- a/test/public-api/notes.e2e-spec.ts +++ b/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/`)