From e886be8f04a1192b3ce5f50e4e745953381ec7a7 Mon Sep 17 00:00:00 2001 From: ThaumRystra <9525416+ThaumRystra@users.noreply.github.com> Date: Sun, 31 Dec 2023 18:06:31 +0200 Subject: [PATCH] Fixed showstopping bugs with tree organize functions --- app/imports/api/library/LibraryNodes.js | 7 + .../api/parenting/parentingFunctions.test.ts | 107 ++++++- .../api/parenting/parentingFunctions.ts | 275 +++++++++++------- .../api/users/methods/linkWithPatreon.js | 1 - .../client/ui/components/tree/TreeNode.vue | 5 +- .../ui/components/tree/TreeNodeList.vue | 47 +-- .../CreaturePropertiesTree.vue | 31 +- .../ui/library/LibraryContentsContainer.vue | 35 +-- app/imports/migrations/server/dbv3/dbv3.ts | 16 +- .../server/config/accountsEmailConfig.js | 1 - app/imports/server/publications/library.js | 8 +- .../server/publications/searchLibraryNodes.js | 8 +- 12 files changed, 360 insertions(+), 181 deletions(-) diff --git a/app/imports/api/library/LibraryNodes.js b/app/imports/api/library/LibraryNodes.js index a55bd74f..56373c55 100644 --- a/app/imports/api/library/LibraryNodes.js +++ b/app/imports/api/library/LibraryNodes.js @@ -115,6 +115,13 @@ for (let key in propertySchemasIndex) { schema.extend(propertySchemasIndex[key]); schema.extend(ChildSchema); schema.extend(SoftRemovableSchema); + // Use the any schema as a default schema for the collection + if (key === 'any') { + // @ts-expect-error don't have types for .attachSchema + LibraryNodes.attachSchema(schema); + } + // TODO make this an else branch and remove all {selector: {type: any}} options + // @ts-expect-error don't have types for .attachSchema LibraryNodes.attachSchema(schema, { selector: { type: key } }); diff --git a/app/imports/api/parenting/parentingFunctions.test.ts b/app/imports/api/parenting/parentingFunctions.test.ts index ab42aaad..eb5e533b 100644 --- a/app/imports/api/parenting/parentingFunctions.test.ts +++ b/app/imports/api/parenting/parentingFunctions.test.ts @@ -1,5 +1,5 @@ import '/imports/api/simpleSchemaConfig'; -import { docsToForest, calculateNestedSetOperations, getFilter, moveDocWithinRoot } from '/imports/api/parenting/parentingFunctions' +import { docsToForest, calculateNestedSetOperations, getFilter, moveDocWithinRoot, moveDocBetweenRoots } from '/imports/api/parenting/parentingFunctions' import { TreeDoc } from '/imports/api/parenting/ChildSchema'; import { assert } from 'chai'; @@ -188,7 +188,7 @@ describe('Document tree filters can fetch other documents based on their positio }); }); -describe('Document can be moved without breaking the tree', function () { +describe('Document can be moved withing root without breaking the tree', function () { /** * Test the following structure * @@ -250,6 +250,37 @@ describe('Document can be moved without breaking the tree', function () { doc('Mains', 20, 21, 'Vegetarian'), ]); }); + it('can move a document within its parent to the start of the tree', async function () { + const videosDoc = await treeCollection.findOneAsync({ _id: 'Videos' }); + if (!videosDoc) throw new Error('Languages doc not found'); + await moveDocWithinRoot(videosDoc, treeCollection, 0.5); + /** + * Expected resulting structure + * + * 1 Videos 12 13 Books 24 + * ┃ ┃ + * 2 Cooking 11 14 Programming 23 + * ┏━━━━━━━━┻━━━━━━━━━┓ ┏━━━━━━━━┻━━━━━━━━━┓ + * 3 Meat 4 5 Vegetarian 10 15 Languages 16 17 Databases 22 + * ┏━━━━━━━┻━━━━━━━┓ ┏━━━━━━━┻━━━━━━━┓ + * 6 Pasta 7 8 Mains 9 18 MongoDB 19 20 dbm 21 + **/ + const docs = await treeCollection.find({}, { sort: { left: 1 } }).fetchAsync(); + assert.deepEqual(docs, [ + doc('Videos', 1, 12, undefined), + doc('Cooking', 2, 11, 'Videos'), + doc('Meat', 3, 4, 'Cooking'), + doc('Vegetarian', 5, 10, 'Cooking'), + doc('Pasta', 6, 7, 'Vegetarian'), + doc('Mains', 8, 9, 'Vegetarian'), + doc('Books', 13, 24, undefined), + doc('Programming', 14, 23, 'Books'), + doc('Languages', 15, 16, 'Programming'), + doc('Databases', 17, 22, 'Programming'), + doc('MongoDB', 18, 19, 'Databases'), + doc('dbm', 20, 21, 'Databases'), + ]); + }); it('can move a document to a whole new parent', async function () { const videos = await treeCollection.findOneAsync({ _id: 'Videos' }); if (!videos) throw new Error('Videos doc not found'); @@ -291,4 +322,74 @@ describe('Document can be moved without breaking the tree', function () { }); -// TODO test moving between roots +describe('Documents can be moved between roots without breaking the trees', function () { + /** + * Test the following structure + * Root 1 Root 2 + * 1 Books 12 1 Videos 12 + * ┃ ┃ + * 2 Programming 11 2 Cooking 11 + * ┏━━━━━━━━┻━━━━━━━━━┓ ┏━━━━━━━━┻━━━━━━━━━┓ + * 3 Languages 4 5 Databases 10 3 Meat 4 5 Vegetarian 10 + * ┏━━━━━━━┻━━━━━━━┓ ┏━━━━━━━┻━━━━━━━┓ + * 6 MongoDB 7 8 dbm 9 6 Pasta 7 8 Mains 9 + */ + const treeCollection: Mongo.Collection = new Mongo.Collection('treeDocsMoveBetween'); + const doc = function (_id, left, right, parentId, rootId): TreeDoc { + const doc = { _id, root: { id: rootId, collection: 'someCol' }, left, right, parentId }; + if (!parentId) delete doc.parentId; + return doc; + } + beforeEach(function () { + treeCollection.remove({}); + [ + doc('Books', 1, 12, undefined, 'root1'), + doc('Programming', 2, 11, 'Books', 'root1'), + doc('Languages', 3, 4, 'Programming', 'root1'), + doc('Databases', 5, 10, 'Programming', 'root1'), + doc('MongoDB', 6, 7, 'Databases', 'root1'), + doc('dbm', 8, 9, 'Databases', 'root1'), + doc('Videos', 1, 12, undefined, 'root2'), + doc('Cooking', 2, 11, 'Videos', 'root2'), + doc('Meat', 3, 4, 'Cooking', 'root2'), + doc('Vegetarian', 5, 10, 'Cooking', 'root2'), + doc('Pasta', 6, 7, 'Vegetarian', 'root2'), + doc('Mains', 8, 9, 'Vegetarian', 'root2'), + ].map(doc => { + return treeCollection.insert(doc); + }); + }); + it('can move a document from one root to another', async function () { + /** + * Move veg to languages + * Root 1 Root 2 + * 1 Books 18 1 Videos 6 + * ┃ ┃ + * 2 Programming 17 2 Cooking 5 + * ┏━━━━━━━━┻━━━━━━━━━━━━━━━━━━━━━━┓ ┃ + * 3 Languages 10 11 Databases 16 3 Meat 4 + * ┃ ┏━━━━━━━┻━━━━━━┓ + * 4 Vegetarian 9 12 MongoDB 13 14 dbm 15 + * ┏━━━━━━━┻━━━━━━━┓ + * 5 Pasta 6 7 Mains 8 + */ + const vegDoc = await treeCollection.findOneAsync({ _id: 'Vegetarian' }); + if (!vegDoc) throw new Error('Vegetarian doc not found'); + await moveDocBetweenRoots(vegDoc, treeCollection, { id: 'root1', collection: 'someCol' }, 3.5); + const docs = await treeCollection.find({}, { sort: { 'root.id': 1, left: 1 } }).fetchAsync(); + assert.deepEqual(docs, [ + doc('Books', 1, 18, undefined, 'root1'), + doc('Programming', 2, 17, 'Books', 'root1'), + doc('Languages', 3, 10, 'Programming', 'root1'), + doc('Vegetarian', 4, 9, 'Languages', 'root1'), + doc('Pasta', 5, 6, 'Vegetarian', 'root1'), + doc('Mains', 7, 8, 'Vegetarian', 'root1'), + doc('Databases', 11, 16, 'Programming', 'root1'), + doc('MongoDB', 12, 13, 'Databases', 'root1'), + doc('dbm', 14, 15, 'Databases', 'root1'), + doc('Videos', 1, 6, undefined, 'root2'), + doc('Cooking', 2, 5, 'Videos', 'root2'), + doc('Meat', 3, 4, 'Cooking', 'root2'), + ]); + }); +}); diff --git a/app/imports/api/parenting/parentingFunctions.ts b/app/imports/api/parenting/parentingFunctions.ts index 67278c91..1563379d 100644 --- a/app/imports/api/parenting/parentingFunctions.ts +++ b/app/imports/api/parenting/parentingFunctions.ts @@ -1,4 +1,4 @@ -import { chain, reverse } from 'lodash'; +import { chain, reverse, set } from 'lodash'; import { TreeDoc, treeDocFields, Reference } from '/imports/api/parenting/ChildSchema'; import { getProperties } from '/imports/api/engine/loadCreatures'; import CreatureProperties from '/imports/api/creature/creatureProperties/CreatureProperties'; @@ -89,20 +89,19 @@ type FilteredDoc = { _ancestorOfMatchedDocument?: boolean, } & TreeDoc; -let filterToForest: undefined | ((...any) => TreeNode[]) = undefined; - -if (Meteor.isClient) filterToForest = function ( +export function filterToForest( collection: Mongo.Collection, rootId: string, - filter: Mongo.Selector, + filter?: Mongo.Query, { options = >{}, includeFilteredDocAncestors = false, includeFilteredDocDescendants = false } = {} ): TreeNode[] { + if (!Meteor.isClient) throw 'Only available on the client'; // Setup the filter - let collectionFilter = { + let collectionFilter: Mongo.Query = { 'root.id': rootId, 'removed': { $ne: true }, }; @@ -173,8 +172,6 @@ if (Meteor.isClient) filterToForest = function ( return docsToForest(nodes); } -export { filterToForest }; - type ForestAndOrphans = { forest: TreeNode[], orphanIds: string[] } /** * Takes a complete set of documents and builds a forest using just their `.parentIds` @@ -385,48 +382,33 @@ export async function moveDocWithinRoot(doc: TreeDoc, collection: Mongo.Collecti }); const newParentId = newParent?._id; - // get ids of the doc and its children, so we can move them even after other docs have been - // moved around to potentially overlap the moving doc - const movedIds = await collection.find({ + // Use bulk operations with $set only, because using $inc caused a lot of trouble with both + // latency compensation and oplog tailing + const bulkOps: any[] = []; + + // Move the doc and its children the move distance + await collection.find({ 'root.id': doc.root.id, left: { $gte: doc.left }, right: { $lte: doc.right }, }, { - fields: { _id: 1 } - }).mapAsync(doc => doc._id); - - // Move all the lefts and rights of documents between the current doc edge and the destination - const moveIncludedLeft = collection.updateAsync({ - 'root.id': doc.root.id, - left: { $gt: includedRange.left, $lt: includedRange.right }, - }, { - $inc: { left: shiftDistance } - }, { - multi: true - }); - const moveIncludedRight = collection.updateAsync({ - 'root.id': doc.root.id, - right: { $gt: includedRange.left, $lt: includedRange.right }, - }, { - $inc: { right: shiftDistance } - }, { - multi: true + fields: { _id: 1, left: 1, right: 1 }, + }).forEachAsync(moveDoc => { + const update = { + $set: { + left: (moveDoc.left + move) || 0, + right: (moveDoc.right + move) || 0, + } + }; + bulkOps.push({ + updateOne: { + filter: { _id: moveDoc._id }, + update, + } + }); }); - // Move the doc and its children to the new location - const moveDocAndChildren = collection.updateAsync({ - _id: { $in: movedIds } - }, { - $inc: { - left: move, - right: move, - } - }, { - multi: true, - }); - - // Set the doc's parent to the new parentId - let changeParent: Promise | undefined; + // Change the doc's parent if necessary if (newParentId !== doc.parentId) { let update; if (newParentId) { @@ -434,9 +416,41 @@ export async function moveDocWithinRoot(doc: TreeDoc, collection: Mongo.Collecti } else { update = { $unset: { parentId: 1 } }; } - changeParent = collection.updateAsync(doc._id, update); + bulkOps.push({ + updateOne: { + filter: { _id: doc._id }, + update, + } + }); } - return Promise.all([moveIncludedLeft, moveIncludedRight, moveDocAndChildren, changeParent]); + + // Move all the lefts and rights of documents between the current doc edge and the destination + await collection.find({ + 'root.id': doc.root.id, + $or: [ + { left: { $gt: includedRange.left, $lt: includedRange.right } }, + { right: { $gt: includedRange.left, $lt: includedRange.right } }, + ], + }, { + fields: { _id: 1, left: 1, right: 1 }, + }).forEachAsync(doc => { + const $set: { [P in keyof TreeDoc]?: TreeDoc[P] } = {}; + if (doc.left > includedRange.left && doc.left < includedRange.right) { + $set.left = (doc.left + shiftDistance) || 0; + } + if (doc.right > includedRange.left && doc.right < includedRange.right) { + $set.right = (doc.right + shiftDistance || 0); + } + bulkOps.push({ + updateOne: { + filter: { _id: doc._id }, + update: { $set }, + } + }); + }); + + await writeBulkOperations(collection, bulkOps); + return rebuildNestedSets(collection, doc.root.id); } export async function moveDocBetweenRoots(doc: TreeDoc, collection: Mongo.Collection, newRoot: Reference, newPosition: number) { @@ -444,65 +458,117 @@ export async function moveDocBetweenRoots(doc: TreeDoc, collection: Mongo.Collec throw new Meteor.Error('invalid-move', 'Document is already in the given root') } - // get ids of the doc and its children, so we can move them even after other docs have been - // moved around to potentially overlap the moving doc - const movedIds = await collection.find({ + // Use bulk operations with $set only, because using $inc caused a lot of trouble with both + // latency compensation and oplog tailing + const bulkOps: { + updateOne: { + filter: Mongo.Query, + update: Mongo.Modifier + } + }[] = []; + + // Get the new parent of the doc after the move + const newParent = await collection.findOneAsync({ + 'root.id': newRoot.id, + left: { $lt: newPosition }, + right: { $gt: newPosition }, + }, { + sort: { left: -1 }, // Many ancestors match, taking the right most one gets the immediate parent + fields: { _id: 1 }, + }); + const newParentId = newParent?._id; + + // Change the doc's parent if necessary + if (newParentId !== doc.parentId) { + let update; + if (newParentId) { + update = { $set: { parentId: newParentId } }; + } else { + update = { $unset: { parentId: 1 } }; + } + bulkOps.push({ + updateOne: { + filter: { _id: doc._id }, + update, + } + }); + } + + // Open a gap in the root we are moving to at the new location + const docSize = doc.right - doc.left + 1; + await collection.find({ + 'root.id': newRoot.id, + $or: [ + { left: { $gt: newPosition } }, + { right: { $gt: newPosition } }, + ], + }, { + fields: { _id: 1, left: 1, right: 1 }, + }).forEachAsync(openGapDoc => { + const $set: { [P in keyof TreeDoc]?: TreeDoc[P] } = {}; + if (openGapDoc.left > newPosition) { + $set.left = (openGapDoc.left + docSize) || 0; + } + if (openGapDoc.right > newPosition) { + $set.right = (openGapDoc.right + docSize) || 0; + } + bulkOps.push({ + updateOne: { + filter: { _id: openGapDoc._id }, + update: { $set }, + } + }); + }); + + // Move the doc and its children the move distance, and set their new root + const move = newPosition + 0.5 - doc.left; + await collection.find({ 'root.id': doc.root.id, left: { $gte: doc.left }, right: { $lte: doc.right }, }, { - fields: { _id: 1 } - }).mapAsync(doc => doc._id); + fields: { _id: 1, left: 1, right: 1 }, + }).forEachAsync(moveDoc => { + bulkOps.push({ + updateOne: { + filter: { _id: moveDoc._id }, + update: { + $set: { + left: (moveDoc.left + move) || 0, + right: (moveDoc.right + move) || 0, + root: newRoot + } + }, + } + }); + }); // Close the gap in the root we are leaving - const docSize = doc.right - doc.left + 1; - const closeGapLeft = collection.updateAsync({ + await collection.find({ 'root.id': doc.root.id, - left: { $gt: doc.right }, + $or: [ + { left: { $gt: doc.right } }, + { right: { $gt: doc.right } }, + ], }, { - $inc: { left: -docSize }, - }, { - multi: true, - }); - const closeGapRight = collection.updateAsync({ - 'root.id': doc.root.id, - right: { $gt: doc.right }, - }, { - $inc: { right: -docSize }, - }, { - multi: true, + fields: { _id: 1, left: 1, right: 1 }, + }).forEachAsync(closeGapDoc => { + const $set: { [P in keyof TreeDoc]?: TreeDoc[P] } = {}; + if (closeGapDoc.left > doc.right) { + $set.left = (closeGapDoc.left - docSize) || 0; + } + if (closeGapDoc.right > doc.right) { + $set.right = (closeGapDoc.right - docSize || 0); + } + bulkOps.push({ + updateOne: { + filter: { _id: closeGapDoc._id }, + update: { $set }, + } + }); }); - // Open a gap in the root we are moving to at the new location - const openGapLeft = collection.updateAsync({ - 'root.id': newRoot.id, - left: { $gt: newPosition }, - }, { - $inc: { left: -docSize }, - }, { - multi: true, - }); - const openGapRight = collection.updateAsync({ - 'root.id': newRoot.id, - right: { $gt: newPosition }, - }, { - $inc: { right: -docSize }, - }, { - multi: true, - }); - - // Move the docs to the new root and update their left and right positions to land in the gap - const moveDistance = newPosition + 0.5 - doc.left; - const moveDocs = collection.updateAsync({ - _id: { $in: movedIds }, - }, { - $set: { root: newRoot }, - $inc: { left: moveDistance, right: moveDistance }, - }, { - multi: true, - }); - - return Promise.all([closeGapLeft, closeGapRight, openGapLeft, openGapRight, moveDocs]); + return writeBulkOperations(collection, bulkOps); } /** @@ -658,7 +724,16 @@ export function calculateNestedSetOperations(docs: TreeDoc[]) { visitedNodes.add(top); stack.pop(); if (top.doc.right !== count) { - opsById[top.doc._id].updateOne.update.$set.right = count; + if (!opsById[top.doc._id]) { + opsById[top.doc._id] = { + updateOne: { + filter: { _id: top.doc._id }, + update: { $set: { right: count } } + } + } + } else { + opsById[top.doc._id].updateOne.update.$set.right = count; + } } count += 1; } else { @@ -753,21 +828,19 @@ async function writeBulkOperations(collection: Mongo.Collection, operat ); }); } else { + // Don't do latency compensation if there are too many operations, it just causes client + // lag without much benefit const promises = operations.map(op => { if (op.updateOne) { return collection.updateAsync( op.updateOne.filter, op.updateOne.update, - { selector: { type: 'any' } } ); } else if (op.updateMany) { return collection.updateAsync( op.updateMany.filter, op.updateMany.update, - { - selector: { type: 'any' }, - multi: true, - }, + { multi: true }, ) } }); diff --git a/app/imports/api/users/methods/linkWithPatreon.js b/app/imports/api/users/methods/linkWithPatreon.js index 05ec62fb..c204e1f2 100644 --- a/app/imports/api/users/methods/linkWithPatreon.js +++ b/app/imports/api/users/methods/linkWithPatreon.js @@ -1,6 +1,5 @@ // Adds accounts-patreon support to bozhao:link-accounts import { Meteor } from 'meteor/meteor'; -import { Accounts } from 'meteor/accounts-base'; export default function linkWithPatreon(options, callback) { if (!Meteor.userId()) { diff --git a/app/imports/client/ui/components/tree/TreeNode.vue b/app/imports/client/ui/components/tree/TreeNode.vue index 968d59c6..9437032b 100644 --- a/app/imports/client/ui/components/tree/TreeNode.vue +++ b/app/imports/client/ui/components/tree/TreeNode.vue @@ -55,14 +55,15 @@
diff --git a/app/imports/client/ui/components/tree/TreeNodeList.vue b/app/imports/client/ui/components/tree/TreeNodeList.vue index b4c4b6a8..51e7ddbd 100644 --- a/app/imports/client/ui/components/tree/TreeNodeList.vue +++ b/app/imports/client/ui/components/tree/TreeNodeList.vue @@ -25,8 +25,8 @@ :start-expanded="startExpanded" :show-external-details="showExternalDetails" @selected="e => $emit('selected', e)" - @reordered="e => $emit('reordered', e)" - @reorganized="e => $emit('reorganized', e)" + @move-within-root="e => $emit('move-within-root', e)" + @move-between-roots="e => $emit('move-between-roots', e)" /> @@ -45,6 +45,10 @@ export default { type: Object, default: undefined, }, + root: { + type: Object, + required: true, + }, group: { type: String, default: undefined, @@ -93,32 +97,31 @@ export default { let event = moved || added; if (event) { let doc = event.element.doc; - let newIndex; - if (event.newIndex === 0) { - newIndex = 0.5; - } else { - if (event.newIndex < this.children.length) { - let childAtNewIndex = this.children[event.newIndex]; - if (event.newIndex > event.oldIndex) { - newIndex = childAtNewIndex.doc.right + 0.5; - } else { - newIndex = childAtNewIndex.doc.left - 0.5; - } + let newPosition; + if (!this.children.length) { + if (this.node) { + newPosition = this.node.left + 0.5; } else { - let childBeforeNewIndex = this.children[event.newIndex - 1]; - newIndex = childBeforeNewIndex.doc.right + 0.5; + newPosition = 0.5; } + } else if (event.newIndex < this.children.length) { + let childAtNewIndex = this.children[event.newIndex]; + if (event.newIndex > event.oldIndex) { + newPosition = childAtNewIndex.doc.right + 0.5; + } else { + newPosition = childAtNewIndex.doc.left - 0.5; + } + } else { + let childBeforeNewIndex = this.children[event.newIndex - 1]; + newPosition = childBeforeNewIndex.doc.right + 0.5; } - if (moved) { - this.$emit('reordered', { doc, newIndex }); - } else if (added) { - this.$emit('reorganized', { doc, parent: this.node, newIndex }); + if (doc.root.id === this.root.id) { + this.$emit('move-within-root', { doc, newPosition }); + } else { + this.$emit('move-between-roots', { doc, newPosition, newRootRef: this.root }); } } }, - move() { - return true; - }, }, }; diff --git a/app/imports/client/ui/creature/creatureProperties/CreaturePropertiesTree.vue b/app/imports/client/ui/creature/creatureProperties/CreaturePropertiesTree.vue index 69ce92fc..5917024b 100644 --- a/app/imports/client/ui/creature/creatureProperties/CreaturePropertiesTree.vue +++ b/app/imports/client/ui/creature/creatureProperties/CreaturePropertiesTree.vue @@ -6,16 +6,17 @@ :organize="organize" :selected-node="selectedNode" :start-expanded="expanded" + :root="root" @selected="e => $emit('selected', e)" - @reordered="reordered" - @reorganized="reorganized" + @move-within-root="moveWithinRoot" + @move-between-roots="moveBetweenRoots" />