Feature/increase test coverage (#204)

* Remove unreachable code

* Test invalid handler and callback for legacy multipart

* Make id generation for saveRequestFiles injectable

By injecting toID as a parameter we make it possible to test errors
caused by file access errors.

* Use unlink instead of rm to support Node 10 and 12

* Fix test scenario message

- use node-tap's testdir as a replacement for os.tmpdir
- inject tmpdir in saveRequestFiles
- do not inject toID in saveRequestFiles

* Skip cleanup tests on Windows

index.d.ts

@@ -66,7 +66,7 @@ declare module "fastify" {
         files: (options?: busboy.BusboyConfig) => AsyncIterableIterator<Multipart>
 
         // Disk mode
-        saveRequestFiles: (options?: busboy.BusboyConfig) => Promise<Array<Multipart>>
+        saveRequestFiles: (options?: busboy.BusboyConfig & { tmpdir: string }) => Promise<Array<Multipart>>
         cleanRequestFiles: () => Promise<void>
         tmpUploads: Array<Multipart>
     }

index.js

@@ -149,11 +149,6 @@ function fastifyMultipart (fastify, options, done) {
     })
   }
 
-  let throwFileSizeLimit = true
-  if (typeof options.throwFileSizeLimit === 'boolean') {
-    throwFileSizeLimit = options.throwFileSizeLimit
-  }
-
   const PartsLimitError = createError('FST_PARTS_LIMIT', 'reach parts limit', 413)
   const FilesLimitError = createError('FST_FILES_LIMIT', 'reach files limit', 413)
   const FieldsLimitError = createError('FST_FIELDS_LIMIT', 'reach fields limit', 413)
@@ -402,6 +397,7 @@ function fastifyMultipart (fastify, options, done) {
         return
       }
 
+      let throwFileSizeLimit = true
       if (typeof opts.throwFileSizeLimit === 'boolean') {
         throwFileSizeLimit = opts.throwFileSizeLimit
       }
