Merge pull request #15 from sharelatex/ja-require-ranges

Validate that ranges are present in a set document request

services/docstore/app/coffee/DocManager.coffee

@@ -42,8 +42,8 @@ module.exports = DocManager =
 					return callback(null, docs)
 
 	updateDoc: (project_id, doc_id, lines, version, ranges, callback = (error, modified, rev) ->) ->
-		if !lines? or !version?
-			return callback(new Error("no lines or version provided"))
+		if !lines? or !version? or !ranges?
+			return callback(new Error("no lines, version or ranges provided"))
 	
 		DocManager.getDoc project_id, doc_id, {version: true, rev: true, lines: true, version: true, ranges: true}, (err, doc)->
 			if err? and !(err instanceof Errors.NotFoundError)

services/docstore/app/coffee/HttpController.coffee

@@ -63,6 +63,11 @@ module.exports = HttpController =
 			res.send 400 # Bad Request
 			return
 		
+		if !ranges?
+			logger.error project_id: project_id, doc_id: doc_id, "no doc ranges 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, ranges, (error, modified, rev) ->
 			return next(error) if error?

services/docstore/app/coffee/RangeManager.coffee

@@ -3,10 +3,8 @@ _ = require "underscore"
 
 module.exports = RangeManager =
 	shouldUpdateRanges: (doc_ranges, incoming_ranges) ->
-		# TODO: If we have no incoming_ranges, just ignore for now while
-		# we're rolling this out, but eventually this should be a mandatory
-		# field and this will become an error condition
-		return false if !incoming_ranges?
+		if !incoming_ranges?
+			throw new Error("expected incoming_ranges")
 
 		# If the ranges are empty, we don't store them in the DB, so set
 		# doc_ranges to an empty object as default, since this is was the 

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

@@ -263,14 +263,21 @@ describe "DocManager", ->
 				@DocManager.updateDoc @project_id, @doc_id, @newDocLines, null, @originalRanges, @callback
 
 			it "should return an error", ->
-				@callback.calledWith(new Error("no lines or version provided")).should.equal true
+				@callback.calledWith(new Error("no lines, version or ranges provided")).should.equal true
 
 		describe "when the lines are null", ->
 			beforeEach ->
 				@DocManager.updateDoc @project_id, @doc_id, null, @version, @originalRanges, @callback
 
 			it "should return an error", ->
-				@callback.calledWith(new Error("no lines or version provided")).should.equal true
+				@callback.calledWith(new Error("no lines, version or ranges provided")).should.equal true
+
+		describe "when the ranges are null", ->
+			beforeEach ->
+				@DocManager.updateDoc @project_id, @doc_id, @newDocLines, @version, null, @callback
+
+			it "should return an error", ->
+				@callback.calledWith(new Error("no lines, version or ranges provided")).should.equal true
 
 		describe "when there is a generic error getting the doc", ->
 			beforeEach ->

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

@@ -241,6 +241,7 @@ describe "HttpController", ->
 				@req.body =
 					lines: @lines = ["hello", "world"]
 					version: @version = 42
+					ranges: {}
 				@DocManager.updateDoc = sinon.stub().yields(null, false, @rev = 5)
 				@HttpController.updateDoc @req, @res, @next
 
@@ -251,7 +252,7 @@ describe "HttpController", ->
 
 		describe "when the doc lines are not provided", ->
 			beforeEach ->
-				@req.body = { version: 42 }
+				@req.body = { version: 42, ranges: {} }
 				@DocManager.updateDoc = sinon.stub().yields(null, false)
 				@HttpController.updateDoc @req, @res, @next
 
@@ -263,9 +264,23 @@ describe "HttpController", ->
 					.calledWith(400)
 					.should.equal true
 
-		describe "when the doc version is not provided", ->
+		describe "when the doc version are not provided", ->
 			beforeEach ->
-				@req.body = { lines : [ "foo" ]}
+				@req.body = { version: 42, lines: ["hello world"] }
+				@DocManager.updateDoc = sinon.stub().yields(null, false)
+				@HttpController.updateDoc @req, @res, @next
+
+			it "should not update the document", ->
+				@DocManager.updateDoc.called.should.equal false
+
+			it "should return a 400 (bad request) response", ->
+				@res.send
+					.calledWith(400)
+					.should.equal true
+
+		describe "when the doc ranges is not provided", ->
+			beforeEach ->
+				@req.body = { lines : [ "foo" ], version: 42 }
 				@DocManager.updateDoc = sinon.stub().yields(null, false)
 				@HttpController.updateDoc @req, @res, @next
 

services/docstore/test/unit/coffee/RangeManagerTests.coffee

@@ -1,6 +1,7 @@
 SandboxedModule = require('sandboxed-module')
 sinon = require('sinon')
 require('chai').should()
+expect = require('chai').expect
 modulePath = require('path').join __dirname, '../../../app/js/RangeManager'
 ObjectId = require("mongojs").ObjectId
 assert = require("chai").assert
@@ -110,8 +111,10 @@ describe "RangeManager", ->
 			@ranges_copy = @RangeManager.jsonRangesToMongo(JSON.parse(JSON.stringify(@ranges)))
 
 		describe "with a blank new range", ->
-			it "should return false", ->
-				@RangeManager.shouldUpdateRanges(@ranges, null).should.equal false
+			it "should throw an error", ->
+				expect(() =>
+					@RangeManager.shouldUpdateRanges(@ranges, null)
+				).to.throw(Error)
 		
 		describe "with a blank old range", ->
 			it "should treat it like {}", ->