From a4374e3bdb6017b02b093885f9370e1fafd541a9 Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Tue, 20 Jun 2017 19:18:19 +0000 Subject: [PATCH] sync fixes --- CliClient/run_test.sh | 5 +- CliClient/tests/synchronizer.js | 128 ++++++++++++---------- CliClient/tests/test-utils.js | 5 + ReactNativeClient/src/base-model.js | 9 +- ReactNativeClient/src/database.js | 5 +- ReactNativeClient/src/models/base-item.js | 6 +- ReactNativeClient/src/models/folder.js | 21 +--- ReactNativeClient/src/models/note.js | 13 ++- ReactNativeClient/src/string-utils.js | 9 +- ReactNativeClient/src/synchronizer.js | 58 ++++++---- 10 files changed, 138 insertions(+), 121 deletions(-) diff --git a/CliClient/run_test.sh b/CliClient/run_test.sh index fc57c8380c..901a36791f 100755 --- a/CliClient/run_test.sh +++ b/CliClient/run_test.sh @@ -5,7 +5,6 @@ rm -f "$CLIENT_DIR/tests-build/src" mkdir -p "$CLIENT_DIR/tests-build/data" ln -s "$CLIENT_DIR/build/src" "$CLIENT_DIR/tests-build" -npm run build && NODE_PATH="$CLIENT_DIR/tests-build/" npm test tests-build/base-model.js - -#npm run build && NODE_PATH="$CLIENT_DIR/tests-build/" npm test tests-build/synchronizer.js +#npm run build && NODE_PATH="$CLIENT_DIR/tests-build/" npm test tests-build/base-model.js +npm run build && NODE_PATH="$CLIENT_DIR/tests-build/" npm test tests-build/synchronizer.js #npm run build && NODE_PATH="$CLIENT_DIR/tests-build/" npm test tests-build/services/note-folder-service.js \ No newline at end of file diff --git a/CliClient/tests/synchronizer.js b/CliClient/tests/synchronizer.js index 53a10783f7..b60ffaaa85 100644 --- a/CliClient/tests/synchronizer.js +++ b/CliClient/tests/synchronizer.js @@ -38,7 +38,7 @@ describe('Synchronizer', function() { beforeEach( async (done) => { await setupDatabaseAndSynchronizer(1); await setupDatabaseAndSynchronizer(2); - switchClient(1); + await switchClient(1); done(); }); @@ -77,7 +77,7 @@ describe('Synchronizer', function() { await Note.save({ title: "un", parent_id: folder.id }); await synchronizer().start(); - switchClient(2); + await switchClient(2); await synchronizer().start(); @@ -92,7 +92,7 @@ describe('Synchronizer', function() { let note1 = await Note.save({ title: "un", parent_id: folder1.id }); await synchronizer().start(); - switchClient(2); + await switchClient(2); await synchronizer().start(); @@ -108,7 +108,7 @@ describe('Synchronizer', function() { let files = await fileApi().list(); - switchClient(1); + await switchClient(1); await synchronizer().start(); @@ -126,7 +126,7 @@ describe('Synchronizer', function() { let note1 = await Note.save({ title: "un", parent_id: folder1.id }); await synchronizer().start(); - switchClient(2); + await switchClient(2); await synchronizer().start(); @@ -139,7 +139,7 @@ describe('Synchronizer', function() { await synchronizer().start(); - switchClient(1); + await switchClient(1); await sleep(0.1); @@ -150,19 +150,17 @@ describe('Synchronizer', function() { await synchronizer().start(); - let conflictFolder = await Folder.conflictFolder(); - let conflictedNotes = await Note.all(conflictFolder.id); + let conflictedNotes = await Note.conflicedNotes(); expect(conflictedNotes.length).toBe(1); - // Other than the id (since the conflicted note is a duplicate), parent_id (which is now the Conflicts folder) and sync_time, - // the note must be the same in every way, to make sure no data has been lost. + // Other than the id (since the conflicted note is a duplicate), and the is_conflict property + // the conflicted and original note must be the same in every way, to make sure no data has been lost. let conflictedNote = conflictedNotes[0]; expect(conflictedNote.id == note2conf.id).toBe(false); - expect(conflictedNote.parent_id == note2conf.parent_id).toBe(false); for (let n in conflictedNote) { if (!conflictedNote.hasOwnProperty(n)) continue; - if (n == 'id' || n == 'parent_id') continue; + if (n == 'id' || n == 'is_conflict') continue; expect(conflictedNote[n]).toBe(note2conf[n], 'Property: ' + n); } @@ -181,7 +179,7 @@ describe('Synchronizer', function() { let note1 = await Note.save({ title: "un", parent_id: folder1.id }); await synchronizer().start(); - switchClient(2); // ---------------------------------- + await switchClient(2); // ---------------------------------- await synchronizer().start(); @@ -194,7 +192,7 @@ describe('Synchronizer', function() { await synchronizer().start(); - switchClient(1); // ---------------------------------- + await switchClient(1); // ---------------------------------- await sleep(0.1); @@ -211,21 +209,68 @@ describe('Synchronizer', function() { done(); }); + it('should delete remote items', async (done) => { + let folder1 = await Folder.save({ title: "folder1" }); + let note1 = await Note.save({ title: "un", parent_id: folder1.id }); + await synchronizer().start(); + await switchClient(2); + await synchronizer().start(); + await sleep(0.1); + await Note.delete(note1.id); + await synchronizer().start(); + let files = await fileApi().list(); + expect(files.length).toBe(1); + expect(files[0].path).toBe(Folder.systemPath(folder1)); + let deletedItems = await BaseModel.deletedItems(); + expect(deletedItems.length).toBe(0); - // it('should delete local items', async (done) => { + done(); + }); + + it('should delete local items', async (done) => { + let folder1 = await Folder.save({ title: "folder1" }); + let note1 = await Note.save({ title: "un", parent_id: folder1.id }); + await synchronizer().start(); + + await switchClient(2); + + await synchronizer().start(); + + await sleep(0.1); + + await Note.delete(note1.id); + + await synchronizer().start(); + + await switchClient(1); + + await synchronizer().start(); + + let items = await Folder.all(true); + + expect(items.length).toBe(1); + + let deletedItems = await BaseModel.deletedItems(); + + expect(deletedItems.length).toBe(0); + + done(); + }); + + // it('should handle conflict when remote note is deleted then local note is modified', async (done) => { // let folder1 = await Folder.save({ title: "folder1" }); // let note1 = await Note.save({ title: "un", parent_id: folder1.id }); // await synchronizer().start(); - // switchClient(2); + // await switchClient(2); // await synchronizer().start(); @@ -235,57 +280,20 @@ describe('Synchronizer', function() { // await synchronizer().start(); - // switchClient(1); + // await switchClient(1); - // let files = await fileApi().list(); - // console.info(files); - - // // await synchronizer().start(); - - // // note1 = await Note.load(note1.id); - - // // expect(!note1).toBe(true); - - // done(); - // }); - - - - - - - - - - - - - // it('should delete remote items', async (done) => { - // let folder1 = await Folder.save({ title: "folder1" }); - // let note1 = await Note.save({ title: "un", parent_id: folder1.id }); - // await synchronizer().start(); - - // switchClient(2); + // await Note.save({ id: note1.id, title: 'Modified after having been deleted' }); // await synchronizer().start(); - // await sleep(0.1); + // // let items = await Folder.all(true); - // await Note.delete(note1.id); + // // expect(items.length).toBe(1); - // await synchronizer().start(); - - // switchClient(1); - - // let files = await fileApi().list(); - // console.info(files); - - // await synchronizer().start(); - - // note1 = await Note.load(note1.id); - - // expect(!note1).toBe(true); + // // let deletedItems = await BaseModel.deletedItems(); + // // expect(deletedItems.length).toBe(0); + // done(); // }); diff --git a/CliClient/tests/test-utils.js b/CliClient/tests/test-utils.js index 14dda329b8..3a5140bec7 100644 --- a/CliClient/tests/test-utils.js +++ b/CliClient/tests/test-utils.js @@ -24,11 +24,16 @@ function sleep(n) { } function switchClient(id) { + Setting.saveAll(); + currentClient_ = id; BaseModel.db_ = databases_[id]; Folder.db_ = databases_[id]; Note.db_ = databases_[id]; BaseItem.db_ = databases_[id]; + Setting.db_ = databases_[id]; + + return Setting.load(); } function clearDatabase(id = null) { diff --git a/ReactNativeClient/src/base-model.js b/ReactNativeClient/src/base-model.js index 9f63ae7a86..a6352c4f53 100644 --- a/ReactNativeClient/src/base-model.js +++ b/ReactNativeClient/src/base-model.js @@ -103,6 +103,7 @@ class BaseModel { options = Object.assign({}, options); } if (!('trackChanges' in options)) options.trackChanges = true; + if (!('trackDeleted' in options)) options.trackDeleted = null; if (!('isNew' in options)) options.isNew = 'auto'; if (!('autoTimestamp' in options)) options.autoTimestamp = true; return options; @@ -247,6 +248,10 @@ class BaseModel { return this.db().selectAll('SELECT * FROM deleted_items'); } + static remoteDeletedItem(itemId) { + return this.db().exec('DELETE FROM deleted_items WHERE item_id = ?', [itemId]); + } + static delete(id, options = null) { options = this.modOptions(options); @@ -256,7 +261,9 @@ class BaseModel { } return this.db().exec('DELETE FROM ' + this.tableName() + ' WHERE id = ?', [id]).then(() => { - if (this.trackDeleted()) { + let trackDeleted = this.trackDeleted(); + if (options.trackDeleted !== null) trackDeleted = options.trackDeleted; + if (trackDeleted) { return this.db().exec('INSERT INTO deleted_items (item_type, item_id, deleted_time) VALUES (?, ?, ?)', [this.itemType(), id, time.unixMs()]); } diff --git a/ReactNativeClient/src/database.js b/ReactNativeClient/src/database.js index f019458dd4..821d4de653 100644 --- a/ReactNativeClient/src/database.js +++ b/ReactNativeClient/src/database.js @@ -22,6 +22,7 @@ CREATE TABLE notes ( created_time INT NOT NULL DEFAULT 0, updated_time INT NOT NULL DEFAULT 0, sync_time INT NOT NULL DEFAULT 0, + is_conflict INT NOT NULL DEFAULT 0, latitude NUMERIC NOT NULL DEFAULT 0, longitude NUMERIC NOT NULL DEFAULT 0, altitude NUMERIC NOT NULL DEFAULT 0, @@ -37,7 +38,7 @@ CREATE TABLE notes ( ); CREATE TABLE deleted_items ( - id TEXT PRIMARY KEY, + id INTEGER PRIMARY KEY, item_type INT NOT NULL, item_id TEXT NOT NULL, deleted_time INT NOT NULL @@ -267,6 +268,7 @@ class Database { let params = []; for (let key in data) { if (!data.hasOwnProperty(key)) continue; + if (key[key.length - 1] == '_') continue; if (keySql != '') keySql += ', '; if (valueSql != '') valueSql += ', '; keySql += '`' + key + '`'; @@ -286,6 +288,7 @@ class Database { let params = []; for (let key in data) { if (!data.hasOwnProperty(key)) continue; + if (key[key.length - 1] == '_') continue; if (sql != '') sql += ', '; sql += '`' + key + '`=?'; params.push(data[key]); diff --git a/ReactNativeClient/src/models/base-item.js b/ReactNativeClient/src/models/base-item.js index 4483517001..9bed85a098 100644 --- a/ReactNativeClient/src/models/base-item.js +++ b/ReactNativeClient/src/models/base-item.js @@ -2,7 +2,6 @@ import { BaseModel } from 'src/base-model.js'; import { Note } from 'src/models/note.js'; import { Folder } from 'src/models/folder.js'; import { Setting } from 'src/models/setting.js'; -import { folderItemFilename } from 'src/string-utils.js' import { Database } from 'src/database.js'; import { time } from 'src/time-utils.js'; import moment from 'moment'; @@ -13,8 +12,9 @@ class BaseItem extends BaseModel { return true; } - static systemPath(item) { - return folderItemFilename(item) + '.md'; + static systemPath(itemOrId) { + if (typeof itemOrId === 'string') return itemOrId + '.md'; + return itemOrId.id + '.md'; } static itemClass(item) { diff --git a/ReactNativeClient/src/models/folder.js b/ReactNativeClient/src/models/folder.js index ca09af23b1..30f8990d72 100644 --- a/ReactNativeClient/src/models/folder.js +++ b/ReactNativeClient/src/models/folder.js @@ -3,7 +3,6 @@ import { Log } from 'src/log.js'; import { promiseChain } from 'src/promise-utils.js'; import { Note } from 'src/models/note.js'; import { Setting } from 'src/models/setting.js'; -import { folderItemFilename } from 'src/string-utils.js' import { _ } from 'src/locale.js'; import moment from 'moment'; import { BaseItem } from 'src/models/base-item.js'; @@ -38,7 +37,7 @@ class Folder extends BaseItem { } static syncedNoteIds() { - return this.db().selectAll('SELECT id FROM notes WHERE sync_time > 0').then((rows) => { + return this.db().selectAll('SELECT id FROM notes WHERE is_conflict = 0 AND sync_time > 0').then((rows) => { let output = []; for (let i = 0; i < rows.length; i++) { output.push(rows[i].id); @@ -48,7 +47,7 @@ class Folder extends BaseItem { } static noteIds(parentId) { - return this.db().selectAll('SELECT id FROM notes WHERE parent_id = ?', [parentId]).then((rows) => { + return this.db().selectAll('SELECT id FROM notes WHERE is_conflict = 0 AND parent_id = ?', [parentId]).then((rows) => { let output = []; for (let i = 0; i < rows.length; i++) { let row = rows[i]; @@ -87,29 +86,17 @@ class Folder extends BaseItem { } static loadNoteByField(folderId, field, value) { - return this.modelSelectAll('SELECT * FROM notes WHERE `parent_id` = ? AND `' + field + '` = ?', [folderId, value]); + return this.modelSelectAll('SELECT * FROM notes WHERE is_conflict = 0 AND `parent_id` = ? AND `' + field + '` = ?', [folderId, value]); } static async all(includeNotes = false) { let folders = await Folder.modelSelectAll('SELECT * FROM folders'); if (!includeNotes) return folders; - let notes = await Note.modelSelectAll('SELECT * FROM notes'); + let notes = await Note.modelSelectAll('SELECT * FROM notes WHERE is_conflict = 0'); return folders.concat(notes); } - static conflictFolder() { - let folderId = Setting.value('sync.conflictFolderId'); - if (!folderId) { - return Folder.save({ title: _('Conflicts') }).then((folder) => { - Setting.setValue('sync.conflictFolderId', folder.id); - return folder; - }); - } - - return Folder.load(folderId); - } - static save(o, options = null) { return Folder.loadByField('title', o.title).then((existingFolder) => { if (existingFolder && existingFolder.id != o.id) throw new Error(_('A folder with title "%s" already exists', o.title)); diff --git a/ReactNativeClient/src/models/note.js b/ReactNativeClient/src/models/note.js index c75043fbfd..8bf5c472fd 100644 --- a/ReactNativeClient/src/models/note.js +++ b/ReactNativeClient/src/models/note.js @@ -2,7 +2,6 @@ import { BaseModel } from 'src/base-model.js'; import { Log } from 'src/log.js'; import { Folder } from 'src/models/folder.js'; import { Geolocation } from 'src/geolocation.js'; -import { folderItemFilename } from 'src/string-utils.js' import { BaseItem } from 'src/models/base-item.js'; import moment from 'moment'; @@ -45,13 +44,15 @@ class Note extends BaseItem { } static previews(parentId) { - return this.modelSelectAll('SELECT ' + this.previewFieldsSql() + ' FROM notes WHERE parent_id = ?', [parentId]); - //return this.db().selectAll('SELECT ' + this.previewFieldsSql() + ' FROM notes WHERE parent_id = ?', [parentId]); + return this.modelSelectAll('SELECT ' + this.previewFieldsSql() + ' FROM is_conflict = 0 AND notes WHERE parent_id = ?', [parentId]); } static preview(noteId) { - return this.modelSelectOne('SELECT ' + this.previewFieldsSql() + ' FROM notes WHERE id = ?', [noteId]); - //return this.db().selectOne('SELECT ' + this.previewFieldsSql() + ' FROM notes WHERE id = ?', [noteId]); + return this.modelSelectOne('SELECT ' + this.previewFieldsSql() + ' FROM is_conflict = 0 AND notes WHERE id = ?', [noteId]); + } + + static conflicedNotes() { + return this.modelSelectAll('SELECT * FROM notes WHERE is_conflict = 1'); } static updateGeolocation(noteId) { @@ -74,7 +75,7 @@ class Note extends BaseItem { } static all(parentId) { - return this.modelSelectAll('SELECT * FROM notes WHERE parent_id = ?', [parentId]); + return this.modelSelectAll('SELECT * FROM notes WHERE is_conflict = 0 AND parent_id = ?', [parentId]); } static save(o, options = null) { diff --git a/ReactNativeClient/src/string-utils.js b/ReactNativeClient/src/string-utils.js index 0fe1131297..e080a03b52 100644 --- a/ReactNativeClient/src/string-utils.js +++ b/ReactNativeClient/src/string-utils.js @@ -113,11 +113,4 @@ function escapeFilename(s, maxLength = 32) { return output.substr(0, maxLength); } -function folderItemFilename(item) { - return item.id; - // let output = escapeFilename(item.title).trim(); - // if (!output.length) output = '_'; - // return output + '.' + item.id.substr(0, 7); -} - -export { removeDiacritics, escapeFilename, folderItemFilename }; \ No newline at end of file +export { removeDiacritics, escapeFilename }; \ No newline at end of file diff --git a/ReactNativeClient/src/synchronizer.js b/ReactNativeClient/src/synchronizer.js index 072a864d09..5b110bb69b 100644 --- a/ReactNativeClient/src/synchronizer.js +++ b/ReactNativeClient/src/synchronizer.js @@ -69,35 +69,36 @@ class Synchronizer { console.info('Sync action (1): ' + action); if (action == 'createRemote' || action == 'updateRemote') { + await this.api().put(path, content); await this.api().setTimestamp(path, local.updated_time); + + await ItemClass.save({ id: local.id, sync_time: time.unixMs(), type_: local.type_ }, { autoTimestamp: false }); + } else if (action == 'folderConflict') { + let remoteContent = await this.api().get(path); local = BaseItem.unserialize(remoteContent); - updateSyncTimeOnly = false; + + local.sync_time = time.unixMs(); + await ItemClass.save(local, { autoTimestamp: false }); + } else if (action == 'noteConflict') { + // - Create a duplicate of local note into Conflicts folder (to preserve the user's changes) // - Overwrite local note with remote note - let conflictFolder = await Folder.conflictFolder(); let conflictedNote = Object.assign({}, local); delete conflictedNote.id; - conflictedNote.parent_id = conflictFolder.id; + conflictedNote.is_conflict = 1; await Note.save(conflictedNote, { autoTimestamp: false }); let remoteContent = await this.api().get(path); local = BaseItem.unserialize(remoteContent); - updateSyncTimeOnly = false; - } - let newLocal = null; - if (updateSyncTimeOnly) { - newLocal = { id: local.id, sync_time: time.unixMs(), type_: local.type_ }; - } else { - newLocal = local; - newLocal.sync_time = time.unixMs(); - } + local.sync_time = time.unixMs(); + await ItemClass.save(local, { autoTimestamp: false }); - await ItemClass.save(newLocal, { autoTimestamp: false }); + } donePaths.push(path); } @@ -106,7 +107,20 @@ class Synchronizer { } // ------------------------------------------------------------------------ - // Then, loop through all the remote items, find those that + // Delete the remote items that have been deleted locally. + // ------------------------------------------------------------------------ + + let deletedItems = await BaseModel.deletedItems(); + for (let i = 0; i < deletedItems.length; i++) { + let item = deletedItems[i]; + let path = BaseItem.systemPath(item.item_id) + console.info('Sync action (2): deleteRemote'); + await this.api().delete(path); + await BaseModel.remoteDeletedItem(item.item_id); + } + + // ------------------------------------------------------------------------ + // Loop through all the remote items, find those that // have been updated, and apply the changes to local. // ------------------------------------------------------------------------ @@ -133,7 +147,7 @@ class Synchronizer { if (!action) continue; - console.info('Sync action (2): ' + action); + console.info('Sync action (3): ' + action); if (action == 'createLocal' || action == 'updateLocal') { let content = await this.api().get(path); @@ -156,13 +170,13 @@ class Synchronizer { // means the item has been deleted. // ------------------------------------------------------------------------ - // let noteIds = Folder.syncedNoteIds(); - // for (let i = 0; i < noteIds.length; i++) { - // if (remoteIds.indexOf(noteIds[i]) < 0) { - // console.info('Sync action (3): Delete ' + noteIds[i]); - // await Note.delete(noteIds[i]); - // } - // } + let noteIds = await Folder.syncedNoteIds(); + for (let i = 0; i < noteIds.length; i++) { + if (remoteIds.indexOf(noteIds[i]) < 0) { + console.info('Sync action (4): deleteLocal: ' + noteIds[i]); + await Note.delete(noteIds[i], { trackDeleted: false }); + } + } return Promise.resolve(); }