From ab2c5bcf2a035520a5ef12f68bf666ea92c82da1 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Mon, 22 Jan 2024 09:45:18 -0500 Subject: [PATCH] Merge pull request #15720 from overleaf/em-remove-docops-code Remove code related to the docOps collection GitOrigin-RevId: 38c1f752edda1cec4ed7051a0dba18b16ae4c754 --- services/docstore/app/js/DocManager.js | 11 +--- services/docstore/app/js/HealthChecker.js | 1 - services/docstore/app/js/MongoManager.js | 36 +++--------- services/docstore/app/js/mongodb.js | 1 - .../test/acceptance/js/DeletingDocsTests.js | 26 +-------- .../docstore/test/unit/js/DocManagerTests.js | 48 +--------------- .../test/unit/js/MongoManagerTests.js | 55 ------------------- 7 files changed, 14 insertions(+), 164 deletions(-) diff --git a/services/docstore/app/js/DocManager.js b/services/docstore/app/js/DocManager.js index 478aee295d..e889875d31 100644 --- a/services/docstore/app/js/DocManager.js +++ b/services/docstore/app/js/DocManager.js @@ -30,11 +30,6 @@ const DocManager = { return await DocManager._getDoc(projectId, docId, filter) } - if (filter.version && doc.version === undefined) { - const version = await MongoManager.promises.getDocVersion(docId) - doc.version = version - } - return doc }, @@ -95,11 +90,7 @@ const DocManager = { // without unarchiving it (avoids unnecessary writes to mongo) async peekDoc(projectId, docId) { const doc = await DocManager._peekRawDoc(projectId, docId) - if (doc.version === undefined) { - const version = await MongoManager.promises.getDocVersion(docId) - await MongoManager.promises.checkRevUnchanged(doc) - doc.version = version - } + await MongoManager.promises.checkRevUnchanged(doc) return doc }, diff --git a/services/docstore/app/js/HealthChecker.js b/services/docstore/app/js/HealthChecker.js index 2462725fb0..9e4a545e5f 100644 --- a/services/docstore/app/js/HealthChecker.js +++ b/services/docstore/app/js/HealthChecker.js @@ -61,7 +61,6 @@ module.exports = { }) }, cb => db.docs.deleteOne({ _id: docId, project_id: projectId }, cb), - cb => db.docOps.deleteOne({ doc_id: docId }, cb), ] return async.series(jobs, callback) }, diff --git a/services/docstore/app/js/MongoManager.js b/services/docstore/app/js/MongoManager.js index 64afa94eec..87d8af8a1b 100644 --- a/services/docstore/app/js/MongoManager.js +++ b/services/docstore/app/js/MongoManager.js @@ -5,20 +5,21 @@ const { callbackify } = require('util') const ARCHIVING_LOCK_DURATION_MS = Settings.archivingLockDurationMs -async function findDoc(projectId, docId, filter) { +async function findDoc(projectId, docId, projection) { const doc = await db.docs.findOne( { _id: new ObjectId(docId.toString()), project_id: new ObjectId(projectId.toString()), }, - { - projection: filter, - } + { projection } ) + if (doc && projection.version && !doc.version) { + doc.version = 0 + } return doc } -async function getProjectsDeletedDocs(projectId, filter) { +async function getProjectsDeletedDocs(projectId, projection) { const docs = await db.docs .find( { @@ -26,7 +27,7 @@ async function getProjectsDeletedDocs(projectId, filter) { deleted: true, }, { - projection: filter, + projection, sort: { deletedAt: -1 }, limit: Settings.max_deleted_docs, } @@ -35,13 +36,13 @@ async function getProjectsDeletedDocs(projectId, filter) { return docs } -async function getProjectsDocs(projectId, options, filter) { +async function getProjectsDocs(projectId, options, projection) { const query = { project_id: new ObjectId(projectId.toString()) } if (!options.include_deleted) { query.deleted = { $ne: true } } const queryOptions = { - projection: filter, + projection, } if (options.limit) { queryOptions.limit = options.limit @@ -203,18 +204,6 @@ async function restoreArchivedDoc(projectId, docId, archivedDoc) { } } -async function getDocVersion(docId) { - const doc = await db.docOps.findOne( - { doc_id: new ObjectId(docId) }, - { - projection: { - version: 1, - }, - } - ) - return (doc && doc.version) || 0 -} - async function getDocRev(docId) { const doc = await db.docs.findOne( { _id: new ObjectId(docId.toString()) }, @@ -248,11 +237,6 @@ async function checkRevUnchanged(doc) { } async function destroyProject(projectId) { - const records = await db.docs - .find({ project_id: new ObjectId(projectId) }, { projection: { _id: 1 } }) - .toArray() - const docIds = records.map(r => r._id) - await db.docOps.deleteMany({ doc_id: { $in: docIds } }) await db.docs.deleteMany({ project_id: new ObjectId(projectId) }) } @@ -270,7 +254,6 @@ module.exports = { patchDoc: callbackify(patchDoc), getDocForArchiving: callbackify(getDocForArchiving), markDocAsArchived: callbackify(markDocAsArchived), - getDocVersion: callbackify(getDocVersion), checkRevUnchanged: callbackify(checkRevUnchanged), destroyProject: callbackify(destroyProject), promises: { @@ -285,7 +268,6 @@ module.exports = { patchDoc, getDocForArchiving, markDocAsArchived, - getDocVersion, checkRevUnchanged, destroyProject, }, diff --git a/services/docstore/app/js/mongodb.js b/services/docstore/app/js/mongodb.js index 9774866956..feea369d2e 100644 --- a/services/docstore/app/js/mongodb.js +++ b/services/docstore/app/js/mongodb.js @@ -7,7 +7,6 @@ const mongoDb = mongoClient.db() const db = { docs: mongoDb.collection('docs'), - docOps: mongoDb.collection('docOps'), } Metrics.mongodb.monitor(mongoClient) diff --git a/services/docstore/test/acceptance/js/DeletingDocsTests.js b/services/docstore/test/acceptance/js/DeletingDocsTests.js index fd06cbf344..3959246767 100644 --- a/services/docstore/test/acceptance/js/DeletingDocsTests.js +++ b/services/docstore/test/acceptance/js/DeletingDocsTests.js @@ -471,15 +471,7 @@ describe("Destroying a project's documents", function () { describe('when the doc exists', function () { beforeEach(function (done) { - db.docOps.insertOne( - { doc_id: new ObjectId(this.doc_id), version: 1 }, - err => { - if (err) { - return done(err) - } - DocstoreClient.destroyAllDoc(this.project_id, done) - } - ) + DocstoreClient.destroyAllDoc(this.project_id, done) }) it('should remove the doc from the docs collection', function (done) { @@ -489,14 +481,6 @@ describe("Destroying a project's documents", function () { done() }) }) - - it('should remove the docOps from the docOps collection', function (done) { - db.docOps.find({ doc_id: this.doc_id }).toArray((err, docOps) => { - expect(err).not.to.exist - expect(docOps).to.deep.equal([]) - done() - }) - }) }) describe('when the doc is archived', function () { @@ -517,14 +501,6 @@ describe("Destroying a project's documents", function () { }) }) - it('should remove the docOps from the docOps collection', function (done) { - db.docOps.find({ doc_id: this.doc_id }).toArray((err, docOps) => { - expect(err).not.to.exist - expect(docOps).to.deep.equal([]) - done() - }) - }) - it('should remove the doc contents from s3', function (done) { DocstoreClient.getS3Doc(this.project_id, this.doc_id, error => { expect(error).to.be.instanceOf(Errors.NotFoundError) diff --git a/services/docstore/test/unit/js/DocManagerTests.js b/services/docstore/test/unit/js/DocManagerTests.js index d426b84c6f..a0d78ff6bc 100644 --- a/services/docstore/test/unit/js/DocManagerTests.js +++ b/services/docstore/test/unit/js/DocManagerTests.js @@ -16,7 +16,6 @@ describe('DocManager', function () { this.MongoManager = { promises: { findDoc: sinon.stub(), - getDocVersion: sinon.stub().resolves(this.version), getProjectsDocs: sinon.stub(), patchDoc: sinon.stub().resolves(), upsertIntoDocCollection: sinon.stub().resolves(), @@ -120,6 +119,7 @@ describe('DocManager', function () { _id: this.doc_id, project_id: this.project_id, lines: ['mock-lines'], + version: this.version, } }) @@ -166,55 +166,12 @@ describe('DocManager', function () { .should.equal(true) }) - it('should get the doc version from the docOps collection', function () { - this.MongoManager.promises.getDocVersion - .calledWith(this.doc_id) - .should.equal(true) - }) - it('should return the doc with the version', function () { this.result.lines.should.equal(this.doc.lines) this.result.version.should.equal(this.version) }) }) - describe('without the version filter', function () { - beforeEach(async function () { - this.MongoManager.promises.findDoc.resolves(this.doc) - await this.DocManager.promises._getDoc(this.project_id, this.doc_id, { - version: false, - inS3: true, - }) - }) - - it('should not get the doc version from the docOps collection', function () { - this.MongoManager.promises.getDocVersion.called.should.equal(false) - }) - }) - - describe('when the doc record already has a version', function () { - beforeEach(async function () { - this.docWithVersion = { ...this.doc, version: 109 } - this.MongoManager.promises.findDoc.resolves(this.docWithVersion) - this.result = await this.DocManager.promises._getDoc( - this.project_id, - this.doc_id, - { - version: true, - inS3: true, - } - ) - }) - - it('should return that version', function () { - expect(this.result).to.deep.equal(this.docWithVersion) - }) - - it('should not fetch the doc version from the docOps collection', function () { - expect(this.MongoManager.promises.getDocVersion).not.to.have.been.called - }) - }) - describe('when MongoManager.findDoc errors', function () { it('should return the error', async function () { this.MongoManager.promises.findDoc.rejects(this.stubbedError) @@ -232,12 +189,14 @@ describe('DocManager', function () { this.doc = { _id: this.doc_id, project_id: this.project_id, + version: 2, inS3: true, } this.unarchivedDoc = { _id: this.doc_id, project_id: this.project_id, lines: ['mock-lines'], + version: 2, inS3: false, } this.MongoManager.promises.findDoc.resolves(this.doc) @@ -271,7 +230,6 @@ describe('DocManager', function () { it('should return the doc', function () { expect(this.result).to.deep.equal({ ...this.unarchivedDoc, - version: this.version, }) }) }) diff --git a/services/docstore/test/unit/js/MongoManagerTests.js b/services/docstore/test/unit/js/MongoManagerTests.js index 1dc3628307..71205d77bd 100644 --- a/services/docstore/test/unit/js/MongoManagerTests.js +++ b/services/docstore/test/unit/js/MongoManagerTests.js @@ -15,7 +15,6 @@ describe('MongoManager', function () { updateOne: sinon.stub().resolves({ matchedCount: 1 }), insertOne: sinon.stub().resolves(), }, - docOps: {}, } this.MongoManager = SandboxedModule.require(modulePath, { requires: { @@ -281,19 +280,7 @@ describe('MongoManager', function () { describe('destroyProject', function () { beforeEach(async function () { this.projectId = new ObjectId() - this.docIds = [new ObjectId(), new ObjectId()] this.db.docs.deleteMany = sinon.stub().resolves() - this.db.docOps.deleteMany = sinon.stub().resolves() - this.db.docs.find = sinon - .stub() - .withArgs({ project_id: this.projectId }) - .returns({ - toArray: sinon.stub().resolves( - this.docIds.map(id => ({ - _id: id, - })) - ), - }) await this.MongoManager.promises.destroyProject(this.projectId) }) @@ -302,48 +289,6 @@ describe('MongoManager', function () { project_id: this.projectId, }) }) - - it('should destroy the docOps', function () { - sinon.assert.calledWith(this.db.docOps.deleteMany, { - doc_id: { $in: this.docIds }, - }) - }) - }) - - describe('getDocVersion', function () { - describe('when the doc exists', function () { - beforeEach(async function () { - this.doc = { version: (this.version = 42) } - this.db.docOps.findOne = sinon.stub().resolves(this.doc) - this.result = await this.MongoManager.promises.getDocVersion(this.docId) - }) - - it('should look for the doc in the database', function () { - this.db.docOps.findOne - .calledWith( - { doc_id: new ObjectId(this.docId) }, - { - projection: { version: 1 }, - } - ) - .should.equal(true) - }) - - it('should return the version', function () { - expect(this.result).to.equal(this.version) - }) - }) - - describe("when the doc doesn't exist", function () { - beforeEach(async function () { - this.db.docOps.findOne = sinon.stub().resolves(null) - this.result = await this.MongoManager.promises.getDocVersion(this.docId) - }) - - it('should return 0', function () { - expect(this.result).to.equal(0) - }) - }) }) describe('checkRevUnchanged', function () {