Merge pull request #9647 from overleaf/bg-dropbox-to-overleaf-existing-doc

handle updates to existing doc in dropbox to overleaf metadata

GitOrigin-RevId: e82955a4a76e62fb649263a94103fdb7f322de85

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

@@ -238,23 +238,32 @@ module.exports = DocumentManager = {
             // Otherwise we should remove it immediately since nothing else
             // is using it.
             if (alreadyLoaded) {
-              DocumentManager.flushDocIfLoaded(projectId, docId, error => {
-                if (error) {
-                  return callback(error)
+              DocumentManager.flushDocIfLoaded(
+                projectId,
+                docId,
+                (error, result) => {
+                  if (error) {
+                    return callback(error)
+                  }
+                  callback(null, result)
                 }
-                callback(null)
-              })
+              )
             } else {
-              DocumentManager.flushAndDeleteDoc(projectId, docId, {}, error => {
-                // There is no harm in flushing project history if the previous
-                // call failed and sometimes it is required
-                HistoryManager.flushProjectChangesAsync(projectId)
+              DocumentManager.flushAndDeleteDoc(
+                projectId,
+                docId,
+                {},
+                (error, result) => {
+                  // There is no harm in flushing project history if the previous
+                  // call failed and sometimes it is required
+                  HistoryManager.flushProjectChangesAsync(projectId)
 
-                if (error) {
-                  return callback(error)
+                  if (error) {
+                    return callback(error)
+                  }
+                  callback(null, result)
                 }
-                callback(null)
-              })
+              )
             }
           })
         })
@@ -302,11 +311,16 @@ module.exports = DocumentManager = {
             ranges,
             lastUpdatedAt,
             lastUpdatedBy,
-            error => {
+            (error, result) => {
               if (error) {
                 return callback(error)
               }
-              RedisManager.clearUnflushedTime(docId, callback)
+              RedisManager.clearUnflushedTime(docId, err => {
+                if (err) {
+                  return callback(err)
+                }
+                callback(null, result)
+              })
             }
           )
         }
@@ -321,7 +335,7 @@ module.exports = DocumentManager = {
       _callback(...args)
     }
 
