Merge pull request #14662 from overleaf/em-revert-docupdater-history-ranges-support

Revert document-updater changes from #14567 GitOrigin-RevId: 72af1d24c94f7c46f2557700e6425f278356cffd
2024-11-21 20:47:08 -05:00 · 2023-09-05 17:07:11 -04:00 · 2023-09-05 17:07:11 -04:00 · 1aff1b64ab
commit 1aff1b64ab
parent 5748ee4496
11 changed files with 167 additions and 155 deletions
--- a/package-lock.json
+++ b/package-lock.json
@ -44410,7 +44410,6 @@
        "mocha": "^10.2.0",
        "sandboxed-module": "^2.0.4",
        "sinon": "^9.2.4",
-        "sinon-chai": "^3.7.0",
        "timekeeper": "^2.0.0"
      }
    },
@ -53410,7 +53409,6 @@
        "requestretry": "^7.1.0",
        "sandboxed-module": "^2.0.4",
        "sinon": "^9.2.4",
-        "sinon-chai": "^3.7.0",
        "timekeeper": "^2.0.0"
      },
      "dependencies": {
--- a/services/document-updater/app/js/DocumentManager.js
+++ b/services/document-updater/app/js/DocumentManager.js
@ -39,27 +39,50 @@ module.exports = DocumentManager = {
            { projectId, docId },
            'doc not in redis so getting from persistence API'
          )
-          PersistenceManager.getDoc(projectId, docId, (error, doc) => {
+          PersistenceManager.getDoc(
+            projectId,
+            docId,
+            (error, lines, version, ranges, pathname, projectHistoryId) => {
              if (error) {
                return callback(error)
              }
-            logger.debug({ doc }, 'got doc from persistence API')
-            RedisManager.putDocInMemory(projectId, docId, doc, error => {
+              logger.debug(
+                {
+                  projectId,
+                  docId,
+                  lines,
+                  version,
+                  pathname,
+                  projectHistoryId,
+                },
+                'got doc from persistence API'
+              )
+              RedisManager.putDocInMemory(
+                projectId,
+                docId,
+                lines,
+                version,
+                ranges,
+                pathname,
+                projectHistoryId,
+                error => {
                  if (error) {
                    return callback(error)
                  }
                  callback(
                    null,
-                doc.lines,
-                doc.version,
-                doc.ranges || {},
-                doc.pathname,
-                doc.projectHistoryId,
+                    lines,
+                    version,
+                    ranges || {},
+                    pathname,
+                    projectHistoryId,
                    null,
                    false
                  )
-            })
-          })
+                }
+              )
+            }
+          )
        } else {
          callback(
            null,
@ -494,7 +517,7 @@ module.exports = DocumentManager = {
            projectId,
            docId,
            { peek: true },
-            (error, doc) => {
+            (error, lines, version, ranges, pathname, projectHistoryId) => {
              if (error) {
                logger.error(
                  { projectId, docId, getDocError: error },
@ -504,10 +527,10 @@ module.exports = DocumentManager = {
              }
              ProjectHistoryRedisManager.queueResyncDocContent(
                projectId,
-                doc.projectHistoryId,
+                projectHistoryId,
                docId,
-                doc.lines,
-                doc.version,
+                lines,
+                version,
                path, // use the path from the resyncProjectStructure update
                callback
              )
--- a/services/document-updater/app/js/PersistenceManager.js
+++ b/services/document-updater/app/js/PersistenceManager.js
@ -93,16 +93,16 @@ function getDoc(projectId, docId, options = {}, _callback) {
          status: body.pathname === '' ? 'zero-length' : 'undefined',
        })
      }
-      callback(null, {
-        lines: body.lines,
-        version: body.version,
-        ranges: body.ranges,
-        pathname: body.pathname,
-        projectHistoryId: body.projectHistoryId?.toString(),
-        historyRangesSupport: body.historyRangesSupport || false,
-      })
+      callback(
+        null,
+        body.lines,
+        body.version,
+        body.ranges,
+        body.pathname,
+        body.projectHistoryId?.toString()
+      )
    } else if (res.statusCode === 404) {
-      callback(new Errors.NotFoundError(`doc not found: ${urlPath}`))
+      callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`))
    } else if (res.statusCode === 413) {
      callback(
        new Errors.FileTooLargeError(`doc exceeds maximum size: ${urlPath}`)
@ -160,7 +160,7 @@ function setDoc(
      if (res.statusCode >= 200 && res.statusCode < 300) {
        callback(null, body)
      } else if (res.statusCode === 404) {
-        callback(new Errors.NotFoundError(`doc not found: ${urlPath}`))
+        callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`))
      } else if (res.statusCode === 413) {
        callback(
          new Errors.FileTooLargeError(`doc exceeds maximum size: ${urlPath}`)
--- a/services/document-updater/app/js/RedisManager.js
+++ b/services/document-updater/app/js/RedisManager.js
@ -30,44 +30,47 @@ const keys = Settings.redis.documentupdater.key_schema
 module.exports = RedisManager = {
  rclient,

-  putDocInMemory(projectId, docId, doc, _callback) {
+  putDocInMemory(
+    projectId,
+    docId,
+    docLines,
+    version,
+    ranges,
+    pathname,
+    projectHistoryId,
+    _callback
+  ) {
    const timer = new metrics.Timer('redis.put-doc')
    const callback = error => {
      timer.done()
      _callback(error)
    }
-    const docLinesJson = JSON.stringify(doc.lines)
-    if (docLinesJson.indexOf('\u0000') !== -1) {
+    const docLinesArray = docLines
+    docLines = JSON.stringify(docLines)
+    if (docLines.indexOf('\u0000') !== -1) {
      const error = new Error('null bytes found in doc lines')
      // this check was added to catch memory corruption in JSON.stringify.
      // It sometimes returned null bytes at the end of the string.
-      logger.error({ err: error, docId, docLines: docLinesJson }, error.message)
+      logger.error({ err: error, docId, docLines }, error.message)
      return callback(error)
    }
    // Do an optimised size check on the docLines using the serialised
    // length as an upper bound
-    const sizeBound = docLinesJson.length
-    if (docIsTooLarge(sizeBound, doc.lines, Settings.max_doc_length)) {
-      const docSize = docLinesJson.length
+    const sizeBound = docLines.length
+    if (docIsTooLarge(sizeBound, docLinesArray, Settings.max_doc_length)) {
+      const docSize = docLines.length
      const err = new Error('blocking doc insert into redis: doc is too large')
      logger.error({ projectId, docId, err, docSize }, err.message)
      return callback(err)
    }
-    const docHash = RedisManager._computeHash(docLinesJson)
+    const docHash = RedisManager._computeHash(docLines)
    // record bytes sent to redis
-    metrics.summary('redis.docLines', docLinesJson.length, { status: 'set' })
+    metrics.summary('redis.docLines', docLines.length, { status: 'set' })
    logger.debug(
-      {
-        projectId,
-        docId,
-        version: doc.version,
-        docHash,
-        pathname: doc.pathname,
-        projectHistoryId: doc.projectHistoryId,
-      },
+      { projectId, docId, version, docHash, pathname, projectHistoryId },
      'putting doc in redis'
    )
-    RedisManager._serializeRanges(doc.ranges, (error, ranges) => {
+    RedisManager._serializeRanges(ranges, (error, ranges) => {
      if (error) {
        logger.error({ err: error, docId, projectId }, error.message)
        return callback(error)
@ -79,27 +82,25 @@ module.exports = RedisManager = {
        error => {
          if (error) return callback(error)

-          if (!doc.pathname) {
+          if (!pathname) {
            metrics.inc('pathname', 1, {
              path: 'RedisManager.setDoc',
-              status: doc.pathname === '' ? 'zero-length' : 'undefined',
+              status: pathname === '' ? 'zero-length' : 'undefined',
            })
          }

-          const multi = rclient.multi()
-          multi.mset({
-            [keys.docLines({ doc_id: docId })]: docLinesJson,
+          rclient.mset(
+            {
+              [keys.docLines({ doc_id: docId })]: docLines,
              [keys.projectKey({ doc_id: docId })]: projectId,
-            [keys.docVersion({ doc_id: docId })]: doc.version,
+              [keys.docVersion({ doc_id: docId })]: version,
              [keys.docHash({ doc_id: docId })]: docHash,
              [keys.ranges({ doc_id: docId })]: ranges,
-            [keys.pathname({ doc_id: docId })]: doc.pathname,
-            [keys.projectHistoryId({ doc_id: docId })]: doc.projectHistoryId,
-          })
-          if (doc.historyRangesSupport) {
-            multi.sadd(keys.historyRangesSupport(), docId)
-          }
-          multi.exec(callback)
+              [keys.pathname({ doc_id: docId })]: pathname,
+              [keys.projectHistoryId({ doc_id: docId })]: projectHistoryId,
+            },
+            callback
+          )
        }
      )
    })
@ -131,7 +132,6 @@ module.exports = 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)
--- a/services/document-updater/config/settings.defaults.js
+++ b/services/document-updater/config/settings.defaults.js
@ -140,9 +140,6 @@ module.exports = {
        flushAndDeleteQueue() {
          return 'DocUpdaterFlushAndDeleteQueue'
        },
-        historyRangesSupport() {
-          return 'HistoryRangesSupport'
-        },
      },
    },
  },
--- a/services/document-updater/package.json
+++ b/services/document-updater/package.json
@ -39,7 +39,6 @@
    "mocha": "^10.2.0",
    "sandboxed-module": "^2.0.4",
    "sinon": "^9.2.4",
-    "sinon-chai": "^3.7.0",
    "timekeeper": "^2.0.0"
  }
 }
--- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js
+++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToADocTests.js
@ -679,7 +679,7 @@ describe('Applying updates to a doc', function () {
      JSON.parse(message).should.deep.include({
        project_id: this.project_id,
        doc_id: this.doc_id,
-        error: `doc not found: /project/${this.project_id}/doc/${this.doc_id}`,
+        error: `doc not not found: /project/${this.project_id}/doc/${this.doc_id}`,
      })
    })
  })
--- a/services/document-updater/test/setup.js
+++ b/services/document-updater/test/setup.js
@ -1,11 +1,9 @@
 const chai = require('chai')
 const SandboxedModule = require('sandboxed-module')
 const sinon = require('sinon')
-const sinonChai = require('sinon-chai')

 // Chai configuration
 chai.should()
-chai.use(sinonChai)

 // Global stubs
 const sandbox = sinon.createSandbox()
--- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js
+++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js
@ -42,14 +42,6 @@ describe('DocumentManager', function () {
    this.lastUpdatedAt = Date.now()
    this.lastUpdatedBy = 'last-author-id'
    this.source = 'external-source'
-    this.doc = {
-      lines: this.lines,
-      version: this.version,
-      ranges: this.ranges,
-      pathname: this.pathname,
-      projectHistoryId: this.projectHistoryId,
-      historyRangesSupport: false,
-    }
  })

  afterEach(function () {
@ -381,7 +373,15 @@ describe('DocumentManager', function () {
          .callsArgWith(2, null, null, null, null, null, null)
        this.PersistenceManager.getDoc = sinon
          .stub()
-          .callsArgWith(2, null, this.doc)
+          .callsArgWith(
+            2,
+            null,
+            this.lines,
+            this.version,
+            this.ranges,
+            this.pathname,
+            this.projectHistoryId
+          )
        this.RedisManager.putDocInMemory = sinon.stub().yields()
        this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback)
      })
@ -400,7 +400,15 @@ describe('DocumentManager', function () {

      it('should set the doc in Redis', function () {
        this.RedisManager.putDocInMemory
-          .calledWith(this.project_id, this.doc_id, this.doc)
+          .calledWith(
+            this.project_id,
+            this.doc_id,
+            this.lines,
+            this.version,
+            this.ranges,
+            this.pathname,
+            this.projectHistoryId
+          )
          .should.equal(true)
      })

@ -1101,7 +1109,16 @@ describe('DocumentManager', function () {
      beforeEach(function () {
        this.pathnameFromProjectStructureUpdate = '/foo/bar.tex'
        this.RedisManager.getDoc = sinon.stub().callsArgWith(2, null)
-        this.PersistenceManager.getDoc = sinon.stub().yields(null, this.doc)
+        this.PersistenceManager.getDoc = sinon
+          .stub()
+          .yields(
+            null,
+            this.lines,
+            this.version,
+            this.ranges,
+            this.pathname,
+            this.projectHistoryId
+          )
        this.ProjectHistoryRedisManager.queueResyncDocContent = sinon.stub()
        this.DocumentManager.resyncDocContents(
          this.project_id,
--- a/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js
+++ b/services/document-updater/test/unit/js/PersistenceManager/PersistenceManagerTests.js
@ -32,14 +32,6 @@ describe('PersistenceManager', function () {
    this.pathname = '/a/b/c.tex'
    this.lastUpdatedAt = Date.now()
    this.lastUpdatedBy = 'last-author-id'
-    this.doc = {
-      lines: this.lines,
-      version: this.version,
-      ranges: this.ranges,
-      projectHistoryId: this.projectHistoryId,
-      pathname: this.pathname,
-      historyRangesSupport: false,
-    }
    this.Settings.apis = {
      web: {
        url: (this.url = 'www.example.com'),
@ -95,7 +87,16 @@ describe('PersistenceManager', function () {
      })

      it('should call the callback with the doc lines, version and ranges', function () {
-        this.callback.should.have.been.calledWith(null, this.doc)
+        this.callback
+          .calledWith(
+            null,
+            this.lines,
+            this.version,
+            this.ranges,
+            this.pathname,
+            this.projectHistoryId
+          )
+          .should.equal(true)
      })

      it('should time the execution', function () {
--- a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js
+++ b/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'
-                },
              },
            },
          },
@ -762,20 +759,16 @@ 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.doc = {
-        lines: ['one', 'two', 'three', 'これは'],
-        version: 42,
-        ranges: { comments: 'mock', entries: 'mock' },
-        pathname: '/a/b/c.tex',
-        projectHistoryId: this.projectHistoryId,
-      }
+      this.lines = ['one', 'two', 'three', 'これは']
+      this.version = 42
      this.hash = crypto
        .createHash('sha1')
-        .update(JSON.stringify(this.doc.lines), 'utf8')
+        .update(JSON.stringify(this.lines), 'utf8')
        .digest('hex')
+      this.ranges = { comments: 'mock', entries: 'mock' }
+      this.pathname = '/a/b/c.tex'
    })

    describe('with non-empty ranges', function () {
@ -783,21 +776,25 @@ describe('RedisManager', function () {
        this.RedisManager.putDocInMemory(
          this.project_id,
          this.docId,
-          this.doc,
+          this.lines,
+          this.version,
+          this.ranges,
+          this.pathname,
+          this.projectHistoryId,
          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.doc.lines),
+            [`doclines:${this.docId}`]: JSON.stringify(this.lines),
            [`ProjectId:${this.docId}`]: this.project_id,
-            [`DocVersion:${this.docId}`]: this.doc.version,
+            [`DocVersion:${this.docId}`]: this.version,
            [`DocHash:${this.docId}`]: this.hash,
-            [`Ranges:${this.docId}`]: JSON.stringify(this.doc.ranges),
-            [`Pathname:${this.docId}`]: this.doc.pathname,
-            [`ProjectHistoryId:${this.docId}`]: this.doc.projectHistoryId,
+            [`Ranges:${this.docId}`]: JSON.stringify(this.ranges),
+            [`Pathname:${this.docId}`]: this.pathname,
+            [`ProjectHistoryId:${this.docId}`]: this.projectHistoryId,
          })
          .should.equal(true)
      })
@ -811,33 +808,32 @@ 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 () {
      beforeEach(function (done) {
-        this.doc.ranges = {}
        this.RedisManager.putDocInMemory(
          this.project_id,
          this.docId,
-          this.doc,
+          this.lines,
+          this.version,
+          {},
+          this.pathname,
+          this.projectHistoryId,
          done
        )
      })

      it('should unset ranges', function () {
-        this.multi.mset
+        this.rclient.mset
          .calledWith({
-            [`doclines:${this.docId}`]: JSON.stringify(this.doc.lines),
+            [`doclines:${this.docId}`]: JSON.stringify(this.lines),
            [`ProjectId:${this.docId}`]: this.project_id,
-            [`DocVersion:${this.docId}`]: this.doc.version,
+            [`DocVersion:${this.docId}`]: this.version,
            [`DocHash:${this.docId}`]: this.hash,
            [`Ranges:${this.docId}`]: null,
-            [`Pathname:${this.docId}`]: this.doc.pathname,
-            [`ProjectHistoryId:${this.docId}`]: this.doc.projectHistoryId,
+            [`Pathname:${this.docId}`]: this.pathname,
+            [`ProjectHistoryId:${this.docId}`]: this.projectHistoryId,
          })
          .should.equal(true)
      })
@ -851,7 +847,11 @@ describe('RedisManager', function () {
        this.RedisManager.putDocInMemory(
          this.project_id,
          this.docId,
-          this.doc,
+          this.lines,
+          this.version,
+          this.ranges,
+          this.pathname,
+          this.projectHistoryId,
          this.callback
        )
      })
@ -879,7 +879,11 @@ describe('RedisManager', function () {
        this.RedisManager.putDocInMemory(
          this.project_id,
          this.docId,
-          this.doc,
+          this.lines,
+          this.version,
+          this.ranges,
+          this.pathname,
+          this.projectHistoryId,
          this.callback
        )
      })
@ -894,25 +898,6 @@ describe('RedisManager', function () {
          .should.equal(true)
      })
    })
-
-    describe('with history ranges support', function () {
-      beforeEach(function () {
-        this.doc.historyRangesSupport = true
-        this.RedisManager.putDocInMemory(
-          this.project_id,
-          this.docId,
-          this.doc,
-          this.callback
-        )
-      })
-
-      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 () {
@ -920,6 +905,7 @@ describe('RedisManager', function () {
      this.multi.strlen = sinon.stub()
      this.multi.del = sinon.stub()
      this.multi.srem = sinon.stub()
+      this.multi.exec.yields()
      this.RedisManager.removeDocFromMemory(this.project_id, this.docId, done)
    })

@ -949,13 +935,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 () {