Merge pull request #2988 from overleaf/msm-oerror-remove-not-found-error

Replace HTTPErrors.NotFoundError with calls to notFound() handler

GitOrigin-RevId: c98582a5bd3d862b3c17fb03d863c75f64851aba

services/web/app/src/Features/Collaborators/CollaboratorsController.js

@@ -70,7 +70,7 @@ async function setCollaboratorInfo(req, res, next) {
     res.sendStatus(204)
   } catch (err) {
     if (err instanceof Errors.NotFoundError) {
-      throw new HttpErrors.NotFoundError({})
+      HttpErrorHandler.notFound(req, res)
     } else {
       throw new HttpErrors.InternalServerError({}).withCause(err)
     }
@@ -93,13 +93,9 @@ async function transferOwnership(req, res, next) {
     res.sendStatus(204)
   } catch (err) {
     if (err instanceof Errors.ProjectNotFoundError) {
-      throw new HttpErrors.NotFoundError({
-        info: { public: { message: `project not found: ${projectId}` } }
-      })
+      HttpErrorHandler.notFound(req, res, `project not found: ${projectId}`)
     } else if (err instanceof Errors.UserNotFoundError) {
-      throw new HttpErrors.NotFoundError({
-        info: { public: { message: `user not found: ${toUserId}` } }
-      })
+      HttpErrorHandler.notFound(req, res, `user not found: ${toUserId}`)
     } else if (err instanceof Errors.UserNotCollaboratorError) {
       HttpErrorHandler.forbidden(
         req,

services/web/app/src/Features/Editor/EditorHttpController.js

@@ -1,4 +1,3 @@
-const HttpErrors = require('@overleaf/o-error/http')
 const ProjectDeleter = require('../Project/ProjectDeleter')
 const EditorController = require('./EditorController')
 const ProjectGetter = require('../Project/ProjectGetter')
@@ -269,9 +268,7 @@ async function convertDocToFile(req, res, next) {
     res.json({ fileId: fileRef._id.toString() })
   } catch (err) {
     if (err instanceof Errors.NotFoundError) {
-      throw new HttpErrors.NotFoundError({
-        info: { public: { message: 'Document not found' } }
-      })
+      return HttpErrorHandler.notFound(req, res, 'Document not found')
     } else if (err instanceof Errors.DocHasRangesError) {
       return HttpErrorHandler.unprocessableEntity(
         req,

services/web/app/src/Features/Errors/HttpErrorHandler.js

@@ -39,6 +39,18 @@ module.exports = {
     }
   },
 
+  notFound(req, res, message = 'not found', info = {}) {
+    res.status(404)
+    switch (req.accepts(['html', 'json'])) {
+      case 'html':
+        return res.render('general/404', { title: 'page_not_found' })
+      case 'json':
+        return renderJSONError(res, message, info)
+      default:
+        return res.send('not found')
+    }
+  },
+
   unprocessableEntity(req, res, message = 'unprocessable entity', info = {}) {
     res.status(422)
     switch (req.accepts(['html', 'json'])) {

services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js

@@ -5,7 +5,6 @@ const AuthenticationController = require('../Authentication/AuthenticationContro
 const UserMembershipHandler = require('./UserMembershipHandler')
 const EntityConfigs = require('./UserMembershipEntityConfigs')
 const Errors = require('../Errors/Errors')
-const HttpErrors = require('@overleaf/o-error/http')
 const HttpErrorHandler = require('../Errors/HttpErrorHandler')
 const TemplatesManager = require('../Templates/TemplatesManager')
 
@@ -165,15 +164,17 @@ let UserMembershipMiddleware = {
     req.params.id = req.query.resource_id
     let entityName = req.query.resource_type
     if (!entityName) {
-      return next(new HttpErrors.NotFoundError('resource_type param missing'))
+      return HttpErrorHandler.notFound(req, res, 'resource_type param missing')
     }
     entityName = entityName.charAt(0).toUpperCase() + entityName.slice(1)
 
     const middleware =
       UserMembershipMiddleware[`require${entityName}MetricsAccess`]
     if (!middleware) {
-      return next(
-        new HttpErrors.NotFoundError(`incorrect entity name: ${entityName}`)
+      return HttpErrorHandler.notFound(
+        req,
+        res,
+        `incorrect entity name: ${entityName}`
       )
     }
     // run the list of middleware functions in series. This is essencially

services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js

@@ -2,7 +2,6 @@ const sinon = require('sinon')
 const { expect } = require('chai')
 const SandboxedModule = require('sandboxed-module')
 const { ObjectId } = require('mongodb')
-const HttpErrors = require('@overleaf/o-error/http')
 const Errors = require('../../../../app/src/Features/Errors/Errors')
 const MockRequest = require('../helpers/MockRequest')
 const MockResponse = require('../helpers/MockResponse')
@@ -34,7 +33,8 @@ describe('CollaboratorsController', function() {
       emitToRoom: sinon.stub()
     }
     this.HttpErrorHandler = {
-      forbidden: sinon.stub()
+      forbidden: sinon.stub(),
+      notFound: sinon.stub()
     }
     this.TagsHandler = {
       promises: {
@@ -70,7 +70,6 @@ describe('CollaboratorsController', function() {
         '../Authentication/AuthenticationController': this
           .AuthenticationController,
         '../Errors/Errors': Errors,
-        '@overleaf/o-error/http': HttpErrors,
         'logger-sharelatex': this.logger
       }
     })
@@ -226,17 +225,16 @@ describe('CollaboratorsController', function() {
     })
 
     it('should return a 404 when the project or collaborator is not found', function(done) {
+      this.HttpErrorHandler.notFound = sinon.spy((req, res) => {
+        expect(req).to.equal(this.req)
+        expect(res).to.equal(this.res)
+        done()
+      })
+
       this.CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel.rejects(
         new Errors.NotFoundError()
       )
-      this.CollaboratorsController.setCollaboratorInfo(
-        this.req,
-        this.res,
-        err => {
-          expect(err).to.be.instanceof(HttpErrors.NotFoundError)
-          done()
-        }
-      )
+      this.CollaboratorsController.setCollaboratorInfo(this.req, this.res)
     })
   })
 
@@ -254,31 +252,29 @@ describe('CollaboratorsController', function() {
     })
 
     it('returns 404 if the project does not exist', function(done) {
+      this.HttpErrorHandler.notFound = sinon.spy((req, res, message) => {
+        expect(req).to.equal(this.req)
+        expect(res).to.equal(this.res)
+        expect(message).to.match(/project not found/)
+        done()
+      })
       this.OwnershipTransferHandler.promises.transferOwnership.rejects(
         new Errors.ProjectNotFoundError()
       )
-      this.CollaboratorsController.transferOwnership(
-        this.req,
-        this.res,
-        err => {
-          expect(err).to.be.instanceof(HttpErrors.NotFoundError)
-          done()
-        }
-      )
+      this.CollaboratorsController.transferOwnership(this.req, this.res)
     })
 
     it('returns 404 if the user does not exist', function(done) {
+      this.HttpErrorHandler.notFound = sinon.spy((req, res, message) => {
+        expect(req).to.equal(this.req)
+        expect(res).to.equal(this.res)
+        expect(message).to.match(/user not found/)
+        done()
+      })
       this.OwnershipTransferHandler.promises.transferOwnership.rejects(
         new Errors.UserNotFoundError()
       )
-      this.CollaboratorsController.transferOwnership(
-        this.req,
-        this.res,
-        err => {
-          expect(err).to.be.instanceof(HttpErrors.NotFoundError)
-          done()
-        }
-      )
+      this.CollaboratorsController.transferOwnership(this.req, this.res)
     })
 
     it('invokes HTTP forbidden error handler if the user is not a collaborator', function(done) {

services/web/test/unit/src/Editor/EditorHttpControllerTests.js

@@ -123,6 +123,7 @@ describe('EditorHttpController', function() {
       }
     }
     this.HttpErrorHandler = {
+      notFound: sinon.stub(),
       unprocessableEntity: sinon.stub()
     }
     this.EditorHttpController = SandboxedModule.require(MODULE_PATH, {
@@ -559,5 +560,20 @@ describe('EditorHttpController', function() {
         this.EditorHttpController.convertDocToFile(this.req, this.res)
       })
     })
+
+    describe("when the doc does't exist", function() {
+      it('should return a 404 - not found', function(done) {
+        this.ProjectEntityUpdateHandler.promises.convertDocToFile.rejects(
+          new Errors.NotFoundError({})
+        )
+        this.HttpErrorHandler.notFound = sinon.spy((req, res, message) => {
+          expect(req).to.exist
+          expect(res).to.exist
+          expect(message).to.equal('Document not found')
+          done()
+        })
+        this.EditorHttpController.convertDocToFile(this.req, this.res)
+      })
+    })
   })
 })

services/web/test/unit/src/Errors/HttpErrorHandlerTests.js

@@ -95,6 +95,38 @@ describe('HttpErrorHandler', function() {
     })
   })
 
+  describe('notFound', function() {
+    it('returns 404', function() {
+      this.HttpErrorHandler.notFound(this.req, this.res)
+      expect(this.res.statusCode).to.equal(404)
+    })
+
+    it('should print a message when no content-type is included', function() {
+      this.HttpErrorHandler.notFound(this.req, this.res)
+      expect(this.res.body).to.equal('not found')
+    })
+
+    it("should render a template when content-type is 'html'", function() {
+      this.req.accepts = () => 'html'
+      this.HttpErrorHandler.notFound(this.req, this.res)
+      expect(this.res.renderedTemplate).to.equal('general/404')
+      expect(this.res.renderedVariables).to.deep.equal({
+        title: 'page_not_found'
+      })
+    })
+
+    it("should return a json object when content-type is 'json'", function() {
+      this.req.accepts = () => 'json'
+      this.HttpErrorHandler.notFound(this.req, this.res, 'an error', {
+        foo: 'bar'
+      })
+      expect(JSON.parse(this.res.body)).to.deep.equal({
+        message: 'an error',
+        foo: 'bar'
+      })
+    })
+  })
+
   describe('unprocessableEntity', function() {
     it('returns 422', function() {
       this.HttpErrorHandler.unprocessableEntity(this.req, this.res)