Remove HttpErrors.InternalServerError (#3027)

* Added legacyInternal() 500 to HttpErrorHandler
* replaced HttpErrors.InternalServerError being thrown with calls to HttpHandler.legacyInternal()

GitOrigin-RevId: 0b7086a9693b57cdf93976d4221b90315960e8bb

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

@@ -1,5 +1,4 @@
 const OError = require('@overleaf/o-error')
-const HttpErrors = require('@overleaf/o-error/http')
 const HttpErrorHandler = require('../../Features/Errors/HttpErrorHandler')
 const { ObjectId } = require('mongodb')
 const CollaboratorsHandler = require('./CollaboratorsHandler')
@@ -72,7 +71,7 @@ async function setCollaboratorInfo(req, res, next) {
     if (err instanceof Errors.NotFoundError) {
       HttpErrorHandler.notFound(req, res)
     } else {
-      throw new HttpErrors.InternalServerError({}).withCause(err)
+      next(err)
     }
   }
 }
@@ -103,7 +102,7 @@ async function transferOwnership(req, res, next) {
         `user ${toUserId} should be a collaborator in project ${projectId} prior to ownership transfer`
       )
     } else {
-      throw new HttpErrors.InternalServerError({}).withCause(err)
+      next(err)
     }
   }
 }

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

@@ -79,5 +79,18 @@ module.exports = {
       default:
         return res.send('unprocessable entity')
     }
+  },
+
+  legacyInternal(req, res, message, error) {
+    logger.error(error)
+    res.status(500)
+    switch (req.accepts(['html', 'json'])) {
+      case 'html':
+        return res.render('general/500', { title: 'Server Error' })
+      case 'json':
+        return renderJSONError(res, message, {})
+      default:
+        return res.send('internal server error')
+    }
   }
 }

services/web/app/src/Features/Subscription/SubscriptionController.js

@@ -33,7 +33,6 @@ const Errors = require('../Errors/Errors')
 const HttpErrorHandler = require('../Errors/HttpErrorHandler')
 const SubscriptionErrors = require('./Errors')
 const OError = require('@overleaf/o-error')
