Merge pull request #16821 from overleaf/em-promisify-ranges-manager

Make RangesManager synchronous

GitOrigin-RevId: 46bc85d952474fa0d83f3fd1299c0a3888fcf436

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

@@ -380,26 +380,29 @@ const DocumentManager = {
             new Errors.NotFoundError(`document not found: ${docId}`)
           )
         }
-        RangesManager.acceptChanges(changeIds, ranges, (error, newRanges) => {
-          if (error) {
-            return callback(error)
-          }
-          RedisManager.updateDocument(
-            projectId,
-            docId,
-            lines,
-            version,
-            [],
-            newRanges,
-            {},
-            error => {
-              if (error) {
-                return callback(error)
-              }
-              callback()
+
+        let newRanges
+        try {
+          newRanges = RangesManager.acceptChanges(changeIds, ranges)
+        } catch (err) {
+          return callback(err)
+        }
+
+        RedisManager.updateDocument(
+          projectId,
+          docId,
+          lines,
+          version,
+          [],
+          newRanges,
+          {},
+          error => {
+            if (error) {
+              return callback(error)
             }
-          )
-        })
+            callback()
+          }
+        )
       }
     )
   },
@@ -423,26 +426,29 @@ const DocumentManager = {
             new Errors.NotFoundError(`document not found: ${docId}`)
           )
         }
-        RangesManager.deleteComment(commentId, ranges, (error, newRanges) => {
-          if (error) {
-            return callback(error)
-          }
-          RedisManager.updateDocument(
-            projectId,
-            docId,
-            lines,
-            version,
-            [],
-            newRanges,
-            {},
-            error => {
-              if (error) {
-                return callback(error)
-              }
-              callback()
+
+        let newRanges
+        try {
+          newRanges = RangesManager.deleteComment(commentId, ranges)
+        } catch (err) {
+          return callback(err)
+        }
+
+        RedisManager.updateDocument(
+          projectId,
+          docId,
+          lines,
+          version,
+          [],
+          newRanges,
+          {},
+          error => {
+            if (error) {
+              return callback(error)
             }
-          )
-        })
+            callback()
+          }
+        )
       }
     )
   },

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

@@ -1,13 +1,3 @@
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const { promisifyAll } = require('@overleaf/promise-utils')
 const RangesTracker = require('@overleaf/ranges-tracker')
 const logger = require('@overleaf/logger')
 const Metrics = require('./Metrics')
