Add an error guard around the version being present

services/docstore/app/coffee/DocManager.coffee

@@ -41,6 +41,9 @@ module.exports = DocManager =
 					return callback(null, docs)
 
 	updateDoc: (project_id, doc_id, lines, version, callback = (error, modified, rev) ->) ->
+		if !lines? or !version?
+			return callback(new Error("no lines or version provided"))
+	
 		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"
@@ -48,10 +51,7 @@ module.exports = DocManager =
 
 			isNewDoc = lines.length == 0
 			linesAreSame =  _.isEqual(doc?.lines, lines)
-			if version?
 			versionsAreSame = (doc?.version == version)
-			else
-				versionsAreSame = true
 
 			if linesAreSame and versionsAreSame and !isNewDoc
 				logger.log project_id: project_id, doc_id: doc_id, rev: doc?.rev, "doc lines have not changed - not updating"
@@ -69,16 +69,9 @@ module.exports = DocManager =
 				}, "updating doc lines"
 				MongoManager.upsertIntoDocCollection project_id, doc_id, lines, (error)->
 					return callback(callback) if error?
-					# TODO: While rolling out this code, setting the version via the docstore is optional,
-					# so if it hasn't been passed, just ignore it. Once the docupdater has totally
-					# handed control of this to the docstore, we can assume it will always be passed
-					# and an error guard on it not being set instead.
-					if version?
 					MongoManager.setDocVersion doc_id, version, (error) ->
 						return callback(error) if error?
 						callback null, true, oldRev + 1 # rev will have been incremented in mongo by MongoManager.updateDoc
-					else
-						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, { version: false }, (error, doc) ->

services/docstore/app/coffee/HealthChecker.coffee

