diff --git a/ReactNativeClient/src/synchronizer.js b/ReactNativeClient/src/synchronizer.js index c5ccffdf7..8d7ea5f80 100644 --- a/ReactNativeClient/src/synchronizer.js +++ b/ReactNativeClient/src/synchronizer.js @@ -1,4 +1,3 @@ -import { Registry } from 'src/registry.js'; import { Log } from 'src/log.js'; import { Setting } from 'src/models/setting.js'; import { Change } from 'src/models/change.js'; @@ -27,130 +26,137 @@ class Synchronizer { return this.api_; } - processState(state) { - // if (this.state() == state) { - // Log.info('Sync: cannot switch to same state: ' + state); - // return; - // } - - Log.info('Sync: processing: ' + state); - this.state_ = state; - - if (state == 'downloadChanges') { - let maxRevId = null; - let hasMore = false; - this.api().get('synchronizer', { last_id: Setting.value('sync.lastRevId') }).then((syncOperations) => { - hasMore = syncOperations.has_more; - let chain = []; - for (let i = 0; i < syncOperations.items.length; i++) { - let syncOp = syncOperations.items[i]; - if (syncOp.id > maxRevId) maxRevId = syncOp.id; + processState_uploadChanges() { + Change.all().then((changes) => { + let mergedChanges = Change.mergeChanges(changes); + let chain = []; + let processedChangeIds = []; + for (let i = 0; i < mergedChanges.length; i++) { + let c = mergedChanges[i]; + chain.push(() => { + let p = null; let ItemClass = null; - if (syncOp.item_type == 'folder') { + let path = null; + if (c.item_type == BaseModel.ITEM_TYPE_FOLDER) { ItemClass = Folder; - } else if (syncOp.item_type == 'note') { + path = 'folders'; + } else if (c.item_type == BaseModel.ITEM_TYPE_NOTE) { ItemClass = Note; + path = 'notes'; } - if (syncOp.type == 'create') { - chain.push(() => { - let item = ItemClass.fromApiResult(syncOp.item); - // TODO: automatically handle NULL fields by checking type and default value of field - if ('parent_id' in item && !item.parent_id) item.parent_id = ''; - return ItemClass.save(item, { isNew: true, trackChanges: false }); + if (c.type == Change.TYPE_NOOP) { + p = Promise.resolve(); + } else if (c.type == Change.TYPE_CREATE) { + p = ItemClass.load(c.item_id).then((item) => { + return this.api().put(path + '/' + item.id, null, item); }); - } - - if (syncOp.type == 'update') { - chain.push(() => { - return ItemClass.load(syncOp.item_id).then((item) => { - if (!item) return; - item = ItemClass.applyPatch(item, syncOp.item); - return ItemClass.save(item, { trackChanges: false }); - }); + } else if (c.type == Change.TYPE_UPDATE) { + p = ItemClass.load(c.item_id).then((item) => { + return this.api().patch(path + '/' + item.id, null, item); }); + } else if (c.type == Change.TYPE_DELETE) { + p = this.api().delete(path + '/' + c.item_id); } - if (syncOp.type == 'delete') { - chain.push(() => { - return ItemClass.delete(syncOp.item_id, { trackChanges: false }); - }); - } - } - return promiseChain(chain); - }).then(() => { - Log.info('All items synced. has_more = ', hasMore); - if (maxRevId) { - Setting.setValue('sync.lastRevId', maxRevId); - return Setting.saveAll(); - } - }).then(() => { - if (hasMore) { - this.processState('downloadChanges'); - } else { - this.processState('uploadingChanges'); - } - }).catch((error) => { - Log.warn('Sync error', error); - }); - } else if (state == 'uploadingChanges') { - Change.all().then((changes) => { - let mergedChanges = Change.mergeChanges(changes); - let chain = []; - let processedChangeIds = []; - for (let i = 0; i < mergedChanges.length; i++) { - let c = mergedChanges[i]; - chain.push(() => { - let p = null; - - let ItemClass = null; - let path = null; - if (c.item_type == BaseModel.ITEM_TYPE_FOLDER) { - ItemClass = Folder; - path = 'folders'; - } else if (c.item_type == BaseModel.ITEM_TYPE_NOTE) { - ItemClass = Note; - path = 'notes'; - } - - if (c.type == Change.TYPE_NOOP) { - p = Promise.resolve(); - } else if (c.type == Change.TYPE_CREATE) { - p = ItemClass.load(c.item_id).then((item) => { - return this.api().put(path + '/' + item.id, null, item); - }); - } else if (c.type == Change.TYPE_UPDATE) { - p = ItemClass.load(c.item_id).then((item) => { - return this.api().patch(path + '/' + item.id, null, item); - }); - } else if (c.type == Change.TYPE_DELETE) { - p = this.api().delete(path + '/' + c.item_id); - } - - return p.then(() => { + return p.then(() => { + processedChangeIds = processedChangeIds.concat(c.ids); + }).catch((error) => { + Log.warn('Failed applying changes', c.ids, error.message, error.type); + // This is fine - trying to apply changes to an object that has been deleted + if (error.type == 'NotFoundException') { processedChangeIds = processedChangeIds.concat(c.ids); - }).catch((error) => { - Log.warn('Failed applying changes', c.ids, error.message, error.type); - // This is fine - trying to apply changes to an object that has been deleted - if (error.type == 'NotFoundException') { - processedChangeIds = processedChangeIds.concat(c.ids); - } else { - throw error; - } + } else { + throw error; + } + }); + }); + } + + return promiseChain(chain).catch((error) => { + Log.warn('Synchronization was interrupted due to an error:', error); + }).then(() => { + Log.info('IDs to delete: ', processedChangeIds); + Change.deleteMultiple(processedChangeIds); + }); + }).then(() => { + this.processState('downloadChanges'); + }); + } + + processState_downloadChanges() { + let maxRevId = null; + let hasMore = false; + this.api().get('synchronizer', { last_id: Setting.value('sync.lastRevId') }).then((syncOperations) => { + hasMore = syncOperations.has_more; + let chain = []; + for (let i = 0; i < syncOperations.items.length; i++) { + let syncOp = syncOperations.items[i]; + if (syncOp.id > maxRevId) maxRevId = syncOp.id; + + let ItemClass = null; + if (syncOp.item_type == 'folder') { + ItemClass = Folder; + } else if (syncOp.item_type == 'note') { + ItemClass = Note; + } + + if (syncOp.type == 'create') { + chain.push(() => { + let item = ItemClass.fromApiResult(syncOp.item); + // TODO: automatically handle NULL fields by checking type and default value of field + if ('parent_id' in item && !item.parent_id) item.parent_id = ''; + return ItemClass.save(item, { isNew: true, trackChanges: false }); + }); + } + + if (syncOp.type == 'update') { + chain.push(() => { + return ItemClass.load(syncOp.item_id).then((item) => { + if (!item) return; + item = ItemClass.applyPatch(item, syncOp.item); + return ItemClass.save(item, { trackChanges: false }); }); }); } - return promiseChain(chain).catch((error) => { - Log.warn('Synchronization was interrupted due to an error:', error); - }).then(() => { - Log.info('IDs to delete: ', processedChangeIds); - Change.deleteMultiple(processedChangeIds); - }); - }).then(() => { + if (syncOp.type == 'delete') { + chain.push(() => { + return ItemClass.delete(syncOp.item_id, { trackChanges: false }); + }); + } + } + return promiseChain(chain); + }).then(() => { + Log.info('All items synced. has_more = ', hasMore); + if (maxRevId) { + Setting.setValue('sync.lastRevId', maxRevId); + return Setting.saveAll(); + } + }).then(() => { + if (hasMore) { + this.processState('downloadChanges'); + } else { this.processState('idle'); - }); + } + }).catch((error) => { + Log.warn('Sync error', error); + }); + } + + processState(state) { + Log.info('Sync: processing: ' + state); + this.state_ = state; + + if (state == 'uploadChanges') { + processState_uploadChanges(); + } else if (state == 'downloadChanges') { + processState_downloadChanges(); + } else if (state == 'idle') { + // Nothing + } else { + throw new Error('Invalid state: ' . state); } } @@ -167,7 +173,7 @@ class Synchronizer { return; } - this.processState('downloadChanges'); + this.processState('uploadChanges'); } } diff --git a/run_tests.sh b/run_tests.sh index 1a6b6bede..57ea97870 100755 --- a/run_tests.sh +++ b/run_tests.sh @@ -1,2 +1,3 @@ #!/bin/bash -php phpunit-5.7.20.phar --bootstrap vendor/autoload.php tests/Model/ \ No newline at end of file +# php phpunit-5.7.20.phar --bootstrap vendor/autoload.php tests/Model/ +php phpunit-5.7.20.phar --filter testConflict ChangeTest tests/Model/ChangeTest.php --bootstrap vendor/autoload.php tests/Model/ \ No newline at end of file diff --git a/src/AppBundle/Model/Change.php b/src/AppBundle/Model/Change.php index 6196f8ed0..b11fdce03 100755 --- a/src/AppBundle/Model/Change.php +++ b/src/AppBundle/Model/Change.php @@ -47,13 +47,8 @@ class Change extends BaseModel { } $itemIdToChange[$change->item_id] = $change; - - // echo BaseModel::hex($change->item_id) . ' ' . $change->id . ' ' . Change::enumName('type', $change->type) . "\n"; } - // die(); - - $output = array(); foreach ($itemIdToChange as $itemId => $change) { if (in_array($itemId, $createdItems) && in_array($itemId, $deletedItems)) { diff --git a/src/AppBundle/Model/Note.php b/src/AppBundle/Model/Note.php index 879f2c0bd..bdcf9dc4f 100755 --- a/src/AppBundle/Model/Note.php +++ b/src/AppBundle/Model/Note.php @@ -30,7 +30,7 @@ class Note extends BaseItem { 'source_url' => null, ); - static public function filter($data) { + static public function filter($data, $keepId = false) { $output = parent::filter($data); if (array_key_exists('longitude', $output)) $output['longitude'] = (string)number_format($output['longitude'], 8); if (array_key_exists('latitude', $output)) $output['latitude'] = (string)number_format($output['latitude'], 8); diff --git a/tests/Model/ChangeTest.php b/tests/Model/ChangeTest.php index 1601c0d46..13ce43763 100755 --- a/tests/Model/ChangeTest.php +++ b/tests/Model/ChangeTest.php @@ -48,6 +48,66 @@ class ChangeTest extends BaseTestCase { $this->assertEquals($r, $text2); } + public function testConflict() { + // Scenario where two different clients change the same note at the same time. + // + // Client 1: 'abcd efgh ijkl' => 'XXXX' + // Client 2: 'abcd efgh ijkl' => 'YYYY' + // Expected: 'cd CLIENT1 efgh ijkl FROMCLIENT2' + + $text1 = 'abcd efgh ijkl'; + + $itemId = $this->createModelId('note'); + + $change = new Change(); + $change->user_id = $this->user()->id; + $change->client_id = $this->clientId(1); + $change->item_type = BaseItem::enumId('type', 'note'); + $change->item_field = 'body'; + $change->type = Change::enumId('type', 'create'); + $change->item_id = $itemId; + $change->createDelta($text1); + $change->save(); + + $changeId1 = $change->id; + + $text2 = 'XXXX'; + + $change = new Change(); + $change->user_id = $this->user()->id; + $change->client_id = $this->clientId(2); + $change->item_type = BaseItem::enumId('type', 'note'); + $change->item_field = 'body'; + $change->type = Change::enumId('type', 'update'); + $change->item_id = $itemId; + $change->previous_id = $changeId1; + $change->createDelta($text2); + $change->save(); + + $changeId2 = $change->id; + + $text3 = 'YYYY'; + + $change = new Change(); + $change->user_id = $this->user()->id; + $change->client_id = $this->clientId(1); + $change->item_type = BaseItem::enumId('type', 'note'); + $change->item_field = 'body'; + $change->type = Change::enumId('type', 'update'); + $change->item_id = $itemId; + $change->previous_id = $changeId1; + $change->createDelta($text3); + $change->save(); + + $changeId3 = $change->id; + + $r = Change::fullFieldText($itemId, 'body'); + + var_dump($r);die(); + + $this->assertEquals($r, 'cd CLIENT1 efgh ijkl FROMCLIENT2'); + } + public function testSame() { $note = new Note(); $note->fromPublicArray(array('body' => 'test'));