Added transaction support to limit-service (#190)

SimonBackx · web-flow · commit 4ebd10a28fee · 2022-05-12T13:40:41.000+02:00
refs TryGhost/Ghost#14780 refs https://github.com/TryGhost/Team/issues/1583 - We need transaction support in the limit-service so that we can run the count queries in the same transaction - This is required to avoid deadlocks when we check the limits when a transaction is in progress on the same tables - This issue specifically is required for newsletters, where we start a transaction when creating a newsletter. - Bumped `eslint-plugin-ghost` so we have newer ECMA features available - Updated README - Renamed `metadata` to `options` in `limit-service`
diff --git a/packages/limit-service/README.md b/packages/limit-service/README.md
@@ -17,6 +17,7 @@ or
 Below is a sample code to wire up limit service and perform few common limit checks:
 
 ```js
+const knex = require('knex');
 const errors = require('@tryghost/errors');
 const LimitService = require('@tryghost/limit-service');
 
@@ -80,15 +81,17 @@ const subscription = {
 const helpLink = 'https://ghost.org/help/';
 
 // initialize knex db connection for the limit service to use when running query checks
-const db = knex({
-    client: 'mysql',
-    connection: {
-        user: 'root',
-        password: 'toor',
-        host: 'localhost',
-        database: 'ghost',
-    }
-});
+const db = {
+    knex: knex({
+        client: 'mysql',
+        connection: {
+            user: 'root',
+            password: 'toor',
+            host: 'localhost',
+            database: 'ghost',
+        }
+    });
+};
 
 // finish initializing the limits service
 limitService.loadLimits({limits, subscription, db, helpLink, errors});
@@ -133,6 +136,22 @@ if (limitService.checkIfAnyOverLimit()) {
 }
 ```
 
+### Transactions
+
+Some limit types (`max` or `maxPeriodic`) need to fetch the current count from the database. Sometimes you need those checks to also run in a transaction. To fix that, you can pass the `transacting` option to all the available checks.
+
+```js
+db.transaction((transacting) => {
+    const options = {transacting};
+
+    await limitService.errorIfWouldGoOverLimit('newsletters', options);
+    await limitService.errorIfIsOverLimit('newsletters', options);
+    const a = await limitService.checkIsOverLimit('newsletters', options);
+    const b = await limitService.checkWouldGoOverLimit('newsletters', options);
+    const c = await limitService.checkIfAnyOverLimit(options);
+});
+```
+
 ### Types of limits
 At the moment there are four different types of limits that limit service allows to define. These types are:
 1. `flag` - is an "on/off" switch for certain feature. Example usecase: "disable all emails". It's identified by a `disabled: true` property in the "limits" configuration.
diff --git a/packages/limit-service/lib/config.js b/packages/limit-service/lib/config.js
@@ -4,14 +4,14 @@
 // 2. MaxLimit should contain a `currentCountQuery` function which would count the resources under limit
 module.exports = {
     members: {
-        currentCountQuery: async (db) => {
-            let result = await db.knex('members').count('id', {as: 'count'}).first();
+        currentCountQuery: async (knex) => {
+            let result = await knex('members').count('id', {as: 'count'}).first();
             return result.count;
         }
     },
     newsletters: {
-        currentCountQuery: async (db) => {
-            let result = await db.knex('newsletters')
+        currentCountQuery: async (knex) => {
+            let result = await knex('newsletters')
                 .count('id', {as: 'count'})
                 .where('status', '=', 'active')
                 .first();
@@ -20,8 +20,8 @@ module.exports = {
         }
     },
     emails: {
-        currentCountQuery: async (db, startDate) => {
-            let result = await db.knex('emails')
+        currentCountQuery: async (knex, startDate) => {
+            let result = await knex('emails')
                 .sum('email_count', {as: 'count'})
                 .where('created_at', '>=', startDate)
                 .first();
@@ -30,13 +30,13 @@ module.exports = {
         }
     },
     staff: {
-        currentCountQuery: async (db) => {
-            let result = await db.knex('users')
+        currentCountQuery: async (knex) => {
+            let result = await knex('users')
                 .select('users.id')
                 .leftJoin('roles_users', 'users.id', 'roles_users.user_id')
                 .leftJoin('roles', 'roles_users.role_id', 'roles.id')
                 .whereNot('roles.name', 'Contributor').andWhereNot('users.status', 'inactive').union([
-                    db.knex('invites')
+                    knex('invites')
                         .select('invites.id')
                         .leftJoin('roles', 'invites.role_id', 'roles.id')
                         .whereNot('roles.name', 'Contributor')
@@ -46,8 +46,8 @@ module.exports = {
         }
     },
     customIntegrations: {
-        currentCountQuery: async (db) => {
-            let result = await db.knex('integrations')
+        currentCountQuery: async (knex) => {
+            let result = await knex('integrations')
                 .count('id', {as: 'count'})
                 .whereNotIn('type', ['internal', 'builtin'])
                 .first();
diff --git a/packages/limit-service/lib/limit-service.js b/packages/limit-service/lib/limit-service.js
@@ -67,13 +67,20 @@ class LimitService {
         return !!this.limits[_.camelCase(limitName)];
     }
 
-    async checkIsOverLimit(limitName) {
+    /**
+     *
+     * @param {String} limitName - name of the configured limit
+     * @param {Object} [options] - limit parameters
+     * @param {Object} [options.transacting] Transaction to run the count query on (if required for the chosen limit)
+     * @returns
+     */
+    async checkIsOverLimit(limitName, options = {}) {
         if (!this.isLimited(limitName)) {
             return;
         }
 
         try {
-            await this.limits[limitName].errorIfIsOverLimit();
+            await this.limits[limitName].errorIfIsOverLimit(options);
             return false;
         } catch (error) {
             if (error instanceof this.errors.HostLimitError) {
@@ -84,13 +91,20 @@ class LimitService {
         }
     }
 
-    async checkWouldGoOverLimit(limitName, metadata = {}) {
+    /**
+     *
+     * @param {String} limitName - name of the configured limit
+     * @param {Object} [options] - limit parameters
+     * @param {Object} [options.transacting] Transaction to run the count query on (if required for the chosen limit)
+     * @returns
+     */
+    async checkWouldGoOverLimit(limitName, options = {}) {
         if (!this.isLimited(limitName)) {
             return;
         }
 
         try {
-            await this.limits[limitName].errorIfWouldGoOverLimit(metadata);
+            await this.limits[limitName].errorIfWouldGoOverLimit(options);
             return false;
         } catch (error) {
             if (error instanceof this.errors.HostLimitError) {
@@ -104,39 +118,43 @@ class LimitService {
     /**
      *
      * @param {String} limitName - name of the configured limit
-     * @param {Object} metadata - limit parameters
+     * @param {Object} [options] - limit parameters
+     * @param {Object} [options.transacting] Transaction to run the count query on (if required for the chosen limit)
      * @returns
      */
-    async errorIfIsOverLimit(limitName, metadata = {}) {
+    async errorIfIsOverLimit(limitName, options = {}) {
         if (!this.isLimited(limitName)) {
             return;
         }
 
-        await this.limits[limitName].errorIfIsOverLimit(metadata);
+        await this.limits[limitName].errorIfIsOverLimit(options);
     }
 
     /**
      *
      * @param {String} limitName - name of the configured limit
-     * @param {Object} metadata - limit parameters
+     * @param {Object} [options] - limit parameters
+     * @param {Object} [options.transacting] Transaction to run the count query on (if required for the chosen limit)
      * @returns
      */
-    async errorIfWouldGoOverLimit(limitName, metadata = {}) {
+    async errorIfWouldGoOverLimit(limitName, options = {}) {
         if (!this.isLimited(limitName)) {
             return;
         }
 
-        await this.limits[limitName].errorIfWouldGoOverLimit(metadata);
+        await this.limits[limitName].errorIfWouldGoOverLimit(options);
     }
 
     /**
      * Checks if any of the configured limits acceded
-     *
+     * 
+     * @param {Object} [options] - limit parameters
+     * @param {Object} [options.transacting] Transaction to run the count queries on (if required for the chosen limit)
      * @returns {Promise<boolean>}
      */
-    async checkIfAnyOverLimit() {
+    async checkIfAnyOverLimit(options = {}) {
         for (const limit in this.limits) {
-            if (await this.checkIsOverLimit(limit)) {
+            if (await this.checkIsOverLimit(limit, options)) {
                 return true;
             }
         }
diff --git a/packages/limit-service/lib/limit.js b/packages/limit-service/lib/limit.js
@@ -98,8 +98,13 @@ class MaxLimit extends Limit {
         return new this.errors.HostLimitError(errorObj);
     }
 
-    async currentCountQuery() {
-        return await this.currentCountQueryFn(this.db);
+    /**
+     * @param {Object} [options]
+     * @param {Object} [options.transacting] Transaction to run the count query on
+     * @returns 
+     */
+    async currentCountQuery(options = {}) {
+        return await this.currentCountQueryFn(options.transacting ?? this.db?.knex);
     }
 
     /**
@@ -108,9 +113,11 @@ class MaxLimit extends Limit {
      * @param {Object} options
      * @param {Number} [options.max] - overrides configured default max value to perform checks against
      * @param {Number} [options.addedCount] - number of items to add to the currentCount during the check
+     * @param {Object} [options.transacting] Transaction to run the count query on
      */
-    async errorIfWouldGoOverLimit({max, addedCount = 1} = {}) {
-        let currentCount = await this.currentCountQuery();
+    async errorIfWouldGoOverLimit(options = {}) {
+        const {max, addedCount = 1} = options;
+        let currentCount = await this.currentCountQuery(options);
 
         if ((currentCount + addedCount) > (max || this.max)) {
             throw this.generateError(currentCount);
@@ -123,11 +130,12 @@ class MaxLimit extends Limit {
      * @param {Object} options
      * @param {Number} [options.max] - overrides configured default max value to perform checks against
      * @param {Number} [options.currentCount] - overrides currentCountQuery to perform checks against
+     * @param {Object} [options.transacting] Transaction to run the count query on
      */
-    async errorIfIsOverLimit({max, currentCount} = {}) {
-        currentCount = currentCount || await this.currentCountQuery();
+    async errorIfIsOverLimit(options = {}) {
+        const currentCount = options.currentCount || await this.currentCountQuery(options);
 
-        if (currentCount > (max || this.max)) {
+        if (currentCount > (options.max || this.max)) {
             throw this.generateError(currentCount);
         }
     }
@@ -202,10 +210,15 @@ class MaxPeriodicLimit extends Limit {
         return new this.errors.HostLimitError(errorObj);
     }
 
-    async currentCountQuery() {
+    /**
+     * @param {Object} [options]
+     * @param {Object} [options.transacting] Transaction to run the count query on
+     * @returns 
+     */
+    async currentCountQuery(options = {}) {
         const lastPeriodStartDate = lastPeriodStart(this.startDate, this.interval);
 
-        return await this.currentCountQueryFn(this.db, lastPeriodStartDate);
+        return await this.currentCountQueryFn(options.transacting ? options.transacting : (this.db ? this.db.knex : undefined), lastPeriodStartDate);
     }
 
     /**
@@ -214,9 +227,11 @@ class MaxPeriodicLimit extends Limit {
      * @param {Object} options
      * @param {Number} [options.max] - overrides configured default maxPeriodic value to perform checks against
      * @param {Number} [options.addedCount] - number of items to add to the currentCount during the check
+     * @param {Object} [options.transacting] Transaction to run the count query on
      */
-    async errorIfWouldGoOverLimit({max, addedCount = 1} = {}) {
-        let currentCount = await this.currentCountQuery();
+    async errorIfWouldGoOverLimit(options = {}) {
+        const {max, addedCount = 1} = options;
+        let currentCount = await this.currentCountQuery(options);
 
         if ((currentCount + addedCount) > (max || this.maxPeriodic)) {
             throw this.generateError(currentCount);
@@ -228,9 +243,11 @@ class MaxPeriodicLimit extends Limit {
      *
      * @param {Object} options
      * @param {Number} [options.max] - overrides configured default maxPeriodic value to perform checks against
+     * @param {Object} [options.transacting] Transaction to run the count query on
      */
-    async errorIfIsOverLimit({max} = {}) {
-        let currentCount = await this.currentCountQuery();
+    async errorIfIsOverLimit(options = {}) {
+        const {max} = options;
+        let currentCount = await this.currentCountQuery(options);
 
         if (currentCount > (max || this.maxPeriodic)) {
             throw this.generateError(currentCount);
diff --git a/packages/limit-service/test/limit-service.test.js b/packages/limit-service/test/limit-service.test.js
diff --git a/packages/limit-service/test/limit.test.js b/packages/limit-service/test/limit.test.js
