Merge pull request #9472 from overleaf/em-promisify-tpdscontroller

Promisify TpdsController

GitOrigin-RevId: 35d1db628c44d39ee71bb3127cb25ece6d184457

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'
-
-    TpdsUpdateHandler.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)
-        }
-      }
-    )
-  },
+// 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.
 
-  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(
+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,
-      source,
-      err => {
-        if (err) {
-          logger.err(
-            { err, userId, filePath },
-            'error receiving update from tpds'
-          )
-          res.sendStatus(500)
-        } else {
-          res.sendStatus(200)
-        }
-      }
+      req,
+      source
     )
-  },
-
-  // 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)
+  } 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 {
+      throw err
     }
-  },
+  }
+
+  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.
 
-  parseParams: (parseParams = function (req) {
-    let filePath, projectName
-    let path = req.params[0]
-    const userId = req.params.user_id
+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'
 
-    path = Path.join('/', path)
-    if (path.substring(1).indexOf('/') === -1) {
-      filePath = '/'
-      projectName = path.substring(1)
+  try {
+    await UpdateMerger.promises.mergeUpdate(null, projectId, path, req, source)
+  } catch (error) {
+    if (error.constructor === Errors.InvalidNameError) {
+      return res.sendStatus(422)
     } else {
-      filePath = path.substring(path.indexOf('/', 1))
-      projectName = path.substring(0, path.indexOf('/', 1))
-      projectName = projectName.replace('/', '')
+      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),
 
-    return { filePath, userId, projectName }
-  }),
+  // for tests only
+  parseParams,
 }

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),
+  },
 }

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)

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