@@ -19,7 +19,7 @@ module.exports =
 		jobs = [
 			(cb)->
 				opts = getOpts()
-				opts.json = {lines: lines}
+				opts.json = {lines: lines, version: 42}
 				request.post(opts, cb)
 			(cb)->
 				opts = getOpts()

services/docstore/app/coffee/HttpController.coffee

@@ -56,6 +56,11 @@ module.exports = HttpController =
 			res.send 400 # Bad Request
 			return
 		
+		if !version? or typeof version is not "number"
+			logger.error project_id: project_id, doc_id: doc_id, "no doc version provided"
+			res.send 400 # Bad Request
+			return
+
 		logger.log project_id: project_id, doc_id: doc_id, "got http request to update doc"
 		DocManager.updateDoc project_id, doc_id, lines, version, (error, modified, rev) ->
 			return next(error) if error?

services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee

@@ -30,7 +30,7 @@ describe "Archiving all docs", ->
 				(callback) =>
 					DocstoreClient.createDoc @project_id, doc._id, doc.lines, (err)=>
 						doc.lines[0] = doc.lines[0]+" added"
-						DocstoreClient.updateDoc @project_id, doc._id, doc.lines, null, callback
+						DocstoreClient.updateDoc @project_id, doc._id, doc.lines, 42, callback
 		async.series jobs, done 
 
 	afterEach (done) ->
@@ -109,7 +109,7 @@ describe "Archiving all docs", ->
 			quarterMegInBytes = 250000
 			lines = require("crypto").randomBytes(quarterMegInBytes).toString("hex")
 			@docs[1].lines = [lines,lines,lines,lines]
-			DocstoreClient.updateDoc @project_id, @docs[1]._id, @docs[1].lines, null, =>
+			DocstoreClient.updateDoc @project_id, @docs[1]._id, @docs[1].lines, 42, =>
 				DocstoreClient.archiveAllDoc @project_id, (error, @res) =>
 					done()
 		

services/docstore/test/acceptance/coffee/GettingAllDocsTests.coffee

@@ -27,7 +27,7 @@ describe "Getting all docs", ->
 				(callback) => 
 					DocstoreClient.createDoc @project_id, doc._id, doc.lines, (err)=>
 						doc.lines[0] = doc.lines[0]+" added"
-						DocstoreClient.updateDoc @project_id, doc._id, doc.lines, null, callback
+						DocstoreClient.updateDoc @project_id, doc._id, doc.lines, 42, callback
 		async.series jobs, done 
 
 	it "should return all the docs", (done) ->

services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee

@@ -18,9 +18,9 @@ describe "Applying updates to a doc", ->
 				throw error if error?
 				done()
 
-	describe "when the content has changed", ->
+	describe "when the lines have changed", ->
 		beforeEach (done) ->
-			DocstoreClient.updateDoc @project_id, @doc_id, @newLines, null, (error, res, @body) =>
+			DocstoreClient.updateDoc @project_id, @doc_id, @newLines, @version, (error, res, @body) =>
 				done()
 
 		it "should return modified = true", ->
@@ -35,9 +35,9 @@ describe "Applying updates to a doc", ->
 				doc.version.should.equal @version
 				done()
 
-	describe "when the content has not been updated", ->
+	describe "when the lines and version have not been updated", ->
 		beforeEach (done) ->
-			DocstoreClient.updateDoc @project_id, @doc_id, @originalLines, null, (error, res, @body) =>
+			DocstoreClient.updateDoc @project_id, @doc_id, @originalLines, @version, (error, res, @body) =>
 				done()
 
 		it "should return modified = false", ->
@@ -51,7 +51,7 @@ describe "Applying updates to a doc", ->
 	describe "when the doc does not exist", ->
 		beforeEach (done) ->
 			@missing_doc_id = ObjectId()
-			DocstoreClient.updateDoc @project_id, @missing_doc_id, @originalLines, null, (error, @res, @body) =>
+			DocstoreClient.updateDoc @project_id, @missing_doc_id, @originalLines, 0, (error, @res, @body) =>
 				done()
 
 		it "should create the doc", ->
@@ -66,7 +66,7 @@ describe "Applying updates to a doc", ->
 	describe "when malformed doc lines are provided", ->
 		describe "when the lines are not an array", ->
 			beforeEach (done) ->
-				DocstoreClient.updateDoc @project_id, @doc_id, { foo: "bar" }, null, (error, @res, @body) =>
+				DocstoreClient.updateDoc @project_id, @doc_id, { foo: "bar" }, @version, (error, @res, @body) =>
 					done()
 
 			it "should return 400", ->
@@ -79,7 +79,7 @@ describe "Applying updates to a doc", ->
 
 		describe "when the lines are not present", ->
 			beforeEach (done) ->
-				DocstoreClient.updateDoc @project_id, @doc_id, null, null, (error, @res, @body) =>
+				DocstoreClient.updateDoc @project_id, @doc_id, null, @version, (error, @res, @body) =>
 					done()
 
 			it "should return 400", ->
@@ -90,11 +90,25 @@ describe "Applying updates to a doc", ->
 					doc.lines.should.deep.equal @originalLines
 					done()
 	
+	describe "when no version is provided", ->
+		beforeEach (done) ->
+			DocstoreClient.updateDoc @project_id, @doc_id, @originalLines, null, (error, @res, @body) =>
+				done()
+
+		it "should return 400", ->
+			@res.statusCode.should.equal 400
+
+		it "should not update the doc in the API", (done) ->
+			DocstoreClient.getDoc @project_id, @doc_id, {}, (error, res, doc) =>
+				doc.lines.should.deep.equal @originalLines
+				doc.version.should.equal @version
+				done()
+
 	describe "when the content is large", ->
 		beforeEach (done) ->
 			line = new Array(1025).join("x") # 1kb
 			@largeLines = Array.apply(null, Array(1024)).map(() -> line) # 1mb
-			DocstoreClient.updateDoc @project_id, @doc_id, @largeLines, null, (error, res, @body) =>
+			DocstoreClient.updateDoc @project_id, @doc_id, @largeLines, @version, (error, res, @body) =>
 				done()
 
 		it "should return modified = true", ->

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

@@ -227,42 +227,19 @@ describe "DocManager", ->
 			it "should return the callback with the new rev", ->
 				@callback.calledWith(null, true, @rev + 1).should.equal true
 
-		describe "when the version is null and the lines are different", ->
+		describe "when the version is null", ->
 			beforeEach ->
-				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc)
 				@DocManager.updateDoc @project_id, @doc_id, @newDocLines, null, @callback
 
