[docstore] add runtime fix for mismatching comment vs thread ids (#26488)

GitOrigin-RevId: e7cefa88d125a73a26863e6fae8b49530efa2b4e

services/docstore/app/js/DocArchiveManager.js

@@ -44,6 +44,8 @@ async function archiveDoc(projectId, docId) {
     throw new Error('doc has no lines')
   }
 
+  RangeManager.fixCommentIds(doc)
+
   // warn about any oversized docs already in mongo
   const linesSize = BSON.calculateObjectSize(doc.lines || {})
   const rangesSize = BSON.calculateObjectSize(doc.ranges || {})

services/docstore/app/js/DocManager.js

@@ -41,6 +41,10 @@ const DocManager = {
       return await DocManager._getDoc(projectId, docId, filter)
     }
 
+    if (filter.ranges) {
+      RangeManager.fixCommentIds(doc)
+    }
+
     return doc
   },
 
@@ -129,6 +133,11 @@ const DocManager = {
     if (docs == null) {
       throw new Errors.NotFoundError(`No docs for project ${projectId}`)
     }
+    if (filter.ranges) {
+      for (const doc of docs) {
+        RangeManager.fixCommentIds(doc)
+      }
+    }
     return docs
   },
 

services/docstore/app/js/RangeManager.js

@@ -49,10 +49,10 @@ module.exports = RangeManager = {
       updateMetadata(change.metadata)
     }
     for (const comment of Array.from(ranges.comments || [])) {
-      comment.id = RangeManager._safeObjectId(comment.id)
-      if ((comment.op != null ? comment.op.t : undefined) != null) {
-        comment.op.t = RangeManager._safeObjectId(comment.op.t)
-      }
+      // Two bugs resulted in mismatched ids, prefer the thread id from the op: https://github.com/overleaf/internal/issues/23272
+      comment.id = RangeManager._safeObjectId(comment.op?.t || comment.id)
+      if (comment.op) comment.op.t = comment.id
+
       // resolved property is added to comments when they are obtained from history, but this state doesn't belong in mongo docs collection
       // more info: https://github.com/overleaf/internal/issues/24371#issuecomment-2913095174
       delete comment.op?.resolved
@@ -61,6 +61,13 @@ module.exports = RangeManager = {
     return ranges
   },
 
+  fixCommentIds(doc) {
+    for (const comment of doc?.ranges?.comments || []) {
+      // Two bugs resulted in mismatched ids, prefer the thread id from the op: https://github.com/overleaf/internal/issues/23272
+      if (comment.op?.t) comment.id = comment.op.t
+    }
+  },
+
   _safeObjectId(data) {
     try {
       return new ObjectId(data)

services/docstore/test/acceptance/js/ArchiveDocsTests.js

@@ -1001,6 +1001,15 @@ describe('Archiving', function () {
         },
         version: 2,
       }
+      this.fixedRanges = {
+        ...this.doc.ranges,
+        comments: [
+          {
+            ...this.doc.ranges.comments[0],
+            id: this.doc.ranges.comments[0].op.t,
+          },
+        ],
+      }
       return DocstoreClient.createDoc(
         this.project_id,
         this.doc._id,
@@ -1048,7 +1057,7 @@ describe('Archiving', function () {
             throw error
           }
           s3Doc.lines.should.deep.equal(this.doc.lines)
-          const ranges = JSON.parse(JSON.stringify(this.doc.ranges)) // ObjectId -> String
+          const ranges = JSON.parse(JSON.stringify(this.fixedRanges)) // ObjectId -> String
           s3Doc.ranges.should.deep.equal(ranges)
           return done()
         }
@@ -1075,7 +1084,7 @@ describe('Archiving', function () {
             throw error
           }
           doc.lines.should.deep.equal(this.doc.lines)
-          doc.ranges.should.deep.equal(this.doc.ranges)
+          doc.ranges.should.deep.equal(this.fixedRanges)
           expect(doc.inS3).not.to.exist
           return done()
         })

services/docstore/test/acceptance/js/GettingAllDocsTests.js

@@ -20,13 +20,15 @@ const DocstoreClient = require('./helpers/DocstoreClient')
 describe('Getting all docs', function () {
   beforeEach(function (done) {
     this.project_id = new ObjectId()
+    this.threadId1 = new ObjectId().toString()
+    this.threadId2 = new ObjectId().toString()
     this.docs = [
       {
         _id: new ObjectId(),
         lines: ['one', 'two', 'three'],
         ranges: {
           comments: [
-            { id: new ObjectId().toString(), op: { t: 'thread-id-1' } },
+            { id: new ObjectId().toString(), op: { t: this.threadId1 } },
           ],
           changes: [
             {
@@ -55,7 +57,7 @@ describe('Getting all docs', function () {
         lines: ['111', '222', '333'],
         ranges: {
           comments: [
-            { id: new ObjectId().toString(), op: { t: 'thread-id-2' } },
+            { id: new ObjectId().toString(), op: { t: this.threadId2 } },
           ],
           changes: [
             {
@@ -67,6 +69,15 @@ describe('Getting all docs', function () {
         rev: 6,
       },
     ]
+    this.fixedRanges = this.docs.map(doc => {
+      if (!doc.ranges?.comments?.length) return doc.ranges
+      return {
+        ...doc.ranges,
+        comments: [
+          { ...doc.ranges.comments[0], id: doc.ranges.comments[0].op.t },
+        ],
+      }
+    })
     this.deleted_doc = {
       _id: new ObjectId(),
       lines: ['deleted'],
@@ -136,7 +147,7 @@ describe('Getting all docs', function () {
       docs.length.should.equal(this.docs.length)
       for (let i = 0; i < docs.length; i++) {
         const doc = docs[i]
-        doc.ranges.should.deep.equal(this.docs[i].ranges)
+        doc.ranges.should.deep.equal(this.fixedRanges[i])
       }
       return done()
     })
@@ -163,8 +174,8 @@ describe('Getting all docs', function () {
           throw error
         }
         threadIds.should.deep.equal({
-          [this.docs[0]._id.toString()]: ['thread-id-1'],
-          [this.docs[2]._id.toString()]: ['thread-id-2'],
+          [this.docs[0]._id.toString()]: [this.threadId1],
+          [this.docs[2]._id.toString()]: [this.threadId2],
         })
         done()
       }

services/docstore/test/acceptance/js/GettingDocsTests.js

@@ -28,10 +28,26 @@ describe('Getting a doc', function () {
           op: { i: 'foo', p: 3 },
           meta: {
             user_id: new ObjectId().toString(),
-            ts: new Date().toString(),
+            ts: new Date().toJSON(),
           },
         },
       ],
+      comments: [
+        {
+          id: new ObjectId().toString(),
+          op: { c: 'comment', p: 1, t: new ObjectId().toString() },
+          metadata: {
+            user_id: new ObjectId().toString(),
+            ts: new Date().toJSON(),
+          },
+        },
+      ],
+    }
+    this.fixedRanges = {
+      ...this.ranges,
+      comments: [
+        { ...this.ranges.comments[0], id: this.ranges.comments[0].op.t },
+      ],
     }
     return DocstoreApp.ensureRunning(() => {
       return DocstoreClient.createDoc(
@@ -60,7 +76,7 @@ describe('Getting a doc', function () {
           if (error) return done(error)
           doc.lines.should.deep.equal(this.lines)
           doc.version.should.equal(this.version)
-          doc.ranges.should.deep.equal(this.ranges)
+          doc.ranges.should.deep.equal(this.fixedRanges)
           return done()
         }
       )
@@ -114,7 +130,7 @@ describe('Getting a doc', function () {
           if (error) return done(error)
           doc.lines.should.deep.equal(this.lines)
           doc.version.should.equal(this.version)
-          doc.ranges.should.deep.equal(this.ranges)
+          doc.ranges.should.deep.equal(this.fixedRanges)
           doc.deleted.should.equal(true)
           return done()
         }

services/docstore/test/unit/js/DocArchiveManagerTests.js

@@ -31,6 +31,7 @@ describe('DocArchiveManager', function () {
 
     RangeManager = {
       jsonRangesToMongo: sinon.stub().returns({ mongo: 'ranges' }),
+      fixCommentIds: sinon.stub(),
     }
     Settings = {
       docstore: {
@@ -192,6 +193,11 @@ describe('DocArchiveManager', function () {
         .eventually.be.fulfilled
     })
 
+    it('should fix comment ids', async function () {
+      await DocArchiveManager.archiveDoc(projectId, mongoDocs[1]._id)
+      expect(RangeManager.fixCommentIds).to.have.been.called
+    })
+
     it('should throw an error if the doc has no lines', async function () {
       const doc = mongoDocs[0]
       doc.lines = null

services/docstore/test/unit/js/DocManagerTests.js

@@ -32,6 +32,7 @@ describe('DocManager', function () {
         return r
       },
       shouldUpdateRanges: sinon.stub().returns(false),
+      fixCommentIds: sinon.stub(),
     }
     this.settings = { docstore: {} }
 
@@ -122,6 +123,27 @@ describe('DocManager', function () {
     })
   })
 
+  describe('_getDoc', function () {
+    it('should return error when get doc does not exist', async function () {
+      this.MongoManager.findDoc.resolves(null)
+      await expect(
+        this.DocManager._getDoc(this.project_id, this.doc_id, { inS3: true })
+      ).to.be.rejectedWith(Errors.NotFoundError)
+    })
+
+    it('should fix comment ids', async function () {
+      this.MongoManager.findDoc.resolves({
+        _id: this.doc_id,
+        ranges: {},
+      })
+      await this.DocManager._getDoc(this.project_id, this.doc_id, {
+        inS3: true,
+        ranges: true,
+      })
+      expect(this.RangeManager.fixCommentIds).to.have.been.called
+    })
+  })
+
   describe('getDoc', function () {
     beforeEach(function () {
       this.project = { name: 'mock-project' }
@@ -270,7 +292,7 @@ describe('DocManager', function () {
         ]
         this.MongoManager.getProjectsDocs.resolves(this.docs)
         this.DocArchiveManager.unArchiveAllDocs.resolves(this.docs)
-        this.filter = { lines: true }
+        this.filter = { lines: true, ranges: true }
         this.result = await this.DocManager.getAllNonDeletedDocs(
           this.project_id,
           this.filter
@@ -285,6 +307,10 @@ describe('DocManager', function () {
         )
       })
 
+      it('should fix comment ids', async function () {
+        expect(this.RangeManager.fixCommentIds).to.have.been.called
+      })
+
       it('should return the docs', function () {
         expect(this.result).to.deep.equal(this.docs)
       })

services/docstore/test/unit/js/RangeManagerTests.js

@@ -30,7 +30,7 @@ describe('RangeManager', function () {
   })
 
   describe('jsonRangesToMongo', function () {
-    it('should convert ObjectIds and dates to proper objects', function () {
+    it('should convert ObjectIds and dates to proper objects and fix comment id', function () {
       const changeId = new ObjectId().toString()
       const commentId = new ObjectId().toString()
       const userId = new ObjectId().toString()
@@ -66,7 +66,7 @@ describe('RangeManager', function () {
         ],
         comments: [
           {
-            id: new ObjectId(commentId),
+            id: new ObjectId(threadId),
             op: { c: 'foo', p: 3, t: new ObjectId(threadId) },
           },
         ],
@@ -110,7 +110,6 @@ describe('RangeManager', function () {
 
     return it('should be consistent when transformed through json -> mongo -> json', function () {
       const changeId = new ObjectId().toString()
-      const commentId = new ObjectId().toString()
       const userId = new ObjectId().toString()
       const threadId = new ObjectId().toString()
       const ts = new Date().toJSON()
@@ -127,7 +126,7 @@ describe('RangeManager', function () {
         ],
         comments: [
           {
-            id: commentId,
+            id: threadId,
             op: { c: 'foo', p: 3, t: threadId },
           },
         ],
@@ -142,6 +141,7 @@ describe('RangeManager', function () {
 
   return describe('shouldUpdateRanges', function () {
     beforeEach(function () {
+      const threadId = new ObjectId()
       this.ranges = {
         changes: [
           {
@@ -155,8 +155,8 @@ describe('RangeManager', function () {
         ],
         comments: [
           {
-            id: new ObjectId(),
-            op: { c: 'foo', p: 3, t: new ObjectId() },
+            id: threadId,
+            op: { c: 'foo', p: 3, t: threadId },
           },
         ],
       }