Merge pull request #12 from sharelatex/ja-set-version-in-docstore

Don't look up version in mongo if not needed

services/docstore/app/coffee/DocManager.coffee

@@ -9,7 +9,7 @@ module.exports = DocManager =
 	# collection (which is all that this collection contains). In future, we should
 	# migrate this version property to be part of the docs collection, to guarantee
 	# consitency between lines and version when writing/reading, and for a simpler schema.
-	getDoc: (project_id, doc_id, callback = (error, doc) ->) ->
+	getDoc: (project_id, doc_id, filter = { version: false }, callback = (error, doc) ->) ->
 		MongoManager.findDoc project_id, doc_id, (err, doc)->
 			if err?
 				return callback(err)
@@ -20,11 +20,14 @@ module.exports = DocManager =
 					if err?
 						logger.err err:err, project_id:project_id, doc_id:doc_id, "error unarchiving doc"
 						return callback(err)
-					DocManager.getDoc project_id, doc_id, callback
+					DocManager.getDoc project_id, doc_id, filter, callback
 			else
-				MongoManager.getDocVersion doc_id, (error, version) ->
-					return callback(error) if error?
-					doc.version = version
+				if filter.version
+					MongoManager.getDocVersion doc_id, (error, version) ->
+						return callback(error) if error?
+						doc.version = version
+						callback err, doc
+				else
 					callback err, doc
 
 	getAllDocs: (project_id, callback = (error, docs) ->) ->
@@ -38,7 +41,7 @@ module.exports = DocManager =
 					return callback(null, docs)
 
 	updateDoc: (project_id, doc_id, lines, version, callback = (error, modified, rev) ->) ->
-		DocManager.getDoc project_id, doc_id, (err, doc)->
+		DocManager.getDoc project_id, doc_id, {version: true}, (err, doc)->
 			if err? and !(err instanceof Errors.NotFoundError)
 				logger.err project_id: project_id, doc_id: doc_id, err:err, "error getting document for update"
 				return callback(err)
@@ -78,7 +81,7 @@ module.exports = DocManager =
 						callback null, true, oldRev + 1 # rev will have been incremented in mongo by MongoManager.updateDoc
 
 	deleteDoc: (project_id, doc_id, callback = (error) ->) ->
-		DocManager.getDoc project_id, doc_id, (error, doc) ->
+		DocManager.getDoc project_id, doc_id, {}, (error, doc) ->
 			return callback(error) if error?
 			return callback new Errors.NotFoundError("No such project/doc to delete: #{project_id}/#{doc_id}") if !doc?
 			MongoManager.upsertIntoDocCollection project_id, doc_id, doc.lines, (error) ->

services/docstore/app/coffee/HttpController.coffee

@@ -10,7 +10,7 @@ module.exports = HttpController =
 		doc_id     = req.params.doc_id
 		include_deleted = req.query?.include_deleted == "true"
 		logger.log project_id: project_id, doc_id: doc_id, "getting doc"
-		DocManager.getDoc project_id, doc_id, (error, doc) ->
+		DocManager.getDoc project_id, doc_id, {version: true}, (error, doc) ->
 			return next(error) if error?
 			logger.log doc: doc, "got doc"
 			if !doc?
@@ -24,7 +24,7 @@ module.exports = HttpController =
 		project_id = req.params.project_id
 		doc_id     = req.params.doc_id
 		logger.log project_id: project_id, doc_id: doc_id, "getting raw doc"
-		DocManager.getDoc project_id, doc_id, (error, doc) ->
+		DocManager.getDoc project_id, doc_id, {version: false}, (error, doc) ->
 			return next(error) if error?
 			if !doc?
 				res.send 404

services/docstore/test/unit/coffee/DocManagerTests.coffee

@@ -33,7 +33,7 @@ describe "DocManager", ->
 		describe "when the doc is in the doc collection", ->
 			beforeEach ->
 				@MongoManager.findDoc.yields(null, @doc)