-const HttpErrors = require('@overleaf/o-error/http')
 
 module.exports = SubscriptionController = {
   plansPage(req, res, next) {
@@ -340,7 +339,7 @@ module.exports = SubscriptionController = {
       error
     ) {
       if (error) {
-        return next(new HttpErrors.InternalServerError({}).withCause(error))
+        return next(error)
       }
       res.sendStatus(200)
     })

services/web/app/src/Features/User/UserController.js

@@ -15,7 +15,6 @@ const SudoModeHandler = require('../SudoMode/SudoModeHandler')
 const Errors = require('../Errors/Errors')
 const HttpErrorHandler = require('../Errors/HttpErrorHandler')
 const OError = require('@overleaf/o-error')
-const HttpErrors = require('@overleaf/o-error/http')
 const EmailHandler = require('../Email/EmailHandler')
 const UrlHelper = require('../Helpers/UrlHelper')
 const { promisify } = require('util')
@@ -236,21 +235,20 @@ const UserController = {
           // update the user email
           UserUpdater.changeEmailAddress(userId, newEmail, err => {
             if (err) {
-              let errorData = {
-                message: 'problem updating users email address',
-                info: { userId, newEmail, public: {} }
-              }
               if (err instanceof Errors.EmailExistsError) {
                 const translation = req.i18n.translate(
                   'email_already_registered'
                 )
                 return HttpErrorHandler.conflict(req, res, translation)
               } else {
-                errorData.info.public.message = req.i18n.translate(
-                  'problem_changing_email_address'
-                )
-                return next(
-                  new HttpErrors.InternalServerError(errorData).withCause(err)
+                return HttpErrorHandler.legacyInternal(
+                  req,
+                  res,
+                  req.i18n.translate('problem_changing_email_address'),
+                  new OError({
+                    message: 'problem_changing_email_address',
+                    info: { userId, newEmail }
+                  }).withCause(err)
                 )
               }
             }

services/web/app/src/Features/User/UserEmailsController.js

@@ -7,7 +7,6 @@ const UserEmailsConfirmationHandler = require('./UserEmailsConfirmationHandler')
 const { endorseAffiliation } = require('../Institutions/InstitutionsAPI')
 const Errors = require('../Errors/Errors')
 const HttpErrorHandler = require('../Errors/HttpErrorHandler')
-const HttpErrors = require('@overleaf/o-error/http')
 
 function add(req, res, next) {
   const userId = AuthenticationController.getLoggedInUserId(req)
@@ -167,6 +166,6 @@ module.exports = UserEmailsController = {
       const message = req.i18n.translate('email_does_not_belong_to_university')
       return HttpErrorHandler.conflict(req, res, message)
     }
-    next(new HttpErrors.InternalServerError().withCause(error))
+    next(error)
   }
 }

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

@@ -236,6 +236,22 @@ describe('CollaboratorsController', function() {
       )
       this.CollaboratorsController.setCollaboratorInfo(this.req, this.res)
     })
+
+    it('should pass the error to the next handler when setting the privilege level fails', function(done) {
+      this.next = sinon.spy(err => {
+        expect(err).instanceOf(Error)
+        done()
+      })
+
+      this.CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel.rejects(
+        new Error()
+      )
+      this.CollaboratorsController.setCollaboratorInfo(
+        this.req,
+        this.res,
+        this.next
+      )
+    })
   })
 
   describe('transferOwnership', function() {

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

@@ -1,4 +1,5 @@
 const { expect } = require('chai')
+const sinon = require('sinon')
 const MockResponse = require('../helpers/MockResponse')
 const MockRequest = require('../helpers/MockRequest')
 const SandboxedModule = require('sandboxed-module')
@@ -9,8 +10,16 @@ describe('HttpErrorHandler', function() {
     this.req = new MockRequest()
     this.res = new MockResponse()
 
+    this.logger = {
+      warn() {},
+      error() {}
+    }
+
     this.HttpErrorHandler = SandboxedModule.require(modulePath, {
-      globals: { console }
+      globals: { console },
+      requires: {
+        'logger-sharelatex': this.logger
+      }
     })
   })
 
@@ -201,5 +210,51 @@ describe('HttpErrorHandler', function() {
         foo: 'bar'
       })
     })
+
+    describe('legacyInternal', function() {
+      it('returns 500', function() {
+        this.HttpErrorHandler.legacyInternal(this.req, this.res, new Error())
+        expect(this.res.statusCode).to.equal(500)
+      })
+
+      it('should send the error to the logger', function() {
+        const error = new Error('message')
+        this.logger.error = sinon.stub()
+        this.HttpErrorHandler.legacyInternal(
+          this.req,
+          this.res,
+          'message',
+          error
+        )
+        expect(this.logger.error).to.have.been.calledWith(error)
+      })
+
+      it('should print a message when no content-type is included', function() {
+        this.HttpErrorHandler.legacyInternal(this.req, this.res, new Error())
+        expect(this.res.body).to.equal('internal server error')
+      })
+
+      it("should render a template when content-type is 'html'", function() {
+        this.req.accepts = () => 'html'
+        this.HttpErrorHandler.legacyInternal(this.req, this.res, new Error())
+        expect(this.res.renderedTemplate).to.equal('general/500')
+        expect(this.res.renderedVariables).to.deep.equal({
+          title: 'Server Error'
+        })
+      })
+
+      it("should return a json object with a static message when content-type is 'json'", function() {
+        this.req.accepts = () => 'json'
+        this.HttpErrorHandler.legacyInternal(
+          this.req,
+          this.res,
+          'a message',
+          new Error()
+        )
+        expect(JSON.parse(this.res.body)).to.deep.equal({
+          message: 'a message'
+        })
+      })
+    })
   })
 })

services/web/test/unit/src/Subscription/SubscriptionControllerTests.js

