diff --git a/services/docstore/app/js/DocManager.js b/services/docstore/app/js/DocManager.js index 288d2725e5..8735300c9a 100644 --- a/services/docstore/app/js/DocManager.js +++ b/services/docstore/app/js/DocManager.js @@ -34,7 +34,7 @@ const DocManager = { return await DocManager._getDoc(projectId, docId, filter) } - if (filter.version) { + if (filter.version && doc.version === undefined) { const version = await MongoManager.promises.getDocVersion(docId) doc.version = version } @@ -99,9 +99,11 @@ const DocManager = { // without unarchiving it (avoids unnecessary writes to mongo) async peekDoc(projectId, docId) { const doc = await DocManager._peekRawDoc(projectId, docId) - const version = await MongoManager.promises.getDocVersion(docId) - await MongoManager.promises.checkRevUnchanged(doc) - doc.version = version + if (doc.version === undefined) { + const version = await MongoManager.promises.getDocVersion(docId) + await MongoManager.promises.checkRevUnchanged(doc) + doc.version = version + } return doc }, diff --git a/services/docstore/test/unit/js/DocManagerTests.js b/services/docstore/test/unit/js/DocManagerTests.js index c47bf360f7..69a8c98c04 100644 --- a/services/docstore/test/unit/js/DocManagerTests.js +++ b/services/docstore/test/unit/js/DocManagerTests.js @@ -7,10 +7,16 @@ const Errors = require('../../../app/js/Errors') describe('DocManager', function () { beforeEach(function () { + this.doc_id = ObjectId().toString() + this.project_id = ObjectId().toString() + this.another_project_id = ObjectId().toString() + this.stubbedError = new Error('blew up') + this.version = 42 + this.MongoManager = { promises: { findDoc: sinon.stub(), - getDocVersion: sinon.stub(), + getDocVersion: sinon.stub().resolves(this.version), getProjectsDocs: sinon.stub(), patchDoc: sinon.stub().resolves(), upsertIntoDocCollection: sinon.stub().resolves(), @@ -41,11 +47,6 @@ describe('DocManager', function () { './Errors': Errors, }, }) - - this.doc_id = ObjectId().toString() - this.project_id = ObjectId().toString() - this.another_project_id = ObjectId().toString() - this.stubbedError = new Error('blew up') }) describe('getFullDoc', function () { @@ -121,8 +122,6 @@ describe('DocManager', function () { project_id: this.project_id, lines: ['mock-lines'], } - this.version = 42 - this.MongoManager.promises.getDocVersion.resolves(this.version) }) describe('when using a filter', function () { @@ -174,7 +173,7 @@ describe('DocManager', function () { .should.equal(true) }) - it('should return the callback with the doc with the version', function () { + it('should return the doc with the version', function () { this.result.lines.should.equal(this.doc.lines) this.result.version.should.equal(this.version) }) @@ -194,6 +193,29 @@ describe('DocManager', function () { }) }) + 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) @@ -222,7 +244,9 @@ describe('DocManager', function () { this.MongoManager.promises.findDoc.resolves(this.doc) this.DocArchiveManager.promises.unarchiveDoc.callsFake( async (projectId, docId) => { - this.MongoManager.promises.findDoc.resolves(this.unarchivedDoc) + this.MongoManager.promises.findDoc.resolves({ + ...this.unarchivedDoc, + }) } ) this.result = await this.DocManager.promises._getDoc( @@ -246,7 +270,10 @@ describe('DocManager', function () { }) it('should return the doc', function () { - expect(this.result).to.deep.equal(this.unarchivedDoc) + expect(this.result).to.deep.equal({ + ...this.unarchivedDoc, + version: this.version, + }) }) }) @@ -280,8 +307,7 @@ describe('DocManager', function () { this.filter = { lines: true } this.result = await this.DocManager.promises.getAllNonDeletedDocs( this.project_id, - this.filter, - this.callback + this.filter ) }) @@ -582,7 +608,7 @@ describe('DocManager', function () { .should.equal(true) }) - it('should return the callback with the old rev', function () { + it('should return the old rev', function () { expect(this.result).to.deep.equal({ modified: true, rev: this.rev }) }) }) @@ -609,7 +635,7 @@ describe('DocManager', function () { this.MongoManager.promises.setDocVersion.called.should.equal(false) }) - it('should return the callback with the old rev and modified == false', function () { + it('should return the old rev and modified == false', function () { expect(this.result).to.deep.equal({ modified: false, rev: this.rev }) }) }) @@ -636,8 +662,7 @@ describe('DocManager', function () { this.doc_id, null, this.version, - this.originalRanges, - this.callback + this.originalRanges ) ).to.be.rejectedWith('no lines, version or ranges provided') }) @@ -701,8 +726,7 @@ describe('DocManager', function () { this.doc_id, this.oldDocLines.slice(), this.version, - this.originalRanges, - this.callback + this.originalRanges ) }) @@ -712,7 +736,7 @@ describe('DocManager', function () { ) }) - it('should return the callback with the existing rev', function () { + it('should return the existing rev', function () { expect(this.result).to.deep.equal({ modified: false, rev: this.rev }) }) }) @@ -744,7 +768,7 @@ describe('DocManager', function () { .should.equal(true) }) - it('should return the callback with the new rev', function () { + it('should return the new rev', function () { expect(this.result).to.deep.equal({ modified: true, rev: 1 }) }) }) @@ -783,7 +807,7 @@ describe('DocManager', function () { this.MongoManager.promises.setDocVersion.should.have.been.calledOnce }) - it('should return the callback with the new rev', function () { + it('should return the new rev', function () { expect(this.result).to.deep.equal({ modified: true, rev: this.rev + 1 }) }) })