Merge pull request #1910 from overleaf/msm-project-cleanedup-after-failed-upload

Project is cleaned up after zip upload fails

GitOrigin-RevId: a09bb51161d0565a5c7eb55a5e29530f8ec65eb6

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

@@ -14,7 +14,7 @@
  * DS207: Consider shorter variations of null checks
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
-let ProjectDeleter
+const { promisify } = require('util')
 const { Project } = require('../../models/Project')
 const { DeletedProject } = require('../../models/DeletedProject')
 const logger = require('logger-sharelatex')
@@ -24,7 +24,7 @@ const async = require('async')
 const FileStoreHandler = require('../FileStore/FileStoreHandler')
 const CollaboratorsHandler = require('../Collaborators/CollaboratorsHandler')
 
-module.exports = ProjectDeleter = {
+const ProjectDeleter = {
   markAsDeletedByExternalSource(project_id, callback) {
     if (callback == null) {
       callback = function(error) {}
@@ -189,3 +189,11 @@ module.exports = ProjectDeleter = {
     )
   }
 }
+
+const promises = {
+  deleteProject: promisify(ProjectDeleter.deleteProject)
+}
+
+ProjectDeleter.promises = promises
+
+module.exports = ProjectDeleter

services/web/app/src/Features/Uploads/ProjectUploadManager.js

@@ -20,7 +20,9 @@ const FileSystemImportManager = require('./FileSystemImportManager')
 const ProjectCreationHandler = require('../Project/ProjectCreationHandler')
 const ProjectRootDocManager = require('../Project/ProjectRootDocManager')
 const ProjectDetailsHandler = require('../Project/ProjectDetailsHandler')
+const ProjectDeleter = require('../Project/ProjectDeleter').promises
 const DocumentHelper = require('../Documents/DocumentHelper')
+const logger = require('logger-sharelatex')
 
 const ProjectUploadManager = {
   createProjectFromZipArchive(ownerId, defaultName, zipPath, callback) {
@@ -143,17 +145,30 @@ const promises = {
       ownerId,
       uniqueName
     )
-    await ProjectUploadManager.promises._insertZipContentsIntoFolder(
-      ownerId,
-      project._id,
-      project.rootFolder[0]._id,
-      destination
-    )
+    try {
+      await ProjectUploadManager.promises._insertZipContentsIntoFolder(
+        ownerId,
+        project._id,
+        project.rootFolder[0]._id,
+        destination
+      )
 
-    if (path) {
-      await ProjectRootDocManager.promises.setRootDocFromName(project._id, path)
+      if (path) {
+        await ProjectRootDocManager.promises.setRootDocFromName(
+          project._id,
+          path
+        )
+      }
+    } catch (err) {
+      // no need to wait for the cleanup here
+      ProjectDeleter.deleteProject(project._id).catch(err =>
+        logger.error(
+          { err, projectId: project._id },
+          'there was an error cleaning up project after importing a zip failed'
+        )
+      )
+      throw err
     }
-
     return project
   },
 
@@ -176,14 +191,25 @@ const promises = {
       projectName,
       attributes
     )
-    await ProjectUploadManager.promises.insertZipArchiveIntoFolder(
-      ownerId,
-      project._id,
-      project.rootFolder[0]._id,
-      zipPath
-    )
 
-    await ProjectRootDocManager.promises.setRootDocAutomatically(project._id)
+    try {
+      await ProjectUploadManager.promises.insertZipArchiveIntoFolder(
+        ownerId,
+        project._id,
+        project.rootFolder[0]._id,
+        zipPath
+      )
+      await ProjectRootDocManager.promises.setRootDocAutomatically(project._id)
+    } catch (err) {
+      // no need to wait for the cleanup here
+      ProjectDeleter.deleteProject(project._id).catch(err =>
+        logger.error(
+          { err, projectId: project._id },
+          'there was an error cleaning up project after importing a zip failed'
+        )
+      )
+      throw err
+    }
 
     return project
   },

services/web/test/unit/src/Uploads/ProjectUploadManagerTests.js

@@ -17,6 +17,7 @@ const modulePath =
 const SandboxedModule = require('sandboxed-module')
 
 const promiseStub = val => new Promise(resolve => resolve(val))
+const failedPromiseStub = err => new Promise((resolve, reject) => reject(err))
 
 describe('ProjectUploadManager', function() {
   beforeEach(function() {
@@ -47,6 +48,9 @@ describe('ProjectUploadManager', function() {
         '../Project/ProjectDetailsHandler': (this.ProjectDetailsHandler = {
           promises: {}
         }),
+        '../Project/ProjectDeleter': (this.ProjectDeleter = {
+          promises: {}
+        }),
         '../Documents/DocumentHelper': (this.DocumentHelper = {}),
         rimraf: (this.rimraf = sinon.stub().callsArg(1))
       }
@@ -211,7 +215,7 @@ describe('ProjectUploadManager', function() {
       this.ProjectUploadManager.promises.insertZipArchiveIntoFolder = sinon
         .stub()
         .returns(promiseStub())
-      return this.ProjectUploadManager.createProjectFromZipArchiveWithName(
+      this.ProjectUploadManager.createProjectFromZipArchiveWithName(
         this.owner_id,
         this.name,
         this.source,
@@ -256,6 +260,71 @@ describe('ProjectUploadManager', function() {
         .calledWith(sinon.match.falsy, this.project)
         .should.equal(true)
     })
+    describe('when inserting the zip file contents into the root folder fails', function() {
+      beforeEach(function(done) {
+        this.callback = sinon.stub()
+        this.ProjectUploadManager.promises.insertZipArchiveIntoFolder = sinon
+          .stub()
+          .returns(failedPromiseStub('insert-zip-error'))
+        this.ProjectDeleter.promises.deleteProject = sinon
+          .stub()
+          .returns(promiseStub())
+        this.ProjectUploadManager.createProjectFromZipArchiveWithName(
+          this.owner_id,
+          this.name,
+          this.source,
+          (err, project) => {
+            this.callback(err, project)
+            return done()
+          }
+        )
+      })
+
+      it('should pass an error to the callback', function() {
+        return this.callback
+          .calledWith('insert-zip-error', sinon.match.falsy)
+          .should.equal(true)
+      })
+
+      it('should cleanup the blank project created', function() {
+        return this.ProjectDeleter.promises.deleteProject
+          .calledWith(this.project_id)
+          .should.equal(true)
+      })
+    })
+
+    describe('when setting automatically the root doc fails', function() {
+      beforeEach(function(done) {
+        this.callback = sinon.stub()
+        this.ProjectRootDocManager.promises.setRootDocAutomatically = sinon
+          .stub()
+          .returns(failedPromiseStub('set-root-auto-error'))
+        this.ProjectDeleter.promises.deleteProject = sinon
+          .stub()
+          .returns(promiseStub())
+        this.ProjectUploadManager.createProjectFromZipArchiveWithName(
+          this.owner_id,
+          this.name,
+          this.source,
+          (err, project) => {
+            this.callback(err, project)
+            return done()
+          }
+        )
+      })
+
+      it('should pass an error to the callback', function() {
+        return this.callback
+          .calledWith('set-root-auto-error', sinon.match.falsy)
+          .should.equal(true)
+      })
+
+      it('should cleanup the blank project created', function() {
+        return this.ProjectDeleter.promises.deleteProject
+          .calledWith(this.project_id)
+          .should.equal(true)
+      })
+    })
   })
 
   describe('insertZipArchiveIntoFolder', function() {