[web+document-updater] Allow appending to documents (#20745)

Co-authored-by: David Powell <david.powell@overleaf.com>
GitOrigin-RevId: f66283926e7da3edf83ada9316c3a001287e1b42

services/document-updater/app.js

@@ -145,6 +145,7 @@ app.post(
 )
 app.post('/project/:project_id/clearState', HttpController.clearProjectState)
 app.post('/project/:project_id/doc/:doc_id', HttpController.setDoc)
+app.post('/project/:project_id/doc/:doc_id/append', HttpController.appendToDoc)
 app.post(
   '/project/:project_id/doc/:doc_id/flush',
   HttpController.flushDocIfLoaded

services/document-updater/app/js/DocumentManager.js

@@ -9,6 +9,8 @@ const HistoryManager = require('./HistoryManager')
 const Errors = require('./Errors')
 const RangesManager = require('./RangesManager')
 const { extractOriginOrSource } = require('./Utils')
+const { getTotalSizeOfLines } = require('./Limits')
+const Settings = require('@overleaf/settings')
 
 const MAX_UNFLUSHED_AGE = 300 * 1000 // 5 mins, document should be flushed to mongo this time after a change
 
@@ -112,7 +114,41 @@ const DocumentManager = {
     }
   },
 
-  async setDoc(projectId, docId, newLines, originOrSource, userId, undoing) {
+  async appendToDoc(projectId, docId, linesToAppend, originOrSource, userId) {
+    const { lines: currentLines } = await DocumentManager.getDoc(
+      projectId,
+      docId
+    )
+    const currentLineSize = getTotalSizeOfLines(currentLines)
+    const addedSize = getTotalSizeOfLines(linesToAppend)
+    const newlineSize = '\n'.length
+
+    if (currentLineSize + newlineSize + addedSize > Settings.max_doc_length) {
+      throw new Errors.FileTooLargeError(
+        'doc would become too large if appending this text'
+      )
+    }
+
+    return await DocumentManager.setDoc(
+      projectId,
+      docId,
+      currentLines.concat(linesToAppend),
+      originOrSource,
+      userId,
+      false,
+      false
+    )
+  },
+
+  async setDoc(
+    projectId,
+    docId,
+    newLines,
+    originOrSource,
+    userId,
+    undoing,
+    external
+  ) {
     if (newLines == null) {
       throw new Error('No lines were provided to setDoc')
     }
@@ -150,10 +186,12 @@ const DocumentManager = {
       op,
       v: version,
       meta: {
-        type: 'external',
         user_id: userId,
       },
     }
+    if (external) {
+      update.meta.type = 'external'
+    }
     if (origin) {
       update.meta.origin = origin
     } else if (source) {
@@ -481,7 +519,15 @@ const DocumentManager = {
     )
   },
 
-  async setDocWithLock(projectId, docId, lines, source, userId, undoing) {
+  async setDocWithLock(
+    projectId,
+    docId,
+    lines,
+    source,
+    userId,
+    undoing,
+    external
+  ) {
     const UpdateManager = require('./UpdateManager')
     return await UpdateManager.promises.lockUpdatesAndDo(
       DocumentManager.setDoc,
@@ -490,7 +536,20 @@ const DocumentManager = {
       lines,
       source,
       userId,
-      undoing
+      undoing,
+      external
+    )
+  },
+
+  async appendToDocWithLock(projectId, docId, lines, source, userId) {
+    const UpdateManager = require('./UpdateManager')
+    return await UpdateManager.promises.lockUpdatesAndDo(
+      DocumentManager.appendToDoc,
+      projectId,
+      docId,
+      lines,
+      source,
+      userId
     )
   },
 

services/document-updater/app/js/HttpController.js

@@ -144,6 +144,7 @@ function setDoc(req, res, next) {
     source,
     userId,
     undoing,
+    true,
     (error, result) => {
       timer.done()
       if (error) {
@@ -155,6 +156,35 @@ function setDoc(req, res, next) {
   )
 }
 
+function appendToDoc(req, res, next) {
+  const docId = req.params.doc_id
+  const projectId = req.params.project_id
+  const { lines, source, user_id: userId } = req.body
+  const timer = new Metrics.Timer('http.appendToDoc')
+  DocumentManager.appendToDocWithLock(
+    projectId,
+    docId,
+    lines,
+    source,
+    userId,
+    (error, result) => {
+      timer.done()
+      if (error instanceof Errors.FileTooLargeError) {
+        logger.warn('refusing to append to file, it would become too large')
+        return res.sendStatus(422)
+      }
+      if (error) {
+        return next(error)
+      }
+      logger.debug(
+        { projectId, docId, lines, source, userId },
+        'appending to doc via http'
+      )
+      res.json(result)
+    }
+  )
+}
+
 function flushDocIfLoaded(req, res, next) {
   const docId = req.params.doc_id
   const projectId = req.params.project_id
@@ -460,6 +490,7 @@ module.exports = {
   peekDoc,
   getProjectDocsAndFlushIfOld,
   clearProjectState,
+  appendToDoc,
   setDoc,
   flushDocIfLoaded,
   deleteDoc,

services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js

@@ -53,6 +53,9 @@ describe('DocumentManager', function () {
       acceptChanges: sinon.stub(),
       deleteComment: sinon.stub(),
     }
+    this.Settings = {
+      max_doc_length: 2 * 1024 * 1024, // 2mb
+    }
 
     this.DocumentManager = SandboxedModule.require(modulePath, {
       requires: {
@@ -65,6 +68,7 @@ describe('DocumentManager', function () {
         './UpdateManager': this.UpdateManager,
         './RangesManager': this.RangesManager,
         './Errors': Errors,
+        '@overleaf/settings': this.Settings,
       },
     })
     this.project_id = 'project-id-123'
@@ -446,7 +450,8 @@ describe('DocumentManager', function () {
             this.beforeLines,
             this.source,
             this.user_id,
-            false
+            false,
+            true
           )
         })
 
@@ -480,7 +485,8 @@ describe('DocumentManager', function () {
             this.beforeLines,
             this.source,
             this.user_id,
-            false
+            false,
+            true
           )
         })
 
@@ -513,7 +519,8 @@ describe('DocumentManager', function () {
             this.afterLines,
             this.source,
             this.user_id,
-            false
+            false,
+            true
           )
         })
 
@@ -582,7 +589,8 @@ describe('DocumentManager', function () {
             this.afterLines,
             this.source,
             this.user_id,
-            false
+            false,
+            true
           )
         })
 
@@ -618,7 +626,8 @@ describe('DocumentManager', function () {
               null,
               this.source,
               this.user_id,
-              false
+              false,
+              true
             )
           ).to.be.rejectedWith('No lines were provided to setDoc')
         })
@@ -642,6 +651,7 @@ describe('DocumentManager', function () {
             this.afterLines,
             this.source,
             this.user_id,
+            true,
             true
           )
         })
@@ -650,6 +660,77 @@ describe('DocumentManager', function () {
           this.ops.map(op => op.u.should.equal(true))
         })
       })
+
+      describe('with the external flag', function () {
+        beforeEach(async function () {
+          this.undoing = false
+          // Copy ops so we don't interfere with other tests
+          this.ops = [
+            { i: 'foo', p: 4 },
+            { d: 'bar', p: 42 },
+          ]
+          this.DiffCodec.diffAsShareJsOp.returns(this.ops)
+          await this.DocumentManager.promises.setDoc(
+            this.project_id,
+            this.doc_id,
+            this.afterLines,
+            this.source,
+            this.user_id,
+            this.undoing,
+            true
+          )
+        })
+
+        it('should add the external type to update metadata', function () {
+          this.UpdateManager.promises.applyUpdate
+            .calledWith(this.project_id, this.doc_id, {
+              doc: this.doc_id,
+              v: this.version,
+              op: this.ops,
+              meta: {
+                type: 'external',
+                source: this.source,
+                user_id: this.user_id,
+              },
+            })
+            .should.equal(true)
+        })
+      })
+
+      describe('without the external flag', function () {
+        beforeEach(async function () {
+          this.undoing = false
+          // Copy ops so we don't interfere with other tests
+          this.ops = [
+            { i: 'foo', p: 4 },
+            { d: 'bar', p: 42 },
+          ]
+          this.DiffCodec.diffAsShareJsOp.returns(this.ops)
+          await this.DocumentManager.promises.setDoc(
+            this.project_id,
+            this.doc_id,
+            this.afterLines,
+            this.source,
+            this.user_id,
+            this.undoing,
+            false
+          )
+        })
+
+        it('should not add the external type to update metadata', function () {
+          this.UpdateManager.promises.applyUpdate
+            .calledWith(this.project_id, this.doc_id, {
+              doc: this.doc_id,
+              v: this.version,
+              op: this.ops,
+              meta: {
+                source: this.source,
+                user_id: this.user_id,
+              },
+            })
+            .should.equal(true)
+        })
+      })
     })
   })
 
