hidden subtree is now built as "predictable" to avoid duplicating it in sync

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}`);
+        }
+    });
+};

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;

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");
+                }
+            }
+        }
+    });
+};

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;

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'
+        if (this.noteId === 'root'
-            || 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({
+        const existingBranch = this.becca.getBranchFromChildAndParent(this.noteId, parentNoteId);
-            noteId: this.noteId,
+
-            parentNoteId: parentNoteId,
+        if (existingBranch) {
-            notePosition: notePosition,
+            existingBranch.notePosition = notePosition;
-            prefix: this.prefix,
+            return existingBranch;
-            isExpanded: this.isExpanded
+        } else {
-        });
+            return new Branch({
+                noteId: this.noteId,
+                parentNoteId: parentNoteId,
+                notePosition: notePosition,
+                prefix: this.prefix,
+                isExpanded: this.isExpanded
+            });
+        }
     }
 }
 

src/becca/entities/note.js

@@ -945,13 +945,14 @@ class Note extends AbstractEntity {
         };
     }
 
-    /** @returns {String[]} */
+    /** @returns {String[]} - includes the subtree node as well */
-    getSubtreeNoteIds({includeArchived = true, resolveSearch = false} = {}) {
+    getSubtreeNoteIds({includeArchived = true, includeHidden = false, resolveSearch = false} = {}) {
-        return this.getSubtree({includeArchived, resolveSearch})
+        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) {

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 {
+                res.status(201).json(mappers.mapBranchToPojo(branch));
-            const branch = new Branch(params).save();
+            } 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);
         }
     });
 

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);

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 APP_DB_VERSION = 212;
-const SYNC_VERSION = 28;
+const SYNC_VERSION = 29;
 const CLIPPER_PROTOCOL_VERSION = "1.0";
 
 module.exports = {

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();
+            elapsedTimeMs = this.runChecksInner();
-
-            this.runDbDiagnostics();
-
-            this.runAllChecksAndFixers();
-
-            elapsedTimeMs = Date.now() - startTimeMs;
         });
 
         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
 };

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') {
+    const attrs = [...(item.attributes || [])];
-            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 });
-                }
 
-                attrs.push({ type: 'label', name: 'builtinWidget', value: item.builtinWidget });
+    if (item.icon) {
-             } else if (item.targetNoteId) {
+        attrs.push({ type: 'label', name: 'iconClass', value: `bx ${item.icon}` });
-                attrs.push({ type: 'relation', name: 'template', value: LBTPL_NOTE_LAUNCHER });
+    }
-                attrs.push({ type: 'relation', name: 'target', value: item.targetNoteId });
+
+    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)) {
+        const attrId = note.noteId + "_" + attr.type.charAt(0) + attr.name;
-            note.addAttribute(attr.type, attr.name, attr.value);
+
+        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();
         }
     }
 

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,

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);