@@ -19,60 +9,44 @@ const RangesManager = {
   MAX_COMMENTS: 500,
   MAX_CHANGES: 2000,
 
-  applyUpdate(projectId, docId, entries, updates, newDocLines, callback) {
-    let error
+  applyUpdate(projectId, docId, entries, updates, newDocLines) {
     if (entries == null) {
       entries = {}
     }
     if (updates == null) {
       updates = []
     }
-    if (callback == null) {
-      callback = function () {}
-    }
     const { changes, comments } = _.cloneDeep(entries)
     const rangesTracker = new RangesTracker(changes, comments)
     const [emptyRangeCountBefore, totalRangeCountBefore] =
       RangesManager._emptyRangesCount(rangesTracker)
-    for (const update of Array.from(updates)) {
+    for (const update of updates) {
       rangesTracker.track_changes = !!update.meta.tc
       if (update.meta.tc) {
         rangesTracker.setIdSeed(update.meta.tc)
       }
-      for (const op of Array.from(update.op)) {
-        try {
-          rangesTracker.applyOp(op, {
-            user_id: update.meta != null ? update.meta.user_id : undefined,
-          })
-        } catch (error1) {
-          error = error1
-          return callback(error)
-        }
+      for (const op of update.op) {
+        rangesTracker.applyOp(op, { user_id: update.meta?.user_id })
       }
     }
 
     if (
-      (rangesTracker.changes != null
-        ? rangesTracker.changes.length
-        : undefined) > RangesManager.MAX_CHANGES ||
-      (rangesTracker.comments != null
-        ? rangesTracker.comments.length
-        : undefined) > RangesManager.MAX_COMMENTS
+      rangesTracker.changes?.length > RangesManager.MAX_CHANGES ||
+      rangesTracker.comments?.length > RangesManager.MAX_COMMENTS
     ) {
-      return callback(new Error('too many comments or tracked changes'))
+      throw new Error('too many comments or tracked changes')
     }
 
     try {
       // This is a consistency check that all of our ranges and
       // comments still match the corresponding text
       rangesTracker.validate(newDocLines.join('\n'))
-    } catch (error2) {
-      error = error2
+    } catch (err) {
       logger.error(
-        { err: error, projectId, docId, newDocLines, updates },
+        { err, projectId, docId, newDocLines, updates },
         'error validating ranges'
       )
-      return callback(error)
+      throw err
     }
 
     const [emptyRangeCountAfter, totalRangeCountAfter] =
@@ -89,65 +63,49 @@ const RangesManager = {
         { status_code: rangesWereCollapsed ? 'saved' : 'unsaved' }
       )
     }
-    const response = RangesManager._getRanges(rangesTracker)
+    const newRanges = RangesManager._getRanges(rangesTracker)
     logger.debug(
       {
         projectId,
         docId,
-        changesCount:
-          response.changes != null ? response.changes.length : undefined,
-        commentsCount:
-          response.comments != null ? response.comments.length : undefined,
+        changesCount: newRanges.changes?.length,
+        commentsCount: newRanges.comments?.length,
         rangesWereCollapsed,
       },
       'applied updates to ranges'
     )
-    return callback(null, response, rangesWereCollapsed)
+    return { newRanges, rangesWereCollapsed }
   },
 
-  acceptChanges(changeIds, ranges, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
+  acceptChanges(changeIds, ranges) {
     const { changes, comments } = ranges
     logger.debug(`accepting ${changeIds.length} changes in ranges`)
     const rangesTracker = new RangesTracker(changes, comments)
     rangesTracker.removeChangeIds(changeIds)
-    const response = RangesManager._getRanges(rangesTracker)
-    return callback(null, response)
+    const newRanges = RangesManager._getRanges(rangesTracker)
+    return newRanges
   },
 
-  deleteComment(commentId, ranges, callback) {
-    if (callback == null) {
-      callback = function () {}
-    }
+  deleteComment(commentId, ranges) {
     const { changes, comments } = ranges
     logger.debug({ commentId }, 'deleting comment in ranges')
     const rangesTracker = new RangesTracker(changes, comments)
     rangesTracker.removeCommentId(commentId)
-    const response = RangesManager._getRanges(rangesTracker)
-    return callback(null, response)
+    const newRanges = RangesManager._getRanges(rangesTracker)
+    return newRanges
   },
 
   _getRanges(rangesTracker) {
     // Return the minimal data structure needed, since most documents won't have any
     // changes or comments
     let response = {}
-    if (
-      (rangesTracker.changes != null
-        ? rangesTracker.changes.length
-        : undefined) > 0
-    ) {
+    if (rangesTracker.changes != null && rangesTracker.changes.length > 0) {
       if (response == null) {
         response = {}
       }
       response.changes = rangesTracker.changes
     }
-    if (
-      (rangesTracker.comments != null
-        ? rangesTracker.comments.length
-        : undefined) > 0
-    ) {
+    if (rangesTracker.comments != null && rangesTracker.comments.length > 0) {
       if (response == null) {
         response = {}
       }
@@ -159,13 +117,13 @@ const RangesManager = {
   _emptyRangesCount(ranges) {
     let emptyCount = 0
     let totalCount = 0
-    for (const comment of Array.from(ranges.comments || [])) {
+    for (const comment of ranges.comments || []) {
       totalCount++
       if (comment.op.c === '') {
         emptyCount++
       }
     }
-    for (const change of Array.from(ranges.changes || [])) {
+    for (const change of ranges.changes || []) {
       totalCount++
       if (change.op.i != null) {
         if (change.op.i === '') {
@@ -178,9 +136,3 @@ const RangesManager = {
 }
 
 module.exports = RangesManager
-module.exports.promises = promisifyAll(RangesManager, {
-  without: ['_getRanges', '_emptyRangesCount'],
-  multiResult: {
-    applyUpdate: ['newRanges', 'rangesWereCollapsed'],
-  },
-})

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

@@ -117,14 +117,13 @@ const UpdateManager = {
         sync: incomingUpdateVersion === previousVersion,
       })
 
-      const { newRanges, rangesWereCollapsed } =
-        await RangesManager.promises.applyUpdate(
-          projectId,
-          docId,
-          ranges,
-          appliedOps,
-          updatedDocLines
-        )
+      const { newRanges, rangesWereCollapsed } = RangesManager.applyUpdate(
+        projectId,
+        docId,
+        ranges,
+        appliedOps,
+        updatedDocLines
+      )
       profile.log('RangesManager.applyUpdate', { sync: true })
 
       UpdateManager._addProjectHistoryMetadataToOps(

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

@@ -731,7 +731,7 @@ describe('DocumentManager', function () {
         .yields(null, this.lines, this.version, this.ranges)
       this.RangesManager.acceptChanges = sinon
         .stub()
-        .yields(null, this.updated_ranges)
+        .returns(this.updated_ranges)
       this.RedisManager.updateDocument = sinon.stub().yields()
     })
 
@@ -830,7 +830,7 @@ describe('DocumentManager', function () {
         .yields(null, this.lines, this.version, this.ranges)
       this.RangesManager.deleteComment = sinon
         .stub()
-        .yields(null, this.updated_ranges)
+        .returns(this.updated_ranges)
       this.RedisManager.updateDocument = sinon.stub().yields()
     })
 

services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js

@@ -1,23 +1,12 @@
-/* eslint-disable
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const sinon = require('sinon')
 const { expect } = require('chai')
-const modulePath = '../../../../app/js/RangesManager.js'
 const SandboxedModule = require('sandboxed-module')
 
+const MODULE_PATH = '../../../../app/js/RangesManager.js'
+
 describe('RangesManager', function () {
   beforeEach(function () {
-    this.RangesManager = SandboxedModule.require(modulePath, {
+    this.RangesManager = SandboxedModule.require(MODULE_PATH, {
       requires: {
         '@overleaf/metrics': (this.Metrics = { histogram: sinon.stub() }),
       },
@@ -26,7 +15,6 @@ describe('RangesManager', function () {
     this.doc_id = 'doc-id-123'
     this.project_id = 'project-id-123'
     this.user_id = 'user-id-123'
-    return (this.callback = sinon.stub())
   })
 
   describe('applyUpdate', function () {
@@ -68,33 +56,27 @@ describe('RangesManager', function () {
           },
         ],
       }
-      return (this.newDocLines = ['one two three four five'])
+      this.newDocLines = ['one two three four five']
     }) // old is "one three four five"
 
     describe('successfully', function () {
       beforeEach(function () {
-        return this.RangesManager.applyUpdate(
+        this.result = this.RangesManager.applyUpdate(
           this.project_id,
           this.doc_id,
           this.entries,
           this.updates,
-          this.newDocLines,
-          this.callback
+          this.newDocLines
         )
       })
 
-      return it('should return the modified the comments and changes', function () {
-        this.callback.called.should.equal(true)
-        const [error, entries, rangesWereCollapsed] = Array.from(
-          this.callback.args[0]
-        )
-        expect(error).to.be.null
-        expect(rangesWereCollapsed).to.equal(false)
-        entries.comments[0].op.should.deep.equal({
+      it('should return the modified the comments and changes', function () {
+        expect(this.result.rangesWereCollapsed).to.equal(false)
+        this.result.newRanges.comments[0].op.should.deep.equal({
           c: 'three ',
           p: 8,
         })
-        return entries.changes[0].op.should.deep.equal({
+        this.result.newRanges.changes[0].op.should.deep.equal({
           i: 'five',
           p: 19,
         })
@@ -104,44 +86,36 @@ describe('RangesManager', function () {
     describe('with empty comments', function () {
       beforeEach(function () {
         this.entries.comments = []
-        return this.RangesManager.applyUpdate(
+        this.result = this.RangesManager.applyUpdate(
           this.project_id,
           this.doc_id,
           this.entries,
           this.updates,
-          this.newDocLines,
-          this.callback
+          this.newDocLines
         )
       })
 
-      return it('should return an object with no comments', function () {
+      it('should return an object with no comments', function () {
         // Save space in redis and don't store just {}
-        this.callback.called.should.equal(true)
-        const [error, entries] = Array.from(this.callback.args[0])
-        expect(error).to.be.null
-        return expect(entries.comments).to.be.undefined
+        expect(this.result.newRanges.comments).to.be.undefined
       })
     })
 
     describe('with empty changes', function () {
       beforeEach(function () {
         this.entries.changes = []
-        return this.RangesManager.applyUpdate(
+        this.result = this.RangesManager.applyUpdate(
           this.project_id,
           this.doc_id,
           this.entries,
           this.updates,
-          this.newDocLines,
-          this.callback
+          this.newDocLines
         )
       })
 
-      return it('should return an object with no changes', function () {
+      it('should return an object with no changes', function () {
         // Save space in redis and don't store just {}
-        this.callback.called.should.equal(true)
-        const [error, entries] = Array.from(this.callback.args[0])
-        expect(error).to.be.null
-        return expect(entries.changes).to.be.undefined
+        expect(this.result.newRanges.changes).to.be.undefined
       })
     })
 
@@ -187,23 +161,18 @@ describe('RangesManager', function () {
           ],
           changes: [],
         }
-        return this.RangesManager.applyUpdate(
-          this.project_id,
-          this.doc_id,
-          this.entries,
-          this.updates,
-          this.newDocLines,
-          this.callback
-        )
       })
 
-      return it('should return an error', function () {
-        this.callback.called.should.equal(true)
-        const [error, entries] = Array.from(this.callback.args[0])
-        expect(error).to.not.be.null
-        return expect(error.message).to.equal(
-          'too many comments or tracked changes'
-        )
+      it('should throw an error', function () {
+        expect(() => {
+          this.RangesManager.applyUpdate(
+            this.project_id,
+            this.doc_id,
+            this.entries,
+            this.updates,
+            this.newDocLines
+          )
+        }).to.throw('too many comments or tracked changes')
       })
     })
 
@@ -248,24 +217,18 @@ describe('RangesManager', function () {
           comments: [],
         }
         this.newDocLines = ['one two three four']
-        return this.RangesManager.applyUpdate(
-          this.project_id,
-          this.doc_id,
-          this.entries,
-          this.updates,
-          this.newDocLines,
-          this.callback
-        )
       })
 
-      return it('should return an error', function () {
-        // Save space in redis and don't store just {}
-        this.callback.called.should.equal(true)
-        const [error, entries] = Array.from(this.callback.args[0])
-        expect(error).to.not.be.null
-        return expect(error.message).to.equal(
-          'too many comments or tracked changes'
-        )
+      it('should throw an error', function () {
+        expect(() => {
+          this.RangesManager.applyUpdate(
+            this.project_id,
+            this.doc_id,
+            this.entries,
+            this.updates,
+            this.newDocLines
+          )
+        }).to.throw('too many comments or tracked changes')
       })
     })
 
@@ -284,22 +247,18 @@ describe('RangesManager', function () {
             ],
           },
         ]
-        return this.RangesManager.applyUpdate(
-          this.project_id,
-          this.doc_id,
-          this.entries,
-          this.updates,
-          this.newDocLines,
-          this.callback
-        )
       })
 
-      return it('should return an error', function () {
-        // Save space in redis and don't store just {}
-        this.callback.called.should.equal(true)
-        const [error, entries] = Array.from(this.callback.args[0])
-        expect(error).to.not.be.null
-        return expect(error.message).to.equal(
+      it('should throw an error', function () {
+        expect(() => {
+          this.RangesManager.applyUpdate(
+            this.project_id,
+            this.doc_id,
+            this.entries,
+            this.updates,
+            this.newDocLines
+          )
+        }).to.throw(
           'Change ({"op":{"i":"five","p":15},"metadata":{"user_id":"user-id-123"}}) doesn\'t match text ("our ")'
         )
       })
@@ -336,22 +295,17 @@ describe('RangesManager', function () {
           ],
           changes: [],
         }
-        return this.RangesManager.applyUpdate(
+        this.result = this.RangesManager.applyUpdate(
           this.project_id,
           this.doc_id,
           this.entries,
           this.updates,
-          this.newDocLines,
-          this.callback
+          this.newDocLines
         )
       })
 
-      return it('should return ranges_were_collapsed == true', function () {
-        this.callback.called.should.equal(true)
-        const [error, entries, rangesWereCollapsed] = Array.from(
-          this.callback.args[0]
-        )
-        return expect(rangesWereCollapsed).to.equal(true)
+      it('should return ranges_were_collapsed == true', function () {
+        expect(this.result.rangesWereCollapsed).to.equal(true)
       })
     })
 
@@ -396,13 +350,12 @@ describe('RangesManager', function () {
             },
           ],
         }
-        return this.RangesManager.applyUpdate(
+        this.result = this.RangesManager.applyUpdate(
           this.project_id,
           this.doc_id,
           this.entries,
           this.updates,
-          this.newDocLines,
-          this.callback
+          this.newDocLines
         )
       })
 
@@ -410,19 +363,15 @@ describe('RangesManager', function () {
         this.Metrics.histogram.called.should.equal(true)
       })
 
-      return it('should return ranges_were_collapsed == true', function () {
-        this.callback.called.should.equal(true)
-        const [error, entries, rangesWereCollapsed] = Array.from(
-          this.callback.args[0]
-        )
-        return expect(rangesWereCollapsed).to.equal(true)
+      it('should return ranges_were_collapsed == true', function () {
+        expect(this.result.rangesWereCollapsed).to.equal(true)
       })
     })
   })
 
-  return describe('acceptChanges', function () {
+  describe('acceptChanges', function () {
     beforeEach(function () {
-      this.RangesManager = SandboxedModule.require(modulePath, {
+      this.RangesManager = SandboxedModule.require(MODULE_PATH, {
         requires: {
           '@overleaf/ranges-tracker': (this.RangesTracker =
             SandboxedModule.require('@overleaf/ranges-tracker')),
@@ -470,118 +419,99 @@ describe('RangesManager', function () {
           },
         ],
       }
-      return (this.removeChangeIdsSpy = sinon.spy(
+      this.removeChangeIdsSpy = sinon.spy(
         this.RangesTracker.prototype,
         'removeChangeIds'
-      ))
+      )
     })
 
     describe('successfully with a single change', function () {
-      beforeEach(function (done) {
+      beforeEach(function () {
         this.change_ids = [this.ranges.changes[1].id]
-        return this.RangesManager.acceptChanges(
+        this.result = this.RangesManager.acceptChanges(
           this.change_ids,
-          this.ranges,
-          (err, ranges) => {
-            if (err) return done(err)
-            this.rangesResponse = ranges
-            return done()
-          }
+          this.ranges
         )
       })
 
       it('should log the call with the correct number of changes', function () {
-        return this.logger.debug
+        this.logger.debug
           .calledWith('accepting 1 changes in ranges')
           .should.equal(true)
       })
 
       it('should delegate the change removal to the ranges tracker', function () {
-        return this.removeChangeIdsSpy
-          .calledWith(this.change_ids)
-          .should.equal(true)
+        this.removeChangeIdsSpy.calledWith(this.change_ids).should.equal(true)
       })
 
       it('should remove the change', function () {
-        return expect(
-          this.rangesResponse.changes.find(
+        expect(
+          this.result.changes.find(
             change => change.id === this.ranges.changes[1].id
           )
         ).to.be.undefined
       })
 
       it('should return the original number of changes minus 1', function () {
-        return this.rangesResponse.changes.length.should.equal(
-          this.ranges.changes.length - 1
-        )
+        this.result.changes.length.should.equal(this.ranges.changes.length - 1)
       })
 
-      return it('should not touch other changes', function () {
-        return [0, 2, 3, 4].map(i =>
+      it('should not touch other changes', function () {
+        for (const i of [0, 2, 3, 4]) {
           expect(
-            this.rangesResponse.changes.find(
+            this.result.changes.find(
               change => change.id === this.ranges.changes[i].id
             )
           ).to.deep.equal(this.ranges.changes[i])
-        )
+        }
       })
     })
 
-    return describe('successfully with multiple changes', function () {
-      beforeEach(function (done) {
+    describe('successfully with multiple changes', function () {
+      beforeEach(function () {
         this.change_ids = [
           this.ranges.changes[1].id,
           this.ranges.changes[3].id,
           this.ranges.changes[4].id,
         ]
-        return this.RangesManager.acceptChanges(
+        this.result = this.RangesManager.acceptChanges(
           this.change_ids,
-          this.ranges,
-          (err, ranges) => {
-            if (err) return done(err)
-            this.rangesResponse = ranges
-            return done()
-          }
+          this.ranges
         )
       })
 
       it('should log the call with the correct number of changes', function () {
-        return this.logger.debug
+        this.logger.debug
           .calledWith(`accepting ${this.change_ids.length} changes in ranges`)
           .should.equal(true)
       })
 
       it('should delegate the change removal to the ranges tracker', function () {
-        return this.removeChangeIdsSpy
-          .calledWith(this.change_ids)
-          .should.equal(true)
+        this.removeChangeIdsSpy.calledWith(this.change_ids).should.equal(true)
       })
 
       it('should remove the changes', function () {
-        return [1, 3, 4].map(
-          i =>
-            expect(
-              this.rangesResponse.changes.find(
-                change => change.id === this.ranges.changes[1].id
-              )
-            ).to.be.undefined
-        )
+        for (const i of [1, 3, 4]) {
+          expect(
+            this.result.changes.find(
+              change => change.id === this.ranges.changes[i].id
+            )
+          ).to.be.undefined
+        }
       })
 
       it('should return the original number of changes minus the number of accepted changes', function () {
-        return this.rangesResponse.changes.length.should.equal(
-          this.ranges.changes.length - 3
-        )
+        this.result.changes.length.should.equal(this.ranges.changes.length - 3)
       })
 
-      return it('should not touch other changes', function () {
-        return [0, 2].map(i =>
+      it('should not touch other changes', function () {
+        for (const i of [0, 2]) {
           expect(
-            this.rangesResponse.changes.find(
+            this.result.changes.find(
               change => change.id === this.ranges.changes[i].id
             )
           ).to.deep.equal(this.ranges.changes[i])
-        )
+        }
       })
     })
   })

services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js

@@ -65,9 +65,7 @@ describe('UpdateManager', function () {
     }
 
     this.RangesManager = {
-      promises: {
-        applyUpdate: sinon.stub(),
-      },
+      applyUpdate: sinon.stub(),
     }
 
     this.SnapshotManager = {
@@ -320,7 +318,7 @@ describe('UpdateManager', function () {
         pathname: this.pathname,
         projectHistoryId: this.projectHistoryId,
       })
-      this.RangesManager.promises.applyUpdate.resolves({
+      this.RangesManager.applyUpdate.returns({
         newRanges: this.updated_ranges,
         rangesWereCollapsed: false,
       })
@@ -357,7 +355,7 @@ describe('UpdateManager', function () {
       })
 
       it('should update the ranges', function () {
-        this.RangesManager.promises.applyUpdate
+        this.RangesManager.applyUpdate
           .calledWith(
             this.project_id,
             this.doc_id,
@@ -444,7 +442,7 @@ describe('UpdateManager', function () {
 
     describe('when ranges get collapsed', function () {
       beforeEach(async function () {
-        this.RangesManager.promises.applyUpdate.resolves({
+        this.RangesManager.applyUpdate.returns({
           newRanges: this.updated_ranges,
           rangesWereCollapsed: true,
         })