From 36776cd6156c8b39b4321d2fde547883375ca55d Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Sun, 31 May 2020 16:57:16 +0100 Subject: [PATCH] Handle resource conflicts --- CliClient/tests/synchronizer.js | 66 +++++++++++++++++-- README.md | 1 + ReactNativeClient/lib/models/Resource.js | 26 +++++++- .../lib/services/ResourceEditWatcher.ts | 2 +- ReactNativeClient/lib/shim-init-node.js | 26 +++++--- ReactNativeClient/lib/synchronizer.js | 33 +++++++++- readme/conflict.md | 17 +++++ 7 files changed, 153 insertions(+), 18 deletions(-) create mode 100644 readme/conflict.md diff --git a/CliClient/tests/synchronizer.js b/CliClient/tests/synchronizer.js index 550fb4523..82aa29a61 100644 --- a/CliClient/tests/synchronizer.js +++ b/CliClient/tests/synchronizer.js @@ -1007,7 +1007,7 @@ describe('synchronizer', function() { let resource1_2 = (await Resource.all())[0]; const modFile = `${__dirname}/tmp/test_mod.txt`; await shim.fsDriver().writeFile(modFile, '1234 MOD', 'utf8'); - await shim.updateResourceBlob(resource1_2.id, modFile); + await Resource.updateResourceBlobContent(resource1_2.id, modFile); const originalSize = resource1_2.size; resource1_2 = (await Resource.all())[0]; const newSize = resource1_2.size; @@ -1022,9 +1022,67 @@ describe('synchronizer', function() { await resourceFetcher().waitForAllFinished(); const resource1_1 = (await Resource.all())[0]; expect(resource1_1.size).toBe(newSize); - const resource1_1Path = Resource.fullPath(resource1_1); - const newContent = await shim.fsDriver().readFile(resource1_1Path, 'utf8'); - expect(newContent).toBe('1234 MOD'); + expect(await Resource.resourceBlobContent(resource1_1.id, 'utf8')).toBe('1234 MOD'); + })); + + it('should handle resource conflicts', asyncTest(async () => { + { + const tempFile = `${__dirname}/tmp/test.txt`; + await shim.fsDriver().writeFile(tempFile, '1234', 'utf8'); + const folder1 = await Folder.save({ title: 'folder1' }); + const note1 = await Note.save({ title: 'ma note', parent_id: folder1.id }); + await shim.attachFileToNote(note1, tempFile); + await synchronizer().start(); + } + + await switchClient(2); + + { + await synchronizer().start(); + await resourceFetcher().start(); + await resourceFetcher().waitForAllFinished(); + const resource = (await Resource.all())[0]; + const modFile2 = `${__dirname}/tmp/test_mod_2.txt`; + await shim.fsDriver().writeFile(modFile2, '1234 MOD 2', 'utf8'); + await Resource.updateResourceBlobContent(resource.id, modFile2); + await synchronizer().start(); + } + + await switchClient(1); + + { + // Going to modify a resource without syncing first, which will cause a conflict + const resource = (await Resource.all())[0]; + const modFile1 = `${__dirname}/tmp/test_mod_1.txt`; + await shim.fsDriver().writeFile(modFile1, '1234 MOD 1', 'utf8'); + await Resource.updateResourceBlobContent(resource.id, modFile1); + await synchronizer().start(); // CONFLICT + + // If we try to read the resource content now, it should throw because the local + // content has been moved to the conflict notebook, and the new local content + // has not been downloaded yet. + await checkThrowAsync(async () => await Resource.resourceBlobContent(resource.id)); + + // Now download resources, and our local content would have been overwritten by + // the content from client 2 + await resourceFetcher().start(); + await resourceFetcher().waitForAllFinished(); + const localContent = await Resource.resourceBlobContent(resource.id, 'utf8'); + expect(localContent).toBe('1234 MOD 2'); + + // Check that the Conflict note has been generated, with the conflict resource + // attached to it, and check that it has the original content. + const allNotes = await Note.all(); + expect(allNotes.length).toBe(2); + const conflictNote = allNotes.find((v) => { + return !!v.is_conflict; + }); + expect(!!conflictNote).toBe(true); + const resourceIds = await Note.linkedResourceIds(conflictNote.body); + expect(resourceIds.length).toBe(1); + const conflictContent = await Resource.resourceBlobContent(resourceIds[0], 'utf8'); + expect(conflictContent).toBe('1234 MOD 1'); + } })); it('should upload decrypted items to sync target after encryption disabled', asyncTest(async () => { diff --git a/README.md b/README.md index 093cb8d70..953d0d8e9 100644 --- a/README.md +++ b/README.md @@ -69,6 +69,7 @@ The Web Clipper is a browser extension that allows you to save web pages and scr - [Joplin Forum](https://discourse.joplinapp.org) - [Markdown Guide](https://github.com/laurent22/joplin/blob/master/readme/markdown.md) - [How to enable end-to-end encryption](https://github.com/laurent22/joplin/blob/master/readme/e2ee.md) + - [What is a conflict?](https://github.com/laurent22/joplin/blob/master/readme/conflict.md) - [End-to-end encryption spec](https://github.com/laurent22/joplin/blob/master/readme/spec.md) - [How to enable debug mode](https://github.com/laurent22/joplin/blob/master/readme/debugging.md) - [API documentation](https://github.com/laurent22/joplin/blob/master/readme/api.md) diff --git a/ReactNativeClient/lib/models/Resource.js b/ReactNativeClient/lib/models/Resource.js index 277d88b04..94389df2b 100644 --- a/ReactNativeClient/lib/models/Resource.js +++ b/ReactNativeClient/lib/models/Resource.js @@ -122,6 +122,11 @@ class Resource extends BaseItem { return 'ok'; } + static async requireIsReady(resource) { + const readyStatus = await Resource.readyStatus(resource); + if (readyStatus !== 'ok') throw new Error(`Resource is not ready. Status: ${readyStatus}`); + } + // For resources, we need to decrypt the item (metadata) and the resource binary blob. static async decrypt(item) { // The item might already be decrypted but not the blob (for instance if it crashes while @@ -236,7 +241,7 @@ class Resource extends BaseItem { return url.substr(2); } - static localState(resourceOrId) { + static async localState(resourceOrId) { return ResourceLocalState.byResourceId(typeof resourceOrId === 'object' ? resourceOrId.id : resourceOrId); } @@ -315,6 +320,25 @@ class Resource extends BaseItem { throw new Error(`Invalid status: ${status}`); } + static async updateResourceBlobContent(resourceId, newBlobFilePath) { + const resource = await Resource.load(resourceId); + await this.requireIsReady(resource); + + const fileStat = await this.fsDriver().stat(newBlobFilePath); + await this.fsDriver().copy(newBlobFilePath, Resource.fullPath(resource)); + + await Resource.save({ + id: resource.id, + size: fileStat.size, + }); + } + + static async resourceBlobContent(resourceId, encoding = 'Buffer') { + const resource = await Resource.load(resourceId); + await this.requireIsReady(resource); + return await this.fsDriver().readFile(Resource.fullPath(resource), encoding); + } + } Resource.IMAGE_MAX_DIMENSION = 1920; diff --git a/ReactNativeClient/lib/services/ResourceEditWatcher.ts b/ReactNativeClient/lib/services/ResourceEditWatcher.ts index 411ce06ce..ba334fc23 100644 --- a/ReactNativeClient/lib/services/ResourceEditWatcher.ts +++ b/ReactNativeClient/lib/services/ResourceEditWatcher.ts @@ -77,7 +77,7 @@ export default class ResourceEditWatcher { const makeSaveAction = (resourceId:string, path:string) => { return async () => { this.logger().info(`ResourceEditWatcher: Saving resource ${resourceId}`); - await shim.updateResourceBlob(resourceId, path); + await Resource.updateResourceBlob(resourceId, path); this.eventEmitter_.emit('resourceChange', { id: resourceId }); }; }; diff --git a/ReactNativeClient/lib/shim-init-node.js b/ReactNativeClient/lib/shim-init-node.js index 9f26b98ca..253433579 100644 --- a/ReactNativeClient/lib/shim-init-node.js +++ b/ReactNativeClient/lib/shim-init-node.js @@ -205,18 +205,26 @@ function shimInit() { return Resource.save(resource, { isNew: true }); }; - shim.updateResourceBlob = async function(resourceId, newBlobFilePath) { + shim.duplicateResource = async function(resourceId) { const resource = await Resource.load(resourceId); - const readyStatus = await Resource.readyStatus(resourceId); - if (readyStatus !== 'ok') throw new Error(`Cannot set resource blob because resource is not ready. Status: ${readyStatus}`); + const localState = await Resource.localState(resource); - const fileStat = await shim.fsDriver().stat(newBlobFilePath); - await shim.fsDriver().copy(newBlobFilePath, Resource.fullPath(resource)); + let newResource = { ...resource }; + delete newResource.id; + newResource = await Resource.save(newResource); - await Resource.save({ - id: resource.id, - size: fileStat.size, - }); + const newLocalState = { ...localState }; + newLocalState.resource_id = newResource.id; + delete newLocalState.id; + + await Resource.setLocalState(newResource, newLocalState); + + const sourcePath = Resource.fullPath(resource); + if (await shim.fsDriver().exists(sourcePath)) { + await shim.fsDriver().copy(sourcePath, Resource.fullPath(newResource)); + } + + return newResource; }; shim.attachFileToNoteBody = async function(noteBody, filePath, position = null, options = null) { diff --git a/ReactNativeClient/lib/synchronizer.js b/ReactNativeClient/lib/synchronizer.js index 01b99f8de..635d5280e 100644 --- a/ReactNativeClient/lib/synchronizer.js +++ b/ReactNativeClient/lib/synchronizer.js @@ -372,6 +372,12 @@ class Synchronizer { let reason = ''; let remoteContent = null; + const getConflictType = (conflictedItem) => { + if (conflictedItem.type_ === BaseModel.TYPE_NOTE) return 'noteConflict'; + if (conflictedItem.type_ === BaseModel.TYPE_RESOURCE) return 'resourceConflict'; + return 'itemConflict'; + }; + if (!remote) { if (!local.sync_time) { action = 'createRemote'; @@ -379,7 +385,7 @@ class Synchronizer { } else { // Note or item was modified after having been deleted remotely // "itemConflict" is for all the items except the notes, which are dealt with in a special way - action = local.type_ == BaseModel.TYPE_NOTE ? 'noteConflict' : 'itemConflict'; + action = getConflictType(local); reason = 'remote has been deleted, but local has changes'; } } else { @@ -416,7 +422,7 @@ class Synchronizer { // Since, in this loop, we are only dealing with items that require sync, if the // remote has been modified after the sync time, it means both items have been // modified and so there's a conflict. - action = local.type_ == BaseModel.TYPE_NOTE ? 'noteConflict' : 'itemConflict'; + action = getConflictType(local); reason = 'both remote and local have changes'; } else { action = 'updateRemote'; @@ -528,8 +534,29 @@ class Synchronizer { conflictedNote.is_conflict = 1; await Note.save(conflictedNote, { autoTimestamp: false, changeSource: ItemChange.SOURCE_SYNC }); } - + } else if (action == 'resourceConflict') { // ------------------------------------------------------------------------------ + // Unlike notes we always handle the conflict for resources + // ------------------------------------------------------------------------------ + + const conflictResource = await shim.duplicateResource(local.id); + + await Note.save({ + title: _('Attachment conflict: "%s"', local.title), + body: _('There was a [conflict](%s) on the attachment below.\n\n%s', 'https://joplinapp.org/conflict', Resource.markdownTag(conflictResource)), + is_conflict: 1, + }, { changeSource: ItemChange.SOURCE_SYNC }); + + // The local content we have is no longer valid and should be re-downloaded + await Resource.setLocalState(local.id, { + fetch_status: Resource.FETCH_STATUS_IDLE, + }); + } + + if (['noteConflict', '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. // Either copy the remote content to local or, if the remote content has // been deleted, delete the local content. // ------------------------------------------------------------------------------ diff --git a/readme/conflict.md b/readme/conflict.md new file mode 100644 index 000000000..4a3c31e25 --- /dev/null +++ b/readme/conflict.md @@ -0,0 +1,17 @@ +# What is a conflict? + +A conflict happens when one note or one attachment is modified in two different places, and then synchronised. In that case, it not possible to determine which version of the note or attachment you want to keep, and thus a conflict is generated. + +# What happens in case of a conflict? + +When Joplin detects a conflict, the local note is copied to the Conflict notebook so as to avoid any data loss. Then the remote note is downloaded. You can then inspect the notes in the Conflict notebook, compare it with your other version, and copy any change that might have been overwritten. + +# How to avoid conflicts? + +Conflicts are always annoying to deal with so it is best to avoid them as much as possible. + +For this, the best way is to synchronise as often as possible, so that you are always working with the latest versions of your notes. + +Joplin attempts to do this by uploading your latest changes within a few seconds. However, downloading changes is done at fixed intervals, every few minutes (as defined in the Config screen) and this is where conflicts may happen. It can also happen if one of your device did not have an internet connection for some times, and then synchronises. A bad internet connection can also hinder synchronisation because it will interrupt the process, which may have to restarted from the beginning to ensure consistency. + +So if you have not opened your application in a while, manually sync it and wait for it to complete, that way you are sure that whatever change you make will be on the latest version of the note.