fix: delete file if exist before uploading another one

.gitignore

@@ -140,3 +140,6 @@ yarn.lock
 
 # uploaded files
 uploads/tasks/*
+
+# sonnar locally
+.scannerwork

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<typeof fileHandlerFactory>
+  }
+}
+
+function buildFilePath (fastify: FastifyInstance, fileDir: string, filename: string) {
+  return path.join(
+    import.meta.dirname,
+    '../../../',
+    fastify.config.UPLOAD_DIRNAME,
+    fileDir,
+    filename
+  )
+}
+
+function fileHandlerFactory (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', fileHandlerFactory(fastify))
+}, { name: 'file-handler' })

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<Task>('tasks')
-          .where({ id })
-          .update({ filename })
+        const task = await trx<Task>('tasks').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<Task>('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

test/routes/api/tasks/tasks.test.ts

@@ -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)
@@ -466,15 +466,19 @@ describe('Tasks api (logged user only)', () => {
         status: TaskStatusEnum.New
       })
 
+      await app.knex<Task>('tasks').where({ id: taskId }).update({ filename: null })
+
       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 })
-      })
+      clearDir(uploadDir)
 
       await app.close()
     })
@@ -509,6 +513,31 @@ 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)
+
+        await uploadImageForTask(app, taskId, testImagePath, uploadDirTask)
+
+        const { mock: mockUnlink } = t.mock.method(fs.promises, 'unlink')
+
+        const form = new FormData()
+        form.append('file', fs.createReadStream(testImagePath))
+
+        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)
+      })
+
       it('should return 404 if task not found', async (t) => {
         app = await build(t)
 
@@ -654,34 +683,6 @@ describe('Tasks api (logged user only)', () => {
     })
 
     describe('Deletion', () => {
-      before(async () => {
-        app = await build()
-
-        await app.knex<Task>('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 +738,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 +763,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 })
+  })
+}