diff --git a/.gitignore b/.gitignore index 2b923efe..15bc41df 100644 --- a/.gitignore +++ b/.gitignore @@ -140,3 +140,6 @@ yarn.lock # uploaded files uploads/tasks/* + +# sonnar locally +.scannerwork diff --git a/src/plugins/custom/file-handling.ts b/src/plugins/custom/file-handling.ts new file mode 100644 index 00000000..84a5f3ae --- /dev/null +++ b/src/plugins/custom/file-handling.ts @@ -0,0 +1,59 @@ +import fp from 'fastify-plugin' +import { BusboyFileStream } from '@fastify/busboy' +import { FastifyInstance } from 'fastify' +import path from 'path' +import { pipeline } from 'node:stream/promises' +import fs from 'node:fs' + +declare module 'fastify' { + export interface FastifyInstance { + fileHandler: ReturnType + } +} + +function buildFilePath (fastify: FastifyInstance, fileDir: string, filename: string) { + return path.join( + import.meta.dirname, + '../../../', + fastify.config.UPLOAD_DIRNAME, + fileDir, + filename + ) +} + +function createFileHandler (fastify: FastifyInstance) { + async function upload (fileDir: string, fileName: string, file: BusboyFileStream) { + const filePath = buildFilePath(fastify, fileDir, fileName) + + await pipeline(file, fs.createWriteStream(filePath)) + } + + async function remove (fileDir: string, fileName: string) { + const filePath = buildFilePath(fastify, fileDir, fileName) + + try { + await fs.promises.unlink(filePath) + } catch (err) { + if (isErrnoException(err) && err.code === 'ENOENT') { + // A file could have been deleted by an external actor, e.g. system administrator. + // We log the error to keep a record of the failure, but consider that the operation was successful. + fastify.log.warn(`File path '${fileName}' not found`) + } else { + throw err + } + } + } + + return { + upload, + remove + } +} + +function isErrnoException (error: unknown): error is NodeJS.ErrnoException { + return error instanceof Error && 'code' in error +} + +export default fp(async (fastify) => { + fastify.decorate('fileHandler', createFileHandler(fastify)) +}, { name: 'file-handler' }) diff --git a/src/routes/api/tasks/index.ts b/src/routes/api/tasks/index.ts index 4d7812cf..9c90ed12 100644 --- a/src/routes/api/tasks/index.ts +++ b/src/routes/api/tasks/index.ts @@ -12,8 +12,6 @@ import { TaskPaginationResultSchema } from '../../../schemas/tasks.js' import path from 'node:path' -import { pipeline } from 'node:stream/promises' -import fs from 'node:fs' const plugin: FastifyPluginAsyncTypebox = async (fastify) => { fastify.get( @@ -246,24 +244,21 @@ const plugin: FastifyPluginAsyncTypebox = async (fastify) => { const filename = `${id}_${file.filename}` - const affectedRows = await trx('tasks') - .where({ id }) - .update({ filename }) + const task = await trx('tasks').select('filename').where({ id }).first() - if (affectedRows === 0) { + if (!task) { return reply.notFound('Task not found') } - const filePath = path.join( - import.meta.dirname, - '../../../..', - fastify.config.UPLOAD_DIRNAME, - fastify.config.UPLOAD_TASKS_DIRNAME, - filename - ) + await trx('tasks') + .where({ id }) + .update({ filename }) - await pipeline(file.file, fs.createWriteStream(filePath)) + if (task.filename) { + await fastify.fileHandler.remove(fastify.config.UPLOAD_TASKS_DIRNAME, task.filename) + } + await fastify.fileHandler.upload(fastify.config.UPLOAD_TASKS_DIRNAME, filename, file.file) return { message: 'File uploaded successfully' } }).catch(() => { reply.internalServerError('Transaction failed.') @@ -326,25 +321,7 @@ const plugin: FastifyPluginAsyncTypebox = async (fastify) => { return reply.notFound(`No task has filename "${filename}"`) } - const filePath = path.join( - import.meta.dirname, - '../../../..', - fastify.config.UPLOAD_DIRNAME, - fastify.config.UPLOAD_TASKS_DIRNAME, - filename - ) - - try { - await fs.promises.unlink(filePath) - } catch (err) { - if (isErrnoException(err) && err.code === 'ENOENT') { - // A file could have been deleted by an external actor, e.g. system administrator. - // We log the error to keep a record of the failure, but consider that the operation was successful. - fastify.log.warn(`File path '${filename}' not found`) - } else { - throw err - } - } + await fastify.fileHandler.remove(fastify.config.UPLOAD_TASKS_DIRNAME, filename) reply.code(204) @@ -356,8 +333,4 @@ const plugin: FastifyPluginAsyncTypebox = async (fastify) => { ) } -function isErrnoException (error: unknown): error is NodeJS.ErrnoException { - return error instanceof Error && 'code' in error -} - export default plugin diff --git a/test/routes/api/tasks/fixtures/short-logo-copy.png b/test/routes/api/tasks/fixtures/short-logo-copy.png new file mode 100644 index 00000000..8041e33d Binary files /dev/null and b/test/routes/api/tasks/fixtures/short-logo-copy.png differ diff --git a/test/routes/api/tasks/tasks.test.ts b/test/routes/api/tasks/tasks.test.ts index a22d4777..5ff1684e 100644 --- a/test/routes/api/tasks/tasks.test.ts +++ b/test/routes/api/tasks/tasks.test.ts @@ -1,4 +1,4 @@ -import { after, before, beforeEach, describe, it } from 'node:test' +import { after, afterEach, before, beforeEach, describe, it } from 'node:test' import assert from 'node:assert' import { build } from '../../../helper.js' import { @@ -39,7 +39,7 @@ async function uploadImageForTask (app: FastifyInstance, taskId: number, filePat await pipeline(file, writeStream) } -describe('Tasks api (logged user only)', () => { +describe('Tasks api (logged user)', () => { describe('GET /api/tasks', () => { let app: FastifyInstance let userId1: number @@ -348,7 +348,7 @@ describe('Tasks api (logged user only)', () => { const res = await app.injectWithLogin('admin', { method: 'DELETE', - url: '/api/tasks/9999' + url: '/api/tasks/1234' }) assert.strictEqual(res.statusCode, 404) @@ -460,26 +460,34 @@ describe('Tasks api (logged user only)', () => { uploadDirTask = path.join(uploadDir, app.config.UPLOAD_TASKS_DIRNAME) assert.ok(fs.existsSync(uploadDir)) + clearDir(uploadDirTask) + + app.close() + }) + + beforeEach(async (t) => { + app = await build() taskId = await createTask(app, { name: 'Task with image', author_id: 1, status: TaskStatusEnum.New }) + await uploadImageForTask(app, taskId, testImagePath, uploadDirTask) + await app.close() + }) - app.close() + afterEach(async () => { + app = await build() + await app.knex('tasks').where({ id: taskId }).update({ filename: null }) + clearDir(uploadDirTask) + await app.close() }) after(async () => { - const files = fs.readdirSync(uploadDirTask) - files.forEach((file) => { - const filePath = path.join(uploadDirTask, file) - fs.rmSync(filePath, { recursive: true }) - }) - await app.close() }) - describe('Upload', () => { + describe('Upload', (t) => { it('should create upload directories at boot if not exist', async (t) => { fs.rmSync(uploadDir, { recursive: true }) assert.ok(!fs.existsSync(uploadDir)) @@ -509,6 +517,37 @@ describe('Tasks api (logged user only)', () => { assert.strictEqual(message, 'File uploaded successfully') }) + it('should delete existing image for a task before uploading a new one', async (t) => { + app = await build(t) + + const { mock: mockUnlink } = t.mock.method(fs.promises, 'unlink') + + const newTestImageFileName = 'short-logo-copy.png' + + const newTestImagePath = path.join(fixturesDir, newTestImageFileName) + + const form = new FormData() + form.append('file', fs.createReadStream(newTestImagePath)) + + const res = await app.injectWithLogin('basic', { + method: 'POST', + url: `/api/tasks/${taskId}/upload`, + payload: form, + headers: form.getHeaders() + }) + + assert.strictEqual(res.statusCode, 200) + + const { message } = JSON.parse(res.payload) + assert.strictEqual(message, 'File uploaded successfully') + + assert.strictEqual(mockUnlink.callCount(), 1) + + const files = fs.readdirSync(uploadDirTask) + assert.strictEqual(files.length, 1) + assert.strictEqual(files[0], `${taskId}_${newTestImageFileName}`) + }) + it('should return 404 if task not found', async (t) => { app = await build(t) @@ -654,34 +693,6 @@ describe('Tasks api (logged user only)', () => { }) describe('Deletion', () => { - before(async () => { - app = await build() - - await app.knex('tasks').where({ id: taskId }).update({ filename: null }) - - const files = fs.readdirSync(uploadDirTask) - files.forEach((file) => { - const filePath = path.join(uploadDirTask, file) - fs.rmSync(filePath, { recursive: true }) - }) - - await app.close() - }) - - beforeEach(async () => { - app = await build() - await uploadImageForTask(app, taskId, testImagePath, uploadDirTask) - await app.close() - }) - - after(async () => { - const files = fs.readdirSync(uploadDirTask) - files.forEach((file) => { - const filePath = path.join(uploadDirTask, file) - fs.rmSync(filePath, { recursive: true }) - }) - }) - it('should remove an existing image for a task', async (t) => { app = await build(t) @@ -737,8 +748,8 @@ describe('Tasks api (logged user only)', () => { it('File deletion transaction should rollback on error', async (t) => { const app = await build(t) - const { mock: mockPipeline } = t.mock.method(fs.promises, 'unlink') - mockPipeline.mockImplementationOnce(() => { + const { mock: mockUnlink } = t.mock.method(fs.promises, 'unlink') + mockUnlink.mockImplementationOnce(() => { return Promise.reject(new Error()) }) @@ -762,3 +773,11 @@ describe('Tasks api (logged user only)', () => { }) }) }) + +function clearDir (dirPath: string) { + const files = fs.readdirSync(dirPath) + files.forEach((file) => { + const filePath = path.join(dirPath, file) + fs.rmSync(filePath, { recursive: true }) + }) +}