-				@DocManager.getDoc @project_id, @doc_id, @callback
+				@DocManager.getDoc @project_id, @doc_id, {version: true}, @callback
 
 			it "should get the doc from the doc collection", ->
 				@MongoManager.findDoc
@@ -50,11 +50,19 @@ describe "DocManager", ->
 				doc = @callback.args[0][1]
 				doc.lines.should.equal @doc.lines
 				doc.version.should.equal @version
+
+		describe "without the version filter", ->
+			beforeEach ->
+				@MongoManager.findDoc.yields(null, @doc)
+				@DocManager.getDoc @project_id, @doc_id, {version: false}, @callback
+
+			it "should not get the doc version from the docOps collection", ->
+				@MongoManager.getDocVersion.called.should.equal false
 		
 		describe "when MongoManager.findDoc errors", ->
 			beforeEach ->
 				@MongoManager.findDoc.yields(@stubbedError)
-				@DocManager.getDoc @project_id, @doc_id, @callback
+				@DocManager.getDoc @project_id, @doc_id, {version: true}, @callback
 
 			it "should return the error", ->
 				@callback.calledWith(@stubbedError).should.equal true
@@ -67,7 +75,7 @@ describe "DocManager", ->
 					@doc.inS3 = false
 					callback()
 				sinon.spy @DocArchiveManager, "unarchiveDoc"
-				@DocManager.getDoc @project_id, @doc_id, @callback
+				@DocManager.getDoc @project_id, @doc_id, {version: true}, @callback
 
 			it "should call the DocArchive to unarchive the doc", ->
 				@DocArchiveManager.unarchiveDoc.calledWith(@project_id, @doc_id).should.equal true
@@ -81,7 +89,7 @@ describe "DocManager", ->
 		describe "when the doc does not exist in the docs collection", ->
 			beforeEach -> 
 				@MongoManager.findDoc = sinon.stub().callsArgWith(2, null, null)
-				@DocManager.getDoc @project_id, @doc_id, @callback
+				@DocManager.getDoc @project_id, @doc_id, {version: true}, @callback
 
 			it "should return a NotFoundError", ->
 				@callback
@@ -120,7 +128,7 @@ describe "DocManager", ->
 			beforeEach ->
 				@lines = ["mock", "doc", "lines"]
 				@rev = 77
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, null, {lines: @lines, rev:@rev})
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, {lines: @lines, rev:@rev})
 				@MongoManager.upsertIntoDocCollection = sinon.stub().callsArg(3)
 				@MongoManager.markDocAsDeleted = sinon.stub().callsArg(1)
 				@DocManager.deleteDoc @project_id, @doc_id, @callback
@@ -145,7 +153,7 @@ describe "DocManager", ->
 
 		describe "when the doc does not exist", ->
 			beforeEach -> 
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, null, null)
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, null)
 				@MongoManager.upsertIntoDocCollection = sinon.stub()
 				@DocManager.deleteDoc @project_id, @doc_id, @callback
 
@@ -170,7 +178,7 @@ describe "DocManager", ->
 
 		describe "when the doc lines have changed", ->
 			beforeEach ->
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc)
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc)
 				@DocManager.updateDoc @project_id, @doc_id, @newDocLines, @version, @callback
 
 			it "should get the existing doc", ->
@@ -207,12 +215,12 @@ describe "DocManager", ->
 
 		describe "when the version has changed", ->
 			beforeEach ->
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc)
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc)
 				@DocManager.updateDoc @project_id, @doc_id, @oldDocLines, @version + 1, @callback
 
-			it "should get the existing doc", ->
+			it "should get the existing doc with the version", ->
 				@DocManager.getDoc
-					.calledWith(@project_id, @doc_id)
+					.calledWith(@project_id, @doc_id, {version: true})
 					.should.equal true
 
 			it "should upsert the document to the doc collection", ->
@@ -230,12 +238,12 @@ describe "DocManager", ->
 
 		describe "when the version is null and the lines are different", ->
 			beforeEach ->
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc)
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc)
 				@DocManager.updateDoc @project_id, @doc_id, @newDocLines, null, @callback
 
