Accept ordered doc and file updates

Add an `updates` parameter to the project update endpoint. It can be
used instead of `docUpdates` and `fileUpdates` to provide a single list
of updates in the order they should be processed.

services/document-updater/.eslintrc

@@ -8,7 +8,7 @@
     "prettier/standard"
   ],
   "parserOptions": {
-    "ecmaVersion": 2017
+    "ecmaVersion": 2018
   },
   "plugins": [
     "mocha",

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

@@ -330,19 +330,23 @@ function updateProject(req, res, next) {
     userId,
     docUpdates,
     fileUpdates,
+    updates,
     version
   } = req.body
   logger.log(
-    { projectId, docUpdates, fileUpdates, version },
+    { projectId, updates, docUpdates, fileUpdates, version },
     'updating project via http'
   )
-
+  const allUpdates = _mergeUpdates(
+    docUpdates || [],
+    fileUpdates || [],
+    updates || []
+  )
   ProjectManager.updateProjectWithLocks(
     projectId,
     projectHistoryId,
     userId,
-    docUpdates,
-    fileUpdates,
+    allUpdates,
     version,
     (error) => {
       timer.done()
@@ -412,3 +416,23 @@ function flushQueuedProjects(req, res, next) {
     }
   })
 }
+
+/**
+ * Merge updates from the previous project update interface (docUpdates +
+ * fileUpdates) and the new update interface (updates).
+ */
+function _mergeUpdates(docUpdates, fileUpdates, updates) {
+  const mergedUpdates = []
+  for (const update of docUpdates) {
+    const type = update.docLines != null ? 'add-doc' : 'rename-doc'
+    mergedUpdates.push({ type, ...update })
+  }
+  for (const update of fileUpdates) {
+    const type = update.url != null ? 'add-file' : 'rename-file'
+    mergedUpdates.push({ type, ...update })
+  }
+  for (const update of updates) {
+    mergedUpdates.push(update)
+  }
+  return mergedUpdates
+}

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

