From b28271082c4177a9bc487ba74e1ca40ff80f600f Mon Sep 17 00:00:00 2001 From: Alexandre Bourdin Date: Wed, 6 Apr 2022 14:33:25 +0200 Subject: [PATCH] Merge pull request #7246 from overleaf/ab-references-handler-unhandled-error [web] Fix unhandled error in ReferencesHandler GitOrigin-RevId: 1b7f2f186780b3ed79434a509ce3dc54e1c38f07 --- .../References/ReferencesController.js | 21 ++++----- .../Features/References/ReferencesHandler.js | 11 +++++ .../References/ReferencesControllerTests.js | 13 +++--- .../src/References/ReferencesHandlerTests.js | 45 +++++++++++++++++++ 4 files changed, 75 insertions(+), 15 deletions(-) diff --git a/services/web/app/src/Features/References/ReferencesController.js b/services/web/app/src/Features/References/ReferencesController.js index 0ea054f2a5..e5fb2f2cf2 100644 --- a/services/web/app/src/Features/References/ReferencesController.js +++ b/services/web/app/src/Features/References/ReferencesController.js @@ -15,9 +15,10 @@ const logger = require('@overleaf/logger') const ReferencesHandler = require('./ReferencesHandler') const settings = require('@overleaf/settings') const EditorRealTimeController = require('../Editor/EditorRealTimeController') +const { OError } = require('../Errors/Errors') module.exports = ReferencesController = { - index(req, res) { + index(req, res, next) { const projectId = req.params.Project_id const { shouldBroadcast } = req.body const { docIds } = req.body @@ -28,10 +29,10 @@ module.exports = ReferencesController = { ) return res.sendStatus(400) } - return ReferencesHandler.index(projectId, docIds, function (err, data) { - if (err != null) { - logger.err({ err, projectId }, 'error indexing all references') - return res.sendStatus(500) + return ReferencesHandler.index(projectId, docIds, function (error, data) { + if (error) { + OError.tag(error, 'failed to index references', { projectId }) + return next(error) } return ReferencesController._handleIndexResponse( req, @@ -43,13 +44,13 @@ module.exports = ReferencesController = { }) }, - indexAll(req, res) { + indexAll(req, res, next) { const projectId = req.params.Project_id const { shouldBroadcast } = req.body - return ReferencesHandler.indexAll(projectId, function (err, data) { - if (err != null) { - logger.err({ err, projectId }, 'error indexing all references') - return res.sendStatus(500) + return ReferencesHandler.indexAll(projectId, function (error, data) { + if (error) { + OError.tag(error, 'failed to index references', { projectId }) + return next(error) } return ReferencesController._handleIndexResponse( req, diff --git a/services/web/app/src/Features/References/ReferencesHandler.js b/services/web/app/src/Features/References/ReferencesHandler.js index f0ba20c5cd..d8312d9e1b 100644 --- a/services/web/app/src/Features/References/ReferencesHandler.js +++ b/services/web/app/src/Features/References/ReferencesHandler.js @@ -23,6 +23,7 @@ const UserGetter = require('../User/UserGetter') const DocumentUpdaterHandler = require('../DocumentUpdater/DocumentUpdaterHandler') const _ = require('underscore') const Async = require('async') +const Errors = require('../Errors/Errors') const oneMinInMs = 60 * 1000 const fiveMinsInMs = oneMinInMs * 5 @@ -111,6 +112,11 @@ module.exports = ReferencesHandler = { }) return callback(err) } + if (!project) { + return callback( + new Errors.NotFoundError(`project does not exist: ${projectId}`) + ) + } logger.log({ projectId }, 'indexing all bib files in project') const docIds = ReferencesHandler._findBibDocIds(project) const fileIds = ReferencesHandler._findBibFileIds(project) @@ -139,6 +145,11 @@ module.exports = ReferencesHandler = { }) return callback(err) } + if (!project) { + return callback( + new Errors.NotFoundError(`project does not exist: ${projectId}`) + ) + } return ReferencesHandler._doIndexOperation( projectId, project, diff --git a/services/web/test/unit/src/References/ReferencesControllerTests.js b/services/web/test/unit/src/References/ReferencesControllerTests.js index faace5493c..0be49cff2e 100644 --- a/services/web/test/unit/src/References/ReferencesControllerTests.js +++ b/services/web/test/unit/src/References/ReferencesControllerTests.js @@ -44,6 +44,7 @@ describe('ReferencesController', function () { this.res = new MockResponse() this.res.json = sinon.stub() this.res.sendStatus = sinon.stub() + this.next = sinon.stub() return (this.fakeResponseData = { projectId: this.projectId, keys: ['one', 'two', 'three'], @@ -59,7 +60,7 @@ describe('ReferencesController', function () { this.fakeResponseData ) return (this.call = callback => { - this.controller.indexAll(this.req, this.res) + this.controller.indexAll(this.req, this.res, this.next) return callback() }) }) @@ -166,7 +167,7 @@ describe('ReferencesController', function () { beforeEach(function () { this.ReferencesHandler.indexAll.callsArgWith(1) return (this.call = callback => { - this.controller.indexAll(this.req, this.res) + this.controller.indexAll(this.req, this.res, this.next) return callback() }) }) @@ -201,7 +202,7 @@ describe('ReferencesController', function () { describe('index', function () { beforeEach(function () { return (this.call = callback => { - this.controller.index(this.req, this.res) + this.controller.index(this.req, this.res, this.next) return callback() }) }) @@ -260,8 +261,10 @@ describe('ReferencesController', function () { it('should produce an error response', function (done) { return this.call(() => { - this.res.sendStatus.callCount.should.equal(1) - this.res.sendStatus.calledWith(500).should.equal(true) + this.next.callCount.should.equal(1) + this.next + .calledWith(sinon.match.instanceOf(Error)) + .should.equal(true) return done() }) }) diff --git a/services/web/test/unit/src/References/ReferencesHandlerTests.js b/services/web/test/unit/src/References/ReferencesHandlerTests.js index 272ef49336..a61534a8d4 100644 --- a/services/web/test/unit/src/References/ReferencesHandlerTests.js +++ b/services/web/test/unit/src/References/ReferencesHandlerTests.js @@ -15,6 +15,7 @@ const SandboxedModule = require('sandboxed-module') const { assert, expect } = require('chai') const sinon = require('sinon') +const Errors = require('../../../../app/src/Features/Errors/Errors') const modulePath = '../../../../app/src/Features/References/ReferencesHandler' describe('ReferencesHandler', function () { @@ -206,6 +207,28 @@ describe('ReferencesHandler', function () { }) }) + describe('when ProjectGetter.getProject returns null', function () { + beforeEach(function () { + return this.ProjectGetter.getProject.callsArgWith(2, null) + }) + + it('should produce an error', function (done) { + return this.call((err, data) => { + expect(err).to.not.equal(null) + expect(err).to.be.instanceof(Errors.NotFoundError) + expect(data).to.equal(undefined) + return done() + }) + }) + + it('should not send request', function (done) { + return this.call((err, data) => { + this.request.post.callCount.should.equal(0) + return done() + }) + }) + }) + describe('when _isFullIndex produces an error', function () { beforeEach(function () { this.ProjectGetter.getProject.callsArgWith(2, null, this.fakeProject) @@ -389,6 +412,28 @@ describe('ReferencesHandler', function () { }) }) + describe('when ProjectGetter.getProject returns null', function () { + beforeEach(function () { + return this.ProjectGetter.getProject.callsArgWith(2, null) + }) + + it('should produce an error', function (done) { + return this.call((err, data) => { + expect(err).to.not.equal(null) + expect(err).to.be.instanceof(Errors.NotFoundError) + expect(data).to.equal(undefined) + return done() + }) + }) + + it('should not send request', function (done) { + return this.call((err, data) => { + this.request.post.callCount.should.equal(0) + return done() + }) + }) + }) + describe('when _isFullIndex produces an error', function () { beforeEach(function () { this.ProjectGetter.getProject.callsArgWith(2, null, this.fakeProject)