-			it "should get the existing doc with the version", ->
-				@DocManager.getDoc
-					.calledWith(@project_id, @doc_id, {version: true})
-					.should.equal true
+			it "should return an error", ->
+				@callback.calledWith(new Error("no lines or version provided")).should.equal true
 
-			it "should upsert the document to the doc collection", ->
-				@MongoManager.upsertIntoDocCollection
-					.calledWith(@project_id, @doc_id, @newDocLines)
-					.should.equal true
-			
-			it "should not update the version", ->
-				@MongoManager.setDocVersion
-					.called
-					.should.equal false
-
-			it "should return the callback with the new rev", ->
-				@callback.calledWith(null, true, @rev + 1).should.equal true
-
-		describe "when the version is null and the lines are the same", ->
+		describe "when the lines are null", ->
 			beforeEach ->
-				@DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc)
-				@DocManager.updateDoc @project_id, @doc_id, @oldDocLines, null, @callback
+				@DocManager.updateDoc @project_id, @doc_id, null, @version, @callback
 
-			it "should not update the version", ->
-				@MongoManager.setDocVersion.called.should.equal false
-
-			it "should not update the doc", ->
-				@MongoManager.upsertIntoDocCollection.called.should.equal false
-
-			it "should return the callback with the existing rev", ->
-				@callback.calledWith(null, false, @rev).should.equal true
+			it "should return an error", ->
+				@callback.calledWith(new Error("no lines or version provided")).should.equal true
 
 		describe "when there is a generic error getting the doc", ->
 			beforeEach ->

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

@@ -195,12 +195,13 @@ describe "HttpController", ->
 			beforeEach ->
 				@req.body =
 					lines: @lines = ["hello", "world"]
+					version: @version = 42
 				@DocManager.updateDoc = sinon.stub().yields(null, true, @rev = 5)
 				@HttpController.updateDoc @req, @res, @next
 
 			it "should update the document", ->
 				@DocManager.updateDoc
-					.calledWith(@project_id, @doc_id, @lines, undefined)
+					.calledWith(@project_id, @doc_id, @lines, @version)
 					.should.equal true
 
 			it "should return a modified status", ->
@@ -212,6 +213,7 @@ describe "HttpController", ->
 			beforeEach ->
 				@req.body =
 					lines: @lines = ["hello", "world"]
+					version: @version = 42
 				@DocManager.updateDoc = sinon.stub().yields(null, false, @rev = 5)
 				@HttpController.updateDoc @req, @res, @next
 
@@ -222,7 +224,7 @@ describe "HttpController", ->
 
 		describe "when the doc lines are not provided", ->
 			beforeEach ->
-				@req.body = {}
+				@req.body = { version: 42 }
 				@DocManager.updateDoc = sinon.stub().yields(null, false)
 				@HttpController.updateDoc @req, @res, @next
 
@@ -234,22 +236,18 @@ describe "HttpController", ->
 					.calledWith(400)
 					.should.equal true
 
-		describe "when the doc version is provided", ->
+		describe "when the doc version is not provided", ->
 			beforeEach ->
-				@req.body =
-					lines: @lines = ["hello", "world"]
-					version: @version = 42
-				@DocManager.updateDoc = sinon.stub().yields(null, true, @rev = 5)
+				@req.body = { lines : [ "foo" ]}
+				@DocManager.updateDoc = sinon.stub().yields(null, false)
 				@HttpController.updateDoc @req, @res, @next
 
-			it "should update the document with the lines and version", ->
-				@DocManager.updateDoc
-					.calledWith(@project_id, @doc_id, @lines, @version)
-					.should.equal true
+			it "should not update the document", ->
+				@DocManager.updateDoc.called.should.equal false
 
-			it "should return a modified status", ->
-				@res.json
-					.calledWith(modified: true, rev: @rev)
+			it "should return a 400 (bad request) response", ->
+				@res.send
+					.calledWith(400)
 					.should.equal true
 
 	describe "deleteDoc", ->