Reverted currentCountQuery support for flag limits

aileen · aileen · commit 84c9e88c1ec9 · 2025-06-27T20:02:08.000+04:00
ref BAE-331

The currentCountQuery functionality for flag limits introduced unexpected behavior that allowed disabled features to remain enabled when already in use. This grandfathering mechanism was intended to prevent breaking existing customer setups when introducing new pricing limits, but it created inconsistencies in limit enforcement.

The changes remove the ability for flag limits to check if a feature is already in use and override the disabled state. Flag limits now behave as simple on/off switches - when disabled, errorIfWouldGoOverLimit throws an error, and errorIfIsOverLimit returns without checking anything. This restores the original, predictable behavior where disabled means disabled without exceptions.
diff --git a/packages/limit-service/README.md b/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 use case: "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.
+1. `flag` - is an "on/off" switch for certain feature. Example use case: "disable all emails". It's identified by a `disabled: true` property in the "limits" configuration.
 2. `max` - checks if the maximum amount of the resource has been used up.Example use case: "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 use case: "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 use case: "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.
diff --git a/packages/limit-service/lib/limit.js b/packages/limit-service/lib/limit.js
@@ -264,7 +264,6 @@ 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)
      */
@@ -274,7 +273,6 @@ 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() {
@@ -290,59 +288,20 @@ class FlagLimit extends 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
+     * Flag limits are on/off so using a feature is always over the limit
      */
-    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') {
+    async errorIfWouldGoOverLimit() {
+        if (this.disabled) {
             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
-     * 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.
+     * NOTE: this method should not be relied on as it's impossible to check the limit was surpassed!
      */
-    async errorIfIsOverLimit(options = {}) {
-        await this._isOrWouldOverLimitError(options);
+    async errorIfIsOverLimit() {
+        return;
     }
 }
 
diff --git a/packages/limit-service/test/LimitService.test.js b/packages/limit-service/test/LimitService.test.js
@@ -281,7 +281,7 @@ describe('Limit Service', function () {
             (await limitService.checkIfAnyOverLimit()).should.be.true();
         });
 
-        it('Confirms when a flag limit without currentCountQuery is disabled', async function () {
+        it('Does not check flag limits when checking if any are over limit', async function () {
             const limitService = new LimitService();
 
             let limits = {
@@ -303,19 +303,15 @@ describe('Limit Service', function () {
                 // },
                 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)
+                    disabled: true
                 },
                 limitStripeConnect: {
                     disabled: true
-                    // No currentCountQuery - will be considered over limit
                 },
                 limitSocialWeb: {
                     disabled: true
-                    // No currentCountQuery - will be considered over limit
                 }
             };
 
@@ -326,8 +322,8 @@ describe('Limit Service', function () {
 
             limitService.loadLimits({limits, errors, subscription});
 
-            // Should return true because customIntegrations is disabled without currentCountQuery
-            (await limitService.checkIfAnyOverLimit()).should.be.true();
+            // Should return false because flag limits' errorIfIsOverLimit does not throw
+            (await limitService.checkIfAnyOverLimit()).should.be.false();
         });
 
         it('Returns nothing if limit is not configured', async function () {
diff --git a/packages/limit-service/test/limit.test.js b/packages/limit-service/test/limit.test.js
@@ -9,24 +9,16 @@ const {MaxLimit, AllowlistLimit, FlagLimit, MaxPeriodicLimit} = require('../lib/
 
 describe('Limit Service', function () {
     describe('Flag Limit', function () {
-        it('throws if is over limit when disabled', async function () {
+        it('do nothing if is over limit', async function () {
+            // NOTE: the behavior of flag limit in "is over limit" use case is flawed and should not be relied on
+            // possible solution could be throwing an error to prevent clients from using it?
             const config = {
                 disabled: true
             };
-            const limit = new FlagLimit({name: 'limitFlaggy', config, errors});
+            const limit = new FlagLimit({name: 'flaggy', config, errors});
 
-            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.');
-            }
+            const result = await limit.errorIfIsOverLimit();
+            should(result).be.undefined();
         });
 
         it('throws if would go over limit', async function () {
@@ -51,78 +43,6 @@ 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 () {
