diff --git a/services/filestore/app/js/Errors.js b/services/filestore/app/js/Errors.js index 1beefb79c8..d2ba18c328 100644 --- a/services/filestore/app/js/Errors.js +++ b/services/filestore/app/js/Errors.js @@ -1,40 +1,20 @@ const OError = require('@overleaf/o-error') -// Error class for legacy errors so they inherit OError while staying -// backward-compatible (can be instantiated with string as argument instead -// of object) -class BackwardCompatibleError extends OError { - constructor(messageOrOptions) { - let options - if (typeof messageOrOptions === 'string') { - options = { message: messageOrOptions } - } else if (!messageOrOptions) { - options = {} - } else { - options = messageOrOptions - } - super(options) - } -} - -class NotFoundError extends BackwardCompatibleError {} -class WriteError extends BackwardCompatibleError {} -class ReadError extends BackwardCompatibleError {} -class HealthCheckError extends BackwardCompatibleError {} -class ConversionsDisabledError extends BackwardCompatibleError {} -class ConversionError extends BackwardCompatibleError {} -class SettingsError extends BackwardCompatibleError {} -class TimeoutError extends BackwardCompatibleError {} -class InvalidParametersError extends BackwardCompatibleError {} +class NotFoundError extends OError {} +class WriteError extends OError {} +class ReadError extends OError {} +class HealthCheckError extends OError {} +class ConversionsDisabledError extends OError {} +class ConversionError extends OError {} +class SettingsError extends OError {} +class TimeoutError extends OError {} +class InvalidParametersError extends OError {} class FailedCommandError extends OError { constructor(command, code, stdout, stderr) { - super({ - message: 'command failed with error exit code', - info: { - command, - code - } + super('command failed with error exit code', { + command, + code }) this.stdout = stdout this.stderr = stderr diff --git a/services/filestore/app/js/FSPersistor.js b/services/filestore/app/js/FSPersistor.js index 2e3b4985cb..60ee0f4053 100644 --- a/services/filestore/app/js/FSPersistor.js +++ b/services/filestore/app/js/FSPersistor.js @@ -46,9 +46,11 @@ async function sendStream(location, target, sourceStream, sourceMd5) { const destMd5 = await getFileMd5Hash(location, target) if (sourceMd5 !== destMd5) { await LocalFileWriter.deleteFile(`${location}/${filterName(target)}`) - throw new WriteError({ - message: 'md5 hash mismatch', - info: { sourceMd5, destMd5, location, target } + throw new WriteError('md5 hash mismatch', { + sourceMd5, + destMd5, + location, + target }) } } finally { @@ -100,9 +102,9 @@ async function getFileMd5Hash(location, filename) { try { return await _getFileMd5HashForPath(fullPath) } catch (err) { - throw new ReadError({ - message: 'unable to get md5 hash from file', - info: { location, filename } + throw new ReadError('unable to get md5 hash from file', { + location, + filename }).withCause(err) } } diff --git a/services/filestore/app/js/FileController.js b/services/filestore/app/js/FileController.js index d72b4c841c..9e978c6a8b 100644 --- a/services/filestore/app/js/FileController.js +++ b/services/filestore/app/js/FileController.js @@ -75,9 +75,11 @@ function getFile(req, res, next) { res.end() } else if (err) { next( - new Errors.ReadError({ - message: 'error transferring stream', - info: { bucket, key, format, style } + new Errors.ReadError('error transferring stream', { + bucket, + key, + format, + style }).withCause(err) ) } diff --git a/services/filestore/app/js/FileConverter.js b/services/filestore/app/js/FileConverter.js index 5ef42cc493..aec8e3bb3a 100644 --- a/services/filestore/app/js/FileConverter.js +++ b/services/filestore/app/js/FileConverter.js @@ -69,9 +69,8 @@ async function preview(sourcePath) { async function _convert(sourcePath, requestedFormat, command) { if (!APPROVED_FORMATS.includes(requestedFormat)) { - throw new ConversionError({ - message: 'invalid format requested', - info: { format: requestedFormat } + throw new ConversionError('invalid format requested', { + format: requestedFormat }) } @@ -87,9 +86,11 @@ async function _convert(sourcePath, requestedFormat, command) { timeout: FOURTY_SECONDS }) } catch (err) { - throw new ConversionError({ - message: 'something went wrong converting file', - info: { stderr: err.stderr, sourcePath, requestedFormat, destPath } + throw new ConversionError('something went wrong converting file', { + stderr: err.stderr, + sourcePath, + requestedFormat, + destPath }).withCause(err) } diff --git a/services/filestore/app/js/FileHandler.js b/services/filestore/app/js/FileHandler.js index e5933ffca7..e3e93b33dd 100644 --- a/services/filestore/app/js/FileHandler.js +++ b/services/filestore/app/js/FileHandler.js @@ -30,9 +30,10 @@ module.exports = { async function insertFile(bucket, key, stream) { const convertedKey = KeyBuilder.getConvertedFolderKey(key) if (!convertedKey.match(/^[0-9a-f]{24}\/([0-9a-f]{24}|v\/[0-9]+\/[a-z]+)/i)) { - throw new InvalidParametersError({ - message: 'key does not match validation regex', - info: { bucket, key, convertedKey } + throw new InvalidParametersError('key does not match validation regex', { + bucket, + key, + convertedKey }) } if (Settings.enableConversions) { @@ -44,9 +45,10 @@ async function insertFile(bucket, key, stream) { async function deleteFile(bucket, key) { const convertedKey = KeyBuilder.getConvertedFolderKey(key) if (!convertedKey.match(/^[0-9a-f]{24}\/([0-9a-f]{24}|v\/[0-9]+\/[a-z]+)/i)) { - throw new InvalidParametersError({ - message: 'key does not match validation regex', - info: { bucket, key, convertedKey } + throw new InvalidParametersError('key does not match validation regex', { + bucket, + key, + convertedKey }) } const jobs = [PersistorManager.promises.deleteFile(bucket, key)] @@ -58,9 +60,9 @@ async function deleteFile(bucket, key) { async function deleteProject(bucket, key) { if (!key.match(/^[0-9a-f]{24}\//i)) { - throw new InvalidParametersError({ - message: 'key does not match validation regex', - info: { bucket, key } + throw new InvalidParametersError('key does not match validation regex', { + bucket, + key }) } await PersistorManager.promises.deleteDirectory(bucket, key) @@ -126,9 +128,11 @@ async function _getConvertedFileAndCache(bucket, key, convertedKey, opts) { ) } catch (err) { LocalFileWriter.deleteFile(convertedFsPath, () => {}) - throw new ConversionError({ - message: 'failed to convert file', - info: { opts, bucket, key, convertedKey } + throw new ConversionError('failed to convert file', { + opts, + bucket, + key, + convertedKey }).withCause(err) } // Send back the converted file from the local copy to avoid problems @@ -155,9 +159,10 @@ async function _convertFile(bucket, originalKey, opts) { try { originalFsPath = await _writeFileToDisk(bucket, originalKey, opts) } catch (err) { - throw new ConversionError({ - message: 'unable to write file to disk', - info: { bucket, originalKey, opts } + throw new ConversionError('unable to write file to disk', { + bucket, + originalKey, + opts }).withCause(err) } @@ -169,22 +174,20 @@ async function _convertFile(bucket, originalKey, opts) { } else if (opts.style === 'preview') { promise = FileConverter.promises.preview(originalFsPath) } else { - throw new ConversionError({ - message: 'invalid file conversion options', - info: { - bucket, - originalKey, - opts - } + throw new ConversionError('invalid file conversion options', { + bucket, + originalKey, + opts }) } let destPath try { destPath = await promise } catch (err) { - throw new ConversionError({ - message: 'error converting file', - info: { bucket, originalKey, opts } + throw new ConversionError('error converting file', { + bucket, + originalKey, + opts }).withCause(err) } LocalFileWriter.deleteFile(originalFsPath, function() {}) diff --git a/services/filestore/app/js/LocalFileWriter.js b/services/filestore/app/js/LocalFileWriter.js index 7af282a558..da5aeb7a3a 100644 --- a/services/filestore/app/js/LocalFileWriter.js +++ b/services/filestore/app/js/LocalFileWriter.js @@ -30,9 +30,9 @@ async function writeStream(stream, key) { } catch (err) { await deleteFile(fsPath) - throw new WriteError({ - message: 'problem writing file locally', - info: { err, fsPath } + throw new WriteError('problem writing file locally', { + err, + fsPath }).withCause(err) } } @@ -45,10 +45,7 @@ async function deleteFile(fsPath) { await promisify(fs.unlink)(fsPath) } catch (err) { if (err.code !== 'ENOENT') { - throw new WriteError({ - message: 'failed to delete file', - info: { fsPath } - }).withCause(err) + throw new WriteError('failed to delete file', { fsPath }).withCause(err) } } } diff --git a/services/filestore/app/js/MigrationPersistor.js b/services/filestore/app/js/MigrationPersistor.js index 347dd58726..2a9fe5d2a0 100644 --- a/services/filestore/app/js/MigrationPersistor.js +++ b/services/filestore/app/js/MigrationPersistor.js @@ -169,27 +169,27 @@ module.exports = function(primary, fallback) { await primary.promises.sendStream(destBucket, destKey, stream, sourceMd5) } catch (err) { - const error = new WriteError({ - message: 'unable to copy file to destination persistor', - info: { + const error = new WriteError( + 'unable to copy file to destination persistor', + { sourceBucket, destBucket, sourceKey, destKey } - }).withCause(err) + ).withCause(err) metrics.inc('fallback.copy.failure') try { await primary.promises.deleteFile(destBucket, destKey) } catch (err) { - error.info.cleanupError = new WriteError({ - message: 'unable to clean up destination copy artifact', - info: { + error.info.cleanupError = new WriteError( + 'unable to clean up destination copy artifact', + { destBucket, destKey } - }).withCause(err) + ).withCause(err) } logger.warn({ error }, 'failed to copy file from fallback') diff --git a/services/filestore/app/js/PersistorHelper.js b/services/filestore/app/js/PersistorHelper.js index 1c2512b690..1a836a2b09 100644 --- a/services/filestore/app/js/PersistorHelper.js +++ b/services/filestore/app/js/PersistorHelper.js @@ -81,14 +81,11 @@ async function verifyMd5(persistor, bucket, key, sourceMd5, destMd5 = null) { logger.warn(err, 'error deleting file for invalid upload') } - throw new WriteError({ - message: 'source and destination hashes do not match', - info: { - sourceMd5, - destMd5, - bucket, - key - } + throw new WriteError('source and destination hashes do not match', { + sourceMd5, + destMd5, + bucket, + key }) } } @@ -165,15 +162,9 @@ function wrapError(error, message, params, ErrorType) { ) || (error.response && error.response.statusCode === 404) ) { - return new NotFoundError({ - message: 'no such file', - info: params - }).withCause(error) + return new NotFoundError('no such file', params).withCause(error) } else { - return new ErrorType({ - message: message, - info: params - }).withCause(error) + return new ErrorType(message, params).withCause(error) } } diff --git a/services/filestore/app/js/S3Persistor.js b/services/filestore/app/js/S3Persistor.js index 7e9a66a0ab..f0df46f10d 100644 --- a/services/filestore/app/js/S3Persistor.js +++ b/services/filestore/app/js/S3Persistor.js @@ -322,10 +322,10 @@ function _getClientForBucket(bucket) { return _defaultClient } - throw new SettingsError({ - message: 'no bucket-specific or default credentials provided', - info: { bucket } - }) + throw new SettingsError( + 'no bucket-specific or default credentials provided', + { bucket } + ) } function _buildClientOptions(bucketCredentials) { diff --git a/services/filestore/app/js/SafeExec.js b/services/filestore/app/js/SafeExec.js index a9d1398441..5ee8e8830b 100644 --- a/services/filestore/app/js/SafeExec.js +++ b/services/filestore/app/js/SafeExec.js @@ -42,13 +42,10 @@ function safeExec(command, options, callback) { process.kill(-child.pid, options.killSignal || 'SIGTERM') } catch (error) { cleanup( - new FailedCommandError({ - message: 'failed to kill process after timeout', - info: { - command, - options, - pid: child.pid - } + new FailedCommandError('failed to kill process after timeout', { + command, + options, + pid: child.pid }) ) } diff --git a/services/filestore/package-lock.json b/services/filestore/package-lock.json index 835e106f18..4753c14cd4 100644 --- a/services/filestore/package-lock.json +++ b/services/filestore/package-lock.json @@ -903,9 +903,8 @@ } }, "@overleaf/o-error": { - "version": "2.1.0", - "resolved": "https://registry.npmjs.org/@overleaf/o-error/-/o-error-2.1.0.tgz", - "integrity": "sha512-Zd9sks9LrLw8ErHt/cXeWIkyxWAqNAvNGn7wIjLQJH6TTEEW835PWOhpch+hQwwWsTxWIx/JDj+IpZ3ouw925g==" + "version": "git://github.com/overleaf/o-error.git#14e515a195d6dbd3711c5a211a730752802d3b03", + "from": "git://github.com/overleaf/o-error.git#14e515a195d6dbd3711c5a211a730752802d3b03" }, "@protobufjs/aspromise": { "version": "1.1.2", @@ -3480,6 +3479,11 @@ "yn": "^3.1.1" }, "dependencies": { + "@overleaf/o-error": { + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/@overleaf/o-error/-/o-error-2.1.0.tgz", + "integrity": "sha512-Zd9sks9LrLw8ErHt/cXeWIkyxWAqNAvNGn7wIjLQJH6TTEEW835PWOhpch+hQwwWsTxWIx/JDj+IpZ3ouw925g==" + }, "qs": { "version": "6.5.2", "resolved": "https://registry.npmjs.org/qs/-/qs-6.5.2.tgz", diff --git a/services/filestore/package.json b/services/filestore/package.json index 8c06e83aac..5680bf33fb 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -21,7 +21,7 @@ }, "dependencies": { "@google-cloud/storage": "^4.7.0", - "@overleaf/o-error": "^2.1.0", + "@overleaf/o-error": "git://github.com/overleaf/o-error.git#14e515a195d6dbd3711c5a211a730752802d3b03", "aws-sdk": "^2.648.0", "body-parser": "^1.19.0", "express": "^4.17.1", diff --git a/services/filestore/test/unit/js/GcsPersistorTests.js b/services/filestore/test/unit/js/GcsPersistorTests.js index 68db78d8a8..1e886e3e0c 100644 --- a/services/filestore/test/unit/js/GcsPersistorTests.js +++ b/services/filestore/test/unit/js/GcsPersistorTests.js @@ -44,6 +44,7 @@ describe('GcsPersistorTests', function() { user_files: 'user_files' }, gcs: { + deleteConcurrency: 1, directoryKeyRegex: /^[0-9a-fA-F]{24}\/[0-9a-fA-F]{24}/ } } @@ -526,7 +527,6 @@ describe('GcsPersistorTests', function() { const directoryName = `${ObjectId()}/${ObjectId()}` describe('with valid parameters', function() { beforeEach(async function() { - console.log(key) return GcsPersistor.promises.deleteDirectory(bucket, directoryName) })