From 5e655e42f6cbadb3380dfd8b34cc3256c3dafdc8 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Fri, 18 Mar 2022 08:59:11 -0700 Subject: [PATCH] chore: `documentStructure` database locking (#3254) --- server/commands/documentMover.test.ts | 65 +++++- server/commands/documentMover.ts | 272 ++++++++++++++------------ server/commands/pinDestroyer.ts | 38 ++-- server/models/Collection.ts | 259 ++++++++++-------------- server/models/Document.test.ts | 56 ++++++ server/models/Document.ts | 260 +++++++++++++++--------- server/routes/api/documents.ts | 49 ++--- 7 files changed, 566 insertions(+), 433 deletions(-) diff --git a/server/commands/documentMover.test.ts b/server/commands/documentMover.test.ts index 64af61f6c..dd273181a 100644 --- a/server/commands/documentMover.test.ts +++ b/server/commands/documentMover.test.ts @@ -1,3 +1,5 @@ +import { sequelize } from "@server/database/sequelize"; +import Pin from "@server/models/Pin"; import { buildDocument, buildCollection, @@ -23,7 +25,7 @@ describe("documentMover", () => { expect(response.documents.length).toEqual(1); }); - it("should not error when not in source collection documentStructure", async () => { + it("should error when not in source collection documentStructure", async () => { const user = await buildUser(); const collection = await buildCollection({ teamId: user.teamId, @@ -32,14 +34,19 @@ describe("documentMover", () => { collectionId: collection.id, }); await document.archive(user.id); - const response = await documentMover({ - user, - document, - collectionId: collection.id, - ip, - }); - expect(response.collections.length).toEqual(1); - expect(response.documents.length).toEqual(1); + + let error; + try { + await documentMover({ + user, + document, + collectionId: collection.id, + ip, + }); + } catch (err) { + error = err; + } + expect(error).toBeTruthy(); }); it("should move with children", async () => { @@ -67,6 +74,7 @@ describe("documentMover", () => { expect(response.collections.length).toEqual(1); expect(response.documents.length).toEqual(1); expect(response.documents[0].collection.id).toEqual(collection.id); + expect(response.documents[0].updatedBy.id).toEqual(user.id); }); it("should move with children to another collection", async () => { @@ -105,6 +113,45 @@ describe("documentMover", () => { expect(response.documents.length).toEqual(2); expect(response.documents[0].collection.id).toEqual(newCollection.id); + expect(response.documents[0].updatedBy.id).toEqual(user.id); expect(response.documents[1].collection.id).toEqual(newCollection.id); + expect(response.documents[1].updatedBy.id).toEqual(user.id); + }); + + it("should remove associated collection pin if moved to another collection", async () => { + const { document, user, collection } = await seed(); + const newCollection = await buildCollection({ + teamId: collection.teamId, + }); + await Pin.create({ + createdById: user.id, + collectionId: collection.id, + documentId: document.id, + teamId: collection.teamId, + }); + + const response = await sequelize.transaction(async (transaction) => + documentMover({ + user, + document, + collectionId: newCollection.id, + parentDocumentId: undefined, + index: 0, + ip, + transaction, + }) + ); + + const pinCount = await Pin.count(); + expect(pinCount).toBe(0); + + // check collection structure updated + expect(response.collections[0].id).toBe(collection.id); + expect(response.collections[1].id).toBe(newCollection.id); + expect(response.collections.length).toEqual(2); + expect(response.documents.length).toEqual(1); + + expect(response.documents[0].collection.id).toEqual(newCollection.id); + expect(response.documents[0].updatedBy.id).toEqual(user.id); }); }); diff --git a/server/commands/documentMover.ts b/server/commands/documentMover.ts index bd2ca461b..89d124b4f 100644 --- a/server/commands/documentMover.ts +++ b/server/commands/documentMover.ts @@ -1,6 +1,6 @@ import invariant from "invariant"; import { Transaction } from "sequelize"; -import { sequelize } from "@server/database/sequelize"; +import { ValidationError } from "@server/errors"; import { APM } from "@server/logging/tracing"; import { User, Document, Collection, Pin, Event } from "@server/models"; import pinDestroyer from "./pinDestroyer"; @@ -12,6 +12,7 @@ type Props = { parentDocumentId?: string | null; index?: number; ip: string; + transaction?: Transaction; }; type Result = { @@ -28,8 +29,8 @@ async function documentMover({ // convert undefined to null so parentId comparison treats them as equal index, ip, + transaction, }: Props): Promise { - let transaction: Transaction | undefined; const collectionChanged = collectionId !== document.collectionId; const previousCollectionId = document.collectionId; const result: Result = { @@ -38,154 +39,165 @@ async function documentMover({ collectionChanged, }; - if (document.template) { - if (!collectionChanged) { - return result; - } + if (document.template && !collectionChanged) { + return result; + } + if (document.template) { document.collectionId = collectionId; document.parentDocumentId = null; document.lastModifiedById = user.id; document.updatedBy = user; - await document.save(); + await document.save({ transaction }); result.documents.push(document); } else { - try { - transaction = await sequelize.transaction(); + // Load the current and the next collection upfront and lock them + const collection = await Collection.findByPk(document.collectionId, { + transaction, + lock: Transaction.LOCK.UPDATE, + paranoid: false, + }); - // remove from original collection - const collection = await Collection.findByPk(document.collectionId, { + let newCollection = collectionChanged + ? await Collection.findByPk(collectionId, { + transaction, + lock: Transaction.LOCK.UPDATE, + }) + : collection; + + invariant(newCollection, "collection should exist"); + + // Remove the document from the current collection + const response = await collection?.removeDocumentInStructure(document, { + transaction, + }); + + const documentJson = response?.[0]; + const fromIndex = response?.[1] || 0; + + if (!documentJson) { + throw ValidationError("The document was not found in the collection"); + } + + // if we're reordering from within the same parent + // the original and destination collection are the same, + // so when the initial item is removed above, the list will reduce by 1. + // We need to compensate for this when reordering + const toIndex = + index !== undefined && + document.parentDocumentId === parentDocumentId && + document.collectionId === collectionId && + fromIndex < index + ? index - 1 + : index; + + // Update the properties on the document record + document.collectionId = collectionId; + document.parentDocumentId = parentDocumentId; + document.lastModifiedById = user.id; + document.updatedBy = user; + + // Add the document and it's tree to the new collection + await newCollection.addDocumentToStructure(document, toIndex, { + documentJson, + transaction, + }); + + if (collection) { + result.collections.push(collection); + } + + // If the collection has changed then we also need to update the properties + // on all of the documents children to reflect the new collectionId + if (collectionChanged) { + // Reload the collection to get relationship data + newCollection = await Collection.scope({ + method: ["withMembership", user.id], + }).findByPk(collectionId, { + transaction, + }); + invariant(newCollection, "collection should exist"); + + result.collections.push(newCollection); + + // Efficiently find the ID's of all the documents that are children of + // the moved document and update in one query + const childDocumentIds = await document.getChildDocumentIds(); + await Document.update( + { + collectionId: newCollection.id, + }, + { + transaction, + where: { + id: childDocumentIds, + }, + } + ); + + // We must reload from the database to get the relationship data + const documents = await Document.findAll({ + where: { + id: childDocumentIds, + }, transaction, - paranoid: false, }); - const response = await collection?.removeDocumentInStructure(document, { - save: false, + document.collection = newCollection; + result.documents.push( + ...documents.map((document) => { + if (newCollection) { + document.collection = newCollection; + } + return document; + }) + ); + + // If the document was pinned to the collection then we also need to + // automatically remove the pin to prevent a confusing situation where + // a document is pinned from another collection. Use the command to ensure + // the correct events are emitted. + const pin = await Pin.findOne({ + where: { + documentId: document.id, + collectionId: previousCollectionId, + }, + transaction, + lock: Transaction.LOCK.UPDATE, }); - const documentJson = response?.[0]; - const fromIndex = response?.[1] || 0; - - // if we're reordering from within the same parent - // the original and destination collection are the same, - // so when the initial item is removed above, the list will reduce by 1. - // We need to compensate for this when reordering - const toIndex = - index !== undefined && - document.parentDocumentId === parentDocumentId && - document.collectionId === collectionId && - fromIndex < index - ? index - 1 - : index; - - // if the collection is the same then it will get saved below, this - // line prevents a pointless intermediate save from occurring. - if (collectionChanged) { - await collection?.save({ + if (pin) { + await pinDestroyer({ + user, + pin, + ip, transaction, }); } - - // add to new collection (may be the same) - document.collectionId = collectionId; - document.parentDocumentId = parentDocumentId; - document.lastModifiedById = user.id; - document.updatedBy = user; - - const newCollection = collectionChanged - ? await Collection.scope({ - method: ["withMembership", user.id], - }).findByPk(collectionId, { - transaction, - }) - : collection; - - invariant(newCollection, "collection should exist"); - - await newCollection?.addDocumentToStructure(document, toIndex, { - documentJson, - }); - - if (collection) { - result.collections.push(collection); - } - - // if collection does not remain the same loop through children and change their - // collectionId and move any attachments they may have too. This includes - // archived children, otherwise their collection would be wrong once restored. - if (collectionChanged) { - result.collections.push(newCollection); - - const loopChildren = async (documentId: string) => { - const childDocuments = await Document.findAll({ - where: { - parentDocumentId: documentId, - }, - }); - await Promise.all( - childDocuments.map(async (child) => { - await loopChildren(child.id); - child.collectionId = collectionId; - await child.save(); - - if (newCollection) { - child.collection = newCollection; - } - result.documents.push(child); - }) - ); - }; - - await loopChildren(document.id); - - const pin = await Pin.findOne({ - where: { - documentId: document.id, - collectionId: previousCollectionId, - }, - }); - - if (pin) { - await pinDestroyer({ - user, - pin, - ip, - }); - } - } - - await document.save({ - transaction, - }); - - if (newCollection) { - document.collection = newCollection; - } - result.documents.push(document); - - await transaction.commit(); - } catch (err) { - if (transaction) { - await transaction.rollback(); - } - - throw err; } } - await Event.create({ - name: "documents.move", - actorId: user.id, - documentId: document.id, - collectionId, - teamId: document.teamId, - data: { - title: document.title, - collectionIds: result.collections.map((c) => c.id), - documentIds: result.documents.map((d) => d.id), + await document.save({ transaction }); + result.documents.push(document); + + await Event.create( + { + name: "documents.move", + actorId: user.id, + documentId: document.id, + collectionId, + teamId: document.teamId, + data: { + title: document.title, + collectionIds: result.collections.map((c) => c.id), + documentIds: result.documents.map((d) => d.id), + }, + ip, }, - ip, - }); + { + transaction, + } + ); // we need to send all updated models back to the client return result; diff --git a/server/commands/pinDestroyer.ts b/server/commands/pinDestroyer.ts index d97543296..594f4bcbf 100644 --- a/server/commands/pinDestroyer.ts +++ b/server/commands/pinDestroyer.ts @@ -1,5 +1,4 @@ import { Transaction } from "sequelize"; -import { sequelize } from "@server/database/sequelize"; import { Event, Pin, User } from "@server/models"; type Props = { @@ -24,31 +23,22 @@ export default async function pinDestroyer({ user, pin, ip, - transaction: t, + transaction, }: Props): Promise { - const transaction = t || (await sequelize.transaction()); + await Event.create( + { + name: "pins.delete", + modelId: pin.id, + teamId: user.teamId, + actorId: user.id, + documentId: pin.documentId, + collectionId: pin.collectionId, + ip, + }, + { transaction } + ); - try { - await Event.create( - { - name: "pins.delete", - modelId: pin.id, - teamId: user.teamId, - actorId: user.id, - documentId: pin.documentId, - collectionId: pin.collectionId, - ip, - }, - { transaction } - ); - - await pin.destroy({ transaction }); - - await transaction.commit(); - } catch (err) { - await transaction.rollback(); - throw err; - } + await pin.destroy({ transaction }); return pin; } diff --git a/server/models/Collection.ts b/server/models/Collection.ts index 5774fda1e..17acd5d6e 100644 --- a/server/models/Collection.ts +++ b/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 & { + 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; diff --git a/server/models/Document.test.ts b/server/models/Document.test.ts index e0e5fc153..715a85381 100644 --- a/server/models/Document.test.ts +++ b/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(); diff --git a/server/models/Document.ts b/server/models/Document.ts index 64548bda1..efea041dd 100644 --- a/server/models/Document.ts +++ b/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 + ): Promise => { + const getChildDocumentIds = async ( + ...parentDocumentId: string[] + ): Promise => { + 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 @@ -702,47 +754,73 @@ class Document extends ParanoidModel { return this.save(options); }; - publish = async (userId: string, options?: FindOptions) => { + 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) => { + 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 => { - 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 = () => { diff --git a/server/routes/api/documents.ts b/server/routes/api/documents.ts index 76214a85d..abab72050 100644 --- a/server/routes/api/documents.ts +++ b/server/routes/api/documents.ts @@ -6,6 +6,7 @@ import documentCreator from "@server/commands/documentCreator"; import documentImporter from "@server/commands/documentImporter"; import documentMover from "@server/commands/documentMover"; import documentPermanentDeleter from "@server/commands/documentPermanentDeleter"; +import { sequelize } from "@server/database/sequelize"; import { NotFoundError, InvalidRequestError, @@ -1041,28 +1042,11 @@ router.post("documents.update", auth(), async (ctx) => { document.lastModifiedById = user.id; const { collection } = document; const changed = document.changed(); - let transaction; - try { - transaction = await document.sequelize.transaction(); - - if (publish) { - await document.publish(user.id, { - transaction, - }); - } else { - await document.save({ - transaction, - }); - } - - await transaction.commit(); - } catch (err) { - if (transaction) { - await transaction.rollback(); - } - - throw err; + if (publish) { + await document.publish(user.id); + } else { + await document.save(); } if (publish) { @@ -1154,14 +1138,21 @@ router.post("documents.move", auth(), async (ctx) => { authorize(user, "update", parent); } - const { documents, collections, collectionChanged } = await documentMover({ - user, - document, - collectionId, - parentDocumentId, - index, - ip: ctx.request.ip, - }); + const { + documents, + collections, + collectionChanged, + } = await sequelize.transaction(async (transaction) => + documentMover({ + user, + document, + collectionId, + parentDocumentId, + index, + ip: ctx.request.ip, + transaction, + }) + ); ctx.body = { data: {