@@ -1136,4 +1217,68 @@ describe('DocumentManager', function () {
       })
     })
   })
+
+  describe('appendToDoc', function () {
+    describe('sucessfully', function () {
+      beforeEach(async function () {
+        this.lines = ['one', 'two', 'three']
+        this.DocumentManager.promises.setDoc = sinon
+          .stub()
+          .resolves({ rev: '123' })
+        this.DocumentManager.promises.getDoc = sinon.stub().resolves({
+          lines: this.lines,
+        })
+        this.result = await this.DocumentManager.promises.appendToDoc(
+          this.project_id,
+          this.doc_id,
+          ['four', 'five', 'six'],
+          this.source,
+          this.user_id
+        )
+      })
+
+      it('should call setDoc with concatenated lines', function () {
+        this.DocumentManager.promises.setDoc
+          .calledWith(
+            this.project_id,
+            this.doc_id,
+            ['one', 'two', 'three', 'four', 'five', 'six'],
+            this.source,
+            this.user_id,
+            false,
+            false
+          )
+          .should.equal(true)
+      })
+
+      it('should return output from setDoc', function () {
+        this.result.should.deep.equal({ rev: '123' })
+      })
+    })
+
+    describe('when doc would become too big', function () {
+      beforeEach(async function () {
+        this.Settings.max_doc_length = 100
+        this.lines = ['one', 'two', 'three']
+        this.DocumentManager.promises.setDoc = sinon
+          .stub()
+          .resolves({ rev: '123' })
+        this.DocumentManager.promises.getDoc = sinon.stub().resolves({
+          lines: this.lines,
+        })
+      })
+
+      it('should fail with FileTooLarge error', async function () {
+        expect(
+          this.DocumentManager.promises.appendToDoc(
+            this.project_id,
+            this.doc_id,
+            ['x'.repeat(1000)],
+            this.source,
+            this.user_id
+          )
+        ).to.eventually.be.rejectedWith(Errors.FileTooLargeError)
+      })
+    })
+  })
 })

