From c66089a55ca7bff1265607ea9f34481b82605b1c Mon Sep 17 00:00:00 2001 From: Mo Bitar Date: Fri, 3 Nov 2017 18:24:09 -0500 Subject: [PATCH] Fix issue with race condition when alternating UUIDs, removing locally, and mapping --- .../javascripts/app/services/modelManager.js | 33 +++++++++++++++---- .../javascripts/app/services/syncManager.js | 22 ++++++++----- app/assets/templates/frontend/notes.html.haml | 2 +- app/assets/templates/frontend/tags.html.haml | 2 +- 4 files changed, 41 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/app/services/modelManager.js b/app/assets/javascripts/app/services/modelManager.js index 4586275ef..11b6e6480 100644 --- a/app/assets/javascripts/app/services/modelManager.js +++ b/app/assets/javascripts/app/services/modelManager.js @@ -6,6 +6,7 @@ class ModelManager { this.tags = []; this.itemSyncObservers = []; this.itemChangeObservers = []; + this.itemsPendingRemoval = []; this.items = []; this._extensions = []; this.acceptableContentTypes = ["Note", "Tag", "Extension", "SN|Editor", "SN|Theme", "SN|Component", "SF|Extension"]; @@ -30,7 +31,7 @@ class ModelManager { }) } - alternateUUIDForItem(item, callback) { + alternateUUIDForItem(item, callback, removeOriginal) { // we need to clone this item and give it a new uuid, then delete item with old uuid from db (you can't mofidy uuid's in our indexeddb setup) var newItem = this.createItem(item); @@ -41,12 +42,20 @@ class ModelManager { this.informModelsOfUUIDChangeForItem(newItem, item.uuid, newItem.uuid); - this.removeItemLocally(item, function(){ + var block = () => { this.addItem(newItem); newItem.setDirty(true); newItem.markAllReferencesDirty(); callback(); - }.bind(this)); + } + + if(removeOriginal) { + this.removeItemLocally(item, function(){ + block(); + }); + } else { + block(); + } } informModelsOfUUIDChangeForItem(newItem, oldUUID, newUUID) { @@ -94,15 +103,23 @@ class ModelManager { // first loop should add and process items for (var json_obj of items) { json_obj = _.omit(json_obj, omitFields || []) - var item = this.findItem(json_obj["uuid"]); - - _.omit(json_obj, omitFields); + var item = this.findItem(json_obj.uuid); if(item) { item.updateFromJSON(json_obj); } - if(json_obj["deleted"] == true || !_.includes(this.acceptableContentTypes, json_obj["content_type"])) { + if(!json_obj.content && !item) { + // A new incoming item must have a content field. If not, something has set an invalid state. + console.error("Content is missing for new item."); + } + + if(this.itemsPendingRemoval.includes(json_obj.uuid)) { + _.pull(this.itemsPendingRemoval, json_obj.uuid); + continue; + } + + if(json_obj.deleted == true || !_.includes(this.acceptableContentTypes, json_obj["content_type"])) { if(item) { allModels.push(item); this.removeItemLocally(item) @@ -302,6 +319,8 @@ class ModelManager { item.isBeingRemovedLocally(); + this.itemsPendingRemoval.push(item.uuid); + if(item.content_type == "Tag") { _.pull(this.tags, item); } else if(item.content_type == "Note") { diff --git a/app/assets/javascripts/app/services/syncManager.js b/app/assets/javascripts/app/services/syncManager.js index 7a998758e..23a333679 100644 --- a/app/assets/javascripts/app/services/syncManager.js +++ b/app/assets/javascripts/app/services/syncManager.js @@ -75,13 +75,16 @@ class SyncManager { Alternating here forces us to to create duplicates of the items instead. */ markAllItemsDirtyAndSaveOffline(callback, alternateUUIDs) { - var originalItems = this.modelManager.allItems; - var block = (items) => { - for(var item of items) { + // use a copy, as alternating uuid will affect array + var originalItems = this.modelManager.allItems.slice(); + + var block = () => { + var allItems = this.modelManager.allItems; + for(var item of allItems) { item.setDirty(true); } - this.writeItemsToLocalStorage(items, false, callback); + this.writeItemsToLocalStorage(allItems, false, callback); } if(alternateUUIDs) { @@ -90,18 +93,19 @@ class SyncManager { let alternateNextItem = () => { if(index >= originalItems.length) { // We don't use originalItems as altnerating UUID will have deleted them. - block(this.modelManager.allItems); + block(); return; } var item = originalItems[index]; - this.modelManager.alternateUUIDForItem(item, alternateNextItem); - ++index; + index++; + // false => dont remove original. We want to keep both copies + this.modelManager.alternateUUIDForItem(item, alternateNextItem, false); } alternateNextItem(); } else { - block(originalItems); + block(); } } @@ -361,7 +365,7 @@ class SyncManager { // UUID conflicts can occur if a user attempts to // import an old data archive with uuids from the old account into a new account handled = true; - this.modelManager.alternateUUIDForItem(item, handleNext); + this.modelManager.alternateUUIDForItem(item, handleNext, true); } else if(error.tag === "sync_conflict") { diff --git a/app/assets/templates/frontend/notes.html.haml b/app/assets/templates/frontend/notes.html.haml index f4d83b032..020cfa552 100644 --- a/app/assets/templates/frontend/notes.html.haml +++ b/app/assets/templates/frontend/notes.html.haml @@ -39,7 +39,7 @@ .scrollable .infinite-scroll{"infinite-scroll" => "ctrl.paginate()", "can-load" => "true", "threshold" => "200"} - .note{"ng-repeat" => "note in (ctrl.sortedNotes = (ctrl.tag.notes | filter: ctrl.filterNotes | sortBy: ctrl.sortBy| limitTo:ctrl.notesToDisplay))", + .note{"ng-repeat" => "note in (ctrl.sortedNotes = (ctrl.tag.notes | filter: ctrl.filterNotes | sortBy: ctrl.sortBy| limitTo:ctrl.notesToDisplay)) track by note.uuid", "ng-click" => "ctrl.selectNote(note)", "ng-class" => "{'selected' : ctrl.selectedNote == note}"} %strong.red.medium{"ng-if" => "note.conflict_of"} Conflicted copy %strong.red.medium{"ng-if" => "note.errorDecrypting"} Error decrypting diff --git a/app/assets/templates/frontend/tags.html.haml b/app/assets/templates/frontend/tags.html.haml index 121705900..4db18a408 100644 --- a/app/assets/templates/frontend/tags.html.haml +++ b/app/assets/templates/frontend/tags.html.haml @@ -10,7 +10,7 @@ .info %input.title{"ng-disabled" => "true", "ng-model" => "ctrl.allTag.title"} .count {{ctrl.noteCount(ctrl.allTag)}} - .tag{"ng-repeat" => "tag in ctrl.tags", "ng-click" => "ctrl.selectTag(tag)", "ng-class" => "{'selected' : ctrl.selectedTag == tag}"} + .tag{"ng-repeat" => "tag in ctrl.tags track by tag.uuid", "ng-click" => "ctrl.selectTag(tag)", "ng-class" => "{'selected' : ctrl.selectedTag == tag}"} .info %input.title{"ng-attr-id" => "tag-{{tag.uuid}}", "ng-click" => "ctrl.selectTag(tag)", "ng-model" => "tag.title", "ng-keyup" => "$event.keyCode == 13 && ctrl.saveTag($event, tag)", "mb-autofocus" => "true", "should-focus" => "ctrl.newTag || ctrl.editingTag == tag",