Merge pull request #16976 from overleaf/revert-16877-em-history-ranges-support

Revert "Store in Redis whether docs support ranges in the history"

GitOrigin-RevId: c682cb49814c59aaec068106ae39c76f0de95493

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

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

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

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

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

@@ -38,7 +38,6 @@ const RedisManager = {
     ranges,
     pathname,
     projectHistoryId,
-    historyRangesSupport,
     _callback
   ) {
     const timer = new metrics.Timer('redis.put-doc')
@@ -90,20 +89,18 @@ const RedisManager = {
             })
           }
 
-          const multi = rclient.multi()
-          multi.mset({
-            [keys.docLines({ doc_id: docId })]: docLines,
-            [keys.projectKey({ doc_id: docId })]: projectId,
-            [keys.docVersion({ doc_id: docId })]: version,
-            [keys.docHash({ doc_id: docId })]: docHash,
-            [keys.ranges({ doc_id: docId })]: ranges,
-            [keys.pathname({ doc_id: docId })]: pathname,
-            [keys.projectHistoryId({ doc_id: docId })]: projectHistoryId,
-          })
-          if (historyRangesSupport) {
-            multi.sadd(keys.historyRangesSupport(), docId)
-          }
-          multi.exec(callback)
+          rclient.mset(
+            {
+              [keys.docLines({ doc_id: docId })]: docLines,
+              [keys.projectKey({ doc_id: docId })]: projectId,
+              [keys.docVersion({ doc_id: docId })]: version,
+              [keys.docHash({ doc_id: docId })]: docHash,
+              [keys.ranges({ doc_id: docId })]: ranges,
+              [keys.pathname({ doc_id: docId })]: pathname,
+              [keys.projectHistoryId({ doc_id: docId })]: projectHistoryId,
+            },
+            callback
+          )
         }
       )
     })
@@ -135,7 +132,6 @@ const RedisManager = {
       keys.lastUpdatedAt({ doc_id: docId }),
       keys.lastUpdatedBy({ doc_id: docId })
     )
-    multi.srem(keys.historyRangesSupport(), docId)
     multi.exec((error, response) => {
       if (error) {
         return callback(error)
@@ -202,76 +198,68 @@ const RedisManager = {
         lastUpdatedAt,
         lastUpdatedBy,
       ] = result
-      rclient.sismember(keys.historyRangesSupport(), docId, (error, result) => {
-        if (error) {
-          return callback(error)
-        }
-        const historyRangesSupport = result === 1
-
-        const timeSpan = timer.done()
-        // check if request took too long and bail out.  only do this for
-        // get, because it is the first call in each update, so if this
-        // 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 timeSpan = timer.done()
+      // check if request took too long and bail out.  only do this for
+      // get, because it is the first call in each update, so if this
+      // 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'
+          )
         }
+      }
 
-        try {
-          docLines = JSON.parse(docLines)
-          ranges = RedisManager._deserializeRanges(ranges)
-        } catch (e) {
-          return callback(e)
-        }
+      try {
+        docLines = JSON.parse(docLines)
+        ranges = RedisManager._deserializeRanges(ranges)
+      } catch (e) {
+        return callback(e)
+      }
 
-        version = parseInt(version || 0, 10)
-        // check doc is in requested project
-        if (docProjectId != null && docProjectId !== projectId) {
-          logger.error({ projectId, docId, docProjectId }, 'doc not in project')
-          return callback(new Errors.NotFoundError('document not found'))
-        }
+      version = parseInt(version || 0, 10)
+      // check doc is in requested project
+      if (docProjectId != null && docProjectId !== projectId) {
+        logger.error({ projectId, docId, docProjectId }, 'doc not in project')
+        return callback(new Errors.NotFoundError('document not found'))
+      }
 
-        if (docLines && version && !pathname) {
-          metrics.inc('pathname', 1, {
-            path: 'RedisManager.getDoc',
-            status: pathname === '' ? 'zero-length' : 'undefined',
-          })
-        }
+      if (docLines && version && !pathname) {
+        metrics.inc('pathname', 1, {
+          path: 'RedisManager.getDoc',
+          status: pathname === '' ? 'zero-length' : 'undefined',
+        })
+      }
 
-        callback(
-          null,
-          docLines,
-          version,
-          ranges,
-          pathname,
-          projectHistoryId,
-          unflushedTime,
-          lastUpdatedAt,
-          lastUpdatedBy,
-          historyRangesSupport
-        )
-      })
+      callback(
+        null,
+        docLines,
+        version,
+        ranges,
+        pathname,
+        projectHistoryId,
+        unflushedTime,
+        lastUpdatedAt,
+        lastUpdatedBy
+      )
     })
   },
 
@@ -643,7 +631,6 @@ module.exports.promises = promisifyAll(RedisManager, {
       'unflushedTime',
       'lastUpdatedAt',
       'lastUpdatedBy',
-      'historyRangesSupport',
     ],
     getNextProjectToFlushAndDelete: [
       'projectId',

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

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

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

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

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

@@ -32,7 +32,6 @@ 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'),
@@ -50,7 +49,6 @@ describe('PersistenceManager', function () {
         ranges: this.ranges,
         pathname: this.pathname,
         projectHistoryId: this.projectHistoryId,
-        historyRangesSupport: this.historyRangesSupport,
       }
     })
 
@@ -96,8 +94,7 @@ describe('PersistenceManager', function () {
             this.version,
             this.ranges,
             this.pathname,
-            this.projectHistoryId,
-            this.historyRangesSupport
+            this.projectHistoryId
           )
           .should.equal(true)
       })

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

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