diff --git a/services/docstore/app/js/DocArchiveManager.js b/services/docstore/app/js/DocArchiveManager.js index d332a27651..d03ee161a8 100644 --- a/services/docstore/app/js/DocArchiveManager.js +++ b/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 || {}) diff --git a/services/docstore/app/js/DocManager.js b/services/docstore/app/js/DocManager.js index 00f03436a1..c9e8dadc2c 100644 --- a/services/docstore/app/js/DocManager.js +++ b/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 }, diff --git a/services/docstore/app/js/RangeManager.js b/services/docstore/app/js/RangeManager.js index 0b97c23cba..2fbadf9468 100644 --- a/services/docstore/app/js/RangeManager.js +++ b/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) diff --git a/services/docstore/test/acceptance/js/ArchiveDocsTests.js b/services/docstore/test/acceptance/js/ArchiveDocsTests.js index d9228103b6..7e254c7e84 100644 --- a/services/docstore/test/acceptance/js/ArchiveDocsTests.js +++ b/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() }) diff --git a/services/docstore/test/acceptance/js/GettingAllDocsTests.js b/services/docstore/test/acceptance/js/GettingAllDocsTests.js index c66c7d15f5..57851b2c3b 100644 --- a/services/docstore/test/acceptance/js/GettingAllDocsTests.js +++ b/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() } diff --git a/services/docstore/test/acceptance/js/GettingDocsTests.js b/services/docstore/test/acceptance/js/GettingDocsTests.js index 121b3c1e24..1cfc53c5c6 100644 --- a/services/docstore/test/acceptance/js/GettingDocsTests.js +++ b/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() } diff --git a/services/docstore/test/unit/js/DocArchiveManagerTests.js b/services/docstore/test/unit/js/DocArchiveManagerTests.js index fbc1667314..2ec1cb2016 100644 --- a/services/docstore/test/unit/js/DocArchiveManagerTests.js +++ b/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 diff --git a/services/docstore/test/unit/js/DocManagerTests.js b/services/docstore/test/unit/js/DocManagerTests.js index f207f8e993..67a2f26547 100644 --- a/services/docstore/test/unit/js/DocManagerTests.js +++ b/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) }) diff --git a/services/docstore/test/unit/js/RangeManagerTests.js b/services/docstore/test/unit/js/RangeManagerTests.js index 7a2de7352e..ba99280a7a 100644 --- a/services/docstore/test/unit/js/RangeManagerTests.js +++ b/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 }, }, ], }