From 37dbea161399f27c2e347d407f22884275f68bf8 Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Sat, 30 Dec 2023 19:05:41 +0000 Subject: [PATCH] Chore: Add more types to synchronizer and improved sync conflict log --- packages/lib/Synchronizer.ts | 41 ++++++++++--------- .../utils/handleConflictAction.ts | 14 ++++--- .../synchronizer/utils/syncDeleteStep.ts | 4 +- .../lib/services/synchronizer/utils/types.ts | 16 +++++++- 4 files changed, 46 insertions(+), 29 deletions(-) diff --git a/packages/lib/Synchronizer.ts b/packages/lib/Synchronizer.ts index e79db6b26..0e517a8ec 100644 --- a/packages/lib/Synchronizer.ts +++ b/packages/lib/Synchronizer.ts @@ -26,10 +26,11 @@ import { fetchSyncInfo, getActiveMasterKey, localSyncInfo, mergeSyncInfos, saveL import { getMasterPassword, setupAndDisableEncryption, setupAndEnableEncryption } from './services/e2ee/utils'; import { generateKeyPair } from './services/e2ee/ppk'; import syncDebugLog from './services/synchronizer/syncDebugLog'; -import handleConflictAction, { ConflictAction } from './services/synchronizer/utils/handleConflictAction'; +import handleConflictAction from './services/synchronizer/utils/handleConflictAction'; import resourceRemotePath from './services/synchronizer/utils/resourceRemotePath'; import syncDeleteStep from './services/synchronizer/utils/syncDeleteStep'; import { ErrorCode } from './errors'; +import { SyncAction } from './services/synchronizer/utils/types'; const { sprintf } = require('sprintf-js'); const { Dirnames } = require('./services/synchronizer/utils/types'); @@ -199,7 +200,7 @@ export default class Synchronizer { return lines; } - public logSyncOperation(action: string, local: any = null, remote: RemoteItem = null, message: string = null, actionCount = 1) { + public logSyncOperation(action: SyncAction | 'cancelling' | 'starting' | 'fetchingTotal' | 'fetchingProcessed' | 'finished', local: any = null, remote: RemoteItem = null, message: string = null, actionCount = 1) { const line = ['Sync']; line.push(action); if (message) line.push(message); @@ -577,20 +578,20 @@ export default class Synchronizer { if (donePaths.indexOf(path) >= 0) throw new JoplinError(sprintf('Processing a path that has already been done: %s. sync_time was not updated? Remote item has an updated_time in the future?', path), 'processingPathTwice'); const remote: RemoteItem = result.neverSyncedItemIds.includes(local.id) ? null : await this.apiCall('stat', path); - let action = null; + let action: SyncAction = null; let itemIsReadOnly = false; let reason = ''; let remoteContent = null; const getConflictType = (conflictedItem: any) => { - if (conflictedItem.type_ === BaseModel.TYPE_NOTE) return 'noteConflict'; - if (conflictedItem.type_ === BaseModel.TYPE_RESOURCE) return 'resourceConflict'; - return 'itemConflict'; + if (conflictedItem.type_ === BaseModel.TYPE_NOTE) return SyncAction.NoteConflict; + if (conflictedItem.type_ === BaseModel.TYPE_RESOURCE) return SyncAction.ResourceConflict; + return SyncAction.ItemConflict; }; if (!remote) { if (!local.sync_time) { - action = 'createRemote'; + action = SyncAction.CreateRemote; reason = 'remote does not exist, and local is new and has never been synced'; } else { // Note or item was modified after having been deleted remotely @@ -635,7 +636,7 @@ export default class Synchronizer { action = getConflictType(local); reason = 'both remote and local have changes'; } else { - action = 'updateRemote'; + action = SyncAction.UpdateRemote; reason = 'local has changes'; } } @@ -648,7 +649,7 @@ export default class Synchronizer { this.logSyncOperation(action, local, remote, reason); - if (local.type_ === BaseModel.TYPE_RESOURCE && (action === 'createRemote' || action === 'updateRemote')) { + if (local.type_ === BaseModel.TYPE_RESOURCE && (action === SyncAction.CreateRemote || action === SyncAction.UpdateRemote)) { const localState = await Resource.localState(local.id); if (localState.fetch_status !== Resource.FETCH_STATUS_DONE) { // This condition normally shouldn't happen @@ -722,7 +723,7 @@ export default class Synchronizer { } } - if (action === 'createRemote' || action === 'updateRemote') { + if (action === SyncAction.CreateRemote || action === SyncAction.UpdateRemote) { let canSync = true; try { if (this.testingHooks_.indexOf('notesRejectedByTarget') >= 0 && local.type_ === BaseModel.TYPE_NOTE) throw new JoplinError('Testing rejectedByTarget', 'rejectedByTarget'); @@ -766,7 +767,7 @@ export default class Synchronizer { } await handleConflictAction( - action as ConflictAction, + action, ItemClass, !!remote, remoteContent, @@ -875,7 +876,7 @@ export default class Synchronizer { const path = remote.path; const remoteId = BaseItem.pathToId(path); - let action = null; + let action: SyncAction = null; let reason = ''; let local = locals.find(l => l.id === remoteId); let ItemClass = null; @@ -884,7 +885,7 @@ export default class Synchronizer { try { if (!local) { if (remote.isDeleted !== true) { - action = 'createLocal'; + action = SyncAction.CreateLocal; reason = 'remote exists but local does not'; content = await loadContent(); ItemClass = content ? BaseItem.itemClass(content) : null; @@ -893,7 +894,7 @@ export default class Synchronizer { ItemClass = BaseItem.itemClass(local); local = ItemClass.filter(local); if (remote.isDeleted) { - action = 'deleteLocal'; + action = SyncAction.DeleteLocal; reason = 'remote has been deleted'; } else { if (this.api().supportsAccurateTimestamp && remote.jop_updated_time === local.updated_time) { @@ -901,7 +902,7 @@ export default class Synchronizer { } else { content = await loadContent(); if (content && content.updated_time > local.updated_time) { - action = 'updateLocal'; + action = SyncAction.UpdateLocal; reason = 'remote is more recent than local'; } } @@ -924,7 +925,7 @@ export default class Synchronizer { this.logSyncOperation(action, local, remote, reason); - if (action === 'createLocal' || action === 'updateLocal') { + if (action === SyncAction.CreateLocal || action === SyncAction.UpdateLocal) { if (content === null) { logger.warn(`Remote has been deleted between now and the delta() call? In that case it will be handled during the next sync: ${path}`); continue; @@ -944,10 +945,10 @@ export default class Synchronizer { nextQueries: BaseItem.updateSyncTimeQueries(syncTargetId, content, time.unixMs()), changeSource: ItemChange.SOURCE_SYNC, }; - if (action === 'createLocal') options.isNew = true; - if (action === 'updateLocal') options.oldItem = local; + if (action === SyncAction.CreateLocal) options.isNew = true; + if (action === SyncAction.UpdateLocal) options.oldItem = local; - const creatingOrUpdatingResource = content.type_ === BaseModel.TYPE_RESOURCE && (action === 'createLocal' || action === 'updateLocal'); + const creatingOrUpdatingResource = content.type_ === BaseModel.TYPE_RESOURCE && (action === SyncAction.CreateLocal || action === SyncAction.UpdateLocal); if (creatingOrUpdatingResource) { if (content.size >= this.maxResourceSize()) { @@ -991,7 +992,7 @@ export default class Synchronizer { // } if (content.encryption_applied) this.dispatch({ type: 'SYNC_GOT_ENCRYPTED_ITEM' }); - } else if (action === 'deleteLocal') { + } else if (action === SyncAction.DeleteLocal) { if (local.type_ === BaseModel.TYPE_FOLDER) { localFoldersToDelete.push(local); continue; diff --git a/packages/lib/services/synchronizer/utils/handleConflictAction.ts b/packages/lib/services/synchronizer/utils/handleConflictAction.ts index be8b86c8f..f96669065 100644 --- a/packages/lib/services/synchronizer/utils/handleConflictAction.ts +++ b/packages/lib/services/synchronizer/utils/handleConflictAction.ts @@ -5,16 +5,18 @@ import ItemChange from '../../../models/ItemChange'; import Note from '../../../models/Note'; import Resource from '../../../models/Resource'; import time from '../../../time'; +import { SyncAction, conflictActions } from './types'; const logger = Logger.create('handleConflictAction'); -export type ConflictAction = 'itemConflict' | 'noteConflict' | 'resourceConflict'; +export default async (action: SyncAction, ItemClass: any, remoteExists: boolean, remoteContent: any, local: any, syncTargetId: number, itemIsReadOnly: boolean, dispatch: Dispatch) => { + if (!conflictActions.includes(action)) return; -export default async (action: ConflictAction, ItemClass: any, remoteExists: boolean, remoteContent: any, local: any, syncTargetId: number, itemIsReadOnly: boolean, dispatch: Dispatch) => { logger.debug(`Handling conflict: ${action}`); + logger.debug('local:', local, 'remoteContent', remoteContent); logger.debug('remoteExists:', remoteExists); - if (action === 'itemConflict') { + if (action === SyncAction.ItemConflict) { // ------------------------------------------------------------------------------ // For non-note conflicts, we take the remote version (i.e. the version that was // synced first) and overwrite the local content. @@ -31,7 +33,7 @@ export default async (action: ConflictAction, ItemClass: any, remoteExists: bool trackDeleted: false, }); } - } else if (action === 'noteConflict') { + } else if (action === SyncAction.NoteConflict) { // ------------------------------------------------------------------------------ // First find out if the conflict matters. For example, if the conflict is on the title or body // we want to preserve all the changes. If it's on todo_completed it doesn't really matter @@ -51,7 +53,7 @@ export default async (action: ConflictAction, ItemClass: any, remoteExists: bool if (mustHandleConflict) { await Note.createConflictNote(local, ItemChange.SOURCE_SYNC); } - } else if (action === 'resourceConflict') { + } else if (action === SyncAction.ResourceConflict) { if (!remoteContent || Resource.mustHandleConflict(local, remoteContent)) { await Resource.createConflictResourceNote(local); @@ -66,7 +68,7 @@ export default async (action: ConflictAction, ItemClass: any, remoteExists: bool } } - if (['noteConflict', 'resourceConflict'].includes(action)) { + if ([SyncAction.NoteConflict, SyncAction.ResourceConflict].includes(action)) { // ------------------------------------------------------------------------------ // For note and resource conflicts, the creation of the conflict item is done // differently. However the way the local content is handled is the same. diff --git a/packages/lib/services/synchronizer/utils/syncDeleteStep.ts b/packages/lib/services/synchronizer/utils/syncDeleteStep.ts index c66083280..63f16b091 100644 --- a/packages/lib/services/synchronizer/utils/syncDeleteStep.ts +++ b/packages/lib/services/synchronizer/utils/syncDeleteStep.ts @@ -5,7 +5,7 @@ import ItemChange from '../../../models/ItemChange'; import Resource from '../../../models/Resource'; import time from '../../../time'; import resourceRemotePath from './resourceRemotePath'; -import { ApiCallFunction, LogSyncOperationFunction } from './types'; +import { ApiCallFunction, LogSyncOperationFunction, SyncAction } from './types'; export default async (syncTargetId: number, cancelling: boolean, logSyncOperation: LogSyncOperationFunction, apiCall: ApiCallFunction, dispatch: Dispatch) => { const deletedItems = await BaseItem.deletedItems(syncTargetId); @@ -24,7 +24,7 @@ export default async (syncTargetId: number, cancelling: boolean, logSyncOperatio await apiCall('delete', remoteContentPath); } - logSyncOperation('deleteRemote', null, { id: item.item_id }, 'local has been deleted'); + logSyncOperation(SyncAction.DeleteRemote, null, { id: item.item_id }, 'local has been deleted'); } catch (error) { if (error.code === 'isReadOnly') { let remoteContent = await apiCall('get', path); diff --git a/packages/lib/services/synchronizer/utils/types.ts b/packages/lib/services/synchronizer/utils/types.ts index 4fa6ae77a..faa4ec8bf 100644 --- a/packages/lib/services/synchronizer/utils/types.ts +++ b/packages/lib/services/synchronizer/utils/types.ts @@ -6,6 +6,20 @@ export enum Dirnames { Temp = 'temp', } -export type LogSyncOperationFunction = (action: string, local?: any, remote?: RemoteItem, message?: string, actionCount?: number)=> void; +export enum SyncAction { + ItemConflict = 'itemConflict', + NoteConflict = 'noteConflict', + ResourceConflict = 'resourceConflict', + CreateRemote = 'createRemote', + UpdateRemote = 'updateRemote', + DeleteRemote = 'deleteRemote', + CreateLocal = 'createLocal', + UpdateLocal = 'updateLocal', + DeleteLocal = 'deleteLocal', +} + +export type LogSyncOperationFunction = (action: SyncAction, local?: any, remote?: RemoteItem, message?: string, actionCount?: number)=> void; export type ApiCallFunction = (fnName: string, ...args: any[])=> Promise; + +export const conflictActions: SyncAction[] = [SyncAction.ItemConflict, SyncAction.NoteConflict, SyncAction.ResourceConflict];