Added currentCountQuery support for flag limits to grandfather usage

ref BAE-336
ref BAE-330

The flag limits were previously simple on/off switches that would disable features
entirely when enabled. This would cause issues when introducing new limits to
existing plans where customers are already using the affected features, as it
would immediately block their access upon implementation of new pricing.

This commit adds support for a currentCountQuery function to flag limits, allowing
them to check if a feature is already in use. When a feature is detected as being
in use, the limit won't be enforced, effectively grandfathering existing usage.
The limitAnalytics limit now includes this query to check both settings and labs
flags for the trafficAnalytics feature.

This change ensures that existing customers who are already using features won't
lose access when new limits are introduced with the upcoming pricing changes,
improving the user experience during plan transitions while still enforcing limits
for new feature adoption.

Author: aileen

packages/limit-service/README.md

@@ -158,7 +158,7 @@ db.transaction((transacting) => {
 
 ### 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.
+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. It is possible to overwrite the limit by providing a `currentCountQuery` for it. This is useful in cases where we introduce new limits to existing plans and customers have already been using the feature affected by the limit. By providing a `currentCountQuery` that detects if the feature is already in use, we won't disable it.
 2. `max` - checks if the maximum amount of the resource has been used up.Example usecase: "disable creating a staff user when maximum of 5 has been reached". To configure this limit add `max: NUMBER` to the configuration. The limits that support max checks are: `members`, and `staff`
 3. `maxPeriodic` - it's a variation of `max` type with a difference that the check is done over certain period of time. Example usecase: "disable sending emails when the sent emails count has acceded a limit for last billing period". To enable this limit define `maxPeriodic: NUMBER` in the limit configuration and provide a subscription configuration when initializing the limit service instance. The subscription object comes as a separate parameter and has to contain two properties: `startDate` and `interval`, where `startDate` is a date in  ISO 8601 format and period is `'month'` (other values like `'year'` are not supported yet)
 4. `allowList` - checks if provided value is defined in configured "allowlist". Example usecase: "disable theme activation if it is not an official theme". To configure this limit define ` allowlist: ['VALUE_1', 'VALUE_2', 'VALUE_N']` property in the "limits" parameter.

packages/limit-service/lib/config.js

@@ -57,6 +57,17 @@ module.exports = {
         formatter: count => `${count / 1000000}MB`
     },
     limitStripeConnect: {},
-    limitAnalytics: {},
+    limitAnalytics: {
+        currentCountQuery: async (knex) => {
+            const key = 'trafficAnalytics';
+            const settings = await knex('settings');
+            const setting = settings.find(s => s.key === key);
+            const labSettings = settings.find(s => s.key === 'labs');
+            const labsSetting = labSettings && labSettings.value && labSettings.value[key];
+            const labsSettingEnabled = labsSetting === true;
+
+            return (setting && setting.value === 'true') || labsSettingEnabled;
+        }
+    },
     limitActivityPub: {}
 };

packages/limit-service/lib/limit.js

@@ -264,6 +264,7 @@ class FlagLimit extends Limit {
      * @param {Number} options.config.disabled - disabled/enabled flag for the limit
      * @param {String} options.config.error - error message to use when limit is reached
      * @param {String} options.helpLink - URL to the resource explaining how the limit works
+     * @param {Function} [options.config.currentCountQuery] - query checking the state that would be compared against the limit
      * @param {Object} [options.db] - instance of knex db connection that currentCountQuery can use to run state check through
      * @param {Object} options.errors - instance of errors compatible with GhostError errors (@tryghost/errors)
      */
@@ -273,6 +274,7 @@ class FlagLimit extends Limit {
 
         this.disabled = config.disabled;
         this.fallbackMessage = `Your plan does not support ${userFacingLimitName}. Please upgrade to enable ${userFacingLimitName}.`;
+        this.currentCountQueryFn = config?.currentCountQuery || null;
     }
 
     generateError() {
@@ -288,20 +290,59 @@ class FlagLimit extends Limit {
     }
 
     /**
-     * Flag limits are on/off so using a feature is always over the limit
+     * @param {Object} [options]
+     * @param {Object} [options.transacting] Transaction to run the count query on
+     * @returns {Promise<boolean>} - returns the current count of items that would be compared against the limit
      */
-    async errorIfWouldGoOverLimit() {
-        if (this.disabled) {
+    async currentCountQuery(options = {}) {
+        if (!this.currentCountQueryFn || typeof this.currentCountQueryFn !== 'function') {
+            return false;
+        }
+
+        return await this.currentCountQueryFn(options.transacting ?? this.db?.knex);
+    }
+
+    // As Flag limits are on/off, we won't check against max values.
+    // `errorIfWouldGoOverLimit` and `errorIfIsOverLimit` end up doing the same thing.
+    async _isOrWouldOverLimitError(options = {}) {
+        if (!this.disabled) {
+            return;
+        }
+
+        // If no currentCountQuery is provided, throw error when disabled
+        if (!this.currentCountQueryFn || typeof this.currentCountQueryFn !== 'function') {
             throw this.generateError();
         }
+
+        // If currentCountQuery is provided, check if feature is in use
+        const featureInUse = await this.currentCountQuery(options);
+
+        // Only throw error if feature is NOT in use (allowing grandfathering)
+        if (!featureInUse) {
+            throw this.generateError();
+        }
+    }
+
+    /**
+     * Flag limits are usually on/off so using a feature is always over the limit,
+     * unless the limit has a currentCountQuery function provided to check if the
+     * feature is in use. This is a use case for when we introduce a new limit and
+     * customers have already been using this feature. We don't want to take it
+     * away from them.
+     */
+    async errorIfWouldGoOverLimit(options = {}) {
+        await this._isOrWouldOverLimitError(options);
     }
 
     /**
      * Flag limits are on/off. They don't necessarily mean the limit wasn't possible to reach
-     * NOTE: this method should not be relied on as it's impossible to check the limit was surpassed!
+     * Exception: the limit has a currentCountQuery function provided to check if the
+     * feature is in use. This is a use case for when we introduce a new limit and
+     * customers have already been using this feature. We don't want to take it
+     * away from them.
      */
-    async errorIfIsOverLimit() {
-        return;
+    async errorIfIsOverLimit(options = {}) {
+        await this._isOrWouldOverLimitError(options);
     }
 }
 

packages/limit-service/test/LimitService.test.js

@@ -131,17 +131,24 @@ describe('Limit Service', function () {
             let limits = {
                 staff: {max: 2},
                 members: {max: 100},
-                emails: {disabled: true}
+                emails: {disabled: true},
+                limitStripeConnect: {disabled: true},
+                limitActivityPub: {disabled: true}
             };
 
             limitService.loadLimits({limits, errors});
 
-            limitService.limits.should.be.an.Object().with.properties(['staff', 'members']);
+            limitService.limits.should.be.an.Object().with.properties(['staff', 'members', 'emails', 'limitStripeConnect', 'limitActivityPub']);
             limitService.limits.staff.should.be.an.instanceOf(MaxLimit);
             limitService.limits.members.should.be.an.instanceOf(MaxLimit);
+            limitService.limits.emails.should.be.an.instanceOf(FlagLimit);
+            limitService.limits.limitStripeConnect.should.be.an.instanceOf(FlagLimit);
+            limitService.limits.limitActivityPub.should.be.an.instanceOf(FlagLimit);
             limitService.isLimited('staff').should.be.true();
             limitService.isLimited('members').should.be.true();
             limitService.isLimited('emails').should.be.true();
+            limitService.isLimited('limitStripeConnect').should.be.true();
+            limitService.isLimited('limitActivityPub').should.be.true();
         });
 
         it('can load camel cased limits', function () {
@@ -255,6 +262,12 @@ describe('Limit Service', function () {
                 },
                 customIntegrations: {
                     disabled: true
+                },
+                limitStripeConnect: {
+                    disabled: true
+                },
+                limitActivityPub: {
+                    disabled: true
                 }
             };
 
@@ -268,7 +281,7 @@ describe('Limit Service', function () {
             (await limitService.checkIfAnyOverLimit()).should.be.true();
         });
 
-        it('Does not confirm if no limits are acceded', async function () {
+        it('Confirms when a flag limit without currentCountQuery is disabled', async function () {
             const limitService = new LimitService();
 
             let limits = {
@@ -288,10 +301,21 @@ describe('Limit Service', function () {
                 // customThemes: {
                 //     allowlist: ['casper', 'dawn', 'lyra']
                 // },
-                // NOTE: the flag limit has flawed assumption of not being acceded previously
-                //       this test might fail when the flaw is addressed
                 customIntegrations: {
                     disabled: true
+                    // No currentCountQuery - will be considered over limit
+                },
+                limitAnalytics: {
+                    disabled: true,
+                    currentCountQuery: () => true // Feature is in use, so limit won't be exceeded (grandfathered)
+                },
+                limitStripeConnect: {
+                    disabled: true
+                    // No currentCountQuery - will be considered over limit
+                },
+                limitActivityPub: {
+                    disabled: true
+                    // No currentCountQuery - will be considered over limit
                 }
             };
 
@@ -302,7 +326,8 @@ describe('Limit Service', function () {
 
             limitService.loadLimits({limits, errors, subscription});
 
-            (await limitService.checkIfAnyOverLimit()).should.be.false();
+            // Should return true because customIntegrations is disabled without currentCountQuery
+            (await limitService.checkIfAnyOverLimit()).should.be.true();
         });
 
         it('Returns nothing if limit is not configured', async function () {
@@ -480,7 +505,17 @@ describe('Limit Service', function () {
                     currentCountQuery: () => 3
                 },
                 customIntegrations: {
-                    disabled: true
+                    disabled: false // Not disabled, so won't be over limit
+                },
+                limitAnalytics: {
+                    disabled: true,
+                    currentCountQuery: () => true // Feature is in use, so limit won't be exceeded (grandfathered)
+                },
+                limitStripeConnect: {
+                    disabled: false // Not disabled, so won't be over limit
+                },
+                limitActivityPub: {
+                    disabled: false // Not disabled, so won't be over limit
                 }
             };
 
@@ -499,9 +534,11 @@ describe('Limit Service', function () {
                 testData: 'true'
             };
 
+            // Should return false because no limits are exceeded
             (await limitService.checkIfAnyOverLimit(options)).should.be.false();
 
-            sinon.assert.callCount(flagSpy, 1);
+            // We have 4 flag limits now: customIntegrations, limitAnalytics, limitStripeConnect, and limitActivityPub
+            sinon.assert.callCount(flagSpy, 4);
             sinon.assert.alwaysCalledWithExactly(flagSpy, options);
 
             sinon.assert.callCount(maxSpy, 2);

packages/limit-service/test/limit.test.js

@@ -9,16 +9,24 @@ const {MaxLimit, AllowlistLimit, FlagLimit, MaxPeriodicLimit} = require('../lib/
 
 describe('Limit Service', function () {
     describe('Flag Limit', function () {
-        it('do nothing if is over limit', async function () {
-            // NOTE: the behavior of flag limit in "is over limit" usecase is flawed and should not be relied on
-            //       possible solution could be throwing an error to prevent clients from using it?
+        it('throws if is over limit when disabled', async function () {
             const config = {
                 disabled: true
             };
-            const limit = new FlagLimit({name: 'flaggy', config, errors});
+            const limit = new FlagLimit({name: 'limitFlaggy', config, errors});
 
-            const result = await limit.errorIfIsOverLimit();
-            should(result).be.undefined();
+            try {
+                await limit.errorIfIsOverLimit();
+                should.fail(limit, 'Should have errored');
+            } catch (err) {
+                should.exist(err);
+                should.exist(err.errorType);
+                should.equal(err.errorType, 'HostLimitError');
+                should.exist(err.errorDetails);
+                should.equal(err.errorDetails.name, 'limitFlaggy');
+                should.exist(err.message);
+                should.equal(err.message, 'Your plan does not support flaggy. Please upgrade to enable flaggy.');
+            }
         });
 
         it('throws if would go over limit', async function () {
@@ -43,6 +51,78 @@ describe('Limit Service', function () {
                 should.equal(err.message, 'Your plan does not support flaggy. Please upgrade to enable flaggy.');
             }
         });
+
+        it('does not throw if feature is in use when currentCountQuery returns true', async function () {
+            const config = {
+                disabled: true,
+                currentCountQuery: () => true
+            };
+            const limit = new FlagLimit({name: 'flaggy', config, errors});
+
+            const result = await limit.errorIfIsOverLimit();
+            should(result).be.undefined();
+        });
+
+        it('throws if feature is not in use when currentCountQuery returns false', async function () {
+            const config = {
+                disabled: true,
+                currentCountQuery: () => false
+            };
+            const limit = new FlagLimit({name: 'limitFlaggy', config, errors});
+
+            try {
+                await limit.errorIfIsOverLimit();
+                should.fail(limit, 'Should have errored');
+            } catch (err) {
+                should.exist(err);
+                should.equal(err.errorType, 'HostLimitError');
+            }
+        });
+
+        it('calls currentCountQuery with transacting option', async function () {
+            const currentCountQueryStub = sinon.stub().resolves(true);
+            const config = {
+                disabled: true,
+                currentCountQuery: currentCountQueryStub
+            };
+            const db = {
+                knex: 'connection'
+            };
+            const limit = new FlagLimit({name: 'flaggy', config, db, errors});
+            const transaction = 'transaction';
+
+            await limit.errorIfIsOverLimit({transacting: transaction});
+            
+            sinon.assert.calledOnce(currentCountQueryStub);
+            sinon.assert.calledWithExactly(currentCountQueryStub, transaction);
+        });
+
+        it('errorIfWouldGoOverLimit behaves the same as errorIfIsOverLimit', async function () {
+            const config = {
+                disabled: true,
+                currentCountQuery: () => true
+            };
+            const limit = new FlagLimit({name: 'flaggy', config, errors});
+
+            // Should not throw when feature is in use
+            const result = await limit.errorIfWouldGoOverLimit();
+            should(result).be.undefined();
+
+            // Should throw when feature is not in use
+            const config2 = {
+                disabled: true,
+                currentCountQuery: () => false
+            };
+            const limit2 = new FlagLimit({name: 'limitFlaggy', config: config2, errors});
+
+            try {
+                await limit2.errorIfWouldGoOverLimit();
+                should.fail(limit2, 'Should have errored');
+            } catch (err) {
+                should.exist(err);
+                should.equal(err.errorType, 'HostLimitError');
+            }
+        });
     });
 
     describe('Max Limit', function () {