Merge pull request #162 from jaydenseric/coverage-to-100

Enforce 100% code coverage and improve processRequest internals and tests (including a new test using vanilla Node.js HTTP), fixing #130 .

changelog.md

@@ -17,12 +17,12 @@
 - Use GitHub Actions instead of Travis for CI.
 - Removed `package-lock.json` from `.gitignore` and `.prettierignore`, as it’s disabled in `.npmrc` anyway.
 - New file structure.
-- Enabled code coverage for tests.
 - Explicitly defined main exports (instead of using `export * from`) to prevent accidental public exposure of internal APIs.
 - Moved JSDoc typedefs into the index main entry file, alphabetically sorted.
 - Nicer Browserslist query syntax.
 - Replaced the `isObject` helper with a smarter and tested `isEnumerableObject`.
 - Removed the `isString` helper.
+- Enforced 100% code coverage for tests, and improved `processRequest` internals and tests (including a new test using vanilla Node.js HTTP), fixing [#130](https://github.com/jaydenseric/graphql-upload/issues/130) via [#162](https://github.com/jaydenseric/graphql-upload/pull/162).
 - Removed a workaround from the `startServer` test helper.
 - Added a new `ProcessRequestFunction` JSDoc type, and applied it to `processRequest`.
 - Renamed the `UploadOptions` JSDoc type to `ProcessRequestOptions`.

package.json

@@ -77,7 +77,7 @@
     "test": "npm run test:eslint && npm run test:prettier && npm run test:tap",
     "test:eslint": "eslint . --ext mjs,js",
     "test:prettier": "prettier '**/*.{json,yml,md}' -l",
-    "test:tap": "tap --test-ignore=src",
+    "test:tap": "tap --test-ignore=src --100",
     "prepublishOnly": "npm test"
   }
 }

src/processRequest.mjs

