From 106a8d87474bd2d070467b1735669cda253b54cd Mon Sep 17 00:00:00 2001 From: Richard Fontein <32132657+rifont@users.noreply.github.com> Date: Thu, 16 Nov 2023 14:45:10 +0000 Subject: [PATCH 1/5] refactor(shared): Move rate limit flag from system-critical to feature-flags --- libs/shared/src/types/feature-flags/feature-flags.ts | 1 + libs/shared/src/types/feature-flags/system-critical-flags.ts | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/shared/src/types/feature-flags/feature-flags.ts b/libs/shared/src/types/feature-flags/feature-flags.ts index 56697725074..2a42dac90da 100644 --- a/libs/shared/src/types/feature-flags/feature-flags.ts +++ b/libs/shared/src/types/feature-flags/feature-flags.ts @@ -3,4 +3,5 @@ export enum FeatureFlagsKeysEnum { IS_TOPIC_NOTIFICATION_ENABLED = 'IS_TOPIC_NOTIFICATION_ENABLED', IS_MULTI_TENANCY_ENABLED = 'IS_MULTI_TENANCY_ENABLED', IS_USE_MERGED_DIGEST_ID_ENABLED = 'IS_USE_MERGED_DIGEST_ID_ENABLED', + IS_REQUEST_RATE_LIMITING_ENABLED = 'IS_REQUEST_RATE_LIMITING_ENABLED', } diff --git a/libs/shared/src/types/feature-flags/system-critical-flags.ts b/libs/shared/src/types/feature-flags/system-critical-flags.ts index 946f6d40a75..23a1a0ffb8d 100644 --- a/libs/shared/src/types/feature-flags/system-critical-flags.ts +++ b/libs/shared/src/types/feature-flags/system-critical-flags.ts @@ -1,4 +1,3 @@ export enum SystemCriticalFlagsEnum { IS_IN_MEMORY_CLUSTER_MODE_ENABLED = 'IS_IN_MEMORY_CLUSTER_MODE_ENABLED', - IS_REQUEST_RATE_LIMITING_ENABLED = 'IS_REQUEST_RATE_LIMITING_ENABLED', } From 1a8f58cb623ff993f5bfd9089828794bc1bef825 Mon Sep 17 00:00:00 2001 From: Richard Fontein <32132657+rifont@users.noreply.github.com> Date: Thu, 16 Nov 2023 14:46:25 +0000 Subject: [PATCH 2/5] refactor(application-generic): Convert rate limit flag from system-critical to feature-flags --- .../get-feature-flag/get-feature-flag.test.ts | 27 +++++++++++++++++++ ...-request-rate-limiting-enabled.use-case.ts | 17 ++++++------ .../get-system-critical-flag.test.ts | 27 +------------------ 3 files changed, 37 insertions(+), 34 deletions(-) diff --git a/packages/application-generic/src/usecases/get-feature-flag/get-feature-flag.test.ts b/packages/application-generic/src/usecases/get-feature-flag/get-feature-flag.test.ts index c278401e575..76d80a04ce4 100644 --- a/packages/application-generic/src/usecases/get-feature-flag/get-feature-flag.test.ts +++ b/packages/application-generic/src/usecases/get-feature-flag/get-feature-flag.test.ts @@ -1,4 +1,5 @@ import { + GetIsRequestRateLimitingEnabled, GetIsTemplateStoreEnabled, GetIsTopicNotificationEnabled, } from './index'; @@ -75,6 +76,32 @@ describe('Get Feature Flag', () => { expect(result).toEqual(false); }); }); + + describe('IS_REQUEST_RATE_LIMITING_ENABLED', () => { + it('should return default hardcoded value when no SDK env is set and no feature flag is set', async () => { + process.env.IS_REQUEST_RATE_LIMITING_ENABLED = ''; + + const getIsRequestRateLimitingEnabled = + new GetIsRequestRateLimitingEnabled(new FeatureFlagsService()); + + const result = await getIsRequestRateLimitingEnabled.execute( + featureFlagCommand + ); + expect(result).toEqual(false); + }); + + it('should return env variable value when no SDK env is set but the feature flag is set', async () => { + process.env.IS_REQUEST_RATE_LIMITING_ENABLED = 'true'; + + const getIsRequestRateLimitingEnabled = + new GetIsRequestRateLimitingEnabled(new FeatureFlagsService()); + + const result = await getIsRequestRateLimitingEnabled.execute( + featureFlagCommand + ); + expect(result).toEqual(true); + }); + }); }); describe('SDK key environment variable is set', () => { diff --git a/packages/application-generic/src/usecases/get-feature-flag/get-is-request-rate-limiting-enabled.use-case.ts b/packages/application-generic/src/usecases/get-feature-flag/get-is-request-rate-limiting-enabled.use-case.ts index 46fbd8a01b5..3b495cee5a4 100644 --- a/packages/application-generic/src/usecases/get-feature-flag/get-is-request-rate-limiting-enabled.use-case.ts +++ b/packages/application-generic/src/usecases/get-feature-flag/get-is-request-rate-limiting-enabled.use-case.ts @@ -1,21 +1,22 @@ import { Injectable } from '@nestjs/common'; -import { SystemCriticalFlagsEnum } from '@novu/shared'; +import { FeatureFlagsKeysEnum } from '@novu/shared'; -import { GetSystemCriticalFlag } from './get-system-critical-flag.use-case'; +import { FeatureFlagCommand } from './get-feature-flag.command'; +import { GetFeatureFlag } from './get-feature-flag.use-case'; @Injectable() -export class GetIsRequestRateLimitingEnabled extends GetSystemCriticalFlag { - execute(): boolean { +export class GetIsRequestRateLimitingEnabled extends GetFeatureFlag { + async execute(featureFlagCommand: FeatureFlagCommand): Promise { const value = process.env.IS_REQUEST_RATE_LIMITING_ENABLED; const fallbackValue = false; - const defaultValue = this.prepareBooleanStringSystemCriticalFlag( + const defaultValue = this.prepareBooleanStringFeatureFlag( value, fallbackValue ); - const key = SystemCriticalFlagsEnum.IS_REQUEST_RATE_LIMITING_ENABLED; + const key = FeatureFlagsKeysEnum.IS_REQUEST_RATE_LIMITING_ENABLED; - this.log(key, defaultValue); + const command = this.buildCommand(key, defaultValue, featureFlagCommand); - return defaultValue; + return await this.featureFlagsService.getWithContext(command); } } diff --git a/packages/application-generic/src/usecases/get-feature-flag/get-system-critical-flag.test.ts b/packages/application-generic/src/usecases/get-feature-flag/get-system-critical-flag.test.ts index 39bcd15cdcf..0e37f47cfeb 100644 --- a/packages/application-generic/src/usecases/get-feature-flag/get-system-critical-flag.test.ts +++ b/packages/application-generic/src/usecases/get-feature-flag/get-system-critical-flag.test.ts @@ -1,7 +1,4 @@ -import { - GetIsInMemoryClusterModeEnabled, - GetIsRequestRateLimitingEnabled, -} from './index'; +import { GetIsInMemoryClusterModeEnabled } from './index'; describe('Get System Critical Flag', () => { describe('SystemCriticalFlagEnum.IS_IN_MEMORY_CLUSTER_MODE_ENABLED', () => { @@ -53,26 +50,4 @@ describe('Get System Critical Flag', () => { expect(result).toEqual(false); }); }); - - describe('SystemCriticalFlagEnum.IS_REQUEST_RATE_LIMITING_ENABLED', () => { - it('should return default hardcoded value when no environment variable is set', async () => { - process.env.IS_REQUEST_RATE_LIMITING_ENABLED = ''; - - const getIsRequestRateLimitingEnabled = - new GetIsRequestRateLimitingEnabled(); - - const result = getIsRequestRateLimitingEnabled.execute(); - expect(result).toEqual(false); - }); - - it('should return environment variable value when environment variable is set', async () => { - process.env.IS_REQUEST_RATE_LIMITING_ENABLED = 'true'; - - const getIsRequestRateLimitingEnabled = - new GetIsRequestRateLimitingEnabled(); - - const result = getIsRequestRateLimitingEnabled.execute(); - expect(result).toEqual(true); - }); - }); }); From 1d45ece2de66920acd1e4d789ea3632385025d24 Mon Sep 17 00:00:00 2001 From: Richard Fontein <32132657+rifont@users.noreply.github.com> Date: Thu, 16 Nov 2023 14:47:43 +0000 Subject: [PATCH 3/5] feat(application-generic): Create custom provider for rate limit feature flag --- .../src/custom-providers/index.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/application-generic/src/custom-providers/index.ts b/packages/application-generic/src/custom-providers/index.ts index 0a36030d08c..17eaed6b9ad 100644 --- a/packages/application-generic/src/custom-providers/index.ts +++ b/packages/application-generic/src/custom-providers/index.ts @@ -12,6 +12,7 @@ import { WorkflowQueueService, } from '../services'; import { + GetIsRequestRateLimitingEnabled, GetIsTopicNotificationEnabled, GetUseMergedDigestId, } from '../usecases'; @@ -50,6 +51,20 @@ export const getIsTopicNotificationEnabled = { inject: [FeatureFlagsService], }; +export const getIsRequestRateLimitingEnabled = { + provide: GetIsRequestRateLimitingEnabled, + useFactory: async ( + featureFlagsServiceItem: FeatureFlagsService + ): Promise => { + const useCase = new GetIsRequestRateLimitingEnabled( + featureFlagsServiceItem + ); + + return useCase; + }, + inject: [FeatureFlagsService], +}; + export const cacheInMemoryProviderService = { provide: CacheInMemoryProviderService, useFactory: (): CacheInMemoryProviderService => { From 897119b2655e2e4e9118a029beea5d0944660e29 Mon Sep 17 00:00:00 2001 From: Richard Fontein <32132657+rifont@users.noreply.github.com> Date: Thu, 16 Nov 2023 14:53:21 +0000 Subject: [PATCH 4/5] chore(api, shared, application-generic): Rename api rate limiting feature flag for consistency --- apps/api/src/.env.development | 2 +- apps/api/src/.env.production | 2 +- apps/api/src/.env.test | 2 +- apps/api/src/.example.env | 2 +- .../src/types/feature-flags/feature-flags.ts | 2 +- .../src/custom-providers/index.ts | 12 +++++----- .../get-feature-flag/get-feature-flag.test.ts | 22 ++++++++++--------- ...-request-rate-limiting-enabled.use-case.ts | 6 ++--- .../src/usecases/get-feature-flag/index.ts | 2 +- 9 files changed, 26 insertions(+), 26 deletions(-) diff --git a/apps/api/src/.env.development b/apps/api/src/.env.development index 2bee90979c8..a714a764ded 100644 --- a/apps/api/src/.env.development +++ b/apps/api/src/.env.development @@ -25,7 +25,7 @@ REDIS_CACHE_FAMILY= REDIS_CACHE_KEY_PREFIX= REDIS_CACHE_ENABLE_AUTOPIPELINING=true -IS_REQUEST_RATE_LIMITING_ENABLED=false +IS_API_RATE_LIMITING_ENABLED=false IS_IN_MEMORY_CLUSTER_MODE_ENABLED=false ELASTICACHE_CLUSTER_SERVICE_HOST= ELASTICACHE_CLUSTER_SERVICE_PORT= diff --git a/apps/api/src/.env.production b/apps/api/src/.env.production index d7fe721448c..93b7d61eb5b 100644 --- a/apps/api/src/.env.production +++ b/apps/api/src/.env.production @@ -13,7 +13,7 @@ REDIS_PORT=6379 REDIS_PREFIX= REDIS_DB_INDEX=2 -IS_REQUEST_RATE_LIMITING_ENABLED=false +IS_API_RATE_LIMITING_ENABLED=false IS_IN_MEMORY_CLUSTER_MODE_ENABLED=false ELASTICACHE_CLUSTER_SERVICE_HOST= ELASTICACHE_CLUSTER_SERVICE_PORT= diff --git a/apps/api/src/.env.test b/apps/api/src/.env.test index 4c4e94f8a1f..0f469e0c054 100644 --- a/apps/api/src/.env.test +++ b/apps/api/src/.env.test @@ -23,7 +23,7 @@ REDIS_CACHE_FAMILY= REDIS_CACHE_KEY_PREFIX= REDIS_CACHE_ENABLE_AUTOPIPELINING=false -IS_REQUEST_RATE_LIMITING_ENABLED=false +IS_API_RATE_LIMITING_ENABLED=false IS_IN_MEMORY_CLUSTER_MODE_ENABLED=false ELASTICACHE_CLUSTER_SERVICE_HOST= ELASTICACHE_CLUSTER_SERVICE_PORT= diff --git a/apps/api/src/.example.env b/apps/api/src/.example.env index 688ddb2bf25..486452b6788 100644 --- a/apps/api/src/.example.env +++ b/apps/api/src/.example.env @@ -23,7 +23,7 @@ REDIS_CACHE_FAMILY= REDIS_CACHE_KEY_PREFIX= REDIS_CACHE_ENABLE_AUTOPIPELINING= -IS_REQUEST_RATE_LIMITING_ENABLED=false +IS_API_RATE_LIMITING_ENABLED=false IS_IN_MEMORY_CLUSTER_MODE_ENABLED=false REDIS_CLUSTER_SERVICE_HOST= REDIS_CLUSTER_SERVICE_PORT= diff --git a/libs/shared/src/types/feature-flags/feature-flags.ts b/libs/shared/src/types/feature-flags/feature-flags.ts index 2a42dac90da..dda55480998 100644 --- a/libs/shared/src/types/feature-flags/feature-flags.ts +++ b/libs/shared/src/types/feature-flags/feature-flags.ts @@ -3,5 +3,5 @@ export enum FeatureFlagsKeysEnum { IS_TOPIC_NOTIFICATION_ENABLED = 'IS_TOPIC_NOTIFICATION_ENABLED', IS_MULTI_TENANCY_ENABLED = 'IS_MULTI_TENANCY_ENABLED', IS_USE_MERGED_DIGEST_ID_ENABLED = 'IS_USE_MERGED_DIGEST_ID_ENABLED', - IS_REQUEST_RATE_LIMITING_ENABLED = 'IS_REQUEST_RATE_LIMITING_ENABLED', + IS_API_RATE_LIMITING_ENABLED = 'IS_API_RATE_LIMITING_ENABLED', } diff --git a/packages/application-generic/src/custom-providers/index.ts b/packages/application-generic/src/custom-providers/index.ts index 17eaed6b9ad..d876d41fef3 100644 --- a/packages/application-generic/src/custom-providers/index.ts +++ b/packages/application-generic/src/custom-providers/index.ts @@ -12,7 +12,7 @@ import { WorkflowQueueService, } from '../services'; import { - GetIsRequestRateLimitingEnabled, + GetIsApiRateLimitingEnabled, GetIsTopicNotificationEnabled, GetUseMergedDigestId, } from '../usecases'; @@ -51,14 +51,12 @@ export const getIsTopicNotificationEnabled = { inject: [FeatureFlagsService], }; -export const getIsRequestRateLimitingEnabled = { - provide: GetIsRequestRateLimitingEnabled, +export const getIsApiRateLimitingEnabled = { + provide: GetIsApiRateLimitingEnabled, useFactory: async ( featureFlagsServiceItem: FeatureFlagsService - ): Promise => { - const useCase = new GetIsRequestRateLimitingEnabled( - featureFlagsServiceItem - ); + ): Promise => { + const useCase = new GetIsApiRateLimitingEnabled(featureFlagsServiceItem); return useCase; }, diff --git a/packages/application-generic/src/usecases/get-feature-flag/get-feature-flag.test.ts b/packages/application-generic/src/usecases/get-feature-flag/get-feature-flag.test.ts index 76d80a04ce4..b5ef823b802 100644 --- a/packages/application-generic/src/usecases/get-feature-flag/get-feature-flag.test.ts +++ b/packages/application-generic/src/usecases/get-feature-flag/get-feature-flag.test.ts @@ -1,5 +1,5 @@ import { - GetIsRequestRateLimitingEnabled, + GetIsApiRateLimitingEnabled, GetIsTemplateStoreEnabled, GetIsTopicNotificationEnabled, } from './index'; @@ -77,26 +77,28 @@ describe('Get Feature Flag', () => { }); }); - describe('IS_REQUEST_RATE_LIMITING_ENABLED', () => { + describe('IS_API_RATE_LIMITING_ENABLED', () => { it('should return default hardcoded value when no SDK env is set and no feature flag is set', async () => { - process.env.IS_REQUEST_RATE_LIMITING_ENABLED = ''; + process.env.IS_API_RATE_LIMITING_ENABLED = ''; - const getIsRequestRateLimitingEnabled = - new GetIsRequestRateLimitingEnabled(new FeatureFlagsService()); + const getIsApiRateLimitingEnabled = new GetIsApiRateLimitingEnabled( + new FeatureFlagsService() + ); - const result = await getIsRequestRateLimitingEnabled.execute( + const result = await getIsApiRateLimitingEnabled.execute( featureFlagCommand ); expect(result).toEqual(false); }); it('should return env variable value when no SDK env is set but the feature flag is set', async () => { - process.env.IS_REQUEST_RATE_LIMITING_ENABLED = 'true'; + process.env.IS_API_RATE_LIMITING_ENABLED = 'true'; - const getIsRequestRateLimitingEnabled = - new GetIsRequestRateLimitingEnabled(new FeatureFlagsService()); + const getIsApiRateLimitingEnabled = new GetIsApiRateLimitingEnabled( + new FeatureFlagsService() + ); - const result = await getIsRequestRateLimitingEnabled.execute( + const result = await getIsApiRateLimitingEnabled.execute( featureFlagCommand ); expect(result).toEqual(true); diff --git a/packages/application-generic/src/usecases/get-feature-flag/get-is-request-rate-limiting-enabled.use-case.ts b/packages/application-generic/src/usecases/get-feature-flag/get-is-request-rate-limiting-enabled.use-case.ts index 3b495cee5a4..be34c7e1379 100644 --- a/packages/application-generic/src/usecases/get-feature-flag/get-is-request-rate-limiting-enabled.use-case.ts +++ b/packages/application-generic/src/usecases/get-feature-flag/get-is-request-rate-limiting-enabled.use-case.ts @@ -5,15 +5,15 @@ import { FeatureFlagCommand } from './get-feature-flag.command'; import { GetFeatureFlag } from './get-feature-flag.use-case'; @Injectable() -export class GetIsRequestRateLimitingEnabled extends GetFeatureFlag { +export class GetIsApiRateLimitingEnabled extends GetFeatureFlag { async execute(featureFlagCommand: FeatureFlagCommand): Promise { - const value = process.env.IS_REQUEST_RATE_LIMITING_ENABLED; + const value = process.env.IS_API_RATE_LIMITING_ENABLED; const fallbackValue = false; const defaultValue = this.prepareBooleanStringFeatureFlag( value, fallbackValue ); - const key = FeatureFlagsKeysEnum.IS_REQUEST_RATE_LIMITING_ENABLED; + const key = FeatureFlagsKeysEnum.IS_API_RATE_LIMITING_ENABLED; const command = this.buildCommand(key, defaultValue, featureFlagCommand); diff --git a/packages/application-generic/src/usecases/get-feature-flag/index.ts b/packages/application-generic/src/usecases/get-feature-flag/index.ts index 3b177d2f159..10429a55033 100644 --- a/packages/application-generic/src/usecases/get-feature-flag/index.ts +++ b/packages/application-generic/src/usecases/get-feature-flag/index.ts @@ -4,7 +4,7 @@ export { } from './get-feature-flag.command'; export { GetFeatureFlag } from './get-feature-flag.use-case'; export { GetIsInMemoryClusterModeEnabled } from './get-is-in-memory-cluster-mode-enabled.use-case'; -export { GetIsRequestRateLimitingEnabled } from './get-is-request-rate-limiting-enabled.use-case'; +export { GetIsApiRateLimitingEnabled } from './get-is-request-rate-limiting-enabled.use-case'; export { GetIsTemplateStoreEnabled } from './get-is-template-store-enabled.use-case'; export { GetIsTopicNotificationEnabled } from './get-is-topic-notification-enabled.use-case'; export { GetUseMergedDigestId } from './get-use-merged-digest-id.use-case'; From 0ed6fe8a04ae65d2324882feb73c843782290f7e Mon Sep 17 00:00:00 2001 From: Richard Fontein <32132657+rifont@users.noreply.github.com> Date: Thu, 16 Nov 2023 15:00:09 +0000 Subject: [PATCH 5/5] fix(app-generic): Rename api rate limiting file --- ...use-case.ts => get-is-api-rate-limiting-enabled.use-case.ts} | 0 .../application-generic/src/usecases/get-feature-flag/index.ts | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename packages/application-generic/src/usecases/get-feature-flag/{get-is-request-rate-limiting-enabled.use-case.ts => get-is-api-rate-limiting-enabled.use-case.ts} (100%) diff --git a/packages/application-generic/src/usecases/get-feature-flag/get-is-request-rate-limiting-enabled.use-case.ts b/packages/application-generic/src/usecases/get-feature-flag/get-is-api-rate-limiting-enabled.use-case.ts similarity index 100% rename from packages/application-generic/src/usecases/get-feature-flag/get-is-request-rate-limiting-enabled.use-case.ts rename to packages/application-generic/src/usecases/get-feature-flag/get-is-api-rate-limiting-enabled.use-case.ts diff --git a/packages/application-generic/src/usecases/get-feature-flag/index.ts b/packages/application-generic/src/usecases/get-feature-flag/index.ts index 10429a55033..8dc07e0e0aa 100644 --- a/packages/application-generic/src/usecases/get-feature-flag/index.ts +++ b/packages/application-generic/src/usecases/get-feature-flag/index.ts @@ -4,7 +4,7 @@ export { } from './get-feature-flag.command'; export { GetFeatureFlag } from './get-feature-flag.use-case'; export { GetIsInMemoryClusterModeEnabled } from './get-is-in-memory-cluster-mode-enabled.use-case'; -export { GetIsApiRateLimitingEnabled } from './get-is-request-rate-limiting-enabled.use-case'; +export { GetIsApiRateLimitingEnabled } from './get-is-api-rate-limiting-enabled.use-case'; export { GetIsTemplateStoreEnabled } from './get-is-template-store-enabled.use-case'; export { GetIsTopicNotificationEnabled } from './get-is-topic-notification-enabled.use-case'; export { GetUseMergedDigestId } from './get-use-merged-digest-id.use-case';