feat(config): implement multi-environment configuration management system#122
Conversation
…stem with feature flags, encryption, validation, and API
WalkthroughThis set of changes introduces a comprehensive Multi-Environment Configuration Management System to the PayStell backend. It adds new entities, services, middleware, REST API endpoints, initialization and test scripts, and extensive documentation. The system supports environment-specific configuration storage, validation, encryption, feature flags, dynamic updates, audit logging, and includes both integration and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressApp
participant ConfigMiddleware
participant ConfigController
participant ConfigService
participant DB
Client->>ExpressApp: HTTP Request (/api/config/...)
ExpressApp->>ConfigMiddleware: Inject config & feature flag access
ConfigMiddleware->>ConfigService: Fetch config/flag (cache/db)
ConfigService->>DB: Query configurations/flags
DB-->>ConfigService: Return data
ConfigService-->>ConfigMiddleware: Return config/flag
ConfigMiddleware-->>ExpressApp: Attach config/flag access to req
ExpressApp->>ConfigController: Route handler
ConfigController->>ConfigService: Perform CRUD/eval operation
ConfigService->>DB: Read/write as needed
ConfigService-->>ConfigController: Return result
ConfigController-->>ExpressApp: Send response
ExpressApp-->>Client: HTTP Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (15)
src/app.ts (1)
159-160: Consider moving import to top of file.While the functionality is correct, the route import is placed unusually late in the file. For better code organization and consistency with other route imports, consider moving this to the top with the other route imports around line 28.
+import configurationRoutes from "./routes/configurationRoutes"; // Route imports import sessionRouter from "./routes/session.routes"; // ... other imports // Later in the file, remove the late import: -// Configuration routes -import configurationRoutes from "./routes/configurationRoutes"; app.use("/api/config", configurationRoutes);src/tests/integration/configurationIntegration.test.ts (1)
276-301: Add more validation test scenarios.The current validation test only covers empty required configurations. Consider testing:
- Invalid values when
allowedValuesis specified- Expired configurations
- Type mismatches
- Invalid validation rules
it("should validate allowed values", async () => { await request(app) .post("/api/config") .set("Authorization", `Bearer ${authToken}`) .send({ key: "LOG_LEVEL", value: "invalid_level", type: "string", category: "general", allowedValues: ["error", "warn", "info", "debug"], }); const validateResponse = await request(app) .get("/api/config/validate") .set("Authorization", `Bearer ${authToken}`); expect(validateResponse.body.data.isValid).toBe(false); expect(validateResponse.body.data.errors).toContain( expect.stringContaining("LOG_LEVEL") ); }); it("should detect expired configurations", async () => { await request(app) .post("/api/config") .set("Authorization", `Bearer ${authToken}`) .send({ key: "TEMP_CONFIG", value: "temp_value", type: "string", category: "general", expiresAt: new Date(Date.now() - 1000).toISOString(), }); const validateResponse = await request(app) .get("/api/config/validate") .set("Authorization", `Bearer ${authToken}`); expect(validateResponse.body.data.warnings).toContain( expect.stringContaining("expired") ); });src/scripts/testConfigurationSystem.ts (4)
108-117: Consider deterministic testing for percentage rollouts.The percentage rollout evaluation for
user3might not be deterministic. With a 50% rollout, the same user should consistently get the same result based on a hash of their ID.// Verify that the same user gets consistent results const evaluation1 = await configurationService.evaluateFeatureFlag("test_targeted_flag", { userId: "user3", }); const evaluation2 = await configurationService.evaluateFeatureFlag("test_targeted_flag", { userId: "user3", }); logger.info(`✅ Percentage rollout consistency: ${evaluation1.isEnabled === evaluation2.isEnabled}`);
143-157: Enhance cache performance testing with more metrics.The current cache test only measures total time. Consider adding more detailed metrics to better understand cache performance.
// Test cache performance with cold and warm cache logger.info("\n📝 Test 7: Cache Management"); // Clear cache first configurationService.clearCache(); // Cold cache test const coldStart = Date.now(); await configurationService.getConfig("TEST_CONFIG"); const coldTime = Date.now() - coldStart; logger.info(`✅ Cold cache retrieval: ${coldTime}ms`); // Warm cache test const warmStart = Date.now(); for (let i = 0; i < 10; i++) { await configurationService.getConfig("TEST_CONFIG"); } const warmTime = Date.now() - warmStart; logger.info(`✅ Warm cache retrieval (10 requests): ${warmTime}ms`); logger.info(`✅ Average warm cache time: ${warmTime / 10}ms per request`); // Clear cache configurationService.clearCache(); logger.info("✅ Cache cleared successfully");
204-211: Improve error handling in cleanup section.The cleanup errors are only logged as warnings. Consider collecting all cleanup errors and reporting them at the end.
const cleanupErrors = []; for (const configKey of testConfigs) { try { await configurationService.deleteConfig(configKey, "test-script"); logger.info(`✅ Deleted configuration: ${configKey}`); } catch (error) { cleanupErrors.push({ key: configKey, error }); logger.warn(`⚠️ Could not delete ${configKey}: ${error}`); } } if (cleanupErrors.length > 0) { logger.error(`❌ Cleanup completed with ${cleanupErrors.length} errors`); }
221-225: Consider making process.exit optional for better reusability.The hardcoded
process.exit()calls make this script difficult to use in test suites or as part of larger scripts.async function testConfigurationSystem(options = { exitOnComplete: true }) { try { // ... existing test code ... logger.info("✅ Environment-specific configurations work correctly"); if (options.exitOnComplete) { process.exit(0); } return { success: true }; } catch (error) { logger.error("❌ Configuration System Test Failed:", error); if (options.exitOnComplete) { process.exit(1); } throw error; } }src/tests/services/ConfigurationService.test.ts (1)
213-220: Make encryption test less implementation-specific.The test assumes encrypted values contain ":" which is implementation-specific. Test that the value is encrypted rather than the format.
expect(mockConfigRepository.create).toHaveBeenCalledWith( expect.objectContaining({ key: "SENSITIVE_CONFIG", - value: expect.stringContaining(":"), // Encrypted format + value: expect.not.stringMatching("secret_value"), // Value should be encrypted isEncrypted: true, }) ); + +// Additionally verify the value was actually encrypted +const savedValue = mockConfigRepository.create.mock.calls[0][0].value; +expect(savedValue).not.toBe("secret_value"); +expect(savedValue.length).toBeGreaterThan(0);src/routes/configurationRoutes.ts (2)
15-28: Add type-based validation for configuration values.The value field should be validated based on the specified type to catch type mismatches early.
const configurationValidation = [ body("key").isString().isLength({ min: 1, max: 255 }).withMessage("Key is required and must be 1-255 characters"), body("value").notEmpty().withMessage("Value is required") .custom((value, { req }) => { const type = req.body.type || ConfigurationType.STRING; switch (type) { case ConfigurationType.NUMBER: return !isNaN(Number(value)); case ConfigurationType.BOOLEAN: return ["true", "false", true, false].includes(value); case ConfigurationType.JSON: try { JSON.parse(typeof value === "string" ? value : JSON.stringify(value)); return true; } catch { return false; } default: return true; } }).withMessage("Value must match the specified type"), // ... rest of validations ];
267-274: Consider adding soft delete option for configuration deletion.Deleting configurations permanently could be risky. Consider adding a soft delete option to maintain audit history.
Add a query parameter for soft delete:
router.delete( "/:key", authMiddleware as RequestHandler, requirePermission(PermissionResource.CONFIGURATION, PermissionAction.DELETE), param("key").isString().withMessage("Key must be a string"), query("permanent").optional().isBoolean().withMessage("Permanent must be a boolean"), handleValidationErrors, configurationController.deleteConfiguration.bind(configurationController) as RequestHandler, );Update the OpenAPI documentation:
parameters: - in: query name: permanent schema: type: boolean default: false description: Permanently delete the configuration (default is soft delete)docs/CONFIGURATION_SYSTEM.md (4)
64-66: Default boolean flags should beNOT NULL
is_encrypted,is_active, andis_requiredareBOOLEANcolumns withoutNOT NULL.
Making themNOT NULL DEFAULT FALSEavoidsNULL/FALSEambiguity and simplifies ORM logic.
84-89:statusduplicatesis_enabled– remove or document differenceThe
feature_flagstable stores bothis_enabledandstatus. Unlessstatushas semantics beyond enabled/disabled (e.g., “scheduled”), the double flag increases cognitive load and can drift.
126-131: Inconsistent casing in JSON payload keysAPI section uses camelCase (
isRequired,isEncrypted) while the DB schema and code sections use snake_case (is_required,is_encrypted). Unify the convention or document the mapping layer.
378-382: Prefer AEAD mode (AES-256-GCM) over CBCAES-256-CBC provides confidentiality but no built-in integrity; misuse can lead to padding-oracle attacks. Modern best practice is an authenticated mode such as AES-256-GCM or XChaCha20-Poly1305.
src/entities/FeatureFlag.ts (1)
95-97: Consider using array type for tagsStoring tags as a comma-separated string limits querying capabilities. Consider using a JSONB array instead for better indexing and querying:
- @Column({ type: "text", nullable: true }) + @Column({ type: "jsonb", nullable: true }) @IsOptional() - tags?: string; // Comma-separated tags + tags?: string[]; // Array of tagssrc/services/ConfigurationService.ts (1)
340-354: Consider using a more efficient hashing algorithmWhile MD5 is sufficient for distribution purposes, consider using a faster non-cryptographic hash like xxHash or CityHash for better performance in high-traffic scenarios:
- const hash = crypto.createHash("md5").update(`${flagName}:${userId}`).digest("hex"); + // Use a faster hash function for better performance + const hash = someNonCryptoHash(`${flagName}:${userId}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
README.md(2 hunks)docs/CONFIGURATION_SYSTEM.md(1 hunks)package.json(1 hunks)src/app.ts(3 hunks)src/config/db.ts(2 hunks)src/controllers/ConfigurationController.ts(1 hunks)src/entities/Configuration.ts(1 hunks)src/entities/FeatureFlag.ts(1 hunks)src/index.ts(1 hunks)src/middlewares/configurationMiddleware.ts(1 hunks)src/routes/configurationRoutes.ts(1 hunks)src/scripts/initializeConfigurations.ts(1 hunks)src/scripts/testConfigurationSystem.ts(1 hunks)src/services/ConfigurationService.ts(1 hunks)src/tests/integration/configurationIntegration.test.ts(1 hunks)src/tests/services/ConfigurationService.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
src/index.ts (1)
src/services/ConfigurationService.ts (1)
configurationService(619-619)
src/app.ts (1)
src/middlewares/configurationMiddleware.ts (2)
configurationMiddleware(23-49)environmentConfigMiddleware(123-153)
src/tests/integration/configurationIntegration.test.ts (2)
src/services/ConfigurationService.ts (1)
configurationService(619-619)src/lib/services/referral-service.ts (1)
request(25-68)
src/scripts/initializeConfigurations.ts (1)
src/services/ConfigurationService.ts (1)
configurationService(619-619)
src/scripts/testConfigurationSystem.ts (1)
src/services/ConfigurationService.ts (1)
configurationService(619-619)
src/controllers/ConfigurationController.ts (1)
src/services/ConfigurationService.ts (1)
configurationService(619-619)
src/services/ConfigurationService.ts (1)
src/services/AuditService.ts (1)
AuditService(34-170)
src/entities/Configuration.ts (1)
src/entities/FeatureFlag.ts (1)
Entity(26-107)
src/entities/FeatureFlag.ts (1)
src/entities/Configuration.ts (1)
Entity(39-119)
src/middlewares/configurationMiddleware.ts (1)
src/services/ConfigurationService.ts (1)
configurationService(619-619)
🔇 Additional comments (36)
src/config/db.ts (1)
19-20: LGTM! Proper entity registration for configuration system.The new
ConfigurationandFeatureFlagentities are correctly imported and registered in the TypeORMDataSource. The import paths follow the established pattern and the entities are properly added to the entities array, enabling the ORM to manage these tables.Also applies to: 50-51
src/index.ts (1)
4-4: Excellent initialization sequence.The configuration service is properly initialized after the database connection and before the server starts listening. This ensures that all configurations and feature flags are loaded and validated before handling requests. The async/await pattern and error handling are correctly implemented.
Also applies to: 12-14
package.json (1)
16-18: Well-structured script additions.The new configuration management scripts follow established naming conventions and use the same
ts-nodepattern as the existinginit-rbacscript. The trailing comma addition maintains consistent JSON formatting.src/app.ts (1)
42-42: Proper middleware integration.The configuration middleware is correctly placed after audit middleware and before route handlers. This ensures that configuration access and environment information are available to all routes while maintaining the audit trail.
Also applies to: 122-123
README.md (1)
16-20: Comprehensive documentation for the new configuration system.The README updates effectively highlight the new multi-environment configuration management capabilities. The feature list clearly communicates the key benefits (dynamic updates, encryption, audit logging), and the configuration management section provides practical API endpoint documentation and setup instructions.
Also applies to: 72-96
src/tests/integration/configurationIntegration.test.ts (1)
215-274: Well-structured service integration tests.The service integration tests properly verify the interaction between the API and the configuration service, including correct type conversion for different data types.
src/scripts/testConfigurationSystem.ts (1)
1-58: Well-structured test initialization and basic operations.The script properly initializes the database and configuration service, with clear logging for test progress tracking.
src/tests/services/ConfigurationService.test.ts (3)
1-53: Well-structured mock setup for unit testing.The mock setup properly isolates the ConfigurationService for unit testing with appropriate repository and service mocks.
290-465: Comprehensive feature flag testing coverage.The feature flag tests thoroughly cover evaluation scenarios including disabled flags, targeting rules, and percentage rollouts.
467-536: Good coverage of validation and cache management scenarios.The tests properly verify required configuration validation, expiration checking, and cache management operations.
src/routes/configurationRoutes.ts (2)
402-588: Well-designed feature flag routes with comprehensive evaluation support.The feature flag routes properly handle different scopes and evaluation contexts with appropriate query parameters for userId, merchantId, and userRole.
677-684: Encrypted values are already masked in export endpointThe
exportConfigurationsmethod replaces any encrypted configuration values with"[ENCRYPTED]"before exporting, so no plaintext encrypted data is exposed.src/entities/Configuration.ts (1)
1-119: Well-structured entity with comprehensive configuration support.The entity design is solid with proper indexing, validation, and support for various configuration types including encryption, validation rules, and metadata.
src/entities/FeatureFlag.ts (3)
1-10: Imports look goodAll necessary TypeORM and validation decorators are properly imported. The shared
Environmentenum creates appropriate coupling between related entities.
12-24: Well-structured enums with clear string valuesThe enums use descriptive string values which improves database readability and debugging. The scope and status options comprehensively cover the feature flag use cases.
26-68: Entity structure and indexing are well-designedThe entity properly uses decorators, has appropriate indexes for performance and uniqueness constraints, and includes all necessary fields for a feature flag system.
src/controllers/ConfigurationController.ts (8)
1-9: Clean imports with proper separation of concernsThe controller properly delegates business logic to the service layer and imports only what's needed.
13-31: Excellent security practice masking encrypted valuesThe consistent masking of encrypted values in responses prevents accidental exposure of sensitive data.
60-70: Fix async function flow after validation errorAfter sending the validation error response, the function continues execution. Add proper return:
if (!errors.isEmpty()) { res.status(400).json({ success: false, message: "Validation failed", errors: errors.array(), }); return; }Actually, the return statement is already there on line 70, so this is correct.
135-142: Good validation of category parameterThe method properly validates that the category parameter is a valid enum value before proceeding.
215-259: Well-structured feature flag creation with proper date handlingThe method correctly handles date string conversion and includes audit information.
264-282: Clean feature flag evaluation implementationThe method properly extracts context from query parameters and delegates to the service for evaluation logic.
287-315: Efficient statistics aggregationThe method efficiently computes statistics in a single pass using reduce, providing valuable insights into configuration usage.
355-409: Robust import implementation with good error handlingThe import method properly:
- Validates input format
- Handles individual configuration errors without failing the entire import
- Provides detailed feedback on import results
- Respects the overwrite flag for existing configurations
src/services/ConfigurationService.ts (7)
1-25: Well-structured interfaces for type safetyThe interfaces provide clear contracts for configuration values, validation results, and feature flag evaluations.
58-76: Robust initialization with proper validationThe initialization method ensures all required configurations are present at startup, preventing runtime failures.
81-121: Efficient caching strategy with proper decryption handlingThe method implements a good caching pattern and transparently handles encrypted values.
126-215: Comprehensive configuration management with audit trailThe setConfig method properly handles:
- Create vs update logic
- Transparent encryption
- Audit logging with old/new values
- Cache invalidation
288-361: Well-implemented feature flag evaluation with comprehensive checksThe evaluation logic properly handles:
- Flag existence and active status
- Time-based scheduling
- Targeting rules evaluation
- Percentage-based rollouts with consistent hashing
506-542: Thorough validation of required configurationsThe validation method comprehensively checks:
- Presence of required values
- Expiration dates
- Allowed values constraints
618-619: Appropriate use of singleton patternThe singleton instance ensures consistent configuration state across the application.
src/middlewares/configurationMiddleware.ts (5)
5-18: Clean TypeScript interface extensionProperly extends the Express Request interface to include configuration methods with appropriate type signatures.
20-49: Well-implemented configuration injection middlewareThe middleware properly injects configuration access methods into the request object with appropriate error handling.
54-84: Useful validation middleware with clear error reportingThe middleware provides clear feedback about missing configurations, making debugging easier.
89-118: Excellent feature flag middleware with context awarenessThe middleware properly extracts user context and provides clear feedback when features are disabled.
158-188: Smart request-scoped caching implementationThe middleware adds an efficient caching layer that prevents redundant configuration lookups within the same request lifecycle.
| @Column({ type: "jsonb", nullable: true }) | ||
| @IsOptional() | ||
| targetingRules?: { | ||
| userIds?: string[]; | ||
| merchantIds?: string[]; | ||
| userRoles?: string[]; | ||
| percentage?: number; // For percentage-based rollouts | ||
| conditions?: Record<string, unknown>; | ||
| }; |
There was a problem hiding this comment.
Add percentage validation in targetingRules
The percentage field in targetingRules should be validated to ensure it's between 0 and 100. Consider adding validation logic in the service layer or database constraints.
🤖 Prompt for AI Agents
In src/entities/FeatureFlag.ts around lines 69 to 77, the percentage field
inside targetingRules lacks validation to ensure its value is between 0 and 100.
Add validation logic either by using class-validator decorators like @Min(0) and
@Max(100) on the percentage property or implement a custom validation method in
the service layer that checks this range before saving or updating the entity.
| /** | ||
| * Middleware to inject environment-specific configurations | ||
| */ | ||
| export const environmentConfigMiddleware = async ( | ||
| req: Request, | ||
| res: Response, | ||
| next: NextFunction | ||
| ): Promise<void> => { | ||
| try { | ||
| // Inject environment-specific configurations | ||
| const environment = process.env.NODE_ENV || "development"; | ||
|
|
||
| // Add environment info to request | ||
| req.environment = environment; | ||
|
|
||
| // Load common configurations | ||
| const commonConfigs = await Promise.all([ | ||
| configurationService.getConfig("APP_NAME", "PayStell"), | ||
| configurationService.getConfig("APP_VERSION", "1.0.0"), | ||
| configurationService.getConfig("ENVIRONMENT", environment), | ||
| ]); | ||
|
|
||
| req.appConfig = { | ||
| name: commonConfigs[0] as string, | ||
| version: commonConfigs[1] as string, | ||
| environment: commonConfigs[2] as string, | ||
| }; | ||
|
|
||
| next(); | ||
| } catch (error) { | ||
| logger.error("Environment config middleware error:", error); | ||
| next(error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript declarations for injected properties
The middleware adds environment and appConfig to the request but these aren't declared in the TypeScript interface:
declare global {
namespace Express {
interface Request {
config?: {
get: (key: string, defaultValue?: string) => Promise<string | number | boolean | Record<string, unknown> | null>;
isFeatureEnabled: (flagName: string, context?: {
userId?: string;
merchantId?: string;
userRole?: string;
}) => Promise<boolean>;
};
+ environment?: string;
+ appConfig?: {
+ name: string;
+ version: string;
+ environment: string;
+ };
}
}
}🤖 Prompt for AI Agents
In src/middlewares/configurationMiddleware.ts around lines 120 to 153, the
middleware adds environment and appConfig properties to the request object but
these are not declared in the TypeScript Request interface. To fix this, extend
the Express Request interface by declaring these properties in a separate type
declaration file or within the same file using module augmentation. Define
environment as a string and appConfig as an object with name, version, and
environment string properties to ensure proper typing and avoid TypeScript
errors.
| // Create a test user and get auth token | ||
| // This would normally be done through the auth system | ||
| authToken = "test-auth-token"; | ||
| }); |
There was a problem hiding this comment.
Generate proper authentication tokens for tests instead of using hardcoded values.
The hardcoded auth token "test-auth-token" bypasses actual authentication testing. Consider generating real JWT tokens or mocking the auth middleware properly to ensure authentication is correctly tested.
- // Create a test user and get auth token
- // This would normally be done through the auth system
- authToken = "test-auth-token";
+ // Create a test user and generate a proper auth token
+ const testUser = await createTestUser();
+ authToken = await generateAuthToken(testUser);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create a test user and get auth token | |
| // This would normally be done through the auth system | |
| authToken = "test-auth-token"; | |
| }); | |
| // Create a test user and generate a proper auth token | |
| const testUser = await createTestUser(); | |
| authToken = await generateAuthToken(testUser); |
🤖 Prompt for AI Agents
In src/tests/integration/configurationIntegration.test.ts around lines 20 to 23,
replace the hardcoded auth token "test-auth-token" with a properly generated
authentication token. Implement code to generate a valid JWT token using the
same secret and payload structure as the real authentication system, or mock the
authentication middleware to simulate real authentication behavior. This ensures
the tests validate authentication logic correctly instead of bypassing it.
| describe("Configuration API Endpoints", () => { | ||
| it("should create and retrieve a configuration", async () => { | ||
| // Create configuration | ||
| const createResponse = await request(app) | ||
| .post("/api/config") | ||
| .set("Authorization", `Bearer ${authToken}`) | ||
| .send({ | ||
| key: "TEST_CONFIG", | ||
| value: "test_value", | ||
| type: "string", | ||
| category: "general", | ||
| description: "Test configuration", | ||
| isRequired: false, | ||
| }); | ||
|
|
||
| expect(createResponse.status).toBe(201); | ||
| expect(createResponse.body.success).toBe(true); | ||
|
|
||
| // Retrieve configuration | ||
| const getResponse = await request(app) | ||
| .get("/api/config/TEST_CONFIG") | ||
| .set("Authorization", `Bearer ${authToken}`); | ||
|
|
||
| expect(getResponse.status).toBe(200); | ||
| expect(getResponse.body.data.value).toBe("test_value"); | ||
| }); | ||
|
|
||
| it("should handle encrypted configurations", async () => { | ||
| // Create encrypted configuration | ||
| const createResponse = await request(app) | ||
| .post("/api/config") | ||
| .set("Authorization", `Bearer ${authToken}`) | ||
| .send({ | ||
| key: "ENCRYPTED_CONFIG", | ||
| value: "secret_value", | ||
| type: "string", | ||
| category: "security", | ||
| description: "Encrypted configuration", | ||
| isEncrypted: true, | ||
| isRequired: false, | ||
| }); | ||
|
|
||
| expect(createResponse.status).toBe(201); | ||
|
|
||
| // Retrieve configuration (should show as encrypted) | ||
| const getResponse = await request(app) | ||
| .get("/api/config") | ||
| .set("Authorization", `Bearer ${authToken}`); | ||
|
|
||
| expect(getResponse.status).toBe(200); | ||
| const encryptedConfig = getResponse.body.data.find( | ||
| (config: any) => config.key === "ENCRYPTED_CONFIG" | ||
| ); | ||
| expect(encryptedConfig.value).toBe("[ENCRYPTED]"); | ||
| }); | ||
|
|
||
| it("should get configurations by category", async () => { | ||
| // Create configurations in different categories | ||
| await request(app) | ||
| .post("/api/config") | ||
| .set("Authorization", `Bearer ${authToken}`) | ||
| .send({ | ||
| key: "PAYMENT_CONFIG", | ||
| value: "100", | ||
| type: "number", | ||
| category: "payment", | ||
| description: "Payment configuration", | ||
| }); | ||
|
|
||
| await request(app) | ||
| .post("/api/config") | ||
| .set("Authorization", `Bearer ${authToken}`) | ||
| .send({ | ||
| key: "SECURITY_CONFIG", | ||
| value: "true", | ||
| type: "boolean", | ||
| category: "security", | ||
| description: "Security configuration", | ||
| }); | ||
|
|
||
| // Get configurations by category | ||
| const response = await request(app) | ||
| .get("/api/config/category/payment") | ||
| .set("Authorization", `Bearer ${authToken}`); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(response.body.data.length).toBe(1); | ||
| expect(response.body.data[0].key).toBe("PAYMENT_CONFIG"); | ||
| }); | ||
|
|
||
| it("should delete configuration", async () => { | ||
| // Create configuration | ||
| await request(app) | ||
| .post("/api/config") | ||
| .set("Authorization", `Bearer ${authToken}`) | ||
| .send({ | ||
| key: "TO_DELETE", | ||
| value: "delete_me", | ||
| type: "string", | ||
| category: "general", | ||
| }); | ||
|
|
||
| // Delete configuration | ||
| const deleteResponse = await request(app) | ||
| .delete("/api/config/TO_DELETE") | ||
| .set("Authorization", `Bearer ${authToken}`); | ||
|
|
||
| expect(deleteResponse.status).toBe(200); | ||
| expect(deleteResponse.body.success).toBe(true); | ||
|
|
||
| // Verify it's deleted | ||
| const getResponse = await request(app) | ||
| .get("/api/config/TO_DELETE") | ||
| .set("Authorization", `Bearer ${authToken}`); | ||
|
|
||
| expect(getResponse.status).toBe(404); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing test scenarios for comprehensive coverage.
The configuration API tests are missing several important scenarios:
- Updating existing configurations
- Type validation (e.g., passing string when number is expected)
- Required field validation
- Unauthorized access attempts (403 scenarios)
- Invalid data handling (400 scenarios)
- Environment-specific configuration handling
Consider adding these test cases:
it("should update existing configuration", async () => {
// Create initial configuration
await request(app)
.post("/api/config")
.set("Authorization", `Bearer ${authToken}`)
.send({
key: "UPDATE_TEST",
value: "initial_value",
type: "string",
category: "general",
});
// Update configuration
const updateResponse = await request(app)
.post("/api/config")
.set("Authorization", `Bearer ${authToken}`)
.send({
key: "UPDATE_TEST",
value: "updated_value",
type: "string",
category: "general",
});
expect(updateResponse.status).toBe(200);
expect(updateResponse.body.data.value).toBe("updated_value");
});
it("should return 403 for unauthorized access", async () => {
const response = await request(app)
.get("/api/config")
.set("Authorization", "Bearer invalid-token");
expect(response.status).toBe(403);
});
it("should validate configuration types", async () => {
const response = await request(app)
.post("/api/config")
.set("Authorization", `Bearer ${authToken}`)
.send({
key: "TYPE_TEST",
value: "not-a-number",
type: "number",
category: "general",
});
expect(response.status).toBe(400);
});🤖 Prompt for AI Agents
In src/tests/integration/configurationIntegration.test.ts between lines 38 and
155, the existing tests lack coverage for updating configurations, type
validation, required field validation, unauthorized access, invalid data
handling, and environment-specific configurations. Add new test cases to cover
these scenarios: a test to update an existing configuration and verify the
update succeeds with status 200; a test to check that unauthorized requests
return a 403 status; and a test to validate that sending incorrect types (e.g.,
string instead of number) returns a 400 error. Implement these tests following
the existing test structure and use appropriate assertions for status codes and
response bodies.
| describe("Feature Flag API Endpoints", () => { | ||
| it("should create and evaluate feature flag", async () => { | ||
| // Create feature flag | ||
| const createResponse = await request(app) | ||
| .post("/api/config/feature-flags") | ||
| .set("Authorization", `Bearer ${authToken}`) | ||
| .send({ | ||
| name: "test_feature", | ||
| description: "Test feature flag", | ||
| isEnabled: true, | ||
| scope: "global", | ||
| }); | ||
|
|
||
| expect(createResponse.status).toBe(201); | ||
| expect(createResponse.body.success).toBe(true); | ||
|
|
||
| // Evaluate feature flag | ||
| const evaluateResponse = await request(app) | ||
| .get("/api/config/feature-flags/test_feature/evaluate") | ||
| .set("Authorization", `Bearer ${authToken}`); | ||
|
|
||
| expect(evaluateResponse.status).toBe(200); | ||
| expect(evaluateResponse.body.data.isEnabled).toBe(true); | ||
| }); | ||
|
|
||
| it("should handle targeted feature flags", async () => { | ||
| // Create targeted feature flag | ||
| await request(app) | ||
| .post("/api/config/feature-flags") | ||
| .set("Authorization", `Bearer ${authToken}`) | ||
| .send({ | ||
| name: "targeted_feature", | ||
| description: "Targeted feature flag", | ||
| isEnabled: true, | ||
| scope: "user", | ||
| targetingRules: { | ||
| userIds: ["user1", "user2"], | ||
| }, | ||
| }); | ||
|
|
||
| // Evaluate with matching user | ||
| const matchResponse = await request(app) | ||
| .get("/api/config/feature-flags/targeted_feature/evaluate?userId=user1") | ||
| .set("Authorization", `Bearer ${authToken}`); | ||
|
|
||
| expect(matchResponse.status).toBe(200); | ||
| expect(matchResponse.body.data.isEnabled).toBe(true); | ||
|
|
||
| // Evaluate with non-matching user | ||
| const noMatchResponse = await request(app) | ||
| .get("/api/config/feature-flags/targeted_feature/evaluate?userId=user3") | ||
| .set("Authorization", `Bearer ${authToken}`); | ||
|
|
||
| expect(noMatchResponse.status).toBe(200); | ||
| expect(noMatchResponse.body.data.isEnabled).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand feature flag tests to cover all targeting scenarios.
The current tests only cover user-based targeting. Add tests for:
- Merchant-based targeting
- Role-based targeting
- Percentage rollouts
- Scheduled feature flags (start/end dates)
it("should handle percentage-based rollouts", async () => {
await request(app)
.post("/api/config/feature-flags")
.set("Authorization", `Bearer ${authToken}`)
.send({
name: "percentage_feature",
description: "Percentage rollout feature",
isEnabled: true,
scope: "user",
targetingRules: {
percentage: 50,
},
});
// Test multiple evaluations to verify percentage distribution
const results = [];
for (let i = 0; i < 100; i++) {
const response = await request(app)
.get(`/api/config/feature-flags/percentage_feature/evaluate?userId=user${i}`)
.set("Authorization", `Bearer ${authToken}`);
results.push(response.body.data.isEnabled);
}
const enabledCount = results.filter(r => r).length;
expect(enabledCount).toBeGreaterThan(30);
expect(enabledCount).toBeLessThan(70);
});
it("should handle role-based targeting", async () => {
await request(app)
.post("/api/config/feature-flags")
.set("Authorization", `Bearer ${authToken}`)
.send({
name: "role_feature",
description: "Role-based feature",
isEnabled: true,
scope: "user",
targetingRules: {
roles: ["admin", "super_admin"],
},
});
const adminResponse = await request(app)
.get("/api/config/feature-flags/role_feature/evaluate?userRole=admin")
.set("Authorization", `Bearer ${authToken}`);
expect(adminResponse.body.data.isEnabled).toBe(true);
const userResponse = await request(app)
.get("/api/config/feature-flags/role_feature/evaluate?userRole=user")
.set("Authorization", `Bearer ${authToken}`);
expect(userResponse.body.data.isEnabled).toBe(false);
});🤖 Prompt for AI Agents
In src/tests/integration/configurationIntegration.test.ts between lines 157 and
213, the feature flag tests currently only cover user-based targeting. Expand
these tests by adding new test cases for merchant-based targeting, role-based
targeting, percentage rollouts, and scheduled feature flags with start and end
dates. For percentage rollouts, simulate multiple evaluations to verify the
distribution falls within expected bounds. For role-based targeting, test with
roles that should and should not enable the feature. Similarly, add tests for
merchant targeting and scheduled flags to fully cover all targeting scenarios.
- Rename 'key' column to 'configKey' to avoid SQL reserved word conflicts - Remove hardcoded passwords, use environment variables instead - Improve encryption key derivation using scryptSync - Add proper TypeScript type declarations for Express Request - Add percentage validation (0-100) in targeting rules - Add CONFIGURATION permission resource - Fix user ID type conversions (number → string) - Enhance test coverage with missing scenarios - Add production confirmation for bulk imports - Fix mock decryption in unit tests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/tests/integration/configurationIntegration.test.ts (1)
20-23: Generate proper authentication tokens for tests instead of using hardcoded values.The hardcoded auth token
"test-auth-token"bypasses actual authentication testing. Consider generating real JWT tokens or mocking the auth middleware properly to ensure authentication is correctly tested.src/middlewares/configurationMiddleware.ts (1)
5-18: Add TypeScript declarations for injected propertiesThe middleware adds
environmentandappConfigproperties to the request object but these aren't declared in the TypeScript interface, which will cause TypeScript errors when accessing these properties.src/services/ConfigurationService.ts (1)
47-53: Security concern: Remove hardcoded default encryption keyUsing a hardcoded default encryption key is a critical security vulnerability. The application should fail to start if the encryption key is not properly configured.
src/routes/configurationRoutes.ts (1)
747-754: Import validation safety checks are already implemented.The suggested safety confirmations for bulk configuration import have already been implemented in the
importValidationschema (lines 43-53). The validation includes array checking, boolean validation for overwrite, and production confirmation with today's date.
🧹 Nitpick comments (4)
src/entities/Configuration.ts (1)
91-110: Consider using JSON column type for better database support.The entity stores JSON data in text columns (validationRules, allowedValues, metadata). While this works, consider using TypeORM's
jsoncolumn type for databases that support it (PostgreSQL, MySQL 5.7+). This provides better type safety and performance:- @Column({ type: "text", nullable: true }) + @Column({ type: "json", nullable: true }) @IsOptional() - validationRules?: string; // JSON string for validation rules + validationRules?: Record<string, any>;PR_DESCRIPTION.md (1)
42-58: Add language specification to the code block.The code block showing API endpoints should specify the language for proper syntax highlighting:
-``` +```bash GET /api/config # Get all configurationsdocs/CONFIGURATION_SYSTEM.md (1)
370-382: Enhance encryption key security guidanceWhile the documentation mentions storing the encryption key in an environment variable, it should emphasize that this key must be securely managed and never committed to version control.
Consider adding a security warning:
### 3. Environment Variables Ensure these environment variables are set: ```bash NODE_ENV=development -CONFIG_ENCRYPTION_KEY=your-encryption-key +CONFIG_ENCRYPTION_KEY=your-encryption-key # CRITICAL: Use a strong, randomly generated key. Never commit to version control!
+
⚠️ Security Warning: TheCONFIG_ENCRYPTION_KEYmust be:
+- At least 32 characters long
+- Randomly generated (use a tool likeopenssl rand -hex 32)
+- Stored securely (e.g., in a secrets management system)
+- Different for each environment
+- Rotated periodically</blockquote></details> <details> <summary>src/middlewares/configurationMiddleware.ts (1)</summary><blockquote> `162-192`: **Document middleware dependency and ordering requirement** The `configCacheMiddleware` assumes that `req.config` already exists, which means it must run after `configurationMiddleware`. This dependency should be documented to prevent middleware ordering issues. Add a comment to clarify the dependency: ```diff /** * Middleware to cache configuration values for the duration of the request + * + * Note: This middleware must be applied AFTER configurationMiddleware + * as it depends on req.config being available. */ export const configCacheMiddleware = async (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
PR_DESCRIPTION.md(1 hunks)docs/CONFIGURATION_SYSTEM.md(1 hunks)src/controllers/ConfigurationController.ts(1 hunks)src/entities/Configuration.ts(1 hunks)src/entities/FeatureFlag.ts(1 hunks)src/entities/Permission.ts(1 hunks)src/middlewares/configurationMiddleware.ts(1 hunks)src/routes/configurationRoutes.ts(1 hunks)src/scripts/initializeConfigurations.ts(1 hunks)src/scripts/testConfigurationSystem.ts(1 hunks)src/services/ConfigurationService.ts(1 hunks)src/tests/integration/configurationIntegration.test.ts(1 hunks)src/tests/services/ConfigurationService.test.ts(1 hunks)src/types/express.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/entities/Permission.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/tests/services/ConfigurationService.test.ts
- src/scripts/testConfigurationSystem.ts
- src/entities/FeatureFlag.ts
- src/scripts/initializeConfigurations.ts
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/tests/integration/configurationIntegration.test.ts (2)
src/services/ConfigurationService.ts (1)
configurationService(637-637)src/lib/services/referral-service.ts (1)
request(25-68)
src/middlewares/configurationMiddleware.ts (1)
src/services/ConfigurationService.ts (1)
configurationService(637-637)
src/routes/configurationRoutes.ts (3)
src/controllers/ConfigurationController.ts (1)
ConfigurationController(9-419)src/middlewares/permissionMiddleware.ts (1)
requirePermission(18-64)src/middlewares/validationErrorHandler.ts (1)
handleValidationErrors(5-42)
src/controllers/ConfigurationController.ts (1)
src/services/ConfigurationService.ts (1)
configurationService(637-637)
src/services/ConfigurationService.ts (1)
src/services/AuditService.ts (1)
AuditService(34-170)
🪛 markdownlint-cli2 (0.17.2)
PR_DESCRIPTION.md
11-11: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
20-20: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
28-28: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
41-41: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
59-59: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
73-73: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
78-78: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
88-88: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
102-102: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
109-109: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
115-115: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
120-120: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (5)
src/tests/integration/configurationIntegration.test.ts (2)
350-397: Great job implementing the previously requested test scenarios!The additional test cases for updating configurations, unauthorized access (403), and type validation (400) properly address the testing gaps identified in the previous review. The implementation correctly validates the expected status codes and response behaviors.
399-450: Excellent implementation of the additional feature flag test scenarios!The tests for percentage-based rollouts and role-based targeting comprehensively cover the previously missing scenarios. The percentage distribution validation (30-70% range for 50% rollout) and role-based access checks are well-implemented.
src/types/express.d.ts (1)
26-39: Well-designed TypeScript interface extension for configuration access.The addition of
config,environment, andappConfigproperties to the Express Request interface is clean and follows best practices. The async methods with proper typing and optional parameters provide a flexible API for configuration access throughout the application.docs/CONFIGURATION_SYSTEM.md (1)
55-75: Configuration schema looks good!The SQL schema properly uses
config_keyinstead of reserved keywords, includes appropriate indexes with the unique constraint, and covers all necessary fields for a robust configuration management system.src/routes/configurationRoutes.ts (1)
1-756: Well-structured configuration routes implementation.The routes module is comprehensively implemented with proper authentication, authorization, validation, and documentation. The code follows consistent patterns and includes all necessary security measures.
Consider these optional enhancements for future iterations:
- Add rate limiting middleware to mutation endpoints (POST, DELETE) to prevent abuse
- Set request size limits for the import endpoint to prevent memory exhaustion
- Consider dependency injection for the ConfigurationController to improve testability
- Improve encryption security with proper salt derivation using SHA-256 - Fix inconsistent percentage rollout calculations (standardize to 0-99 range) - Add getConfigDetails method for secure configuration retrieval - Mask encrypted values in single configuration endpoint for consistency - Ensure all percentage calculations use same range and logic
respp
left a comment
respp
left a comment
There was a problem hiding this comment.
Excellent work @akintewe , you’ve implemented a very complete and well-structured system. Just a few minor adjustments are needed before it can be merged: standardize the percentage rollout calculation in both methods, and remove the PR_DESCRIPTION.md file from the repository, as it shouldn’t be included in the merge. The rest of the code is well-executed and fully meets the issue requirements. As a final suggestion, I recommend revisiting your commit messages—it's best to use more descriptive names that reflect the actual changes made, since when reviewing the commit history, what matters is not why coderabbit suggested the change, but what was actually modified or fixed.
…escription file - Standardize percentage calculation in evaluateTargetingRules to use 'anonymous' fallback like evaluateFeatureFlag - Ensure consistent variable naming (userPercentage) across both methods - Remove PR_DESCRIPTION.md from repository as requested by maintainer - Both methods now use identical 0-99 range and < comparison logic
Thank you so much for the feedback, i will make the fix. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/services/ConfigurationService.ts (1)
47-53: Security concern: Hardcoded default encryption keyUsing a hardcoded default encryption key is a security risk. The application should fail to start if the encryption key is not properly configured.
- this.encryptionKey = process.env.CONFIG_ENCRYPTION_KEY || "default-encryption-key"; + if (!process.env.CONFIG_ENCRYPTION_KEY) { + throw new Error("CONFIG_ENCRYPTION_KEY environment variable is required"); + } + this.encryptionKey = process.env.CONFIG_ENCRYPTION_KEY;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/ConfigurationService.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/services/ConfigurationService.ts (1)
src/services/AuditService.ts (1)
AuditService(34-170)
🔇 Additional comments (2)
src/services/ConfigurationService.ts (2)
579-588: Good implementation of secure key derivationThe encryption implementation properly uses a deterministic salt with key derivation, addressing the previous security concern.
565-566: Incorrect handling of ENCRYPTED configuration typeThe ENCRYPTED type case is calling
decryptValue, but this seems incorrect. The type indicates the value should remain encrypted, not be decrypted during parsing.case ConfigurationType.ENCRYPTED: - return this.decryptValue(value); + // ENCRYPTED type should return the encrypted value as-is + return value;Likely an incorrect or invalid review comment.
…n service - Add safeJsonParse method with error handling to prevent crashes from malformed JSON - Replace all JSON.parse calls with safeJsonParse for validationRules, allowedValues, and metadata - Fix hash calculation inconsistency in evaluateTargetingRules to use 'targeting:userId' prefix - Ensure consistent percentage rollout behavior across all evaluation methods - Add proper error logging for JSON parsing failures with fallback defaults
- Remove unused import statements in controllers, tests, and type definitions - Replace 'any' types with proper TypeScript types for better type safety - Fix Repository import missing in ConfigurationService.test.ts - Remove namespace declaration from configurationMiddleware (moved to express.d.ts) - Ensure all tests use proper typing with Partial<Repository<T>> instead of any - Maintain backward compatibility while improving code quality
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/tests/integration/configurationIntegration.test.ts (1)
20-23: Generate proper authentication tokens for tests instead of using hardcoded values.The hardcoded auth token
"test-auth-token"bypasses actual authentication testing. Consider generating real JWT tokens or mocking the auth middleware properly to ensure authentication is correctly tested.src/services/ConfigurationService.ts (1)
46-52: Security concern: Hardcoded default encryption keyUsing a hardcoded default encryption key is a security risk. The application should fail to start if the encryption key is not properly configured.
- this.encryptionKey = process.env.CONFIG_ENCRYPTION_KEY || "default-encryption-key"; + if (!process.env.CONFIG_ENCRYPTION_KEY) { + throw new Error("CONFIG_ENCRYPTION_KEY environment variable is required"); + } + this.encryptionKey = process.env.CONFIG_ENCRYPTION_KEY;
🧹 Nitpick comments (2)
src/tests/integration/configurationIntegration.test.ts (1)
157-213: Add test coverage for merchant-based targeting and scheduled feature flags.While the new tests for percentage rollouts and role-based targeting are good additions, the test suite is still missing coverage for:
- Merchant-based targeting
- Scheduled feature flags with start/end dates
Add these test cases to ensure complete coverage:
+ it("should handle merchant-based targeting", async () => { + await request(app) + .post("/api/config/feature-flags") + .set("Authorization", `Bearer ${authToken}`) + .send({ + name: "merchant_feature", + description: "Merchant-based feature", + isEnabled: true, + scope: "merchant", + targetingRules: { + merchantIds: ["merchant1", "merchant2"], + }, + }); + + const matchResponse = await request(app) + .get("/api/config/feature-flags/merchant_feature/evaluate?merchantId=merchant1") + .set("Authorization", `Bearer ${authToken}`); + expect(matchResponse.body.data.isEnabled).toBe(true); + + const noMatchResponse = await request(app) + .get("/api/config/feature-flags/merchant_feature/evaluate?merchantId=merchant3") + .set("Authorization", `Bearer ${authToken}`); + expect(noMatchResponse.body.data.isEnabled).toBe(false); + }); + + it("should handle scheduled feature flags", async () => { + const now = new Date(); + const futureDate = new Date(now.getTime() + 24 * 60 * 60 * 1000); // Tomorrow + const pastDate = new Date(now.getTime() - 24 * 60 * 60 * 1000); // Yesterday + + await request(app) + .post("/api/config/feature-flags") + .set("Authorization", `Bearer ${authToken}`) + .send({ + name: "scheduled_feature", + description: "Scheduled feature", + isEnabled: true, + scope: "global", + scheduledStartDate: futureDate.toISOString(), + }); + + const notStartedResponse = await request(app) + .get("/api/config/feature-flags/scheduled_feature/evaluate") + .set("Authorization", `Bearer ${authToken}`); + expect(notStartedResponse.body.data.isEnabled).toBe(false); + expect(notStartedResponse.body.data.reason).toContain("not yet scheduled"); + + // Test expired flag + await request(app) + .post("/api/config/feature-flags") + .set("Authorization", `Bearer ${authToken}`) + .send({ + name: "expired_feature", + description: "Expired feature", + isEnabled: true, + scope: "global", + scheduledEndDate: pastDate.toISOString(), + }); + + const expiredResponse = await request(app) + .get("/api/config/feature-flags/expired_feature/evaluate") + .set("Authorization", `Bearer ${authToken}`); + expect(expiredResponse.body.data.isEnabled).toBe(false); + expect(expiredResponse.body.data.reason).toContain("expired"); + });Also applies to: 399-450
src/controllers/ConfigurationController.ts (1)
403-406: Avoid logging full error objects that may contain sensitive information.The error logging at line 404 logs the entire error object, which might contain sensitive configuration values or system information.
Log only the error message instead of the full error object:
- logger.error(`Failed to import configuration ${config.key}:`, error); + logger.error(`Failed to import configuration ${config.key}: ${error instanceof Error ? error.message : 'Unknown error'}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/controllers/ConfigurationController.ts(1 hunks)src/middlewares/configurationMiddleware.ts(1 hunks)src/services/ConfigurationService.ts(1 hunks)src/tests/integration/configurationIntegration.test.ts(1 hunks)src/tests/services/ConfigurationService.test.ts(1 hunks)src/types/express.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tests/services/ConfigurationService.test.ts
- src/types/express.d.ts
- src/middlewares/configurationMiddleware.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/tests/integration/configurationIntegration.test.ts (2)
src/services/ConfigurationService.ts (1)
configurationService(665-665)src/lib/services/referral-service.ts (1)
request(25-68)
src/controllers/ConfigurationController.ts (1)
src/services/ConfigurationService.ts (1)
configurationService(665-665)
- Add error handling to JSON parsing in parseValue method for ConfigurationType.JSON - Fix hash calculation consistency by passing flagName to evaluateTargetingRules - Ensure percentage rollout uses same hash format across all evaluation methods - Add proper TypeScript types for safeJsonParse method with generic constraints - Fix import path for Wallet entity in database configuration - Resolve type assertion issues in AuditService for Express request extensions - Maintain backward compatibility while improving error handling and type safety
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/config/db.ts(2 hunks)src/services/AuditService.ts(2 hunks)src/services/ConfigurationService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/config/db.ts
- src/services/ConfigurationService.ts
🔇 Additional comments (2)
src/services/AuditService.ts (2)
4-5: LGTM! Proper import and type organization.The Express Request import is necessary for the type casting improvements, and the comment correctly indicates that request interface extensions are handled in the dedicated type definition file.
161-167: Excellent type safety improvements.The type casting approach with optional chaining provides robust access to extended request properties while maintaining type safety. Converting the user ID to string is appropriate for the audit context requirements.
- Replace deprecated req.connection.remoteAddress with req.socket.remoteAddress - Add support for proxy headers (x-forwarded-for, x-real-ip) - Implement proper fallback chain for reliable IP detection in various deployment scenarios - Handle comma-separated IPs in x-forwarded-for header by extracting first IP - Maintain backward compatibility while improving production reliability
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/controllers/ConfigurationController.ts (1)
43-69: LGTM: Security issue resolved.The method now properly uses
getConfigDetailsand masks encrypted values, addressing the security concern raised in previous reviews. The implementation is consistent with other methods.src/tests/integration/configurationIntegration.test.ts (1)
20-23: Generate proper authentication tokens for tests instead of using hardcoded values.The hardcoded auth token bypasses actual authentication testing and should be replaced with proper JWT token generation or auth middleware mocking.
src/scripts/initializeConfigurations.ts (3)
42-49: LGTM: Security issue resolved with environment variables.The sensitive configurations now properly load from environment variables, addressing the security concerns from previous reviews. This prevents hardcoded secrets from being exposed in version control.
196-211: Handle required sensitive configurations consistently.SMTP credentials are marked as required but have empty values, which will fail validation. Consider making them optional for initial setup or providing environment-based defaults.
399-465: Don't initialize all environments by default - this is dangerous for production.The script initializes configurations for all environments including production, which could accidentally overwrite production settings. Consider targeting only the intended environment.
🧹 Nitpick comments (2)
src/controllers/ConfigurationController.ts (2)
74-130: Consider using appropriate HTTP status codes for create vs update operations.The method always returns 201 (Created) even when updating existing configurations. Consider differentiating between creates and updates for better REST semantics.
- res.status(201).json({ + const existingConfig = await configurationService.getConfig(key); + const statusCode = existingConfig === null ? 201 : 200; + + res.status(statusCode).json({ success: true, - message: "Configuration updated successfully", + message: existingConfig === null ? "Configuration created successfully" : "Configuration updated successfully", data: { ...config, value: isEncrypted ? "[ENCRYPTED]" : config.value, }, });
273-330: Consider using appropriate HTTP status codes for create vs update operations.Similar to
setConfiguration, this method always returns 201 even for updates. Consider differentiating between creates and updates for better REST semantics.- res.status(201).json({ + const existingFlag = await configurationService.getFeatureFlag(name); + const statusCode = existingFlag ? 200 : 201; + + res.status(statusCode).json({ success: true, - message: "Feature flag updated successfully", + message: existingFlag ? "Feature flag updated successfully" : "Feature flag created successfully", data: flag, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/app.ts(3 hunks)src/controllers/ConfigurationController.ts(1 hunks)src/entities/Configuration.ts(1 hunks)src/entities/FeatureFlag.ts(1 hunks)src/middlewares/configurationMiddleware.ts(1 hunks)src/routes/configurationRoutes.ts(1 hunks)src/scripts/initializeConfigurations.ts(1 hunks)src/scripts/testConfigurationSystem.ts(1 hunks)src/services/AuditService.ts(2 hunks)src/services/ConfigurationService.ts(1 hunks)src/tests/integration/configurationIntegration.test.ts(1 hunks)src/tests/services/ConfigurationService.test.ts(1 hunks)src/types/express.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/app.ts
- src/scripts/testConfigurationSystem.ts
- src/services/AuditService.ts
- src/middlewares/configurationMiddleware.ts
- src/tests/services/ConfigurationService.test.ts
- src/types/express.d.ts
- src/routes/configurationRoutes.ts
- src/entities/Configuration.ts
- src/entities/FeatureFlag.ts
- src/services/ConfigurationService.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/scripts/initializeConfigurations.ts (1)
src/services/ConfigurationService.ts (1)
configurationService(780-780)
src/tests/integration/configurationIntegration.test.ts (1)
src/services/ConfigurationService.ts (1)
configurationService(780-780)
src/controllers/ConfigurationController.ts (1)
src/services/ConfigurationService.ts (1)
configurationService(780-780)
🔇 Additional comments (13)
src/controllers/ConfigurationController.ts (10)
16-38: LGTM: Proper security handling for encrypted configurations.The method correctly masks encrypted values before returning them to the client, maintaining security while providing useful metadata. The response structure is consistent and includes a helpful count field.
135-152: LGTM: Clean delete implementation with audit support.The method properly handles the delete operation and includes user identification for audit trails.
157-191: LGTM: Proper input validation and security handling.The method correctly validates the category parameter against allowed values and maintains consistent security practices by masking encrypted values.
196-211: LGTM: Supports hot-reload functionality.The method provides the hot-reload capability mentioned in the PR objectives, allowing configuration updates without server restarts.
216-245: LGTM: Comprehensive configuration validation.The method properly validates required configurations and provides detailed feedback about missing or empty values, which aligns with the startup validation requirements mentioned in the PR objectives.
252-268: LGTM: Consistent implementation pattern.The method follows the same response structure pattern as configuration methods, maintaining API consistency.
335-360: LGTM: Proper feature flag evaluation with context.The method correctly passes evaluation context (userId, merchantId, userRole) to the service, enabling targeted feature flag functionality as described in the PR objectives.
365-405: LGTM: Comprehensive statistics for monitoring.The method provides valuable insights into the configuration system, supporting the observability goals mentioned in the PR objectives. The aggregation logic is correct and efficient.
410-447: LGTM: Secure configuration export functionality.The method properly masks encrypted values in exports and sets appropriate headers for file download. The export format includes useful metadata like timestamp and environment.
452-512: LGTM: Robust batch import with proper error handling.The method handles batch imports gracefully, continuing processing even when individual configurations fail. The statistics tracking (imported, skipped, errors) provides valuable feedback about the import operation.
src/tests/integration/configurationIntegration.test.ts (3)
38-156: LGTM: Comprehensive basic configuration API testing.The test cases cover essential CRUD operations, security handling for encrypted configurations, category filtering, and deletion. The assertions are appropriate and verify both status codes and response data.
158-214: LGTM: Basic feature flag testing with targeted rules.The tests cover basic feature flag functionality and user-targeted rules. The additional advanced targeting tests at the end of the file (percentage rollouts, role-based targeting) provide comprehensive coverage.
367-470: LGTM: Advanced test scenarios address previous feedback.These test cases successfully address the missing scenarios from previous reviews, including configuration updates, unauthorized access handling, type validation, and advanced feature flag targeting (percentage rollouts, role-based targeting). The test coverage is now comprehensive.
|
@respp please any response on my pr? |
3 participants
Pull Request Overview
📝 Summary
This PR implements a complete Multi-Environment Configuration Management System that centralizes all application configurations, provides dynamic feature flags, and enables secure configuration management across development, staging, and production environments. The system eliminates hardcoded configurations and provides a robust API for real-time configuration updates.
🔗 Related Issues
🔄 Changes Made
Core Features Implemented:
Technical Implementation:
ConfigurationandFeatureFlagentities with comprehensive metadata/api/config)Files Added/Modified:
Configuration.ts,FeatureFlag.tsConfigurationService.tsConfigurationController.tsconfigurationRoutes.tsconfigurationMiddleware.tsinitializeConfigurations.ts,testConfigurationSystem.tsCONFIGURATION_SYSTEM.md🖼️ Current Output
API Endpoints Available:
Configuration Categories Supported:
🧪 Testing
Automated Tests:
ConfigurationService.test.ts- 536 lines of comprehensive test coverageconfigurationIntegration.test.ts- 350 lines testing API endpointstestConfigurationSystem.ts- 233 lines for manual verificationTest Coverage Includes:
Manual Testing Commands:
💬 Comments
Key Benefits:
Migration Notes:
Next Steps:
Security Considerations:
Ready for Review ✅
This implementation provides a complete, production-ready configuration management system that addresses all requirements from issue #117 while maintaining backward compatibility and providing comprehensive testing coverage.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores