From 0758c829836e3aefd9e095fef1cf030f9b9569de Mon Sep 17 00:00:00 2001 From: zadam Date: Tue, 27 Dec 2022 14:44:28 +0100 Subject: [PATCH] hidden subtree is now built as "predictable" to avoid duplicating it in sync --- db/migrations/0210__consistency_checks.js | 20 ++++++ db/migrations/0211__rename_branchIds.sql | 12 ++++ ...2__delete_all_attributes_of_named_notes.js | 21 ++++++ src/becca/entities/abstract_entity.js | 7 +- src/becca/entities/branch.js | 36 ++++++---- src/becca/entities/note.js | 16 +++-- src/etapi/branches.js | 15 ++--- src/routes/api/attributes.js | 2 +- src/services/app_info.js | 4 +- src/services/consistency_checks.js | 32 +++++---- src/services/hidden_subtree.js | 66 ++++++++++++------- src/services/notes.js | 3 + src/services/scheduler.js | 4 +- 13 files changed, 169 insertions(+), 69 deletions(-) create mode 100644 db/migrations/0210__consistency_checks.js create mode 100644 db/migrations/0211__rename_branchIds.sql create mode 100644 db/migrations/0212__delete_all_attributes_of_named_notes.js diff --git a/db/migrations/0210__consistency_checks.js b/db/migrations/0210__consistency_checks.js new file mode 100644 index 000000000..f49fd0fd4 --- /dev/null +++ b/db/migrations/0210__consistency_checks.js @@ -0,0 +1,20 @@ +module.exports = async () => { + const cls = require("../../src/services/cls"); + const beccaLoader = require("../../src/becca/becca_loader"); + const log = require("../../src/services/log"); + const consistencyChecks = require("../../src/services/consistency_checks"); + + await cls.init(async () => { + beccaLoader.load(); + + try { + // precaution before running 211 which might produce unique constraint problems if the DB was not consistent + consistencyChecks.runOnDemandChecksWithoutExclusiveLock(true); + } + catch (e) { + // consistency checks might start failing in the future if there's some incompatible migration down the road + // we can optimistically assume the DB is consistent and still continue + log.error(`Consistency checks failed in migration 0210: ${e.message} ${e.stack}`); + } + }); +}; diff --git a/db/migrations/0211__rename_branchIds.sql b/db/migrations/0211__rename_branchIds.sql new file mode 100644 index 000000000..a5dd8a5d3 --- /dev/null +++ b/db/migrations/0211__rename_branchIds.sql @@ -0,0 +1,12 @@ +-- case based on isDeleted is needed, otherwise 2 branches (1 deleted, 1 not) might get the same ID +UPDATE entity_changes SET entityId = ( + SELECT + CASE isDeleted + WHEN 0 THEN parentNoteId || '_' || noteId + WHEN 1 THEN branchId + END + FROM branches WHERE branchId = entityId +) +WHERE entityName = 'branches'; + +UPDATE branches SET branchId = parentNoteId || '_' || noteId WHERE isDeleted = 0; diff --git a/db/migrations/0212__delete_all_attributes_of_named_notes.js b/db/migrations/0212__delete_all_attributes_of_named_notes.js new file mode 100644 index 000000000..47ddb9114 --- /dev/null +++ b/db/migrations/0212__delete_all_attributes_of_named_notes.js @@ -0,0 +1,21 @@ +module.exports = () => { + const cls = require("../../src/services/cls"); + const beccaLoader = require("../../src/becca/becca_loader"); + const becca = require("../../src/becca/becca"); + + cls.init(() => { + beccaLoader.load(); + + const hidden = becca.getNote("_hidden"); + + for (const noteId of hidden.getSubtreeNoteIds({includeHidden: true})) { + if (noteId.startsWith("_")) { // is "named" note + const note = becca.getNote(noteId); + + for (const attr of note.getOwnedAttributes()) { + attr.markAsDeleted("0212__delete_all_attributes_of_named_notes"); + } + } + } + }); +}; diff --git a/src/becca/entities/abstract_entity.js b/src/becca/entities/abstract_entity.js index 8c1b7b755..8d28604ca 100644 --- a/src/becca/entities/abstract_entity.js +++ b/src/becca/entities/abstract_entity.js @@ -46,7 +46,10 @@ class AbstractEntity { return this.utcDateModified || this.utcDateCreated; } - /** @protected */ + /** + * @protected + * @returns {Becca} + */ get becca() { if (!becca) { becca = require('../becca'); @@ -75,7 +78,7 @@ class AbstractEntity { /** * Saves entity - executes SQL, but doesn't commit the transaction on its own * - * @returns {AbstractEntity} + * @returns {this} */ save() { const entityName = this.constructor.entityName; diff --git a/src/becca/entities/branch.js b/src/becca/entities/branch.js index 127c58b3c..27f6babe9 100644 --- a/src/becca/entities/branch.js +++ b/src/becca/entities/branch.js @@ -78,7 +78,7 @@ class Branch extends AbstractEntity { childNote.parentBranches.push(this); } - if (this.branchId === 'root') { + if (this.noteId === 'root') { return; } @@ -165,8 +165,7 @@ class Branch extends AbstractEntity { } } - if (this.branchId === 'root' - || this.noteId === 'root' + if (this.noteId === 'root' || this.noteId === cls.getHoistedNoteId()) { throw new Error("Can't delete root or hoisted branch/note"); @@ -209,11 +208,19 @@ class Branch extends AbstractEntity { } beforeSaving() { + if (!this.noteId || !this.parentNoteId) { + throw new Error(`noteId and parentNoteId are mandatory properties for Branch`); + } + + this.branchId = `${this.parentNoteId}_${this.noteId}`; + if (this.notePosition === undefined || this.notePosition === null) { let maxNotePos = 0; for (const childBranch of this.parentNote.getChildBranches()) { - if (maxNotePos < childBranch.notePosition && childBranch.noteId !== '_hidden') { + if (maxNotePos < childBranch.notePosition + && childBranch.noteId !== '_hidden' // hidden has very large notePosition to always stay last + ) { maxNotePos = childBranch.notePosition; } } @@ -246,13 +253,20 @@ class Branch extends AbstractEntity { } createClone(parentNoteId, notePosition) { - return new Branch({ - noteId: this.noteId, - parentNoteId: parentNoteId, - notePosition: notePosition, - prefix: this.prefix, - isExpanded: this.isExpanded - }); + const existingBranch = this.becca.getBranchFromChildAndParent(this.noteId, parentNoteId); + + if (existingBranch) { + existingBranch.notePosition = notePosition; + return existingBranch; + } else { + return new Branch({ + noteId: this.noteId, + parentNoteId: parentNoteId, + notePosition: notePosition, + prefix: this.prefix, + isExpanded: this.isExpanded + }); + } } } diff --git a/src/becca/entities/note.js b/src/becca/entities/note.js index 58ecf51fb..ae1b1df52 100644 --- a/src/becca/entities/note.js +++ b/src/becca/entities/note.js @@ -945,13 +945,14 @@ class Note extends AbstractEntity { }; } - /** @returns {String[]} */ - getSubtreeNoteIds({includeArchived = true, resolveSearch = false} = {}) { - return this.getSubtree({includeArchived, resolveSearch}) + /** @returns {String[]} - includes the subtree node as well */ + getSubtreeNoteIds({includeArchived = true, includeHidden = false, resolveSearch = false} = {}) { + return this.getSubtree({includeArchived, includeHidden, resolveSearch}) .notes .map(note => note.noteId); } + /** @deprecated use getSubtreeNoteIds() instead */ getDescendantNoteIds() { return this.getSubtreeNoteIds(); } @@ -1171,7 +1172,8 @@ class Note extends AbstractEntity { * @param {string} type - attribute type (label / relation) * @param {string} name - name of the attribute, not including the leading ~/# * @param {string} [value] - value of the attribute - text for labels, target note ID for relations; optional. - * + * @param {boolean} [isInheritable=false] + * @param {int} [position] * @return {Attribute} */ addAttribute(type, name, value = "", isInheritable = false, position = 1000) { @@ -1192,7 +1194,7 @@ class Note extends AbstractEntity { * * @param {string} name - name of the label, not including the leading # * @param {string} [value] - text value of the label; optional - * + * @param {boolean} [isInheritable=false] * @return {Attribute} */ addLabel(name, value = "", isInheritable = false) { @@ -1204,8 +1206,8 @@ class Note extends AbstractEntity { * returned. * * @param {string} name - name of the relation, not including the leading ~ - * @param {string} value - ID of the target note of the relation - * + * @param {string} targetNoteId + * @param {boolean} [isInheritable=false] * @return {Attribute} */ addRelation(name, targetNoteId, isInheritable = false) { diff --git a/src/etapi/branches.js b/src/etapi/branches.js index bac449322..ba06afee1 100644 --- a/src/etapi/branches.js +++ b/src/etapi/branches.js @@ -35,15 +35,14 @@ function register(router) { existing.save(); return res.status(200).json(mappers.mapBranchToPojo(existing)); - } + } else { + try { + const branch = new Branch(params).save(); - try { - const branch = new Branch(params).save(); - - res.status(201).json(mappers.mapBranchToPojo(branch)); - } - catch (e) { - throw new eu.EtapiError(400, eu.GENERIC_CODE, e.message); + res.status(201).json(mappers.mapBranchToPojo(branch)); + } catch (e) { + throw new eu.EtapiError(400, eu.GENERIC_CODE, e.message); + } } }); diff --git a/src/routes/api/attributes.js b/src/routes/api/attributes.js index 54ef7a39f..cb59239a9 100644 --- a/src/routes/api/attributes.js +++ b/src/routes/api/attributes.js @@ -172,7 +172,7 @@ function updateNoteAttributes(req) { continue; } - // no existing attribute has been matched so we need to create a new one + // no existing attribute has been matched, so we need to create a new one // type, name and isInheritable are immutable so even if there is an attribute with matching type & name, we need to create a new one and delete the former one note.addAttribute(incAttr.type, incAttr.name, incAttr.value, incAttr.isInheritable, position); diff --git a/src/services/app_info.js b/src/services/app_info.js index eba845c1e..4a8f50158 100644 --- a/src/services/app_info.js +++ b/src/services/app_info.js @@ -4,8 +4,8 @@ const build = require('./build'); const packageJson = require('../../package'); const {TRILIUM_DATA_DIR} = require('./data_dir'); -const APP_DB_VERSION = 209; -const SYNC_VERSION = 28; +const APP_DB_VERSION = 212; +const SYNC_VERSION = 29; const CLIPPER_PROTOCOL_VERSION = "1.0"; module.exports = { diff --git a/src/services/consistency_checks.js b/src/services/consistency_checks.js index 6c23a71a1..85e401da9 100644 --- a/src/services/consistency_checks.js +++ b/src/services/consistency_checks.js @@ -144,7 +144,7 @@ class ConsistencyChecks { FROM branches LEFT JOIN notes ON notes.noteId = branches.parentNoteId WHERE branches.isDeleted = 0 - AND branches.branchId != 'root' + AND branches.noteId != 'root' AND notes.noteId IS NULL`, ({branchId, parentNoteId}) => { if (this.autoFix) { @@ -670,7 +670,7 @@ class ConsistencyChecks { this.findSyncIssues(); // root branch should always be expanded - sql.execute("UPDATE branches SET isExpanded = 1 WHERE branchId = 'root'"); + sql.execute("UPDATE branches SET isExpanded = 1 WHERE noteId = 'root'"); if (!this.unrecoveredConsistencyErrors) { // we run this only if basic checks passed since this assumes basic data consistency @@ -701,13 +701,7 @@ class ConsistencyChecks { let elapsedTimeMs; await syncMutexService.doExclusively(() => { - const startTimeMs = Date.now(); - - this.runDbDiagnostics(); - - this.runAllChecksAndFixers(); - - elapsedTimeMs = Date.now() - startTimeMs; + elapsedTimeMs = this.runChecksInner(); }); if (this.unrecoveredConsistencyErrors) { @@ -721,6 +715,16 @@ class ConsistencyChecks { ); } } + + runChecksInner() { + const startTimeMs = Date.now(); + + this.runDbDiagnostics(); + + this.runAllChecksAndFixers(); + + return Date.now() - startTimeMs; + } } function getBlankContent(isProtected, type, mime) { @@ -750,9 +754,14 @@ function runPeriodicChecks() { consistencyChecks.runChecks(); } -function runOnDemandChecks(autoFix) { +async function runOnDemandChecks(autoFix) { const consistencyChecks = new ConsistencyChecks(autoFix); - consistencyChecks.runChecks(); + await consistencyChecks.runChecks(); +} + +function runOnDemandChecksWithoutExclusiveLock(autoFix) { + const consistencyChecks = new ConsistencyChecks(autoFix); + consistencyChecks.runChecksInner(); } function runEntityChangesChecks() { @@ -769,5 +778,6 @@ sqlInit.dbReady.then(() => { module.exports = { runOnDemandChecks, + runOnDemandChecksWithoutExclusiveLock, runEntityChangesChecks }; diff --git a/src/services/hidden_subtree.js b/src/services/hidden_subtree.js index 4580a3e14..2029c2fa2 100644 --- a/src/services/hidden_subtree.js +++ b/src/services/hidden_subtree.js @@ -1,5 +1,6 @@ const becca = require("../becca/becca"); const noteService = require("./notes"); +const Attribute = require("../becca/entities/attribute.js"); const LBTPL_ROOT = "_lbTplRoot"; const LBTPL_BASE = "_lbTplBase"; @@ -10,6 +11,12 @@ const LBTPL_BUILTIN_WIDGET = "_lbTplBuiltinWidget"; const LBTPL_SPACER = "_lbTplSpacer"; const LBTPL_CUSTOM_WIDGET = "_lbTplCustomWidget"; +/* + * Hidden subtree is generated as a "predictable structure" which means that it avoids generating random IDs to always + * produce same structure. This is needed because it is run on multiple instances in the sync cluster which might produce + * duplicate subtrees. This way, all instances will generate the same structure with same IDs. + */ + const HIDDEN_SUBTREE_DEFINITION = { id: '_hidden', title: 'Hidden Notes', @@ -239,12 +246,6 @@ function checkHiddenSubtreeRecursively(parentNoteId, item) { let note = becca.notes[item.id]; let branch = becca.branches[item.id]; - const attrs = [...(item.attributes || [])]; - - if (item.icon) { - attrs.push({ type: 'label', name: 'iconClass', value: `bx ${item.icon}` }); - } - if (!note) { ({note, branch} = noteService.createNewNote({ noteId: item.id, @@ -254,27 +255,33 @@ function checkHiddenSubtreeRecursively(parentNoteId, item) { content: '', ignoreForbiddenParents: true })); + } - if (item.type === 'launcher') { - if (item.command) { - attrs.push({ type: 'relation', name: 'template', value: LBTPL_COMMAND }); - attrs.push({ type: 'label', name: 'command', value: item.command }); - } else if (item.builtinWidget) { - if (item.builtinWidget === 'spacer') { - attrs.push({ type: 'relation', name: 'template', value: LBTPL_SPACER }); - attrs.push({ type: 'label', name: 'baseSize', value: item.baseSize }); - attrs.push({ type: 'label', name: 'growthFactor', value: item.growthFactor }); - } else { - attrs.push({ type: 'relation', name: 'template', value: LBTPL_BUILTIN_WIDGET }); - } + const attrs = [...(item.attributes || [])]; - attrs.push({ type: 'label', name: 'builtinWidget', value: item.builtinWidget }); - } else if (item.targetNoteId) { - attrs.push({ type: 'relation', name: 'template', value: LBTPL_NOTE_LAUNCHER }); - attrs.push({ type: 'relation', name: 'target', value: item.targetNoteId }); + if (item.icon) { + attrs.push({ type: 'label', name: 'iconClass', value: `bx ${item.icon}` }); + } + + if (item.type === 'launcher') { + if (item.command) { + attrs.push({ type: 'relation', name: 'template', value: LBTPL_COMMAND }); + attrs.push({ type: 'label', name: 'command', value: item.command }); + } else if (item.builtinWidget) { + if (item.builtinWidget === 'spacer') { + attrs.push({ type: 'relation', name: 'template', value: LBTPL_SPACER }); + attrs.push({ type: 'label', name: 'baseSize', value: item.baseSize }); + attrs.push({ type: 'label', name: 'growthFactor', value: item.growthFactor }); } else { - throw new Error(`No action defined for launcher ${JSON.stringify(item)}`); + attrs.push({ type: 'relation', name: 'template', value: LBTPL_BUILTIN_WIDGET }); } + + attrs.push({ type: 'label', name: 'builtinWidget', value: item.builtinWidget }); + } else if (item.targetNoteId) { + attrs.push({ type: 'relation', name: 'template', value: LBTPL_NOTE_LAUNCHER }); + attrs.push({ type: 'relation', name: 'target', value: item.targetNoteId }); + } else { + throw new Error(`No action defined for launcher ${JSON.stringify(item)}`); } } @@ -299,8 +306,17 @@ function checkHiddenSubtreeRecursively(parentNoteId, item) { } for (const attr of attrs) { - if (!note.hasAttribute(attr.type, attr.name)) { - note.addAttribute(attr.type, attr.name, attr.value); + const attrId = note.noteId + "_" + attr.type.charAt(0) + attr.name; + + if (!note.getAttributes().find(attr => attr.attributeId === attrId)) {console.log("EEEEE"); + new Attribute({ + attributeId: attrId, + noteId: note.noteId, + type: attr.type, + name: attr.name, + value: attr.value, + isInheritable: false + }).save(); } } diff --git a/src/services/notes.js b/src/services/notes.js index 21e858861..95dff2ec3 100644 --- a/src/services/notes.js +++ b/src/services/notes.js @@ -155,6 +155,9 @@ function createNewNote(params) { cls.disableEntityEvents(); } + // TODO: think about what can happen if the note already exists with the forced ID + // I guess on DB it's going to be fine, but becca references between entities + // might get messed up (two Note instance for the same ID existing in the references) note = new Note({ noteId: params.noteId, // optionally can force specific noteId title: params.title, diff --git a/src/services/scheduler.js b/src/services/scheduler.js index 70dc23589..9897f5642 100644 --- a/src/services/scheduler.js +++ b/src/services/scheduler.js @@ -51,9 +51,9 @@ function runNotesWithLabel(runAttrValue) { } sqlInit.dbReady.then(() => { - if (!process.env.TRILIUM_SAFE_MODE) { - cls.init(() => hiddenSubtreeService.checkHiddenSubtree()); + cls.init(() => hiddenSubtreeService.checkHiddenSubtree()); + if (!process.env.TRILIUM_SAFE_MODE) { setTimeout(cls.wrap(() => runNotesWithLabel('backendStartup')), 10 * 1000); setInterval(cls.wrap(() => runNotesWithLabel('hourly')), 3600 * 1000);