chore: documentStructure database locking (#3254)

server/models/Collection.ts

@@ -1,12 +1,6 @@
 import { find, findIndex, remove, uniq } from "lodash";
 import randomstring from "randomstring";
-import {
-  Identifier,
-  Transaction,
-  Op,
-  FindOptions,
-  SaveOptions,
-} from "sequelize";
+import { Identifier, Transaction, Op, FindOptions } from "sequelize";
 import {
   Sequelize,
   Table,
@@ -400,8 +394,8 @@ class Collection extends ParanoidModel {
     };
   };
 
-  deleteDocument = async function (document: Document) {
-    await this.removeDocumentInStructure(document);
+  deleteDocument = async function (document: Document, options?: FindOptions) {
+    await this.removeDocumentInStructure(document, options);
 
     // Helper to destroy all child documents for a document
     const loopChildren = async (
@@ -419,13 +413,13 @@ class Collection extends ParanoidModel {
       });
     };
 
-    await loopChildren(document.id);
-    await document.destroy();
+    await loopChildren(document.id, options);
+    await document.destroy(options);
   };
 
   removeDocumentInStructure = async function (
     document: Document,
-    options?: SaveOptions<Collection> & {
+    options?: FindOptions & {
       save?: boolean;
     }
   ) {
@@ -434,66 +428,55 @@ class Collection extends ParanoidModel {
     }
 
     let result: [NavigationNode, number] | undefined;
-    let transaction;
 
-    try {
-      // documentStructure can only be updated by one request at the time
-      transaction = await this.sequelize.transaction();
+    const removeFromChildren = async (
+      children: NavigationNode[],
+      id: string
+    ) => {
+      children = await Promise.all(
+        children.map(async (childDocument) => {
+          return {
+            ...childDocument,
+            children: await removeFromChildren(childDocument.children, id),
+          };
+        })
+      );
+      const match = find(children, {
+        id,
+      });
 
-      const removeFromChildren = async (
-        children: NavigationNode[],
-        id: string
-      ) => {
-        children = await Promise.all(
-          children.map(async (childDocument) => {
-            return {
-              ...childDocument,
-              children: await removeFromChildren(childDocument.children, id),
-            };
-          })
-        );
-        const match = find(children, {
-          id,
-        });
-
-        if (match) {
-          if (!result) {
-            result = [
-              match,
-              findIndex(children, {
-                id,
-              }),
-            ];
-          }
-
-          remove(children, {
-            id,
-          });
+      if (match) {
+        if (!result) {
+          result = [
+            match,
+            findIndex(children, {
+              id,
+            }),
+          ];
         }
 
-        return children;
-      };
+        remove(children, {
+          id,
+        });
+      }
 
-      this.documentStructure = await removeFromChildren(
-        this.documentStructure,
-        document.id
-      );
+      return children;
+    };
 
-      // Sequelize doesn't seem to set the value with splice on JSONB field
-      // https://github.com/sequelize/sequelize/blob/e1446837196c07b8ff0c23359b958d68af40fd6d/src/model.js#L3937
-      this.changed("documentStructure", true);
+    this.documentStructure = await removeFromChildren(
+      this.documentStructure,
+      document.id
+    );
+
+    // Sequelize doesn't seem to set the value with splice on JSONB field
+    // https://github.com/sequelize/sequelize/blob/e1446837196c07b8ff0c23359b958d68af40fd6d/src/model.js#L3937
+    this.changed("documentStructure", true);
+
+    if (options?.save !== false) {
       await this.save({
         ...options,
         fields: ["documentStructure"],
-        transaction,
       });
-      await transaction.commit();
-    } catch (err) {
-      if (transaction) {
-        await transaction.rollback();
-      }
-
-      throw err;
     }
 
     return result;
@@ -553,48 +536,39 @@ class Collection extends ParanoidModel {
   /**
    * Update document's title and url in the documentStructure
    */
-  updateDocument = async function (updatedDocument: Document) {
+  updateDocument = async function (
+    updatedDocument: Document,
+    options?: { transaction: Transaction }
+  ) {
     if (!this.documentStructure) {
       return;
     }
-    let transaction;
 
-    try {
-      // documentStructure can only be updated by one request at the time
-      transaction = await this.sequelize.transaction();
-      const { id } = updatedDocument;
+    const { id } = updatedDocument;
 
-      const updateChildren = (documents: NavigationNode[]) => {
-        return documents.map((document) => {
-          if (document.id === id) {
-            document = {
-              ...(updatedDocument.toJSON() as NavigationNode),
-              children: document.children,
-            };
-          } else {
-            document.children = updateChildren(document.children);
-          }
+    const updateChildren = (documents: NavigationNode[]) => {
+      return documents.map((document) => {
+        if (document.id === id) {
+          document = {
+            ...(updatedDocument.toJSON() as NavigationNode),
+            children: document.children,
+          };
+        } else {
+          document.children = updateChildren(document.children);
+        }
 
-          return document;
-        });
-      };
-
-      this.documentStructure = updateChildren(this.documentStructure);
-      // Sequelize doesn't seem to set the value with splice on JSONB field
-      // https://github.com/sequelize/sequelize/blob/e1446837196c07b8ff0c23359b958d68af40fd6d/src/model.js#L3937
-      this.changed("documentStructure", true);
-      await this.save({
-        fields: ["documentStructure"],
-        transaction,
+        return document;
       });
-      await transaction.commit();
-    } catch (err) {
-      if (transaction) {
-        await transaction.rollback();
-      }
+    };
 
-      throw err;
-    }
+    this.documentStructure = updateChildren(this.documentStructure);
+    // Sequelize doesn't seem to set the value with splice on JSONB field
+    // https://github.com/sequelize/sequelize/blob/e1446837196c07b8ff0c23359b958d68af40fd6d/src/model.js#L3937
+    this.changed("documentStructure", true);
+    await this.save({
+      fields: ["documentStructure"],
+      ...options,
+    });
 
     return this;
   };
@@ -602,7 +576,7 @@ class Collection extends ParanoidModel {
   addDocumentToStructure = async function (
     document: Document,
     index?: number,
-    options: {
+    options: FindOptions & {
       save?: boolean;
       documentJson?: NavigationNode;
     } = {}
@@ -610,67 +584,48 @@ class Collection extends ParanoidModel {
     if (!this.documentStructure) {
       this.documentStructure = [];
     }
-    let transaction;
 
-    try {
-      // documentStructure can only be updated by one request at a time
-      if (options?.save !== false) {
-        transaction = await this.sequelize.transaction();
-      }
+    // If moving existing document with children, use existing structure
+    const documentJson = { ...document.toJSON(), ...options.documentJson };
 
-      // If moving existing document with children, use existing structure
-      const documentJson = { ...document.toJSON(), ...options.documentJson };
+    if (!document.parentDocumentId) {
+      // Note: Index is supported on DB level but it's being ignored
+      // by the API presentation until we build product support for it.
+      this.documentStructure.splice(
+        index !== undefined ? index : this.documentStructure.length,
+        0,
+        documentJson
+      );
+    } else {
+      // Recursively place document
+      const placeDocument = (documentList: NavigationNode[]) => {
+        return documentList.map((childDocument) => {
+          if (document.parentDocumentId === childDocument.id) {
+            childDocument.children.splice(
+              index !== undefined ? index : childDocument.children.length,
+              0,
+              documentJson
+            );
+          } else {
+            childDocument.children = placeDocument(childDocument.children);
+          }
 
-      if (!document.parentDocumentId) {
-        // Note: Index is supported on DB level but it's being ignored
-        // by the API presentation until we build product support for it.
-        this.documentStructure.splice(
-          index !== undefined ? index : this.documentStructure.length,
-          0,
-          documentJson
-        );
-      } else {
-        // Recursively place document
-        const placeDocument = (documentList: NavigationNode[]) => {
-          return documentList.map((childDocument) => {
-            if (document.parentDocumentId === childDocument.id) {
-              childDocument.children.splice(
-                index !== undefined ? index : childDocument.children.length,
-                0,
-                documentJson
-              );
-            } else {
-              childDocument.children = placeDocument(childDocument.children);
-            }
-
-            return childDocument;
-          });
-        };
-
-        this.documentStructure = placeDocument(this.documentStructure);
-      }
-
-      // Sequelize doesn't seem to set the value with splice on JSONB field
-      // https://github.com/sequelize/sequelize/blob/e1446837196c07b8ff0c23359b958d68af40fd6d/src/model.js#L3937
-      this.changed("documentStructure", true);
-
-      if (options?.save !== false) {
-        await this.save({
-          ...options,
-          fields: ["documentStructure"],
-          transaction,
+          return childDocument;
         });
+      };
 
-        if (transaction) {
-          await transaction.commit();
-        }
-      }
-    } catch (err) {
-      if (transaction) {
-        await transaction.rollback();
-      }
+      this.documentStructure = placeDocument(this.documentStructure);
+    }
 
-      throw err;
+    // Sequelize doesn't seem to set the value with splice on JSONB field
+    // https://github.com/sequelize/sequelize/blob/e1446837196c07b8ff0c23359b958d68af40fd6d/src/model.js#L3937
+    this.changed("documentStructure", true);
+
+    if (options?.save !== false) {
+      await this.save({
+        ...options,
+        fields: ["documentStructure"],
+      });
     }
 
     return this;

server/models/Document.test.ts

@@ -422,6 +422,62 @@ describe("#save", () => {
   });
 });
 
+describe("#getChildDocumentIds", () => {
+  test("should return empty array if no children", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({
+      teamId: team.id,
+    });
+    const collection = await buildCollection({
+      userId: user.id,
+      teamId: team.id,
+    });
+    const document = await buildDocument({
+      userId: user.id,
+      teamId: team.id,
+      collectionId: collection.id,
+      title: "test",
+    });
+    const results = await document.getChildDocumentIds();
+    expect(results.length).toBe(0);
+  });
+
+  test("should return nested child document ids", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({
+      teamId: team.id,
+    });
+    const collection = await buildCollection({
+      userId: user.id,
+      teamId: team.id,
+    });
+    const document = await buildDocument({
+      userId: user.id,
+      teamId: team.id,
+      collectionId: collection.id,
+      title: "test",
+    });
+    const document2 = await buildDocument({
+      userId: user.id,
+      teamId: team.id,
+      collectionId: collection.id,
+      parentDocumentId: document.id,
+      title: "test",
+    });
+    const document3 = await buildDocument({
+      userId: user.id,
+      teamId: team.id,
+      collectionId: collection.id,
+      parentDocumentId: document2.id,
+      title: "test",
+    });
+    const results = await document.getChildDocumentIds();
+    expect(results.length).toBe(2);
+    expect(results[0]).toBe(document2.id);
+    expect(results[1]).toBe(document3.id);
+  });
+});
+
 describe("#findByPk", () => {
   test("should return document when urlId is correct", async () => {
     const { document } = await seed();

server/models/Document.ts

@@ -232,35 +232,50 @@ class Document extends ParanoidModel {
   // hooks
 
   @BeforeSave
-  static async updateInCollectionStructure(model: Document) {
-    if (!model.publishedAt || model.template) {
+  static async updateTitleInCollectionStructure(model: Document) {
+    // templates, drafts, and archived documents don't appear in the structure
+    // and so never need to be updated when the title changes
+    if (
+      model.archivedAt ||
+      model.template ||
+      !model.publishedAt ||
+      !model.changed("title")
+    ) {
       return;
     }
 
-    const collection = await Collection.findByPk(model.collectionId);
+    return this.sequelize!.transaction(async (transaction: Transaction) => {
+      const collection = await Collection.findByPk(model.collectionId, {
+        transaction,
+        lock: transaction.LOCK.UPDATE,
+      });
+      if (!collection) {
+        return;
+      }
 
-    if (!collection) {
-      return;
-    }
-
-    await collection.updateDocument(model);
-    model.collection = collection;
+      await collection.updateDocument(model, { transaction });
+      model.collection = collection;
+    });
   }
 
   @AfterCreate
   static async addDocumentToCollectionStructure(model: Document) {
-    if (!model.publishedAt || model.template) {
+    if (model.archivedAt || model.template || !model.publishedAt) {
       return;
     }
 
-    const collection = await Collection.findByPk(model.collectionId);
+    return this.sequelize!.transaction(async (transaction: Transaction) => {
+      const collection = await Collection.findByPk(model.collectionId, {
+        transaction,
+        lock: transaction.LOCK.UPDATE,
+      });
+      if (!collection) {
+        return;
+      }
 
-    if (!collection) {
-      return;
-    }
-
-    await collection.addDocumentToStructure(model, 0);
-    model.collection = collection;
+      await collection.addDocumentToStructure(model, 0, { transaction });
+      model.collection = collection;
+    });
   }
 
   @BeforeValidate
@@ -674,6 +689,43 @@ class Document extends ParanoidModel {
     return undefined;
   };
 
+  /**
+   * Calculate all of the document ids that are children of this document by
+   * iterating through parentDocumentId references in the most efficient way.
+   *
+   * @param options FindOptions
+   * @returns A promise that resolves to a list of document ids
+   */
+  getChildDocumentIds = async (
+    options?: FindOptions<Document>
+  ): Promise<string[]> => {
+    const getChildDocumentIds = async (
+      ...parentDocumentId: string[]
+    ): Promise<string[]> => {
+      const childDocuments = await (this
+        .constructor as typeof Document).findAll({
+        attributes: ["id"],
+        where: {
+          parentDocumentId,
+        },
+        ...options,
+      });
+
+      const childDocumentIds = childDocuments.map((doc) => doc.id);
+
+      if (childDocumentIds.length > 0) {
+        return [
+          ...childDocumentIds,
+          ...(await getChildDocumentIds(...childDocumentIds)),
+        ];
+      }
+
+      return childDocumentIds;
+    };
+
+    return getChildDocumentIds(this.id);
+  };
+
   archiveWithChildren = async (
     userId: string,
     options?: FindOptions<Document>
@@ -702,47 +754,73 @@ class Document extends ParanoidModel {
     return this.save(options);
   };
 
-  publish = async (userId: string, options?: FindOptions<Document>) => {
+  publish = async (userId: string) => {
+    // If the document is already published then calling publish should act like
+    // a regular save
     if (this.publishedAt) {
-      return this.save(options);
+      return this.save();
     }
 
-    if (!this.template) {
-      const collection = await Collection.findByPk(this.collectionId);
-      await collection?.addDocumentToStructure(this, 0);
-    }
+    await this.sequelize.transaction(async (transaction: Transaction) => {
+      if (!this.template) {
+        const collection = await Collection.findByPk(this.collectionId, {
+          transaction,
+          lock: transaction.LOCK.UPDATE,
+        });
+
+        if (collection) {
+          await collection.addDocumentToStructure(this, 0, { transaction });
+          this.collection = collection;
+        }
+      }
+    });
 
     this.lastModifiedById = userId;
     this.publishedAt = new Date();
-    await this.save(options);
-    return this;
+    return this.save();
   };
 
-  unpublish = async (userId: string, options?: FindOptions<Document>) => {
+  unpublish = async (userId: string) => {
+    // If the document is already a draft then calling unpublish should act like
+    // a regular save
     if (!this.publishedAt) {
-      return this;
+      return this.save();
     }
-    const collection = await this.$get("collection");
-    await collection?.removeDocumentInStructure(this);
 
-    // unpublishing a document converts the "ownership" to yourself, so that it
-    // can appear in your drafts rather than the original creators
+    await this.sequelize.transaction(async (transaction: Transaction) => {
+      const collection = await Collection.findByPk(this.collectionId, {
+        transaction,
+        lock: transaction.LOCK.UPDATE,
+      });
+
+      if (collection) {
+        await collection.removeDocumentInStructure(this, { transaction });
+        this.collection = collection;
+      }
+    });
+
+    // unpublishing a document converts the ownership to yourself, so that it
+    // will appear in your drafts rather than the original creators
     this.createdById = userId;
     this.lastModifiedById = userId;
     this.publishedAt = null;
-    await this.save(options);
-    return this;
+    return this.save();
   };
 
   // Moves a document from being visible to the team within a collection
   // to the archived area, where it can be subsequently restored.
   archive = async (userId: string) => {
-    // archive any children and remove from the document structure
-    const collection = await this.$get("collection");
-    if (collection) {
-      await collection.removeDocumentInStructure(this);
-      this.collection = collection;
-    }
+    await this.sequelize.transaction(async (transaction: Transaction) => {
+      const collection = await Collection.findByPk(this.collectionId, {
+        transaction,
+        lock: transaction.LOCK.UPDATE,
+      });
+
+      if (collection) {
+        await collection.removeDocumentInStructure(this, { transaction });
+        this.collection = collection;
+      }
+    });
 
     await this.archiveWithChildren(userId);
     return this;
@@ -750,28 +828,35 @@ class Document extends ParanoidModel {
 
   // Restore an archived document back to being visible to the team
   unarchive = async (userId: string) => {
-    const collection = await this.$get("collection");
-
-    // check to see if the documents parent hasn't been archived also
-    // If it has then restore the document to the collection root.
-    if (this.parentDocumentId) {
-      const parent = await (this.constructor as typeof Document).findOne({
-        where: {
-          id: this.parentDocumentId,
-          archivedAt: {
-            [Op.is]: null,
-          },
-        },
+    await this.sequelize.transaction(async (transaction: Transaction) => {
+      const collection = await Collection.findByPk(this.collectionId, {
+        transaction,
+        lock: transaction.LOCK.UPDATE,
       });
-      if (!parent) {
-        this.parentDocumentId = null;
-      }
-    }
 
-    if (!this.template && collection) {
-      await collection.addDocumentToStructure(this);
-      this.collection = collection;
-    }
+      // check to see if the documents parent hasn't been archived also
+      // If it has then restore the document to the collection root.
+      if (this.parentDocumentId) {
+        const parent = await (this.constructor as typeof Document).findOne({
+          where: {
+            id: this.parentDocumentId,
+            archivedAt: {
+              [Op.is]: null,
+            },
+          },
+        });
+        if (!parent) {
+          this.parentDocumentId = null;
+        }
+      }
+
+      if (!this.template && collection) {
+        await collection.addDocumentToStructure(this, undefined, {
+          transaction,
+        });
+        this.collection = collection;
+      }
+    });
 
     if (this.deletedAt) {
       await this.restore();
@@ -785,39 +870,36 @@ class Document extends ParanoidModel {
 
   // Delete a document, archived or otherwise.
   delete = (userId: string) => {
-    return this.sequelize.transaction(
-      async (transaction: Transaction): Promise<Document> => {
-        if (!this.archivedAt && !this.template) {
-          // delete any children and remove from the document structure
-          const collection = await this.$get("collection", {
-            transaction,
-          });
-          if (collection) {
-            await collection.deleteDocument(this);
-          }
-        } else {
-          await this.destroy({
-            transaction,
-          });
-        }
-
-        await Revision.destroy({
-          where: {
-            documentId: this.id,
-          },
+    return this.sequelize.transaction(async (transaction: Transaction) => {
+      if (!this.archivedAt && !this.template) {
+        // delete any children and remove from the document structure
+        const collection = await Collection.findByPk(this.collectionId, {
+          transaction,
+          lock: transaction.LOCK.UPDATE,
+        });
+        await collection?.deleteDocument(this, { transaction });
+      } else {
+        await this.destroy({
           transaction,
         });
-        await this.update(
-          {
-            lastModifiedById: userId,
-          },
-          {
-            transaction,
-          }
-        );
-        return this;
       }
-    );
+
+      await Revision.destroy({
+        where: {
+          documentId: this.id,
+        },
+        transaction,
+      });
+      await this.update(
+        {
+          lastModifiedById: userId,
+        },
+        {
+          transaction,
+        }
+      );
+      return this;
+    });
   };
 
   getTimestamp = () => {