From 3cd3c118d1aefef65e5da280a7902f1a044dbd91 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Mon, 23 Feb 2015 13:17:55 +0000 Subject: [PATCH 1/5] don't log out when doc lines are [] as this is a new empty doc --- services/docstore/app/coffee/DocManager.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/docstore/app/coffee/DocManager.coffee b/services/docstore/app/coffee/DocManager.coffee index 49d34a7399..b0f540fecb 100644 --- a/services/docstore/app/coffee/DocManager.coffee +++ b/services/docstore/app/coffee/DocManager.coffee @@ -16,7 +16,8 @@ module.exports = DocManager = if docFromDocCollection? return callback null, docFromDocCollection, mongoPath else if doc? - logger.warn project_id:project_id, doc_id:doc_id, "doc just used from project collection, why?" + if doc?.lines?.length > 0 + logger.warn project_id:project_id, doc_id:doc_id, "doc just used from project collection, why?" return callback null, doc, mongoPath else return callback new Errors.NotFoundError("No such doc: #{project_id}") From 5861231548ac993c558f02f9c0770a763c72df1b Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Fri, 20 Feb 2015 14:28:16 +0000 Subject: [PATCH 2/5] docstore now only touches doc collection - need to run migration 1 before using this for this to work - added acceptence test so new documents updated should be upserted, not return 404 --- .../docstore/app/coffee/DocManager.coffee | 115 ++++------ .../docstore/app/coffee/MongoManager.coffee | 13 +- services/docstore/app/coffee/mongojs.coffee | 2 +- .../coffee/DeletingDocsTests.coffee | 9 +- .../coffee/GettingAllDocsTests.coffee | 19 +- .../acceptance/coffee/GettingDocsTests.coffee | 9 +- .../coffee/UpdatingDocsTests.coffee | 30 ++- .../coffee/helpers/DocstoreClient.coffee | 19 +- .../test/unit/coffee/DocManagerTests.coffee | 196 +++--------------- .../test/unit/coffee/MongoManagerTests.coffee | 48 ++--- 10 files changed, 114 insertions(+), 346 deletions(-) diff --git a/services/docstore/app/coffee/DocManager.coffee b/services/docstore/app/coffee/DocManager.coffee index b0f540fecb..2cbd0b65c7 100644 --- a/services/docstore/app/coffee/DocManager.coffee +++ b/services/docstore/app/coffee/DocManager.coffee @@ -5,57 +5,50 @@ _ = require "underscore" async = require "async" module.exports = DocManager = - getDoc: (project_id, doc_id, callback = (error, doc, mongoPath) ->) -> - MongoManager.findDoc doc_id, (err, docFromDocCollection)-> - return callback(err) if err? - MongoManager.findProject project_id, (error, project) -> - return callback(error) if error? - return callback new Errors.NotFoundError("No such project: #{project_id}") if !project? - DocManager.findDocInProject project, doc_id, (error, doc, mongoPath) -> - return callback(error) if error? - if docFromDocCollection? - return callback null, docFromDocCollection, mongoPath - else if doc? - if doc?.lines?.length > 0 - logger.warn project_id:project_id, doc_id:doc_id, "doc just used from project collection, why?" - return callback null, doc, mongoPath - else - return callback new Errors.NotFoundError("No such doc: #{project_id}") + + getDoc: (project_id, doc_id, callback = (error, doc) ->) -> + MongoManager.findDoc doc_id, (err, doc)-> + if err? + return callback(err) + else if !doc? + return callback new Errors.NotFoundError("No such doc: #{doc_id} in project #{project_id}") + callback null, doc getAllDocs: (project_id, callback = (error, docs) ->) -> - MongoManager.findProject project_id, (error, project) -> - return callback(error) if error? - return callback new Errors.NotFoundError("No such project: #{project_id}") if !project? - DocManager.findAllDocsInProject project, (error, docs) -> - return callback(error) if error? - return callback null, docs + MongoManager.getProjectsDocs project_id, (error, docs) -> + if err? + return callback(error) + else if !docs? + return callback new Errors.NotFoundError("No docs for project #{project_id}") + else + return callback(null, docs) updateDoc: (project_id, doc_id, lines, callback = (error, modified, rev) ->) -> - DocManager.getDoc project_id, doc_id, (error, doc, mongoPath) -> - return callback(error) if error? - return callback new Errors.NotFoundError("No such project/doc to update: #{project_id}/#{doc_id}") if !doc? or !mongoPath? + DocManager.getDoc project_id, doc_id, (error, doc) -> - if _.isEqual(doc.lines, lines) - logger.log { - project_id: project_id, doc_id: doc_id, rev: doc.rev - }, "doc lines have not changed" - return callback null, false, doc.rev - else - logger.log { - project_id: project_id - doc_id: doc_id, - oldDocLines: doc.lines - newDocLines: lines - rev: doc.rev - }, "updating doc lines" - async.series [ - (cb)-> - MongoManager.upsertIntoDocCollection project_id, doc_id, lines, doc.rev, cb - (cb)-> - MongoManager.updateDoc project_id, mongoPath, lines, cb - ], (error)-> - return callback(error) if error? - callback null, true, doc.rev + 1 # rev will have been incremented in mongo by MongoManager.updateDoc + isNewDoc = error?.name == new Errors.NotFoundError().name + + if !isNewDoc and error? + logger.err project_id: project_id, doc_id: doc_id, err:error, "error getting document for update" + return callback(error) + else if !isNewDoc and !doc? + logger.err project_id: project_id, doc_id: doc_id, "existing document to update could not be found" + return callback new Errors.NotFoundError("No such project/doc to update: #{project_id}/#{doc_id}") + else if _.isEqual(doc?.lines, lines) + logger.log project_id: project_id, doc_id: doc_id, rev: doc?.rev, "doc lines have not changed - not updating" + return callback null, false, doc?.rev + + oldRev = doc?.rev || 0 + logger.log { + project_id: project_id + doc_id: doc_id, + oldDocLines: doc?.lines + newDocLines: lines + rev: oldRev + }, "updating doc lines" + MongoManager.upsertIntoDocCollection project_id, doc_id, lines, oldRev, (error)-> + return callback(callback) if error? + 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) -> @@ -67,33 +60,3 @@ module.exports = DocManager = return callback(error) if error? callback() - findAllDocsInProject: (project, callback = (error, docs) ->) -> - callback null, @_findAllDocsInFolder project.rootFolder[0] - - findDocInProject: (project, doc_id, callback = (error, doc, mongoPath) ->) -> - result = @_findDocInFolder project.rootFolder[0], doc_id, "rootFolder.0" - if result? - callback null, result.doc, result.mongoPath - else - callback null, null, null - - _findDocInFolder: (folder = {}, doc_id, currentPath) -> - for doc, i in folder.docs or [] - if doc?._id? and doc._id.toString() == doc_id.toString() - return { - doc: doc - mongoPath: "#{currentPath}.docs.#{i}" - } - - for childFolder, i in folder.folders or [] - result = @_findDocInFolder childFolder, doc_id, "#{currentPath}.folders.#{i}" - return result if result? - - return null - - _findAllDocsInFolder: (folder = {}) -> - docs = folder.docs or [] - for childFolder in folder.folders or [] - docs = docs.concat @_findAllDocsInFolder childFolder - return docs - diff --git a/services/docstore/app/coffee/MongoManager.coffee b/services/docstore/app/coffee/MongoManager.coffee index 05a61e4ccf..be13b6a084 100644 --- a/services/docstore/app/coffee/MongoManager.coffee +++ b/services/docstore/app/coffee/MongoManager.coffee @@ -1,22 +1,13 @@ {db, ObjectId} = require "./mongojs" module.exports = MongoManager = - findProject: (project_id, callback = (error, project) ->) -> - db.projects.find _id: ObjectId(project_id.toString()), {}, (error, projects = []) -> - callback error, projects[0] findDoc: (doc_id, callback = (error, doc) ->) -> db.docs.find _id: ObjectId(doc_id.toString()), {}, (error, docs = []) -> callback error, docs[0] - updateDoc: (project_id, docPath, lines, callback = (error) ->) -> - update = - $set: {} - $inc: {} - update.$set["#{docPath}.lines"] = lines - update.$inc["#{docPath}.rev"] = 1 - - db.projects.update _id: ObjectId(project_id), update, callback + getProjectsDocs: (project_id, callback)-> + db.docs.find project_id: ObjectId(project_id.toString()), {}, callback upsertIntoDocCollection: (project_id, doc_id, lines, oldRev, callback)-> update = diff --git a/services/docstore/app/coffee/mongojs.coffee b/services/docstore/app/coffee/mongojs.coffee index aae672b8f3..9a8d6c38e5 100644 --- a/services/docstore/app/coffee/mongojs.coffee +++ b/services/docstore/app/coffee/mongojs.coffee @@ -1,6 +1,6 @@ Settings = require "settings-sharelatex" mongojs = require "mongojs" -db = mongojs.connect(Settings.mongo.url, ["projects", "docs"]) +db = mongojs.connect(Settings.mongo.url, ["docs"]) module.exports = db: db ObjectId: mongojs.ObjectId diff --git a/services/docstore/test/acceptance/coffee/DeletingDocsTests.coffee b/services/docstore/test/acceptance/coffee/DeletingDocsTests.coffee index fa7e15f61c..d2d5a05727 100644 --- a/services/docstore/test/acceptance/coffee/DeletingDocsTests.coffee +++ b/services/docstore/test/acceptance/coffee/DeletingDocsTests.coffee @@ -11,14 +11,9 @@ describe "Deleting a doc", -> @doc_id = ObjectId() @lines = ["original", "lines"] @version = 42 - DocstoreClient.createProject @project_id, (error) => + DocstoreClient.createDoc @project_id, @doc_id, @lines, (error) => throw error if error? - DocstoreClient.createDoc @project_id, @doc_id, @lines, (error) => - throw error if error? - done() - - afterEach (done) -> - DocstoreClient.deleteProject @project_id, done + done() describe "when the doc exists", -> beforeEach (done) -> diff --git a/services/docstore/test/acceptance/coffee/GettingAllDocsTests.coffee b/services/docstore/test/acceptance/coffee/GettingAllDocsTests.coffee index 53b26d453b..4463abcdad 100644 --- a/services/docstore/test/acceptance/coffee/GettingAllDocsTests.coffee +++ b/services/docstore/test/acceptance/coffee/GettingAllDocsTests.coffee @@ -22,18 +22,13 @@ describe "Getting all docs", -> lines: ["111", "222", "333"] rev: 6 }] - DocstoreClient.createProject @project_id, (error) => - throw error if error? - jobs = for doc in @docs - do (doc) => - (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, callback - async.series jobs, done - - afterEach (done) -> - DocstoreClient.deleteProject @project_id, done + jobs = for doc in @docs + do (doc) => + (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, callback + async.series jobs, done it "should return all the docs", (done) -> DocstoreClient.getAllDocs @project_id, (error, res, docs) => diff --git a/services/docstore/test/acceptance/coffee/GettingDocsTests.coffee b/services/docstore/test/acceptance/coffee/GettingDocsTests.coffee index 35b51537fa..345f29f89f 100644 --- a/services/docstore/test/acceptance/coffee/GettingDocsTests.coffee +++ b/services/docstore/test/acceptance/coffee/GettingDocsTests.coffee @@ -10,14 +10,9 @@ describe "Getting a doc", -> @project_id = ObjectId() @doc_id = ObjectId() @lines = ["original", "lines"] - DocstoreClient.createProject @project_id, (error) => + DocstoreClient.createDoc @project_id, @doc_id, @lines, (error) => throw error if error? - DocstoreClient.createDoc @project_id, @doc_id, @lines, (error) => - throw error if error? - done() - - afterEach (done) -> - DocstoreClient.deleteProject @project_id, done + done() describe "when the doc exists", -> it "should get the doc lines and version", (done) -> diff --git a/services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee b/services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee index 7d168922af..b18be15c25 100644 --- a/services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee +++ b/services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee @@ -11,14 +11,9 @@ describe "Applying updates to a doc", -> @doc_id = ObjectId() @originalLines = ["original", "lines"] @newLines = ["new", "lines"] - DocstoreClient.createProject @project_id, (error) => + DocstoreClient.createDoc @project_id, @doc_id, @lines, (error) => throw error if error? - DocstoreClient.createDoc @project_id, @doc_id, @lines, (error) => - throw error if error? - done() - - afterEach (done) -> - DocstoreClient.deleteProject @project_id, done + done() describe "when the content has changed", -> beforeEach (done) -> @@ -28,6 +23,9 @@ describe "Applying updates to a doc", -> it "should return modified = true", -> @body.modified.should.equal true + it "should return the rev", -> + @body.rev.should.equal 2 + it "should update the doc in the API", (done) -> DocstoreClient.getDoc @project_id, @doc_id, {}, (error, res, doc) => doc.lines.should.deep.equal @newLines @@ -48,22 +46,18 @@ 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, (error, @res, @body) => + @missing_doc_id = ObjectId() + DocstoreClient.updateDoc @project_id, @missing_doc_id, @originalLines, (error, @res, @body) => done() - it "should return a 404", -> - @res.statusCode.should.equal 404 + it "should create the doc", -> + @body.rev.should.equal 1 - describe "when the project does not exist", -> - beforeEach (done) -> - missing_project_id = ObjectId() - DocstoreClient.updateDoc missing_project_id, @doc_id, @originalLines, (error, @res, @body) => + it "should be retreivable", (done)-> + DocstoreClient.getDoc @project_id, @missing_doc_id, {}, (error, res, doc) => + doc.lines.should.deep.equal @originalLines done() - it "should return a 404", -> - @res.statusCode.should.equal 404 - describe "when malformed doc lines are provided", -> describe "when the lines are not an array", -> beforeEach (done) -> diff --git a/services/docstore/test/acceptance/coffee/helpers/DocstoreClient.coffee b/services/docstore/test/acceptance/coffee/helpers/DocstoreClient.coffee index d3a65790b8..1d2bcadc75 100644 --- a/services/docstore/test/acceptance/coffee/helpers/DocstoreClient.coffee +++ b/services/docstore/test/acceptance/coffee/helpers/DocstoreClient.coffee @@ -2,23 +2,9 @@ request = require("request").defaults(jar: false) {db, ObjectId} = require("../../../../app/js/mongojs") module.exports = DocstoreClient = - createProject: (project_id, callback = (error) ->) -> - db.projects.insert { - _id: project_id - rootFolder: [{ docs: [] }] - }, callback createDoc: (project_id, doc_id, lines, callback = (error) ->) -> - db.projects.update { - _id: project_id - }, { - $push: { - "rootFolder.0.docs": { - _id: doc_id - lines: lines - } - } - }, callback + db.docs.save({_id: doc_id, project_id:project_id, lines: lines, rev:1}, callback) createDeletedDoc: (project_id, doc_id, lines, callback = (error) ->) -> db.docs.insert { @@ -28,9 +14,6 @@ module.exports = DocstoreClient = deleted: true }, callback - deleteProject: (project_id, callback = (error, res, body) ->) -> - db.projects.remove _id: project_id, callback - getDoc: (project_id, doc_id, qs, callback = (error, res, body) ->) -> request.get { url: "http://localhost:3016/project/#{project_id}/doc/#{doc_id}" diff --git a/services/docstore/test/unit/coffee/DocManagerTests.coffee b/services/docstore/test/unit/coffee/DocManagerTests.coffee index cf4f76f0f8..712047f914 100644 --- a/services/docstore/test/unit/coffee/DocManagerTests.coffee +++ b/services/docstore/test/unit/coffee/DocManagerTests.coffee @@ -11,7 +11,10 @@ describe "DocManager", -> beforeEach -> @DocManager = SandboxedModule.require modulePath, requires: "./MongoManager": @MongoManager = {} - "logger-sharelatex": @logger = {log: sinon.stub(), warn:->} + "logger-sharelatex": @logger = + log: sinon.stub() + warn:-> + err:-> @doc_id = ObjectId().toString() @project_id = ObjectId().toString() @callback = sinon.stub() @@ -27,8 +30,6 @@ describe "DocManager", -> beforeEach -> @MongoManager.findDoc = sinon.stub() - @MongoManager.findProject = sinon.stub().callsArgWith(1, null, @project) - @DocManager.findDocInProject = sinon.stub().callsArgWith(2, null, @doc, @mongoPath) it "should get the doc from the doc collection when it is present there", (done)-> @MongoManager.findDoc.callsArgWith(1, null, @docCollectionDoc) @@ -42,46 +43,8 @@ describe "DocManager", -> err.should.equal @stubbedError done() - describe "when the project exists and the doc is in it", -> - beforeEach -> - @mongoPath = "mock.mongo.path" - @MongoManager.findProject = sinon.stub().callsArgWith(1, null, @project) - @MongoManager.findDoc = sinon.stub().callsArg(1) - @DocManager.findDocInProject = sinon.stub().callsArgWith(2, null, @doc, @mongoPath) - @DocManager.getDoc @project_id, @doc_id, @callback - - it "should get the project from the database", -> - @MongoManager.findProject - .calledWith(@project_id) - .should.equal true - - it "should find the doc in the project", -> - @DocManager.findDocInProject - .calledWith(@project, @doc_id) - .should.equal true - - it "should return the doc", -> - @callback.calledWith(null, @doc, @mongoPath).should.equal true - - describe "when the project does not exist", -> - beforeEach -> - @MongoManager.findProject = sinon.stub().callsArgWith(1, null, null) - @MongoManager.findDoc = sinon.stub().callsArg(1) - @DocManager.findDocInProject = sinon.stub() - @DocManager.getDoc @project_id, @doc_id, @callback - - it "should not try to find the doc in the project", -> - @DocManager.findDocInProject.called.should.equal false - - it "should return a NotFoundError", -> - @callback - .calledWith(new Errors.NotFoundError("No such project: #{@project_id}")) - .should.equal true - describe "when the doc is deleted", -> beforeEach -> - @MongoManager.findProject = sinon.stub().callsArgWith(1, null, @project) - @DocManager.findDocInProject = sinon.stub().callsArgWith(2, null, null, null) @MongoManager.findDoc = sinon.stub().callsArgWith(1, null, @doc) @DocManager.getDoc @project_id, @doc_id, @callback @@ -96,55 +59,39 @@ describe "DocManager", -> .calledWith(null, @doc) .should.equal true - - describe "when the doc does not exist anywhere", -> beforeEach -> - @MongoManager.findProject = sinon.stub().callsArgWith(1, null, @project) - @DocManager.findDocInProject = sinon.stub().callsArgWith(2, null, null, null) @MongoManager.findDoc = sinon.stub().callsArgWith(1, null, null) @DocManager.getDoc @project_id, @doc_id, @callback it "should return a NotFoundError", -> @callback - .calledWith(new Errors.NotFoundError("No such doc: #{@doc_id}")) + .calledWith(new Errors.NotFoundError("No such doc: #{@doc_id} in project #{@project_id}")) .should.equal true describe "getAllDocs", -> describe "when the project exists", -> beforeEach -> - @project = { name: "mock-project" } @docs = [{ _id: @doc_id, lines: ["mock-lines"] }] - @mongoPath = "mock.mongo.path" - @MongoManager.findProject = sinon.stub().callsArgWith(1, null, @project) - @DocManager.findAllDocsInProject = sinon.stub().callsArgWith(1, null, @docs) + @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(1, null, @docs) @DocManager.getAllDocs @project_id, @callback it "should get the project from the database", -> - @MongoManager.findProject + @MongoManager.getProjectsDocs .calledWith(@project_id) .should.equal true - it "should find all the docs in the project", -> - @DocManager.findAllDocsInProject - .calledWith(@project) - .should.equal true - it "should return the docs", -> @callback.calledWith(null, @docs).should.equal true - describe "when the project does not exist", -> + describe "when there are no docs for the project", -> beforeEach -> - @MongoManager.findProject = sinon.stub().callsArgWith(1, null, null) - @DocManager.findAllDocsInProject = sinon.stub() + @MongoManager.getProjectsDocs = sinon.stub().callsArgWith(1, null, null) @DocManager.getAllDocs @project_id, @callback - it "should not try to find the doc in the project", -> - @DocManager.findAllDocsInProject.called.should.equal false - it "should return a NotFoundError", -> @callback - .calledWith(new Errors.NotFoundError("No such project: #{@project_id}")) + .calledWith(new Errors.NotFoundError("No such docs for project #{@project_id}")) .should.equal true describe "deleteDoc", -> @@ -194,14 +141,13 @@ describe "DocManager", -> @oldDocLines = ["old", "doc", "lines"] @newDocLines = ["new", "doc", "lines"] @doc = { _id: @doc_id, lines: @oldDocLines, rev: @rev = 5 } - @mongoPath = "mock.mongo.path" @MongoManager.updateDoc = sinon.stub().callsArg(3) @MongoManager.upsertIntoDocCollection = sinon.stub().callsArg(4) describe "when the doc lines have changed", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc, @mongoPath) + @DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc) @DocManager.updateDoc @project_id, @doc_id, @newDocLines, @callback it "should get the existing doc", -> @@ -209,11 +155,6 @@ describe "DocManager", -> .calledWith(@project_id, @doc_id) .should.equal true - it "should update the doc with the new doc lines", -> - @MongoManager.updateDoc - .calledWith(@project_id, @mongoPath, @newDocLines) - .should.equal true - it "should upsert the document to the doc collection", -> @MongoManager.upsertIntoDocCollection .calledWith(@project_id, @doc_id, @newDocLines, @rev) @@ -234,13 +175,25 @@ describe "DocManager", -> it "should return the callback with the new rev", -> @callback.calledWith(null, true, @rev + 1).should.equal true + 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.updateDoc @project_id, @doc_id, @newDocLines, @callback + + it "should not upsert the document to the doc collection", -> + @MongoManager.upsertIntoDocCollection.called.should.equal false + + it "should return the callback with the error", -> + @callback.calledWith(@error).should.equal true + describe "when the doc lines have not changed", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc, @mongoPath) + @DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc) @DocManager.updateDoc @project_id, @doc_id, @oldDocLines.slice(), @callback it "should not update the doc", -> - @MongoManager.updateDoc.called.should.equal false @MongoManager.upsertIntoDocCollection.called.should.equal false it "should return the callback with the existing rev", -> @@ -248,98 +201,17 @@ describe "DocManager", -> describe "when the doc does not exist", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(2, null, null, null) + NotFoundError = Errors.NotFoundError("doc could not be found") + @DocManager.getDoc = sinon.stub().callsArgWith(2, NotFoundError, null, null) @DocManager.updateDoc @project_id, @doc_id, @newDocLines, @callback + + it "should upsert the document to the doc collection", -> + @MongoManager.upsertIntoDocCollection + .calledWith(@project_id, @doc_id, @newDocLines) + .should.equal true - it "should not try to update the doc", -> - @MongoManager.updateDoc.called.should.equal false - @MongoManager.upsertIntoDocCollection.called.should.equal false + it "should return the callback with the new rev", -> + @callback.calledWith(null, true, 1).should.equal true - it "should return a NotFoundError", -> - @callback - .calledWith(new Errors.NotFoundError("No such project/doc: #{@project_id}/#{@doc_id}")) - .should.equal true - describe "findDocInProject", -> - it "should find the doc when it is in the root folder", (done) -> - @DocManager.findDocInProject { - rootFolder: [{ - docs: [{ - _id: ObjectId(@doc_id) - }] - }] - }, @doc_id, (error, doc, mongoPath) => - expect(doc).to.deep.equal { _id: ObjectId(@doc_id) } - mongoPath.should.equal "rootFolder.0.docs.0" - done() - it "should find the doc when it is in a sub folder", (done) -> - @DocManager.findDocInProject { - rootFolder: [{ - folders: [{ - docs: [{ - _id: ObjectId(@doc_id) - }] - }] - }] - }, @doc_id, (error, doc, mongoPath) => - expect(doc).to.deep.equal { _id: ObjectId(@doc_id) } - mongoPath.should.equal "rootFolder.0.folders.0.docs.0" - done() - - it "should find the doc when it there are other docs", (done) -> - @DocManager.findDocInProject { - rootFolder: [{ - folders: [{ - docs: [{ - _id: ObjectId() - }] - }, { - docs: [{ - _id: ObjectId() - }, { - _id: ObjectId(@doc_id) - }] - }], - docs: [{ - _id: ObjectId() - }] - }] - }, @doc_id, (error, doc, mongoPath) => - expect(doc).to.deep.equal { _id: ObjectId(@doc_id) } - mongoPath.should.equal "rootFolder.0.folders.1.docs.1" - done() - - it "should return null when the doc doesn't exist", (done) -> - @DocManager.findDocInProject { - rootFolder: [{ - folders: [{ - docs: [] - }] - }] - }, @doc_id, (error, doc, mongoPath) => - expect(doc).to.be.null - expect(mongoPath).to.be.null - done() - - describe "findAllDocsInProject", -> - it "should return all the docs", (done) -> - @DocManager.findAllDocsInProject { - rootFolder: [{ - docs: [ - @doc1 = { _id: ObjectId() } - ], - folders: [{ - docs: [ - @doc2 = { _id: ObjectId() } - ] - }, { - docs: [ - @doc3 = { _id: ObjectId() } - @doc4 = { _id: ObjectId() } - ] - }] - }] - }, (error, docs) => - expect(docs).to.deep.equal [@doc1, @doc2, @doc3, @doc4] - done() diff --git a/services/docstore/test/unit/coffee/MongoManagerTests.coffee b/services/docstore/test/unit/coffee/MongoManagerTests.coffee index aac8447a4c..add0b33c37 100644 --- a/services/docstore/test/unit/coffee/MongoManagerTests.coffee +++ b/services/docstore/test/unit/coffee/MongoManagerTests.coffee @@ -9,29 +9,13 @@ describe "MongoManager", -> beforeEach -> @MongoManager = SandboxedModule.require modulePath, requires: "./mongojs": - db: @db = { projects: {}, docs: {} } + db: @db = { docs: {} } ObjectId: ObjectId @project_id = ObjectId().toString() @doc_id = ObjectId().toString() @callback = sinon.stub() @stubbedErr = new Error("hello world") - describe "findProject", -> - beforeEach -> - @project = { name: "mock-project" } - @db.projects.find = sinon.stub().callsArgWith(2, null, [@project]) - @MongoManager.findProject @project_id, @callback - - it "should find the project", -> - @db.projects.find - .calledWith({ - _id: ObjectId(@project_id) - }, {}) - .should.equal true - - it "should call the callback with the project", -> - @callback.calledWith(null, @project).should.equal true - describe "findDoc", -> beforeEach -> @doc = { name: "mock-doc" } @@ -48,28 +32,24 @@ describe "MongoManager", -> it "should call the callback with the doc", -> @callback.calledWith(null, @doc).should.equal true - describe "updateDoc", -> + describe "getProjectsDocs", -> beforeEach -> - @lines = ["mock-lines"] - @docPath = "rootFolder.0.folders.1.docs.0" - @db.projects.update = sinon.stub().callsArg(2) - @MongoManager.updateDoc @project_id, @docPath, @lines, @callback + @doc1 = { name: "mock-doc1" } + @doc2 = { name: "mock-doc2" } + @doc3 = { name: "mock-doc3" } + @doc4 = { name: "mock-doc4" } + @db.docs.find = sinon.stub().callsArgWith(2, null, [@doc, @doc3, @doc4]) + @MongoManager.getProjectsDocs @project_id, @callback - it "should update the doc lines and increment the TPDS rev", -> - @db.projects.update + it "should find the docs via the project_id", -> + @db.docs.find .calledWith({ - _id: ObjectId(@project_id) - }, { - $set: - "rootFolder.0.folders.1.docs.0.lines": @lines - $inc: - "rootFolder.0.folders.1.docs.0.rev": 1 - }) + project_id: ObjectId(@project_id) + }, {}) .should.equal true - it "should call the callback with the project", -> - @callback.called.should.equal true - + it "should call the callback with the docs", -> + @callback.calledWith(null, [@doc, @doc3, @doc4]).should.equal true describe "upsertIntoDocCollection", -> beforeEach -> From 724211953273bd654c29747bcd966f5d5a0d6c5d Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Thu, 26 Feb 2015 16:00:28 +0000 Subject: [PATCH 3/5] simplified DocManager.updateDoc so it calls mongomanager.findDoc makes the logic for dealing new documents that are not in mongo yet much simpler --- .../docstore/app/coffee/DocManager.coffee | 41 ++++++++----------- .../test/unit/coffee/DocManagerTests.coffee | 15 ++++--- 2 files changed, 25 insertions(+), 31 deletions(-) diff --git a/services/docstore/app/coffee/DocManager.coffee b/services/docstore/app/coffee/DocManager.coffee index 2cbd0b65c7..fbf201bcd5 100644 --- a/services/docstore/app/coffee/DocManager.coffee +++ b/services/docstore/app/coffee/DocManager.coffee @@ -24,31 +24,26 @@ module.exports = DocManager = return callback(null, docs) updateDoc: (project_id, doc_id, lines, callback = (error, modified, rev) ->) -> - DocManager.getDoc project_id, doc_id, (error, doc) -> - - isNewDoc = error?.name == new Errors.NotFoundError().name - - if !isNewDoc and error? - logger.err project_id: project_id, doc_id: doc_id, err:error, "error getting document for update" - return callback(error) - else if !isNewDoc and !doc? - logger.err project_id: project_id, doc_id: doc_id, "existing document to update could not be found" - return callback new Errors.NotFoundError("No such project/doc to update: #{project_id}/#{doc_id}") - else if _.isEqual(doc?.lines, lines) + MongoManager.findDoc doc_id, (err, doc)-> + if err? + logger.err project_id: project_id, doc_id: doc_id, err:err, "error getting document for update" + return callback(err) + + if _.isEqual(doc?.lines, lines) logger.log project_id: project_id, doc_id: doc_id, rev: doc?.rev, "doc lines have not changed - not updating" return callback null, false, doc?.rev - - oldRev = doc?.rev || 0 - logger.log { - project_id: project_id - doc_id: doc_id, - oldDocLines: doc?.lines - newDocLines: lines - rev: oldRev - }, "updating doc lines" - MongoManager.upsertIntoDocCollection project_id, doc_id, lines, oldRev, (error)-> - return callback(callback) if error? - callback null, true, oldRev + 1 # rev will have been incremented in mongo by MongoManager.updateDoc + else + oldRev = doc?.rev || 0 + logger.log { + project_id: project_id + doc_id: doc_id, + oldDocLines: doc?.lines + newDocLines: lines + rev: oldRev + }, "updating doc lines" + MongoManager.upsertIntoDocCollection project_id, doc_id, lines, oldRev, (error)-> + return callback(callback) if error? + 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) -> diff --git a/services/docstore/test/unit/coffee/DocManagerTests.coffee b/services/docstore/test/unit/coffee/DocManagerTests.coffee index 712047f914..81bd9a3a6d 100644 --- a/services/docstore/test/unit/coffee/DocManagerTests.coffee +++ b/services/docstore/test/unit/coffee/DocManagerTests.coffee @@ -142,17 +142,17 @@ describe "DocManager", -> @newDocLines = ["new", "doc", "lines"] @doc = { _id: @doc_id, lines: @oldDocLines, rev: @rev = 5 } - @MongoManager.updateDoc = sinon.stub().callsArg(3) @MongoManager.upsertIntoDocCollection = sinon.stub().callsArg(4) + @MongoManager.findDoc = sinon.stub() describe "when the doc lines have changed", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc) + @MongoManager.findDoc = sinon.stub().callsArgWith(1, null, @doc) @DocManager.updateDoc @project_id, @doc_id, @newDocLines, @callback it "should get the existing doc", -> - @DocManager.getDoc - .calledWith(@project_id, @doc_id) + @MongoManager.findDoc + .calledWith(@doc_id) .should.equal true it "should upsert the document to the doc collection", -> @@ -179,7 +179,7 @@ describe "DocManager", -> beforeEach -> @error = new Error("doc could not be found") - @DocManager.getDoc = sinon.stub().callsArgWith(2, @error, null, null) + @MongoManager.findDoc = sinon.stub().callsArgWith(1, @error, null, null) @DocManager.updateDoc @project_id, @doc_id, @newDocLines, @callback it "should not upsert the document to the doc collection", -> @@ -190,7 +190,7 @@ describe "DocManager", -> describe "when the doc lines have not changed", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(2, null, @doc) + @MongoManager.findDoc = sinon.stub().callsArgWith(1, null, @doc) @DocManager.updateDoc @project_id, @doc_id, @oldDocLines.slice(), @callback it "should not update the doc", -> @@ -201,8 +201,7 @@ describe "DocManager", -> describe "when the doc does not exist", -> beforeEach -> - NotFoundError = Errors.NotFoundError("doc could not be found") - @DocManager.getDoc = sinon.stub().callsArgWith(2, NotFoundError, null, null) + @MongoManager.findDoc = sinon.stub().callsArgWith(1, null, null, null) @DocManager.updateDoc @project_id, @doc_id, @newDocLines, @callback it "should upsert the document to the doc collection", -> From a73dc90b00930d3808dd2bfa7e2075e9cb7c7a29 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Thu, 26 Feb 2015 16:01:10 +0000 Subject: [PATCH 4/5] change mongomangers upsert to use an inc this is tested and does work with new documents --- services/docstore/app/coffee/MongoManager.coffee | 3 ++- services/docstore/test/unit/coffee/MongoManagerTests.coffee | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/services/docstore/app/coffee/MongoManager.coffee b/services/docstore/app/coffee/MongoManager.coffee index be13b6a084..8db8d57dc3 100644 --- a/services/docstore/app/coffee/MongoManager.coffee +++ b/services/docstore/app/coffee/MongoManager.coffee @@ -12,9 +12,10 @@ module.exports = MongoManager = upsertIntoDocCollection: (project_id, doc_id, lines, oldRev, callback)-> update = $set:{} + $inc:{} update.$set["lines"] = lines update.$set["project_id"] = ObjectId(project_id) - update.$set["rev"] = oldRev + 1 + update.$inc["rev"] = 1 #on new docs being created this will set the rev to 1 db.docs.update _id: ObjectId(doc_id), update, {upsert: true}, callback diff --git a/services/docstore/test/unit/coffee/MongoManagerTests.coffee b/services/docstore/test/unit/coffee/MongoManagerTests.coffee index add0b33c37..97f09035e8 100644 --- a/services/docstore/test/unit/coffee/MongoManagerTests.coffee +++ b/services/docstore/test/unit/coffee/MongoManagerTests.coffee @@ -61,7 +61,7 @@ describe "MongoManager", -> args = @db.docs.update.args[0] assert.deepEqual args[0], {_id: ObjectId(@doc_id)} assert.equal args[1]["$set"]["lines"], @lines - assert.equal args[1]["$set"]["rev"], 78 + assert.equal args[1]["$inc"]["rev"], 1 assert.deepEqual args[1]["$set"]["project_id"], ObjectId(@project_id) done() From 32829f0e6ea7b4a7550bb2fec89683b2b7043db0 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Fri, 27 Feb 2015 14:06:06 +0000 Subject: [PATCH 5/5] remove old rev, not needed any more --- services/docstore/app/coffee/DocManager.coffee | 4 ++-- services/docstore/app/coffee/MongoManager.coffee | 2 +- services/docstore/test/unit/coffee/DocManagerTests.coffee | 8 ++++---- .../docstore/test/unit/coffee/MongoManagerTests.coffee | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/services/docstore/app/coffee/DocManager.coffee b/services/docstore/app/coffee/DocManager.coffee index fbf201bcd5..4cef291854 100644 --- a/services/docstore/app/coffee/DocManager.coffee +++ b/services/docstore/app/coffee/DocManager.coffee @@ -41,7 +41,7 @@ module.exports = DocManager = newDocLines: lines rev: oldRev }, "updating doc lines" - MongoManager.upsertIntoDocCollection project_id, doc_id, lines, oldRev, (error)-> + MongoManager.upsertIntoDocCollection project_id, doc_id, lines, (error)-> return callback(callback) if error? callback null, true, oldRev + 1 # rev will have been incremented in mongo by MongoManager.updateDoc @@ -49,7 +49,7 @@ module.exports = DocManager = 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, doc.rev, (error) -> + MongoManager.upsertIntoDocCollection project_id, doc_id, doc.lines, (error) -> return callback(error) if error? MongoManager.markDocAsDeleted doc_id, (error) -> return callback(error) if error? diff --git a/services/docstore/app/coffee/MongoManager.coffee b/services/docstore/app/coffee/MongoManager.coffee index 8db8d57dc3..352483fbfc 100644 --- a/services/docstore/app/coffee/MongoManager.coffee +++ b/services/docstore/app/coffee/MongoManager.coffee @@ -9,7 +9,7 @@ module.exports = MongoManager = getProjectsDocs: (project_id, callback)-> db.docs.find project_id: ObjectId(project_id.toString()), {}, callback - upsertIntoDocCollection: (project_id, doc_id, lines, oldRev, callback)-> + upsertIntoDocCollection: (project_id, doc_id, lines, callback)-> update = $set:{} $inc:{} diff --git a/services/docstore/test/unit/coffee/DocManagerTests.coffee b/services/docstore/test/unit/coffee/DocManagerTests.coffee index 81bd9a3a6d..b09ed900c8 100644 --- a/services/docstore/test/unit/coffee/DocManagerTests.coffee +++ b/services/docstore/test/unit/coffee/DocManagerTests.coffee @@ -100,7 +100,7 @@ describe "DocManager", -> @lines = ["mock", "doc", "lines"] @rev = 77 @DocManager.getDoc = sinon.stub().callsArgWith(2, null, {lines: @lines, rev:@rev}) - @MongoManager.upsertIntoDocCollection = sinon.stub().callsArg(4) + @MongoManager.upsertIntoDocCollection = sinon.stub().callsArg(3) @MongoManager.markDocAsDeleted = sinon.stub().callsArg(1) @DocManager.deleteDoc @project_id, @doc_id, @callback @@ -111,7 +111,7 @@ describe "DocManager", -> it "should update the doc lines", -> @MongoManager.upsertIntoDocCollection - .calledWith(@project_id, @doc_id, @lines, @rev) + .calledWith(@project_id, @doc_id, @lines) .should.equal true it "should mark doc as deleted", -> @@ -142,7 +142,7 @@ describe "DocManager", -> @newDocLines = ["new", "doc", "lines"] @doc = { _id: @doc_id, lines: @oldDocLines, rev: @rev = 5 } - @MongoManager.upsertIntoDocCollection = sinon.stub().callsArg(4) + @MongoManager.upsertIntoDocCollection = sinon.stub().callsArg(3) @MongoManager.findDoc = sinon.stub() describe "when the doc lines have changed", -> @@ -157,7 +157,7 @@ describe "DocManager", -> it "should upsert the document to the doc collection", -> @MongoManager.upsertIntoDocCollection - .calledWith(@project_id, @doc_id, @newDocLines, @rev) + .calledWith(@project_id, @doc_id, @newDocLines) .should.equal true it "should log out the old and new doc lines", -> diff --git a/services/docstore/test/unit/coffee/MongoManagerTests.coffee b/services/docstore/test/unit/coffee/MongoManagerTests.coffee index 97f09035e8..b0e7c07e97 100644 --- a/services/docstore/test/unit/coffee/MongoManagerTests.coffee +++ b/services/docstore/test/unit/coffee/MongoManagerTests.coffee @@ -57,7 +57,7 @@ describe "MongoManager", -> @oldRev = 77 it "should upsert the document", (done)-> - @MongoManager.upsertIntoDocCollection @project_id, @doc_id, @lines, @oldRev, (err)=> + @MongoManager.upsertIntoDocCollection @project_id, @doc_id, @lines, (err)=> args = @db.docs.update.args[0] assert.deepEqual args[0], {_id: ObjectId(@doc_id)} assert.equal args[1]["$set"]["lines"], @lines @@ -66,7 +66,7 @@ describe "MongoManager", -> done() it "should return the error", (done)-> - @MongoManager.upsertIntoDocCollection @project_id, @doc_id, @lines, @oldRev, (err)=> + @MongoManager.upsertIntoDocCollection @project_id, @doc_id, @lines, (err)=> err.should.equal @stubbedErr done()