From f82177a46a3609f72029f10d38566886fcdaac70 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 10:41:16 +0100 Subject: [PATCH 01/14] [Errors] migrate to OError --- services/real-time/app/js/Errors.js | 8 ++++---- services/real-time/app/js/Router.js | 4 ++-- services/real-time/package.json | 1 + services/real-time/test/unit/js/WebApiManagerTests.js | 4 +++- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index bdda1b4c21..06bf33cf5d 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -1,8 +1,8 @@ -class CodedError extends Error { +const OError = require('@overleaf/o-error') + +class CodedError extends OError { constructor(message, code) { - super(message) - this.name = this.constructor.name - this.code = code + super(message, { code }) } } diff --git a/services/real-time/app/js/Router.js b/services/real-time/app/js/Router.js index ae10d12e2b..87c1991d4c 100644 --- a/services/real-time/app/js/Router.js +++ b/services/real-time/app/js/Router.js @@ -33,8 +33,8 @@ module.exports = Router = { attrs.client_id = client.id attrs.err = error if (error.name === 'CodedError') { - logger.warn(attrs, error.message, { code: error.code }) - const serializedError = { message: error.message, code: error.code } + logger.warn(attrs, error.message) + const serializedError = { message: error.message, code: error.info.code } callback(serializedError) } else if (error.message === 'unexpected arguments') { // the payload might be very large, put it on level info diff --git a/services/real-time/package.json b/services/real-time/package.json index 4a7acfc1cd..0fab05972f 100644 --- a/services/real-time/package.json +++ b/services/real-time/package.json @@ -19,6 +19,7 @@ "format:fix": "node_modules/.bin/prettier-eslint $PWD'/**/*.js' --write" }, "dependencies": { + "@overleaf/o-error": "^3.0.0", "async": "^0.9.0", "base64id": "0.1.0", "basic-auth-connect": "^1.0.0", diff --git a/services/real-time/test/unit/js/WebApiManagerTests.js b/services/real-time/test/unit/js/WebApiManagerTests.js index 2d792b563b..52c6da137a 100644 --- a/services/real-time/test/unit/js/WebApiManagerTests.js +++ b/services/real-time/test/unit/js/WebApiManagerTests.js @@ -152,7 +152,9 @@ describe('WebApiManager', function () { .calledWith( sinon.match({ message: 'rate-limit hit when joining project', - code: 'TooManyRequests' + info: { + code: 'TooManyRequests' + } }) ) .should.equal(true) From 5950b26a42bc8e977d77eaaf054f2f907810ddca Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 11:03:34 +0100 Subject: [PATCH 02/14] [SafeJsonParse] migrate to OError and use a new DataTooLargeToParseError --- services/real-time/app/js/Errors.js | 11 ++++++++++- services/real-time/app/js/SafeJsonParse.js | 8 ++------ services/real-time/test/unit/js/SafeJsonParseTest.js | 2 +- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index 06bf33cf5d..12c644c839 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -6,4 +6,13 @@ class CodedError extends OError { } } -module.exports = { CodedError } +class DataTooLargeToParseError extends OError { + constructor(data) { + super('data too large to parse', { + head: data.slice(0, 1024), + length: data.length + }) + } +} + +module.exports = { CodedError, DataTooLargeToParseError } diff --git a/services/real-time/app/js/SafeJsonParse.js b/services/real-time/app/js/SafeJsonParse.js index 982fd9d424..a8a3afae4d 100644 --- a/services/real-time/app/js/SafeJsonParse.js +++ b/services/real-time/app/js/SafeJsonParse.js @@ -1,14 +1,10 @@ const Settings = require('settings-sharelatex') -const logger = require('logger-sharelatex') +const { DataTooLargeToParseError } = require('./Errors') module.exports = { parse(data, callback) { if (data.length > Settings.maxUpdateSize) { - logger.error( - { head: data.slice(0, 1024), length: data.length }, - 'data too large to parse' - ) - return callback(new Error('data too large to parse')) + return callback(new DataTooLargeToParseError(data)) } let parsed try { diff --git a/services/real-time/test/unit/js/SafeJsonParseTest.js b/services/real-time/test/unit/js/SafeJsonParseTest.js index 4fb558a6b0..51a60dea23 100644 --- a/services/real-time/test/unit/js/SafeJsonParseTest.js +++ b/services/real-time/test/unit/js/SafeJsonParseTest.js @@ -50,7 +50,7 @@ describe('SafeJsonParse', function () { const data = `{\"foo\": \"${big_blob}\"}` this.Settings.maxUpdateSize = 2 * 1024 return this.SafeJsonParse.parse(data, (error, parsed) => { - this.logger.error.called.should.equal(true) + this.logger.error.called.should.equal(false) expect(error).to.exist return done() }) From af50f9b02c768dbd7a766916ade7f5d1c4eaf54e Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 11:16:26 +0100 Subject: [PATCH 03/14] [DocumentUpdaterManager] use a new UpdateTooLargeError --- services/real-time/app/js/DocumentUpdaterManager.js | 5 ++--- services/real-time/app/js/Errors.js | 8 +++++++- services/real-time/app/js/WebsocketController.js | 2 +- .../real-time/test/unit/js/DocumentUpdaterManagerTests.js | 2 +- .../real-time/test/unit/js/WebsocketControllerTests.js | 4 ++-- 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/services/real-time/app/js/DocumentUpdaterManager.js b/services/real-time/app/js/DocumentUpdaterManager.js index 3fd09d7b1d..8a2ebda1f9 100644 --- a/services/real-time/app/js/DocumentUpdaterManager.js +++ b/services/real-time/app/js/DocumentUpdaterManager.js @@ -6,6 +6,7 @@ const _ = require('underscore') const logger = require('logger-sharelatex') const settings = require('settings-sharelatex') const metrics = require('metrics-sharelatex') +const { UpdateTooLargeError } = require('./Errors') const rclient = require('redis-sharelatex').createClient( settings.redis.documentupdater @@ -125,9 +126,7 @@ const DocumentUpdaterManager = { const updateSize = jsonChange.length if (updateSize > settings.maxUpdateSize) { - const error = new Error('update is too large') - error.updateSize = updateSize - return callback(error) + return callback(new UpdateTooLargeError(updateSize)) } // record metric for each update added to queue diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index 12c644c839..56034023a0 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -15,4 +15,10 @@ class DataTooLargeToParseError extends OError { } } -module.exports = { CodedError, DataTooLargeToParseError } +class UpdateTooLargeError extends OError { + constructor(updateSize) { + super('update is too large', { updateSize }) + } +} + +module.exports = { CodedError, DataTooLargeToParseError, UpdateTooLargeError } diff --git a/services/real-time/app/js/WebsocketController.js b/services/real-time/app/js/WebsocketController.js index 8867320dbe..a0c33f5cbe 100644 --- a/services/real-time/app/js/WebsocketController.js +++ b/services/real-time/app/js/WebsocketController.js @@ -509,7 +509,7 @@ module.exports = WebsocketController = { function (error) { if ((error && error.message) === 'update is too large') { metrics.inc('update_too_large') - const { updateSize } = error + const { updateSize } = error.info logger.warn( { user_id, project_id, doc_id, updateSize }, 'update is too large' diff --git a/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js b/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js index dc42b52140..2ded504051 100644 --- a/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js +++ b/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js @@ -346,7 +346,7 @@ describe('DocumentUpdaterManager', function () { }) it('should add the size to the error', function () { - return this.callback.args[0][0].updateSize.should.equal(7782422) + return this.callback.args[0][0].info.updateSize.should.equal(7782422) }) return it('should not push the change onto the pending-updates-list queue', function () { diff --git a/services/real-time/test/unit/js/WebsocketControllerTests.js b/services/real-time/test/unit/js/WebsocketControllerTests.js index 515d407cc2..7c42dd6256 100644 --- a/services/real-time/test/unit/js/WebsocketControllerTests.js +++ b/services/real-time/test/unit/js/WebsocketControllerTests.js @@ -19,6 +19,7 @@ const { expect } = chai const modulePath = '../../../app/js/WebsocketController.js' const SandboxedModule = require('sandboxed-module') const tk = require('timekeeper') +const { UpdateTooLargeError } = require('../../../app/js/Errors') describe('WebsocketController', function () { beforeEach(function () { @@ -1507,8 +1508,7 @@ describe('WebsocketController', function () { this.client.emit = sinon.stub() this.client.ol_context.user_id = this.user_id this.client.ol_context.project_id = this.project_id - const error = new Error('update is too large') - error.updateSize = 7372835 + const error = new UpdateTooLargeError(7372835) this.DocumentUpdaterManager.queueChange = sinon .stub() .callsArgWith(3, error) From 6828becb46bcbdf1d9ce13a63f6a978e70b93774 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 11:30:22 +0100 Subject: [PATCH 04/14] [DocumentUpdaterManager] use a new NullBytesInOpError --- services/real-time/app/js/DocumentUpdaterManager.js | 9 ++------- services/real-time/app/js/Errors.js | 13 ++++++++++++- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/services/real-time/app/js/DocumentUpdaterManager.js b/services/real-time/app/js/DocumentUpdaterManager.js index 8a2ebda1f9..167bb000a4 100644 --- a/services/real-time/app/js/DocumentUpdaterManager.js +++ b/services/real-time/app/js/DocumentUpdaterManager.js @@ -6,7 +6,7 @@ const _ = require('underscore') const logger = require('logger-sharelatex') const settings = require('settings-sharelatex') const metrics = require('metrics-sharelatex') -const { UpdateTooLargeError } = require('./Errors') +const { NullBytesInOpError, UpdateTooLargeError } = require('./Errors') const rclient = require('redis-sharelatex').createClient( settings.redis.documentupdater @@ -116,12 +116,7 @@ const DocumentUpdaterManager = { const jsonChange = JSON.stringify(change) if (jsonChange.indexOf('\u0000') !== -1) { // memory corruption check - const error = new Error('null bytes found in op') - logger.error( - { err: error, project_id, doc_id, jsonChange }, - error.message - ) - return callback(error) + return callback(new NullBytesInOpError(jsonChange)) } const updateSize = jsonChange.length diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index 56034023a0..dc90af856a 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -15,10 +15,21 @@ class DataTooLargeToParseError extends OError { } } +class NullBytesInOpError extends OError { + constructor(jsonChange) { + super('null bytes found in op', { jsonChange }) + } +} + class UpdateTooLargeError extends OError { constructor(updateSize) { super('update is too large', { updateSize }) } } -module.exports = { CodedError, DataTooLargeToParseError, UpdateTooLargeError } +module.exports = { + CodedError, + DataTooLargeToParseError, + NullBytesInOpError, + UpdateTooLargeError +} From de518ea4eb27a96accc12c69dc11d6cbf5449b57 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 11:38:10 +0100 Subject: [PATCH 05/14] [SessionSockets] use a new MissingSessionError --- services/real-time/app/js/Errors.js | 7 +++++++ services/real-time/app/js/SessionSockets.js | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index dc90af856a..7a039c54c1 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -15,6 +15,12 @@ class DataTooLargeToParseError extends OError { } } +class MissingSessionError extends OError { + constructor() { + super('could not look up session by key') + } +} + class NullBytesInOpError extends OError { constructor(jsonChange) { super('null bytes found in op', { jsonChange }) @@ -30,6 +36,7 @@ class UpdateTooLargeError extends OError { module.exports = { CodedError, DataTooLargeToParseError, + MissingSessionError, NullBytesInOpError, UpdateTooLargeError } diff --git a/services/real-time/app/js/SessionSockets.js b/services/real-time/app/js/SessionSockets.js index 4ade959829..84f87bf872 100644 --- a/services/real-time/app/js/SessionSockets.js +++ b/services/real-time/app/js/SessionSockets.js @@ -1,7 +1,8 @@ const { EventEmitter } = require('events') +const { MissingSessionError } = require('./Errors') module.exports = function (io, sessionStore, cookieParser, cookieName) { - const missingSessionError = new Error('could not look up session by key') + const missingSessionError = new MissingSessionError() const sessionSockets = new EventEmitter() function next(error, socket, session) { From a8c51de5105307bb19c8ffc2a8edbfbde6053ae3 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 11:50:35 +0100 Subject: [PATCH 06/14] [AuthorizationManager] use a new NotAuthorizedError --- services/real-time/app/js/AuthorizationManager.js | 6 ++++-- services/real-time/app/js/Errors.js | 7 +++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/services/real-time/app/js/AuthorizationManager.js b/services/real-time/app/js/AuthorizationManager.js index a545594479..0c37ac1423 100644 --- a/services/real-time/app/js/AuthorizationManager.js +++ b/services/real-time/app/js/AuthorizationManager.js @@ -1,6 +1,8 @@ /* eslint-disable camelcase, */ +const { NotAuthorizedError } = require('./Errors') + let AuthorizationManager module.exports = AuthorizationManager = { assertClientCanViewProject(client, callback) { @@ -23,7 +25,7 @@ module.exports = AuthorizationManager = { if (allowedLevels.includes(client.ol_context.privilege_level)) { callback(null) } else { - callback(new Error('not authorized')) + callback(new NotAuthorizedError()) } }, @@ -49,7 +51,7 @@ module.exports = AuthorizationManager = { if (client.ol_context[`doc:${doc_id}`] === 'allowed') { callback(null) } else { - callback(new Error('not authorized')) + callback(new NotAuthorizedError()) } }, diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index 7a039c54c1..57190b75e5 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -21,6 +21,12 @@ class MissingSessionError extends OError { } } +class NotAuthorizedError extends OError { + constructor() { + super('not authorized') + } +} + class NullBytesInOpError extends OError { constructor(jsonChange) { super('null bytes found in op', { jsonChange }) @@ -37,6 +43,7 @@ module.exports = { CodedError, DataTooLargeToParseError, MissingSessionError, + NotAuthorizedError, NullBytesInOpError, UpdateTooLargeError } From 59c4c884a5a84e8eca772b787e85c710876d9e78 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 11:50:43 +0100 Subject: [PATCH 07/14] [WebsocketController] use the new NotAuthorizedError --- services/real-time/app/js/WebsocketController.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/services/real-time/app/js/WebsocketController.js b/services/real-time/app/js/WebsocketController.js index a0c33f5cbe..a695d02b96 100644 --- a/services/real-time/app/js/WebsocketController.js +++ b/services/real-time/app/js/WebsocketController.js @@ -9,6 +9,7 @@ const DocumentUpdaterManager = require('./DocumentUpdaterManager') const ConnectedUsersManager = require('./ConnectedUsersManager') const WebsocketLoadBalancer = require('./WebsocketLoadBalancer') const RoomManager = require('./RoomManager') +const { NotAuthorizedError } = require('./Errors') let WebsocketController module.exports = WebsocketController = { @@ -48,12 +49,7 @@ module.exports = WebsocketController = { } if (!privilegeLevel) { - const err = new Error('not authorized') - logger.warn( - { err, project_id, user_id, client_id: client.id }, - 'user is not authorized to join project' - ) - return callback(err) + return callback(new NotAuthorizedError()) } client.ol_context = {} From 68bc9d0d23ccc7c2580d1ede900b1c45e0f75677 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 12:03:54 +0100 Subject: [PATCH 08/14] [WebApiManager] use a new WebApiRequestFailedError --- services/real-time/app/js/Errors.js | 9 ++++++++- services/real-time/app/js/WebApiManager.js | 8 ++------ services/real-time/test/unit/js/WebApiManagerTests.js | 5 ++++- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index 57190b75e5..5b41334fb3 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -39,11 +39,18 @@ class UpdateTooLargeError extends OError { } } +class WebApiRequestFailedError extends OError { + constructor(statusCode) { + super('non-success status code from web', { statusCode }) + } +} + module.exports = { CodedError, DataTooLargeToParseError, MissingSessionError, NotAuthorizedError, NullBytesInOpError, - UpdateTooLargeError + UpdateTooLargeError, + WebApiRequestFailedError } diff --git a/services/real-time/app/js/WebApiManager.js b/services/real-time/app/js/WebApiManager.js index 62020a98ab..dffcca51c8 100644 --- a/services/real-time/app/js/WebApiManager.js +++ b/services/real-time/app/js/WebApiManager.js @@ -4,7 +4,7 @@ const request = require('request') const settings = require('settings-sharelatex') const logger = require('logger-sharelatex') -const { CodedError } = require('./Errors') +const { CodedError, WebApiRequestFailedError } = require('./Errors') module.exports = { joinProject(project_id, user, callback) { @@ -57,11 +57,7 @@ module.exports = { ) ) } else { - err = new Error( - `non-success status code from web: ${response.statusCode}` - ) - logger.error({ err, project_id, user_id }, 'error accessing web api') - callback(err) + callback(new WebApiRequestFailedError(response.statusCode)) } } ) diff --git a/services/real-time/test/unit/js/WebApiManagerTests.js b/services/real-time/test/unit/js/WebApiManagerTests.js index 52c6da137a..4435bc14f9 100644 --- a/services/real-time/test/unit/js/WebApiManagerTests.js +++ b/services/real-time/test/unit/js/WebApiManagerTests.js @@ -106,7 +106,10 @@ describe('WebApiManager', function () { return it('should call the callback with an error', function () { return this.callback .calledWith( - sinon.match({ message: 'non-success status code from web: 500' }) + sinon.match({ + message: 'non-success status code from web', + info: { statusCode: 500 } + }) ) .should.equal(true) }) From 02a23822647433906731501b41bd9fa310c9adba Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 12:14:02 +0100 Subject: [PATCH 09/14] [WebApiManager] use a new CorruptedJoinProjectResponseError --- services/real-time/app/js/Errors.js | 7 +++++++ services/real-time/app/js/WebApiManager.js | 14 ++++++-------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index 5b41334fb3..ebcf1d1d81 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -6,6 +6,12 @@ class CodedError extends OError { } } +class CorruptedJoinProjectResponseError extends OError { + constructor() { + super('no data returned from joinProject request') + } +} + class DataTooLargeToParseError extends OError { constructor(data) { super('data too large to parse', { @@ -47,6 +53,7 @@ class WebApiRequestFailedError extends OError { module.exports = { CodedError, + CorruptedJoinProjectResponseError, DataTooLargeToParseError, MissingSessionError, NotAuthorizedError, diff --git a/services/real-time/app/js/WebApiManager.js b/services/real-time/app/js/WebApiManager.js index dffcca51c8..a39d9dd832 100644 --- a/services/real-time/app/js/WebApiManager.js +++ b/services/real-time/app/js/WebApiManager.js @@ -4,7 +4,11 @@ const request = require('request') const settings = require('settings-sharelatex') const logger = require('logger-sharelatex') -const { CodedError, WebApiRequestFailedError } = require('./Errors') +const { + CodedError, + CorruptedJoinProjectResponseError, + WebApiRequestFailedError +} = require('./Errors') module.exports = { joinProject(project_id, user, callback) { @@ -32,15 +36,9 @@ module.exports = { if (error) { return callback(error) } - let err if (response.statusCode >= 200 && response.statusCode < 300) { if (!(data && data.project)) { - err = new Error('no data returned from joinProject request') - logger.error( - { err, project_id, user_id }, - 'error accessing web api' - ) - return callback(err) + return callback(new CorruptedJoinProjectResponseError()) } callback( null, From 8abfdb87ff48d59fe3fe71d95d9fd6b1dd5ce721 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 12:52:59 +0100 Subject: [PATCH 10/14] [DocumentUpdaterManager] use a new DocumentUpdaterRequestFailedError --- .../app/js/DocumentUpdaterManager.js | 29 +++++++-------- services/real-time/app/js/Errors.js | 10 ++++++ .../unit/js/DocumentUpdaterManagerTests.js | 36 ++++++++++++------- 3 files changed, 45 insertions(+), 30 deletions(-) diff --git a/services/real-time/app/js/DocumentUpdaterManager.js b/services/real-time/app/js/DocumentUpdaterManager.js index 167bb000a4..1b99ed7ab4 100644 --- a/services/real-time/app/js/DocumentUpdaterManager.js +++ b/services/real-time/app/js/DocumentUpdaterManager.js @@ -6,7 +6,11 @@ const _ = require('underscore') const logger = require('logger-sharelatex') const settings = require('settings-sharelatex') const metrics = require('metrics-sharelatex') -const { NullBytesInOpError, UpdateTooLargeError } = require('./Errors') +const { + DocumentUpdaterRequestFailedError, + NullBytesInOpError, + UpdateTooLargeError +} = require('./Errors') const rclient = require('redis-sharelatex').createClient( settings.redis.documentupdater @@ -51,15 +55,9 @@ const DocumentUpdaterManager = { ) callback(err) } else { - err = new Error( - `doc updater returned a non-success status code: ${res.statusCode}` + callback( + new DocumentUpdaterRequestFailedError('getDocument', res.statusCode) ) - err.statusCode = res.statusCode - logger.error( - { err, project_id, doc_id, url }, - `doc updater returned a non-success status code: ${res.statusCode}` - ) - callback(err) } }) }, @@ -89,15 +87,12 @@ const DocumentUpdaterManager = { logger.log({ project_id }, 'deleted project from document updater') callback(null) } else { - err = new Error( - `document updater returned a failure status code: ${res.statusCode}` + callback( + new DocumentUpdaterRequestFailedError( + 'flushProjectToMongoAndDelete', + res.statusCode + ) ) - err.statusCode = res.statusCode - logger.error( - { err, project_id }, - `document updater returned failure status code: ${res.statusCode}` - ) - callback(err) } }) }, diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index ebcf1d1d81..1e6ffbd4ef 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -21,6 +21,15 @@ class DataTooLargeToParseError extends OError { } } +class DocumentUpdaterRequestFailedError extends OError { + constructor(action, statusCode) { + super('doc updater returned a non-success status code', { + action, + statusCode + }) + } +} + class MissingSessionError extends OError { constructor() { super('could not look up session by key') @@ -55,6 +64,7 @@ module.exports = { CodedError, CorruptedJoinProjectResponseError, DataTooLargeToParseError, + DocumentUpdaterRequestFailedError, MissingSessionError, NotAuthorizedError, NullBytesInOpError, diff --git a/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js b/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js index 2ded504051..f5023b3e30 100644 --- a/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js +++ b/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js @@ -163,13 +163,18 @@ describe('DocumentUpdaterManager', function () { return it('should return the callback with an error', function () { this.callback.called.should.equal(true) - const err = this.callback.getCall(0).args[0] - err.should.have.property('statusCode', 500) - err.should.have.property( - 'message', - 'doc updater returned a non-success status code: 500' - ) - return this.logger.error.called.should.equal(true) + this.callback + .calledWith( + sinon.match({ + message: 'doc updater returned a non-success status code', + info: { + action: 'getDocument', + statusCode: 500 + } + }) + ) + .should.equal(true) + this.logger.error.called.should.equal(false) }) }) }) @@ -234,12 +239,17 @@ describe('DocumentUpdaterManager', function () { return it('should return the callback with an error', function () { this.callback.called.should.equal(true) - const err = this.callback.getCall(0).args[0] - err.should.have.property('statusCode', 500) - return err.should.have.property( - 'message', - 'document updater returned a failure status code: 500' - ) + this.callback + .calledWith( + sinon.match({ + message: 'doc updater returned a non-success status code', + info: { + action: 'flushProjectToMongoAndDelete', + statusCode: 500 + } + }) + ) + .should.equal(true) }) }) }) From 4cb8cc4a85935031de4c3bf5922247e2e46ae3f7 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 12:59:52 +0100 Subject: [PATCH 11/14] [DocumentUpdaterManager] use a new ClientRequestedMissingOpsError --- .../real-time/app/js/DocumentUpdaterManager.js | 9 ++------- services/real-time/app/js/Errors.js | 9 +++++++++ .../test/unit/js/DocumentUpdaterManagerTests.js | 16 +++++++++------- 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/services/real-time/app/js/DocumentUpdaterManager.js b/services/real-time/app/js/DocumentUpdaterManager.js index 1b99ed7ab4..67ecd82c29 100644 --- a/services/real-time/app/js/DocumentUpdaterManager.js +++ b/services/real-time/app/js/DocumentUpdaterManager.js @@ -7,6 +7,7 @@ const logger = require('logger-sharelatex') const settings = require('settings-sharelatex') const metrics = require('metrics-sharelatex') const { + ClientRequestedMissingOpsError, DocumentUpdaterRequestFailedError, NullBytesInOpError, UpdateTooLargeError @@ -47,13 +48,7 @@ const DocumentUpdaterManager = { body = body || {} callback(null, body.lines, body.version, body.ranges, body.ops) } else if ([404, 422].includes(res.statusCode)) { - err = new Error('doc updater could not load requested ops') - err.statusCode = res.statusCode - logger.warn( - { err, project_id, doc_id, url, fromVersion }, - 'doc updater could not load requested ops' - ) - callback(err) + callback(new ClientRequestedMissingOpsError(res.statusCode)) } else { callback( new DocumentUpdaterRequestFailedError('getDocument', res.statusCode) diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index 1e6ffbd4ef..24c0b3d316 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -1,5 +1,13 @@ const OError = require('@overleaf/o-error') +class ClientRequestedMissingOpsError extends OError { + constructor(statusCode) { + super('doc updater could not load requested ops', { + statusCode + }) + } +} + class CodedError extends OError { constructor(message, code) { super(message, { code }) @@ -63,6 +71,7 @@ class WebApiRequestFailedError extends OError { module.exports = { CodedError, CorruptedJoinProjectResponseError, + ClientRequestedMissingOpsError, DataTooLargeToParseError, DocumentUpdaterRequestFailedError, MissingSessionError, diff --git a/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js b/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js index f5023b3e30..828f516364 100644 --- a/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js +++ b/services/real-time/test/unit/js/DocumentUpdaterManagerTests.js @@ -136,14 +136,16 @@ describe('DocumentUpdaterManager', function () { return it('should return the callback with an error', function () { this.callback.called.should.equal(true) - const err = this.callback.getCall(0).args[0] - err.should.have.property('statusCode', statusCode) - err.should.have.property( - 'message', - 'doc updater could not load requested ops' - ) + this.callback + .calledWith( + sinon.match({ + message: 'doc updater could not load requested ops', + info: { statusCode } + }) + ) + .should.equal(true) this.logger.error.called.should.equal(false) - return this.logger.warn.called.should.equal(true) + this.logger.warn.called.should.equal(false) }) }) ) From 0462e3e4371156e7ebd7db940141406475764ac0 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 13:07:27 +0100 Subject: [PATCH 12/14] [WebsocketController] use a new NotJoinedError --- services/real-time/app/js/Errors.js | 7 +++++++ services/real-time/app/js/WebsocketController.js | 8 ++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index 24c0b3d316..d684764b0c 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -50,6 +50,12 @@ class NotAuthorizedError extends OError { } } +class NotJoinedError extends OError { + constructor() { + super('no project_id found on client') + } +} + class NullBytesInOpError extends OError { constructor(jsonChange) { super('null bytes found in op', { jsonChange }) @@ -76,6 +82,7 @@ module.exports = { DocumentUpdaterRequestFailedError, MissingSessionError, NotAuthorizedError, + NotJoinedError, NullBytesInOpError, UpdateTooLargeError, WebApiRequestFailedError diff --git a/services/real-time/app/js/WebsocketController.js b/services/real-time/app/js/WebsocketController.js index a695d02b96..b099d8e4e4 100644 --- a/services/real-time/app/js/WebsocketController.js +++ b/services/real-time/app/js/WebsocketController.js @@ -9,7 +9,7 @@ const DocumentUpdaterManager = require('./DocumentUpdaterManager') const ConnectedUsersManager = require('./ConnectedUsersManager') const WebsocketLoadBalancer = require('./WebsocketLoadBalancer') const RoomManager = require('./RoomManager') -const { NotAuthorizedError } = require('./Errors') +const { NotAuthorizedError, NotJoinedError } = require('./Errors') let WebsocketController module.exports = WebsocketController = { @@ -158,7 +158,7 @@ module.exports = WebsocketController = { metrics.inc('editor.join-doc') const { project_id, user_id, is_restricted_user } = client.ol_context if (!project_id) { - return callback(new Error('no project_id found on client')) + return callback(new NotJoinedError()) } logger.log( { user_id, project_id, doc_id, fromVersion, client_id: client.id }, @@ -424,7 +424,7 @@ module.exports = WebsocketController = { return callback(null, []) } if (!project_id) { - return callback(new Error('no project_id found on client')) + return callback(new NotJoinedError()) } logger.log( { user_id, project_id, client_id: client.id }, @@ -459,7 +459,7 @@ module.exports = WebsocketController = { // client may have disconnected, but we can submit their update to doc-updater anyways. const { user_id, project_id } = client.ol_context if (!project_id) { - return callback(new Error('no project_id found on client')) + return callback(new NotJoinedError()) } WebsocketController._assertClientCanApplyUpdate( From 50140f785a2fbfb5e78b524b5261db95994e931a Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 13:10:22 +0100 Subject: [PATCH 13/14] [WebsocketController] use a new JoinLeaveEpochMismatchError --- services/real-time/app/js/Errors.js | 7 +++++++ services/real-time/app/js/WebsocketController.js | 8 ++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index d684764b0c..c5c082bfac 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -38,6 +38,12 @@ class DocumentUpdaterRequestFailedError extends OError { } } +class JoinLeaveEpochMismatchError extends OError { + constructor() { + super('joinLeaveEpoch mismatch') + } +} + class MissingSessionError extends OError { constructor() { super('could not look up session by key') @@ -80,6 +86,7 @@ module.exports = { ClientRequestedMissingOpsError, DataTooLargeToParseError, DocumentUpdaterRequestFailedError, + JoinLeaveEpochMismatchError, MissingSessionError, NotAuthorizedError, NotJoinedError, diff --git a/services/real-time/app/js/WebsocketController.js b/services/real-time/app/js/WebsocketController.js index b099d8e4e4..a443a49f95 100644 --- a/services/real-time/app/js/WebsocketController.js +++ b/services/real-time/app/js/WebsocketController.js @@ -9,7 +9,11 @@ const DocumentUpdaterManager = require('./DocumentUpdaterManager') const ConnectedUsersManager = require('./ConnectedUsersManager') const WebsocketLoadBalancer = require('./WebsocketLoadBalancer') const RoomManager = require('./RoomManager') -const { NotAuthorizedError, NotJoinedError } = require('./Errors') +const { + JoinLeaveEpochMismatchError, + NotAuthorizedError, + NotJoinedError +} = require('./Errors') let WebsocketController module.exports = WebsocketController = { @@ -180,7 +184,7 @@ module.exports = WebsocketController = { } if (joinLeaveEpoch !== client.joinLeaveEpoch) { // another joinDoc or leaveDoc rpc overtook us - return callback(new Error('joinLeaveEpoch mismatch')) + return callback(new JoinLeaveEpochMismatchError()) } // ensure the per-doc applied-ops channel is subscribed before sending the // doc to the client, so that no events are missed. From 880056d397137c26b5769c45ecfff6a6d9ba7e59 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 20 Aug 2020 13:12:56 +0100 Subject: [PATCH 14/14] [Router] use a new UnexpectedArgumentsError --- services/real-time/app/js/Errors.js | 7 +++++++ services/real-time/app/js/Router.js | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/services/real-time/app/js/Errors.js b/services/real-time/app/js/Errors.js index c5c082bfac..562c9888ab 100644 --- a/services/real-time/app/js/Errors.js +++ b/services/real-time/app/js/Errors.js @@ -68,6 +68,12 @@ class NullBytesInOpError extends OError { } } +class UnexpectedArgumentsError extends OError { + constructor() { + super('unexpected arguments') + } +} + class UpdateTooLargeError extends OError { constructor(updateSize) { super('update is too large', { updateSize }) @@ -91,6 +97,7 @@ module.exports = { NotAuthorizedError, NotJoinedError, NullBytesInOpError, + UnexpectedArgumentsError, UpdateTooLargeError, WebApiRequestFailedError } diff --git a/services/real-time/app/js/Router.js b/services/real-time/app/js/Router.js index 87c1991d4c..5e0ae4bf76 100644 --- a/services/real-time/app/js/Router.js +++ b/services/real-time/app/js/Router.js @@ -9,6 +9,7 @@ const HttpController = require('./HttpController') const HttpApiController = require('./HttpApiController') const bodyParser = require('body-parser') const base64id = require('base64id') +const { UnexpectedArgumentsError } = require('./Errors') const basicAuth = require('basic-auth-connect') const httpAuth = basicAuth(function (user, pass) { @@ -64,7 +65,7 @@ module.exports = Router = { }, _handleInvalidArguments(client, method, args) { - const error = new Error('unexpected arguments') + const error = new UnexpectedArgumentsError() let callback = args[args.length - 1] if (typeof callback !== 'function') { callback = function () {}