@@ -466,11 +462,12 @@ function fastifyMultipart (fastify, options, done) {
 
   async function saveRequestFiles (options) {
     const requestFiles = []
+    const tmpdir = (options && options.tmpdir) || os.tmpdir()
 
     const files = await this.files(options)
     this.tmpUploads = []
     for await (const file of files) {
-      const filepath = path.join(os.tmpdir(), toID() + path.extname(file.filename))
+      const filepath = path.join(tmpdir, toID() + path.extname(file.filename))
       const target = createWriteStream(filepath)
       try {
         await pump(file.file, target)

test/legacy/multipart.test.js

@@ -181,6 +181,88 @@ test('should error if it is not multipart', { skip: process.platform === 'win32'
   })
 })
 
+test('should error if handler is not a function', { skip: process.platform === 'win32' }, function (t) {
+  t.plan(3)
+
+  const fastify = Fastify()
+
+  t.tearDown(fastify.close.bind(fastify))
+  fastify.register(multipart)
+
+  fastify.post('/', function (req, reply) {
+    const handler = null
+
+    req.multipart(handler, function (err) {
+      t.ok(err)
+      reply.code(500).send()
+    })
+  })
+
+  fastify.listen(0, function () {
+    // request
+    const form = new FormData()
+    const opts = {
+      protocol: 'http:',
+      hostname: 'localhost',
+      port: fastify.server.address().port,
+      path: '/',
+      headers: form.getHeaders(),
+      method: 'POST'
+    }
+
+    const req = http.request(opts, (res) => {
+      res.resume()
+      res.on('end', () => {
+        t.equal(res.statusCode, 500)
+        t.pass('res ended successfully')
+      })
+    })
+    pump(form, req, function (err) {
+      t.error(err, 'client pump: no err')
+    })
+  })
+})
+
+test('should error if callback is not a function', { skip: process.platform === 'win32' }, function (t) {
+  t.plan(3)
+
+  const fastify = Fastify()
+
+  t.tearDown(fastify.close.bind(fastify))
+  fastify.register(multipart)
+
+  fastify.post('/', function (req) {
+    const callback = null
+    req.multipart(handler, callback)
+
+    function handler () {}
+  })
+
+  fastify.listen(0, function () {
+    // request
+    const form = new FormData()
+    const opts = {
+      protocol: 'http:',
+      hostname: 'localhost',
+      port: fastify.server.address().port,
+      path: '/',
+      headers: form.getHeaders(),
+      method: 'POST'
+    }
+
+    const req = http.request(opts, (res) => {
+      res.resume()
+      res.on('end', () => {
+        t.equal(res.statusCode, 500)
+        t.pass('res ended successfully')
+      })
+    })
+    pump(form, req, function (err) {
+      t.error(err, 'client pump: no err')
+    })
+  })
+})
+
 test('should error if it is invalid multipart', { skip: process.platform === 'win32' }, function (t) {
   t.plan(5)
 

test/multipart-disk.test.js

@@ -11,6 +11,7 @@ const { Readable } = require('readable-stream')
 const path = require('path')
 const fs = require('fs')
 const { access } = require('fs').promises
+const rimraf = require('rimraf')
 const stream = require('stream')
 const EventEmitter = require('events')
 const { once } = EventEmitter
@@ -184,6 +185,104 @@ test('should throw on file limit error', async function (t) {
   }
 })
 
+test('should throw on file save error', async function (t) {
+  t.plan(2)
+
+  const fastify = Fastify()
+  t.tearDown(fastify.close.bind(fastify))
+
+  fastify.register(require('..'))
+
+  fastify.post('/', async function (req, reply) {
+    t.ok(req.isMultipart())
+
+    try {
+      await req.saveRequestFiles({ tmpdir: 'something' })
+      reply.code(200).send()
+    } catch (error) {
+      reply.code(500).send()
+    }
+  })
+
+  await fastify.listen(0)
+
+  // request
+  const form = new FormData()
+  const opts = {
+    protocol: 'http:',
+    hostname: 'localhost',
+    port: fastify.server.address().port,
+    path: '/',
+    headers: form.getHeaders(),
+    method: 'POST'
+  }
+  const req = http.request(opts)
+  const readStream = fs.createReadStream(filePath)
+  form.append('upload', readStream)
+
+  pump(form, req)
+
+  try {
+    const [res] = await once(req, 'response')
+    t.equal(res.statusCode, 500)
+    res.resume()
+    await once(res, 'end')
+  } catch (error) {
+    t.error(error, 'request')
+  }
+})
+
+test('should not throw on request files cleanup error', { skip: process.platform === 'win32' }, async function (t) {
+  t.plan(2)
+
+  const fastify = Fastify()
+  t.tearDown(fastify.close.bind(fastify))
+
+  fastify.register(require('..'))
+
+  const tmpdir = t.testdir()
+
+  fastify.post('/', async function (req, reply) {
+    t.ok(req.isMultipart())
+
+    try {
+      await req.saveRequestFiles({ tmpdir })
+      // temp file saved, remove before the onResponse hook
+      rimraf.sync(tmpdir)
+      reply.code(200).send()
+    } catch (error) {
+      reply.code(500).send()
+    }
+  })
+
+  await fastify.listen(0)
+
+  // request
+  const form = new FormData()
+  const opts = {
+    protocol: 'http:',
+    hostname: 'localhost',
+    port: fastify.server.address().port,
+    path: '/',
+    headers: form.getHeaders(),
+    method: 'POST'
+  }
+  const req = http.request(opts)
+  const readStream = fs.createReadStream(filePath)
+  form.append('upload', readStream)
+
+  pump(form, req)
+
+  try {
+    const [res] = await once(req, 'response')
+    t.equal(res.statusCode, 200)
+    res.resume()
+    await once(res, 'end')
+  } catch (error) {
+    t.error(error, 'request')
+  }
+})
+
 test('should throw on file limit error, after highWaterMark', async function (t) {
   t.plan(5)