-    DocumentManager.flushDocIfLoaded(projectId, docId, error => {
+    DocumentManager.flushDocIfLoaded(projectId, docId, (error, result) => {
       if (error) {
         if (options.ignoreFlushErrors) {
           logger.warn(
@@ -340,7 +354,7 @@ module.exports = DocumentManager = {
         if (error) {
           return callback(error)
         }
-        callback(null)
+        callback(null, result)
       })
     })
   },

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

@@ -163,13 +163,13 @@ function setDoc(req, res, next) {
     source,
     userId,
     undoing,
-    error => {
+    (error, result) => {
       timer.done()
       if (error) {
         return next(error)
       }
       logger.debug({ projectId, docId }, 'set doc via http')
-      res.sendStatus(204) // No Content
+      res.json(result)
     }
   )
 }

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

@@ -159,7 +159,7 @@ function setDoc(
         return callback(new Error('error connecting to web API'))
       }
       if (res.statusCode >= 200 && res.statusCode < 300) {
-        callback(null)
+        callback(null, body)
       } else if (res.statusCode === 404) {
         callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`))
       } else if (res.statusCode === 413) {

services/document-updater/test/acceptance/js/SettingADocumentTests.js

@@ -76,6 +76,7 @@ describe('Setting a document', function () {
                     return done(error)
                   }
                   this.statusCode = res.statusCode
+                  this.body = body
                   done()
                 }
               )
@@ -91,8 +92,8 @@ describe('Setting a document', function () {
       MockWebApi.setDocument.resetHistory()
     })
 
-    it('should return a 204 status code', function () {
-      this.statusCode.should.equal(204)
+    it('should return a 200 status code', function () {
+      this.statusCode.should.equal(200)
     })
 
     it('should send the updated doc lines and version to the web api', function () {
@@ -141,6 +142,10 @@ describe('Setting a document', function () {
         }
       )
     })
+
+    it('should return the mongo rev in the json response', function () {
+      this.body.should.deep.equal({ rev: '123' })
+    })
   })
 
   describe('when the updated doc does not exist in the doc updater', function () {
@@ -163,6 +168,7 @@ describe('Setting a document', function () {
             return done(error)
           }
           this.statusCode = res.statusCode
+          this.body = body
           setTimeout(done, 200)
         }
       )
@@ -174,8 +180,8 @@ describe('Setting a document', function () {
       MockWebApi.setDocument.resetHistory()
     })
 
-    it('should return a 204 status code', function () {
-      this.statusCode.should.equal(204)
+    it('should return a 200 status code', function () {
+      this.statusCode.should.equal(200)
     })
 
     it('should send the updated doc lines to the web api', function () {
@@ -206,6 +212,10 @@ describe('Setting a document', function () {
         }
       )
     })
+
+    it('should return the mongo rev in the json response', function () {
+      this.body.should.deep.equal({ rev: '123' })
+    })
   })
 
   const DOC_TOO_LARGE_TEST_CASES = [
@@ -302,6 +312,7 @@ describe('Setting a document', function () {
             return done(error)
           }
           this.statusCode = res.statusCode
+          this.body = body
           setTimeout(done, 200)
         }
       )
@@ -313,8 +324,8 @@ describe('Setting a document', function () {
       MockWebApi.setDocument.resetHistory()
     })
 
-    it('should return a 204 status code', function () {
-      this.statusCode.should.equal(204)
+    it('should return a 200 status code', function () {
+      this.statusCode.should.equal(200)
     })
 
     it('should send the updated doc lines to the web api', function () {
@@ -322,6 +333,10 @@ describe('Setting a document', function () {
         .calledWith(this.project_id, this.doc_id, this.newLines)
         .should.equal(true)
     })
+
+    it('should return the mongo rev in the json response', function () {
+      this.body.should.deep.equal({ rev: '123' })
+    })
   })
 
   describe('with track changes', function () {

services/document-updater/test/acceptance/js/helpers/MockWebApi.js

@@ -99,7 +99,7 @@ module.exports = MockWebApi = {
             if (error != null) {
               return res.sendStatus(500)
             } else {
-              return res.sendStatus(204)
+              return res.json({ rev: '123' })
             }
           }
         )

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

@@ -17,6 +17,7 @@ describe('HttpController', function () {
         './RedisManager': (this.RedisManager = {}),
         './Metrics': (this.Metrics = {}),
         './Errors': Errors,
+        '@overleaf/settings': { max_doc_length: 2 * 1024 * 1024 },
       },
     })
     this.Metrics.Timer = class Timer {}
@@ -202,7 +203,9 @@ describe('HttpController', function () {
 
     describe('successfully', function () {
       beforeEach(function () {
-        this.DocumentManager.setDocWithLock = sinon.stub().callsArgWith(6)
+        this.DocumentManager.setDocWithLock = sinon
+          .stub()
+          .callsArgWith(6, null, { rev: '123' })
         this.HttpController.setDoc(this.req, this.res, this.next)
       })
 
@@ -219,8 +222,8 @@ describe('HttpController', function () {
           .should.equal(true)
       })
 
-      it('should return a successful No Content response', function () {
-        this.res.sendStatus.calledWith(204).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 () {

services/web/app/src/Features/Documents/DocumentController.js

@@ -94,7 +94,7 @@ function setDocument(req, res, next) {
     ranges,
     lastUpdatedAt,
     lastUpdatedBy,
-    error => {
+    (error, result) => {
       if (error) {
         OError.tag(error, 'error finding element for getDocument', {
           docId,
@@ -106,7 +106,7 @@ function setDocument(req, res, next) {
         { docId, projectId },
         'finished receiving set document request from api (docupdater)'
       )
-      res.sendStatus(200)
+      res.json(result)
     }
   )
 }

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

@@ -201,7 +201,7 @@ const ProjectEntityUpdateHandler = {
           )
           // path will only be present if the doc is not deleted
           if (!modified || isDeletedDoc) {
-            return callback()
+            return callback(null, { rev })
           }
           // Don't need to block for marking as updated
           ProjectUpdateHandler.markAsUpdated(
@@ -218,7 +218,12 @@ const ProjectEntityUpdateHandler = {
               rev,
               folderId: folder?._id,
             },
-            callback
+            err => {
+              if (err) {
+                return callback(err)
+              }
+              callback(null, { rev, modified })
+            }
           )
         }
       )
@@ -741,7 +746,7 @@ const ProjectEntityUpdateHandler = {
             userId,
             docLines,
             source,
-            err => {
+            (err, result) => {
               if (err != null) {
                 return callback(err)
               }
@@ -749,16 +754,11 @@ const ProjectEntityUpdateHandler = {
                 { projectId, docId: existingDoc._id },
                 'notifying users that the document has been updated'
               )
-              DocumentUpdaterHandler.flushDocToMongo(
-                projectId,
-                existingDoc._id,
-                err => {
-                  if (err != null) {
-                    return callback(err)
-                  }
-                  callback(null, existingDoc, existingDoc == null)
-                }
-              )
+              // there is no need to flush the doc to mongo at this point as docupdater
+              // flushes it as part of setDoc.
+              //
+              // combine rev from response with existing doc metadata
+              callback(null, { ...existingDoc, ...result }, existingDoc == null)
             }
           )
         } else {

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

@@ -790,12 +790,6 @@ describe('ProjectEntityUpdateHandler', function () {
           .should.equal(true)
       })
 
-      it('flushes the doc contents', function () {
-        this.DocumentUpdaterHandler.flushDocToMongo
-          .calledWith(projectId, this.existingDoc._id)
-          .should.equal(true)
-      })
-
       it('returns the doc', function () {
         this.callback.calledWith(null, this.existingDoc, false)
       })