From 2b1572965845d1f827d17def7d720ed22ac49e3d Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Sun, 23 Feb 2020 22:34:40 +0000 Subject: [PATCH] [misc] promisify FileHandler and remove dependency on async Signed-off-by: Jakob Ackermann --- services/filestore/app/js/FileHandler.js | 253 ++++++++---------- services/filestore/package-lock.json | 5 - services/filestore/package.json | 1 - .../test/unit/js/FileHandlerTests.js | 108 +++++--- 4 files changed, 180 insertions(+), 187 deletions(-) diff --git a/services/filestore/app/js/FileHandler.js b/services/filestore/app/js/FileHandler.js index 3c5b50e693..02831fa3d0 100644 --- a/services/filestore/app/js/FileHandler.js +++ b/services/filestore/app/js/FileHandler.js @@ -1,176 +1,153 @@ -const { promisify } = require('util') +const { callbackify } = require('util') const fs = require('fs') const PersistorManager = require('./PersistorManager') const LocalFileWriter = require('./LocalFileWriter') const FileConverter = require('./FileConverter') const KeyBuilder = require('./KeyBuilder') -const async = require('async') const ImageOptimiser = require('./ImageOptimiser') const { ConversionError } = require('./Errors') module.exports = { - insertFile, - deleteFile, - getFile, - getFileSize, - getDirectorySize, + insertFile: callbackify(insertFile), + deleteFile: callbackify(deleteFile), + getFile: callbackify(getFile), + getFileSize: callbackify(getFileSize), + getDirectorySize: callbackify(getDirectorySize), promises: { - getFile: promisify(getFile), - insertFile: promisify(insertFile), - deleteFile: promisify(deleteFile), - getFileSize: promisify(getFileSize), - getDirectorySize: promisify(getDirectorySize) + getFile, + insertFile, + deleteFile, + getFileSize, + getDirectorySize } } -function insertFile(bucket, key, stream, callback) { +async function insertFile(bucket, key, stream) { const convertedKey = KeyBuilder.getConvertedFolderKey(key) - PersistorManager.deleteDirectory(bucket, convertedKey, function(error) { - if (error) { - return callback(error) - } - PersistorManager.sendStream(bucket, key, stream, callback) - }) + await PersistorManager.promises.deleteDirectory(bucket, convertedKey) + await PersistorManager.promises.sendStream(bucket, key, stream) } -function deleteFile(bucket, key, callback) { +async function deleteFile(bucket, key) { const convertedKey = KeyBuilder.getConvertedFolderKey(key) - async.parallel( - [ - done => PersistorManager.deleteFile(bucket, key, done), - done => PersistorManager.deleteDirectory(bucket, convertedKey, done) - ], - callback - ) + await Promise.all([ + PersistorManager.promises.deleteFile(bucket, key), + PersistorManager.promises.deleteDirectory(bucket, convertedKey) + ]) } -function getFile(bucket, key, opts, callback) { +async function getFile(bucket, key, opts) { opts = opts || {} if (!opts.format && !opts.style) { - PersistorManager.getFileStream(bucket, key, opts, callback) + return PersistorManager.promises.getFileStream(bucket, key, opts) } else { - _getConvertedFile(bucket, key, opts, callback) + return _getConvertedFile(bucket, key, opts) } } -function getFileSize(bucket, key, callback) { - PersistorManager.getFileSize(bucket, key, callback) +async function getFileSize(bucket, key) { + return PersistorManager.promises.getFileSize(bucket, key) } -function getDirectorySize(bucket, projectId, callback) { - PersistorManager.directorySize(bucket, projectId, callback) +async function getDirectorySize(bucket, projectId) { + return PersistorManager.promises.directorySize(bucket, projectId) } -function _getConvertedFile(bucket, key, opts, callback) { +async function _getConvertedFile(bucket, key, opts) { const convertedKey = KeyBuilder.addCachingToKey(key, opts) - PersistorManager.checkIfFileExists(bucket, convertedKey, (err, exists) => { - if (err) { - return callback(err) - } - - if (exists) { - PersistorManager.getFileStream(bucket, convertedKey, opts, callback) - } else { - _getConvertedFileAndCache(bucket, key, convertedKey, opts, callback) - } - }) -} - -function _getConvertedFileAndCache(bucket, key, convertedKey, opts, callback) { - let convertedFsPath - - async.series( - [ - cb => { - _convertFile(bucket, key, opts, function(err, fileSystemPath) { - convertedFsPath = fileSystemPath - cb(err) - }) - }, - cb => ImageOptimiser.compressPng(convertedFsPath, cb), - cb => PersistorManager.sendFile(bucket, convertedKey, convertedFsPath, cb) - ], - function(err) { - if (err) { - LocalFileWriter.deleteFile(convertedFsPath, function() {}) - return callback( - new ConversionError({ - message: 'failed to convert file', - info: { opts, bucket, key, convertedKey } - }).withCause(err) - ) - } - // Send back the converted file from the local copy to avoid problems - // with the file not being present in S3 yet. As described in the - // documentation below, we have already made a 'HEAD' request in - // checkIfFileExists so we only have "eventual consistency" if we try - // to stream it from S3 here. This was a cause of many 403 errors. - // - // "Amazon S3 provides read-after-write consistency for PUTS of new - // objects in your S3 bucket in all regions with one caveat. The - // caveat is that if you make a HEAD or GET request to the key name - // (to find if the object exists) before creating the object, Amazon - // S3 provides eventual consistency for read-after-write."" - // https://docs.aws.amazon.com/AmazonS3/latest/dev/Introduction.html#ConsistencyModel - const readStream = fs.createReadStream(convertedFsPath) - readStream.on('end', function() { - LocalFileWriter.deleteFile(convertedFsPath, function() {}) - }) - callback(null, readStream) - } + const exists = await PersistorManager.promises.checkIfFileExists( + bucket, + convertedKey ) + if (exists) { + return PersistorManager.promises.getFileStream(bucket, convertedKey, opts) + } else { + return _getConvertedFileAndCache(bucket, key, convertedKey, opts) + } } -function _convertFile(bucket, originalKey, opts, callback) { - _writeFileToDisk(bucket, originalKey, opts, function(err, originalFsPath) { - if (err) { - return callback( - new ConversionError({ - message: 'unable to write file to disk', - info: { bucket, originalKey, opts } - }).withCause(err) - ) - } +async function _getConvertedFileAndCache(bucket, key, convertedKey, opts) { + let convertedFsPath + try { + convertedFsPath = await _convertFile(bucket, key, opts) + await ImageOptimiser.promises.compressPng(convertedFsPath) + await PersistorManager.promises.sendFile( + bucket, + convertedKey, + convertedFsPath + ) + } catch (err) { + LocalFileWriter.deleteFile(convertedFsPath, () => {}) + throw new ConversionError({ + message: 'failed to convert file', + info: { opts, bucket, key, convertedKey } + }).withCause(err) + } + // Send back the converted file from the local copy to avoid problems + // with the file not being present in S3 yet. As described in the + // documentation below, we have already made a 'HEAD' request in + // checkIfFileExists so we only have "eventual consistency" if we try + // to stream it from S3 here. This was a cause of many 403 errors. + // + // "Amazon S3 provides read-after-write consistency for PUTS of new + // objects in your S3 bucket in all regions with one caveat. The + // caveat is that if you make a HEAD or GET request to the key name + // (to find if the object exists) before creating the object, Amazon + // S3 provides eventual consistency for read-after-write."" + // https://docs.aws.amazon.com/AmazonS3/latest/dev/Introduction.html#ConsistencyModel + const readStream = fs.createReadStream(convertedFsPath) + readStream.on('end', function() { + LocalFileWriter.deleteFile(convertedFsPath, function() {}) + }) + return readStream +} - const done = function(err, destPath) { - if (err) { - return callback( - new ConversionError({ - message: 'error converting file', - info: { bucket, originalKey, opts } - }).withCause(err) - ) +async function _convertFile(bucket, originalKey, opts) { + let originalFsPath + try { + originalFsPath = await _writeFileToDisk(bucket, originalKey, opts) + } catch (err) { + throw new ConversionError({ + message: 'unable to write file to disk', + info: { bucket, originalKey, opts } + }).withCause(err) + } + + let promise + if (opts.format) { + promise = FileConverter.promises.convert(originalFsPath, opts.format) + } else if (opts.style === 'thumbnail') { + promise = FileConverter.promises.thumbnail(originalFsPath) + } else if (opts.style === 'preview') { + promise = FileConverter.promises.preview(originalFsPath) + } else { + throw new ConversionError({ + message: 'invalid file conversion options', + info: { + bucket, + originalKey, + opts } - LocalFileWriter.deleteFile(originalFsPath, function() {}) - callback(err, destPath) - } - - if (opts.format) { - FileConverter.convert(originalFsPath, opts.format, done) - } else if (opts.style === 'thumbnail') { - FileConverter.thumbnail(originalFsPath, done) - } else if (opts.style === 'preview') { - FileConverter.preview(originalFsPath, done) - } else { - callback( - new ConversionError({ - message: 'invalid file conversion options', - info: { - bucket, - originalKey, - opts - } - }) - ) - } - }) + }) + } + let destPath + try { + destPath = await promise + } catch (err) { + throw new ConversionError({ + message: 'error converting file', + info: { bucket, originalKey, opts } + }).withCause(err) + } + LocalFileWriter.deleteFile(originalFsPath, function() {}) + return destPath } -function _writeFileToDisk(bucket, key, opts, callback) { - PersistorManager.getFileStream(bucket, key, opts, function(err, fileStream) { - if (err) { - return callback(err) - } - LocalFileWriter.writeStream(fileStream, key, callback) - }) +async function _writeFileToDisk(bucket, key, opts) { + const fileStream = await PersistorManager.promises.getFileStream( + bucket, + key, + opts + ) + return LocalFileWriter.promises.writeStream(fileStream, key) } diff --git a/services/filestore/package-lock.json b/services/filestore/package-lock.json index f8b4a70297..132250950e 100644 --- a/services/filestore/package-lock.json +++ b/services/filestore/package-lock.json @@ -1256,11 +1256,6 @@ "integrity": "sha512-+Ryf6g3BKoRc7jfp7ad8tM4TtMiaWvbF/1/sQcZPkkS7ag3D5nMBCe2UfOTONtAkaG0tO0ij3C5Lwmf1EiyjHg==", "dev": true }, - "async": { - "version": "0.2.10", - "resolved": "https://registry.npmjs.org/async/-/async-0.2.10.tgz", - "integrity": "sha512-eAkdoKxU6/LkKDBzLpT+t6Ff5EtfSF4wx1WfJiPEEV7WNLnDaRXk0oVysiEPm262roaachGexwUv94WhSgN5TQ==" - }, "async-listener": { "version": "0.6.10", "resolved": "https://registry.npmjs.org/async-listener/-/async-listener-0.6.10.tgz", diff --git a/services/filestore/package.json b/services/filestore/package.json index 4a5d72abb5..1c77eea173 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -21,7 +21,6 @@ }, "dependencies": { "@overleaf/o-error": "^2.1.0", - "async": "~0.2.10", "aws-sdk": "^2.628.0", "body-parser": "^1.2.0", "express": "^4.2.0", diff --git a/services/filestore/test/unit/js/FileHandlerTests.js b/services/filestore/test/unit/js/FileHandlerTests.js index 771ff998eb..623ed440b0 100644 --- a/services/filestore/test/unit/js/FileHandlerTests.js +++ b/services/filestore/test/unit/js/FileHandlerTests.js @@ -4,6 +4,9 @@ const { expect } = chai const modulePath = '../../../app/js/FileHandler.js' const SandboxedModule = require('sandboxed-module') +chai.use(require('sinon-chai')) +chai.use(require('chai-as-promised')) + describe('FileHandler', function() { let PersistorManager, LocalFileWriter, @@ -32,29 +35,41 @@ describe('FileHandler', function() { beforeEach(function() { PersistorManager = { - getFileStream: sinon.stub().yields(null, sourceStream), - checkIfFileExists: sinon.stub().yields(), - deleteFile: sinon.stub().yields(), - deleteDirectory: sinon.stub().yields(), - sendStream: sinon.stub().yields(), - insertFile: sinon.stub().yields(), - sendFile: sinon.stub().yields(), - directorySize: sinon.stub().yields() + promises: { + getFileStream: sinon.stub().resolves(sourceStream), + checkIfFileExists: sinon.stub().resolves(), + deleteFile: sinon.stub().resolves(), + deleteDirectory: sinon.stub().resolves(), + sendStream: sinon.stub().resolves(), + insertFile: sinon.stub().resolves(), + sendFile: sinon.stub().resolves(), + directorySize: sinon.stub().resolves() + } } LocalFileWriter = { - writeStream: sinon.stub().yields(), - deleteFile: sinon.stub().yields() + // the callback style is used for detached cleanup calls + deleteFile: sinon.stub().yields(), + promises: { + writeStream: sinon.stub().resolves(), + deleteFile: sinon.stub().resolves() + } } FileConverter = { - convert: sinon.stub().yields(), - thumbnail: sinon.stub().yields(), - preview: sinon.stub().yields() + promises: { + convert: sinon.stub().resolves(), + thumbnail: sinon.stub().resolves(), + preview: sinon.stub().resolves() + } } KeyBuilder = { addCachingToKey: sinon.stub().returns(convertedKey), getConvertedFolderKey: sinon.stub().returns(convertedFolderKey) } - ImageOptimiser = { compressPng: sinon.stub().yields() } + ImageOptimiser = { + promises: { + compressPng: sinon.stub().resolves() + } + } fs = { createReadStream: sinon.stub().returns(readStream) } @@ -79,7 +94,7 @@ describe('FileHandler', function() { it('should send file to the filestore', function(done) { FileHandler.insertFile(bucket, key, stream, err => { expect(err).not.to.exist - expect(PersistorManager.sendStream).to.have.been.calledWith( + expect(PersistorManager.promises.sendStream).to.have.been.calledWith( bucket, key, stream @@ -91,10 +106,9 @@ describe('FileHandler', function() { it('should delete the convertedKey folder', function(done) { FileHandler.insertFile(bucket, key, stream, err => { expect(err).not.to.exist - expect(PersistorManager.deleteDirectory).to.have.been.calledWith( - bucket, - convertedFolderKey - ) + expect( + PersistorManager.promises.deleteDirectory + ).to.have.been.calledWith(bucket, convertedFolderKey) done() }) }) @@ -104,7 +118,10 @@ describe('FileHandler', function() { it('should tell the filestore manager to delete the file', function(done) { FileHandler.deleteFile(bucket, key, err => { expect(err).not.to.exist - expect(PersistorManager.deleteFile).to.have.been.calledWith(bucket, key) + expect(PersistorManager.promises.deleteFile).to.have.been.calledWith( + bucket, + key + ) done() }) }) @@ -112,10 +129,9 @@ describe('FileHandler', function() { it('should tell the filestore manager to delete the cached folder', function(done) { FileHandler.deleteFile(bucket, key, err => { expect(err).not.to.exist - expect(PersistorManager.deleteDirectory).to.have.been.calledWith( - bucket, - convertedFolderKey - ) + expect( + PersistorManager.promises.deleteDirectory + ).to.have.been.calledWith(bucket, convertedFolderKey) done() }) }) @@ -134,7 +150,7 @@ describe('FileHandler', function() { const options = { start: 0, end: 8 } FileHandler.getFile(bucket, key, options, err => { expect(err).not.to.exist - expect(PersistorManager.getFileStream).to.have.been.calledWith( + expect(PersistorManager.promises.getFileStream).to.have.been.calledWith( bucket, key, options @@ -155,23 +171,27 @@ describe('FileHandler', function() { }) it('should convert the file', function() { - expect(FileConverter.convert).to.have.been.called - expect(ImageOptimiser.compressPng).to.have.been.called + expect(FileConverter.promises.convert).to.have.been.called + }) + + it('should compress the converted file', function() { + expect(ImageOptimiser.promises.compressPng).to.have.been.called }) it('should return the the converted stream', function() { expect(result.err).not.to.exist expect(result.stream).to.equal(readStream) - expect(PersistorManager.getFileStream).to.have.been.calledWith( - bucket, - key - ) + expect( + PersistorManager.promises.getFileStream + ).to.have.been.calledWith(bucket, key) }) }) describe('when the file is cached', function() { beforeEach(function(done) { - PersistorManager.checkIfFileExists = sinon.stub().yields(null, true) + PersistorManager.promises.checkIfFileExists = sinon + .stub() + .resolves(true) FileHandler.getFile(bucket, key, { format: 'png' }, (err, stream) => { result = { err, stream } done() @@ -179,17 +199,19 @@ describe('FileHandler', function() { }) it('should not convert the file', function() { - expect(FileConverter.convert).not.to.have.been.called - expect(ImageOptimiser.compressPng).not.to.have.been.called + expect(FileConverter.promises.convert).not.to.have.been.called + }) + + it('should not compress the converted file again', function() { + expect(ImageOptimiser.promises.compressPng).not.to.have.been.called }) it('should return the cached stream', function() { expect(result.err).not.to.exist expect(result.stream).to.equal(sourceStream) - expect(PersistorManager.getFileStream).to.have.been.calledWith( - bucket, - convertedKey - ) + expect( + PersistorManager.promises.getFileStream + ).to.have.been.calledWith(bucket, convertedKey) }) }) }) @@ -198,8 +220,8 @@ describe('FileHandler', function() { it('generates a thumbnail when requested', function(done) { FileHandler.getFile(bucket, key, { style: 'thumbnail' }, err => { expect(err).not.to.exist - expect(FileConverter.thumbnail).to.have.been.called - expect(FileConverter.preview).not.to.have.been.called + expect(FileConverter.promises.thumbnail).to.have.been.called + expect(FileConverter.promises.preview).not.to.have.been.called done() }) }) @@ -207,8 +229,8 @@ describe('FileHandler', function() { it('generates a preview when requested', function(done) { FileHandler.getFile(bucket, key, { style: 'preview' }, err => { expect(err).not.to.exist - expect(FileConverter.thumbnail).not.to.have.been.called - expect(FileConverter.preview).to.have.been.called + expect(FileConverter.promises.thumbnail).not.to.have.been.called + expect(FileConverter.promises.preview).to.have.been.called done() }) }) @@ -219,7 +241,7 @@ describe('FileHandler', function() { it('should call the filestore manager to get directory size', function(done) { FileHandler.getDirectorySize(bucket, key, err => { expect(err).not.to.exist - expect(PersistorManager.directorySize).to.have.been.calledWith( + expect(PersistorManager.promises.directorySize).to.have.been.calledWith( bucket, key )