diff --git a/services/docstore/app/coffee/DocManager.coffee b/services/docstore/app/coffee/DocManager.coffee index e49e96f03f..64b5b41522 100644 --- a/services/docstore/app/coffee/DocManager.coffee +++ b/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 + versionsAreSame = (doc?.version == version) 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,15 +69,8 @@ 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 + 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 deleteDoc: (project_id, doc_id, callback = (error) ->) -> diff --git a/services/docstore/app/coffee/HealthChecker.coffee b/services/docstore/app/coffee/HealthChecker.coffee index d0c181143d..639d752477 100644 --- a/services/docstore/app/coffee/HealthChecker.coffee +++ b/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() diff --git a/services/docstore/app/coffee/HttpController.coffee b/services/docstore/app/coffee/HttpController.coffee index e87d89ea64..5425b78c13 100644 --- a/services/docstore/app/coffee/HttpController.coffee +++ b/services/docstore/app/coffee/HttpController.coffee @@ -55,6 +55,11 @@ module.exports = HttpController = logger.error project_id: project_id, doc_id: doc_id, "no doc lines provided" 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) -> diff --git a/services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee b/services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee index 26c57e7712..0d602b49cb 100644 --- a/services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee +++ b/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() diff --git a/services/docstore/test/acceptance/coffee/GettingAllDocsTests.coffee b/services/docstore/test/acceptance/coffee/GettingAllDocsTests.coffee index eb4261b7ae..6a9c164001 100644 --- a/services/docstore/test/acceptance/coffee/GettingAllDocsTests.coffee +++ b/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) -> diff --git a/services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee b/services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee index 5f21f55845..0fdf3ae0b2 100644 --- a/services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee +++ b/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", -> @@ -89,12 +89,26 @@ describe "Applying updates to a doc", -> DocstoreClient.getDoc @project_id, @doc_id, {}, (error, res, 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", -> diff --git a/services/docstore/test/unit/coffee/DocManagerTests.coffee b/services/docstore/test/unit/coffee/DocManagerTests.coffee index 109c36a2b2..676a7fbe02 100644 --- a/services/docstore/test/unit/coffee/DocManagerTests.coffee +++ b/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 - - it "should not update the version", -> - @MongoManager.setDocVersion.called.should.equal false + @DocManager.updateDoc @project_id, @doc_id, null, @version, @callback - 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 -> diff --git a/services/docstore/test/unit/coffee/HttpControllerTests.coffee b/services/docstore/test/unit/coffee/HttpControllerTests.coffee index a34237982e..8293eb7b40 100644 --- a/services/docstore/test/unit/coffee/HttpControllerTests.coffee +++ b/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", ->