From fe26f1689c6779a169cabd2b68afed53111144d7 Mon Sep 17 00:00:00 2001
From: David Mehren <git@herrmehren.de>
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 <git@herrmehren.de>
---
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<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) {
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<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) {
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<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.');
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/`)