Merge pull request #9526 from overleaf/em-promisify-tpdsupdatehandler

Promisify TpdsUpdateHandler

GitOrigin-RevId: 3955a535e8d0b702751883d05c21c4c78717fed5

services/web/app/src/Features/Cooldown/CooldownManager.js

@@ -14,6 +14,7 @@ let CooldownManager
 const RedisWrapper = require('../../infrastructure/RedisWrapper')
 const rclient = RedisWrapper.client('cooldown')
 const logger = require('@overleaf/logger')
+const { promisifyAll } = require('../../util/promises')
 
 const COOLDOWN_IN_SECONDS = 60 * 10
 
@@ -54,3 +55,7 @@ module.exports = CooldownManager = {
     )
   },
 }
+
+module.exports.promises = promisifyAll(module.exports, {
+  without: ['_buildKey'],
+})

services/web/app/src/Features/Project/ProjectRootDocManager.js

@@ -24,6 +24,7 @@ const { promisify } = require('util')
 const async = require('async')
 const globby = require('globby')
 const _ = require('underscore')
+const { promisifyAll } = require('../../util/promises')
 
 module.exports = ProjectRootDocManager = {
   setRootDocAutomatically(project_id, callback) {
@@ -300,27 +301,10 @@ module.exports = ProjectRootDocManager = {
   },
 }
 
-const promises = {
-  setRootDocAutomatically: promisify(
-    ProjectRootDocManager.setRootDocAutomatically
-  ),
-
-  findRootDocFileFromDirectory: directoryPath =>
-    new Promise((resolve, reject) => {
-      ProjectRootDocManager.findRootDocFileFromDirectory(
-        directoryPath,
-        (error, path, content) => {
-          if (error) {
-            reject(error)
-          } else {
-            resolve({ path, content })
-          }
-        }
-      )
-    }),
-  setRootDocFromName: promisify(ProjectRootDocManager.setRootDocFromName),
-}
-
-ProjectRootDocManager.promises = promises
-
 module.exports = ProjectRootDocManager
+module.exports.promises = promisifyAll(module.exports, {
+  without: ['_rootDocSort'],
+  multiResult: {
+    findRootDocFileFromDirectory: ['path', 'content'],
+  },
+})

services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js

@@ -1,4 +1,4 @@
-const { promisify } = require('util')
+const { callbackify } = require('util')
 const UpdateMerger = require('./UpdateMerger')
 const logger = require('@overleaf/logger')
 const NotificationsBuilder = require('../Notifications/NotificationsBuilder')
@@ -19,157 +19,135 @@ const ROOT_DOC_TIMEOUT_LENGTH = 30 * 1000
 
 const rootDocResets = new BackgroundTaskTracker('root doc resets')
 
-function newUpdate(userId, projectName, path, updateRequest, source, callback) {
-  getOrCreateProject(userId, projectName, (err, project) => {
-    if (err) {
-      return callback(err)
-    }
-    if (project == null) {
-      return callback()
-    }
-    CooldownManager.isProjectOnCooldown(
-      project._id,
-      (err, projectIsOnCooldown) => {
-        if (err) {
-          return callback(err)
-        }
-        if (projectIsOnCooldown) {
-          return callback(
-            new Errors.TooManyRequestsError('project on cooldown')
-          )
-        }
-        FileTypeManager.shouldIgnore(path, (err, shouldIgnore) => {
-          if (err) {
-            return callback(err)
-          }
-          if (shouldIgnore) {
-            return callback()
-          }
-          UpdateMerger.mergeUpdate(
-            userId,
-            project._id,
-            path,
-            updateRequest,
-            source,
-            callback
-          )
-        })
-      }
-    )
-  })
+async function newUpdate(userId, projectName, path, updateRequest, source) {
+  const project = await getOrCreateProject(userId, projectName)
+  if (project == null) {
+    return
+  }
+
+  const projectIsOnCooldown =
+    await CooldownManager.promises.isProjectOnCooldown(project._id)
+  if (projectIsOnCooldown) {
+    throw new Errors.TooManyRequestsError('project on cooldown')
+  }
+
+  const shouldIgnore = await FileTypeManager.promises.shouldIgnore(path)
+  if (shouldIgnore) {
+    return
+  }
+
+  await UpdateMerger.promises.mergeUpdate(
+    userId,
+    project._id,
+    path,
+    updateRequest,
+    source
+  )
 }
 
-function deleteUpdate(userId, projectName, path, source, callback) {
+async function deleteUpdate(userId, projectName, path, source) {
   logger.debug({ userId, filePath: path }, 'handling delete update from tpds')
-  ProjectGetter.findUsersProjectsByName(
+
+  const projects = await ProjectGetter.promises.findUsersProjectsByName(
     userId,
-    projectName,
-    (err, projects) => {
-      if (err) {
-        return callback(err)
-      }
-      const activeProjects = projects.filter(
-        project => !ProjectHelper.isArchivedOrTrashed(project, userId)
-      )
-      if (activeProjects.length === 0) {
-        logger.debug(
-          { userId, filePath: path, projectName },
-          'project not found from tpds update, ignoring folder or project'
-        )
-        return callback()
-      }
-      if (projects.length > 1) {
-        // There is more than one project with that name, and one of them is
-        // active (previous condition)
-        return handleDuplicateProjects(userId, projectName, callback)
-      }
-
-      const project = activeProjects[0]
-      if (path === '/') {
-        logger.debug(
-          { userId, filePath: path, projectName, project_id: project._id },
-          'project found for delete update, path is root so marking project as deleted'
-        )
-        ProjectDeleter.markAsDeletedByExternalSource(project._id, callback)
-      } else {
-        UpdateMerger.deleteUpdate(userId, project._id, path, source, err => {
-          callback(err)
-        })
-      }
-    }
+    projectName
+  )
+  const activeProjects = projects.filter(
+    project => !ProjectHelper.isArchivedOrTrashed(project, userId)
   )
+
+  if (activeProjects.length === 0) {
+    logger.debug(
+      { userId, filePath: path, projectName },
+      'project not found from tpds update, ignoring folder or project'
+    )
+    return
+  }
+
+  if (projects.length > 1) {
+    // There is more than one project with that name, and one of them is
+    // active (previous condition)
+    await handleDuplicateProjects(userId, projectName)
+    return
+  }
+
+  const project = activeProjects[0]
+  if (path === '/') {
+    logger.debug(
+      { userId, filePath: path, projectName, project_id: project._id },
+      'project found for delete update, path is root so marking project as deleted'
+    )
+    await ProjectDeleter.promises.markAsDeletedByExternalSource(project._id)
+  } else {
+    await UpdateMerger.promises.deleteUpdate(userId, project._id, path, source)
+  }
 }
 
-function getOrCreateProject(userId, projectName, callback) {
-  ProjectGetter.findUsersProjectsByName(
+async function getOrCreateProject(userId, projectName) {
+  const projects = await ProjectGetter.promises.findUsersProjectsByName(
     userId,
-    projectName,
-    (err, projects) => {
-      if (err) {
-        return callback(err)
-      }
-
-      if (projects.length === 0) {
-        // No project with that name -- active, archived or trashed -- has been
-        // found. Create one.
-        return ProjectCreationHandler.createBlankProject(
-          userId,
-          projectName,
-          (err, project) => {
-            // have a crack at setting the root doc after a while, on creation
-            // we won't have it yet, but should have been sent it it within 30
-            // seconds
-            rootDocResets.add()
-            setTimeout(() => {
-              ProjectRootDocManager.setRootDocAutomatically(project._id, () => {
-                rootDocResets.done()
-              })
-            }, ROOT_DOC_TIMEOUT_LENGTH)
-            callback(err, project)
-          }
-        )
-      }
-      const activeProjects = projects.filter(
-        project => !ProjectHelper.isArchivedOrTrashed(project, userId)
-      )
-      if (activeProjects.length === 0) {
-        // All projects with that name are archived or trashed. Ignore.
-        return callback(null, null)
-      }
-
-      if (projects.length > 1) {
-        // There is more than one project with that name, and one of them is
-        // active (previous condition)
-        return handleDuplicateProjects(userId, projectName, err => {
-          if (err) {
-            return callback(err)
-          }
-          callback(null, null)
+    projectName
+  )
+
+  if (projects.length === 0) {
+    // No project with that name -- active, archived or trashed -- has been
+    // found. Create one.
+    const project = await ProjectCreationHandler.promises.createBlankProject(
+      userId,
+      projectName
+    )
+
+    // have a crack at setting the root doc after a while, on creation
+    // we won't have it yet, but should have been sent it it within 30
+    // seconds
+    rootDocResets.add()
+    setTimeout(() => {
+      ProjectRootDocManager.promises
+        .setRootDocAutomatically(project._id)
+        .then(() => {
+          rootDocResets.done()
         })
-      }
+        .catch(err => {
+          logger.warn({ err }, 'failed to set root doc after project creation')
+        })
+    }, ROOT_DOC_TIMEOUT_LENGTH)
+    return project
+  }
 
-      callback(err, activeProjects[0])
-    }
+  const activeProjects = projects.filter(
+    project => !ProjectHelper.isArchivedOrTrashed(project, userId)
   )
+  if (activeProjects.length === 0) {
+    // All projects with that name are archived or trashed. Ignore.
+    return null
+  }
+
+  if (projects.length > 1) {
+    // There is more than one project with that name, and one of them is
+    // active (previous condition)
+    await handleDuplicateProjects(userId, projectName)
+    return null
+  }
+
+  return activeProjects[0]
 }
 
-function handleDuplicateProjects(userId, projectName, callback) {
-  Modules.hooks.fire('removeDropbox', userId, 'duplicate-projects', err => {
-    if (err) {
-      return callback(err)
-    }
-    NotificationsBuilder.dropboxDuplicateProjectNames(userId).create(
-      projectName,
-      callback
-    )
-  })
+async function handleDuplicateProjects(userId, projectName) {
+  await Modules.promises.hooks.fire(
+    'removeDropbox',
+    userId,
+    'duplicate-projects'
+  )
+  await NotificationsBuilder.promises
+    .dropboxDuplicateProjectNames(userId)
+    .create(projectName)
 }
 
 module.exports = {
-  newUpdate,
-  deleteUpdate,
+  newUpdate: callbackify(newUpdate),
+  deleteUpdate: callbackify(deleteUpdate),
   promises: {
-    newUpdate: promisify(newUpdate),
-    deleteUpdate: promisify(deleteUpdate),
+    newUpdate,
+    deleteUpdate,
   },
 }

services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js

@@ -38,28 +38,42 @@ describe('TpdsUpdateHandler', function () {
     this.update = {}
 
     this.CooldownManager = {
-      isProjectOnCooldown: sinon.stub().yields(null, false),
+      promises: {
+        isProjectOnCooldown: sinon.stub().resolves(false),
+      },
     }
     this.FileTypeManager = {
-      shouldIgnore: sinon.stub().yields(null, false),
+      promises: {
+        shouldIgnore: sinon.stub().resolves(false),
+      },
     }
     this.Modules = {
-      hooks: { fire: sinon.stub().yields() },
+      promises: {
+        hooks: { fire: sinon.stub().resolves() },
+      },
     }
     this.notification = {
-      create: sinon.stub().yields(),
+      create: sinon.stub().resolves(),
     }
     this.NotificationsBuilder = {
-      dropboxDuplicateProjectNames: sinon.stub().returns(this.notification),
+      promises: {
+        dropboxDuplicateProjectNames: sinon.stub().returns(this.notification),
+      },
     }
     this.ProjectCreationHandler = {
-      createBlankProject: sinon.stub().yields(null, this.projects.active1),
+      promises: {
+        createBlankProject: sinon.stub().resolves(this.projects.active1),
+      },
     }
     this.ProjectDeleter = {
-      markAsDeletedByExternalSource: sinon.stub().yields(),
+      promises: {
+        markAsDeletedByExternalSource: sinon.stub().resolves(),
+      },
     }
     this.ProjectGetter = {
-      findUsersProjectsByName: sinon.stub(),
+      promises: {
+        findUsersProjectsByName: sinon.stub(),
+      },
     }
     this.ProjectHelper = {
       isArchivedOrTrashed: sinon.stub().returns(false),
@@ -70,10 +84,16 @@ describe('TpdsUpdateHandler', function () {
     this.ProjectHelper.isArchivedOrTrashed
       .withArgs(this.projects.archived2, this.userId)
       .returns(true)
-    this.RootDocManager = { setRootDocAutomatically: sinon.stub() }
+    this.RootDocManager = {
+      promises: {
+        setRootDocAutomatically: sinon.stub().resolves(),
+      },
+    }
     this.UpdateMerger = {
-      deleteUpdate: sinon.stub().yields(),
-      mergeUpdate: sinon.stub().yields(),
+      promises: {
+        deleteUpdate: sinon.stub().resolves(),
+        mergeUpdate: sinon.stub().resolves(),
+      },
     }
 
     this.TpdsUpdateHandler = SandboxedModule.require(MODULE_PATH, {
@@ -139,10 +159,10 @@ describe('TpdsUpdateHandler', function () {
       expectDropboxUnlinked()
     })
 
-    describe('update to a file that should be ignored', function (done) {
+    describe('update to a file that should be ignored', async function () {
       setupMatchingProjects(['active1'])
       beforeEach(function () {
-        this.FileTypeManager.shouldIgnore.yields(null, true)
+        this.FileTypeManager.promises.shouldIgnore.resolves(true)
       })
       receiveUpdate()
       expectProjectNotCreated()
@@ -150,21 +170,19 @@ describe('TpdsUpdateHandler', function () {
       expectDropboxNotUnlinked()
     })
 
-    describe('update to a project on cooldown', function (done) {
+    describe('update to a project on cooldown', async function () {
       setupMatchingProjects(['active1'])
       setupProjectOnCooldown()
-      beforeEach(function (done) {
-        this.TpdsUpdateHandler.newUpdate(
-          this.userId,
-          this.projectName,
-          this.path,
-          this.update,
-          this.source,
-          err => {
-            expect(err).to.be.instanceof(Errors.TooManyRequestsError)
-            done()
-          }
-        )
+      beforeEach(async function () {
+        await expect(
+          this.TpdsUpdateHandler.promises.newUpdate(
+            this.userId,
+            this.projectName,
+            this.path,
+            this.update,
+            this.source
+          )
+        ).to.be.rejectedWith(Errors.TooManyRequestsError)
       })
       expectUpdateNotProcessed()
     })
@@ -270,55 +288,52 @@ describe('TpdsUpdateHandler', function () {
 function setupMatchingProjects(projectKeys) {
   beforeEach(function () {
     const projects = projectKeys.map(key => this.projects[key])
-    this.ProjectGetter.findUsersProjectsByName
+    this.ProjectGetter.promises.findUsersProjectsByName
       .withArgs(this.userId, this.projectName)
-      .yields(null, projects)
+      .resolves(projects)
   })
 }
 
 function setupProjectOnCooldown() {
   beforeEach(function () {
-    this.CooldownManager.isProjectOnCooldown
+    this.CooldownManager.promises.isProjectOnCooldown
       .withArgs(this.projects.active1._id)
-      .yields(null, true)
+      .resolves(true)
   })
 }
 
 /* Test helpers */
 
 function receiveUpdate() {
-  beforeEach(function (done) {
-    this.TpdsUpdateHandler.newUpdate(
+  beforeEach(async function () {
+    await this.TpdsUpdateHandler.promises.newUpdate(
       this.userId,
       this.projectName,
       this.path,
       this.update,
-      this.source,
-      done
+      this.source
     )
   })
 }
 
 function receiveFileDelete() {
-  beforeEach(function (done) {
-    this.TpdsUpdateHandler.deleteUpdate(
+  beforeEach(async function () {
+    await this.TpdsUpdateHandler.promises.deleteUpdate(
       this.userId,
       this.projectName,
       this.path,
-      this.source,
-      done
+      this.source
     )
   })
 }
 
 function receiveProjectDelete() {
-  beforeEach(function (done) {
-    this.TpdsUpdateHandler.deleteUpdate(
+  beforeEach(async function () {
+    await this.TpdsUpdateHandler.promises.deleteUpdate(
       this.userId,
       this.projectName,
       '/',
-      this.source,
-      done
+      this.source
     )
   })
 }
@@ -328,35 +343,36 @@ function receiveProjectDelete() {
 function expectProjectCreated() {
   it('creates a project', function () {
     expect(
-      this.ProjectCreationHandler.createBlankProject
+      this.ProjectCreationHandler.promises.createBlankProject
     ).to.have.been.calledWith(this.userId, this.projectName)
   })
 
   it('sets the root doc', function () {
     // Fire pending timers
     this.clock.runAll()
-    expect(this.RootDocManager.setRootDocAutomatically).to.have.been.calledWith(
-      this.projects.active1._id
-    )
+    expect(
+      this.RootDocManager.promises.setRootDocAutomatically
+    ).to.have.been.calledWith(this.projects.active1._id)
   })
 }
 
 function expectProjectNotCreated() {
   it('does not create a project', function () {
-    expect(this.ProjectCreationHandler.createBlankProject).not.to.have.been
-      .called
+    expect(this.ProjectCreationHandler.promises.createBlankProject).not.to.have
+      .been.called
   })
 
   it('does not set the root doc', function () {
     // Fire pending timers
     this.clock.runAll()
-    expect(this.RootDocManager.setRootDocAutomatically).not.to.have.been.called
+    expect(this.RootDocManager.promises.setRootDocAutomatically).not.to.have
+      .been.called
   })
 }
 
 function expectUpdateProcessed() {
   it('processes the update', function () {
-    expect(this.UpdateMerger.mergeUpdate).to.have.been.calledWith(
+    expect(this.UpdateMerger.promises.mergeUpdate).to.have.been.calledWith(
       this.userId,
       this.projects.active1._id,
       this.path,
@@ -368,13 +384,13 @@ function expectUpdateProcessed() {
 
 function expectUpdateNotProcessed() {
   it('does not process the update', function () {
-    expect(this.UpdateMerger.mergeUpdate).not.to.have.been.called
+    expect(this.UpdateMerger.promises.mergeUpdate).not.to.have.been.called
   })
 }
 
 function expectDropboxUnlinked() {
   it('unlinks Dropbox', function () {
-    expect(this.Modules.hooks.fire).to.have.been.calledWith(
+    expect(this.Modules.promises.hooks.fire).to.have.been.calledWith(
       'removeDropbox',
       this.userId,
       'duplicate-projects'
@@ -383,7 +399,7 @@ function expectDropboxUnlinked() {
 
   it('creates a notification that dropbox was unlinked', function () {
     expect(
-      this.NotificationsBuilder.dropboxDuplicateProjectNames
+      this.NotificationsBuilder.promises.dropboxDuplicateProjectNames
     ).to.have.been.calledWith(this.userId)
     expect(this.notification.create).to.have.been.calledWith(this.projectName)
   })
@@ -391,18 +407,18 @@ function expectDropboxUnlinked() {
 
 function expectDropboxNotUnlinked() {
   it('does not unlink Dropbox', function () {
-    expect(this.Modules.hooks.fire).not.to.have.been.called
+    expect(this.Modules.promises.hooks.fire).not.to.have.been.called
   })
 
   it('does not create a notification that dropbox was unlinked', function () {
-    expect(this.NotificationsBuilder.dropboxDuplicateProjectNames).not.to.have
-      .been.called
+    expect(this.NotificationsBuilder.promises.dropboxDuplicateProjectNames).not
+      .to.have.been.called
   })
 }
 
 function expectDeleteProcessed() {
   it('processes the delete', function () {
-    expect(this.UpdateMerger.deleteUpdate).to.have.been.calledWith(
+    expect(this.UpdateMerger.promises.deleteUpdate).to.have.been.calledWith(
       this.userId,
       this.projects.active1._id,
       this.path,
@@ -413,21 +429,21 @@ function expectDeleteProcessed() {
 
 function expectDeleteNotProcessed() {
   it('does not process the delete', function () {
-    expect(this.UpdateMerger.deleteUpdate).not.to.have.been.called
+    expect(this.UpdateMerger.promises.deleteUpdate).not.to.have.been.called
   })
 }
 
 function expectProjectDeleted() {
   it('deletes the project', function () {
     expect(
-      this.ProjectDeleter.markAsDeletedByExternalSource
+      this.ProjectDeleter.promises.markAsDeletedByExternalSource
     ).to.have.been.calledWith(this.projects.active1._id)
   })
 }
 
 function expectProjectNotDeleted() {
   it('does not delete the project', function () {
-    expect(this.ProjectDeleter.markAsDeletedByExternalSource).not.to.have.been
-      .called
+    expect(this.ProjectDeleter.promises.markAsDeletedByExternalSource).not.to
+      .have.been.called
   })
 }