From 244709df5e3260f00dfd6fcd546aba509e847a12 Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Thu, 16 Jul 2020 16:55:05 +0200 Subject: [PATCH] Merge pull request #2988 from overleaf/msm-oerror-remove-not-found-error Replace HTTPErrors.NotFoundError with calls to notFound() handler GitOrigin-RevId: c98582a5bd3d862b3c17fb03d863c75f64851aba --- .../Collaborators/CollaboratorsController.js | 10 ++-- .../Features/Editor/EditorHttpController.js | 5 +- .../src/Features/Errors/HttpErrorHandler.js | 12 +++++ .../UserMembershipMiddleware.js | 9 ++-- .../CollaboratorsControllerTests.js | 50 +++++++++---------- .../src/Editor/EditorHttpControllerTests.js | 16 ++++++ .../unit/src/Errors/HttpErrorHandlerTests.js | 32 ++++++++++++ 7 files changed, 92 insertions(+), 42 deletions(-) diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsController.js b/services/web/app/src/Features/Collaborators/CollaboratorsController.js index abc53e216f..ee82de8ea5 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsController.js +++ b/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, diff --git a/services/web/app/src/Features/Editor/EditorHttpController.js b/services/web/app/src/Features/Editor/EditorHttpController.js index 5931cf275d..8d3573cc8c 100644 --- a/services/web/app/src/Features/Editor/EditorHttpController.js +++ b/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, diff --git a/services/web/app/src/Features/Errors/HttpErrorHandler.js b/services/web/app/src/Features/Errors/HttpErrorHandler.js index 69d4655627..ff4f7dc325 100644 --- a/services/web/app/src/Features/Errors/HttpErrorHandler.js +++ b/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'])) { diff --git a/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js b/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js index 84b6995fc1..ff844b5376 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js +++ b/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 diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js index 99cde048ce..7dc275d6f9 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsControllerTests.js +++ b/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) { diff --git a/services/web/test/unit/src/Editor/EditorHttpControllerTests.js b/services/web/test/unit/src/Editor/EditorHttpControllerTests.js index dd674559cd..3e212d109a 100644 --- a/services/web/test/unit/src/Editor/EditorHttpControllerTests.js +++ b/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) + }) + }) }) }) diff --git a/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js b/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js index f83114eb37..25fe471946 100644 --- a/services/web/test/unit/src/Errors/HttpErrorHandlerTests.js +++ b/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)