Merge pull request #16982 from overleaf/em-history-ranges-support

Store in Redis whether docs support ranges in the history

GitOrigin-RevId: 040acd8dbdb2f3cddfccb90af902612cf5c0edda

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

@@ -29,7 +29,10 @@ const DocumentManager = {
         ranges,
         pathname,
         projectHistoryId,
-        unflushedTime
+        unflushedTime,
+        lastUpdatedAt,
+        lastUpdatedBy,
+        historyRangesSupport
       ) => {
         if (error) {
           return callback(error)
@@ -42,7 +45,15 @@ const DocumentManager = {
           PersistenceManager.getDoc(
             projectId,
             docId,
-            (error, lines, version, ranges, pathname, projectHistoryId) => {
+            (
+              error,
+              lines,
+              version,
+              ranges,
+              pathname,
+              projectHistoryId,
+              historyRangesSupport
+            ) => {
               if (error) {
                 return callback(error)
               }
@@ -54,6 +65,7 @@ const DocumentManager = {
                   version,
                   pathname,
                   projectHistoryId,
+                  historyRangesSupport,
                 },
                 'got doc from persistence API'
               )
@@ -65,6 +77,7 @@ const DocumentManager = {
                 ranges,
                 pathname,
                 projectHistoryId,
+                historyRangesSupport,
                 error => {
                   if (error) {
                     return callback(error)
@@ -77,7 +90,8 @@ const DocumentManager = {
                     pathname,
                     projectHistoryId,
                     null,
-                    false
+                    false,
+                    historyRangesSupport
                   )
                 }
               )
@@ -92,7 +106,8 @@ const DocumentManager = {
             pathname,
             projectHistoryId,
             unflushedTime,
-            true
+            true,
+            historyRangesSupport
           )
         }
       }
@@ -698,6 +713,7 @@ module.exports.promises = promisifyAll(DocumentManager, {
       'projectHistoryId',
       'unflushedTime',
       'alreadyLoaded',
+      'historyRangesSupport',
     ],
     getDocWithLock: [
       'lines',
@@ -707,6 +723,7 @@ module.exports.promises = promisifyAll(DocumentManager, {
       'projectHistoryId',
       'unflushedTime',
       'alreadyLoaded',
+      'historyRangesSupport',
     ],
     getDocAndFlushIfOld: ['lines', 'version'],
     getDocAndFlushIfOldWithLock: ['lines', 'version'],

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

@@ -101,7 +101,8 @@ function getDoc(projectId, docId, options = {}, _callback) {
         body.version,
         body.ranges,
         body.pathname,
-        body.projectHistoryId?.toString()
+        body.projectHistoryId?.toString(),
+        body.historyRangesSupport || false
       )
     } else if (res.statusCode === 404) {
       callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`))
@@ -186,6 +187,7 @@ module.exports = {
       'ranges',
       'pathname',
       'projectHistoryId',
+      'historyRangesSupport',
     ]),
     setDoc: promisify(setDoc),
   },

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

@@ -38,6 +38,7 @@ const RedisManager = {
     ranges,
     pathname,
     projectHistoryId,
+    historyRangesSupport,
     _callback
   ) {
     const timer = new metrics.Timer('redis.put-doc')
@@ -89,18 +90,20 @@ const RedisManager = {
             })
           }
 
-          rclient.mset(
+          const multi = rclient.multi()
-            {
+          multi.mset({
-              [keys.docLines({ doc_id: docId })]: docLines,
+            [keys.docLines({ doc_id: docId })]: docLines,
-              [keys.projectKey({ doc_id: docId })]: projectId,
+            [keys.projectKey({ doc_id: docId })]: projectId,
-              [keys.docVersion({ doc_id: docId })]: version,
+            [keys.docVersion({ doc_id: docId })]: version,
-              [keys.docHash({ doc_id: docId })]: docHash,
+            [keys.docHash({ doc_id: docId })]: docHash,
-              [keys.ranges({ doc_id: docId })]: ranges,
+            [keys.ranges({ doc_id: docId })]: ranges,
-              [keys.pathname({ doc_id: docId })]: pathname,
+            [keys.pathname({ doc_id: docId })]: pathname,
-              [keys.projectHistoryId({ doc_id: docId })]: projectHistoryId,
+            [keys.projectHistoryId({ doc_id: docId })]: projectHistoryId,
-            },
+          })
-            callback
+          if (historyRangesSupport) {
-          )
+            multi.sadd(keys.historyRangesSupport(), docId)
+          }
+          multi.exec(callback)
         }
       )
     })
@@ -144,7 +147,12 @@ const RedisManager = {
       multi = rclient.multi()
       multi.srem(keys.docsInProject({ project_id: projectId }), docId)
       multi.del(keys.projectState({ project_id: projectId }))
-      multi.exec(callback)
+      multi.exec(err => {
+        if (err) {
+          return callback(err)
+        }
+        rclient.srem(keys.historyRangesSupport(), docId, callback)
+      })
     })
   },
 
@@ -198,68 +206,76 @@ const RedisManager = {
         lastUpdatedAt,
         lastUpdatedBy,
       ] = result
-      const timeSpan = timer.done()
+      rclient.sismember(keys.historyRangesSupport(), docId, (error, result) => {
-      // check if request took too long and bail out.  only do this for
+        if (error) {
-      // get, because it is the first call in each update, so if this
+          return callback(error)
-      // passes we'll assume others have a reasonable chance to succeed.
-      if (timeSpan > MAX_REDIS_REQUEST_LENGTH) {
-        error = new Error('redis getDoc exceeded timeout')
-        return callback(error)
-      }
-      // record bytes loaded from redis
-      if (docLines != null) {
-        metrics.summary('redis.docLines', docLines.length, { status: 'get' })
-      }
-      // check sha1 hash value if present
-      if (docLines != null && storedHash != null) {
-        const computedHash = RedisManager._computeHash(docLines)
-        if (logHashReadErrors && computedHash !== storedHash) {
-          logger.error(
-            {
-              projectId,
-              docId,
-              docProjectId,
-              computedHash,
-              storedHash,
-              docLines,
-            },
-            'hash mismatch on retrieved document'
-          )
         }
-      }
+        const historyRangesSupport = result === 1
 
-      try {
+        const timeSpan = timer.done()
-        docLines = JSON.parse(docLines)
+        // check if request took too long and bail out.  only do this for
-        ranges = RedisManager._deserializeRanges(ranges)
+        // get, because it is the first call in each update, so if this
-      } catch (e) {
+        // passes we'll assume others have a reasonable chance to succeed.
-        return callback(e)
+        if (timeSpan > MAX_REDIS_REQUEST_LENGTH) {
-      }
+          error = new Error('redis getDoc exceeded timeout')
+          return callback(error)
+        }
+        // record bytes loaded from redis
+        if (docLines != null) {
+          metrics.summary('redis.docLines', docLines.length, { status: 'get' })
+        }
+        // check sha1 hash value if present
+        if (docLines != null && storedHash != null) {
+          const computedHash = RedisManager._computeHash(docLines)
+          if (logHashReadErrors && computedHash !== storedHash) {
+            logger.error(
+              {
+                projectId,
+                docId,
+                docProjectId,
+                computedHash,
+                storedHash,
+                docLines,
+              },
+              'hash mismatch on retrieved document'
+            )
+          }
+        }
 
-      version = parseInt(version || 0, 10)
+        try {
-      // check doc is in requested project
+          docLines = JSON.parse(docLines)
-      if (docProjectId != null && docProjectId !== projectId) {
+          ranges = RedisManager._deserializeRanges(ranges)
-        logger.error({ projectId, docId, docProjectId }, 'doc not in project')
+        } catch (e) {
-        return callback(new Errors.NotFoundError('document not found'))
+          return callback(e)
-      }
+        }
 
-      if (docLines && version && !pathname) {
+        version = parseInt(version || 0, 10)
-        metrics.inc('pathname', 1, {
+        // check doc is in requested project
-          path: 'RedisManager.getDoc',
+        if (docProjectId != null && docProjectId !== projectId) {
-          status: pathname === '' ? 'zero-length' : 'undefined',
+          logger.error({ projectId, docId, docProjectId }, 'doc not in project')
-        })
+          return callback(new Errors.NotFoundError('document not found'))
-      }
+        }
 
-      callback(
+        if (docLines && version && !pathname) {
-        null,
+          metrics.inc('pathname', 1, {
-        docLines,
+            path: 'RedisManager.getDoc',
-        version,
+            status: pathname === '' ? 'zero-length' : 'undefined',
-        ranges,
+          })
-        pathname,
+        }
-        projectHistoryId,
+
-        unflushedTime,
+        callback(
-        lastUpdatedAt,
+          null,
-        lastUpdatedBy
+          docLines,
-      )
+          version,
+          ranges,
+          pathname,
+          projectHistoryId,
+          unflushedTime,
+          lastUpdatedAt,
+          lastUpdatedBy,
+          historyRangesSupport
+        )
+      })
     })
   },
 
@@ -631,6 +647,7 @@ module.exports.promises = promisifyAll(RedisManager, {
       'unflushedTime',
       'lastUpdatedAt',
       'lastUpdatedBy',
+      'historyRangesSupport',
     ],
     getNextProjectToFlushAndDelete: [
       'projectId',

services/document-updater/config/settings.defaults.js

@@ -140,6 +140,9 @@ module.exports = {
         flushAndDeleteQueue() {
           return 'DocUpdaterFlushAndDeleteQueue'
         },
+        historyRangesSupport() {
+          return 'HistoryRangesSupport'
+        },
       },
     },
   },

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

@@ -42,6 +42,7 @@ describe('DocumentManager', function () {
     this.lastUpdatedAt = Date.now()
     this.lastUpdatedBy = 'last-author-id'
     this.source = 'external-source'
+    this.historyRangesSupport = false
   })
 
   afterEach(function () {
@@ -335,7 +336,10 @@ describe('DocumentManager', function () {
             this.ranges,
             this.pathname,
             this.projectHistoryId,
-            this.unflushedTime
+            this.unflushedTime,
+            this.lastUpdatedAt,
+            this.lastUpdatedBy,
+            this.historyRangesSupport
           )
         this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback)
       })
@@ -356,7 +360,8 @@ describe('DocumentManager', function () {
             this.pathname,
             this.projectHistoryId,
             this.unflushedTime,
-            true
+            true,
+            this.historyRangesSupport
           )
           .should.equal(true)
       })
@@ -380,7 +385,8 @@ describe('DocumentManager', function () {
             this.version,
             this.ranges,
             this.pathname,
-            this.projectHistoryId
+            this.projectHistoryId,
+            this.historyRangesSupport
           )
         this.RedisManager.putDocInMemory = sinon.stub().yields()
         this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback)
@@ -407,7 +413,8 @@ describe('DocumentManager', function () {
             this.version,
             this.ranges,
             this.pathname,
-            this.projectHistoryId
+            this.projectHistoryId,
+            this.historyRangesSupport
           )
           .should.equal(true)
       })
@@ -422,7 +429,8 @@ describe('DocumentManager', function () {
             this.pathname,
             this.projectHistoryId,
             null,
-            false
+            false,
+            this.historyRangesSupport
           )
           .should.equal(true)
       })

services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js

@@ -32,6 +32,7 @@ describe('PersistenceManager', function () {
     this.pathname = '/a/b/c.tex'
     this.lastUpdatedAt = Date.now()
     this.lastUpdatedBy = 'last-author-id'
+    this.historyRangesSupport = false
     this.Settings.apis = {
       web: {
         url: (this.url = 'www.example.com'),
@@ -49,6 +50,7 @@ describe('PersistenceManager', function () {
         ranges: this.ranges,
         pathname: this.pathname,
         projectHistoryId: this.projectHistoryId,
+        historyRangesSupport: this.historyRangesSupport,
       }
     })
 
@@ -94,7 +96,8 @@ describe('PersistenceManager', function () {
             this.version,
             this.ranges,
             this.pathname,
-            this.projectHistoryId
+            this.projectHistoryId,
+            this.historyRangesSupport
           )
           .should.equal(true)
       })

services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js

@@ -7,8 +7,8 @@ const tk = require('timekeeper')
 
 describe('RedisManager', function () {
   beforeEach(function () {
-    this.multi = { exec: sinon.stub() }
+    this.multi = { exec: sinon.stub().yields() }
-    this.rclient = { multi: () => this.multi }
+    this.rclient = { multi: () => this.multi, srem: sinon.stub().yields() }
     tk.freeze(new Date())
     this.RedisManager = SandboxedModule.require(modulePath, {
       requires: {
@@ -63,6 +63,9 @@ describe('RedisManager', function () {
                 lastUpdatedAt({ doc_id: docId }) {
                   return `lastUpdatedAt:${docId}`
                 },
+                historyRangesSupport() {
+                  return 'HistoryRangesSupport'
+                },
               },
             },
           },
@@ -91,6 +94,7 @@ describe('RedisManager', function () {
     this.docId = 'doc-id-123'
     this.project_id = 'project-id-123'
     this.projectHistoryId = '123'
+    this.historyRangesSupport = false
     this.callback = sinon.stub()
   })
 
@@ -123,6 +127,10 @@ describe('RedisManager', function () {
           this.projectHistoryId.toString(),
           this.unflushed_time,
         ])
+      this.rclient.sismember = sinon.stub()
+      this.rclient.sismember
+        .withArgs('HistoryRangesSupport', this.docId)
+        .yields(null, 0)
     })
 
     describe('successfully', function () {
@@ -148,19 +156,18 @@ describe('RedisManager', function () {
       })
 
       it('should return the document', function () {
-        this.callback
+        this.callback.should.have.been.calledWithExactly(
-          .calledWithExactly(
+          null,
-            null,
+          this.lines,
-            this.lines,
+          this.version,
-            this.version,
+          this.ranges,
-            this.ranges,
+          this.pathname,
-            this.pathname,
+          this.projectHistoryId,
-            this.projectHistoryId,
+          this.unflushed_time,
-            this.unflushed_time,
+          this.lastUpdatedAt,
-            this.lastUpdatedAt,
+          this.lastUpdatedBy,
-            this.lastUpdatedBy
+          this.historyRangesSupport
-          )
+        )
-          .should.equal(true)
       })
 
       it('should not log any errors', function () {
@@ -247,6 +254,30 @@ describe('RedisManager', function () {
           .should.equal(true)
       })
     })
+
+    describe('with history ranges support', function () {
+      beforeEach(function () {
+        this.rclient.sismember
+          .withArgs('HistoryRangesSupport', this.docId)
+          .yields(null, 1)
+        this.RedisManager.getDoc(this.project_id, this.docId, this.callback)
+      })
+
+      it('should return the document with the history ranges flag set', function () {
+        this.callback.should.have.been.calledWithExactly(
+          null,
+          this.lines,
+          this.version,
+          this.ranges,
+          this.pathname,
+          this.projectHistoryId,
+          this.unflushed_time,
+          this.lastUpdatedAt,
+          this.lastUpdatedBy,
+          true
+        )
+      })
+    })
   })
 
   describe('getPreviousDocOpsTests', function () {
@@ -765,7 +796,8 @@ describe('RedisManager', function () {
 
   describe('putDocInMemory', function () {
     beforeEach(function () {
-      this.rclient.mset = sinon.stub().yields(null)
+      this.multi.mset = sinon.stub()
+      this.multi.sadd = sinon.stub()
       this.rclient.sadd = sinon.stub().yields()
       this.lines = ['one', 'two', 'three', 'これは']
       this.version = 42
@@ -787,12 +819,13 @@ describe('RedisManager', function () {
           this.ranges,
           this.pathname,
           this.projectHistoryId,
+          this.historyRangesSupport,
           done
         )
       })
 
       it('should set all the details in a single MSET call', function () {
-        this.rclient.mset
+        this.multi.mset
           .calledWith({
             [`doclines:${this.docId}`]: JSON.stringify(this.lines),
             [`ProjectId:${this.docId}`]: this.project_id,
@@ -814,6 +847,10 @@ describe('RedisManager', function () {
       it('should not log any errors', function () {
         this.logger.error.calledWith().should.equal(false)
       })
+
+      it('should not add the document to the HistoryRangesSupport set in Redis', function () {
+        this.multi.sadd.should.not.have.been.calledWith('HistoryRangesSupport')
+      })
     })
 
     describe('with empty ranges', function () {
@@ -826,22 +863,21 @@ describe('RedisManager', function () {
           {},
           this.pathname,
           this.projectHistoryId,
+          this.historyRangesSupport,
           done
         )
       })
 
       it('should unset ranges', function () {
-        this.rclient.mset
+        this.multi.mset.should.have.been.calledWith({
-          .calledWith({
+          [`doclines:${this.docId}`]: JSON.stringify(this.lines),
-            [`doclines:${this.docId}`]: JSON.stringify(this.lines),
+          [`ProjectId:${this.docId}`]: this.project_id,
-            [`ProjectId:${this.docId}`]: this.project_id,
+          [`DocVersion:${this.docId}`]: this.version,
-            [`DocVersion:${this.docId}`]: this.version,
+          [`DocHash:${this.docId}`]: this.hash,
-            [`DocHash:${this.docId}`]: this.hash,
+          [`Ranges:${this.docId}`]: null,
-            [`Ranges:${this.docId}`]: null,
+          [`Pathname:${this.docId}`]: this.pathname,
-            [`Pathname:${this.docId}`]: this.pathname,
+          [`ProjectHistoryId:${this.docId}`]: this.projectHistoryId,
-            [`ProjectHistoryId:${this.docId}`]: this.projectHistoryId,
+        })
-          })
-          .should.equal(true)
       })
     })
 
@@ -858,6 +894,7 @@ describe('RedisManager', function () {
           this.ranges,
           this.pathname,
           this.projectHistoryId,
+          this.historyRangesSupport,
           this.callback
         )
       })
@@ -890,6 +927,7 @@ describe('RedisManager', function () {
           this.ranges,
           this.pathname,
           this.projectHistoryId,
+          this.historyRangesSupport,
           this.callback
         )
       })
@@ -904,6 +942,30 @@ describe('RedisManager', function () {
           .should.equal(true)
       })
     })
+
+    describe('with history ranges support', function () {
+      beforeEach(function (done) {
+        this.historyRangesSupport = true
+        this.RedisManager.putDocInMemory(
+          this.project_id,
+          this.docId,
+          this.lines,
+          this.version,
+          this.ranges,
+          this.pathname,
+          this.projectHistoryId,
+          this.historyRangesSupport,
+          done
+        )
+      })
+
+      it('should add the document to the HistoryRangesSupport set in Redis', function () {
+        this.multi.sadd.should.have.been.calledWith(
+          'HistoryRangesSupport',
+          this.docId
+        )
+      })
+    })
   })
 
   describe('removeDocFromMemory', function () {
@@ -941,6 +1003,13 @@ describe('RedisManager', function () {
         .calledWith(`DocsIn:${this.project_id}`, this.docId)
         .should.equal(true)
     })
+
+    it('should remove the docId from the HistoryRangesSupport set', function () {
+      this.rclient.srem.should.have.been.calledWith(
+        'HistoryRangesSupport',
+        this.docId
+      )
+    })
   })
 
   describe('clearProjectState', function () {