feature/risk_rule_logic #2472
feature/risk_rule_logic #2472
Conversation
bal 2254 rule engine v2
* feat: risk-rule-service * fix: pr comments * fix: tests
|
WalkthroughThe changes introduce a new integration with Notion API within the Changes
Sequence DiagramssequenceDiagram
participant App
participant NotionService
participant NotionAPI as Notion API
participant RuleEngineService
participant RuleEngine
App->>NotionService: getAllDatabaseRecordsValues({ databaseId })
NotionService->>NotionAPI: Query database with databaseId
NotionAPI-->>NotionService: Return database records
NotionService-->>App: Return transformed records
App->>RuleEngineService: run(storeOptions, formData)
RuleEngineService->>RuleEngine: Execute rules with formData
RuleEngine-->>RuleEngineService: Rule execution results
RuleEngineService-->>App: Return results
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration 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: 9
Outside diff range and nitpick comments (2)
services/workflows-service/src/rule-engine/core/operators/helpers.ts (1)
188-188: Error handling inLastYear.eval.The error thrown for unsupported data types in
LastYear.evalis a good practice. However, consider adding more specific error messages or handling more types if applicable. This would enhance the robustness and user-friendliness of the rule engine.services/workflows-service/src/rule-engine/core/rule-engine.unit.test.ts (1)
12-194: Comprehensive review of unit tests inrule-engine.unit.test.ts.The unit tests are well-structured and cover a variety of rule evaluation scenarios. However, consider adding more detailed comments explaining the purpose of each test case, especially for complex rule sets. This would improve the maintainability and understandability of the tests.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (19)
- services/workflows-service/.env.example (1 hunks)
- services/workflows-service/package.json (3 hunks)
- services/workflows-service/prisma/data-migrations (1 hunks)
- services/workflows-service/src/app.module.ts (2 hunks)
- services/workflows-service/src/env.ts (1 hunks)
- services/workflows-service/src/notion.service.ts (1 hunks)
- services/workflows-service/src/risk-rule.service.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/errors.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/operators/constants.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/operators/enums.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/operators/helpers.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/operators/schemas.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/operators/types.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/rule-engine.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/rule-engine.unit.test.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/types.ts (1 hunks)
- services/workflows-service/src/rule-engine/rule-engine.module.ts (1 hunks)
- services/workflows-service/src/rule-engine/rule-engine.service.ts (1 hunks)
- services/workflows-service/src/rule-engine/rule-store.service.ts (1 hunks)
Files skipped from review due to trivial changes (4)
- services/workflows-service/prisma/data-migrations
- services/workflows-service/src/rule-engine/core/operators/constants.ts
- services/workflows-service/src/rule-engine/core/operators/enums.ts
- services/workflows-service/src/rule-engine/rule-engine.module.ts
Additional context used
Gitleaks
services/workflows-service/.env.example
36-37: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Biome
services/workflows-service/src/rule-engine/core/rule-engine.ts
[error] 54-56: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Additional comments not posted (18)
services/workflows-service/src/rule-engine/core/operators/types.ts (4)
1-1: Type definition for primitive values.The definition of
primitiveasnumber | string | booleanis accurate and covers the typical primitive data types used in condition checks. This is essential for ensuring that the rule engine can handle the most common types of data it will encounter.
3-6: Type definition for BetweenParams.The
BetweenParamstype is correctly defined withminandmaxas numbers. This is appropriate for conditions that check if a value falls between two numbers. The definition is clear and correctly restricts the parameters to numeric types, which is suitable for most between operations.
[APROVED]
8-10: Type definition for LastYearsParams.The
LastYearsParamstype is well-defined with a singleyearsproperty of type number. This type is likely used for operations that need to calculate or check values relative to a number of years, which is a common scenario in date-related operations.
[APROVED]
12-16: Definition of ConditionFn and IConditionHelpers.The
ConditionFngeneric type is well-designed, allowing for a function that takes a primitive and a parameter of type T, and returns a boolean. This is a flexible setup that can accommodate various types of conditions. TheIConditionHelpersinterface maps strings to these condition functions, providing a structured way to access different condition checks by name. This setup promotes modularity and reusability in the rule engine.
[APROVED]services/workflows-service/src/rule-engine/rule-store.service.ts (1)
1-7: Imports and type definitions.The
Injectabledecorator from NestJS is correctly used, and theRuleSettype is imported from the appropriate location. TheRuleStoreServiceFindAllOptionstype is appropriately defined withdatabaseIdandsource, which are necessary for fetching rules from a specific source like Notion.services/workflows-service/src/rule-engine/core/operators/schemas.ts (2)
1-3: Imports and constant definitions.The import of the Zod library (
z) and theOPERATIONenum fromenums.tsare correct and necessary for defining the schemas. This setup correctly establishes the dependencies needed for the schema definitions.
4-9: Definition of PrimitiveSchema and BetweenSchema.The
PrimitiveSchemais correctly defined as a union of number, string, and boolean, which are the basic data types handled by the rule engine. TheBetweenSchemais well-defined withminandmaxasPrimitiveSchema, ensuring that the values for these properties adhere to the defined primitive types. This is crucial for validating 'between' operations accurately.
[APROVED]services/workflows-service/src/rule-engine/rule-engine.service.ts (2)
1-5: Imports and service decoration.The use of the
Injectabledecorator is correct, ensuring that the service can be managed by NestJS's dependency injection system. The imports are appropriate, bringing in necessary classes and helpers for the service's functionality.
6-18: Implementation of RuleEngineService and run method.The
runmethod is well-implemented, using theRuleStoreServiceto fetch rules and theRuleEngineto execute these rules against provided formData. This method effectively ties together the rule fetching and execution logic, providing a clean and functional interface for running rule sets.services/workflows-service/src/rule-engine/core/types.ts (1)
23-23: Clarify the optionalrulesfield inPassedRuleResult.The comment suggests that the
rulesfield should always be present, which conflicts with its optional declaration (rules?). Please confirm if this field is optional or required, and adjust the type accordingly.services/workflows-service/src/risk-rule.service.ts (1)
19-22: Validate recursive schema definition.Ensure that the recursive definition in
RuleSetSchemais correctly implemented and will not cause runtime issues due to infinite recursion.services/workflows-service/src/notion.service.ts (2)
14-18: Secure API key usage.Ensure that the API key used in the Notion client is securely handled and not logged or exposed in any logs or error messages.
[SECURITY]
34-52: Optimize database content extraction logic.Consider optimizing the loop for querying database pages to handle large datasets more efficiently and prevent potential performance issues.
[PERFORMANCE]- do { - database = await this.client.databases.query({ - database_id: databaseId, - ...(database?.next_cursor && { start_cursor: database.next_cursor }), - }); - records.push(...database.results); - } while (database.next_cursor); + // Proposed refactor to use a more efficient querying strategyservices/workflows-service/src/env.ts (1)
78-78: Validate new environment variable addition.Ensure that the addition of
NOTION_API_KEYto the environment schema is correctly implemented and documented.services/workflows-service/src/app.module.ts (1)
48-48: Confirm integration of new module.Ensure that the integration of
RuleEngineModuleis properly configured and does not introduce any circular dependencies or configuration issues.Verification successful
Confirm integration of new module.
The
RuleEngineModuleis properly integrated inapp.module.tswithout introducing any circular dependencies or configuration issues.
services/workflows-service/src/app.module.ts:RuleEngineModuleis imported and added to theimportsarray.services/workflows-service/src/rule-engine/rule-engine.module.ts:RuleEngineModuleprovides and exportsRuleEngineService.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify module integration and check for circular dependencies. rg --type ts 'RuleEngineModule' -C 10Length of output: 4295
services/workflows-service/package.json (2)
64-64: Added dependency@notionhq/client.The addition of the Notion client is in alignment with the PR's objective to integrate Notion API functionality. Ensure that the usage of this client is covered by appropriate unit tests and that its integration is secure and efficient.
77-77: Re-added dependencyballerine-nestjs-typebox.The re-addition of this dependency should be justified in the PR description or documentation, especially if it was previously removed and then added back. This action might indicate dependency management issues or version conflicts that were resolved.
Verification successful
Re-added dependency
ballerine-nestjs-typebox.The commit history shows multiple entries related to
ballerine-nestjs-typebox, including fixes and integrations. It appears that the re-addition was necessary to resolve issues and improve the workflows service.
- Commit
e34070518522975e92f80846b08edf2cbc266e5e: Updatedballerine-nestjs-typebox.- Commit
b5ad350be4ba5c35811e13c522ed9ed4e0a83dd3: Integratedballerine-nestjs-typeboxwith multiple changes and fixes.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search commit history for removal and re-addition reasons. # Test: Search for commits related to 'ballerine-nestjs-typebox'. Expect: Commit messages explaining the reasons. git log --grep='ballerine-nestjs-typebox'Length of output: 1057
services/workflows-service/src/rule-engine/core/operators/helpers.ts (1)
55-67: Review of specific operator implementations.Each operator class (
Equals,GreaterThan,LessThan, etc.) correctly implements theevalmethod with appropriate logic. However, ensure that these implementations are thoroughly tested, especially edge cases like type mismatches and boundary values.Also applies to: 69-81, 83-95, 97-117, 119-139, 142-165, 167-189
| @@ -34,3 +34,4 @@ COLLECTION_FLOW_URL=http://localhost:5201 | |||
| WEB_UI_SDK_URL=http://localhost:5202 | |||
| #HASHING_KEY_SECRET="$2b$10$FovZTB91/QQ4Yu28nvL8e." | |||
| HASHING_KEY_SECRET_BASE64=JDJiJDEwJDNFeWtwWEs4QkdiczlRaWFwLkM4Vk8= | |||
| NOTION_API_KEY=secret | |||
There was a problem hiding this comment.
Potential security risk with the hardcoded API key.
The addition of NOTION_API_KEY with a hardcoded value in the .env.example file poses a security risk. It's essential to ensure that sensitive keys are not exposed in version control. Consider using a placeholder or a more secure method of handling sensitive keys.
- NOTION_API_KEY=secret
+ NOTION_API_KEY=<YOUR_NOTION_API_KEY_HERE>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.
| NOTION_API_KEY=secret | |
| NOTION_API_KEY=<YOUR_NOTION_API_KEY_HERE> |
Tools
Gitleaks
36-37: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
| export class ValidationFailedError extends Error { | ||
| errors: | ||
| | Array<{ | ||
| message: string; | ||
| path: string; | ||
| }> | ||
| | undefined; | ||
|
|
||
| constructor(key: string, operation: string, error?: ZodError) { | ||
| super(`Validation failed for '${key}', message: ${operation}`); | ||
| this.name = 'ValidationFailedError'; | ||
|
|
||
| this.errors = error?.errors.map(zodIssue => ({ | ||
| message: zodIssue.message, | ||
| path: zodIssue.path.join('.'), | ||
| })); | ||
| } |
There was a problem hiding this comment.
Improve error handling in ValidationFailedError.
The ValidationFailedError class could be improved by ensuring that the errors field is always an array, even when there are no errors to report. This can avoid potential issues in error handling where the consumer expects an array but receives undefined.
- | undefined;
+ = [];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.
| export class ValidationFailedError extends Error { | |
| errors: | |
| | Array<{ | |
| message: string; | |
| path: string; | |
| }> | |
| | undefined; | |
| constructor(key: string, operation: string, error?: ZodError) { | |
| super(`Validation failed for '${key}', message: ${operation}`); | |
| this.name = 'ValidationFailedError'; | |
| this.errors = error?.errors.map(zodIssue => ({ | |
| message: zodIssue.message, | |
| path: zodIssue.path.join('.'), | |
| })); | |
| } | |
| export class ValidationFailedError extends Error { | |
| errors: | |
| | Array<{ | |
| message: string; | |
| path: string; | |
| }> | |
| = []; | |
| constructor(key: string, operation: string, error?: ZodError) { | |
| super(`Validation failed for '${key}', message: ${operation}`); | |
| this.name = 'ValidationFailedError'; | |
| this.errors = error?.errors.map(zodIssue => ({ | |
| message: zodIssue.message, | |
| path: zodIssue.path.join('.'), | |
| })); | |
| } |
| export abstract class BaseOperator<T = primitive> { | ||
| operator: string; | ||
| conditionValueSchema?: ZodSchema<any>; | ||
| dataValueSchema?: ZodSchema<any>; | ||
|
|
||
| constructor(options: { | ||
| operator: TOperation; | ||
| conditionValueSchema?: ZodSchema<any>; | ||
| dataValueSchema?: ZodSchema<any>; | ||
| }) { | ||
| const { operator, conditionValueSchema, dataValueSchema } = options; | ||
|
|
||
| this.operator = operator; | ||
| this.conditionValueSchema = conditionValueSchema; | ||
| this.dataValueSchema = dataValueSchema; | ||
| } | ||
|
|
||
| abstract eval(dataValue: primitive, conditionValue: T): boolean; | ||
|
|
||
| execute(dataValue: primitive, conditionValue: T): boolean { | ||
| this.validate({ dataValue, conditionValue }); | ||
|
|
||
| return this.eval(dataValue, conditionValue); | ||
| } | ||
|
|
||
| protected validate(args: { dataValue: unknown; conditionValue: unknown }) { | ||
| if (this.conditionValueSchema) { | ||
| this.validateSchema( | ||
| this.conditionValueSchema, | ||
| args.conditionValue, | ||
| `Invalid condition value`, | ||
| ); | ||
| } | ||
|
|
||
| if (this.dataValueSchema) { | ||
| this.validateSchema(this.dataValueSchema, args.dataValue, `Invalid data value`); | ||
| } | ||
| } | ||
|
|
||
| protected validateSchema(schema: ZodSchema<any>, value: unknown, message: string) { | ||
| const result = schema.safeParse(value); | ||
|
|
||
| if (!result.success) { | ||
| throw new ValidationFailedError(this.operator, message, result.error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor suggestion for BaseOperator class.
The BaseOperator class is well-structured with clear separation of concerns. However, consider using generics more effectively to enforce type safety across different operator implementations. This would reduce the risk of runtime type errors and improve maintainability.
- export abstract class BaseOperator<T = primitive> {
+ export abstract class BaseOperator<T extends primitive> {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.
| export abstract class BaseOperator<T = primitive> { | |
| operator: string; | |
| conditionValueSchema?: ZodSchema<any>; | |
| dataValueSchema?: ZodSchema<any>; | |
| constructor(options: { | |
| operator: TOperation; | |
| conditionValueSchema?: ZodSchema<any>; | |
| dataValueSchema?: ZodSchema<any>; | |
| }) { | |
| const { operator, conditionValueSchema, dataValueSchema } = options; | |
| this.operator = operator; | |
| this.conditionValueSchema = conditionValueSchema; | |
| this.dataValueSchema = dataValueSchema; | |
| } | |
| abstract eval(dataValue: primitive, conditionValue: T): boolean; | |
| execute(dataValue: primitive, conditionValue: T): boolean { | |
| this.validate({ dataValue, conditionValue }); | |
| return this.eval(dataValue, conditionValue); | |
| } | |
| protected validate(args: { dataValue: unknown; conditionValue: unknown }) { | |
| if (this.conditionValueSchema) { | |
| this.validateSchema( | |
| this.conditionValueSchema, | |
| args.conditionValue, | |
| `Invalid condition value`, | |
| ); | |
| } | |
| if (this.dataValueSchema) { | |
| this.validateSchema(this.dataValueSchema, args.dataValue, `Invalid data value`); | |
| } | |
| } | |
| protected validateSchema(schema: ZodSchema<any>, value: unknown, message: string) { | |
| const result = schema.safeParse(value); | |
| if (!result.success) { | |
| throw new ValidationFailedError(this.operator, message, result.error); | |
| } | |
| } | |
| export abstract class BaseOperator<T extends primitive> { | |
| operator: string; | |
| conditionValueSchema?: ZodSchema<any>; | |
| dataValueSchema?: ZodSchema<any>; | |
| constructor(options: { | |
| operator: TOperation; | |
| conditionValueSchema?: ZodSchema<any>; | |
| dataValueSchema?: ZodSchema<any>; | |
| }) { | |
| const { operator, conditionValueSchema, dataValueSchema } = options; | |
| this.operator = operator; | |
| this.conditionValueSchema = conditionValueSchema; | |
| this.dataValueSchema = dataValueSchema; | |
| } | |
| abstract eval(dataValue: primitive, conditionValue: T): boolean; | |
| execute(dataValue: primitive, conditionValue: T): boolean { | |
| this.validate({ dataValue, conditionValue }); | |
| return this.eval(dataValue, conditionValue); | |
| } | |
| protected validate(args: { dataValue: unknown; conditionValue: unknown }) { | |
| if (this.conditionValueSchema) { | |
| this.validateSchema( | |
| this.conditionValueSchema, | |
| args.conditionValue, | |
| `Invalid condition value`, | |
| ); | |
| } | |
| if (this.dataValueSchema) { | |
| this.validateSchema(this.dataValueSchema, args.dataValue, `Invalid data value`); | |
| } | |
| } | |
| protected validateSchema(schema: ZodSchema<any>, value: unknown, message: string) { | |
| const result = schema.safeParse(value); | |
| if (!result.success) { | |
| throw new ValidationFailedError(this.operator, message, result.error); | |
| } | |
| } |
| public async findAll({ databaseId, source }: { databaseId: string; source: 'notion' }) { | ||
| if (source === 'notion') { | ||
| const records = await this.notionService.getAllDatabaseRecordsValues({ | ||
| databaseId, | ||
| }); | ||
|
|
||
| const validatedRecords = []; | ||
|
|
||
| for (const record of records) { | ||
| const validatedRecord = NotionRiskRuleRecordSchema.safeParse(record); | ||
|
|
||
| if (!validatedRecord.success) { | ||
| this.logger.error('Notion risk rule record schema validation failed', { | ||
| databaseId, | ||
| record, | ||
| error: validatedRecord.error, | ||
| }); | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| validatedRecords.push(validatedRecord.data); | ||
| } | ||
|
|
||
| return validatedRecords; | ||
| } | ||
|
|
||
| throw new Error('Unsupported source'); | ||
| } |
There was a problem hiding this comment.
Error handling and logging enhancement.
When handling errors from external services, consider adding retry logic or more detailed error handling to improve resilience and diagnostic capabilities.
- throw new Error('Unsupported source');
+ throw new Error(`Unsupported source: ${source}. Please check the configuration.`);Committable suggestion was skipped due to low confidence.
| import { Injectable } from '@nestjs/common'; | ||
| import { NotionService } from '@/notion.service'; | ||
| import z from 'zod'; | ||
| import { AppLoggerService } from '@/common/app-logger/app-logger.service'; |
There was a problem hiding this comment.
Organize imports for better readability.
Consider grouping imports from the same library together to enhance readability and maintainability.
- import { Injectable } from '@nestjs/common';
- import { NotionService } from '@/notion.service';
- import z from 'zod';
- import { AppLoggerService } from '@/common/app-logger/app-logger.service';
+ import { Injectable } from '@nestjs/common';
+ import z from 'zod';
+ import { NotionService, AppLoggerService } from '@/common';Committable suggestion was skipped due to low confidence.
| @Injectable() | ||
| export class RuleStoreService { | ||
| public async findAll({ databaseId, source }: RuleStoreServiceFindAllOptions): Promise<RuleSet> { | ||
| throw new Error('Unsupported source'); | ||
| } |
There was a problem hiding this comment.
Implementation of RuleStoreService and findAll method.
The findAll method currently throws an 'Unsupported source' error, which suggests that the implementation is not yet complete or is a placeholder. This needs to be addressed to ensure the service can actually retrieve data as expected.
Please implement the findAll method or handle different sources appropriately.
| export const LastYearsSchema = z.object({ | ||
| years: z.number().positive(), | ||
| }); | ||
|
|
||
| // TODO: TBD | ||
| const ruleSchema = z.object({ | ||
| key: z.string(), | ||
| operation: z.literal(OPERATION.LAST_YEAR), | ||
| value: z.object({ | ||
| years: z.number(), | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Definition of LastYearsSchema and ruleSchema.
The LastYearsSchema is appropriately defined with a positive number for years, which makes sense for operations dealing with year calculations. The ruleSchema includes a TODO comment, suggesting it is not finalized. This schema is crucial for validating rule data, and it should be completed and thoroughly tested to ensure robustness.
Please address the TODO in ruleSchema and ensure it is fully implemented and tested.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- services/workflows-service/src/rule-engine/core/rule-engine.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/test/data-helper.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/types.ts (1 hunks)
Additional context used
Biome
services/workflows-service/src/rule-engine/core/rule-engine.ts
[error] 53-55: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Additional comments not posted (2)
services/workflows-service/src/rule-engine/core/test/data-helper.ts (1)
1-64: Well-structured test data forcontext.The
contextobject is well-defined and includes comprehensive details necessary for testing various scenarios in the rule engine. This should provide a robust basis for unit tests.services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (1)
1-246: Comprehensive unit tests for the rule engine.The tests in
rule-engine.unit.test.tsare well-structured and cover a broad range of scenarios, which is crucial for ensuring the reliability of the rule engine. Good job on covering edge cases and using mock data effectively.
| export type Rule = { | ||
| key: string; | ||
| operation: TOperation; | ||
| value: any; // This should be a union of all possible values for the operation |
There was a problem hiding this comment.
Specify more precise types for the value field in the Rule type.
Currently, the value field in the Rule type is annotated with any, which is too broad and can lead to potential bugs. Considering the operations you support, it would be beneficial to define a more specific union type that includes all possible values for the operation.
// Suggested change:
value: string | number | boolean | BetweenParams | LastYearsParams;| } else { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Remove unnecessary else clause.
The else clause in the runRuleSet function is unnecessary because the previous catch block always returns early. Removing it can simplify the control flow and improve code readability.
- } else {
- throw error;
- }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.
| } else { | |
| throw error; | |
| } |
Tools
Biome
[error] 53-55: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- services/workflows-service/src/rule-engine/core/operators/constants.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/operators/enums.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/operators/helpers.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/operators/types.ts (1 hunks)
- services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- services/workflows-service/src/rule-engine/core/operators/enums.ts
Additional comments not posted (24)
services/workflows-service/src/rule-engine/core/operators/types.ts (6)
3-3: Consider renamingprimitivetoPrimitive.In TypeScript, types are usually capitalized to distinguish them from variables.
- export type primitive = number | string | boolean; + export type Primitive = number | string | boolean;
5-8: Consider renamingBetweenParamstoBetweenParamsType.This will help to clearly indicate that it is a type definition.
- export type BetweenParams = { + export type BetweenParamsType = {
10-12: Consider renamingLastYearsParamstoLastYearsParamsType.This will help to clearly indicate that it is a type definition.
- export type LastYearsParams = { + export type LastYearsParamsType = {
14-16: Consider renamingExistsParamstoExistsParamsType.This will help to clearly indicate that it is a type definition.
- export type ExistsParams = { + export type ExistsParamsType = {
18-18: Consider renamingConditionFntoConditionFunction.This will help to clearly indicate that it is a function type.
- export type ConditionFn<T = primitive> = (value: primitive, param: T) => boolean; + export type ConditionFunction<T = primitive> = (value: primitive, param: T) => boolean;
20-22: Consider renamingIConditionHelperstoConditionHelpers.The
Iprefix is not necessary and does not add value.- export interface IConditionHelpers { + export interface ConditionHelpers {services/workflows-service/src/rule-engine/core/operators/constants.ts (1)
1-1: Consider removingEXISTSfrom the import statement.The
EXISTSconstant is not used in this file.- import { EQUALS, BETWEEN, LT, LTE, GT, GTE, LAST_YEAR, EXISTS } from './helpers'; + import { EQUALS, BETWEEN, LT, LTE, GT, GTE, LAST_YEAR } from './helpers';services/workflows-service/src/rule-engine/core/operators/helpers.ts (8)
56-68: No issues found in theEqualsclass.The class is well-structured and implements the
evalmethod correctly.
70-82: No issues found in theGreaterThanclass.The class is well-structured and implements the
evalmethod correctly.
84-96: No issues found in theLessThanclass.The class is well-structured and implements the
evalmethod correctly.
98-118: No issues found in theGreaterThanOrEqualclass.The class is well-structured and implements the
evalmethod correctly.
120-141: No issues found in theLessThanOrEqualclass.The class is well-structured and implements the
evalmethod correctly.
143-165: No issues found in theBetweenclass.The class is well-structured and implements the
evalmethod correctly.
168-191: No issues found in theLastYearclass.The class is well-structured and implements the
evalmethod correctly.
193-211: No issues found in theExistsclass.The class is well-structured and implements the
evalmethod correctly.services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (9)
19-64: No issues found in theshould validate a simple rule settest case.The test case is well-structured and verifies the validation of a simple rule set correctly.
66-84: No issues found in theshould handle missing key in ruletest case.The test case is well-structured and verifies the handling of a missing key in the rule correctly.
86-112: No issues found in theshould throw an error for unknown operationtest case.The test case is well-structured and verifies the handling of an unknown operation correctly.
114-129: No issues found in theshould fail for incorrect valuetest case.The test case is well-structured and verifies the handling of an incorrect value correctly.
131-146: No issues found in theshould validate custom operation with additional paramstest case.The test case is well-structured and verifies the validation of a custom operation with additional params correctly.
148-165: No issues found in theshould fail custom operation with missing additional paramstest case.The test case is well-structured and verifies the handling of a custom operation with missing additional params correctly.
167-192: No issues found in theshould throw MissingKeyError when rule is missing key fieldtest case.The test case is well-structured and verifies the handling of a missing key field in the rule correctly.
194-246: No issues found in theshould resolve a nested property from contexttest case.The test case is well-structured and verifies the resolution of a nested property from the context correctly.
247-378: No issues found in theexists operatortest suite.The test suite is well-structured and verifies the functionality of the
existsoperator correctly.
| export const operationHelpers = { | ||
| [OPERATION.EQUALS]: EQUALS, | ||
| [OPERATION.BETWEEN]: BETWEEN, | ||
| [OPERATION.GT]: GT, | ||
| [OPERATION.GTE]: GTE, | ||
| [OPERATION.LT]: LT, | ||
| [OPERATION.LTE]: LTE, | ||
| [OPERATION.LAST_YEAR]: LAST_YEAR, | ||
| [OPERATION.EXISTS]: EXISTS, | ||
| } as const; |
There was a problem hiding this comment.
Consider removing EXISTS from the operationHelpers constant.
The EXISTS operation is not defined in the OPERATION enum.
- [OPERATION.EXISTS]: EXISTS,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.
| export const operationHelpers = { | |
| [OPERATION.EQUALS]: EQUALS, | |
| [OPERATION.BETWEEN]: BETWEEN, | |
| [OPERATION.GT]: GT, | |
| [OPERATION.GTE]: GTE, | |
| [OPERATION.LT]: LT, | |
| [OPERATION.LTE]: LTE, | |
| [OPERATION.LAST_YEAR]: LAST_YEAR, | |
| [OPERATION.EXISTS]: EXISTS, | |
| } as const; | |
| export const operationHelpers = { | |
| [OPERATION.EQUALS]: EQUALS, | |
| [OPERATION.BETWEEN]: BETWEEN, | |
| [OPERATION.GT]: GT, | |
| [OPERATION.GTE]: GTE, | |
| [OPERATION.LT]: LT, | |
| [OPERATION.LTE]: LTE, | |
| [OPERATION.LAST_YEAR]: LAST_YEAR, | |
| } as const; |
| export abstract class BaseOperator<T = primitive> { | ||
| operator: string; | ||
| conditionValueSchema?: ZodSchema<any>; | ||
| dataValueSchema?: ZodSchema<any>; | ||
|
|
||
| constructor(options: { | ||
| operator: TOperation; | ||
| conditionValueSchema?: ZodSchema<any>; | ||
| dataValueSchema?: ZodSchema<any>; | ||
| }) { | ||
| const { operator, conditionValueSchema, dataValueSchema } = options; | ||
|
|
||
| this.operator = operator; | ||
| this.conditionValueSchema = conditionValueSchema; | ||
| this.dataValueSchema = dataValueSchema; | ||
| } | ||
|
|
||
| abstract eval(dataValue: primitive, conditionValue: T): boolean; | ||
|
|
||
| execute(dataValue: primitive, conditionValue: T): boolean { | ||
| this.validate({ dataValue, conditionValue }); | ||
|
|
||
| return this.eval(dataValue, conditionValue); | ||
| } | ||
|
|
||
| protected validate(args: { dataValue: unknown; conditionValue: unknown }) { | ||
| if (this.conditionValueSchema) { | ||
| this.validateSchema( | ||
| this.conditionValueSchema, | ||
| args.conditionValue, | ||
| `Invalid condition value`, | ||
| ); | ||
| } | ||
|
|
||
| if (this.dataValueSchema) { | ||
| this.validateSchema(this.dataValueSchema, args.dataValue, `Invalid data value`); | ||
| } | ||
| } | ||
|
|
||
| protected validateSchema(schema: ZodSchema<any>, value: unknown, message: string) { | ||
| const result = schema.safeParse(value); | ||
|
|
||
| if (!result.success) { | ||
| throw new ValidationFailedError(this.operator, message, result.error); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor suggestion for BaseOperator class.
Consider using generics more effectively to enforce type safety across different operator implementations.
- export abstract class BaseOperator<T = primitive> {
+ export abstract class BaseOperator<T extends primitive> {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.
| export abstract class BaseOperator<T = primitive> { | |
| operator: string; | |
| conditionValueSchema?: ZodSchema<any>; | |
| dataValueSchema?: ZodSchema<any>; | |
| constructor(options: { | |
| operator: TOperation; | |
| conditionValueSchema?: ZodSchema<any>; | |
| dataValueSchema?: ZodSchema<any>; | |
| }) { | |
| const { operator, conditionValueSchema, dataValueSchema } = options; | |
| this.operator = operator; | |
| this.conditionValueSchema = conditionValueSchema; | |
| this.dataValueSchema = dataValueSchema; | |
| } | |
| abstract eval(dataValue: primitive, conditionValue: T): boolean; | |
| execute(dataValue: primitive, conditionValue: T): boolean { | |
| this.validate({ dataValue, conditionValue }); | |
| return this.eval(dataValue, conditionValue); | |
| } | |
| protected validate(args: { dataValue: unknown; conditionValue: unknown }) { | |
| if (this.conditionValueSchema) { | |
| this.validateSchema( | |
| this.conditionValueSchema, | |
| args.conditionValue, | |
| `Invalid condition value`, | |
| ); | |
| } | |
| if (this.dataValueSchema) { | |
| this.validateSchema(this.dataValueSchema, args.dataValue, `Invalid data value`); | |
| } | |
| } | |
| protected validateSchema(schema: ZodSchema<any>, value: unknown, message: string) { | |
| const result = schema.safeParse(value); | |
| if (!result.success) { | |
| throw new ValidationFailedError(this.operator, message, result.error); | |
| } | |
| } | |
| } | |
| export abstract class BaseOperator<T extends primitive> { | |
| operator: string; | |
| conditionValueSchema?: ZodSchema<any>; | |
| dataValueSchema?: ZodSchema<any>; | |
| constructor(options: { | |
| operator: TOperation; | |
| conditionValueSchema?: ZodSchema<any>; | |
| dataValueSchema?: ZodSchema<any>; | |
| }) { | |
| const { operator, conditionValueSchema, dataValueSchema } = options; | |
| this.operator = operator; | |
| this.conditionValueSchema = conditionValueSchema; | |
| this.dataValueSchema = dataValueSchema; | |
| } | |
| abstract eval(dataValue: primitive, conditionValue: T): boolean; | |
| execute(dataValue: primitive, conditionValue: T): boolean { | |
| this.validate({ dataValue, conditionValue }); | |
| return this.eval(dataValue, conditionValue); | |
| } | |
| protected validate(args: { dataValue: unknown; conditionValue: unknown }) { | |
| if (this.conditionValueSchema) { | |
| this.validateSchema( | |
| this.conditionValueSchema, | |
| args.conditionValue, | |
| `Invalid condition value`, | |
| ); | |
| } | |
| if (this.dataValueSchema) { | |
| this.validateSchema(this.dataValueSchema, args.dataValue, `Invalid data value`); | |
| } | |
| } | |
| protected validateSchema(schema: ZodSchema<any>, value: unknown, message: string) { | |
| const result = schema.safeParse(value); | |
| if (!result.success) { | |
| throw new ValidationFailedError(this.operator, message, result.error); | |
| } | |
| } | |
| } |
Bal 2130 epic rule engine
Summary by CodeRabbit
New Features
RiskRuleServicefor validating risk rule records from Notion.RuleEngineServicefor executing rules on form data.Bug Fixes
Tests
Chores
NOTION_API_KEY.