diff --git a/package.json b/package.json index c7ac1588..10d6f0b4 100644 --- a/package.json +++ b/package.json @@ -141,7 +141,6 @@ "react-router-dom": "^4.3.1", "react-waypoint": "^7.3.1", "redis": "^2.6.2", - "redis-lock": "^0.1.0", "rich-markdown-editor": "^9.6.1", "safestart": "1.1.0", "sequelize": "^5.8.12", @@ -149,8 +148,8 @@ "sequelize-encrypted": "^0.1.0", "slug": "^1.0.0", "socket.io": "^2.2.0", - "socketio-auth": "^0.1.1", "socket.io-redis": "^5.2.0", + "socketio-auth": "^0.1.1", "string-replace-to-array": "^1.0.3", "style-loader": "^0.18.2", "styled-components": "^4.2.0", diff --git a/server/models/Collection.js b/server/models/Collection.js index 990d9fe1..f030371e 100644 --- a/server/models/Collection.js +++ b/server/models/Collection.js @@ -3,7 +3,6 @@ import { find, remove } from 'lodash'; import slug from 'slug'; import randomstring from 'randomstring'; import { DataTypes, sequelize } from '../sequelize'; -import { asyncLock } from '../redis'; import Document from './Document'; import CollectionUser from './CollectionUser'; import { welcomeMessage } from '../utils/onboarding'; @@ -143,53 +142,65 @@ Collection.prototype.addDocumentToStructure = async function( ) { if (!this.documentStructure) return; - let unlock; + let transaction; - // documentStructure can only be updated by one request at a time - if (options.save !== false) { - unlock = await asyncLock(`collection-${this.id}`); - } + try { + // documentStructure can only be updated by one request at a time + if (options.save !== false) { + transaction = await sequelize.transaction(); + } - // 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 => { - 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; - }); + // If moving existing document with children, use existing structure + const documentJson = { + ...document.toJSON(), + ...options.documentJson, }; - this.documentStructure = placeDocument(this.documentStructure); - } - // Sequelize doesn't seem to set the value with splice on JSONB field - this.documentStructure = this.documentStructure; + 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 => { + 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 (options.save !== false) { - await this.save(options); - if (unlock) unlock(); + return childDocument; + }); + }; + this.documentStructure = placeDocument(this.documentStructure); + } + + // Sequelize doesn't seem to set the value with splice on JSONB field + this.documentStructure = this.documentStructure; + + if (options.save !== false) { + await this.save({ + ...options, + transaction, + }); + if (transaction) { + await transaction.commit(); + } + } + } catch (err) { + if (transaction) { + await transaction.rollback(); + } + throw err; } return this; @@ -203,28 +214,39 @@ Collection.prototype.updateDocument = async function( ) { if (!this.documentStructure) return; - // documentStructure can only be updated by one request at the time - const unlock = await asyncLock(`collection-${this.id}`); + let transaction; - const { id } = updatedDocument; + try { + // documentStructure can only be updated by one request at the time + transaction = await sequelize.transaction(); - const updateChildren = documents => { - return documents.map(document => { - if (document.id === id) { - document = { - ...updatedDocument.toJSON(), - children: document.children, - }; - } else { - document.children = updateChildren(document.children); - } - return document; - }); - }; + const { id } = updatedDocument; + + const updateChildren = documents => { + return documents.map(document => { + if (document.id === id) { + document = { + ...updatedDocument.toJSON(), + children: document.children, + }; + } else { + document.children = updateChildren(document.children); + } + return document; + }); + }; + + this.documentStructure = updateChildren(this.documentStructure); + await this.save({ transaction }); + await transaction.commit(); + + } catch (err) { + if (transaction) { + await transaction.rollback(); + } + throw err; + } - this.documentStructure = updateChildren(this.documentStructure); - await this.save(); - unlock(); return this; }; @@ -239,37 +261,48 @@ Collection.prototype.removeDocumentInStructure = async function( ) { if (!this.documentStructure) return; let returnValue; - let unlock; + let transaction; - // documentStructure can only be updated by one request at the time - unlock = await asyncLock(`collection-${this.id}`); + try { + // documentStructure can only be updated by one request at the time + transaction = await sequelize.transaction(); - const removeFromChildren = async (children, id) => { - children = await Promise.all( - children.map(async childDocument => { - return { - ...childDocument, - children: await removeFromChildren(childDocument.children, id), - }; - }) + const removeFromChildren = async (children, id) => { + children = await Promise.all( + children.map(async childDocument => { + return { + ...childDocument, + children: await removeFromChildren(childDocument.children, id), + }; + }) + ); + + const match = find(children, { id }); + if (match) { + if (!returnValue) returnValue = match; + remove(children, { id }); + } + + return children; + }; + + this.documentStructure = await removeFromChildren( + this.documentStructure, + document.id ); - const match = find(children, { id }); - if (match) { - if (!returnValue) returnValue = match; - remove(children, { id }); + await this.save({ + ...options, + transaction, + }); + await transaction.commit(); + + } catch (err) { + if (transaction) { + await transaction.rollback(); } - - return children; - }; - - this.documentStructure = await removeFromChildren( - this.documentStructure, - document.id - ); - - await this.save(options); - if (unlock) await unlock(); + throw err; + } return returnValue; }; diff --git a/server/redis.js b/server/redis.js index f3ce2537..6c48b0ce 100644 --- a/server/redis.js +++ b/server/redis.js @@ -1,11 +1,6 @@ // @flow import redis from 'redis'; -import redisLock from 'redis-lock'; const client = redis.createClient(process.env.REDIS_URL); -const lock = redisLock(client); -const asyncLock = (lockName: string): * => - new Promise(resolve => lock(lockName, unlock => resolve(unlock))); - -export { client, asyncLock }; +export { client }; diff --git a/yarn.lock b/yarn.lock index 0ddef90b..a350690e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7590,10 +7590,6 @@ redis-errors@^1.0.0, redis-errors@^1.2.0: version "1.2.0" resolved "https://registry.yarnpkg.com/redis-errors/-/redis-errors-1.2.0.tgz#eb62d2adb15e4eaf4610c04afe1529384250abad" -redis-lock@^0.1.0: - version "0.1.4" - resolved "https://registry.yarnpkg.com/redis-lock/-/redis-lock-0.1.4.tgz#e83590bee22b5f01cdb65bfbd88d988045356272" - redis-parser@^2.6.0: version "2.6.0" resolved "https://registry.yarnpkg.com/redis-parser/-/redis-parser-2.6.0.tgz#52ed09dacac108f1a631c07e9b69941e7a19504b"