-			it "should get the existing doc", ->
+			it "should get the existing doc with the version", ->
 				@DocManager.getDoc
-					.calledWith(@project_id, @doc_id)
+					.calledWith(@project_id, @doc_id, {version: true})
 					.should.equal true
 
 			it "should upsert the document to the doc collection", ->
@@ -253,7 +261,7 @@ describe "DocManager", ->
 
 		describe "when the version is null and the lines are the same", ->
 			beforeEach ->
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc)
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc)
 				@DocManager.updateDoc @project_id, @doc_id, @oldDocLines, null, @callback
 			
 			it "should not update the version", ->
@@ -268,7 +276,7 @@ describe "DocManager", ->
 		describe "when there is a generic error getting the doc", ->
 			beforeEach ->
 				@error =  new Error("doc could not be found")
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, @error, null, null)
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, @error, null, null)
 				@DocManager.updateDoc @project_id, @doc_id, @newDocLines, @version, @callback
 			
 			it "should not upsert the document to the doc collection", ->
@@ -279,7 +287,7 @@ describe "DocManager", ->
 
 		describe "when the doc lines have not changed", ->
 			beforeEach ->
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc)
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc)
 				@DocManager.updateDoc @project_id, @doc_id, @oldDocLines.slice(), @version, @callback
 
 			it "should not update the doc", ->
@@ -291,7 +299,7 @@ describe "DocManager", ->
 		describe "when the doc lines are an empty array", ->
 			beforeEach ->
 				@doc.lines = []
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc)
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc)
 				@DocManager.updateDoc @project_id, @doc_id, @doc.lines, @callback
 
 			it "should upsert the document to the doc collection", ->
@@ -301,7 +309,7 @@ describe "DocManager", ->
 
 		describe "when the doc does not exist", ->
 			beforeEach ->
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, null, null, null)
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, null, null)
 				@DocManager.updateDoc @project_id, @doc_id, @newDocLines, @version, @callback
 			
 			it "should upsert the document to the doc collection", ->

services/docstore/test/unit/coffee/HttpControllerTests.coffee

@@ -40,12 +40,12 @@ describe "HttpController", ->
 				@req.params =
 					project_id: @project_id
 					doc_id: @doc_id
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc)
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc)
 				@HttpController.getDoc @req, @res, @next
 
-			it "should get the document (including deleted)", ->
+			it "should get the document with the version (including deleted)", ->
 				@DocManager.getDoc
-					.calledWith(@project_id, @doc_id)
+					.calledWith(@project_id, @doc_id, {version: true})
 					.should.equal true
 
 			it "should return the doc as JSON", ->
@@ -64,11 +64,11 @@ describe "HttpController", ->
 				@req.params =
 					project_id: @project_id
 					doc_id: @doc_id
-				@DocManager.getDoc = sinon.stub().callsArgWith(2, null, @deletedDoc)
+				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, @deletedDoc)
 
 			it "should get the doc from the doc manager", ->
 				@HttpController.getDoc @req, @res, @next
-				@DocManager.getDoc.calledWith(@project_id, @doc_id).should.equal true
+				@DocManager.getDoc.calledWith(@project_id, @doc_id, {version: true}).should.equal true
 
 			it "should return 404 if the query string delete is not set ", ->
 				@HttpController.getDoc @req, @res, @next
@@ -92,12 +92,12 @@ describe "HttpController", ->
 			@req.params =
 				project_id: @project_id
 				doc_id: @doc_id
-			@DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc)
+			@DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc)
 			@HttpController.getRawDoc @req, @res, @next
 
-		it "should get the document", ->
+		it "should get the document without the version", ->
 			@DocManager.getDoc
-				.calledWith(@project_id, @doc_id)
+				.calledWith(@project_id, @doc_id, {version: false})
 				.should.equal true
 
 		it "should set the content type header", ->