Merge pull request #7246 from overleaf/ab-references-handler-unhandled-error

[web] Fix unhandled error in ReferencesHandler

GitOrigin-RevId: 1b7f2f186780b3ed79434a509ce3dc54e1c38f07

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,

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,

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()
           })
         })

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)