@@ -538,25 +538,32 @@ describe('SubscriptionController', function() {
   })
 
   describe('updateAccountEmailAddress via put', function() {
-    beforeEach(function(done) {
-      this.res = {
-        sendStatus() {
-          return done()
-        }
-      }
-      sinon.spy(this.res, 'sendStatus')
-      this.SubscriptionController.updateAccountEmailAddress(this.req, this.res)
-    })
-
     it('should send the user and subscriptionId to RecurlyWrapper', function() {
+      this.res.sendStatus = sinon.spy()
+      this.SubscriptionController.updateAccountEmailAddress(this.req, this.res)
       this.RecurlyWrapper.updateAccountEmailAddress
         .calledWith(this.user._id, this.user.email)
         .should.equal(true)
     })
 
-    it('shouldrespond with 200', function() {
+    it('should respond with 200', function() {
+      this.res.sendStatus = sinon.spy()
+      this.SubscriptionController.updateAccountEmailAddress(this.req, this.res)
       this.res.sendStatus.calledWith(200).should.equal(true)
     })
+
+    it('should send the error to the next handler when updating recurly account email fails', function(done) {
+      this.RecurlyWrapper.updateAccountEmailAddress.yields(new Error())
+      this.next = sinon.spy(error => {
+        expect(error).instanceOf(Error)
+        done()
+      })
+      this.SubscriptionController.updateAccountEmailAddress(
+        this.req,
+        this.res,
+        this.next
+      )
+    })
   })
 
   describe('reactivateSubscription', function() {

services/web/test/unit/src/User/UserControllerTests.js

@@ -72,7 +72,8 @@ describe('UserController', function() {
     this.SudoModeHandler = { clearSudoMode: sinon.stub() }
     this.HttpErrorHandler = {
       conflict: sinon.stub(),
-      unprocessableEntity: sinon.stub()
+      unprocessableEntity: sinon.stub(),
+      legacyInternal: sinon.stub()
     }
     this.UserController = SandboxedModule.require(modulePath, {
       globals: {
@@ -416,18 +417,16 @@ describe('UserController', function() {
       it('should pass on an error and not send a success status', function(done) {
         this.req.body.email = this.newEmail.toUpperCase()
         this.UserUpdater.changeEmailAddress.callsArgWith(2, new Error())
-        const next = err => {
-          expect(err).to.exist
-          process.nextTick(() => {
-            // logic in User.findById
-            expect(this.res.send.called).to.equal(false)
-            expect(this.res.sendStatus.called).to.equal(false)
-            // logic after error handling
-            expect(this.User.findById.callCount).to.equal(1)
+        this.HttpErrorHandler.legacyInternal = sinon.spy(
+          (req, res, message, error) => {
+            expect(req).to.exist
+            expect(req).to.exist
+            message.should.equal('problem_changing_email_address')
+            expect(error).to.be.instanceof(OError)
             done()
-          })
-        }
-        this.UserController.updateUserSettings(this.req, this.res, next)
+          }
+        )
+        this.UserController.updateUserSettings(this.req, this.res, this.next)
       })
 
       it('should call the HTTP conflict error handler when the email already exists', function(done) {

services/web/test/unit/src/User/UserEmailsControllerTests.js

@@ -2,7 +2,7 @@ const sinon = require('sinon')
 const assertCalledWith = sinon.assert.calledWith
 const assertNotCalled = sinon.assert.notCalled
 const chai = require('chai')
-const { assert } = chai
+const { assert, expect } = chai
 const modulePath = '../../../../app/src/Features/User/UserEmailsController.js'
 const SandboxedModule = require('sandboxed-module')
 const MockRequest = require('../helpers/MockRequest')
@@ -146,6 +146,16 @@ describe('UserEmailsController', function() {
         }
       })
     })
+
+    it('should pass the error to the next handler when adding the email fails', function(done) {
+      this.UserUpdater.addEmailAddress.callsArgWith(3, new Error())
+      this.next = sinon.spy(error => {
+        expect(error).instanceOf(Error)
+        done()
+      })
+      this.UserEmailsController.add(this.req, this.res, this.next)
+    })
+
     it('should call the HTTP conflict handler when the email already exists', function(done) {
       this.UserUpdater.addEmailAddress.callsArgWith(
         3,