From ad7fa5e096f3011afc8de38ba3d7d408bbe324ff Mon Sep 17 00:00:00 2001 From: azivner Date: Mon, 1 Jan 2018 22:28:19 -0500 Subject: [PATCH] better conflict detection --- public/javascripts/context_menu.js | 5 +- public/javascripts/note_tree.js | 6 -- public/javascripts/tree_changes.js | 28 +++++++-- routes/api/notes_move.js | 97 ++++++++++++++++++++++-------- views/login.ejs | 1 - 5 files changed, 97 insertions(+), 40 deletions(-) diff --git a/public/javascripts/context_menu.js b/public/javascripts/context_menu.js index 6f18e7ae7..a8782378f 100644 --- a/public/javascripts/context_menu.js +++ b/public/javascripts/context_menu.js @@ -11,7 +11,7 @@ const contextMenu = (function() { for (const nodeKey of clipboardIds) { const subjectNode = treeUtils.getNodeByKey(nodeKey); - treeChanges.moveAfterNode(subjectNode, node); + treeChanges.moveAfterNode([subjectNode], node); } clipboardIds = []; @@ -37,7 +37,7 @@ const contextMenu = (function() { for (const nodeKey of clipboardIds) { const subjectNode = treeUtils.getNodeByKey(nodeKey); - treeChanges.moveToNode(subjectNode, node); + treeChanges.moveToNode([subjectNode], node); } clipboardIds = []; @@ -47,7 +47,6 @@ const contextMenu = (function() { for (const noteId of clipboardIds) { treeChanges.cloneNoteTo(noteId, node.data.note_id); } - // copy will keep clipboardIds and clipboardMode so it's possible to paste into multiple places } else if (clipboardIds.length === 0) { diff --git a/public/javascripts/note_tree.js b/public/javascripts/note_tree.js index a09d98350..c91629a23 100644 --- a/public/javascripts/note_tree.js +++ b/public/javascripts/note_tree.js @@ -478,22 +478,16 @@ const noteTree = (function() { "ctrl+c": () => { contextMenu.copy(getSelectedNodes()); - showMessage("Note(s) copied into clipboard."); - return false; }, "ctrl+x": () => { contextMenu.cut(getSelectedNodes()); - showMessage("Note(s) cut into clipboard."); - return false; }, "ctrl+v": node => { contextMenu.pasteInto(node); - showMessage("Note(s) pasted from clipboard into current note."); - return false; }, "ctrl+return": node => { diff --git a/public/javascripts/tree_changes.js b/public/javascripts/tree_changes.js index 8c39c0709..d7f5af811 100644 --- a/public/javascripts/tree_changes.js +++ b/public/javascripts/tree_changes.js @@ -3,7 +3,12 @@ const treeChanges = (function() { async function moveBeforeNode(nodesToMove, beforeNode) { for (const nodeToMove of nodesToMove) { - await server.put('notes/' + nodeToMove.data.note_tree_id + '/move-before/' + beforeNode.data.note_tree_id); + const resp = await server.put('notes/' + nodeToMove.data.note_tree_id + '/move-before/' + beforeNode.data.note_tree_id); + + if (!resp.success) { + alert(resp.message); + return; + } changeNode(nodeToMove, node => node.moveTo(beforeNode, 'before')); } @@ -11,7 +16,12 @@ const treeChanges = (function() { async function moveAfterNode(nodesToMove, afterNode) { for (const nodeToMove of nodesToMove) { - await server.put('notes/' + nodeToMove.data.note_tree_id + '/move-after/' + afterNode.data.note_tree_id); + const resp = await server.put('notes/' + nodeToMove.data.note_tree_id + '/move-after/' + afterNode.data.note_tree_id); + + if (!resp.success) { + alert(resp.message); + return; + } changeNode(nodeToMove, node => node.moveTo(afterNode, 'after')); } @@ -31,7 +41,12 @@ const treeChanges = (function() { async function moveToNode(nodesToMove, toNode) { for (const nodeToMove of nodesToMove) { - await server.put('notes/' + nodeToMove.data.note_tree_id + '/move-to/' + toNode.data.note_id); + const resp = await server.put('notes/' + nodeToMove.data.note_tree_id + '/move-to/' + toNode.data.note_id); + + if (!resp.success) { + alert(resp.message); + return; + } changeNode(nodeToMove, node => { // first expand which will force lazy load and only then move the node @@ -100,7 +115,12 @@ const treeChanges = (function() { return; } - await server.put('notes/' + node.data.note_tree_id + '/move-after/' + node.getParent().data.note_tree_id); + const resp = await server.put('notes/' + node.data.note_tree_id + '/move-after/' + node.getParent().data.note_tree_id); + + if (!resp.success) { + alert(resp.message); + return; + } if (!isTopLevelNode(node) && node.getParent().getChildren().length <= 1) { node.getParent().folder = false; diff --git a/routes/api/notes_move.js b/routes/api/notes_move.js index 362042478..00d555308 100644 --- a/routes/api/notes_move.js +++ b/routes/api/notes_move.js @@ -12,6 +12,15 @@ router.put('/:noteTreeId/move-to/:parentNoteId', auth.checkApiAuth, async (req, const parentNoteId = req.params.parentNoteId; const sourceId = req.headers.source_id; + const noteToMove = await sql.getFirst("SELECT * FROM notes_tree WHERE note_tree_id = ?", [noteTreeId]); + + if (!await checkTreeCycle(parentNoteId, noteToMove.note_id)) { + return res.send({ + success: false, + message: 'Moving note here would create cycle.' + }); + } + const maxNotePos = await sql.getFirstValue('SELECT MAX(note_position) FROM notes_tree WHERE parent_note_id = ? AND is_deleted = 0', [parentNoteId]); const newNotePos = maxNotePos === null ? 0 : maxNotePos + 1; @@ -24,7 +33,7 @@ router.put('/:noteTreeId/move-to/:parentNoteId', auth.checkApiAuth, async (req, await sync_table.addNoteTreeSync(noteTreeId, sourceId); }); - res.send({}); + res.send({ success: true }); }); router.put('/:noteTreeId/move-before/:beforeNoteTreeId', auth.checkApiAuth, async (req, res, next) => { @@ -32,8 +41,16 @@ router.put('/:noteTreeId/move-before/:beforeNoteTreeId', auth.checkApiAuth, asyn const beforeNoteTreeId = req.params.beforeNoteTreeId; const sourceId = req.headers.source_id; + const noteToMove = await sql.getFirst("SELECT * FROM notes_tree WHERE note_tree_id = ?", [noteTreeId]); const beforeNote = await sql.getFirst("SELECT * FROM notes_tree WHERE note_tree_id = ?", [beforeNoteTreeId]); + if (!await checkTreeCycle(beforeNote.parent_note_id, noteToMove.note_id)) { + return res.send({ + success: false, + message: 'Moving note here would create cycle.' + }); + } + if (beforeNote) { await sql.doInTransaction(async () => { // we don't change date_modified so other changes are prioritized in case of conflict @@ -51,7 +68,7 @@ router.put('/:noteTreeId/move-before/:beforeNoteTreeId', auth.checkApiAuth, asyn await sync_table.addNoteTreeSync(noteTreeId, sourceId); }); - res.send({}); + res.send({ success: true }); } else { res.status(500).send("Before note " + beforeNoteTreeId + " doesn't exist."); @@ -63,8 +80,16 @@ router.put('/:noteTreeId/move-after/:afterNoteTreeId', auth.checkApiAuth, async const afterNoteTreeId = req.params.afterNoteTreeId; const sourceId = req.headers.source_id; + const noteToMove = await sql.getFirst("SELECT * FROM notes_tree WHERE note_tree_id = ?", [noteTreeId]); const afterNote = await sql.getFirst("SELECT * FROM notes_tree WHERE note_tree_id = ?", [afterNoteTreeId]); + if (!await checkTreeCycle(afterNote.parent_note_id, noteToMove.note_id)) { + return res.send({ + success: false, + message: 'Moving note here would create cycle.' + }); + } + if (afterNote) { await sql.doInTransaction(async () => { // we don't change date_modified so other changes are prioritized in case of conflict @@ -80,7 +105,7 @@ router.put('/:noteTreeId/move-after/:afterNoteTreeId', auth.checkApiAuth, async await sync_table.addNoteTreeSync(noteTreeId, sourceId); }); - res.send({}); + res.send({ success: true }); } else { res.status(500).send("After note " + afterNoteTreeId + " doesn't exist."); @@ -102,7 +127,7 @@ router.put('/:childNoteId/clone-to/:parentNoteId', auth.checkApiAuth, async (req }); } - if (!await checkCycle(parentNoteId, childNoteId)) { + if (!await checkTreeCycle(parentNoteId, childNoteId)) { return res.send({ success: false, message: 'Cloning note here would create cycle.' @@ -131,9 +156,7 @@ router.put('/:childNoteId/clone-to/:parentNoteId', auth.checkApiAuth, async (req await sql.execute("UPDATE notes_tree SET is_expanded = 1 WHERE note_id = ?", [parentNoteId]); }); - res.send({ - success: true - }); + res.send({ success: true }); }); router.put('/:noteId/clone-after/:afterNoteTreeId', auth.checkApiAuth, async (req, res, next) => { @@ -147,7 +170,7 @@ router.put('/:noteId/clone-after/:afterNoteTreeId', auth.checkApiAuth, async (re return res.status(500).send("After note " + afterNoteTreeId + " doesn't exist."); } - if (!await checkCycle(afterNote.parent_note_id, noteId)) { + if (!await checkTreeCycle(afterNote.parent_note_id, noteId)) { return res.send({ success: false, message: 'Cloning note here would create cycle.' @@ -186,29 +209,51 @@ router.put('/:noteId/clone-after/:afterNoteTreeId', auth.checkApiAuth, async (re await sync_table.addNoteTreeSync(noteTree.note_tree_id, sourceId); }); - res.send({ - success: true - }); + res.send({ success: true }); }); -async function checkCycle(parentNoteId, childNoteId) { - if (parentNoteId === 'root') { +async function loadSubTreeNoteIds(parentNoteId, subTreeNoteIds) { + subTreeNoteIds.push(parentNoteId); + + const children = await sql.getFirstColumn("SELECT note_id FROM notes_tree WHERE parent_note_id = ?", [parentNoteId]); + + for (const childNoteId of children) { + await loadSubTreeNoteIds(childNoteId, subTreeNoteIds); + } +} + +/** + * Tree cycle can be created when cloning or when moving existing clone. This method should detect both cases. + */ +async function checkTreeCycle(parentNoteId, childNoteId) { + const subTreeNoteIds = []; + + // we'll load the whole sub tree - because the cycle can start in one of the notes in the sub tree + await loadSubTreeNoteIds(childNoteId, subTreeNoteIds); + + async function checkTreeCycleInner(parentNoteId) { + if (parentNoteId === 'root') { + return true; + } + + if (subTreeNoteIds.includes(parentNoteId)) { + // while towards the root of the tree we encountered noteId which is already present in the subtree + // joining parentNoteId with childNoteId would then clearly create a cycle + return false; + } + + const parentNoteIds = await sql.getFirstColumn("SELECT DISTINCT parent_note_id FROM notes_tree WHERE note_id = ?", [parentNoteId]); + + for (const pid of parentNoteIds) { + if (!await checkTreeCycleInner(pid)) { + return false; + } + } + return true; } - if (parentNoteId === childNoteId) { - return false; - } - - const parentNoteIds = await sql.getFirstColumn("SELECT DISTINCT parent_note_id FROM notes_tree WHERE note_id = ?", [parentNoteId]); - - for (const pid of parentNoteIds) { - if (!await checkCycle(pid, childNoteId)) { - return false; - } - } - - return true; + return await checkTreeCycleInner(parentNoteId); } router.put('/:noteTreeId/expanded/:expanded', auth.checkApiAuth, async (req, res, next) => { diff --git a/views/login.ejs b/views/login.ejs index 55f4187e8..59684efa2 100644 --- a/views/login.ejs +++ b/views/login.ejs @@ -48,6 +48,5 @@ - \ No newline at end of file