diff --git a/services/docstore/app/coffee/DocArchiveManager.coffee b/services/docstore/app/coffee/DocArchiveManager.coffee index a4ae7c2344..9c16d13d67 100644 --- a/services/docstore/app/coffee/DocArchiveManager.coffee +++ b/services/docstore/app/coffee/DocArchiveManager.coffee @@ -84,7 +84,7 @@ module.exports = DocArchive = logger.err err:err, res:res, project_id:project_id, doc_id:doc_id, "something went wrong deleting doc from aws" return callback new Errors.NotFoundError("Error in S3 request") callback() - + _s3DocToMongoDoc: (doc, callback = (error, mongo_doc) ->) -> mongo_doc = {} if doc.schema_v == 1 and doc.lines? @@ -119,4 +119,4 @@ module.exports = DocArchive = bucket: settings.docstore.s3.bucket timeout: thirtySeconds uri:"https://#{settings.docstore.s3.bucket}.s3.amazonaws.com/#{key}" - } \ No newline at end of file + } diff --git a/services/docstore/app/coffee/DocManager.coffee b/services/docstore/app/coffee/DocManager.coffee index 6cf679c6c1..d9f0bd6747 100644 --- a/services/docstore/app/coffee/DocManager.coffee +++ b/services/docstore/app/coffee/DocManager.coffee @@ -6,11 +6,16 @@ DocArchive = require "./DocArchiveManager" RangeManager = require "./RangeManager" module.exports = DocManager = + + # TODO: For historical reasons, the doc version is currently stored in the docOps # collection (which is all that this collection contains). In future, we should # migrate this version property to be part of the docs collection, to guarantee # consitency between lines and version when writing/reading, and for a simpler schema. - getDoc: (project_id, doc_id, filter = { version: false }, callback = (error, doc) ->) -> + _getDoc: (project_id, doc_id, filter = {}, callback = (error, doc) ->) -> + if filter.inS3 != true + return callback("must include inS3 when getting doc") + MongoManager.findDoc project_id, doc_id, filter, (err, doc)-> if err? return callback(err) @@ -21,7 +26,7 @@ module.exports = DocManager = if err? logger.err err:err, project_id:project_id, doc_id:doc_id, "error unarchiving doc" return callback(err) - DocManager.getDoc project_id, doc_id, filter, callback + DocManager._getDoc project_id, doc_id, filter, callback else if filter.version MongoManager.getDocVersion doc_id, (error, version) -> @@ -31,8 +36,29 @@ module.exports = DocManager = else callback err, doc + checkDocExists: (project_id, doc_id, callback = (err, exists)->)-> + DocManager._getDoc project_id, doc_id, {_id:1, inS3:true}, (err, doc)-> + if err? + return callback(err) + callback(err, doc?) + + getFullDoc: (project_id, doc_id, callback = (err, doc)->)-> + DocManager._getDoc project_id, doc_id, {lines: true, rev: true, deleted: true, version: true, ranges: true, inS3:true}, (err, doc)-> + if err? + return callback(err) + callback(err, doc) + + + getDocLines: (project_id, doc_id, callback = (err, doc)->)-> + DocManager._getDoc project_id, doc_id, {lines:true, inS3:true}, (err, doc)-> + if err? + return callback(err) + callback(err, doc) + getAllNonDeletedDocs: (project_id, filter, callback = (error, docs) ->) -> DocArchive.unArchiveAllDocs project_id, (error) -> + if error? + return callback(error) MongoManager.getProjectsDocs project_id, {include_deleted: false}, filter, (error, docs) -> if err? return callback(error) @@ -45,7 +71,7 @@ module.exports = DocManager = 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)-> + DocManager._getDoc project_id, doc_id, {version: true, rev: true, lines: true, version: true, ranges: true, inS3: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" return callback(err) @@ -97,8 +123,8 @@ module.exports = DocManager = callback null, modified, rev deleteDoc: (project_id, doc_id, callback = (error) ->) -> - DocManager.getDoc project_id, doc_id, { version: false }, (error, doc) -> + DocManager.checkDocExists project_id, doc_id, (error, exists) -> return callback(error) if error? - return callback new Errors.NotFoundError("No such project/doc to delete: #{project_id}/#{doc_id}") if !doc? + return callback new Errors.NotFoundError("No such project/doc to delete: #{project_id}/#{doc_id}") if !exists MongoManager.markDocAsDeleted project_id, doc_id, callback diff --git a/services/docstore/app/coffee/HttpController.coffee b/services/docstore/app/coffee/HttpController.coffee index 7c8dd0aecf..75298580b3 100644 --- a/services/docstore/app/coffee/HttpController.coffee +++ b/services/docstore/app/coffee/HttpController.coffee @@ -10,7 +10,7 @@ module.exports = HttpController = doc_id = req.params.doc_id include_deleted = req.query?.include_deleted == "true" logger.log project_id: project_id, doc_id: doc_id, "getting doc" - DocManager.getDoc project_id, doc_id, {lines: true, rev: true, deleted: true, version: true, ranges: true}, (error, doc) -> + DocManager.getFullDoc project_id, doc_id, (error, doc) -> return next(error) if error? logger.log doc: doc, "got doc" if !doc? @@ -24,7 +24,7 @@ module.exports = HttpController = project_id = req.params.project_id doc_id = req.params.doc_id logger.log project_id: project_id, doc_id: doc_id, "getting raw doc" - DocManager.getDoc project_id, doc_id, {lines: true}, (error, doc) -> + DocManager.getDocLines project_id, doc_id, (error, doc) -> return next(error) if error? if !doc? res.send 404 diff --git a/services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee b/services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee index 00d6ade7a4..2c457922c0 100644 --- a/services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee +++ b/services/docstore/test/acceptance/coffee/ArchiveDocsTests.coffee @@ -33,7 +33,7 @@ describe "Archiving", -> throw error if error? DocstoreClient.archiveAllDoc @project_id, (error, @res) => done() - + it "should archive all the docs", (done) -> @res.statusCode.should.equal 204 done() @@ -384,3 +384,4 @@ describe "Archiving", -> it "should return the doc", (done) -> @fetched_docs[0].lines.should.deep.equal @doc.lines done() + diff --git a/services/docstore/test/acceptance/scripts/full-test.sh b/services/docstore/test/acceptance/scripts/full-test.sh index 9f6167e667..afd5ef2ecb 100755 --- a/services/docstore/test/acceptance/scripts/full-test.sh +++ b/services/docstore/test/acceptance/scripts/full-test.sh @@ -1,6 +1,6 @@ #! /usr/bin/env bash -npm rebuild +# npm rebuild echo ">> Starting server..." @@ -8,7 +8,7 @@ grunt --no-color forever:app:start echo ">> Server started" -sleep 5 +sleep 20 echo ">> Running acceptance tests..." grunt --no-color mochaTest:acceptance diff --git a/services/docstore/test/unit/coffee/DocArchiveManager.coffee b/services/docstore/test/unit/coffee/DocArchiveManagerTests.coffee similarity index 95% rename from services/docstore/test/unit/coffee/DocArchiveManager.coffee rename to services/docstore/test/unit/coffee/DocArchiveManagerTests.coffee index baa5e57bb1..c6c9eacea2 100644 --- a/services/docstore/test/unit/coffee/DocArchiveManager.coffee +++ b/services/docstore/test/unit/coffee/DocArchiveManagerTests.coffee @@ -134,6 +134,14 @@ describe "DocArchiveManager", -> should.exist err done() + it "should error if the doc lines are a string not an array", (done)-> + @request.get = sinon.stub().callsArgWith(1, null, statusCode:200, "this is a string") + @request.del = sinon.stub() + @DocArchiveManager.unarchiveDoc @project_id, @mongoDocs[0], (err)=> + should.exist err + @request.del.called.should.equal false + done() + describe "archiveAllDocs", -> it "should archive all project docs which are not in s3", (done)-> diff --git a/services/docstore/test/unit/coffee/DocManagerTests.coffee b/services/docstore/test/unit/coffee/DocManagerTests.coffee index 9b43bc05af..082badd0f3 100644 --- a/services/docstore/test/unit/coffee/DocManagerTests.coffee +++ b/services/docstore/test/unit/coffee/DocManagerTests.coffee @@ -1,6 +1,7 @@ SandboxedModule = require('sandboxed-module') sinon = require('sinon') chai = require('chai') +assert = require("chai").assert chai.should() expect = chai.expect modulePath = require('path').join __dirname, '../../../app/js/DocManager' @@ -26,6 +27,72 @@ describe "DocManager", -> @callback = sinon.stub() @stubbedError = new Error("blew up") + + describe "checkDocExists", -> + beforeEach -> + @DocManager._getDoc = sinon.stub() + + it "should call get doc with a quick filter", (done)-> + @DocManager._getDoc.callsArgWith(3, null, {_id:@doc_id}) + @DocManager.checkDocExists @project_id, @doc_id, (err, exist)=> + exist.should.equal true + @DocManager._getDoc.calledWith(@project_id, @doc_id, {_id:1, inS3:true}).should.equal true + done() + + it "should return false when doc is not there", (done)-> + @DocManager._getDoc.callsArgWith(3, null) + @DocManager.checkDocExists @project_id, @doc_id, (err, exist)=> + exist.should.equal false + done() + + it "should return error when get doc errors", (done)-> + @DocManager._getDoc.callsArgWith(3, "error") + @DocManager.checkDocExists @project_id, @doc_id, (err, exist)=> + err.should.equal "error" + done() + + describe "getFullDoc", -> + beforeEach -> + @DocManager._getDoc = sinon.stub() + @doc = + _id: @doc_id + lines:["2134"] + + it "should call get doc with a quick filter", (done)-> + @DocManager._getDoc.callsArgWith(3, null, @doc) + @DocManager.getFullDoc @project_id, @doc_id, (err, doc)=> + doc.should.equal @doc + @DocManager._getDoc.calledWith(@project_id, @doc_id, {lines: true, rev: true, deleted: true, version: true, ranges: true, inS3:true}).should.equal true + done() + + it "should return error when get doc errors", (done)-> + @DocManager._getDoc.callsArgWith(3, "error") + @DocManager.getFullDoc @project_id, @doc_id, (err, exist)=> + err.should.equal "error" + done() + + describe "getRawDoc", -> + + beforeEach -> + @DocManager._getDoc = sinon.stub() + @doc = + lines:["2134"] + + it "should call get doc with a quick filter", (done)-> + @DocManager._getDoc.callsArgWith(3, null, @doc) + @DocManager.getDocLines @project_id, @doc_id, (err, doc)=> + doc.should.equal @doc + @DocManager._getDoc.calledWith(@project_id, @doc_id, {lines: true, inS3:true}).should.equal true + done() + + it "should return error when get doc errors", (done)-> + @DocManager._getDoc.callsArgWith(3, "error") + @DocManager.getDocLines @project_id, @doc_id, (err, exist)=> + err.should.equal "error" + done() + + + describe "getDoc", -> beforeEach -> @project = { name: "mock-project" } @@ -34,10 +101,30 @@ describe "DocManager", -> @MongoManager.findDoc = sinon.stub() @MongoManager.getDocVersion = sinon.stub().yields(null, @version) + describe "when using a filter", -> + beforeEach -> + @MongoManager.findDoc.yields(null, @doc) + + it "should error if inS3 is not set to true", (done)-> + @DocManager._getDoc @project_id, @doc_id, {inS3: false}, (err)-> + expect(err).to.exist + done() + + it "should always get inS3 even when no filter is passed", (done)-> + @DocManager._getDoc @project_id, @doc_id, undefined, (err)=> + @MongoManager.findDoc.called.should.equal false + expect(err).to.exist + done() + + it "should not error if inS3 is set to true", (done)-> + @DocManager._getDoc @project_id, @doc_id, {inS3: true}, (err)-> + expect(err).to.not.exist + done() + describe "when the doc is in the doc collection", -> beforeEach -> @MongoManager.findDoc.yields(null, @doc) - @DocManager.getDoc @project_id, @doc_id, {version: true}, @callback + @DocManager._getDoc @project_id, @doc_id, {version: true, inS3:true}, @callback it "should get the doc from the doc collection", -> @MongoManager.findDoc @@ -58,7 +145,7 @@ describe "DocManager", -> describe "without the version filter", -> beforeEach -> @MongoManager.findDoc.yields(null, @doc) - @DocManager.getDoc @project_id, @doc_id, {version: false}, @callback + @DocManager._getDoc @project_id, @doc_id, {version: false, inS3:true}, @callback it "should not get the doc version from the docOps collection", -> @MongoManager.getDocVersion.called.should.equal false @@ -66,7 +153,7 @@ describe "DocManager", -> describe "when MongoManager.findDoc errors", -> beforeEach -> @MongoManager.findDoc.yields(@stubbedError) - @DocManager.getDoc @project_id, @doc_id, {version: true}, @callback + @DocManager._getDoc @project_id, @doc_id, {version: true, inS3:true}, @callback it "should return the error", -> @callback.calledWith(@stubbedError).should.equal true @@ -79,7 +166,7 @@ describe "DocManager", -> @doc.inS3 = false callback() sinon.spy @DocArchiveManager, "unarchiveDoc" - @DocManager.getDoc @project_id, @doc_id, {version: true}, @callback + @DocManager._getDoc @project_id, @doc_id, {version: true, inS3:true}, @callback it "should call the DocArchive to unarchive the doc", -> @DocArchiveManager.unarchiveDoc.calledWith(@project_id, @doc_id).should.equal true @@ -93,7 +180,7 @@ describe "DocManager", -> describe "when the doc does not exist in the docs collection", -> beforeEach -> @MongoManager.findDoc = sinon.stub().yields(null, null) - @DocManager.getDoc @project_id, @doc_id, {version: true}, @callback + @DocManager._getDoc @project_id, @doc_id, {version: true, inS3:true}, @callback it "should return a NotFoundError", -> @callback @@ -133,12 +220,12 @@ describe "DocManager", -> beforeEach -> @lines = ["mock", "doc", "lines"] @rev = 77 - @DocManager.getDoc = sinon.stub().callsArgWith(3, null, {lines: @lines, rev:@rev}) + @DocManager.checkDocExists = sinon.stub().callsArgWith(2, null, true) @MongoManager.markDocAsDeleted = sinon.stub().callsArg(2) @DocManager.deleteDoc @project_id, @doc_id, @callback it "should get the doc", -> - @DocManager.getDoc + @DocManager.checkDocExists .calledWith(@project_id, @doc_id) .should.equal true @@ -152,10 +239,9 @@ describe "DocManager", -> describe "when the doc does not exist", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(3, null, null) + @DocManager.checkDocExists = sinon.stub().callsArgWith(2, null, false) @DocManager.deleteDoc @project_id, @doc_id, @callback - it "should return a NotFoundError", -> @callback .calledWith(new Errors.NotFoundError("No such doc: #{@doc_id}")) @@ -188,16 +274,16 @@ describe "DocManager", -> @MongoManager.upsertIntoDocCollection = sinon.stub().callsArg(3) @MongoManager.setDocVersion = sinon.stub().yields() - @DocManager.getDoc = sinon.stub() + @DocManager._getDoc = sinon.stub() describe "when only the doc lines have changed", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc) + @DocManager._getDoc = sinon.stub().callsArgWith(3, null, @doc) @DocManager.updateDoc @project_id, @doc_id, @newDocLines, @version, @originalRanges, @callback it "should get the existing doc", -> - @DocManager.getDoc - .calledWith(@project_id, @doc_id, {version: true, rev: true, lines: true, version: true, ranges: true}) + @DocManager._getDoc + .calledWith(@project_id, @doc_id, {version: true, rev: true, lines: true, version: true, ranges: true, inS3:true}) .should.equal true it "should upsert the document to the doc collection", -> @@ -213,7 +299,7 @@ describe "DocManager", -> describe "when the doc ranges have changed", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc) + @DocManager._getDoc = sinon.stub().callsArgWith(3, null, @doc) @RangeManager.shouldUpdateRanges.returns true @DocManager.updateDoc @project_id, @doc_id, @oldDocLines, @version, @newRanges, @callback @@ -230,7 +316,7 @@ describe "DocManager", -> describe "when only the version has changed", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc) + @DocManager._getDoc = sinon.stub().callsArgWith(3, null, @doc) @DocManager.updateDoc @project_id, @doc_id, @oldDocLines, @version + 1, @originalRanges, @callback it "should not change the lines or ranges", -> @@ -246,7 +332,7 @@ describe "DocManager", -> describe "when the doc has not changed at all", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc) + @DocManager._getDoc = sinon.stub().callsArgWith(3, null, @doc) @DocManager.updateDoc @project_id, @doc_id, @oldDocLines, @version, @originalRanges, @callback it "should not update the ranges or lines", -> @@ -282,7 +368,7 @@ describe "DocManager", -> describe "when there is a generic error getting the doc", -> beforeEach -> @error = new Error("doc could not be found") - @DocManager.getDoc = sinon.stub().callsArgWith(3, @error, null, null) + @DocManager._getDoc = sinon.stub().callsArgWith(3, @error, null, null) @DocManager.updateDoc @project_id, @doc_id, @newDocLines, @version, @originalRanges, @callback it "should not upsert the document to the doc collection", -> @@ -293,7 +379,7 @@ describe "DocManager", -> describe "when the doc lines have not changed", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc) + @DocManager._getDoc = sinon.stub().callsArgWith(3, null, @doc) @DocManager.updateDoc @project_id, @doc_id, @oldDocLines.slice(), @version, @originalRanges, @callback it "should not update the doc", -> @@ -304,7 +390,7 @@ describe "DocManager", -> describe "when the doc does not exist", -> beforeEach -> - @DocManager.getDoc = sinon.stub().callsArgWith(3, null, null, null) + @DocManager._getDoc = sinon.stub().callsArgWith(3, null, null, null) @DocManager.updateDoc @project_id, @doc_id, @newDocLines, @version, @originalRanges, @callback it "should upsert the document to the doc collection", -> diff --git a/services/docstore/test/unit/coffee/HttpControllerTests.coffee b/services/docstore/test/unit/coffee/HttpControllerTests.coffee index 1a0752d850..0f32f4b9e3 100644 --- a/services/docstore/test/unit/coffee/HttpControllerTests.coffee +++ b/services/docstore/test/unit/coffee/HttpControllerTests.coffee @@ -40,12 +40,12 @@ describe "HttpController", -> @req.params = project_id: @project_id doc_id: @doc_id - @DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc) + @DocManager.getFullDoc = sinon.stub().callsArgWith(2, null, @doc) @HttpController.getDoc @req, @res, @next it "should get the document with the version (including deleted)", -> - @DocManager.getDoc - .calledWith(@project_id, @doc_id, {lines: true, rev: true, deleted: true, version: true, ranges: true}) + @DocManager.getFullDoc + .calledWith(@project_id, @doc_id) .should.equal true it "should return the doc as JSON", -> @@ -63,11 +63,11 @@ describe "HttpController", -> @req.params = project_id: @project_id doc_id: @doc_id - @DocManager.getDoc = sinon.stub().callsArgWith(3, null, @deletedDoc) + @DocManager.getFullDoc = sinon.stub().callsArgWith(2, null, @deletedDoc) it "should get the doc from the doc manager", -> @HttpController.getDoc @req, @res, @next - @DocManager.getDoc.calledWith(@project_id, @doc_id, {lines: true, rev: true, deleted: true, version: true, ranges: true}).should.equal true + @DocManager.getFullDoc.calledWith(@project_id, @doc_id).should.equal true it "should return 404 if the query string delete is not set ", -> @HttpController.getDoc @req, @res, @next @@ -91,12 +91,12 @@ describe "HttpController", -> @req.params = project_id: @project_id doc_id: @doc_id - @DocManager.getDoc = sinon.stub().callsArgWith(3, null, @doc) + @DocManager.getDocLines = sinon.stub().callsArgWith(2, null, @doc) @HttpController.getRawDoc @req, @res, @next it "should get the document without the version", -> - @DocManager.getDoc - .calledWith(@project_id, @doc_id, {lines: true}) + @DocManager.getDocLines + .calledWith(@project_id, @doc_id) .should.equal true it "should set the content type header", ->