diff --git a/services/web/app/src/Features/Docstore/DocstoreManager.js b/services/web/app/src/Features/Docstore/DocstoreManager.js index 6811bfc770..a141084d35 100644 --- a/services/web/app/src/Features/Docstore/DocstoreManager.js +++ b/services/web/app/src/Features/Docstore/DocstoreManager.js @@ -169,6 +169,35 @@ const DocstoreManager = { ) }, + isDocDeleted(project_id, doc_id, callback) { + const url = `${settings.apis.docstore.url}/project/${project_id}/doc/${doc_id}/deleted` + request.get({ url, timeout: TIMEOUT, json: true }, function( + err, + res, + body + ) { + if (err) { + callback(err) + } else if (res.statusCode === 200) { + callback(null, body.deleted) + } else if (res.statusCode === 404) { + callback( + new Errors.NotFoundError({ + message: 'doc does not exist in project', + info: { project_id, doc_id } + }) + ) + } else { + callback( + new OError( + `docstore api responded with non-success code: ${res.statusCode}`, + { project_id, doc_id } + ) + ) + } + }) + }, + updateDoc(project_id, doc_id, lines, version, ranges, callback) { if (callback == null) { callback = function(error, modified, rev) {} diff --git a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js index 5ce7c565aa..b51fef4ae7 100644 --- a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js @@ -70,6 +70,86 @@ function wrapWithLock(methodWithoutLock) { } } +function getDocContext(projectId, docId, callback) { + ProjectGetter.getProject( + projectId, + { name: true, rootFolder: true }, + (err, project) => { + if (err) { + return callback( + OError.tag(err, 'error fetching project', { + projectId + }) + ) + } + if (!project) { + return callback(new Errors.NotFoundError('project not found')) + } + ProjectLocator.findElement( + { project, element_id: docId, type: 'docs' }, + (err, doc, path) => { + if (err && err instanceof Errors.NotFoundError) { + // (Soft-)Deleted docs are removed from the file-tree (rootFolder). + // docstore can tell whether it exists and is (soft)-deleted. + DocstoreManager.isDocDeleted( + projectId, + docId, + (err, isDeletedDoc) => { + if (err && err instanceof Errors.NotFoundError) { + logger.warn( + { projectId, docId }, + 'doc not found while updating doc lines' + ) + callback(err) + } else if (err) { + callback( + OError.tag( + err, + 'error checking deletion status with docstore', + { projectId, docId } + ) + ) + } else { + if (!isDeletedDoc) { + // NOTE: This can happen while we delete a doc: + // 1. web will update the projects entry + // 2. web triggers flushes to tpds/doc-updater + // 3. web triggers (soft)-delete in docstore + // Specifically when an update comes in after 1 + // and before 3 completes. + logger.info( + { projectId, docId }, + 'updating doc that is in process of getting soft-deleted' + ) + } + callback(null, { + projectName: project.name, + isDeletedDoc: true, + path: null + }) + } + } + ) + } else if (err) { + callback( + OError.tag(err, 'error finding doc in rootFolder', { + docId, + projectId + }) + ) + } else { + callback(null, { + projectName: project.name, + isDeletedDoc: false, + path: path.fileSystem + }) + } + } + ) + } + ) +} + const ProjectEntityUpdateHandler = { updateDocLines( projectId, @@ -81,86 +161,57 @@ const ProjectEntityUpdateHandler = { lastUpdatedBy, callback ) { - ProjectGetter.getProjectWithoutDocLines(projectId, (err, project) => { - if (err != null) { + getDocContext(projectId, docId, (err, ctx) => { + if (err && err instanceof Errors.NotFoundError) { + // Do not allow an update to a doc which has never exist on this project + logger.warn( + { docId, projectId }, + 'project or doc not found while updating doc lines' + ) return callback(err) } - if (project == null) { - return callback(new Errors.NotFoundError('project not found')) + if (err) { + return callback(err) } - logger.log({ projectId, docId }, 'updating doc lines') - ProjectLocator.findElement( - { project, element_id: docId, type: 'docs' }, - (err, doc, path) => { - let isDeletedDoc = false + const { projectName, isDeletedDoc, path } = ctx + logger.log({ projectId, docId }, 'telling docstore manager to update doc') + DocstoreManager.updateDoc( + projectId, + docId, + lines, + version, + ranges, + (err, modified, rev) => { if (err != null) { - if (err instanceof Errors.NotFoundError) { - // We need to be able to update the doclines of deleted docs. This is - // so the doc-updater can flush a doc's content to the doc-store after - // the doc is deleted. - isDeletedDoc = true - doc = _.find( - project.deletedDocs, - doc => doc._id.toString() === docId.toString() - ) - } else { - return callback(err) - } + OError.tag(err, 'error sending doc to docstore', { + docId, + projectId + }) + return callback(err) } - - if (doc == null) { - // Do not allow an update to a doc which has never exist on this project - logger.warn( - { docId, projectId }, - 'doc not found while updating doc lines' - ) - return callback(new Errors.NotFoundError('doc not found')) - } - logger.log( - { projectId, docId }, - 'telling docstore manager to update doc' + { projectId, docId, modified }, + 'finished updating doc lines' ) - DocstoreManager.updateDoc( + // path will only be present if the doc is not deleted + if (!modified || isDeletedDoc) { + return callback() + } + // Don't need to block for marking as updated + ProjectUpdateHandler.markAsUpdated( projectId, - docId, - lines, - version, - ranges, - (err, modified, rev) => { - if (err != null) { - OError.tag(err, 'error sending doc to docstore', { - docId, - projectId - }) - return callback(err) - } - logger.log( - { projectId, docId, modified }, - 'finished updating doc lines' - ) - // path will only be present if the doc is not deleted - if (modified && !isDeletedDoc) { - // Don't need to block for marking as updated - ProjectUpdateHandler.markAsUpdated( - projectId, - lastUpdatedAt, - lastUpdatedBy - ) - TpdsUpdateSender.addDoc( - { - project_id: projectId, - path: path.fileSystem, - doc_id: docId, - project_name: project.name, - rev - }, - callback - ) - } else { - callback() - } - } + lastUpdatedAt, + lastUpdatedBy + ) + TpdsUpdateSender.addDoc( + { + project_id: projectId, + path, + doc_id: docId, + project_name: projectName, + rev + }, + callback ) } ) diff --git a/services/web/test/acceptance/src/DocUpdateTests.js b/services/web/test/acceptance/src/DocUpdateTests.js new file mode 100644 index 0000000000..7dcf2cff78 --- /dev/null +++ b/services/web/test/acceptance/src/DocUpdateTests.js @@ -0,0 +1,158 @@ +const User = require('./helpers/User') +const request = require('./helpers/request') +const { expect } = require('chai') +const settings = require('settings-sharelatex') +const { ObjectId } = require('mongodb') +require('./helpers/MockDocstoreApi') +require('./helpers/MockV1Api') +require('./helpers/MockProjectHistoryApi') + +describe('DocUpdate', function() { + beforeEach(function(done) { + this.user = new User() + this.projectName = 'wombat' + this.user.ensureUserExists(() => { + this.user.login(() => { + this.user.createProject(this.projectName, (error, projectId) => { + if (error) return done(error) + this.projectId = projectId + + this.user.getProject(this.projectId, (error, project) => { + if (error) return done(error) + this.project = project + this.user.createDocInProject( + this.projectId, + this.project.rootFolder[0]._id, + 'potato', + (error, docId) => { + this.docId = docId + done(error) + } + ) + }) + }) + }) + }) + }) + + function writeContent( + { projectId, docId, lines, version, ranges, lastUpdatedAt, lastUpdatedBy }, + callback + ) { + request( + { + method: 'POST', + url: `/project/${projectId}/doc/${docId}`, + auth: { + user: settings.apis.web.user, + pass: settings.apis.web.pass, + sendImmediately: true + }, + json: { lines, version, ranges, lastUpdatedAt, lastUpdatedBy } + }, + (error, res) => { + if (error) return callback(error) + if (res.statusCode !== 200) + return callback( + new Error(`non-success statusCode: ${res.statusCode}`) + ) + callback() + } + ) + } + + function updateContent(options, callback) { + writeContent(options, err => { + if (err) return callback(err) + + options.lines.push('foo') + options.version++ + writeContent(options, callback) + }) + } + + function writeContentTwice(options, callback) { + writeContent(options, err => { + if (err) return callback(err) + + writeContent(options, callback) + }) + } + + let writeOptions + beforeEach(function() { + writeOptions = { + projectId: this.projectId, + docId: this.docId, + lines: ['a'], + version: 1, + ranges: {}, + lastUpdatedAt: new Date(), + lastUpdatedBy: this.user.id + } + }) + + function shouldAcceptChanges() { + it('should accept writes', function(done) { + writeContent(writeOptions, done) + }) + + it('should accept updates', function(done) { + updateContent(writeOptions, done) + }) + + it('should accept same write twice', function(done) { + writeContentTwice(writeOptions, done) + }) + } + + function shouldBlockChanges() { + it('should block writes', function(done) { + writeContent(writeOptions, err => { + expect(err).to.exist + expect(err.message).to.equal('non-success statusCode: 404') + done() + }) + }) + + it('should block updates', function(done) { + updateContent(writeOptions, err => { + expect(err).to.exist + expect(err.message).to.equal('non-success statusCode: 404') + done() + }) + }) + } + + describe('a regular doc', function() { + shouldAcceptChanges() + }) + + describe('after deleting the doc', function() { + beforeEach(function(done) { + this.user.deleteItemInProject(this.projectId, 'doc', this.docId, done) + }) + + shouldAcceptChanges() + }) + + describe('unknown doc', function() { + beforeEach(function() { + writeOptions.docId = ObjectId() + }) + + shouldBlockChanges() + }) + + describe('doc in another project', function() { + beforeEach(function(done) { + this.user.createProject('foo', (error, projectId) => { + if (error) return done(error) + writeOptions.projectId = projectId + done() + }) + }) + + shouldBlockChanges() + }) +}) diff --git a/services/web/test/acceptance/src/helpers/MockDocstoreApi.js b/services/web/test/acceptance/src/helpers/MockDocstoreApi.js index 5e7f980a9c..fdc8bb2be6 100644 --- a/services/web/test/acceptance/src/helpers/MockDocstoreApi.js +++ b/services/web/test/acceptance/src/helpers/MockDocstoreApi.js @@ -29,14 +29,18 @@ module.exports = MockDocStoreApi = { if (this.docs[project_id] == null) { this.docs[project_id] = {} } - this.docs[project_id][doc_id] = { lines, version, ranges } + if (this.docs[project_id][doc_id] == null) { + this.docs[project_id][doc_id] = {} + } + const { version: oldVersion, deleted } = this.docs[project_id][doc_id] + this.docs[project_id][doc_id] = { lines, version, ranges, deleted } if (this.docs[project_id][doc_id].rev == null) { this.docs[project_id][doc_id].rev = 0 } this.docs[project_id][doc_id].rev += 1 this.docs[project_id][doc_id]._id = doc_id return res.json({ - modified: true, + modified: oldVersion !== version, rev: this.docs[project_id][doc_id].rev }) } @@ -64,6 +68,15 @@ module.exports = MockDocStoreApi = { } }) + app.get('/project/:project_id/doc/:doc_id/deleted', (req, res) => { + const { project_id, doc_id } = req.params + if (!this.docs[project_id] || !this.docs[project_id][doc_id]) { + res.sendStatus(404) + } else { + res.json({ deleted: Boolean(this.docs[project_id][doc_id].deleted) }) + } + }) + app.delete('/project/:project_id/doc/:doc_id', (req, res, next) => { const { project_id, doc_id } = req.params if (this.docs[project_id] == null) { diff --git a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js index 4f98c6de7e..7e2e61994a 100644 --- a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js +++ b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js @@ -71,6 +71,7 @@ describe('ProjectEntityUpdateHandler', function() { this.DocstoreManager = { getDoc: sinon.stub(), + isDocDeleted: sinon.stub(), updateDoc: sinon.stub(), deleteDoc: sinon.stub() } @@ -82,6 +83,7 @@ describe('ProjectEntityUpdateHandler', function() { deleteDoc: sinon.stub().yields() } this.logger = { + info: sinon.stub(), log: sinon.stub(), warn: sinon.stub(), error: sinon.stub(), @@ -188,7 +190,8 @@ describe('ProjectEntityUpdateHandler', function() { this.ranges = { mock: 'ranges' } this.lastUpdatedAt = new Date().getTime() this.lastUpdatedBy = 'fake-last-updater-id' - this.ProjectGetter.getProjectWithoutDocLines.yields(null, this.project) + this.DocstoreManager.isDocDeleted.yields(null, false) + this.ProjectGetter.getProject.yields(null, this.project) this.ProjectLocator.findElement.yields(null, this.doc, { fileSystem: this.path }) @@ -210,9 +213,12 @@ describe('ProjectEntityUpdateHandler', function() { ) }) - it('should get the project without doc lines', function() { - this.ProjectGetter.getProjectWithoutDocLines - .calledWith(projectId) + it('should get the project with very few fields', function() { + this.ProjectGetter.getProject + .calledWith(projectId, { + name: true, + rootFolder: true + }) .should.equal(true) }) @@ -294,9 +300,56 @@ describe('ProjectEntityUpdateHandler', function() { describe('when the doc has been deleted', function() { beforeEach(function() { - this.project.deletedDocs = [{ _id: docId }] - this.ProjectGetter.getProjectWithoutDocLines.yields(null, this.project) + this.ProjectGetter.getProject.yields(null, this.project) this.ProjectLocator.findElement.yields(new Errors.NotFoundError()) + this.DocstoreManager.isDocDeleted.yields(null, true) + this.DocstoreManager.updateDoc.yields() + this.ProjectEntityUpdateHandler.updateDocLines( + projectId, + docId, + this.docLines, + this.version, + this.ranges, + this.lastUpdatedAt, + this.lastUpdatedBy, + this.callback + ) + }) + + it('should update the doc in the docstore', function() { + this.DocstoreManager.updateDoc + .calledWith( + projectId, + docId, + this.docLines, + this.version, + this.ranges + ) + .should.equal(true) + }) + + it('should not mark the project as updated', function() { + this.ProjectUpdater.markAsUpdated.called.should.equal(false) + }) + + it('should not send the doc the to the TPDS', function() { + this.TpdsUpdateSender.addDoc.called.should.equal(false) + }) + + it('should call the callback', function() { + this.callback.called.should.equal(true) + }) + }) + + describe('when projects and docs collection are de-synced', function() { + beforeEach(function() { + this.ProjectGetter.getProject.yields(null, this.project) + + // The doc is not in the file-tree, but also not marked as deleted. + // This should not happen, but web should handle it. + this.ProjectLocator.findElement.yields(new Errors.NotFoundError()) + this.DocstoreManager.isDocDeleted.yields(null, false) + this.DocstoreManager.updateDoc.yields() this.ProjectEntityUpdateHandler.updateDocLines( projectId, @@ -337,7 +390,9 @@ describe('ProjectEntityUpdateHandler', function() { describe('when the doc is not related to the project', function() { beforeEach(function() { - this.ProjectLocator.findElement.yields() + this.ProjectGetter.getProject.yields(null, this.project) + this.ProjectLocator.findElement.yields(new Errors.NotFoundError()) + this.DocstoreManager.isDocDeleted.yields(new Errors.NotFoundError()) this.ProjectEntityUpdateHandler.updateDocLines( projectId, docId, @@ -355,11 +410,19 @@ describe('ProjectEntityUpdateHandler', function() { .calledWith(sinon.match.instanceOf(Errors.NotFoundError)) .should.equal(true) }) + + it('should not update the doc', function() { + this.DocstoreManager.updateDoc.called.should.equal(false) + }) + + it('should not send the doc the to the TPDS', function() { + this.TpdsUpdateSender.addDoc.called.should.equal(false) + }) }) describe('when the project is not found', function() { beforeEach(function() { - this.ProjectGetter.getProjectWithoutDocLines.yields() + this.ProjectGetter.getProject.yields(new Errors.NotFoundError()) this.ProjectEntityUpdateHandler.updateDocLines( projectId, docId, @@ -377,6 +440,14 @@ describe('ProjectEntityUpdateHandler', function() { .calledWith(sinon.match.instanceOf(Errors.NotFoundError)) .should.equal(true) }) + + it('should not update the doc', function() { + this.DocstoreManager.updateDoc.called.should.equal(false) + }) + + it('should not send the doc the to the TPDS', function() { + this.TpdsUpdateSender.addDoc.called.should.equal(false) + }) }) })