From e3a51ee3851c0d75529cb54ff9b58a12ec61feda Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Thu, 1 Sep 2022 11:32:51 -0400 Subject: [PATCH] Merge pull request #9472 from overleaf/em-promisify-tpdscontroller Promisify TpdsController GitOrigin-RevId: 35d1db628c44d39ee71bb3127cb25ece6d184457 --- .../ThirdPartyDataStore/TpdsController.js | 229 +++++++++--------- .../ThirdPartyDataStore/TpdsUpdateHandler.js | 5 + .../ThirdPartyDataStore/UpdateMerger.js | 3 + .../TpdsControllerTests.js | 125 ++++++---- 4 files changed, 192 insertions(+), 170 deletions(-) diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js index 1d3bead17b..2cd7782847 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js @@ -1,5 +1,4 @@ -let parseParams - +const { expressify } = require('../../util/promises') const TpdsUpdateHandler = require('./TpdsUpdateHandler') const UpdateMerger = require('./UpdateMerger') const Errors = require('../Errors/Errors') @@ -8,132 +7,122 @@ const Path = require('path') const metrics = require('@overleaf/metrics') const NotificationsBuilder = require('../Notifications/NotificationsBuilder') const SessionManager = require('../Authentication/SessionManager') -const TpdsQueueManager = require('./TpdsQueueManager').promises +const TpdsQueueManager = require('./TpdsQueueManager') -module.exports = { - // mergeUpdate and deleteUpdate are used by Dropbox, where the project is only passed as the name, as the - // first part of the file path. They have to check the project exists, find it, and create it if not. - // They also ignore 'noisy' files like .DS_Store, .gitignore, etc. - mergeUpdate(req, res) { - metrics.inc('tpds.merge-update') - const { filePath, userId, projectName } = parseParams(req) - const source = req.headers['x-sl-update-source'] || 'unknown' +// mergeUpdate and deleteUpdate are used by Dropbox, where the project is only passed as the name, as the +// first part of the file path. They have to check the project exists, find it, and create it if not. +// They also ignore 'noisy' files like .DS_Store, .gitignore, etc. - TpdsUpdateHandler.newUpdate( +async function mergeUpdate(req, res) { + metrics.inc('tpds.merge-update') + const { filePath, userId, projectName } = parseParams(req) + const source = req.headers['x-sl-update-source'] || 'unknown' + + try { + await TpdsUpdateHandler.promises.newUpdate( userId, projectName, filePath, req, - source, - err => { - if (err) { - if (err.name === 'TooManyRequestsError') { - logger.warn( - { err, userId, filePath }, - 'tpds update failed to be processed, too many requests' - ) - res.sendStatus(429) - } else if (err.message === 'project_has_too_many_files') { - logger.warn( - { err, userId, filePath }, - 'tpds trying to append to project over file limit' - ) - NotificationsBuilder.tpdsFileLimit(userId).create(projectName) - res.sendStatus(400) - } else { - logger.err( - { err, userId, filePath }, - 'error receiving update from tpds' - ) - res.sendStatus(500) - } - } else { - res.sendStatus(200) - } - } + source ) - }, - - deleteUpdate(req, res) { - metrics.inc('tpds.delete-update') - const { filePath, userId, projectName } = parseParams(req) - const source = req.headers['x-sl-update-source'] || 'unknown' - TpdsUpdateHandler.deleteUpdate( - userId, - projectName, - filePath, - source, - err => { - if (err) { - logger.err( - { err, userId, filePath }, - 'error receiving update from tpds' - ) - res.sendStatus(500) - } else { - res.sendStatus(200) - } - } - ) - }, - - // updateProjectContents and deleteProjectContents are used by GitHub. The project_id is known so we - // can skip right ahead to creating/updating/deleting the file. These methods will not ignore noisy - // files like .DS_Store, .gitignore, etc because people are generally more explicit with the files they - // want in git. - updateProjectContents(req, res, next) { - const projectId = req.params.project_id - const path = `/${req.params[0]}` // UpdateMerger expects leading slash - const source = req.headers['x-sl-update-source'] || 'unknown' - UpdateMerger.mergeUpdate(null, projectId, path, req, source, error => { - if (error) { - if (error.constructor === Errors.InvalidNameError) { - return res.sendStatus(422) - } else { - return next(error) - } - } - res.sendStatus(200) - }) - }, - - deleteProjectContents(req, res, next) { - const projectId = req.params.project_id - const path = `/${req.params[0]}` // UpdateMerger expects leading slash - const source = req.headers['x-sl-update-source'] || 'unknown' - - UpdateMerger.deleteUpdate(null, projectId, path, source, error => { - if (error) { - return next(error) - } - res.sendStatus(200) - }) - }, - - async getQueues(req, res, next) { - const userId = SessionManager.getLoggedInUserId(req.session) - try { - res.json(await TpdsQueueManager.getQueues(userId)) - } catch (err) { - next(err) - } - }, - - parseParams: (parseParams = function (req) { - let filePath, projectName - let path = req.params[0] - const userId = req.params.user_id - - path = Path.join('/', path) - if (path.substring(1).indexOf('/') === -1) { - filePath = '/' - projectName = path.substring(1) + } catch (err) { + if (err.name === 'TooManyRequestsError') { + logger.warn( + { err, userId, filePath }, + 'tpds update failed to be processed, too many requests' + ) + return res.sendStatus(429) + } else if (err.message === 'project_has_too_many_files') { + logger.warn( + { err, userId, filePath }, + 'tpds trying to append to project over file limit' + ) + NotificationsBuilder.tpdsFileLimit(userId).create(projectName) + return res.sendStatus(400) } else { - filePath = path.substring(path.indexOf('/', 1)) - projectName = path.substring(0, path.indexOf('/', 1)) - projectName = projectName.replace('/', '') + throw err } + } - return { filePath, userId, projectName } - }), + res.sendStatus(200) +} + +async function deleteUpdate(req, res) { + metrics.inc('tpds.delete-update') + const { filePath, userId, projectName } = parseParams(req) + const source = req.headers['x-sl-update-source'] || 'unknown' + + await TpdsUpdateHandler.promises.deleteUpdate( + userId, + projectName, + filePath, + source + ) + res.sendStatus(200) +} + +// updateProjectContents and deleteProjectContents are used by GitHub. The project_id is known so we +// can skip right ahead to creating/updating/deleting the file. These methods will not ignore noisy +// files like .DS_Store, .gitignore, etc because people are generally more explicit with the files they +// want in git. + +async function updateProjectContents(req, res, next) { + const projectId = req.params.project_id + const path = `/${req.params[0]}` // UpdateMerger expects leading slash + const source = req.headers['x-sl-update-source'] || 'unknown' + + try { + await UpdateMerger.promises.mergeUpdate(null, projectId, path, req, source) + } catch (error) { + if (error.constructor === Errors.InvalidNameError) { + return res.sendStatus(422) + } else { + throw error + } + } + res.sendStatus(200) +} + +async function deleteProjectContents(req, res, next) { + const projectId = req.params.project_id + const path = `/${req.params[0]}` // UpdateMerger expects leading slash + const source = req.headers['x-sl-update-source'] || 'unknown' + + await UpdateMerger.promises.deleteUpdate(null, projectId, path, source) + res.sendStatus(200) +} + +async function getQueues(req, res, next) { + const userId = SessionManager.getLoggedInUserId(req.session) + res.json(await TpdsQueueManager.promises.getQueues(userId)) +} + +function parseParams(req) { + let filePath, projectName + let path = req.params[0] + const userId = req.params.user_id + + path = Path.join('/', path) + if (path.substring(1).indexOf('/') === -1) { + filePath = '/' + projectName = path.substring(1) + } else { + filePath = path.substring(path.indexOf('/', 1)) + projectName = path.substring(0, path.indexOf('/', 1)) + projectName = projectName.replace('/', '') + } + + return { filePath, userId, projectName } +} + +module.exports = { + mergeUpdate: expressify(mergeUpdate), + deleteUpdate: expressify(deleteUpdate), + updateProjectContents: expressify(updateProjectContents), + deleteProjectContents: expressify(deleteProjectContents), + getQueues: expressify(getQueues), + + // for tests only + parseParams, } diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js index 33cc249fb1..b35d1ff516 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js @@ -1,3 +1,4 @@ +const { promisify } = require('util') const UpdateMerger = require('./UpdateMerger') const logger = require('@overleaf/logger') const NotificationsBuilder = require('../Notifications/NotificationsBuilder') @@ -167,4 +168,8 @@ function handleDuplicateProjects(userId, projectName, callback) { module.exports = { newUpdate, deleteUpdate, + promises: { + newUpdate: promisify(newUpdate), + deleteUpdate: promisify(deleteUpdate), + }, } diff --git a/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js b/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js index 65daa5cbef..ecbd87439e 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js @@ -21,6 +21,7 @@ const EditorController = require('../Editor/EditorController') const FileTypeManager = require('../Uploads/FileTypeManager') const FileWriter = require('../../infrastructure/FileWriter') const ProjectEntityHandler = require('../Project/ProjectEntityHandler') +const { promisifyAll } = require('../../util/promises') module.exports = UpdateMerger = { mergeUpdate(user_id, project_id, path, updateRequest, source, callback) { @@ -259,3 +260,5 @@ module.exports = UpdateMerger = { }, }, } + +module.exports.promises = promisifyAll(module.exports) diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js index 4a84c52920..92c2ea6cf2 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js @@ -1,34 +1,44 @@ const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') +const { expect } = require('chai') const Errors = require('../../../../app/src/Features/Errors/Errors') -const modulePath = require('path').join( - __dirname, + +const MODULE_PATH = '../../../../app/src/Features/ThirdPartyDataStore/TpdsController.js' -) describe('TpdsController', function () { beforeEach(function () { - this.TpdsUpdateHandler = {} + this.TpdsUpdateHandler = { + promises: { + newUpdate: sinon.stub().resolves(), + deleteUpdate: sinon.stub().resolves(), + }, + } + this.UpdateMerger = { + promises: { + mergeUpdate: sinon.stub().resolves(), + deleteUpdate: sinon.stub().resolves(), + }, + } + this.NotificationsBuilder = { + tpdsFileLimit: sinon.stub().returns({ create: sinon.stub() }), + } this.SessionManager = { getLoggedInUserId: sinon.stub().returns('user-id'), } this.TpdsQueueManager = { promises: { - getQueues: sinon.stub().returns('queues'), + getQueues: sinon.stub().resolves('queues'), }, } - this.TpdsController = SandboxedModule.require(modulePath, { + + this.TpdsController = SandboxedModule.require(MODULE_PATH, { requires: { './TpdsUpdateHandler': this.TpdsUpdateHandler, - './UpdateMerger': (this.UpdateMerger = {}), - '../Notifications/NotificationsBuilder': (this.NotificationsBuilder = { - tpdsFileLimit: sinon.stub().returns({ create: sinon.stub() }), - }), + './UpdateMerger': this.UpdateMerger, + '../Notifications/NotificationsBuilder': this.NotificationsBuilder, '../Authentication/SessionManager': this.SessionManager, './TpdsQueueManager': this.TpdsQueueManager, - '@overleaf/metrics': { - inc() {}, - }, }, }) @@ -48,10 +58,9 @@ describe('TpdsController', function () { 'x-sl-update-source': (this.source = 'dropbox'), }, } - this.TpdsUpdateHandler.newUpdate = sinon.stub().callsArg(5) const res = { sendStatus: () => { - this.TpdsUpdateHandler.newUpdate + this.TpdsUpdateHandler.promises.newUpdate .calledWith( this.user_id, 'projectName', @@ -66,7 +75,7 @@ describe('TpdsController', function () { this.TpdsController.mergeUpdate(req, res) }) - it('should return a 500 error when the update receiver fails', function () { + it('should return a 500 error when the update receiver fails', function (done) { const path = '/projectName/here.txt' const req = { pause() {}, @@ -78,17 +87,18 @@ describe('TpdsController', function () { 'x-sl-update-source': (this.source = 'dropbox'), }, } - this.TpdsUpdateHandler.newUpdate = sinon - .stub() - .callsArgWith(5, 'update-receiver-error') + this.TpdsUpdateHandler.promises.newUpdate.rejects(new Error()) const res = { sendStatus: sinon.stub(), } - this.TpdsController.mergeUpdate(req, res) - res.sendStatus.calledWith(500).should.equal(true) + this.TpdsController.mergeUpdate(req, res, err => { + expect(err).to.exist + expect(res.sendStatus).not.to.have.been.called + done() + }) }) - it('should return a 400 error when the project is too big', function () { + it('should return a 400 error when the project is too big', function (done) { const path = '/projectName/here.txt' const req = { pause() {}, @@ -100,20 +110,22 @@ describe('TpdsController', function () { 'x-sl-update-source': (this.source = 'dropbox'), }, } - this.TpdsUpdateHandler.newUpdate = sinon - .stub() - .callsArgWith(5, { message: 'project_has_too_many_files' }) + this.TpdsUpdateHandler.promises.newUpdate.rejects({ + message: 'project_has_too_many_files', + }) const res = { - sendStatus: sinon.stub(), + sendStatus: status => { + expect(status).to.equal(400) + this.NotificationsBuilder.tpdsFileLimit.should.have.been.calledWith( + this.user_id + ) + done() + }, } this.TpdsController.mergeUpdate(req, res) - res.sendStatus.calledWith(400).should.equal(true) - this.NotificationsBuilder.tpdsFileLimit - .calledWith(this.user_id) - .should.equal(true) }) - it('should return a 429 error when the update receiver fails due to too many requests error', function () { + it('should return a 429 error when the update receiver fails due to too many requests error', function (done) { const path = '/projectName/here.txt' const req = { pause() {}, @@ -125,14 +137,16 @@ describe('TpdsController', function () { 'x-sl-update-source': (this.source = 'dropbox'), }, } - this.TpdsUpdateHandler.newUpdate = sinon - .stub() - .callsArgWith(5, new Errors.TooManyRequestsError('project on cooldown')) + this.TpdsUpdateHandler.promises.newUpdate.rejects( + new Errors.TooManyRequestsError('project on cooldown') + ) const res = { - sendStatus: sinon.stub(), + sendStatus: status => { + expect(status).to.equal(429) + done() + }, } this.TpdsController.mergeUpdate(req, res) - res.sendStatus.calledWith(429).should.equal(true) }) }) @@ -148,10 +162,9 @@ describe('TpdsController', function () { 'x-sl-update-source': (this.source = 'dropbox'), }, } - this.TpdsUpdateHandler.deleteUpdate = sinon.stub().callsArg(4) const res = { sendStatus: () => { - this.TpdsUpdateHandler.deleteUpdate + this.TpdsUpdateHandler.promises.deleteUpdate .calledWith(this.user_id, 'projectName', '/here.txt', this.source) .should.equal(true) done() @@ -190,8 +203,7 @@ describe('TpdsController', function () { }) describe('updateProjectContents', function () { - beforeEach(function () { - this.UpdateMerger.mergeUpdate = sinon.stub().callsArg(5) + beforeEach(function (done) { this.req = { params: { 0: (this.path = 'chapters/main.tex'), @@ -204,13 +216,17 @@ describe('TpdsController', function () { 'x-sl-update-source': (this.source = 'github'), }, } - this.res = { sendStatus: sinon.stub() } + this.res = { + sendStatus: sinon.stub().callsFake(() => { + done() + }), + } this.TpdsController.updateProjectContents(this.req, this.res, this.next) }) it('should merge the update', function () { - this.UpdateMerger.mergeUpdate + this.UpdateMerger.promises.mergeUpdate .calledWith( null, this.project_id, @@ -227,8 +243,7 @@ describe('TpdsController', function () { }) describe('deleteProjectContents', function () { - beforeEach(function () { - this.UpdateMerger.deleteUpdate = sinon.stub().callsArg(4) + beforeEach(function (done) { this.req = { params: { 0: (this.path = 'chapters/main.tex'), @@ -241,13 +256,17 @@ describe('TpdsController', function () { 'x-sl-update-source': (this.source = 'github'), }, } - this.res = { sendStatus: sinon.stub() } + this.res = { + sendStatus: sinon.stub().callsFake(() => { + done() + }), + } this.TpdsController.deleteProjectContents(this.req, this.res, this.next) }) it('should delete the file', function () { - this.UpdateMerger.deleteUpdate + this.UpdateMerger.promises.deleteUpdate .calledWith(null, this.project_id, `/${this.path}`, this.source) .should.equal(true) }) @@ -265,8 +284,11 @@ describe('TpdsController', function () { }) describe('success', function () { - beforeEach(async function () { - await this.TpdsController.getQueues(this.req, this.res, this.next) + beforeEach(function (done) { + this.res.json.callsFake(() => { + done() + }) + this.TpdsController.getQueues(this.req, this.res, this.next) }) it('should use userId from session', function () { @@ -283,12 +305,15 @@ describe('TpdsController', function () { }) describe('error', function () { - beforeEach(async function () { + beforeEach(function (done) { this.err = new Error() this.TpdsQueueManager.promises.getQueues = sinon .stub() .rejects(this.err) - await this.TpdsController.getQueues(this.req, this.res, this.next) + this.next.callsFake(() => { + done() + }) + this.TpdsController.getQueues(this.req, this.res, this.next) }) it('should call next with error', function () {