Merge pull request #3571 from overleaf/jpa-ask-docstore-is-doc-deleted

[ProjectEntityUpdateHandler] ask docstore whether a doc exists/isDeleted

GitOrigin-RevId: 54c6666b514b466b908b9ed57a26bc6cf66037d7

services/web/app/src/Features/Docstore/DocstoreManager.js

@@ -169,6 +169,35 @@ const DocstoreManager = {
     )
   },
 
+  isDocDeleted(project_id, doc_id, callback) {
+    const url = `${settings.apis.docstore.url}/project/${project_id}/doc/${doc_id}/deleted`
+    request.get({ url, timeout: TIMEOUT, json: true }, function(
+      err,
+      res,
+      body
+    ) {
+      if (err) {
+        callback(err)
+      } else if (res.statusCode === 200) {
+        callback(null, body.deleted)
+      } else if (res.statusCode === 404) {
+        callback(
+          new Errors.NotFoundError({
+            message: 'doc does not exist in project',
+            info: { project_id, doc_id }
+          })
+        )
+      } else {
+        callback(
+          new OError(
+            `docstore api responded with non-success code: ${res.statusCode}`,
+            { project_id, doc_id }
+          )
+        )
+      }
+    })
+  },
+
   updateDoc(project_id, doc_id, lines, version, ranges, callback) {
     if (callback == null) {
       callback = function(error, modified, rev) {}

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

@@ -70,6 +70,86 @@ function wrapWithLock(methodWithoutLock) {
   }
 }
 
+function getDocContext(projectId, docId, callback) {
+  ProjectGetter.getProject(
+    projectId,
+    { name: true, rootFolder: true },
+    (err, project) => {
+      if (err) {
+        return callback(
+          OError.tag(err, 'error fetching project', {
+            projectId
+          })
+        )
+      }
+      if (!project) {
+        return callback(new Errors.NotFoundError('project not found'))
+      }
+      ProjectLocator.findElement(
+        { project, element_id: docId, type: 'docs' },
+        (err, doc, path) => {
+          if (err && err instanceof Errors.NotFoundError) {
+            // (Soft-)Deleted docs are removed from the file-tree (rootFolder).
+            // docstore can tell whether it exists and is (soft)-deleted.
+            DocstoreManager.isDocDeleted(
+              projectId,
+              docId,
+              (err, isDeletedDoc) => {
+                if (err && err instanceof Errors.NotFoundError) {
+                  logger.warn(
+                    { projectId, docId },
+                    'doc not found while updating doc lines'
+                  )
+                  callback(err)
+                } else if (err) {
+                  callback(
+                    OError.tag(
+                      err,
+                      'error checking deletion status with docstore',
+                      { projectId, docId }
+                    )
+                  )
+                } else {
+                  if (!isDeletedDoc) {
+                    // NOTE: This can happen while we delete a doc:
+                    //  1. web will update the projects entry
+                    //  2. web triggers flushes to tpds/doc-updater
+                    //  3. web triggers (soft)-delete in docstore
+                    // Specifically when an update comes in after 1
+                    //  and before 3 completes.
+                    logger.info(
+                      { projectId, docId },
+                      'updating doc that is in process of getting soft-deleted'
+                    )
+                  }
+                  callback(null, {
+                    projectName: project.name,
+                    isDeletedDoc: true,
+                    path: null
+                  })
+                }
+              }
+            )
+          } else if (err) {
+            callback(
+              OError.tag(err, 'error finding doc in rootFolder', {
+                docId,
+                projectId
+              })
+            )
+          } else {
+            callback(null, {
+              projectName: project.name,
+              isDeletedDoc: false,
+              path: path.fileSystem
+            })
+          }
+        }
+      )
+    }
+  )
+}
+
 const ProjectEntityUpdateHandler = {
   updateDocLines(
     projectId,
@@ -81,86 +161,57 @@ const ProjectEntityUpdateHandler = {
     lastUpdatedBy,
     callback
   ) {
-    ProjectGetter.getProjectWithoutDocLines(projectId, (err, project) => {
+    getDocContext(projectId, docId, (err, ctx) => {
-      if (err != null) {
+      if (err && err instanceof Errors.NotFoundError) {
+        // Do not allow an update to a doc which has never exist on this project
+        logger.warn(
+          { docId, projectId },
+          'project or doc not found while updating doc lines'
+        )
         return callback(err)
       }
-      if (project == null) {
+      if (err) {
-        return callback(new Errors.NotFoundError('project not found'))
+        return callback(err)
       }
-      logger.log({ projectId, docId }, 'updating doc lines')
+      const { projectName, isDeletedDoc, path } = ctx
-      ProjectLocator.findElement(
+      logger.log({ projectId, docId }, 'telling docstore manager to update doc')
-        { project, element_id: docId, type: 'docs' },
+      DocstoreManager.updateDoc(
-        (err, doc, path) => {
+        projectId,
-          let isDeletedDoc = false
+        docId,
+        lines,
+        version,
+        ranges,
+        (err, modified, rev) => {
           if (err != null) {
-            if (err instanceof Errors.NotFoundError) {
+            OError.tag(err, 'error sending doc to docstore', {
-              // We need to be able to update the doclines of deleted docs. This is
+              docId,
-              // so the doc-updater can flush a doc's content to the doc-store after
+              projectId
-              // the doc is deleted.
+            })
-              isDeletedDoc = true
+            return callback(err)
-              doc = _.find(
-                project.deletedDocs,
-                doc => doc._id.toString() === docId.toString()
-              )
-            } else {
-              return callback(err)
-            }
           }
-
-          if (doc == null) {
-            // Do not allow an update to a doc which has never exist on this project
-            logger.warn(
-              { docId, projectId },
-              'doc not found while updating doc lines'
-            )
-            return callback(new Errors.NotFoundError('doc not found'))
-          }
-
           logger.log(
-            { projectId, docId },
+            { projectId, docId, modified },
-            'telling docstore manager to update doc'
+            'finished updating doc lines'
           )
-          DocstoreManager.updateDoc(
+          // path will only be present if the doc is not deleted
+          if (!modified || isDeletedDoc) {
+            return callback()
+          }
+          // Don't need to block for marking as updated
+          ProjectUpdateHandler.markAsUpdated(
             projectId,
-            docId,
+            lastUpdatedAt,
-            lines,
+            lastUpdatedBy
-            version,
+          )
-            ranges,
+          TpdsUpdateSender.addDoc(
-            (err, modified, rev) => {
+            {
-              if (err != null) {
+              project_id: projectId,
-                OError.tag(err, 'error sending doc to docstore', {
+              path,
-                  docId,
+              doc_id: docId,
-                  projectId
+              project_name: projectName,
-                })
+              rev
-                return callback(err)
+            },
-              }
+            callback
-              logger.log(
-                { projectId, docId, modified },
-                'finished updating doc lines'
-              )
-              // path will only be present if the doc is not deleted
-              if (modified && !isDeletedDoc) {
-                // Don't need to block for marking as updated
-                ProjectUpdateHandler.markAsUpdated(
-                  projectId,
-                  lastUpdatedAt,
-                  lastUpdatedBy
-                )
-                TpdsUpdateSender.addDoc(
-                  {
-                    project_id: projectId,
-                    path: path.fileSystem,
-                    doc_id: docId,
-                    project_name: project.name,
-                    rev
-                  },
-                  callback
-                )
-              } else {
-                callback()
-              }
-            }
           )
         }
       )

services/web/test/acceptance/src/DocUpdateTests.js

@@ -0,0 +1,158 @@
+const User = require('./helpers/User')
+const request = require('./helpers/request')
+const { expect } = require('chai')
+const settings = require('settings-sharelatex')
+const { ObjectId } = require('mongodb')
+require('./helpers/MockDocstoreApi')
+require('./helpers/MockV1Api')
+require('./helpers/MockProjectHistoryApi')
+
+describe('DocUpdate', function() {
+  beforeEach(function(done) {
+    this.user = new User()
+    this.projectName = 'wombat'
+    this.user.ensureUserExists(() => {
+      this.user.login(() => {
+        this.user.createProject(this.projectName, (error, projectId) => {
+          if (error) return done(error)
+          this.projectId = projectId
+
+          this.user.getProject(this.projectId, (error, project) => {
+            if (error) return done(error)
+            this.project = project
+            this.user.createDocInProject(
+              this.projectId,
+              this.project.rootFolder[0]._id,
+              'potato',
+              (error, docId) => {
+                this.docId = docId
+                done(error)
+              }
+            )
+          })
+        })
+      })
+    })
+  })
+
+  function writeContent(
+    { projectId, docId, lines, version, ranges, lastUpdatedAt, lastUpdatedBy },
+    callback
+  ) {
+    request(
+      {
+        method: 'POST',
+        url: `/project/${projectId}/doc/${docId}`,
+        auth: {
+          user: settings.apis.web.user,
+          pass: settings.apis.web.pass,
+          sendImmediately: true
+        },
+        json: { lines, version, ranges, lastUpdatedAt, lastUpdatedBy }
+      },
+      (error, res) => {
+        if (error) return callback(error)
+        if (res.statusCode !== 200)
+          return callback(
+            new Error(`non-success statusCode: ${res.statusCode}`)
+          )
+        callback()
+      }
+    )
+  }
+
+  function updateContent(options, callback) {
+    writeContent(options, err => {
+      if (err) return callback(err)
+
+      options.lines.push('foo')
+      options.version++
+      writeContent(options, callback)
+    })
+  }
+
+  function writeContentTwice(options, callback) {
+    writeContent(options, err => {
+      if (err) return callback(err)
+
+      writeContent(options, callback)
+    })
+  }
+
+  let writeOptions
+  beforeEach(function() {
+    writeOptions = {
+      projectId: this.projectId,
+      docId: this.docId,
+      lines: ['a'],
+      version: 1,
+      ranges: {},
+      lastUpdatedAt: new Date(),
+      lastUpdatedBy: this.user.id
+    }
+  })
+
+  function shouldAcceptChanges() {
+    it('should accept writes', function(done) {
+      writeContent(writeOptions, done)
+    })
+
+    it('should accept updates', function(done) {
+      updateContent(writeOptions, done)
+    })
+
+    it('should accept same write twice', function(done) {
+      writeContentTwice(writeOptions, done)
+    })
+  }
+
+  function shouldBlockChanges() {
+    it('should block writes', function(done) {
+      writeContent(writeOptions, err => {
+        expect(err).to.exist
+        expect(err.message).to.equal('non-success statusCode: 404')
+        done()
+      })
+    })
+
+    it('should block updates', function(done) {
+      updateContent(writeOptions, err => {
+        expect(err).to.exist
+        expect(err.message).to.equal('non-success statusCode: 404')
+        done()
+      })
+    })
+  }
+
+  describe('a regular doc', function() {
+    shouldAcceptChanges()
+  })
+
+  describe('after deleting the doc', function() {
+    beforeEach(function(done) {
+      this.user.deleteItemInProject(this.projectId, 'doc', this.docId, done)
+    })
+
+    shouldAcceptChanges()
+  })
+
+  describe('unknown doc', function() {
+    beforeEach(function() {
+      writeOptions.docId = ObjectId()
+    })
+
+    shouldBlockChanges()
+  })
+
+  describe('doc in another project', function() {
+    beforeEach(function(done) {
+      this.user.createProject('foo', (error, projectId) => {
+        if (error) return done(error)
+        writeOptions.projectId = projectId
+        done()
+      })
+    })
+
+    shouldBlockChanges()
+  })
+})

services/web/test/acceptance/src/helpers/MockDocstoreApi.js

@@ -29,14 +29,18 @@ module.exports = MockDocStoreApi = {
         if (this.docs[project_id] == null) {
           this.docs[project_id] = {}
         }
-        this.docs[project_id][doc_id] = { lines, version, ranges }
+        if (this.docs[project_id][doc_id] == null) {
+          this.docs[project_id][doc_id] = {}
+        }
+        const { version: oldVersion, deleted } = this.docs[project_id][doc_id]
+        this.docs[project_id][doc_id] = { lines, version, ranges, deleted }
         if (this.docs[project_id][doc_id].rev == null) {
           this.docs[project_id][doc_id].rev = 0
         }
         this.docs[project_id][doc_id].rev += 1
         this.docs[project_id][doc_id]._id = doc_id
         return res.json({
-          modified: true,
+          modified: oldVersion !== version,
           rev: this.docs[project_id][doc_id].rev
         })
       }
@@ -64,6 +68,15 @@ module.exports = MockDocStoreApi = {
       }
     })
 
+    app.get('/project/:project_id/doc/:doc_id/deleted', (req, res) => {
+      const { project_id, doc_id } = req.params
+      if (!this.docs[project_id] || !this.docs[project_id][doc_id]) {
+        res.sendStatus(404)
+      } else {
+        res.json({ deleted: Boolean(this.docs[project_id][doc_id].deleted) })
+      }
+    })
+
     app.delete('/project/:project_id/doc/:doc_id', (req, res, next) => {
       const { project_id, doc_id } = req.params
       if (this.docs[project_id] == null) {

services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js

@@ -71,6 +71,7 @@ describe('ProjectEntityUpdateHandler', function() {
 
     this.DocstoreManager = {
       getDoc: sinon.stub(),
+      isDocDeleted: sinon.stub(),
       updateDoc: sinon.stub(),
       deleteDoc: sinon.stub()
     }
@@ -82,6 +83,7 @@ describe('ProjectEntityUpdateHandler', function() {
       deleteDoc: sinon.stub().yields()
     }
     this.logger = {
+      info: sinon.stub(),
       log: sinon.stub(),
       warn: sinon.stub(),
       error: sinon.stub(),
@@ -188,7 +190,8 @@ describe('ProjectEntityUpdateHandler', function() {
       this.ranges = { mock: 'ranges' }
       this.lastUpdatedAt = new Date().getTime()
       this.lastUpdatedBy = 'fake-last-updater-id'
-      this.ProjectGetter.getProjectWithoutDocLines.yields(null, this.project)
+      this.DocstoreManager.isDocDeleted.yields(null, false)
+      this.ProjectGetter.getProject.yields(null, this.project)
       this.ProjectLocator.findElement.yields(null, this.doc, {
         fileSystem: this.path
       })
@@ -210,9 +213,12 @@ describe('ProjectEntityUpdateHandler', function() {
         )
       })
 
-      it('should get the project without doc lines', function() {
+      it('should get the project with very few fields', function() {
-        this.ProjectGetter.getProjectWithoutDocLines
+        this.ProjectGetter.getProject
-          .calledWith(projectId)
+          .calledWith(projectId, {
+            name: true,
+            rootFolder: true
+          })
           .should.equal(true)
       })
 
@@ -294,9 +300,56 @@ describe('ProjectEntityUpdateHandler', function() {
 
     describe('when the doc has been deleted', function() {
       beforeEach(function() {
-        this.project.deletedDocs = [{ _id: docId }]
+        this.ProjectGetter.getProject.yields(null, this.project)
-        this.ProjectGetter.getProjectWithoutDocLines.yields(null, this.project)
         this.ProjectLocator.findElement.yields(new Errors.NotFoundError())
+        this.DocstoreManager.isDocDeleted.yields(null, true)
+        this.DocstoreManager.updateDoc.yields()
+        this.ProjectEntityUpdateHandler.updateDocLines(
+          projectId,
+          docId,
+          this.docLines,
+          this.version,
+          this.ranges,
+          this.lastUpdatedAt,
+          this.lastUpdatedBy,
+          this.callback
+        )
+      })
+
+      it('should update the doc in the docstore', function() {
+        this.DocstoreManager.updateDoc
+          .calledWith(
+            projectId,
+            docId,
+            this.docLines,
+            this.version,
+            this.ranges
+          )
+          .should.equal(true)
+      })
+
+      it('should not mark the project as updated', function() {
+        this.ProjectUpdater.markAsUpdated.called.should.equal(false)
+      })
+
+      it('should not send the doc the to the TPDS', function() {
+        this.TpdsUpdateSender.addDoc.called.should.equal(false)
+      })
+
+      it('should call the callback', function() {
+        this.callback.called.should.equal(true)
+      })
+    })
+
+    describe('when projects and docs collection are de-synced', function() {
+      beforeEach(function() {
+        this.ProjectGetter.getProject.yields(null, this.project)
+
+        // The doc is not in the file-tree, but also not marked as deleted.
+        // This should not happen, but web should handle it.
+        this.ProjectLocator.findElement.yields(new Errors.NotFoundError())
+        this.DocstoreManager.isDocDeleted.yields(null, false)
+
         this.DocstoreManager.updateDoc.yields()
         this.ProjectEntityUpdateHandler.updateDocLines(
           projectId,
@@ -337,7 +390,9 @@ describe('ProjectEntityUpdateHandler', function() {
 
     describe('when the doc is not related to the project', function() {
       beforeEach(function() {
-        this.ProjectLocator.findElement.yields()
+        this.ProjectGetter.getProject.yields(null, this.project)
+        this.ProjectLocator.findElement.yields(new Errors.NotFoundError())
+        this.DocstoreManager.isDocDeleted.yields(new Errors.NotFoundError())
         this.ProjectEntityUpdateHandler.updateDocLines(
           projectId,
           docId,
@@ -355,11 +410,19 @@ describe('ProjectEntityUpdateHandler', function() {
           .calledWith(sinon.match.instanceOf(Errors.NotFoundError))
           .should.equal(true)
       })
+
+      it('should not update the doc', function() {
+        this.DocstoreManager.updateDoc.called.should.equal(false)
+      })
+
+      it('should not send the doc the to the TPDS', function() {
+        this.TpdsUpdateSender.addDoc.called.should.equal(false)
+      })
     })
 
     describe('when the project is not found', function() {
       beforeEach(function() {
-        this.ProjectGetter.getProjectWithoutDocLines.yields()
+        this.ProjectGetter.getProject.yields(new Errors.NotFoundError())
         this.ProjectEntityUpdateHandler.updateDocLines(
           projectId,
           docId,
@@ -377,6 +440,14 @@ describe('ProjectEntityUpdateHandler', function() {
           .calledWith(sinon.match.instanceOf(Errors.NotFoundError))
           .should.equal(true)
       })
+
+      it('should not update the doc', function() {
+        this.DocstoreManager.updateDoc.called.should.equal(false)
+      })
+
+      it('should not send the doc the to the TPDS', function() {
+        this.TpdsUpdateSender.addDoc.called.should.equal(false)
+      })
     })
   })