diff --git a/services/docstore/app/coffee/DocManager.coffee b/services/docstore/app/coffee/DocManager.coffee index 9102b22749..6cf679c6c1 100644 --- a/services/docstore/app/coffee/DocManager.coffee +++ b/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) diff --git a/services/docstore/app/coffee/HttpController.coffee b/services/docstore/app/coffee/HttpController.coffee index 36996d943a..7c8dd0aecf 100644 --- a/services/docstore/app/coffee/HttpController.coffee +++ b/services/docstore/app/coffee/HttpController.coffee @@ -62,6 +62,11 @@ module.exports = HttpController = logger.error project_id: project_id, doc_id: doc_id, "no doc version provided" 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) -> diff --git a/services/docstore/app/coffee/RangeManager.coffee b/services/docstore/app/coffee/RangeManager.coffee index b46fe50b26..661d6e4b28 100644 --- a/services/docstore/app/coffee/RangeManager.coffee +++ b/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 diff --git a/services/docstore/test/unit/coffee/DocManagerTests.coffee b/services/docstore/test/unit/coffee/DocManagerTests.coffee index c1356e6e6a..9b43bc05af 100644 --- a/services/docstore/test/unit/coffee/DocManagerTests.coffee +++ b/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 -> diff --git a/services/docstore/test/unit/coffee/HttpControllerTests.coffee b/services/docstore/test/unit/coffee/HttpControllerTests.coffee index 6b157050d4..1a0752d850 100644 --- a/services/docstore/test/unit/coffee/HttpControllerTests.coffee +++ b/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 diff --git a/services/docstore/test/unit/coffee/RangeManagerTests.coffee b/services/docstore/test/unit/coffee/RangeManagerTests.coffee index 0fe3983982..ea2416ab63 100644 --- a/services/docstore/test/unit/coffee/RangeManagerTests.coffee +++ b/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 {}", ->