diff --git a/services/web/app/src/Features/Editor/EditorController.js b/services/web/app/src/Features/Editor/EditorController.js index 9952f60c3e..6a3b346a56 100644 --- a/services/web/app/src/Features/Editor/EditorController.js +++ b/services/web/app/src/Features/Editor/EditorController.js @@ -213,7 +213,7 @@ const EditorController = { userId ) } - callback() + callback(null, doc) } ) } @@ -273,7 +273,7 @@ const EditorController = { linkedFileData, userId ) - callback() + callback(null, newFile) } ) } diff --git a/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js index 99a21234da..2585c435d6 100644 --- a/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js @@ -43,6 +43,7 @@ module.exports = { 'project', 'path', 'newProject', + 'newFileRef', ]), replaceDocWithFile: callbackify(replaceDocWithFile), replaceFileWithDoc: callbackify(replaceFileWithDoc), @@ -188,7 +189,9 @@ async function replaceFileWithNew(projectId, fileId, newFileRef) { path, }) } - return { oldFileRef: fileRef, project, path, newProject } + // Refresh newFileRef with the version returned from the database + newFileRef = ProjectLocator.findElementByMongoPath(newProject, path.mongo) + return { oldFileRef: fileRef, project, path, newProject, newFileRef } } async function replaceDocWithFile(projectId, docId, fileRef) { diff --git a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js index 81aeb9e1bd..e456218a1d 100644 --- a/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityUpdateHandler.js @@ -343,6 +343,7 @@ const ProjectEntityUpdateHandler = { if (err != null) { return callback(err) } + doc.rev = rev next( projectId, folderId, @@ -576,7 +577,7 @@ const ProjectEntityUpdateHandler = { projectId, fileId, newFileRef, - (err, oldFileRef, project, path, newProject) => { + (err, oldFileRef, project, path, newProject, newFileRef) => { if (err != null) { return callback(err) } @@ -597,18 +598,12 @@ const ProjectEntityUpdateHandler = { project.overleaf && project.overleaf.history && project.overleaf.history.id - // Increment the rev for an in-place update (with the same path) so the third-party-datastore - // knows this is a new file. - // Ideally we would get this from ProjectEntityMongoUpdateHandler.replaceFileWithNew - // but it returns the original oldFileRef (after incrementing the rev value in mongo), - // so we add 1 to the rev from that. This isn't atomic and relies on the lock - // but it is acceptable for now. TpdsUpdateSender.addFile( { projectId: project._id, fileId: newFileRef._id, path: path.fileSystem, - rev: oldFileRef.rev + 1, + rev: newFileRef.rev, projectName: project.name, folderId, }, @@ -624,7 +619,12 @@ const ProjectEntityUpdateHandler = { userId, { oldFiles, newFiles, newProject }, source, - callback + err => { + if (err) { + return callback(err) + } + callback(null, newFileRef) + } ) } ) @@ -670,6 +670,7 @@ const ProjectEntityUpdateHandler = { if (err != null) { return callback(err) } + doc.rev = rev ProjectEntityMongoUpdateHandler.replaceFileWithDoc( projectId, existingFile._id, @@ -936,7 +937,7 @@ const ProjectEntityUpdateHandler = { fileStoreUrl, folderId, source, - err => { + (err, newFileRef) => { if (err != null) { return callback(err) } diff --git a/services/web/app/src/Features/Project/ProjectLocator.js b/services/web/app/src/Features/Project/ProjectLocator.js index 237ab79dc7..b34aea9b10 100644 --- a/services/web/app/src/Features/Project/ProjectLocator.js +++ b/services/web/app/src/Features/Project/ProjectLocator.js @@ -1,5 +1,6 @@ const _ = require('underscore') const logger = require('@overleaf/logger') +const OError = require('@overleaf/o-error') const async = require('async') const ProjectGetter = require('./ProjectGetter') const Errors = require('../Errors/Errors') @@ -282,10 +283,31 @@ function getIndexOf(searchEntity, id) { } } +/** + * Follow the given Mongo path (as returned by findElement) and return the + * entity at the end of it. + */ +function findElementByMongoPath(project, mongoPath) { + const components = mongoPath.split('.') + let node = project + for (const component of components) { + const key = Array.isArray(node) ? parseInt(component, 10) : component + node = node[key] + if (node == null) { + throw new OError('entity not found', { + projectId: project._id, + mongoPath, + }) + } + } + return node +} + module.exports = { findElement, findElementByPath, findRootDoc, + findElementByMongoPath, promises: { findElement: promisifyMultiResult(findElement, [ 'element', diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js index 2cd7782847..ad105df8c2 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js @@ -9,17 +9,19 @@ const NotificationsBuilder = require('../Notifications/NotificationsBuilder') const SessionManager = require('../Authentication/SessionManager') const TpdsQueueManager = require('./TpdsQueueManager') -// mergeUpdate and deleteUpdate are used by Dropbox, where the project is only passed as the name, as the -// first part of the file path. They have to check the project exists, find it, and create it if not. -// They also ignore 'noisy' files like .DS_Store, .gitignore, etc. +// mergeUpdate and deleteUpdate are used by Dropbox, where the project is only +// passed as the name, as the first part of the file path. They have to check +// the project exists, find it, and create it if not. They also ignore 'noisy' +// files like .DS_Store, .gitignore, etc. async function mergeUpdate(req, res) { metrics.inc('tpds.merge-update') const { filePath, userId, projectName } = parseParams(req) const source = req.headers['x-sl-update-source'] || 'unknown' + let metadata try { - await TpdsUpdateHandler.promises.newUpdate( + metadata = await TpdsUpdateHandler.promises.newUpdate( userId, projectName, filePath, @@ -45,7 +47,19 @@ async function mergeUpdate(req, res) { } } - res.sendStatus(200) + if (metadata == null) { + return res.json({ status: 'rejected' }) + } + + const payload = { + status: 'applied', + entityId: metadata.entityId.toString(), + entityType: metadata.entityType, + } + if (metadata.rev != null) { + payload.rev = metadata.rev + } + res.json(payload) } async function deleteUpdate(req, res) { @@ -62,10 +76,11 @@ async function deleteUpdate(req, res) { res.sendStatus(200) } -// updateProjectContents and deleteProjectContents are used by GitHub. The project_id is known so we -// can skip right ahead to creating/updating/deleting the file. These methods will not ignore noisy -// files like .DS_Store, .gitignore, etc because people are generally more explicit with the files they -// want in git. +// updateProjectContents and deleteProjectContents are used by GitHub. The +// project_id is known so we can skip right ahead to creating/updating/deleting +// the file. These methods will not ignore noisy files like .DS_Store, +// .gitignore, etc because people are generally more explicit with the files +// they want in git. async function updateProjectContents(req, res, next) { const projectId = req.params.project_id diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js index 072ea8d93e..40633f1d56 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js @@ -22,7 +22,7 @@ const rootDocResets = new BackgroundTaskTracker('root doc resets') async function newUpdate(userId, projectName, path, updateRequest, source) { const project = await getOrCreateProject(userId, projectName) if (project == null) { - return + return null } const projectIsOnCooldown = @@ -33,16 +33,17 @@ async function newUpdate(userId, projectName, path, updateRequest, source) { const shouldIgnore = await FileTypeManager.promises.shouldIgnore(path) if (shouldIgnore) { - return + return null } - await UpdateMerger.promises.mergeUpdate( + const metadata = await UpdateMerger.promises.mergeUpdate( userId, project._id, path, updateRequest, source ) + return metadata } async function deleteUpdate(userId, projectName, path, source) { diff --git a/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js b/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js index 68b454d5d9..7ade1cb230 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js @@ -13,7 +13,8 @@ async function mergeUpdate(userId, projectId, path, updateRequest, source) { updateRequest ) try { - await _mergeUpdate(userId, projectId, path, fsPath, source) + const metadata = await _mergeUpdate(userId, projectId, path, fsPath, source) + return metadata } finally { try { await fsPromises.unlink(fsPath) @@ -52,54 +53,37 @@ async function _determineFileType(projectId, path, fsPath) { // sync, so we'll treat non-utf8 files as binary const isBinary = binary || encoding !== 'utf-8' - // Existing | Update | Action - // ---------|-----------|------- - // file | isBinary | existing-file - // file | !isBinary | existing-file - // doc | isBinary | new-file, delete-existing-doc - // doc | !isBinary | existing-doc - // null | isBinary | new-file - // null | !isBinary | new-doc + // Existing | Update | Resulting file type + // ---------|-----------|-------------------- + // file | isBinary | file + // file | !isBinary | file + // doc | isBinary | file + // doc | !isBinary | doc + // null | isBinary | file + // null | !isBinary | doc // if a binary file already exists, always keep it as a binary file // even if the update looks like a text file if (existingFileType === 'file') { - return { fileType: 'existing-file' } + return 'file' + } else { + return isBinary ? 'file' : 'doc' } - - // if there is an existing doc, keep it as a doc except when the - // incoming update is binary. In that case delete the doc and replace - // it with a new file. - if (existingFileType === 'doc') { - if (isBinary) { - return { - fileType: 'new-file', - deleteOriginalEntity: 'delete-existing-doc', - } - } else { - return { fileType: 'existing-doc' } - } - } - - // if there no existing file, create a file or doc as needed - return { fileType: isBinary ? 'new-file' : 'new-doc' } } async function _mergeUpdate(userId, projectId, path, fsPath, source) { - const { fileType, deleteOriginalEntity } = await _determineFileType( - projectId, - path, - fsPath - ) + const fileType = await _determineFileType(projectId, path, fsPath) - if (deleteOriginalEntity) { - await deleteUpdate(userId, projectId, path, source) - } - - if (['existing-file', 'new-file'].includes(fileType)) { - await _processFile(projectId, fsPath, path, source, userId) - } else if (['existing-doc', 'new-doc'].includes(fileType)) { - await _processDoc(projectId, userId, fsPath, path, source) + if (fileType === 'file') { + const file = await _processFile(projectId, fsPath, path, source, userId) + return { entityType: 'file', entityId: file._id, rev: file.rev } + } else if (fileType === 'doc') { + const doc = await _processDoc(projectId, userId, fsPath, path, source) + // The doc entry doesn't have a rev. Since the document is set in + // docupdater, it's possible that the next rev contains a merge of changes + // in Dropbox and changes from docupdater. + const metadata = { entityType: 'doc', entityId: doc._id, rev: doc.rev } + return metadata } else { throw new Error('unrecognized file') } @@ -124,17 +108,18 @@ async function deleteUpdate(userId, projectId, path, source) { async function _processDoc(projectId, userId, fsPath, path, source) { const docLines = await _readFileIntoTextArray(fsPath) logger.debug({ docLines }, 'processing doc update from tpds') - await EditorController.promises.upsertDocWithPath( + const doc = await EditorController.promises.upsertDocWithPath( projectId, path, docLines, source, userId ) + return doc } async function _processFile(projectId, fsPath, path, source, userId) { - await EditorController.promises.upsertFileWithPath( + const file = await EditorController.promises.upsertFileWithPath( projectId, path, fsPath, @@ -142,6 +127,7 @@ async function _processFile(projectId, fsPath, path, source, userId) { source, userId ) + return file } async function _readFileIntoTextArray(path) { diff --git a/services/web/test/acceptance/src/TpdsUpdateTests.js b/services/web/test/acceptance/src/TpdsUpdateTests.js index aa1b7aa4ce..998ef0004a 100644 --- a/services/web/test/acceptance/src/TpdsUpdateTests.js +++ b/services/web/test/acceptance/src/TpdsUpdateTests.js @@ -1,16 +1,3 @@ -/* eslint-disable - camelcase, - max-len, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const { expect } = require('chai') const ProjectGetter = require('../../../app/src/Features/Project/ProjectGetter.js') const request = require('./helpers/request') @@ -19,19 +6,19 @@ const User = require('./helpers/User') describe('TpdsUpdateTests', function () { beforeEach(function (done) { this.owner = new User() - return this.owner.login(error => { - if (error != null) { + this.owner.login(error => { + if (error) { throw error } - return this.owner.createProject( + this.owner.createProject( 'test-project', { template: 'example' }, - (error, project_id) => { - if (error != null) { + (error, projectId) => { + if (error) { throw error } - this.project_id = project_id - return done() + this.projectId = projectId + done() } ) }) @@ -39,10 +26,10 @@ describe('TpdsUpdateTests', function () { describe('adding a file', function () { beforeEach(function (done) { - return request( + request( { method: 'POST', - url: `/project/${this.project_id}/contents/test.tex`, + url: `/project/${this.projectId}/contents/test.tex`, auth: { username: 'sharelatex', password: 'password', @@ -51,34 +38,34 @@ describe('TpdsUpdateTests', function () { body: 'test one two', }, (error, response, body) => { - if (error != null) { + if (error) { throw error } expect(response.statusCode).to.equal(200) - return done() + done() } ) }) it('should have added the file', function (done) { - return ProjectGetter.getProject(this.project_id, (error, project) => { - if (error != null) { + ProjectGetter.getProject(this.projectId, (error, project) => { + if (error) { throw error } const projectFolder = project.rootFolder[0] const file = projectFolder.docs.find(e => e.name === 'test.tex') expect(file).to.exist - return done() + done() }) }) }) describe('deleting a file', function () { beforeEach(function (done) { - return request( + request( { method: 'DELETE', - url: `/project/${this.project_id}/contents/main.tex`, + url: `/project/${this.projectId}/contents/main.tex`, auth: { username: 'sharelatex', password: 'password', @@ -86,34 +73,34 @@ describe('TpdsUpdateTests', function () { }, }, (error, response, body) => { - if (error != null) { + if (error) { throw error } expect(response.statusCode).to.equal(200) - return done() + done() } ) }) it('should have deleted the file', function (done) { - return ProjectGetter.getProject(this.project_id, (error, project) => { - if (error != null) { + ProjectGetter.getProject(this.projectId, (error, project) => { + if (error) { throw error } const projectFolder = project.rootFolder[0] - for (const doc of Array.from(projectFolder.docs)) { + for (const doc of projectFolder.docs) { if (doc.name === 'main.tex') { throw new Error('expected main.tex to have been deleted') } } - return done() + done() }) }) }) describe('update a new file', function () { beforeEach(function (done) { - return request( + request( { method: 'POST', url: `/user/${this.owner._id}/update/test-project/other.tex`, @@ -125,24 +112,29 @@ describe('TpdsUpdateTests', function () { body: 'test one two', }, (error, response, body) => { - if (error != null) { + if (error) { throw error } expect(response.statusCode).to.equal(200) - return done() + const json = JSON.parse(response.body) + expect(json.status).to.equal('applied') + expect(json.entityType).to.equal('doc') + expect(json).to.have.property('entityId') + expect(json).to.have.property('rev') + done() } ) }) it('should have added the file', function (done) { - return ProjectGetter.getProject(this.project_id, (error, project) => { - if (error != null) { + ProjectGetter.getProject(this.projectId, (error, project) => { + if (error) { throw error } const projectFolder = project.rootFolder[0] const file = projectFolder.docs.find(e => e.name === 'other.tex') expect(file).to.exist - return done() + done() }) }) }) @@ -151,12 +143,12 @@ describe('TpdsUpdateTests', function () { beforeEach(function (done) { this.owner.request( { - url: `/Project/${this.project_id}/archive`, + url: `/Project/${this.projectId}/archive`, method: 'post', }, (err, response, body) => { expect(err).to.not.exist - return request( + request( { method: 'POST', url: `/user/${this.owner._id}/update/test-project/test.tex`, @@ -168,11 +160,13 @@ describe('TpdsUpdateTests', function () { body: 'test one two', }, (error, response, body) => { - if (error != null) { + if (error) { throw error } expect(response.statusCode).to.equal(200) - return done() + const json = JSON.parse(response.body) + expect(json.status).to.equal('rejected') + done() } ) } @@ -192,14 +186,14 @@ describe('TpdsUpdateTests', function () { }) it('should not have added the file', function (done) { - return ProjectGetter.getProject(this.project_id, (error, project) => { - if (error != null) { + ProjectGetter.getProject(this.projectId, (error, project) => { + if (error) { throw error } const projectFolder = project.rootFolder[0] const file = projectFolder.docs.find(e => e.name === 'test.tex') expect(file).to.not.exist - return done() + done() }) }) }) diff --git a/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js index d5ade92860..5013f03e48 100644 --- a/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js +++ b/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js @@ -77,6 +77,7 @@ describe('ProjectEntityMongoUpdateHandler', function () { getAllEntitiesFromProject: sinon.stub(), } this.ProjectLocator = { + findElementByMongoPath: sinon.stub().throws(new Error('not found')), promises: { findElement: sinon.stub().rejects(new Error('not found')), findElementByPath: sinon.stub().rejects(new Error('not found')), @@ -388,6 +389,9 @@ describe('ProjectEntityMongoUpdateHandler', function () { ) .chain('exec') .resolves(this.project) + this.ProjectLocator.findElementByMongoPath + .withArgs(this.project, 'rootFolder.0.fileRefs.0') + .returns(newFile) await this.subject.promises.replaceFileWithNew( this.project._id, this.file._id, diff --git a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js index d870d4c6df..313c687a7b 100644 --- a/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js +++ b/services/web/test/unit/src/Project/ProjectEntityUpdateHandlerTests.js @@ -34,7 +34,7 @@ describe('ProjectEntityUpdateHandler', function () { this.name = options.name this.lines = options.lines this._id = docId - this.rev = 0 + this.rev = options.rev ?? 0 } } this.FileModel = class File { @@ -516,14 +516,15 @@ describe('ProjectEntityUpdateHandler', function () { describe('adding a doc', function () { beforeEach(function () { this.path = '/path/to/doc' + this.rev = 5 this.newDoc = new this.DocModel({ name: this.docName, lines: undefined, _id: docId, - rev: 0, + rev: this.rev, }) - this.DocstoreManager.updateDoc.yields(null, false, (this.rev = 5)) + this.DocstoreManager.updateDoc.yields(null, false, this.rev) this.TpdsUpdateSender.addDoc.yields() this.ProjectEntityMongoUpdateHandler.addDoc.yields( null, @@ -555,18 +556,16 @@ describe('ProjectEntityUpdateHandler', function () { docLines: this.docLines.join('\n'), }, ] - this.DocumentUpdaterHandler.updateProjectStructure - .calledWith( - projectId, - projectHistoryId, - userId, - { - newDocs, - newProject: this.project, - }, - this.source - ) - .should.equal(true) + this.DocumentUpdaterHandler.updateProjectStructure.should.have.been.calledWith( + projectId, + projectHistoryId, + userId, + { + newDocs, + newProject: this.project, + }, + this.source + ) }) }) @@ -1020,6 +1019,7 @@ describe('ProjectEntityUpdateHandler', function () { describe('updating an existing file', function () { beforeEach(function () { this.existingFile = { _id: fileId, name: this.fileName, rev: 1 } + this.newFile = { _id: ObjectId(), name: this.fileName, rev: 3 } this.folder = { _id: folderId, fileRefs: [this.existingFile], docs: [] } this.ProjectLocator.findElement.yields(null, this.folder) this.newProject = 'new-project-stub' @@ -1028,7 +1028,8 @@ describe('ProjectEntityUpdateHandler', function () { this.existingFile, this.project, { fileSystem: this.fileSystemPath }, - this.newProject + this.newProject, + this.newFile ) this.ProjectEntityUpdateHandler.upsertFile( projectId, @@ -1065,8 +1066,8 @@ describe('ProjectEntityUpdateHandler', function () { this.TpdsUpdateSender.addFile.should.have.been.calledWith({ projectId, projectName: this.project.name, - fileId: this.file._id, - rev: this.existingFile.rev + 1, + fileId: this.newFile._id, + rev: this.newFile.rev, path: this.fileSystemPath, folderId, }) @@ -1088,7 +1089,7 @@ describe('ProjectEntityUpdateHandler', function () { ] const newFiles = [ { - file: this.file, + file: this.newFile, path: this.fileSystemPath, url: this.fileUrl, }, diff --git a/services/web/test/unit/src/Project/ProjectLocatorTests.js b/services/web/test/unit/src/Project/ProjectLocatorTests.js index d460e74a29..40e710deee 100644 --- a/services/web/test/unit/src/Project/ProjectLocatorTests.js +++ b/services/web/test/unit/src/Project/ProjectLocatorTests.js @@ -557,4 +557,23 @@ describe('ProjectLocator', function () { }) }) }) + + describe('findElementByMongoPath', function () { + it('traverses the file tree like Mongo would do', function () { + const element = this.locator.findElementByMongoPath( + project, + 'rootFolder.0.folders.1.folders.0.fileRefs.0' + ) + expect(element).to.equal(subSubFile) + }) + + it('throws an error if no element is found', function () { + expect(() => + this.locator.findElementByMongoPath( + project, + 'rootolder.0.folders.0.folders.0.fileRefs.0' + ) + ).to.throw + }) + }) }) diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js index 92c2ea6cf2..9484443dfe 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js @@ -1,6 +1,7 @@ const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') const { expect } = require('chai') +const { ObjectId } = require('mongodb') const Errors = require('../../../../app/src/Features/Errors/Errors') const MODULE_PATH = @@ -8,15 +9,20 @@ const MODULE_PATH = describe('TpdsController', function () { beforeEach(function () { + this.metadata = { + entityId: ObjectId(), + entityType: 'doc', + rev: 2, + } this.TpdsUpdateHandler = { promises: { - newUpdate: sinon.stub().resolves(), + newUpdate: sinon.stub().resolves(this.metadata), deleteUpdate: sinon.stub().resolves(), }, } this.UpdateMerger = { promises: { - mergeUpdate: sinon.stub().resolves(), + mergeUpdate: sinon.stub().resolves(this.file), deleteUpdate: sinon.stub().resolves(), }, } @@ -46,70 +52,65 @@ describe('TpdsController', function () { }) describe('getting an update', function () { - it('should process the update with the update receiver', function (done) { - const path = '/projectName/here.txt' - const req = { - pause() {}, - params: { 0: path, user_id: this.user_id }, - session: { - destroy() {}, - }, + beforeEach(function () { + this.projectName = 'projectName' + this.path = '/here.txt' + this.req = { + params: { 0: `${this.projectName}${this.path}`, user_id: this.user_id }, headers: { 'x-sl-update-source': (this.source = 'dropbox'), }, } + }) + + it('should process the update with the update receiver', function (done) { const res = { - sendStatus: () => { + json: payload => { + expect(payload).to.deep.equal({ + status: 'applied', + entityId: this.metadata.entityId.toString(), + entityType: this.metadata.entityType, + rev: this.metadata.rev, + }) this.TpdsUpdateHandler.promises.newUpdate .calledWith( this.user_id, - 'projectName', - '/here.txt', - req, + this.projectName, + this.path, + this.req, this.source ) .should.equal(true) done() }, } - this.TpdsController.mergeUpdate(req, res) + this.TpdsController.mergeUpdate(this.req, res) + }) + + it('should indicate in the response when the update was rejected', function (done) { + this.TpdsUpdateHandler.promises.newUpdate.resolves(null) + const res = { + json: payload => { + expect(payload).to.deep.equal({ status: 'rejected' }) + done() + }, + } + this.TpdsController.mergeUpdate(this.req, res) }) it('should return a 500 error when the update receiver fails', function (done) { - const path = '/projectName/here.txt' - const req = { - pause() {}, - params: { 0: path, user_id: this.user_id }, - session: { - destroy() {}, - }, - headers: { - 'x-sl-update-source': (this.source = 'dropbox'), - }, - } this.TpdsUpdateHandler.promises.newUpdate.rejects(new Error()) const res = { - sendStatus: sinon.stub(), + json: sinon.stub(), } - this.TpdsController.mergeUpdate(req, res, err => { + this.TpdsController.mergeUpdate(this.req, res, err => { expect(err).to.exist - expect(res.sendStatus).not.to.have.been.called + expect(res.json).not.to.have.been.called done() }) }) it('should return a 400 error when the project is too big', function (done) { - const path = '/projectName/here.txt' - const req = { - pause() {}, - params: { 0: path, user_id: this.user_id, projectName: 'projectName' }, - session: { - destroy() {}, - }, - headers: { - 'x-sl-update-source': (this.source = 'dropbox'), - }, - } this.TpdsUpdateHandler.promises.newUpdate.rejects({ message: 'project_has_too_many_files', }) @@ -122,21 +123,10 @@ describe('TpdsController', function () { done() }, } - this.TpdsController.mergeUpdate(req, res) + this.TpdsController.mergeUpdate(this.req, res) }) it('should return a 429 error when the update receiver fails due to too many requests error', function (done) { - const path = '/projectName/here.txt' - const req = { - pause() {}, - params: { 0: path, user_id: this.user_id }, - session: { - destroy() {}, - }, - headers: { - 'x-sl-update-source': (this.source = 'dropbox'), - }, - } this.TpdsUpdateHandler.promises.newUpdate.rejects( new Errors.TooManyRequestsError('project on cooldown') ) @@ -146,7 +136,7 @@ describe('TpdsController', function () { done() }, } - this.TpdsController.mergeUpdate(req, res) + this.TpdsController.mergeUpdate(this.req, res) }) }) diff --git a/services/web/test/unit/src/ThirdPartyDataStore/UpdateMergerTests.js b/services/web/test/unit/src/ThirdPartyDataStore/UpdateMergerTests.js index be696b738d..903406b5c8 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/UpdateMergerTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/UpdateMergerTests.js @@ -2,6 +2,7 @@ const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') const { expect } = require('chai') const BufferedStream = require('bufferedstream') +const { ObjectId } = require('mongodb') const MODULE_PATH = '../../../../app/src/Features/ThirdPartyDataStore/UpdateMerger.js' @@ -38,11 +39,21 @@ describe('UpdateMerger :', function () { readFile: sinon.stub().withArgs(this.fsPath).resolves(this.fileContents), } + this.doc = { + _id: ObjectId(), + rev: 2, + } + + this.file = { + _id: ObjectId(), + rev: 6, + } + this.EditorController = { promises: { deleteEntityWithPath: sinon.stub().resolves(), - upsertDocWithPath: sinon.stub().resolves(), - upsertFileWithPath: sinon.stub().resolves(), + upsertDocWithPath: sinon.stub().resolves(this.doc), + upsertFileWithPath: sinon.stub().resolves(this.file), }, } @@ -239,17 +250,6 @@ describe('UpdateMerger :', function () { expect(this.FileTypeManager.promises.getType).to.have.been.called }) - it('should delete the existing doc', function () { - expect( - this.EditorController.promises.deleteEntityWithPath - ).to.have.been.calledWith( - this.projectId, - this.existingDocPath, - this.source, - this.userId - ) - }) - it('should process update as file', function () { expect( this.EditorController.promises.upsertFileWithPath