@@ -76,8 +76,7 @@ export const processRequest = (
   } = {}
 ) =>
   new Promise((resolve, reject) => {
-    let requestEnded = false
-    let released = false
+    let released
     let exitError
     let currentStream
     let operations
@@ -134,6 +133,7 @@ export const processRequest = (
      * @ignore
      */
     const release = () => {
+      // istanbul ignore next
       if (released) return
       released = true
 
@@ -142,6 +142,21 @@ export const processRequest = (
           if (upload.file) upload.file.capacitor.destroy()
     }
 
+    /**
+     * Handles when the request is closed before it properly ended.
+     * @kind function
+     * @name processRequest~abort
+     * @ignore
+     */
+    const abort = () => {
+      exit(
+        createError(
+          499,
+          'Request disconnected during file upload stream parsing.'
+        )
+      )
+    }
+
     parser.on(
       'field',
       (fieldName, value, fieldNameTruncated, valueTruncated) => {
@@ -275,7 +290,7 @@ export const processRequest = (
 
       currentStream = stream
       stream.on('end', () => {
-        if (currentStream === stream) currentStream = null
+        currentStream = null
       })
 
       const upload = map.get(fieldName)
@@ -295,7 +310,6 @@ export const processRequest = (
       })
 
       stream.on('limit', () => {
-        if (currentStream === stream) currentStream = null
         stream.unpipe()
         capacitor.destroy(
           createError(
@@ -306,8 +320,8 @@ export const processRequest = (
       })
 
       stream.on('error', error => {
-        if (currentStream === stream) currentStream = null
         stream.unpipe()
+        // istanbul ignore next
         capacitor.destroy(exitError || error)
       })
 
@@ -368,18 +382,9 @@ export const processRequest = (
     response.once('finish', release)
     response.once('close', release)
 
+    request.once('close', abort)
     request.once('end', () => {
-      requestEnded = true
-    })
-
-    request.once('close', () => {
-      if (!requestEnded)
-        exit(
-          createError(
-            499,
-            'Request disconnected during file upload stream parsing.'
-          )
-        )
+      request.removeListener('close', abort)
     })
 
     request.pipe(parser)

src/processRequest.test.mjs

@@ -10,6 +10,7 @@ import fetch from 'node-fetch'
 import t from 'tap'
 import { graphqlUploadExpress } from './graphqlUploadExpress'
 import { graphqlUploadKoa } from './graphqlUploadKoa'
+import { processRequest } from './processRequest'
 import { snapshotError } from './test-helpers/snapshotError'
 import { startServer } from './test-helpers/startServer'
 import { streamToString } from './test-helpers/streamToString'
@@ -34,6 +35,48 @@ t.test('Single file.', async t => {
     t.equals(await streamToString(stream), 'a', 'Contents.')
   }
 
+  await t.test('Node.js HTTP.', async t => {
+    t.plan(2)
+
+    let variables
+
+    const app = http.createServer(async (request, response) => {
+      const send = data => {
+        if (request.complete) response.end(data)
+        else {
+          request.on('end', () => response.end(data))
+          request.resume()
+        }
+      }
+
+      const contentType = request.headers['content-type']
+      if (!contentType || contentType.substr(0, 19) !== 'multipart/form-data') {
+        response.statusCode = 415
+        return send()
+      }
+
+      try {
+        const body = await processRequest(request, response)
+        ;({ variables } = body)
+        await t.test('Upload.', uploadTest(body.variables.file))
+        send()
+      } catch (error) {
+        console.error(error)
+        response.statusCode = 500
+        send()
+      }
+    })
+
+    const port = await startServer(t, app)
+
+    await sendRequest(port)
+
+    const file = await variables.file
+    if (!file.capacitor.closed)
+      await new Promise(resolve => file.capacitor.once('close', resolve))
+    t.false(fs.existsSync(file.capacitor.path), 'Cleanup.')
+  })
+
   await t.test('Koa middleware.', async t => {
     t.plan(2)
 
@@ -720,7 +763,7 @@ t.test('Aborted request.', async t => {
     }
 
     await t.test('Koa middleware.', async t => {
-      t.plan(5)
+      t.plan(6)
 
       let requestHasBeenReceived
       const requestHasBeenReceivedPromise = new Promise(
@@ -732,6 +775,9 @@ t.test('Aborted request.', async t => {
 
       const finished = new Promise(resolve => (finish = resolve))
       const app = new Koa()
+        .on('error', error =>
+          t.matchSnapshot(snapshotError(error), 'Middleware throws.')
+        )
         .use(async (ctx, next) => {
           requestHasBeenReceived()
           await next()
@@ -851,7 +897,7 @@ t.test('Aborted request.', async t => {
     }
 
     await t.test('Koa middleware.', async t => {
-      t.plan(5)
+      t.plan(6)
 
       let requestHasBeenReceived
       const requestHasBeenReceivedPromise = new Promise(
@@ -863,6 +909,9 @@ t.test('Aborted request.', async t => {
 
       const finished = new Promise(resolve => (finish = resolve))
       const app = new Koa()
+        .on('error', error =>
+          t.matchSnapshot(snapshotError(error), 'Middleware throws.')
+        )
         .use(async (ctx, next) => {
           requestHasBeenReceived()
           await next()
@@ -894,6 +943,7 @@ t.test('Aborted request.', async t => {
         })
 
       const port = await startServer(t, app)
+
       await sendRequest(port, requestHasBeenReceivedPromise)
       await finished
 

tap-snapshots/lib-processRequest.test.js-TAP.test.js

@@ -35,6 +35,13 @@ exports[`lib/processRequest.test.js TAP Aborted request. Delayed stream creation
 }
 `
 
+exports[`lib/processRequest.test.js TAP Aborted request. Delayed stream creation. Koa middleware. > Middleware throws. 1`] = `
+{
+  "name": "Error",
+  "message": "Parse Error"
+}
+`
+
 exports[`lib/processRequest.test.js TAP Aborted request. Delayed stream creation. Koa middleware. Upload A. > Stream error. 1`] = `
 {
   "name": "BadRequestError",
@@ -93,6 +100,13 @@ exports[`lib/processRequest.test.js TAP Aborted request. Immediate stream creati
 }
 `
 
+exports[`lib/processRequest.test.js TAP Aborted request. Immediate stream creation. Koa middleware. > Middleware throws. 1`] = `
+{
+  "name": "Error",
+  "message": "Parse Error"
+}
+`
+
 exports[`lib/processRequest.test.js TAP Aborted request. Immediate stream creation. Koa middleware. Upload A. > Enumerable properties. 1`] = `
 {
   "filename": "a.txt",
@@ -614,3 +628,11 @@ exports[`lib/processRequest.test.js TAP Single file. Koa middleware. Upload. > E
   "encoding": "7bit"
 }
 `
+
+exports[`lib/processRequest.test.js TAP Single file. Node.js HTTP. Upload. > Enumerable properties. 1`] = `
+{
+  "filename": "a.txt",
+  "mimetype": "text/plain",
+  "encoding": "7bit"
+}
+`

tap-snapshots/lib-processRequest.test.mjs-TAP.test.js

@@ -35,6 +35,13 @@ exports[`lib/processRequest.test.mjs TAP Aborted request. Delayed stream creatio
 }
 `
 
+exports[`lib/processRequest.test.mjs TAP Aborted request. Delayed stream creation. Koa middleware. > Middleware throws. 1`] = `
+{
+  "name": "Error",
+  "message": "Parse Error"
+}
+`
+
 exports[`lib/processRequest.test.mjs TAP Aborted request. Delayed stream creation. Koa middleware. Upload A. > Stream error. 1`] = `
 {
   "name": "BadRequestError",
@@ -93,6 +100,13 @@ exports[`lib/processRequest.test.mjs TAP Aborted request. Immediate stream creat
 }
 `
 
+exports[`lib/processRequest.test.mjs TAP Aborted request. Immediate stream creation. Koa middleware. > Middleware throws. 1`] = `
+{
+  "name": "Error",
+  "message": "Parse Error"
+}
+`
+
 exports[`lib/processRequest.test.mjs TAP Aborted request. Immediate stream creation. Koa middleware. Upload A. > Enumerable properties. 1`] = `
 {
   "filename": "a.txt",
@@ -614,3 +628,11 @@ exports[`lib/processRequest.test.mjs TAP Single file. Koa middleware. Upload. >
   "encoding": "7bit"
 }
 `
+
+exports[`lib/processRequest.test.mjs TAP Single file. Node.js HTTP. Upload. > Enumerable properties. 1`] = `
+{
+  "filename": "a.txt",
+  "mimetype": "text/plain",
+  "encoding": "7bit"
+}
+`