From 2fe768c79be424ad1e6348cf4795f52c36b7dc50 Mon Sep 17 00:00:00 2001 From: yan Date: Thu, 8 Jun 2017 17:42:08 -0700 Subject: [PATCH] handle empty objects in sync DELETEs fix #9308 --- app/browser/reducers/sitesReducer.js | 34 ++++---- app/sync.js | 116 +++++++++++++-------------- js/actions/syncActions.js | 7 -- js/constants/syncConstants.js | 1 - js/state/siteUtil.js | 39 +++++++-- js/state/syncUtil.js | 26 +++++- 6 files changed, 127 insertions(+), 96 deletions(-) diff --git a/app/browser/reducers/sitesReducer.js b/app/browser/reducers/sitesReducer.js index 8c7c9709e16..c651d9fd994 100644 --- a/app/browser/reducers/sitesReducer.js +++ b/app/browser/reducers/sitesReducer.js @@ -9,7 +9,6 @@ const filtering = require('../../filtering') const siteCache = require('../../common/state/siteCache') const siteTags = require('../../../js/constants/siteTags') const siteUtil = require('../../../js/state/siteUtil') -const syncActions = require('../../../js/actions/syncActions') const syncUtil = require('../../../js/state/syncUtil') const Immutable = require('immutable') const settings = require('../../../js/constants/settings') @@ -79,8 +78,7 @@ const sitesReducer = (state, action, immutableAction) => { state = updateActiveTabBookmarked(state) break case appConstants.APP_REMOVE_SITE: - const removeSiteSyncCallback = action.skipSync ? undefined : syncActions.removeSite - state = siteUtil.removeSite(state, action.siteDetail, action.tag, true, removeSiteSyncCallback) + state = siteUtil.removeSite(state, action.siteDetail, action.tag, true) if (syncEnabled()) { state = syncUtil.updateSiteCache(state, action.siteDetail) } @@ -96,6 +94,7 @@ const sitesReducer = (state, action, immutableAction) => { } break case appConstants.APP_APPLY_SITE_RECORDS: + // Applies site records fetched by sync let nextFolderId = siteUtil.getNextFolderId(state.get('sites')) // Ensure that all folders are assigned folderIds action.records.forEach((record, i) => { @@ -109,21 +108,22 @@ const sitesReducer = (state, action, immutableAction) => { } }) action.records.forEach((record) => { - const siteData = syncUtil.getSiteDataFromRecord(record, state, action.records) - const tag = siteData.tag - let siteDetail = siteData.siteDetail - switch (record.action) { - case writeActions.CREATE: - state = siteUtil.addSite(state, siteDetail, tag, undefined, true) - break - case writeActions.UPDATE: - state = siteUtil.addSite(state, siteDetail, tag, siteData.existingObjectData, true) - break - case writeActions.DELETE: - state = siteUtil.removeSite(state, siteDetail, tag) - break + if (record.action === writeActions.DELETE) { + state = siteUtil.removeSiteByObjectId(state, record.objectId, record.objectData) + } else { + const siteData = syncUtil.getSiteDataFromRecord(record, state, action.records) + const tag = siteData.tag + let siteDetail = siteData.siteDetail + switch (record.action) { + case writeActions.CREATE: + state = siteUtil.addSite(state, siteDetail, tag, undefined, true) + break + case writeActions.UPDATE: + state = siteUtil.addSite(state, siteDetail, tag, siteData.existingObjectData, true) + break + } + state = syncUtil.updateSiteCache(state, siteDetail) } - state = syncUtil.updateSiteCache(state, siteDetail) }) break case appConstants.APP_TAB_UPDATED: diff --git a/app/sync.js b/app/sync.js index f06247ede13..a9126e004da 100644 --- a/app/sync.js +++ b/app/sync.js @@ -10,6 +10,7 @@ const qr = require('qr-image') const ipcMain = electron.ipcMain const locale = require('./locale') const messages = require('../js/constants/messages') +const siteTags = require('../js/constants/siteTags') const syncMessages = require('../js/constants/sync/messages') const categories = require('../js/constants/sync/proto').categories const writeActions = require('../js/constants/sync/proto').actions @@ -27,7 +28,6 @@ const extensions = require('./extensions') const CATEGORY_MAP = syncUtil.CATEGORY_MAP const CATEGORY_NAMES = Object.keys(categories) -const SYNC_ACTIONS = Object.values(syncConstants) // The sync background script message sender let backgroundSender = null @@ -47,7 +47,7 @@ let pollIntervalId = null let deviceIdSent = false let bookmarksToolbarShown = false -// Determines what to sync +// Syncs state diffs to the sync server if needed const appStoreChangeCallback = function (diffs) { if (!backgroundSender) { return @@ -73,34 +73,61 @@ const appStoreChangeCallback = function (diffs) { return } - const isInsert = diff.op === 'add' && path.length === 3 - const isUpdate = fieldsToPick.includes(path[3]) // Ignore insignicant updates + let action = null - // DELETES are handled in appState because the old object is no longer - // available by the time emitChanges is received - if (isInsert || isUpdate) { - // Get the item's path and entry in appStore - const statePath = path.slice(1, 3).map((item) => item.replace(/~1/g, '/')) - const entry = AppStore.getState().getIn(statePath) - if (!entry || !entry.toJS) { - return - } - - let action = null - - if (isInsert && !entry.get('skipSync')) { + if (path.length === 3 || path[3] === 'tags') { + // XXX: adding/removing a tag (e.g. 'bookmark') corresponds to adding/deleting a site record in sync + if (diff.op === 'add') { action = writeActions.CREATE - } else if (isUpdate) { - action = writeActions.UPDATE + } else if (diff.op === 'remove') { + action = writeActions.DELETE } + } else if (fieldsToPick.includes(path[3])) { + action = writeActions.UPDATE + } - if (action !== null) { - // Set the object ID if there is not already one - const entryJS = entry.toJS() - entryJS.objectId = entryJS.objectId || syncUtil.newObjectId(statePath) + if (action === null) { + return + } - sendSyncRecords(backgroundSender, action, - [type === 'sites' ? syncUtil.createSiteData(entryJS) : syncUtil.createSiteSettingsData(statePath[1], entryJS)]) + const statePath = path.slice(1, 3).map((item) => item.replace(/~1/g, '/')) + const state = AppStore.getState() + const entry = state.getIn(statePath) + const isSite = type === 'sites' + + if (isSite && action === writeActions.DELETE && !entry) { + // If we deleted the site, it is no longer availble in appState. + // Find the corresponding objectId using the sync cache + // and send this in the sync record + const objectId = syncUtil.siteKeyToObjectId(state, statePath[1]) + if (objectId) { + // Delete the site from both history and bookmarks + sendSyncRecords(backgroundSender, action, [{ + name: 'bookmark', + objectId, + value: {} + }]) + sendSyncRecords(backgroundSender, action, [{ + name: 'historySite', + objectId, + value: {} + }]) + } + } else if (entry && entry.toJS) { + const entryJS = entry.toJS() + if (action === writeActions.DELETE && isSite) { + const tags = entryJS.tags || [] + sendSyncRecords(backgroundSender, action, [{ + objectId: entryJS.objectId, + value: {}, + name: tags.includes(siteTags.BOOKMARK) || siteTags.includes(siteTags.BOOKMARK_FOLDER) + ? 'historySite' // if the site is still a bookmark, it must have been deleted from history + : 'bookmark' + }]) + } else { + sendSyncRecords(backgroundSender, action, [ + isSite ? syncUtil.createSiteData(entryJS, state) : syncUtil.createSiteSettingsData(statePath[1], entryJS) + ]) } } }) @@ -110,7 +137,7 @@ const appStoreChangeCallback = function (diffs) { * Sends sync records of the same category to the sync server. * @param {event.sender} sender * @param {number} action - * @param {Array.<{name: string, value: Object}>} data + * @param {Array.<{objectId: Array, name: string, value: Object}>} data */ const sendSyncRecords = (sender, action, data) => { if (!deviceId) { @@ -125,7 +152,7 @@ const sendSyncRecords = (sender, action, data) => { return } sender.send(syncMessages.SEND_SYNC_RECORDS, category.categoryName, data.map((item) => { - if (!item || !item.name || !item.value) { + if (!item || !item.name || !item.value || !item.objectId) { return } return { @@ -137,34 +164,6 @@ const sendSyncRecords = (sender, action, data) => { })) } -/** - * @param {Object} action - * @returns {boolean} - */ -const validateAction = (action) => { - const SYNC_ACTIONS_WITHOUT_ITEMS = [ - syncConstants.SYNC_CLEAR_HISTORY, - syncConstants.SYNC_CLEAR_SITE_SETTINGS - ] - if (SYNC_ACTIONS.includes(action.actionType) !== true) { - return false - } - - // If the action requires an item, validate the item. - if (SYNC_ACTIONS_WITHOUT_ITEMS.includes(action.actionType) !== true) { - if (!action.item || !action.item.toJS) { - log('Missing item!') - return false - } - // Only accept items who have an objectId set already - if (!action.item.get('objectId')) { - log(`Missing object ID! ${action.item.toJS()}`) - return false - } - } - return true -} - const dispatcherCallback = (action) => { if (!backgroundSender) { return @@ -176,14 +175,10 @@ const dispatcherCallback = (action) => { } } // If sync is not enabled, the following actions should be ignored. - if (!syncEnabled() || validateAction(action) !== true || backgroundSender.isDestroyed()) { + if (!syncEnabled() || backgroundSender.isDestroyed()) { return } switch (action.actionType) { - case syncConstants.SYNC_REMOVE_SITE: - sendSyncRecords(backgroundSender, writeActions.DELETE, - [syncUtil.createSiteData(action.item.toJS())]) - break case syncConstants.SYNC_CLEAR_HISTORY: backgroundSender.send(syncMessages.DELETE_SYNC_CATEGORY, CATEGORY_MAP.historySite.categoryName) break @@ -318,7 +313,6 @@ module.exports.init = function (appState) { } // sent by about:preferences when sync should be reloaded ipcMain.on(messages.RELOAD_SYNC_EXTENSION, () => { - console.log('reloading sync') extensions.reloadExtension(syncExtensionId) }) // sent by about:preferences when resetting sync diff --git a/js/actions/syncActions.js b/js/actions/syncActions.js index bc50ac2cec7..4731321886a 100644 --- a/js/actions/syncActions.js +++ b/js/actions/syncActions.js @@ -7,13 +7,6 @@ const AppDispatcher = require('../dispatcher/appDispatcher') const syncConstants = require('../constants/syncConstants') const syncActions = { - removeSite: function (item) { - AppDispatcher.dispatch({ - actionType: syncConstants.SYNC_REMOVE_SITE, - item - }) - }, - clearHistory: function () { AppDispatcher.dispatch({ actionType: syncConstants.SYNC_CLEAR_HISTORY diff --git a/js/constants/syncConstants.js b/js/constants/syncConstants.js index 1ddea1899a5..36d1da8ec0c 100644 --- a/js/constants/syncConstants.js +++ b/js/constants/syncConstants.js @@ -6,7 +6,6 @@ const mapValuesByKeys = require('../lib/functional').mapValuesByKeys const _ = null const syncConstants = { - SYNC_REMOVE_SITE: _, /** @param {Immutable.Map} item */ SYNC_CLEAR_HISTORY: _, SYNC_CLEAR_SITE_SETTINGS: _ } diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index 595df5a97b7..a7320978fbb 100644 --- a/js/state/siteUtil.js +++ b/js/state/siteUtil.js @@ -331,6 +331,37 @@ module.exports.addSite = function (state, siteDetail, tag, originalSiteDetail, s return state } +/** + * Removes the appropriate tag from a site given the site's sync objectId. + * @param {Immutable.Map} state - Application state + * @param {Array} objectId - Object ID of object to be deleted + * @param {string} objectData - oneof 'historySite', 'bookmark' + * @returns {Immutable.Map} + */ +module.exports.removeSiteByObjectId = function (state, objectId, objectData) { + if (!objectId) { + return state + } + const cacheKey = ['sync', 'objectsById', objectId.join('|')] + const objectKey = state.getIn(cacheKey) + const site = objectKey && state.getIn(objectKey) + if (!site) { + return state + } + let tag = '' + if (objectData === 'bookmark') { + // determine whether this is a folder + tag = site.get('folderId') ? siteTags.BOOKMARK_FOLDER : siteTags.BOOKMARK + } + state = module.exports.removeSite(state, site, tag) + // If the object has been removed completely, purge it from the sync site + // cache + if (!state.getIn(objectKey)) { + state = state.deleteIn(cacheKey) + } + return state +} + /** * Removes the specified tag from a siteDetail * @@ -338,18 +369,14 @@ module.exports.addSite = function (state, siteDetail, tag, originalSiteDetail, s * @param {Immutable.Map} siteDetail The siteDetail to remove a tag from * @param {string} tag * @param {boolean} reorder whether to reorder sites (default with reorder) - * @param {Function=} syncCallback * @return {Immutable.Map} The new state Immutable object */ -module.exports.removeSite = function (state, siteDetail, tag, reorder = true, syncCallback) { +module.exports.removeSite = function (state, siteDetail, tag, reorder = true) { let sites = state.get('sites') const key = module.exports.getSiteKey(siteDetail) if (!key) { return state } - if (getSetting(settings.SYNC_ENABLED) === true && syncCallback) { - syncCallback(sites.getIn([key])) - } const tags = sites.getIn([key, 'tags']) if (isBookmarkFolder(tags)) { @@ -358,7 +385,7 @@ module.exports.removeSite = function (state, siteDetail, tag, reorder = true, sy childSites.forEach((site) => { const tags = site.get('tags') tags.forEach((tag) => { - state = module.exports.removeSite(state, site, tag, false, syncCallback) + state = module.exports.removeSite(state, site, tag, false) }) }) } diff --git a/js/state/syncUtil.js b/js/state/syncUtil.js index 47d10cb1447..3ca0b93c268 100644 --- a/js/state/syncUtil.js +++ b/js/state/syncUtil.js @@ -258,7 +258,6 @@ module.exports.getExistingObject = (categoryName, syncRecord) => { throw new Error(`Invalid category: ${categoryName}`) } if (!item) { - console.log(`Warning: Can't create JS from existingObject! ${JSON.stringify(existingObjectData)}`) return null } return { @@ -305,8 +304,27 @@ module.exports.updateSiteCache = (appState, siteDetail) => { const cacheKey = ['sync', 'objectsById', objectId.toJS().join('|')] if (object) { return appState.setIn(cacheKey, ['sites', siteKey]) - } else { - return appState.deleteIn(cacheKey) + } + // Keep obsolete object keys in the cache because they are used to get + // objectIds of deleted objects in appStoreChangeCallback +} + +/** + * Gets the objectId of a site based on its siteKey. + * Does not assume the site hasn't been deleted. + * @param {Immutable.Map} appState - application state + * @param {string} siteKey - The state path, or undefined if none is found + * @returns {Array=} + */ +module.exports.siteKeyToObjectId = (appState, siteKey) => { + const objectsById = appState.getIn(['sync', 'objectsById']) + if (objectsById) { + const objectIdKey = objectsById.findKey((value, key) => { + return value[0] === 'sites' && value[1] === siteKey + }) + if (objectIdKey) { + return objectIdKey.split('|') + } } } @@ -485,7 +503,7 @@ module.exports.createSiteData = (site, appState) => { value: siteData } } - console.log(`Warning: Can't create site data: ${JSON.stringify(site)}`) + console.log(`Ignoring entry because we can't create site data: ${JSON.stringify(site)}`) } /**