diff --git a/services/filestore/app/js/Errors.js b/services/filestore/app/js/Errors.js index 4231571cb3..57dbdbe522 100644 --- a/services/filestore/app/js/Errors.js +++ b/services/filestore/app/js/Errors.js @@ -18,7 +18,10 @@ class BackwardCompatibleError extends OError { } class NotFoundError extends BackwardCompatibleError {} +class WriteError extends BackwardCompatibleError {} +class ReadError extends BackwardCompatibleError {} class ConversionsDisabledError extends BackwardCompatibleError {} +class ConversionError extends BackwardCompatibleError {} class FailedCommandError extends OError { constructor(command, code, stdout, stderr) { @@ -35,4 +38,11 @@ class FailedCommandError extends OError { } } -module.exports = { NotFoundError, FailedCommandError, ConversionsDisabledError } +module.exports = { + NotFoundError, + FailedCommandError, + ConversionsDisabledError, + WriteError, + ReadError, + ConversionError +} diff --git a/services/filestore/app/js/FileHandler.js b/services/filestore/app/js/FileHandler.js index e63c813167..3e102b316b 100644 --- a/services/filestore/app/js/FileHandler.js +++ b/services/filestore/app/js/FileHandler.js @@ -1,18 +1,5 @@ -/* eslint-disable - camelcase, - no-self-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -let FileHandler -const settings = require('settings-sharelatex') +const { promisify } = require('util') +const fs = require('fs') const PersistorManager = require('./PersistorManager') const LocalFileWriter = require('./LocalFileWriter') const logger = require('logger-sharelatex') @@ -20,216 +7,196 @@ const FileConverter = require('./FileConverter') const KeyBuilder = require('./KeyBuilder') const async = require('async') const ImageOptimiser = require('./ImageOptimiser') -const Errors = require('./Errors') +const { WriteError, ReadError, ConversionError } = require('./Errors') -module.exports = FileHandler = { - insertFile(bucket, key, stream, callback) { - const convertedKey = KeyBuilder.getConvertedFolderKey(key) - return PersistorManager.deleteDirectory(bucket, convertedKey, function( - error - ) { - if (error != null) { - return callback(error) - } - return PersistorManager.sendStream(bucket, key, stream, callback) - }) - }, - - deleteFile(bucket, key, callback) { - const convertedKey = KeyBuilder.getConvertedFolderKey(key) - return async.parallel( - [ - done => PersistorManager.deleteFile(bucket, key, done), - done => PersistorManager.deleteDirectory(bucket, convertedKey, done) - ], - callback - ) - }, - - getFile(bucket, key, opts, callback) { - // In this call, opts can contain credentials - if (opts == null) { - opts = {} - } - logger.log({ bucket, key, opts: this._scrubSecrets(opts) }, 'getting file') - if (opts.format == null && opts.style == null) { - return this._getStandardFile(bucket, key, opts, callback) - } else { - return this._getConvertedFile(bucket, key, opts, callback) - } - }, - - getFileSize(bucket, key, callback) { - return PersistorManager.getFileSize(bucket, key, callback) - }, - - _getStandardFile(bucket, key, opts, callback) { - return PersistorManager.getFileStream(bucket, key, opts, function( - err, - fileStream - ) { - if (err != null && !(err instanceof Errors.NotFoundError)) { - logger.err( - { bucket, key, opts: FileHandler._scrubSecrets(opts) }, - 'error getting fileStream' - ) - } - return callback(err, fileStream) - }) - }, - - _getConvertedFile(bucket, key, opts, callback) { - const convertedKey = KeyBuilder.addCachingToKey(key, opts) - return PersistorManager.checkIfFileExists( - bucket, - convertedKey, - (err, exists) => { - if (err != null) { - return callback(err) - } - if (exists) { - return PersistorManager.getFileStream( - bucket, - convertedKey, - opts, - callback - ) - } else { - return this._getConvertedFileAndCache( - bucket, - key, - convertedKey, - opts, - callback - ) - } - } - ) - }, - - _getConvertedFileAndCache(bucket, key, convertedKey, opts, callback) { - let convertedFsPath = '' - const originalFsPath = '' - return async.series( - [ - cb => { - return this._convertFile(bucket, key, opts, function( - err, - fileSystemPath, - originalFsPath - ) { - convertedFsPath = fileSystemPath - originalFsPath = originalFsPath - return cb(err) - }) - }, - cb => ImageOptimiser.compressPng(convertedFsPath, cb), - cb => - PersistorManager.sendFile(bucket, convertedKey, convertedFsPath, cb) - ], - function(err) { - if (err != null) { - LocalFileWriter.deleteFile(convertedFsPath, function() {}) - LocalFileWriter.deleteFile(originalFsPath, function() {}) - return callback(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 - return LocalFileWriter.getStream(convertedFsPath, function( - err, - readStream - ) { - if (err != null) { - return callback(err) - } - readStream.on('end', function() { - logger.log({ convertedFsPath }, 'deleting temporary file') - return LocalFileWriter.deleteFile(convertedFsPath, function() {}) - }) - return callback(null, readStream) - }) - } - ) - }, - - _convertFile(bucket, originalKey, opts, callback) { - return this._writeS3FileToDisk(bucket, originalKey, opts, function( - err, - originalFsPath - ) { - if (err != null) { - return callback(err) - } - const done = function(err, destPath) { - if (err != null) { - logger.err( - { err, bucket, originalKey, opts: FileHandler._scrubSecrets(opts) }, - 'error converting file' - ) - return callback(err) - } - LocalFileWriter.deleteFile(originalFsPath, function() {}) - return callback(err, destPath, originalFsPath) - } - - logger.log({ opts }, 'converting file depending on opts') - - if (opts.format != null) { - return FileConverter.convert(originalFsPath, opts.format, done) - } else if (opts.style === 'thumbnail') { - return FileConverter.thumbnail(originalFsPath, done) - } else if (opts.style === 'preview') { - return FileConverter.preview(originalFsPath, done) - } else { - return callback( - new Error( - `should have specified opts to convert file with ${JSON.stringify( - opts - )}` - ) - ) - } - }) - }, - - _writeS3FileToDisk(bucket, key, opts, callback) { - return PersistorManager.getFileStream(bucket, key, opts, function( - err, - fileStream - ) { - if (err != null) { - return callback(err) - } - return LocalFileWriter.writeStream(fileStream, key, callback) - }) - }, - - getDirectorySize(bucket, project_id, callback) { - logger.log({ bucket, project_id }, 'getting project size') - return PersistorManager.directorySize(bucket, project_id, function( - err, - size - ) { - if (err != null) { - logger.err({ bucket, project_id }, 'error getting size') - } - return callback(err, size) - }) - }, - - _scrubSecrets(opts) { - const safe = Object.assign({}, opts) - delete safe.credentials - return safe +module.exports = { + insertFile, + deleteFile, + getFile, + getFileSize, + getDirectorySize, + promises: { + getFile: promisify(getFile), + insertFile: promisify(insertFile), + deleteFile: promisify(deleteFile), + getFileSize: promisify(getFileSize), + getDirectorySize: promisify(getDirectorySize) } } + +function insertFile(bucket, key, stream, callback) { + const convertedKey = KeyBuilder.getConvertedFolderKey(key) + PersistorManager.deleteDirectory(bucket, convertedKey, function(error) { + if (error) { + return callback(new WriteError('error inserting file').withCause(error)) + } + PersistorManager.sendStream(bucket, key, stream, callback) + }) +} + +function deleteFile(bucket, key, callback) { + const convertedKey = KeyBuilder.getConvertedFolderKey(key) + async.parallel( + [ + done => PersistorManager.deleteFile(bucket, key, done), + done => PersistorManager.deleteDirectory(bucket, convertedKey, done) + ], + callback + ) +} + +function getFile(bucket, key, opts, callback) { + // In this call, opts can contain credentials + if (!opts) { + opts = {} + } + logger.log({ bucket, key, opts: _scrubSecrets(opts) }, 'getting file') + if (!opts.format && !opts.style) { + _getStandardFile(bucket, key, opts, callback) + } else { + _getConvertedFile(bucket, key, opts, callback) + } +} + +function getFileSize(bucket, key, callback) { + PersistorManager.getFileSize(bucket, key, callback) +} + +function getDirectorySize(bucket, projectId, callback) { + logger.log({ bucket, project_id: projectId }, 'getting project size') + PersistorManager.directorySize(bucket, projectId, function(err, size) { + if (err) { + logger.err({ bucket, project_id: projectId }, 'error getting size') + err = new ReadError('error getting project size').withCause(err) + } + return callback(err, size) + }) +} + +function _getStandardFile(bucket, key, opts, callback) { + PersistorManager.getFileStream(bucket, key, opts, function(err, fileStream) { + if (err && err.name !== 'NotFoundError') { + logger.err( + { bucket, key, opts: _scrubSecrets(opts) }, + 'error getting fileStream' + ) + } + callback(err, fileStream) + }) +} + +function _getConvertedFile(bucket, key, opts, callback) { + 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('failed to convert file').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) + } + ) +} + +function _convertFile(bucket, originalKey, opts, callback) { + _writeFileToDisk(bucket, originalKey, opts, function(err, originalFsPath) { + if (err) { + return callback( + new ConversionError('unable to write file to disk').withCause(err) + ) + } + + const done = function(err, destPath) { + if (err) { + logger.err( + { err, bucket, originalKey, opts: _scrubSecrets(opts) }, + 'error converting file' + ) + return callback( + new ConversionError('error converting file').withCause(err) + ) + } + LocalFileWriter.deleteFile(originalFsPath, function() {}) + callback(err, destPath) + } + + logger.log({ opts }, 'converting file depending on opts') + + 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( + `should have specified opts to convert file with ${JSON.stringify( + opts + )}` + ) + ) + } + }) +} + +function _writeFileToDisk(bucket, key, opts, callback) { + PersistorManager.getFileStream(bucket, key, opts, function(err, fileStream) { + if (err) { + return callback( + new ReadError('unable to get read stream for file').withCause(err) + ) + } + LocalFileWriter.writeStream(fileStream, key, callback) + }) +} + +function _scrubSecrets(opts) { + const safe = Object.assign({}, opts) + delete safe.credentials + return safe +} diff --git a/services/filestore/app/js/LocalFileWriter.js b/services/filestore/app/js/LocalFileWriter.js index 8a541a35e9..44f3f9433a 100644 --- a/services/filestore/app/js/LocalFileWriter.js +++ b/services/filestore/app/js/LocalFileWriter.js @@ -1,91 +1,57 @@ -/* eslint-disable - handle-callback-err, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const fs = require('fs') const uuid = require('node-uuid') const path = require('path') -const _ = require('underscore') +const Stream = require('stream') +const { callbackify, promisify } = require('util') const logger = require('logger-sharelatex') const metrics = require('metrics-sharelatex') const Settings = require('settings-sharelatex') -const Errors = require('./Errors') +const { WriteError } = require('./Errors') module.exports = { - writeStream(stream, key, callback) { - const timer = new metrics.Timer('writingFile') - callback = _.once(callback) - const fsPath = this._getPath(key) - logger.log({ fsPath }, 'writing file locally') - const writeStream = fs.createWriteStream(fsPath) - writeStream.on('finish', function() { - timer.done() - logger.log({ fsPath }, 'finished writing file locally') - return callback(null, fsPath) - }) - writeStream.on('error', function(err) { - logger.err( - { err, fsPath }, - 'problem writing file locally, with write stream' - ) - return callback(err) - }) - stream.on('error', function(err) { - logger.log( - { err, fsPath }, - 'problem writing file locally, with read stream' - ) - return callback(err) - }) - return stream.pipe(writeStream) + promises: { + writeStream, + deleteFile }, + writeStream: callbackify(writeStream), + deleteFile: callbackify(deleteFile) +} - getStream(fsPath, _callback) { - if (_callback == null) { - _callback = function(err, res) {} - } - const callback = _.once(_callback) - const timer = new metrics.Timer('readingFile') - logger.log({ fsPath }, 'reading file locally') - const readStream = fs.createReadStream(fsPath) - readStream.on('end', function() { - timer.done() - return logger.log({ fsPath }, 'finished reading file locally') - }) - readStream.on('error', function(err) { - logger.err( - { err, fsPath }, - 'problem reading file locally, with read stream' - ) - if (err.code === 'ENOENT') { - return callback(new Errors.NotFoundError(err.message), null) - } else { - return callback(err) - } - }) - return callback(null, readStream) - }, +const pipeline = promisify(Stream.pipeline) - deleteFile(fsPath, callback) { - if (fsPath == null || fsPath === '') { - return callback() - } - logger.log({ fsPath }, 'removing local temp file') - return fs.unlink(fsPath, callback) - }, +async function writeStream(stream, key) { + const timer = new metrics.Timer('writingFile') + const fsPath = _getPath(key) - _getPath(key) { - if (key == null) { - key = uuid.v1() - } - key = key.replace(/\//g, '-') - return path.join(Settings.path.uploadFolder, key) + logger.log({ fsPath }, 'writing file locally') + + const writeStream = fs.createWriteStream(fsPath) + try { + await pipeline(stream, writeStream) + timer.done() + logger.log({ fsPath }, 'finished writing file locally') + return fsPath + } catch (err) { + logger.err({ err, fsPath }, 'problem writing file locally') + throw new WriteError({ + message: 'problem writing file locally', + info: { err, fsPath } + }).withCause(err) } } + +async function deleteFile(fsPath) { + if (!fsPath) { + return + } + logger.log({ fsPath }, 'removing local temp file') + await promisify(fs.unlink)(fsPath) +} + +function _getPath(key) { + if (key == null) { + key = uuid.v1() + } + key = key.replace(/\//g, '-') + return path.join(Settings.path.uploadFolder, key) +} diff --git a/services/filestore/npm-shrinkwrap.json b/services/filestore/npm-shrinkwrap.json index ef0f78fc15..44f5ec3263 100644 --- a/services/filestore/npm-shrinkwrap.json +++ b/services/filestore/npm-shrinkwrap.json @@ -4763,6 +4763,12 @@ "type-detect": "^4.0.8" } }, + "sinon-chai": { + "version": "3.3.0", + "resolved": "https://registry.npmjs.org/sinon-chai/-/sinon-chai-3.3.0.tgz", + "integrity": "sha512-r2JhDY7gbbmh5z3Q62pNbrjxZdOAjpsqW/8yxAZRSqLZqowmfGZPGUZPFf3UX36NLis0cv8VEM5IJh9HgkSOAA==", + "dev": true + }, "slice-ansi": { "version": "2.1.0", "resolved": "https://registry.npmjs.org/slice-ansi/-/slice-ansi-2.1.0.tgz", diff --git a/services/filestore/package.json b/services/filestore/package.json index fd03757072..d39d1027be 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -62,6 +62,7 @@ "prettier-eslint": "^9.0.1", "prettier-eslint-cli": "^5.0.0", "sandboxed-module": "2.0.3", - "sinon": "7.1.1" + "sinon": "7.1.1", + "sinon-chai": "^3.3.0" } } diff --git a/services/filestore/test/unit/js/FileHandlerTests.js b/services/filestore/test/unit/js/FileHandlerTests.js index e641ffdd16..671e5c41ea 100644 --- a/services/filestore/test/unit/js/FileHandlerTests.js +++ b/services/filestore/test/unit/js/FileHandlerTests.js @@ -1,367 +1,233 @@ -/* eslint-disable - handle-callback-err, - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const { assert } = require('chai') const sinon = require('sinon') const chai = require('chai') -const should = chai.should() const { expect } = chai const modulePath = '../../../app/js/FileHandler.js' const SandboxedModule = require('sandboxed-module') describe('FileHandler', function() { - beforeEach(function() { - this.settings = { - s3: { - buckets: { - user_files: 'user_files' - } + let PersistorManager, + LocalFileWriter, + FileConverter, + KeyBuilder, + ImageOptimiser, + FileHandler, + fs + const settings = { + s3: { + buckets: { + user_files: 'user_files' } } - this.PersistorManager = { - getFileStream: sinon.stub(), - checkIfFileExists: sinon.stub(), - deleteFile: sinon.stub(), - deleteDirectory: sinon.stub(), - sendStream: sinon.stub(), - insertFile: sinon.stub(), - directorySize: sinon.stub() + } + + const bucket = 'my_bucket' + const key = 'key/here' + const convertedFolderKey = 'convertedFolder' + const sourceStream = 'sourceStream' + const convertedKey = 'convertedKey' + const readStream = { + stream: 'readStream', + on: sinon.stub() + } + + 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() } - this.LocalFileWriter = { - writeStream: sinon.stub(), - getStream: sinon.stub(), - deleteFile: sinon.stub() + LocalFileWriter = { + writeStream: sinon.stub().yields(), + deleteFile: sinon.stub().yields() } - this.FileConverter = { - convert: sinon.stub(), - thumbnail: sinon.stub(), - preview: sinon.stub() + FileConverter = { + convert: sinon.stub().yields(), + thumbnail: sinon.stub().yields(), + preview: sinon.stub().yields() } - this.keyBuilder = { - addCachingToKey: sinon.stub(), - getConvertedFolderKey: sinon.stub() + KeyBuilder = { + addCachingToKey: sinon.stub().returns(convertedKey), + getConvertedFolderKey: sinon.stub().returns(convertedFolderKey) } - this.ImageOptimiser = { compressPng: sinon.stub() } - this.handler = SandboxedModule.require(modulePath, { + ImageOptimiser = { compressPng: sinon.stub().yields() } + fs = { + createReadStream: sinon.stub().returns(readStream) + } + + FileHandler = SandboxedModule.require(modulePath, { requires: { - 'settings-sharelatex': this.settings, - './PersistorManager': this.PersistorManager, - './LocalFileWriter': this.LocalFileWriter, - './FileConverter': this.FileConverter, - './KeyBuilder': this.keyBuilder, - './ImageOptimiser': this.ImageOptimiser, + 'settings-sharelatex': settings, + './PersistorManager': PersistorManager, + './LocalFileWriter': LocalFileWriter, + './FileConverter': FileConverter, + './KeyBuilder': KeyBuilder, + './ImageOptimiser': ImageOptimiser, + fs: fs, 'logger-sharelatex': { log() {}, err() {} } - } + }, + globals: { console } }) - this.bucket = 'my_bucket' - this.key = 'key/here' - this.stubbedPath = '/var/somewhere/path' - this.format = 'png' - return (this.formattedStubbedPath = `${this.stubbedPath}.${this.format}`) }) describe('insertFile', function() { - beforeEach(function() { - this.stream = {} - this.PersistorManager.deleteDirectory.callsArgWith(2) - return this.PersistorManager.sendStream.callsArgWith(3) - }) + const stream = 'stream' it('should send file to the filestore', function(done) { - return this.handler.insertFile(this.bucket, this.key, this.stream, () => { - this.PersistorManager.sendStream - .calledWith(this.bucket, this.key, this.stream) - .should.equal(true) - return done() + FileHandler.insertFile(bucket, key, stream, err => { + expect(err).not.to.exist + expect(PersistorManager.sendStream).to.have.been.calledWith( + bucket, + key, + stream + ) + done() }) }) - return it('should delete the convetedKey folder', function(done) { - this.keyBuilder.getConvertedFolderKey.returns(this.stubbedConvetedKey) - return this.handler.insertFile(this.bucket, this.key, this.stream, () => { - this.PersistorManager.deleteDirectory - .calledWith(this.bucket, this.stubbedConvetedKey) - .should.equal(true) - return done() + 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 + ) + done() }) }) }) describe('deleteFile', function() { - beforeEach(function() { - this.keyBuilder.getConvertedFolderKey.returns(this.stubbedConvetedKey) - this.PersistorManager.deleteFile.callsArgWith(2) - return this.PersistorManager.deleteDirectory.callsArgWith(2) - }) - it('should tell the filestore manager to delete the file', function(done) { - return this.handler.deleteFile(this.bucket, this.key, () => { - this.PersistorManager.deleteFile - .calledWith(this.bucket, this.key) - .should.equal(true) - return done() + FileHandler.deleteFile(bucket, key, err => { + expect(err).not.to.exist + expect(PersistorManager.deleteFile).to.have.been.calledWith(bucket, key) + done() }) }) - return it('should tell the filestore manager to delete the cached foler', function(done) { - return this.handler.deleteFile(this.bucket, this.key, () => { - this.PersistorManager.deleteDirectory - .calledWith(this.bucket, this.stubbedConvetedKey) - .should.equal(true) - return done() + 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 + ) + done() }) }) }) describe('getFile', function() { - beforeEach(function() { - this.handler._getStandardFile = sinon.stub().callsArgWith(3) - return (this.handler._getConvertedFile = sinon.stub().callsArgWith(3)) - }) - - it('should call _getStandardFile if no format or style are defined', function(done) { - return this.handler.getFile(this.bucket, this.key, null, () => { - this.handler._getStandardFile.called.should.equal(true) - this.handler._getConvertedFile.called.should.equal(false) - return done() + it('should return the source stream no format or style are defined', function(done) { + FileHandler.getFile(bucket, key, null, (err, stream) => { + expect(err).not.to.exist + expect(stream).to.equal(sourceStream) + done() }) }) - it('should pass options to _getStandardFile', function(done) { + it('should pass options through to PersistorManager', function(done) { const options = { start: 0, end: 8 } - return this.handler.getFile(this.bucket, this.key, options, () => { - expect(this.handler._getStandardFile.lastCall.args[2].start).to.equal(0) - expect(this.handler._getStandardFile.lastCall.args[2].end).to.equal(8) - return done() + FileHandler.getFile(bucket, key, options, err => { + expect(err).not.to.exist + expect(PersistorManager.getFileStream).to.have.been.calledWith( + bucket, + key, + options + ) + done() }) }) - return it('should call _getConvertedFile if a format is defined', function(done) { - return this.handler.getFile( - this.bucket, - this.key, - { format: 'png' }, - () => { - this.handler._getStandardFile.called.should.equal(false) - this.handler._getConvertedFile.called.should.equal(true) - return done() - } - ) - }) - }) + describe('when a format is defined', function() { + let result - describe('_getStandardFile', function() { - beforeEach(function() { - this.fileStream = { on() {} } - return this.PersistorManager.getFileStream.callsArgWith( - 3, - 'err', - this.fileStream - ) - }) + describe('when the file is not cached', function() { + beforeEach(function(done) { + FileHandler.getFile(bucket, key, { format: 'png' }, (err, stream) => { + result = { err, stream } + done() + }) + }) - it('should get the stream', function(done) { - return this.handler.getFile(this.bucket, this.key, null, () => { - this.PersistorManager.getFileStream - .calledWith(this.bucket, this.key) - .should.equal(true) - return done() + it('should convert the file', function() { + expect(FileConverter.convert).to.have.been.called + expect(ImageOptimiser.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 + ) + }) + }) + + describe('when the file is cached', function() { + beforeEach(function(done) { + PersistorManager.checkIfFileExists = sinon.stub().yields(null, true) + FileHandler.getFile(bucket, key, { format: 'png' }, (err, stream) => { + result = { err, stream } + done() + }) + }) + + it('should not convert the file', function() { + expect(FileConverter.convert).not.to.have.been.called + expect(ImageOptimiser.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 + ) + }) }) }) - it('should return the stream and error', function(done) { - return this.handler.getFile( - this.bucket, - this.key, - null, - (err, stream) => { - err.should.equal('err') - stream.should.equal(this.fileStream) - return done() - } - ) - }) - - return it('should pass options to PersistorManager', function(done) { - return this.handler.getFile( - this.bucket, - this.key, - { start: 0, end: 8 }, - () => { - expect( - this.PersistorManager.getFileStream.lastCall.args[2].start - ).to.equal(0) - expect( - this.PersistorManager.getFileStream.lastCall.args[2].end - ).to.equal(8) - return done() - } - ) - }) - }) - - describe('_getConvertedFile', function() { - it('should getFileStream if it does exists', function(done) { - this.PersistorManager.checkIfFileExists.callsArgWith(2, null, true) - this.PersistorManager.getFileStream.callsArgWith(3) - return this.handler._getConvertedFile(this.bucket, this.key, {}, () => { - this.PersistorManager.getFileStream - .calledWith(this.bucket) - .should.equal(true) - return done() + describe('when a style is defined', 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 + done() + }) }) - }) - return it('should call _getConvertedFileAndCache if it does exists', function(done) { - this.PersistorManager.checkIfFileExists.callsArgWith(2, null, false) - this.handler._getConvertedFileAndCache = sinon.stub().callsArgWith(4) - return this.handler._getConvertedFile(this.bucket, this.key, {}, () => { - this.handler._getConvertedFileAndCache - .calledWith(this.bucket, this.key) - .should.equal(true) - return done() + 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 + done() + }) }) }) }) - describe('_getConvertedFileAndCache', () => - it('should _convertFile ', function(done) { - this.stubbedStream = { something: 'here' } - this.localStream = { - on() {} - } - this.PersistorManager.sendFile = sinon.stub().callsArgWith(3) - this.LocalFileWriter.getStream = sinon - .stub() - .callsArgWith(1, null, this.localStream) - this.convetedKey = this.key + 'converted' - this.handler._convertFile = sinon - .stub() - .callsArgWith(3, null, this.stubbedPath) - this.ImageOptimiser.compressPng = sinon.stub().callsArgWith(1) - return this.handler._getConvertedFileAndCache( - this.bucket, - this.key, - this.convetedKey, - {}, - (err, fsStream) => { - this.handler._convertFile.called.should.equal(true) - this.PersistorManager.sendFile - .calledWith(this.bucket, this.convetedKey, this.stubbedPath) - .should.equal(true) - this.ImageOptimiser.compressPng - .calledWith(this.stubbedPath) - .should.equal(true) - this.LocalFileWriter.getStream - .calledWith(this.stubbedPath) - .should.equal(true) - fsStream.should.equal(this.localStream) - return done() - } - ) - })) - - describe('_convertFile', function() { - beforeEach(function() { - this.FileConverter.convert.callsArgWith( - 2, - null, - this.formattedStubbedPath - ) - this.FileConverter.thumbnail.callsArgWith( - 1, - null, - this.formattedStubbedPath - ) - this.FileConverter.preview.callsArgWith( - 1, - null, - this.formattedStubbedPath - ) - this.handler._writeS3FileToDisk = sinon - .stub() - .callsArgWith(3, null, this.stubbedPath) - return this.LocalFileWriter.deleteFile.callsArgWith(1) - }) - - it('should call thumbnail on the writer path if style was thumbnail was specified', function(done) { - return this.handler._convertFile( - this.bucket, - this.key, - { style: 'thumbnail' }, - (err, path) => { - path.should.equal(this.formattedStubbedPath) - this.FileConverter.thumbnail - .calledWith(this.stubbedPath) - .should.equal(true) - this.LocalFileWriter.deleteFile - .calledWith(this.stubbedPath) - .should.equal(true) - return done() - } - ) - }) - - it('should call preview on the writer path if style was preview was specified', function(done) { - return this.handler._convertFile( - this.bucket, - this.key, - { style: 'preview' }, - (err, path) => { - path.should.equal(this.formattedStubbedPath) - this.FileConverter.preview - .calledWith(this.stubbedPath) - .should.equal(true) - this.LocalFileWriter.deleteFile - .calledWith(this.stubbedPath) - .should.equal(true) - return done() - } - ) - }) - - return it('should call convert on the writer path if a format was specified', function(done) { - return this.handler._convertFile( - this.bucket, - this.key, - { format: this.format }, - (err, path) => { - path.should.equal(this.formattedStubbedPath) - this.FileConverter.convert - .calledWith(this.stubbedPath, this.format) - .should.equal(true) - this.LocalFileWriter.deleteFile - .calledWith(this.stubbedPath) - .should.equal(true) - return done() - } - ) - }) - }) - - return describe('getDirectorySize', function() { - beforeEach(function() { - return this.PersistorManager.directorySize.callsArgWith(2) - }) - - return it('should call the filestore manager to get directory size', function(done) { - return this.handler.getDirectorySize(this.bucket, this.key, () => { - this.PersistorManager.directorySize - .calledWith(this.bucket, this.key) - .should.equal(true) - return done() + describe('getDirectorySize', 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( + bucket, + key + ) + done() }) }) }) diff --git a/services/filestore/test/unit/js/LocalFileWriterTests.js b/services/filestore/test/unit/js/LocalFileWriterTests.js index 04cc2fb049..5d7008a91f 100644 --- a/services/filestore/test/unit/js/LocalFileWriterTests.js +++ b/services/filestore/test/unit/js/LocalFileWriterTests.js @@ -1,120 +1,79 @@ -/* eslint-disable - handle-callback-err, - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ - -const { assert } = require('chai') const sinon = require('sinon') const chai = require('chai') -const should = chai.should() const { expect } = chai const modulePath = '../../../app/js/LocalFileWriter.js' const SandboxedModule = require('sandboxed-module') +chai.use(require('sinon-chai')) describe('LocalFileWriter', function() { + const writeStream = 'writeStream' + const readStream = 'readStream' + const settings = { path: { uploadFolder: '/uploads' } } + const fsPath = '/uploads/wombat' + const filename = 'wombat' + let stream, fs, LocalFileWriter + beforeEach(function() { - this.writeStream = { - on(type, cb) { - if (type === 'finish') { - return cb() - } - } + fs = { + createWriteStream: sinon.stub().returns(writeStream), + unlink: sinon.stub().yields() } - this.readStream = { on() {} } - this.fs = { - createWriteStream: sinon.stub().returns(this.writeStream), - createReadStream: sinon.stub().returns(this.readStream), - unlink: sinon.stub() + stream = { + pipeline: sinon.stub().yields() } - this.settings = { - path: { - uploadFolder: 'somewhere' - } - } - this.writer = SandboxedModule.require(modulePath, { + + LocalFileWriter = SandboxedModule.require(modulePath, { requires: { - fs: this.fs, + fs, + stream, 'logger-sharelatex': { log() {}, err() {} }, - 'settings-sharelatex': this.settings, + 'settings-sharelatex': settings, 'metrics-sharelatex': { inc: sinon.stub(), Timer: sinon.stub().returns({ done: sinon.stub() }) } } }) - - return (this.stubbedFsPath = 'something/uploads/eio2k1j3') }) - describe('writeStrem', function() { - beforeEach(function() { - return (this.writer._getPath = sinon.stub().returns(this.stubbedFsPath)) - }) - - it('write the stream to ./uploads', function(done) { - const stream = { - pipe: dest => { - dest.should.equal(this.writeStream) - return done() - }, - on() {} - } - return this.writer.writeStream(stream, null, () => {}) - }) - - return it('should send the path in the callback', function(done) { - const stream = { - pipe: dest => {}, - on(type, cb) { - if (type === 'end') { - return cb() - } - } - } - return this.writer.writeStream(stream, null, (err, fsPath) => { - fsPath.should.equal(this.stubbedFsPath) - return done() + describe('writeStream', function() { + it('writes the stream to the upload folder', function(done) { + LocalFileWriter.writeStream(readStream, filename, (err, path) => { + expect(err).not.to.exist + expect(fs.createWriteStream).to.have.been.calledWith(fsPath) + expect(stream.pipeline).to.have.been.calledWith(readStream, writeStream) + expect(path).to.equal(fsPath) + done() }) }) }) - describe('getStream', function() { - it('should read the stream from the file ', function(done) { - return this.writer.getStream(this.stubbedFsPath, (err, stream) => { - this.fs.createReadStream - .calledWith(this.stubbedFsPath) - .should.equal(true) - return done() - }) - }) - - return it('should send the stream in the callback', function(done) { - return this.writer.getStream(this.stubbedFsPath, (err, readStream) => { - readStream.should.equal(this.readStream) - return done() - }) - }) - }) - - return describe('delete file', () => + describe('deleteFile', function() { it('should unlink the file', function(done) { - const error = 'my error' - this.fs.unlink.callsArgWith(1, error) - return this.writer.deleteFile(this.stubbedFsPath, err => { - this.fs.unlink.calledWith(this.stubbedFsPath).should.equal(true) - err.should.equal(error) - return done() + LocalFileWriter.deleteFile(fsPath, err => { + expect(err).not.to.exist + expect(fs.unlink).to.have.been.calledWith(fsPath) + done() }) - })) + }) + + it('should not do anything if called with an empty path', function(done) { + fs.unlink = sinon.stub().yields(new Error('failed to reticulate splines')) + LocalFileWriter.deleteFile(fsPath, err => { + expect(err).to.exist + done() + }) + }) + + it('should not call unlink with an empty path', function(done) { + LocalFileWriter.deleteFile('', err => { + expect(err).not.to.exist + expect(fs.unlink).not.to.have.been.called + done() + }) + }) + }) })