From bb24ec117fcae03f4e4ff8a5fef3ac234a060a41 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Wed, 21 Sep 2022 08:01:56 -0400 Subject: [PATCH] Merge pull request #9658 from overleaf/em-dropbox-folder-sync Sync folder creation from Dropbox to Overleaf GitOrigin-RevId: a2749ab8d9db7dd312818b46d6e72f1dbaaaff2e --- .../src/Features/Editor/EditorController.js | 6 +- .../ProjectEntityMongoUpdateHandler.js | 4 +- .../src/Features/Project/ProjectLocator.js | 7 +- .../ThirdPartyDataStore/TpdsController.js | 37 +++++++- .../ThirdPartyDataStore/TpdsUpdateHandler.js | 28 ++++++ .../ThirdPartyDataStore/UpdateMerger.js | 10 ++ services/web/app/src/router.js | 5 + .../ProjectEntityMongoUpdateHandlerTests.js | 18 ++-- .../unit/src/Project/ProjectLocatorTests.js | 24 +++-- .../TpdsControllerTests.js | 46 +++++++++ .../TpdsUpdateHandlerTests.js | 94 +++++++++++++++++++ 11 files changed, 257 insertions(+), 22 deletions(-) diff --git a/services/web/app/src/Features/Editor/EditorController.js b/services/web/app/src/Features/Editor/EditorController.js index adbbc38232..87dfe7f208 100644 --- a/services/web/app/src/Features/Editor/EditorController.js +++ b/services/web/app/src/Features/Editor/EditorController.js @@ -653,5 +653,9 @@ const EditorController = { }, } -EditorController.promises = promisifyAll(EditorController) +EditorController.promises = promisifyAll(EditorController, { + multiResult: { + mkdirp: ['newFolders', 'lastFolder'], + }, +}) module.exports = EditorController diff --git a/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js b/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js index 2585c435d6..249892841b 100644 --- a/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js +++ b/services/web/app/src/Features/Project/ProjectEntityMongoUpdateHandler.js @@ -50,6 +50,7 @@ module.exports = { mkdirp: callbackifyMultiResult(wrapWithLock(mkdirp), [ 'newFolders', 'folder', + 'parentFolder', ]), moveEntity: callbackifyMultiResult(wrapWithLock(moveEntity), [ 'project', @@ -279,13 +280,14 @@ async function mkdirp(projectId, path, options = {}) { for (const folderName of folders) { builtUpPath += `/${folderName}` try { - const { element: foundFolder } = + const { element: foundFolder, folder: parentFolder } = await ProjectLocator.promises.findElementByPath({ project, path: builtUpPath, exactCaseMatch: options.exactCaseMatch, }) lastFolder = foundFolder + lastFolder.parentFolder_id = parentFolder._id } catch (err) { // Folder couldn't be found. Create it. const parentFolderId = lastFolder && lastFolder._id diff --git a/services/web/app/src/Features/Project/ProjectLocator.js b/services/web/app/src/Features/Project/ProjectLocator.js index b34aea9b10..aad676e15a 100644 --- a/services/web/app/src/Features/Project/ProjectLocator.js +++ b/services/web/app/src/Features/Project/ProjectLocator.js @@ -203,7 +203,7 @@ function _findElementByPathWithProject( function getEntity(folder, entityName, cb) { let result, type if (entityName == null) { - return cb(null, folder, 'folder') + return cb(null, folder, 'folder', null) } for (const file of iterablePaths(folder, 'fileRefs')) { if (matchFn(file != null ? file.name : undefined, entityName)) { @@ -227,7 +227,7 @@ function _findElementByPathWithProject( } if (result != null) { - cb(null, result, type) + cb(null, result, type, folder) } else { cb( new Error( @@ -241,7 +241,7 @@ function _findElementByPathWithProject( return callback(new Error('Tried to find an element for a null project')) } if (needlePath === '' || needlePath === '/') { - return callback(null, project.rootFolder[0], 'folder') + return callback(null, project.rootFolder[0], 'folder', null) } if (needlePath.indexOf('/') === 0) { @@ -317,6 +317,7 @@ module.exports = { findElementByPath: promisifyMultiResult(findElementByPath, [ 'element', 'type', + 'folder', ]), findRootDoc: promisifyMultiResult(findRootDoc, [ 'element', diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js index bc2dc6f118..44cd3a0c33 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js @@ -7,6 +7,7 @@ const Path = require('path') const metrics = require('@overleaf/metrics') const NotificationsBuilder = require('../Notifications/NotificationsBuilder') const SessionManager = require('../Authentication/SessionManager') +const HttpErrorHandler = require('../Errors/HttpErrorHandler') const TpdsQueueManager = require('./TpdsQueueManager') // mergeUpdate and deleteUpdate are used by Dropbox, where the project is only @@ -80,6 +81,32 @@ async function deleteUpdate(req, res) { res.sendStatus(200) } +/** + * Update endpoint that accepts update details as JSON + */ +async function updateFolder(req, res) { + const userId = req.body.userId + const { projectName, filePath } = splitPath(req.body.path) + const metadata = await TpdsUpdateHandler.promises.createFolder( + userId, + projectName, + filePath + ) + if (metadata == null) { + return HttpErrorHandler.conflict(req, res, 'Could not create folder', { + userId, + projectName, + filePath, + }) + } + res.json({ + entityId: metadata.folderId.toString(), + projectId: metadata.projectId.toString(), + path: metadata.path, + folderId: metadata.parentFolderId.toString(), + }) +} + // 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, @@ -118,10 +145,13 @@ async function getQueues(req, res, next) { } function parseParams(req) { - let filePath, projectName - let path = req.params[0] const userId = req.params.user_id + const { projectName, filePath } = splitPath(req.params[0]) + return { filePath, userId, projectName } +} +function splitPath(path) { + let filePath, projectName path = Path.join('/', path) if (path.substring(1).indexOf('/') === -1) { filePath = '/' @@ -132,12 +162,13 @@ function parseParams(req) { projectName = projectName.replace('/', '') } - return { filePath, userId, projectName } + return { projectName, filePath } } module.exports = { mergeUpdate: expressify(mergeUpdate), deleteUpdate: expressify(deleteUpdate), + updateFolder: expressify(updateFolder), updateProjectContents: expressify(updateProjectContents), deleteProjectContents: expressify(deleteProjectContents), getQueues: expressify(getQueues), diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js index 40633f1d56..7da0e79325 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js @@ -144,11 +144,39 @@ async function handleDuplicateProjects(userId, projectName) { .create(projectName) } +async function createFolder(userId, projectName, path) { + const project = await getOrCreateProject(userId, projectName) + if (project == null) { + return null + } + + const projectIsOnCooldown = + await CooldownManager.promises.isProjectOnCooldown(project._id) + if (projectIsOnCooldown) { + throw new Errors.TooManyRequestsError('project on cooldown') + } + + const shouldIgnore = await FileTypeManager.promises.shouldIgnore(path) + if (shouldIgnore) { + return null + } + + const folder = await UpdateMerger.promises.createFolder(project._id, path) + return { + folderId: folder._id, + parentFolderId: folder.parentFolder_id, + projectId: project._id, + path, + } +} + module.exports = { newUpdate: callbackify(newUpdate), deleteUpdate: callbackify(deleteUpdate), + createFolder: callbackify(createFolder), promises: { newUpdate, deleteUpdate, + createFolder, }, } diff --git a/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js b/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js index 815ba4d85b..976c88b466 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/UpdateMerger.js @@ -157,13 +157,23 @@ async function _readFileIntoTextArray(path) { return lines } +async function createFolder(projectId, path) { + const { lastFolder: folder } = await EditorController.promises.mkdirp( + projectId, + path + ) + return folder +} + module.exports = { mergeUpdate: callbackify(mergeUpdate), _mergeUpdate: callbackify(_mergeUpdate), deleteUpdate: callbackify(deleteUpdate), + createFolder: callbackify(createFolder), promises: { mergeUpdate, _mergeUpdate, // called by GitBridgeHandler deleteUpdate, + createFolder, }, } diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index 898eeb62d7..9b447428eb 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -908,6 +908,11 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { DocumentController.setDocument ) + privateApiRouter.post( + '/tpds/folder-update', + AuthenticationController.requirePrivateApiAuth(), + TpdsController.updateFolder + ) privateApiRouter.post( '/user/:user_id/update/*', AuthenticationController.requirePrivateApiAuth(), diff --git a/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js b/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js index 5013f03e48..6a22a416bf 100644 --- a/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js +++ b/services/web/test/unit/src/Project/ProjectEntityMongoUpdateHandlerTests.js @@ -144,7 +144,7 @@ describe('ProjectEntityMongoUpdateHandler', function () { path: '/', }) ) - .resolves({ element: this.rootFolder, type: 'folder' }) + .resolves({ element: this.rootFolder, type: 'folder', folder: null }) this.ProjectLocator.promises.findElementByPath .withArgs( sinon.match({ @@ -152,7 +152,11 @@ describe('ProjectEntityMongoUpdateHandler', function () { path: '/test-folder', }) ) - .resolves({ element: this.folder, type: 'folder' }) + .resolves({ + element: this.folder, + type: 'folder', + folder: this.rootFolder, + }) this.ProjectLocator.promises.findElementByPath .withArgs( sinon.match({ @@ -160,7 +164,11 @@ describe('ProjectEntityMongoUpdateHandler', function () { path: '/test-folder/test-subfolder', }) ) - .resolves({ element: this.subfolder, type: 'folder' }) + .resolves({ + element: this.subfolder, + type: 'folder', + folder: this.folder, + }) this.ProjectGetter = { promises: { @@ -437,9 +445,7 @@ describe('ProjectEntityMongoUpdateHandler', function () { }) it('should report the parent folder', function () { - expect(this.result.folder.parentFolder_id).not.equal( - this.rootFolder._id - ) + expect(this.result.folder.parentFolder_id).to.equal(this.rootFolder._id) }) it('should not return new folders', function () { diff --git a/services/web/test/unit/src/Project/ProjectLocatorTests.js b/services/web/test/unit/src/Project/ProjectLocatorTests.js index 40e710deee..b3fb8a71d3 100644 --- a/services/web/test/unit/src/Project/ProjectLocatorTests.js +++ b/services/web/test/unit/src/Project/ProjectLocatorTests.js @@ -308,12 +308,13 @@ describe('ProjectLocator', function () { const path = `${doc1.name}` this.locator.findElementByPath( { project, path }, - (err, element, type) => { + (err, element, type, folder) => { if (err != null) { return done(err) } element.should.deep.equal(doc1) expect(type).to.equal('doc') + expect(folder).to.equal(rootFolder) done() } ) @@ -323,12 +324,13 @@ describe('ProjectLocator', function () { const path = `/${doc1.name}` this.locator.findElementByPath( { project, path }, - (err, element, type) => { + (err, element, type, folder) => { if (err != null) { return done(err) } element.should.deep.equal(doc1) expect(type).to.equal('doc') + expect(folder).to.equal(rootFolder) done() } ) @@ -338,12 +340,13 @@ describe('ProjectLocator', function () { const path = `${subFolder.name}/${secondSubFolder.name}/${subSubDoc.name}` this.locator.findElementByPath( { project, path }, - (err, element, type) => { + (err, element, type, folder) => { if (err != null) { return done(err) } element.should.deep.equal(subSubDoc) expect(type).to.equal('doc') + expect(folder).to.equal(secondSubFolder) done() } ) @@ -353,12 +356,13 @@ describe('ProjectLocator', function () { const path = `${file1.name}` this.locator.findElementByPath( { project, path }, - (err, element, type) => { + (err, element, type, folder) => { if (err != null) { return done(err) } element.should.deep.equal(file1) expect(type).to.equal('file') + expect(folder).to.equal(rootFolder) done() } ) @@ -368,12 +372,13 @@ describe('ProjectLocator', function () { const path = `${subFolder.name}/${secondSubFolder.name}/${subSubFile.name}` this.locator.findElementByPath( { project, path }, - (err, element, type) => { + (err, element, type, folder) => { if (err != null) { return done(err) } element.should.deep.equal(subSubFile) expect(type).to.equal('file') + expect(folder).to.equal(secondSubFolder) done() } ) @@ -383,12 +388,13 @@ describe('ProjectLocator', function () { const path = `${subFolder.name.toUpperCase()}/${secondSubFolder.name.toUpperCase()}/${subSubFile.name.toUpperCase()}` this.locator.findElementByPath( { project, path }, - (err, element, type) => { + (err, element, type, folder) => { if (err != null) { return done(err) } element.should.deep.equal(subSubFile) expect(type).to.equal('file') + expect(folder).to.equal(secondSubFolder) done() } ) @@ -398,12 +404,13 @@ describe('ProjectLocator', function () { const path = `${subFolder.name}/${secondSubFolder.name}` this.locator.findElementByPath( { project, path }, - (err, element, type) => { + (err, element, type, folder) => { if (err != null) { return done(err) } element.should.deep.equal(secondSubFolder) expect(type).to.equal('folder') + expect(folder).to.equal(subFolder) done() } ) @@ -413,12 +420,13 @@ describe('ProjectLocator', function () { const path = '/' this.locator.findElementByPath( { project, path }, - (err, element, type) => { + (err, element, type, folder) => { if (err != null) { return done(err) } element.should.deep.equal(rootFolder) expect(type).to.equal('folder') + expect(folder).to.equal(null) done() } ) diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js index 7b5adaaa0c..2492c78471 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js @@ -19,6 +19,7 @@ describe('TpdsController', function () { promises: { newUpdate: sinon.stub().resolves(this.metadata), deleteUpdate: sinon.stub().resolves(), + createFolder: sinon.stub().resolves(), }, } this.UpdateMerger = { @@ -38,6 +39,9 @@ describe('TpdsController', function () { getQueues: sinon.stub().resolves('queues'), }, } + this.HttpErrorHandler = { + conflict: sinon.stub(), + } this.TpdsController = SandboxedModule.require(MODULE_PATH, { requires: { @@ -45,6 +49,7 @@ describe('TpdsController', function () { './UpdateMerger': this.UpdateMerger, '../Notifications/NotificationsBuilder': this.NotificationsBuilder, '../Authentication/SessionManager': this.SessionManager, + '../Errors/HttpErrorHandler': this.HttpErrorHandler, './TpdsQueueManager': this.TpdsQueueManager, }, }) @@ -166,6 +171,47 @@ describe('TpdsController', function () { }) }) + describe('updateFolder', function () { + beforeEach(function () { + this.req = { + body: { userId: this.user_id, path: '/abc/def/ghi.txt' }, + } + this.res = { + json: sinon.stub(), + } + }) + + it("creates a folder if it doesn't exist", function (done) { + const metadata = { + folderId: ObjectId(), + projectId: ObjectId(), + path: '/def/ghi.txt', + parentFolderId: ObjectId(), + } + this.TpdsUpdateHandler.promises.createFolder.resolves(metadata) + this.res.json.callsFake(body => { + expect(body).to.deep.equal({ + entityId: metadata.folderId.toString(), + projectId: metadata.projectId.toString(), + path: metadata.path, + folderId: metadata.parentFolderId.toString(), + }) + done() + }) + this.TpdsController.updateFolder(this.req, this.res) + }) + + it("returns a 409 if the folder couldn't be created", function (done) { + this.TpdsUpdateHandler.promises.createFolder.resolves(null) + this.HttpErrorHandler.conflict.callsFake((req, res) => { + expect(req).to.equal(this.req) + expect(res).to.equal(this.res) + done() + }) + this.TpdsController.updateFolder(this.req, this.res) + }) + }) + describe('parseParams', function () { it('should take the project name off the start and replace with slash', function () { const path = 'noSlashHere' diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js index 2d34c74e49..c3e440b9a5 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js @@ -36,6 +36,11 @@ describe('TpdsUpdateHandler', function () { this.source = 'dropbox' this.path = `/some/file` this.update = {} + this.folderPath = '/some/folder' + this.folder = { + _id: ObjectId(), + parentFolder_id: ObjectId(), + } this.CooldownManager = { promises: { @@ -93,6 +98,7 @@ describe('TpdsUpdateHandler', function () { promises: { deleteUpdate: sinon.stub().resolves(), mergeUpdate: sinon.stub().resolves(), + createFolder: sinon.stub().resolves(this.folder), }, } @@ -281,6 +287,69 @@ describe('TpdsUpdateHandler', function () { expectDropboxUnlinked() }) }) + + describe('getting a folder update', function () { + describe('with no matching project', function () { + setupMatchingProjects([]) + receiveFolderUpdate() + expectProjectCreated() + expectFolderUpdateProcessed() + }) + + describe('with one matching active project', function () { + setupMatchingProjects(['active1']) + receiveFolderUpdate() + expectProjectNotCreated() + expectFolderUpdateProcessed() + }) + + describe('with one matching archived project', function () { + setupMatchingProjects(['archived1']) + receiveFolderUpdate() + expectProjectNotCreated() + expectFolderUpdateNotProcessed() + expectDropboxNotUnlinked() + }) + + describe('with two matching active projects', function () { + setupMatchingProjects(['active1', 'active2']) + receiveFolderUpdate() + expectProjectNotCreated() + expectFolderUpdateNotProcessed() + expectDropboxUnlinked() + }) + + describe('with two matching archived projects', function () { + setupMatchingProjects(['archived1', 'archived2']) + receiveFolderUpdate() + expectProjectNotCreated() + expectFolderUpdateNotProcessed() + expectDropboxNotUnlinked() + }) + + describe('with one matching active and one matching archived project', function () { + setupMatchingProjects(['active1', 'archived1']) + receiveFolderUpdate() + expectProjectNotCreated() + expectFolderUpdateNotProcessed() + expectDropboxUnlinked() + }) + + describe('update to a project on cooldown', async function () { + setupMatchingProjects(['active1']) + setupProjectOnCooldown() + beforeEach(async function () { + await expect( + this.TpdsUpdateHandler.promises.createFolder( + this.userId, + this.projectName, + this.path + ) + ).to.be.rejectedWith(Errors.TooManyRequestsError) + }) + expectFolderUpdateNotProcessed() + }) + }) }) /* Setup helpers */ @@ -338,6 +407,16 @@ function receiveProjectDelete() { }) } +function receiveFolderUpdate() { + beforeEach(async function () { + await this.TpdsUpdateHandler.promises.createFolder( + this.userId, + this.projectName, + this.folderPath + ) + }) +} + /* Expectations */ function expectProjectCreated() { @@ -388,6 +467,21 @@ function expectUpdateNotProcessed() { }) } +function expectFolderUpdateProcessed() { + it('processes the folder update', function () { + expect(this.UpdateMerger.promises.createFolder).to.have.been.calledWith( + this.projects.active1._id, + this.folderPath + ) + }) +} + +function expectFolderUpdateNotProcessed() { + it("doesn't process the folder update", function () { + expect(this.UpdateMerger.promises.createFolder).not.to.have.been.called + }) +} + function expectDropboxUnlinked() { it('unlinks Dropbox', function () { expect(this.Modules.promises.hooks.fire).to.have.been.calledWith(