services/document-updater/test/unit/js/HttpController/HttpControllerTests.js

@@ -209,7 +209,7 @@ describe('HttpController', function () {
       beforeEach(function () {
         this.DocumentManager.setDocWithLock = sinon
           .stub()
-          .callsArgWith(6, null, { rev: '123' })
+          .callsArgWith(7, null, { rev: '123' })
         this.HttpController.setDoc(this.req, this.res, this.next)
       })
 
@@ -221,7 +221,8 @@ describe('HttpController', function () {
             this.lines,
             this.source,
             this.user_id,
-            this.undoing
+            this.undoing,
+            true
           )
           .should.equal(true)
       })
@@ -255,7 +256,7 @@ describe('HttpController', function () {
       beforeEach(function () {
         this.DocumentManager.setDocWithLock = sinon
           .stub()
-          .callsArgWith(6, new Error('oops'))
+          .callsArgWith(7, new Error('oops'))
         this.HttpController.setDoc(this.req, this.res, this.next)
       })
 
@@ -1103,4 +1104,96 @@ describe('HttpController', function () {
       })
     })
   })
+
+  describe('appendToDoc', function () {
+    beforeEach(function () {
+      this.lines = ['one', 'two', 'three']
+      this.source = 'dropbox'
+      this.user_id = 'user-id-123'
+      this.req = {
+        headers: {},
+        params: {
+          project_id: this.project_id,
+          doc_id: this.doc_id,
+        },
+        query: {},
+        body: {
+          lines: this.lines,
+          source: this.source,
+          user_id: this.user_id,
+          undoing: (this.undoing = true),
+        },
+      }
+    })
+
+    describe('successfully', function () {
+      beforeEach(function () {
+        this.DocumentManager.appendToDocWithLock = sinon
+          .stub()
+          .callsArgWith(5, null, { rev: '123' })
+        this.HttpController.appendToDoc(this.req, this.res, this.next)
+      })
+
+      it('should append to the doc', function () {
+        this.DocumentManager.appendToDocWithLock
+          .calledWith(
+            this.project_id,
+            this.doc_id,
+            this.lines,
+            this.source,
+            this.user_id
+          )
+          .should.equal(true)
+      })
+
+      it('should return a json response with the document rev from web', function () {
+        this.res.json.calledWithMatch({ rev: '123' }).should.equal(true)
+      })
+
+      it('should log the request', function () {
+        this.logger.debug
+          .calledWith(
+            {
+              docId: this.doc_id,
+              projectId: this.project_id,
+              lines: this.lines,
+              source: this.source,
+              userId: this.user_id,
+            },
+            'appending to doc via http'
+          )
+          .should.equal(true)
+      })
+
+      it('should time the request', function () {
+        this.Metrics.Timer.prototype.done.called.should.equal(true)
+      })
+    })
+
+    describe('when an errors occurs', function () {
+      beforeEach(function () {
+        this.DocumentManager.appendToDocWithLock = sinon
+          .stub()
+          .callsArgWith(5, new Error('oops'))
+        this.HttpController.appendToDoc(this.req, this.res, this.next)
+      })
+
+      it('should call next with the error', function () {
+        this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true)
+      })
+    })
+
+    describe('when the payload is too large', function () {
+      beforeEach(function () {
+        this.DocumentManager.appendToDocWithLock = sinon
+          .stub()
+          .callsArgWith(5, new Errors.FileTooLargeError())
+        this.HttpController.appendToDoc(this.req, this.res, this.next)
+      })
+
+      it('should send back a 422 response', function () {
+        this.res.sendStatus.calledWith(422).should.equal(true)
+      })
+    })
+  })
 })