Merge pull request #9349 from brave/fix/9308

handle empty objects in sync DELETEs, refactor DELETE code

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:

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

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

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: _
 }

js/state/siteUtil.js

@@ -331,25 +331,52 @@ 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
  *
  * @param {Immutable.Map} state The application state Immutable map
  * @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)
       })
     })
   }

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,28 @@ 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)
+  }
+  return appState
+  // 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 +504,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)}`)
 }
 
 /**