@@ -212,9 +212,8 @@ function updateProjectWithLocks(
   projectId,
   projectHistoryId,
   userId,
-  docUpdates,
-  fileUpdates,
-  version,
+  updates,
+  projectVersion,
   _callback
 ) {
   const timer = new Metrics.Timer('projectManager.updateProject')
@@ -223,92 +222,85 @@ function updateProjectWithLocks(
     _callback(...args)
   }
 
-  const projectVersion = version
   let projectSubversion = 0 // project versions can have multiple operations
-
   let projectOpsLength = 0
 
-  const handleDocUpdate = function (projectUpdate, cb) {
-    const docId = projectUpdate.id
-    projectUpdate.version = `${projectVersion}.${projectSubversion++}`
-    if (projectUpdate.docLines != null) {
-      ProjectHistoryRedisManager.queueAddEntity(
-        projectId,
-        projectHistoryId,
-        'doc',
-        docId,
-        userId,
-        projectUpdate,
-        (error, count) => {
-          projectOpsLength = count
-          cb(error)
-        }
-      )
-    } else {
-      DocumentManager.renameDocWithLock(
-        projectId,
-        docId,
-        userId,
-        projectUpdate,
-        projectHistoryId,
-        (error, count) => {
-          projectOpsLength = count
-          cb(error)
-        }
-      )
+  function handleUpdate(update, cb) {
+    update.version = `${projectVersion}.${projectSubversion++}`
+    switch (update.type) {
+      case 'add-doc':
+        ProjectHistoryRedisManager.queueAddEntity(
+          projectId,
+          projectHistoryId,
+          'doc',
+          update.id,
+          userId,
+          update,
+          (error, count) => {
+            projectOpsLength = count
+            cb(error)
+          }
+        )
+        break
+      case 'rename-doc':
+        DocumentManager.renameDocWithLock(
+          projectId,
+          update.id,
+          userId,
+          update,
+          projectHistoryId,
+          (error, count) => {
+            projectOpsLength = count
+            cb(error)
+          }
+        )
+        break
+      case 'add-file':
+        ProjectHistoryRedisManager.queueAddEntity(
+          projectId,
+          projectHistoryId,
+          'file',
+          update.id,
+          userId,
+          update,
+          (error, count) => {
+            projectOpsLength = count
+            cb(error)
+          }
+        )
+        break
+      case 'rename-file':
+        ProjectHistoryRedisManager.queueRenameEntity(
+          projectId,
+          projectHistoryId,
+          'file',
+          update.id,
+          userId,
+          update,
+          (error, count) => {
+            projectOpsLength = count
+            cb(error)
+          }
+        )
+        break
+      default:
+        cb(new Error(`Unknown update type: ${update.type}`))
     }
   }
 
-  const handleFileUpdate = function (projectUpdate, cb) {
-    const fileId = projectUpdate.id
-    projectUpdate.version = `${projectVersion}.${projectSubversion++}`
-    if (projectUpdate.url != null) {
-      ProjectHistoryRedisManager.queueAddEntity(
-        projectId,
-        projectHistoryId,
-        'file',
-        fileId,
-        userId,
-        projectUpdate,
-        (error, count) => {
-          projectOpsLength = count
-          cb(error)
-        }
-      )
-    } else {
-      ProjectHistoryRedisManager.queueRenameEntity(
-        projectId,
-        projectHistoryId,
-        'file',
-        fileId,
-        userId,
-        projectUpdate,
-        (error, count) => {
-          projectOpsLength = count
-          cb(error)
-        }
-      )
-    }
-  }
-
-  async.eachSeries(docUpdates, handleDocUpdate, (error) => {
+  async.eachSeries(updates, handleUpdate, (error) => {
     if (error) {
       return callback(error)
     }
-    async.eachSeries(fileUpdates, handleFileUpdate, (error) => {
-      if (error) {
-        return callback(error)
-      }
-      if (
-        HistoryManager.shouldFlushHistoryOps(
-          projectOpsLength,
-          docUpdates.length + fileUpdates.length,
-          HistoryManager.FLUSH_PROJECT_EVERY_N_OPS
-        )
-      ) {
-        HistoryManager.flushProjectChangesAsync(projectId)
-      }
-      callback()
-    })
+    if (
+      HistoryManager.shouldFlushHistoryOps(
+        projectOpsLength,
+        updates.length,
+        HistoryManager.FLUSH_PROJECT_EVERY_N_OPS
+      )
+    ) {
+      HistoryManager.flushProjectChangesAsync(projectId)
+    }
+    callback()
   })
 }

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

@@ -809,12 +809,34 @@ describe('HttpController', function () {
     })
   })
 
-  describe('updateProject', function () {
+  describe('updateProject (split doc and file updates)', function () {
     beforeEach(function () {
       this.projectHistoryId = 'history-id-123'
       this.userId = 'user-id-123'
-      this.docUpdates = sinon.stub()
-      this.fileUpdates = sinon.stub()
+      this.docUpdates = [
+        { id: 1, pathname: 'thesis.tex', newPathname: 'book.tex' },
+        { id: 2, pathname: 'article.tex', docLines: 'hello' }
+      ]
+      this.fileUpdates = [
+        { id: 3, pathname: 'apple.png', newPathname: 'banana.png' },
+        { id: 4, url: 'filestore.example.com/4' }
+      ]
+      this.expectedUpdates = [
+        {
+          type: 'rename-doc',
+          id: 1,
+          pathname: 'thesis.tex',
+          newPathname: 'book.tex'
+        },
+        { type: 'add-doc', id: 2, pathname: 'article.tex', docLines: 'hello' },
+        {
+          type: 'rename-file',
+          id: 3,
+          pathname: 'apple.png',
+          newPathname: 'banana.png'
+        },
+        { type: 'add-file', id: 4, url: 'filestore.example.com/4' }
+      ]
       this.version = 1234567
       this.req = {
         query: {},
@@ -833,9 +855,7 @@ describe('HttpController', function () {
 
     describe('successfully', function () {
       beforeEach(function () {
-        this.ProjectManager.updateProjectWithLocks = sinon
-          .stub()
-          .callsArgWith(6)
+        this.ProjectManager.updateProjectWithLocks = sinon.stub().yields()
         this.HttpController.updateProject(this.req, this.res, this.next)
       })
 
@@ -845,8 +865,7 @@ describe('HttpController', function () {
             this.project_id,
             this.projectHistoryId,
             this.userId,
-            this.docUpdates,
-            this.fileUpdates,
+            this.expectedUpdates,
             this.version
           )
           .should.equal(true)
@@ -865,7 +884,83 @@ describe('HttpController', function () {
       beforeEach(function () {
         this.ProjectManager.updateProjectWithLocks = sinon
           .stub()
-          .callsArgWith(6, new Error('oops'))
+          .yields(new Error('oops'))
+        this.HttpController.updateProject(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('updateProject (single updates parameter)', function () {
+    beforeEach(function () {
+      this.projectHistoryId = 'history-id-123'
+      this.userId = 'user-id-123'
+      this.updates = [
+        {
+          type: 'rename-doc',
+          id: 1,
+          pathname: 'thesis.tex',
+          newPathname: 'book.tex'
+        },
+        { type: 'add-doc', id: 2, pathname: 'article.tex', docLines: 'hello' },
+        {
+          type: 'rename-file',
+          id: 3,
+          pathname: 'apple.png',
+          newPathname: 'banana.png'
+        },
+        { type: 'add-file', id: 4, url: 'filestore.example.com/4' }
+      ]
+      this.version = 1234567
+      this.req = {
+        query: {},
+        body: {
+          projectHistoryId: this.projectHistoryId,
+          userId: this.userId,
+          updates: this.updates,
+          version: this.version
+        },
+        params: {
+          project_id: this.project_id
+        }
+      }
+    })
+
+    describe('successfully', function () {
+      beforeEach(function () {
+        this.ProjectManager.updateProjectWithLocks = sinon.stub().yields()
+        this.HttpController.updateProject(this.req, this.res, this.next)
+      })
+
+      it('should accept the change', function () {
+        this.ProjectManager.updateProjectWithLocks
+          .calledWith(
+            this.project_id,
+            this.projectHistoryId,
+            this.userId,
+            this.updates,
+            this.version
+          )
+          .should.equal(true)
+      })
+
+      it('should return a successful No Content response', function () {
+        this.res.sendStatus.calledWith(204).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.ProjectManager.updateProjectWithLocks = sinon
+          .stub()
+          .yields(new Error('oops'))
         this.HttpController.updateProject(this.req, this.res, this.next)
       })
 

services/document-updater/test/unit/js/ProjectManager/updateProjectTests.js

@@ -49,22 +49,28 @@ describe('ProjectManager', function () {
     describe('rename operations', function () {
       beforeEach(function () {
         this.firstDocUpdate = {
+          type: 'rename-doc',
           id: 1,
           pathname: 'foo',
           newPathname: 'foo'
         }
         this.secondDocUpdate = {
+          type: 'rename-doc',
           id: 2,
           pathname: 'bar',
           newPathname: 'bar2'
         }
-        this.docUpdates = [this.firstDocUpdate, this.secondDocUpdate]
         this.firstFileUpdate = {
+          type: 'rename-file',
           id: 2,
           pathname: 'bar',
           newPathname: 'bar2'
         }
-        this.fileUpdates = [this.firstFileUpdate]
+        this.updates = [
+          this.firstDocUpdate,
+          this.secondDocUpdate,
+          this.firstFileUpdate
+        ]
       })
 
       describe('successfully', function () {
@@ -73,8 +79,7 @@ describe('ProjectManager', function () {
             this.project_id,
             this.projectHistoryId,
             this.user_id,
-            this.docUpdates,
-            this.fileUpdates,
+            this.updates,
             this.version,
             this.callback
           )
@@ -146,8 +151,7 @@ describe('ProjectManager', function () {
             this.project_id,
             this.projectHistoryId,
             this.user_id,
-            this.docUpdates,
-            this.fileUpdates,
+            this.updates,
             this.version,
             this.callback
           )
@@ -166,8 +170,7 @@ describe('ProjectManager', function () {
             this.project_id,
             this.projectHistoryId,
             this.user_id,
-            this.docUpdates,
-            this.fileUpdates,
+            this.updates,
             this.version,
             this.callback
           )
@@ -185,8 +188,7 @@ describe('ProjectManager', function () {
             this.project_id,
             this.projectHistoryId,
             this.user_id,
-            this.docUpdates,
-            this.fileUpdates,
+            this.updates,
             this.version,
             this.callback
           )
@@ -203,23 +205,31 @@ describe('ProjectManager', function () {
     describe('add operations', function () {
       beforeEach(function () {
         this.firstDocUpdate = {
+          type: 'add-doc',
           id: 1,
           docLines: 'a\nb'
         }
         this.secondDocUpdate = {
+          type: 'add-doc',
           id: 2,
           docLines: 'a\nb'
         }
-        this.docUpdates = [this.firstDocUpdate, this.secondDocUpdate]
         this.firstFileUpdate = {
+          type: 'add-file',
           id: 3,
           url: 'filestore.example.com/2'
         }
         this.secondFileUpdate = {
+          type: 'add-file',
           id: 4,
           url: 'filestore.example.com/3'
         }
-        this.fileUpdates = [this.firstFileUpdate, this.secondFileUpdate]
+        this.updates = [
+          this.firstDocUpdate,
+          this.secondDocUpdate,
+          this.firstFileUpdate,
+          this.secondFileUpdate
+        ]
       })
 
       describe('successfully', function () {
@@ -228,8 +238,7 @@ describe('ProjectManager', function () {
             this.project_id,
             this.projectHistoryId,
             this.user_id,
-            this.docUpdates,
-            this.fileUpdates,
+            this.updates,
             this.version,
             this.callback
           )
@@ -322,8 +331,7 @@ describe('ProjectManager', function () {
             this.project_id,
             this.projectHistoryId,
             this.user_id,
-            this.docUpdates,
-            this.fileUpdates,
+            this.updates,
             this.version,
             this.callback
           )
@@ -342,8 +350,7 @@ describe('ProjectManager', function () {
             this.project_id,
             this.projectHistoryId,
             this.user_id,
-            this.docUpdates,
-            this.fileUpdates,
+            this.updates,
             this.version,
             this.callback
           )
@@ -361,8 +368,7 @@ describe('ProjectManager', function () {
             this.project_id,
             this.projectHistoryId,
             this.user_id,
-            this.docUpdates,
-            this.fileUpdates,
+            this.updates,
             this.version,
             this.callback
           )
@@ -375,5 +381,23 @@ describe('ProjectManager', function () {
         })
       })
     })
+
+    describe('when given an unknown operation type', function () {
+      beforeEach(function () {
+        this.updates = [{ type: 'brew-coffee' }]
+        this.ProjectManager.updateProjectWithLocks(
+          this.project_id,
+          this.projectHistoryId,
+          this.user_id,
+          this.updates,
+          this.version,
+          this.callback
+        )
+      })
+
+      it('should call back with an error', function () {
+        this.callback.calledWith(sinon.match.instanceOf(Error)).should.be.true
+      })
+    })
   })
 })