From 1a2484006298c1a850e39480dc31631ac0003393 Mon Sep 17 00:00:00 2001 From: Juan Tejada Date: Mon, 5 Dec 2016 18:50:11 -0800 Subject: [PATCH] [local-sync] Correctly sync folders and labels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit will correctly keep track of folder and label ids when creating them from N1. Previously, when we sent the request to create a folder or label to our api, we would immediately get back a serverId because it was created optimistically in the back end— given that K2 is strictly non-optimistic, we won’t have a serverId until some undetermined time in the future, and we need to somehow reference the object that /was/ optimistically created in N1 to update the ui when we do get the server id. Since we can deterministically generate ids for folders and labels, we "guess" what its going to be, and include it in the props of the syncback request returned to N1. This is the simplest solution to get thing working correctly right now, but we’ll need to revisit this in the future for other types of objects (drafts, contacts, events), and revisit how we will manage optimistic updates in N1 when we merge the 2 codebases with K2 (given that K2 was designed to be non-optimisitc). --- .../local-sync/src/local-api/route-helpers.js | 17 +-- .../src/local-api/routes/categories.js | 102 ++++++++++++------ .../imap/fetch-folder-list.js | 3 +- .../local-sync-worker/sync-process-manager.js | 2 +- .../src/local-sync-worker/sync-worker.js | 13 +-- ...k-factory.js => syncback-task-factory.es6} | 17 ++- .../syncback_tasks/create-category.imap.js | 14 +++ .../syncback_tasks/create-folder.imap.js | 13 --- .../syncback_tasks/delete-folder.imap.js | 9 +- .../syncback_tasks/delete-label.imap.js | 14 +++ .../syncback_tasks/rename-folder.imap.js | 9 +- .../syncback_tasks/rename-label.imap.js | 15 +++ packages/local-sync/src/models/folder.js | 8 +- packages/local-sync/src/models/label.js | 6 ++ .../local-sync/src/models/syncbackRequest.js | 4 +- 15 files changed, 167 insertions(+), 79 deletions(-) rename packages/local-sync/src/local-sync-worker/{syncback-task-factory.js => syncback-task-factory.es6} (82%) create mode 100644 packages/local-sync/src/local-sync-worker/syncback_tasks/create-category.imap.js delete mode 100644 packages/local-sync/src/local-sync-worker/syncback_tasks/create-folder.imap.js create mode 100644 packages/local-sync/src/local-sync-worker/syncback_tasks/delete-label.imap.js create mode 100644 packages/local-sync/src/local-sync-worker/syncback_tasks/rename-label.imap.js diff --git a/packages/local-sync/src/local-api/route-helpers.js b/packages/local-sync/src/local-api/route-helpers.js index 6dc9cc620..5f938d328 100644 --- a/packages/local-sync/src/local-api/route-helpers.js +++ b/packages/local-sync/src/local-api/route-helpers.js @@ -1,13 +1,14 @@ const Serialization = require('./serialization'); +const SyncProcessManager = require('../local-sync-worker/sync-process-manager') module.exports = { - createSyncbackRequest: function createSyncbackRequest(request, reply, syncRequestArgs) { - request.getAccountDatabase().then((db) => { - const accountId = request.auth.credentials.id; - syncRequestArgs.accountId = accountId - db.SyncbackRequest.create(syncRequestArgs).then((syncbackRequest) => { - reply(Serialization.jsonStringify(syncbackRequest)) - }) - }) + async createSyncbackRequest(request, reply, syncRequestArgs) { + const account = request.auth.credentials + syncRequestArgs.accountId = account.id + + const db = await request.getAccountDatabase() + const syncbackRequest = await db.SyncbackRequest.create(syncRequestArgs) + SyncProcessManager.wakeWorkerForAccount(account) + reply(Serialization.jsonStringify(syncbackRequest)) }, } diff --git a/packages/local-sync/src/local-api/routes/categories.js b/packages/local-sync/src/local-api/routes/categories.js index c1e97de3a..0380c622c 100644 --- a/packages/local-sync/src/local-api/routes/categories.js +++ b/packages/local-sync/src/local-api/routes/categories.js @@ -25,16 +25,14 @@ module.exports = (server) => { ), }, }, - handler: (request, reply) => { - request.getAccountDatabase().then((db) => { - const Klass = db[klass]; - Klass.findAll({ - limit: request.query.limit, - offset: request.query.offset, - }).then((items) => { - reply(Serialization.jsonStringify(items)); - }) + async handler(request, reply) { + const db = await request.getAccountDatabase() + const Klass = db[klass]; + const items = await Klass.findAll({ + limit: request.query.limit, + offset: request.query.offset, }) + reply(Serialization.jsonStringify(items)); }, }); @@ -47,17 +45,30 @@ module.exports = (server) => { config: { description: `Create ${term}`, tags: [term], - validate: {}, + validate: { + params: { + payload: { + display_name: Joi.string().required(), + }, + }, + }, response: { schema: Serialization.jsonSchema('SyncbackRequest'), }, }, - handler: (request, reply) => { - if (request.payload.display_name) { + async handler(request, reply) { + const {payload} = request + if (payload.display_name) { + const accountId = request.auth.credentials.id + const db = await request.getAccountDatabase() + const objectId = db[klass].hash({boxName: payload.display_name, accountId}) + createSyncbackRequest(request, reply, { - type: "CreateFolder", + type: "CreateCategory", props: { - displayName: request.payload.display_name, + objectId, + object: klass.toLowerCase(), + displayName: payload.display_name, }, }) } @@ -72,22 +83,42 @@ module.exports = (server) => { tags: [term], validate: { params: { - id: Joi.string(), + id: Joi.string().required(), + payload: { + display_name: Joi.string().required(), + }, }, }, response: { schema: Serialization.jsonSchema('SyncbackRequest'), }, }, - handler: (request, reply) => { - if (request.payload.display_name) { - createSyncbackRequest(request, reply, { - type: "RenameFolder", - props: { - displayName: request.payload.display_name, - id: request.params.id, - }, - }) + async handler(request, reply) { + const {payload} = request + if (payload.display_name) { + const accountId = request.auth.credentials.id + const db = await request.getAccountDatabase() + const objectId = db[klass].hash({boxName: payload.display_name, accountId}) + + if (klass === 'Label') { + createSyncbackRequest(request, reply, { + type: "RenameLabel", + props: { + objectId, + labelId: request.params.id, + displayName: payload.display_name, + }, + }) + } else { + createSyncbackRequest(request, reply, { + type: "RenameFolder", + props: { + objectId, + folderId: request.params.id, + displayName: payload.display_name, + }, + }) + } } }, }) @@ -100,7 +131,7 @@ module.exports = (server) => { tags: [term], validate: { params: { - id: Joi.number().integer(), + id: Joi.string().required(), }, }, response: { @@ -108,12 +139,21 @@ module.exports = (server) => { }, }, handler: (request, reply) => { - createSyncbackRequest(request, reply, { - type: "DeleteFolder", - props: { - id: request.params.id, - }, - }) + if (klass === 'Label') { + createSyncbackRequest(request, reply, { + type: "DeleteLabel", + props: { + labelId: request.params.id, + }, + }) + } else { + createSyncbackRequest(request, reply, { + type: "DeleteFolder", + props: { + folderId: request.params.id, + }, + }) + } }, }) }); diff --git a/packages/local-sync/src/local-sync-worker/imap/fetch-folder-list.js b/packages/local-sync/src/local-sync-worker/imap/fetch-folder-list.js index b0ab88578..40f4da679 100644 --- a/packages/local-sync/src/local-sync-worker/imap/fetch-folder-list.js +++ b/packages/local-sync/src/local-sync-worker/imap/fetch-folder-list.js @@ -1,4 +1,3 @@ -const crypto = require('crypto') const {Provider, PromiseUtils} = require('isomorphic-core'); const {localizedCategoryNames} = require('../sync-utils') @@ -110,7 +109,7 @@ class FetchFolderList { const {accountId} = this._db category = Klass.build({ accountId, - id: crypto.createHash('sha256').update(`${accountId}${boxName}`, 'utf8').digest('hex'), + id: Klass.hash({boxName, accountId}), name: boxName, role: role, }); diff --git a/packages/local-sync/src/local-sync-worker/sync-process-manager.js b/packages/local-sync/src/local-sync-worker/sync-process-manager.js index 3fcd2ac20..00f9bc892 100644 --- a/packages/local-sync/src/local-sync-worker/sync-process-manager.js +++ b/packages/local-sync/src/local-sync-worker/sync-process-manager.js @@ -68,4 +68,4 @@ class SyncProcessManager { } } -module.exports = new SyncProcessManager(); +module.exports = new SyncProcessManager() diff --git a/packages/local-sync/src/local-sync-worker/sync-worker.js b/packages/local-sync/src/local-sync-worker/sync-worker.js index d80532fd8..82f94886a 100644 --- a/packages/local-sync/src/local-sync-worker/sync-worker.js +++ b/packages/local-sync/src/local-sync-worker/sync-worker.js @@ -4,10 +4,7 @@ const { PromiseUtils, } = require('isomorphic-core'); const LocalDatabaseConnector = require('../shared/local-database-connector') -const { - jsonError, -} = require('./sync-utils') - +const {jsonError} = require('./sync-utils') const FetchFolderList = require('./imap/fetch-folder-list') const FetchMessagesInFolder = require('./imap/fetch-messages-in-folder') const SyncbackTaskFactory = require('./syncback-task-factory') @@ -112,8 +109,7 @@ class SyncWorker { conn.on('update', () => { this._onConnectionIdleUpdate(); }) - conn.on('queue-empty', () => { - }); + conn.on('queue-empty', () => {}); this._conn = conn; return await this._conn.connect(); @@ -123,9 +119,8 @@ class SyncWorker { const {SyncbackRequest} = this._db; const where = {where: {status: "NEW"}, limit: 100}; - const tasks = (await SyncbackRequest.findAll(where)).map((req) => - SyncbackTaskFactory.create(this._account, req) - ); + const tasks = await SyncbackRequest.findAll(where) + .map((req) => SyncbackTaskFactory.create(this._account, req)); return PromiseUtils.each(tasks, this.runSyncbackTask.bind(this)); } diff --git a/packages/local-sync/src/local-sync-worker/syncback-task-factory.js b/packages/local-sync/src/local-sync-worker/syncback-task-factory.es6 similarity index 82% rename from packages/local-sync/src/local-sync-worker/syncback-task-factory.js rename to packages/local-sync/src/local-sync-worker/syncback-task-factory.es6 index 43d939074..dfd4257ef 100644 --- a/packages/local-sync/src/local-sync-worker/syncback-task-factory.js +++ b/packages/local-sync/src/local-sync-worker/syncback-task-factory.es6 @@ -4,6 +4,15 @@ * */ class SyncbackTaskFactory { + + static TaskTypesAffectingMessageFolderUIDs = [ + 'MoveThreadToFolder', + 'MoveMessageToFolder', + 'SetThreadFolderAndLabels', + 'RenameFolder', + 'DeleteFolder', + ] + static create(account, syncbackRequest) { let Task = null; switch (syncbackRequest.type) { @@ -33,12 +42,16 @@ class SyncbackTaskFactory { Task = require('./syncback_tasks/star-message.imap'); break; case "UnstarMessage": Task = require('./syncback_tasks/unstar-message.imap'); break; - case "CreateFolder": - Task = require('./syncback_tasks/create-folder.imap'); break; + case "CreateCategory": + Task = require('./syncback_tasks/create-category.imap'); break; case "RenameFolder": Task = require('./syncback_tasks/rename-folder.imap'); break; + case "RenameLabel": + Task = require('./syncback_tasks/rename-label.imap'); break; case "DeleteFolder": Task = require('./syncback_tasks/delete-folder.imap'); break; + case "DeleteLabel": + Task = require('./syncback_tasks/delete-label.imap'); break; case "DeleteMessage": Task = require('./syncback_tasks/delete-message.imap'); break; case "SaveSentMessage": diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/create-category.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/create-category.imap.js new file mode 100644 index 000000000..3c1e2b76f --- /dev/null +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/create-category.imap.js @@ -0,0 +1,14 @@ +const SyncbackTask = require('./syncback-task') + +class CreateCategoryIMAP extends SyncbackTask { + description() { + return `CreateCategory`; + } + + async run(db, imap) { + const syncbackRequestObject = this.syncbackRequestObject() + const displayName = syncbackRequestObject.props.displayName + await imap.addBox(displayName) + } +} +module.exports = CreateCategoryIMAP diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/create-folder.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/create-folder.imap.js deleted file mode 100644 index c5ecec491..000000000 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/create-folder.imap.js +++ /dev/null @@ -1,13 +0,0 @@ -const SyncbackTask = require('./syncback-task') - -class CreateFolderIMAP extends SyncbackTask { - description() { - return `CreateFolder`; - } - - run(db, imap) { - const folderName = this.syncbackRequestObject().props.displayName - return imap.addBox(folderName) - } -} -module.exports = CreateFolderIMAP diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-folder.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-folder.imap.js index 005d6c9af..4629b0d3b 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-folder.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-folder.imap.js @@ -5,11 +5,10 @@ class DeleteFolderIMAP extends SyncbackTask { return `DeleteFolder`; } - run(db, imap) { - const folderId = this.syncbackRequestObject().props.id - return db.Folder.findById(folderId).then((folder) => { - return imap.delBox(folder.name); - }) + async run(db, imap) { + const folderId = this.syncbackRequestObject().props.folderId + const folder = await db.Folder.findById(folderId) + return imap.delBox(folder.name); } } module.exports = DeleteFolderIMAP diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-label.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-label.imap.js new file mode 100644 index 000000000..498cc7c09 --- /dev/null +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-label.imap.js @@ -0,0 +1,14 @@ +const SyncbackTask = require('./syncback-task') + +class DeleteLabelIMAP extends SyncbackTask { + description() { + return `DeleteLabel`; + } + + async run(db, imap) { + const labelId = this.syncbackRequestObject().props.labelId + const label = await db.Label.findById(labelId) + return imap.delBox(label.name); + } +} +module.exports = DeleteLabelIMAP diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-folder.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-folder.imap.js index 003e12bb3..70728afbe 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-folder.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-folder.imap.js @@ -5,12 +5,11 @@ class RenameFolderIMAP extends SyncbackTask { return `RenameFolder`; } - run(db, imap) { - const folderId = this.syncbackRequestObject().props.id + async run(db, imap) { + const folderId = this.syncbackRequestObject().props.folderId const newFolderName = this.syncbackRequestObject().props.displayName - return db.Folder.findById(folderId).then((folder) => { - return imap.renameBox(folder.name, newFolderName); - }) + const folder = await db.Folder.findById(folderId) + return imap.renameBox(folder.name, newFolderName); } } module.exports = RenameFolderIMAP diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-label.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-label.imap.js new file mode 100644 index 000000000..b0779d8ab --- /dev/null +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-label.imap.js @@ -0,0 +1,15 @@ +const SyncbackTask = require('./syncback-task') + +class RenameLabelIMAP extends SyncbackTask { + description() { + return `RenameLabel`; + } + + async run(db, imap) { + const labelId = this.syncbackRequestObject().props.labelId + const newLabelName = this.syncbackRequestObject().props.displayName + const folder = await db.Label.findById(labelId) + return imap.renameBox(folder.name, newLabelName); + } +} +module.exports = RenameLabelIMAP diff --git a/packages/local-sync/src/models/folder.js b/packages/local-sync/src/models/folder.js index 6a810c420..cea714496 100644 --- a/packages/local-sync/src/models/folder.js +++ b/packages/local-sync/src/models/folder.js @@ -1,3 +1,4 @@ +const crypto = require('crypto') const {DatabaseTypes: {buildJSONColumnOptions}} = require('isomorphic-core'); const {formatImapPath} = require('../shared/imap-paths-utils'); @@ -21,10 +22,15 @@ module.exports = (sequelize, Sequelize) => { }, ], classMethods: { - associate: ({Folder, Message, Thread}) => { + associate({Folder, Message, Thread}) { Folder.hasMany(Message) Folder.belongsToMany(Thread, {through: 'thread_folders'}) }, + + hash({boxName, accountId}) { + const cleanName = formatImapPath(boxName) + return crypto.createHash('sha256').update(`${accountId}${cleanName}`, 'utf8').digest('hex') + }, }, instanceMethods: { toJSON: function toJSON() { diff --git a/packages/local-sync/src/models/label.js b/packages/local-sync/src/models/label.js index 5f1315bd9..a893b9830 100644 --- a/packages/local-sync/src/models/label.js +++ b/packages/local-sync/src/models/label.js @@ -1,3 +1,4 @@ +const crypto = require('crypto') const {formatImapPath} = require('../shared/imap-paths-utils'); module.exports = (sequelize, Sequelize) => { @@ -40,6 +41,11 @@ module.exports = (sequelize, Sequelize) => { where: sequelize.or({name: labelNames}, {role: labelRoles}), }) }, + + hash({boxName, accountId}) { + const cleanName = formatImapPath(boxName) + return crypto.createHash('sha256').update(`${accountId}${cleanName}`, 'utf8').digest('hex') + }, }, instanceMethods: { imapLabelIdentifier() { diff --git a/packages/local-sync/src/models/syncbackRequest.js b/packages/local-sync/src/models/syncbackRequest.js index 4cfdaac21..4907b9232 100644 --- a/packages/local-sync/src/models/syncbackRequest.js +++ b/packages/local-sync/src/models/syncbackRequest.js @@ -13,14 +13,14 @@ module.exports = (sequelize, Sequelize) => { accountId: { type: Sequelize.STRING, allowNull: false }, }, { instanceMethods: { - toJSON: function toJSON() { + toJSON() { return { id: `${this.id}`, type: this.type, error: JSON.stringify(this.error || {}), props: this.props, status: this.status, - object: 'providerSyncbackRequest', + object: 'syncbackRequest', account_id: this.accountId, } },