From b1e8cb9cf01fecb9eabc828090b0894844a4d123 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Mon, 1 Jul 2019 15:54:23 +0200 Subject: [PATCH] Merge pull request #1924 from overleaf/ta-full-error Don't Callback with String GitOrigin-RevId: 82e3efb055ef197d95ff9c8a876bee0d6a0327a3 --- .../Announcements/AnnouncementsHandler.js | 2 +- .../web/app/src/Features/Blog/BlogHandler.js | 2 +- .../Features/Editor/EditorHttpController.js | 12 ++-- .../web/app/src/Features/Email/EmailSender.js | 2 +- .../ProjectEntityMongoUpdateHandler.js | 2 +- .../Project/ProjectEntityUpdateHandler.js | 6 +- .../Subscription/LimitationsManager.js | 2 +- .../Features/Subscription/RecurlyWrapper.js | 4 +- .../Subscription/SubscriptionHandler.js | 2 +- .../Uploads/FileSystemImportManager.js | 10 +-- .../web/app/src/Features/User/UserDeleter.js | 2 +- .../src/Editor/EditorHttpControllerTests.js | 63 ++++++++++++++++--- .../src/Subscription/RecurlyWrapperTests.js | 2 +- 13 files changed, 78 insertions(+), 33 deletions(-) diff --git a/services/web/app/src/Features/Announcements/AnnouncementsHandler.js b/services/web/app/src/Features/Announcements/AnnouncementsHandler.js index dafcc2d165..03a2ce5aee 100644 --- a/services/web/app/src/Features/Announcements/AnnouncementsHandler.js +++ b/services/web/app/src/Features/Announcements/AnnouncementsHandler.js @@ -40,7 +40,7 @@ module.exports = AnnouncementsHandler = { callback = function(err, announcements) {} } if (user == null && user._id == null) { - return callback('user not supplied') + return callback(new Error('user not supplied')) } const timestamp = user._id.toString().substring(0, 8) diff --git a/services/web/app/src/Features/Blog/BlogHandler.js b/services/web/app/src/Features/Blog/BlogHandler.js index b0e0d51aff..bd52278c03 100644 --- a/services/web/app/src/Features/Blog/BlogHandler.js +++ b/services/web/app/src/Features/Blog/BlogHandler.js @@ -30,7 +30,7 @@ module.exports = BlogHandler = { return callback(err) } if (res.statusCode !== 200) { - return callback('blog announcement returned non 200') + return callback(new Error('blog announcement returned non 200')) } logger.log( { diff --git a/services/web/app/src/Features/Editor/EditorHttpController.js b/services/web/app/src/Features/Editor/EditorHttpController.js index 79a1b387f1..2f3a29866c 100644 --- a/services/web/app/src/Features/Editor/EditorHttpController.js +++ b/services/web/app/src/Features/Editor/EditorHttpController.js @@ -159,11 +159,11 @@ module.exports = EditorHttpController = { 'editor', user_id, function(error, doc) { - if (error === 'project_has_to_many_files') { + if (error && error.message === 'project_has_to_many_files') { return res .status(400) .json(req.i18n.translate('project_has_to_many_files')) - } else if (error != null) { + } else if (error) { return next(error) } else { return res.json(doc) @@ -185,15 +185,13 @@ module.exports = EditorHttpController = { name, 'editor', function(error, doc) { - if (error === 'project_has_to_many_files') { + if (error && error.message === 'project_has_to_many_files') { return res .status(400) .json(req.i18n.translate('project_has_to_many_files')) - } else if ( - (error != null ? error.message : undefined) === 'invalid element name' - ) { + } else if (error && error.message === 'invalid element name') { return res.status(400).json(req.i18n.translate('invalid_file_name')) - } else if (error != null) { + } else if (error) { return next(error) } else { return res.json(doc) diff --git a/services/web/app/src/Features/Email/EmailSender.js b/services/web/app/src/Features/Email/EmailSender.js index 0ce3668fa6..5ce8c97025 100644 --- a/services/web/app/src/Features/Email/EmailSender.js +++ b/services/web/app/src/Features/Email/EmailSender.js @@ -173,7 +173,7 @@ module.exports = { }, 'rate limit hit for sending email, not sending' ) - return callback('rate limit hit sending email') + return callback(new Error('rate limit hit sending email')) } metrics.inc('email') options = { diff --git a/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js index a666253539..f747ef792c 100644 --- a/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js @@ -651,7 +651,7 @@ module.exports = ProjectEntityMongoUpdateHandler = self = { 'project too big, stopping insertions' ) CooldownManager.putProjectOnCooldown(project._id) - return callback('project_has_to_many_files') + return callback(new Error('project_has_to_many_files')) } return ProjectLocator.findElement( diff --git a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js index 0d28f9eece..cc544eab3b 100644 --- a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js @@ -1130,7 +1130,7 @@ module.exports = ProjectEntityUpdateHandler = self = { logger.log({ entity_id, entityType, project_id }, 'deleting project entity') if (entityType == null) { logger.warn({ err: 'No entityType set', project_id, entity_id }) - return callback('No entityType set') + return callback(new Error('No entityType set')) } entityType = entityType.toLowerCase() return ProjectEntityMongoUpdateHandler.deleteEntity( @@ -1261,7 +1261,7 @@ module.exports = ProjectEntityUpdateHandler = self = { ) if (entityType == null) { logger.warn({ err: 'No entityType set', project_id, entity_id }) - return callback('No entityType set') + return callback(new Error('No entityType set')) } entityType = entityType.toLowerCase() return ProjectEntityMongoUpdateHandler.moveEntity( @@ -1309,7 +1309,7 @@ module.exports = ProjectEntityUpdateHandler = self = { logger.log({ entity_id, project_id }, `renaming ${entityType}`) if (entityType == null) { logger.warn({ err: 'No entityType set', project_id, entity_id }) - return callback('No entityType set') + return callback(new Error('No entityType set')) } entityType = entityType.toLowerCase() diff --git a/services/web/app/src/Features/Subscription/LimitationsManager.js b/services/web/app/src/Features/Subscription/LimitationsManager.js index 841fa0bcd7..074d5ce671 100644 --- a/services/web/app/src/Features/Subscription/LimitationsManager.js +++ b/services/web/app/src/Features/Subscription/LimitationsManager.js @@ -246,7 +246,7 @@ module.exports = LimitationsManager = { } if (subscription == null) { logger.warn({ subscriptionId }, 'no subscription found') - return callback('no subscription found') + return callback(new Error('no subscription found')) } const limitReached = LimitationsManager.teamHasReachedMemberLimit( diff --git a/services/web/app/src/Features/Subscription/RecurlyWrapper.js b/services/web/app/src/Features/Subscription/RecurlyWrapper.js index 9d4d87f9aa..a0cc20bdb3 100644 --- a/services/web/app/src/Features/Subscription/RecurlyWrapper.js +++ b/services/web/app/src/Features/Subscription/RecurlyWrapper.js @@ -505,7 +505,9 @@ module.exports = RecurlyWrapper = { /accounts\/(.*)/ )[1] } else { - return callback("I don't understand the response from Recurly") + return callback( + new Error("I don't understand the response from Recurly") + ) } return RecurlyWrapper.getAccount(accountId, function( diff --git a/services/web/app/src/Features/Subscription/SubscriptionHandler.js b/services/web/app/src/Features/Subscription/SubscriptionHandler.js index 10436e1635..0fa637d64a 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionHandler.js +++ b/services/web/app/src/Features/Subscription/SubscriptionHandler.js @@ -225,7 +225,7 @@ module.exports = { return callback(error) } if (user == null) { - return callback('no user found') + return callback(new Error('no user found')) } return SubscriptionUpdater.syncSubscription( recurlySubscription, diff --git a/services/web/app/src/Features/Uploads/FileSystemImportManager.js b/services/web/app/src/Features/Uploads/FileSystemImportManager.js index 007c98dfa6..c82325c835 100644 --- a/services/web/app/src/Features/Uploads/FileSystemImportManager.js +++ b/services/web/app/src/Features/Uploads/FileSystemImportManager.js @@ -44,7 +44,7 @@ module.exports = FileSystemImportManager = { { user_id, project_id, folder_id, name, path }, 'add doc is from symlink, stopping process' ) - return callback('path is symlink') + return callback(new Error('path is symlink')) } return fs.readFile(path, charset, function(error, content) { if (error != null) { @@ -90,7 +90,7 @@ module.exports = FileSystemImportManager = { { user_id, project_id, folder_id, name, path }, 'add file is from symlink, stopping insert' ) - return callback('path is symlink') + return callback(new Error('path is symlink')) } if (replace) { @@ -132,7 +132,7 @@ module.exports = FileSystemImportManager = { { user_id, project_id, folder_id, path }, 'add folder is from symlink, stopping insert' ) - return callback('path is symlink') + return callback(new Error('path is symlink')) } return EditorController.addFolder( project_id, @@ -181,7 +181,7 @@ module.exports = FileSystemImportManager = { { user_id, project_id, parent_folder_id, folderPath }, 'add folder contents is from symlink, stopping insert' ) - return callback('path is symlink') + return callback(new Error('path is symlink')) } return fs.readdir(folderPath, (error, entries) => { if (entries == null) { @@ -231,7 +231,7 @@ module.exports = FileSystemImportManager = { { user_id, project_id, folder_id, path }, 'add entry is from symlink, stopping insert' ) - return callback('path is symlink') + return callback(new Error('path is symlink')) } return FileTypeManager.isDirectory(path, (error, isDirectory) => { diff --git a/services/web/app/src/Features/User/UserDeleter.js b/services/web/app/src/Features/User/UserDeleter.js index 18797806ba..8ad95fa593 100644 --- a/services/web/app/src/Features/User/UserDeleter.js +++ b/services/web/app/src/Features/User/UserDeleter.js @@ -65,7 +65,7 @@ module.exports = UserDeleter = { } if (user_id == null) { logger.warn('user_id is null when trying to delete user') - return callback('no user_id') + return callback(new Error('no user_id')) } return User.findById(user_id, function(err, user) { if (err != null) { diff --git a/services/web/test/unit/src/Editor/EditorHttpControllerTests.js b/services/web/test/unit/src/Editor/EditorHttpControllerTests.js index 845b19a113..da465edb4f 100644 --- a/services/web/test/unit/src/Editor/EditorHttpControllerTests.js +++ b/services/web/test/unit/src/Editor/EditorHttpControllerTests.js @@ -51,7 +51,7 @@ describe('EditorHttpController', function() { this.AuthenticationController.getLoggedInUserId = sinon .stub() .returns(this.userId) - this.req = {} + this.req = { i18n: { translate: string => string } } this.res = { send: sinon.stub(), sendStatus: sinon.stub(), @@ -306,13 +306,27 @@ describe('EditorHttpController', function() { }) describe('unsuccesfully', function() { - beforeEach(function() { + it('handle name too short', function() { this.req.body.name = '' - return this.EditorHttpController.addDoc(this.req, this.res) + this.EditorHttpController.addDoc(this.req, this.res) + this.res.sendStatus.calledWith(400).should.equal(true) }) - it('should send back a bad request status code', function() { - return this.res.sendStatus.calledWith(400).should.equal(true) + it('handle too many files', function() { + this.EditorController.addDoc.yields( + new Error('project_has_to_many_files') + ) + let res = { + status: status => { + status.should.equal(400) + return { + json: json => { + json.should.equal('project_has_to_many_files') + } + } + } + } + this.EditorHttpController.addDoc(this.req, res) }) }) }) @@ -352,13 +366,44 @@ describe('EditorHttpController', function() { }) describe('unsuccesfully', function() { - beforeEach(function() { + it('handle name too short', function() { this.req.body.name = '' - return this.EditorHttpController.addFolder(this.req, this.res) + this.EditorHttpController.addFolder(this.req, this.res) + this.res.sendStatus.calledWith(400).should.equal(true) }) - it('should send back a bad request status code', function() { - return this.res.sendStatus.calledWith(400).should.equal(true) + it('handle too many files', function() { + this.EditorController.addFolder.yields( + new Error('project_has_to_many_files') + ) + let res = { + status: status => { + status.should.equal(400) + return { + json: json => { + json.should.equal('project_has_to_many_files') + } + } + } + } + this.EditorHttpController.addFolder(this.req, res) + }) + + it('handle invalid element name', function() { + this.EditorController.addFolder.yields( + new Error('invalid element name') + ) + let res = { + status: status => { + status.should.equal(400) + return { + json: json => { + json.should.equal('invalid_file_name') + } + } + } + } + this.EditorHttpController.addFolder(this.req, res) }) }) }) diff --git a/services/web/test/unit/src/Subscription/RecurlyWrapperTests.js b/services/web/test/unit/src/Subscription/RecurlyWrapperTests.js index bc182c9936..3b3b3568ff 100644 --- a/services/web/test/unit/src/Subscription/RecurlyWrapperTests.js +++ b/services/web/test/unit/src/Subscription/RecurlyWrapperTests.js @@ -110,7 +110,7 @@ const mockApiRequest = function(options, callback) { if (fixtures[options.url]) { return callback(null, { statusCode: 200 }, fixtures[options.url]) } else { - return callback('Not found', { statusCode: 404 }) + return callback(new Error('Not found'), { statusCode: 404 }) } }