From ccf9882452d4f7e1311cac1601e741247436307c Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Fri, 15 Oct 2021 12:38:14 +0100 Subject: [PATCH] All: Ensure that shared notebook children are not deleted when shared, unshared and shared again, and a conflict happens --- packages/lib/BaseModel.ts | 8 +++- packages/lib/Synchronizer.ts | 29 +++--------- packages/lib/file-api.ts | 28 +++++++++++- packages/lib/models/BaseItem.ts | 2 +- .../synchronizer/Synchronizer.basics.test.ts | 45 +++++++++++++++++++ 5 files changed, 86 insertions(+), 26 deletions(-) diff --git a/packages/lib/BaseModel.ts b/packages/lib/BaseModel.ts index 773bb9ccf..e1f05d82e 100644 --- a/packages/lib/BaseModel.ts +++ b/packages/lib/BaseModel.ts @@ -28,9 +28,15 @@ export enum ModelType { export interface DeleteOptions { idFieldName?: string; - trackDeleted?: boolean; changeSource?: number; deleteChildren?: boolean; + + // By default the application tracks item deletions, so that they can be + // applied to the remote items during synchronisation. However, in some + // cases, we don't want this. In particular when an item is deleted via + // sync, we don't need to track the deletion, because the operation doesn't + // need to applied again on next sync. + trackDeleted?: boolean; } class BaseModel { diff --git a/packages/lib/Synchronizer.ts b/packages/lib/Synchronizer.ts index 4f92544c0..ae44c7d6d 100644 --- a/packages/lib/Synchronizer.ts +++ b/packages/lib/Synchronizer.ts @@ -20,7 +20,7 @@ import JoplinError from './JoplinError'; import ShareService from './services/share/ShareService'; import TaskQueue from './TaskQueue'; import ItemUploader from './services/synchronizer/ItemUploader'; -import { FileApi } from './file-api'; +import { FileApi, RemoteItem } from './file-api'; import JoplinDatabase from './JoplinDatabase'; import { fetchSyncInfo, getActiveMasterKey, localSyncInfo, mergeSyncInfos, saveLocalSyncInfo, SyncInfo, syncInfoEquals, uploadSyncInfo } from './services/synchronizer/syncInfoUtils'; import { getMasterPassword, setupAndDisableEncryption, setupAndEnableEncryption } from './services/e2ee/utils'; @@ -31,24 +31,6 @@ const { Dirnames } = require('./services/synchronizer/utils/types'); const logger = Logger.create('Synchronizer'); -interface RemoteItem { - id: string; - path?: string; - type_?: number; - isDeleted?: boolean; - - // This the time when the file was created on the server. It is used for - // example for the locking mechanim or any file that's not an actual Joplin - // item. - updated_time?: number; - - // This is the time that corresponds to the actual Joplin item updated_time - // value. A note is always uploaded with a delay so the server updated_time - // value will always be ahead. However for synchronising we need to know the - // exact Joplin item updated_time value. - jop_updated_time?: number; -} - function isCannotSyncError(error: any): boolean { if (!error) return false; if (['rejectedByTarget', 'fileNotFound'].indexOf(error.code) >= 0) return true; @@ -550,7 +532,7 @@ export default class Synchronizer { if (this.cancelling()) break; let local = locals[i]; - const ItemClass = BaseItem.itemClass(local); + const ItemClass: typeof BaseItem = BaseItem.itemClass(local); const path = BaseItem.systemPath(local); // Safety check to avoid infinite loops. @@ -744,7 +726,10 @@ export default class Synchronizer { const syncTimeQueries = BaseItem.updateSyncTimeQueries(syncTargetId, local, time.unixMs()); await ItemClass.save(local, { autoTimestamp: false, changeSource: ItemChange.SOURCE_SYNC, nextQueries: syncTimeQueries }); } else { - await ItemClass.delete(local.id, { changeSource: ItemChange.SOURCE_SYNC }); + await ItemClass.delete(local.id, { + changeSource: ItemChange.SOURCE_SYNC, + trackDeleted: false, + }); } } else if (action == 'noteConflict') { // ------------------------------------------------------------------------------ @@ -797,7 +782,7 @@ export default class Synchronizer { if (local.encryption_applied) this.dispatch({ type: 'SYNC_GOT_ENCRYPTED_ITEM' }); } else { // Remote no longer exists (note deleted) so delete local one too - await ItemClass.delete(local.id, { changeSource: ItemChange.SOURCE_SYNC }); + await ItemClass.delete(local.id, { changeSource: ItemChange.SOURCE_SYNC, trackDeleted: false }); } } diff --git a/packages/lib/file-api.ts b/packages/lib/file-api.ts index edd34cead..a9b46e187 100644 --- a/packages/lib/file-api.ts +++ b/packages/lib/file-api.ts @@ -16,6 +16,30 @@ export interface MultiPutItem { body: string; } +export interface RemoteItem { + id: string; + path?: string; + type_?: number; + isDeleted?: boolean; + + // This the time when the file was created on the server. It is used for + // example for the locking mechanim or any file that's not an actual Joplin + // item. + updated_time?: number; + + // This is the time that corresponds to the actual Joplin item updated_time + // value. A note is always uploaded with a delay so the server updated_time + // value will always be ahead. However for synchronising we need to know the + // exact Joplin item updated_time value. + jop_updated_time?: number; +} + +export interface PaginatedList { + items: RemoteItem[]; + hasMore: boolean; + context: any; +} + function requestCanBeRepeated(error: any) { const errorCode = typeof error === 'object' && error.code ? error.code : null; @@ -226,7 +250,7 @@ class FileApi { // DRIVER MUST RETURN PATHS RELATIVE TO `path` // eslint-disable-next-line no-unused-vars, @typescript-eslint/no-unused-vars - async list(path = '', options: any = null) { + public async list(path = '', options: any = null): Promise { if (!options) options = {}; if (!('includeHidden' in options)) options.includeHidden = false; if (!('context' in options)) options.context = null; @@ -235,7 +259,7 @@ class FileApi { logger.debug(`list ${this.baseDir()}`); - const result = await tryAndRepeat(() => this.driver_.list(this.fullPath(path), options), this.requestRepeatCount()); + const result: PaginatedList = await tryAndRepeat(() => this.driver_.list(this.fullPath(path), options), this.requestRepeatCount()); if (!options.includeHidden) { const temp = []; diff --git a/packages/lib/models/BaseItem.ts b/packages/lib/models/BaseItem.ts index b8ec26942..60203820b 100644 --- a/packages/lib/models/BaseItem.ts +++ b/packages/lib/models/BaseItem.ts @@ -236,7 +236,7 @@ export default class BaseItem extends BaseModel { return ItemClass.delete(id); } - static async delete(id: string, options: any = null) { + static async delete(id: string, options: DeleteOptions = null) { return this.batchDelete([id], options); } diff --git a/packages/lib/services/synchronizer/Synchronizer.basics.test.ts b/packages/lib/services/synchronizer/Synchronizer.basics.test.ts index e30a795cd..13d7c2cd5 100644 --- a/packages/lib/services/synchronizer/Synchronizer.basics.test.ts +++ b/packages/lib/services/synchronizer/Synchronizer.basics.test.ts @@ -394,4 +394,49 @@ describe('Synchronizer.basics', function() { expect((await Note.all()).length).toBe(11); })); + it('should not sync deletions that came via sync even when there is a conflict', (async () => { + // This test is mainly to simulate sharing, unsharing and sharing a note + // again. Previously, when doing so, the app would create deleted_items + // objects on the recipient when the owner unshares. It means that when + // sharing again, the recipient would apply the deletions and delete + // everything in the shared notebook. + // + // Specifically it was happening when a conflict was generated as a + // result of the items being deleted. + // + // - C1 creates a note and sync + // - C2 sync and get the note + // - C2 deletes the note and sync + // - C1 modify the note, and sync + // + // => A conflict is created. The note is deleted and a copy is created + // in the Conflict folder. + // + // After this, we recreate the note on the sync target (simulates the + // note being shared again), and we check that C2 doesn't attempt to + // delete that note. + + const note = await Note.save({}); + await synchronizerStart(); + const noteSerialized = await fileApi().get(`${note.id}.md`); + + await switchClient(2); + + await synchronizerStart(); + await Note.delete(note.id); + await synchronizerStart(); + + await switchClient(1); + + await Note.save({ id: note.id }); + await synchronizerStart(); + expect((await Note.all())[0].is_conflict).toBe(1); + await fileApi().put(`${note.id}.md`, noteSerialized); // Recreate the note - simulate sharing again. + await synchronizerStart(); + + // Check that the client didn't delete the note + const remotes = (await fileApi().list()).items; + expect(remotes.find(r => r.path === `${note.id}.md`)).toBeTruthy(); + })); + });