From 7df70449b1b0d61827d9d7029086cff168d90f66 Mon Sep 17 00:00:00 2001 From: Jayden Seric Date: Fri, 17 Aug 2018 14:39:43 +1000 Subject: [PATCH 1/3] Added back, but deprecated, the file upload stream property. --- changelog.md | 3 +- src/processRequest.mjs | 43 +++++++------ src/test.mjs | 99 ++++++++++++++++++++++++++++++ tap-snapshots/lib-test-TAP.test.js | 30 +++++++++ 4 files changed, 154 insertions(+), 21 deletions(-) diff --git a/changelog.md b/changelog.md index 9dd07dd..bcc3e21 100644 --- a/changelog.md +++ b/changelog.md @@ -5,12 +5,13 @@ ### Major - The `processRequest` function now requires a [`http.ServerResponse`](https://nodejs.org/api/http.html#http_class_http_serverresponse) instance as its second argument. -- `Upload` scalar promises now resolve with a `createReadStream` method instead of a `stream` property, via [#92](https://github.com/jaydenseric/apollo-upload-server/pull/92). - Replaced the previously exported error classes with [`http-errors`](https://npm.im/http-errors) and snapshot tested error details, via [#105](https://github.com/jaydenseric/apollo-upload-server/pull/105). - No longer exporting the `SPEC_URL` constant. ### Minor +- `Upload` scalar promises now resolve with a `createReadStream` method instead of a `stream` property, via [#92](https://github.com/jaydenseric/apollo-upload-server/pull/92). +- Accessing an `Upload` scalar promise resolved `stream` property results in a deprecation warning that recommends using `createReadStream` instead. It will be removed in a future release. - An `Upload` variable can now be used by multiple resolvers, via [#92](https://github.com/jaydenseric/apollo-upload-server/pull/92). - Multiple `Upload` scalar variables can now use the same multipart data, via [#92](https://github.com/jaydenseric/apollo-upload-server/pull/92). - Malformed requests containing invalid JSON for `operations` or `map` multipart fields cause an appropriate error with a `400` status instead of crashing the process, relating to [#81](https://github.com/jaydenseric/apollo-upload-server/pull/81) and [#95](https://github.com/jaydenseric/apollo-upload-server/issues/95). diff --git a/src/processRequest.mjs b/src/processRequest.mjs index e9d0f5e..3ca7b7a 100644 --- a/src/processRequest.mjs +++ b/src/processRequest.mjs @@ -1,3 +1,4 @@ +import util from 'util' import Busboy from 'busboy' import objectPath from 'object-path' import WriteStream from 'fs-capacitor' @@ -267,26 +268,28 @@ export const processRequest = ( stream.pipe(capacitor) - upload.resolve( - Object.create( - {}, - { - filename: { value: filename, enumerable: true }, - mimetype: { value: mimetype, enumerable: true }, - encoding: { value: encoding, enumerable: true }, - createReadStream: { - value() { - const error = capacitor.error || (released ? exitError : null) - if (error) throw error - - return capacitor.createReadStream() - }, - enumerable: true - }, - capacitor: { value: capacitor } - } - ) - ) + const file = { + filename, + mimetype, + encoding, + createReadStream() { + const error = capacitor.error || (released ? exitError : null) + if (error) throw error + return capacitor.createReadStream() + } + } + + let capacitorStream + Object.defineProperty(file, 'stream', { + get: util.deprecate(function() { + if (!capacitorStream) capacitorStream = this.createReadStream() + return capacitorStream + }, 'File upload property ‘stream’ is deprecated. Use ‘createReadStream()’ instead.') + }) + + Object.defineProperty(file, 'capacitor', { value: capacitor }) + + upload.resolve(file) } else { // Discard the unexpected file. stream.on('error', () => {}) diff --git a/src/test.mjs b/src/test.mjs index fd21043..1fccb58 100644 --- a/src/test.mjs +++ b/src/test.mjs @@ -1438,3 +1438,102 @@ t.test('Missing ‘operations’, ‘map’ and files.', async t => { await sendRequest(t, port) }) }) + +t.test('Deprecated file upload ‘stream’ property.', async t => { + const sendRequest = async port => { + const body = new FormData() + + body.append( + 'operations', + JSON.stringify({ + variables: { + file: null + } + }) + ) + + body.append('map', JSON.stringify({ 1: ['variables.file'] })) + body.append('1', 'a', { filename: 'a.txt' }) + + await fetch(`http://localhost:${port}`, { method: 'POST', body }) + } + + const uploadTest = upload => async t => { + // Store the original process deprecation modes. + const { noDeprecation, throwDeprecation } = process + const resolved = await upload + + // Allow deprecation warning to be tested. + process.throwDeprecation = true + + try { + resolved.stream + t.fail('No deprecation warning.') + } catch (error) { + t.matchSnapshot(snapshotError(error), 'Deprecation warning.') + } + + // Restore process deprecation mode. + process.throwDeprecation = throwDeprecation + + t.matchSnapshot(JSON.stringify(resolved, null, 2), 'Enumerable properties.') + + // Silence deprecation warnings. + process.noDeprecation = true + + t.true( + resolved.stream === resolved.stream, + 'Accessing ‘stream’ multiple times gets the same stream.' + ) + t.type(resolved.stream, ReadStream, 'Stream type.') + t.equals(await streamToString(resolved.stream), 'a', 'Contents.') + + // Restore process deprecation mode. + process.noDeprecation = noDeprecation + } + + await t.test('Koa middleware.', async t => { + t.plan(2) + + let variables + + const app = new Koa().use(apolloUploadKoa()).use(async (ctx, next) => { + ;({ variables } = ctx.request.body) + await t.test('Upload.', uploadTest(ctx.request.body.variables.file)) + + ctx.status = 204 + await next() + }) + + const port = await startServer(t, app) + + await sendRequest(port) + + const file = await variables.file + await new Promise(resolve => file.capacitor.once('close', resolve)) + t.false(fs.existsSync(file.capacitor.path), 'Cleanup.') + }) + + await t.test('Express middleware.', async t => { + t.plan(2) + + let variables + + const app = express() + .use(apolloUploadExpress()) + .use((request, response, next) => { + ;({ variables } = request.body) + t.test('Upload.', uploadTest(request.body.variables.file)) + .then(() => next()) + .catch(next) + }) + + const port = await startServer(t, app) + + await sendRequest(port) + + const file = await variables.file + await new Promise(resolve => file.capacitor.once('close', resolve)) + t.false(fs.existsSync(file.capacitor.path), 'Cleanup.') + }) +}) diff --git a/tap-snapshots/lib-test-TAP.test.js b/tap-snapshots/lib-test-TAP.test.js index a7b95f6..53ac247 100644 --- a/tap-snapshots/lib-test-TAP.test.js +++ b/tap-snapshots/lib-test-TAP.test.js @@ -432,3 +432,33 @@ exports[`lib/test TAP Missing ‘operations’, ‘map’ and files. Express mid "expose": true } ` + +exports[`lib/test TAP Deprecated file upload ‘stream’ property. Koa middleware. Upload. > Deprecation warning. 1`] = ` +{ + "name": "DeprecationWarning", + "message": "File upload property ‘stream’ is deprecated. Use ‘createReadStream()’ instead." +} +` + +exports[`lib/test TAP Deprecated file upload ‘stream’ property. Koa middleware. Upload. > Enumerable properties. 1`] = ` +{ + "filename": "a.txt", + "mimetype": "text/plain", + "encoding": "7bit" +} +` + +exports[`lib/test TAP Deprecated file upload ‘stream’ property. Express middleware. Upload. > Deprecation warning. 1`] = ` +{ + "name": "DeprecationWarning", + "message": "File upload property ‘stream’ is deprecated. Use ‘createReadStream()’ instead." +} +` + +exports[`lib/test TAP Deprecated file upload ‘stream’ property. Express middleware. Upload. > Enumerable properties. 1`] = ` +{ + "filename": "a.txt", + "mimetype": "text/plain", + "encoding": "7bit" +} +` From 8b9807af8d925f59ef81f2362701a1feca00a00a Mon Sep 17 00:00:00 2001 From: Jayden Seric Date: Fri, 17 Aug 2018 14:56:00 +1000 Subject: [PATCH 2/3] Simpler handling of deprecation warnings. --- src/test.mjs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/test.mjs b/src/test.mjs index 1fccb58..af0d953 100644 --- a/src/test.mjs +++ b/src/test.mjs @@ -1459,10 +1459,11 @@ t.test('Deprecated file upload ‘stream’ property.', async t => { } const uploadTest = upload => async t => { - // Store the original process deprecation modes. - const { noDeprecation, throwDeprecation } = process const resolved = await upload + // Store the original process deprecation mode. + const { throwDeprecation } = process + // Allow deprecation warning to be tested. process.throwDeprecation = true @@ -1473,23 +1474,18 @@ t.test('Deprecated file upload ‘stream’ property.', async t => { t.matchSnapshot(snapshotError(error), 'Deprecation warning.') } - // Restore process deprecation mode. + // Restore process deprecation mode. The warning won't appear again as + // Node.js only displays it once per process. process.throwDeprecation = throwDeprecation t.matchSnapshot(JSON.stringify(resolved, null, 2), 'Enumerable properties.') - // Silence deprecation warnings. - process.noDeprecation = true - t.true( resolved.stream === resolved.stream, 'Accessing ‘stream’ multiple times gets the same stream.' ) t.type(resolved.stream, ReadStream, 'Stream type.') t.equals(await streamToString(resolved.stream), 'a', 'Contents.') - - // Restore process deprecation mode. - process.noDeprecation = noDeprecation } await t.test('Koa middleware.', async t => { From 73ed354b2efc84f0fe1399758fca12eb7527230a Mon Sep 17 00:00:00 2001 From: Jayden Seric Date: Fri, 17 Aug 2018 14:57:12 +1000 Subject: [PATCH 3/3] Add PR link to the changelog. --- changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.md b/changelog.md index bcc3e21..40bd463 100644 --- a/changelog.md +++ b/changelog.md @@ -11,7 +11,7 @@ ### Minor - `Upload` scalar promises now resolve with a `createReadStream` method instead of a `stream` property, via [#92](https://github.com/jaydenseric/apollo-upload-server/pull/92). -- Accessing an `Upload` scalar promise resolved `stream` property results in a deprecation warning that recommends using `createReadStream` instead. It will be removed in a future release. +- Accessing an `Upload` scalar promise resolved `stream` property results in a deprecation warning that recommends using `createReadStream` instead. It will be removed in a future release. Via [#107](https://github.com/jaydenseric/apollo-upload-server/pull/107). - An `Upload` variable can now be used by multiple resolvers, via [#92](https://github.com/jaydenseric/apollo-upload-server/pull/92). - Multiple `Upload` scalar variables can now use the same multipart data, via [#92](https://github.com/jaydenseric/apollo-upload-server/pull/92). - Malformed requests containing invalid JSON for `operations` or `map` multipart fields cause an appropriate error with a `400` status instead of crashing the process, relating to [#81](https://github.com/jaydenseric/apollo-upload-server/pull/81) and [#95](https://github.com/jaydenseric/apollo-upload-server/issues/95).