RiskRuleService with Notion #2468
Conversation
|
WalkthroughThe recent updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant NotionService
participant NotionAPI
Client->>Controller: Request to get database records
Controller->>NotionService: Get all records for `databaseId`
NotionService->>NotionAPI: Request records from Notion API
NotionAPI-->>NotionService: Respond with records
NotionService->>Controller: Transform and provide records
Controller-->>Client: Send transformed records
sequenceDiagram
participant Client
participant Controller
participant RiskRuleService
Client->>Controller: Request all risk rules
Controller->>RiskRuleService: Find all risk rules
RiskRuleService-->>Controller: Return risk rules
Controller-->>Client: Send risk rules
Poem
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: 3
Outside diff range and nitpick comments (1)
services/workflows-service/src/risk-rule.service.ts (1)
6-11: Ensure that theRuleSchemacovers all necessary fields and types as expected. Consider adding comments to describe each field for better maintainability.
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 (5)
- services/workflows-service/package.json (4 hunks)
- services/workflows-service/prisma/data-migrations (1 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)
Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
Additional comments not posted (9)
services/workflows-service/src/risk-rule.service.ts (3)
13-16: TheRuleSetSchemalooks well-defined. Verify that the operations and the structure align with the business logic.
18-29: The transformation usingdeepCamelKeysis a good practice for consistent data formatting. Ensure that all fields expected from the Notion API are correctly mapped and transformed.
31-47: The type definition forRiskRuleRecordis comprehensive. Ensure that all these fields are indeed utilized and necessary.#!/bin/bash # Description: Check for unused fields in RiskRuleRecord across the codebase. # Search for usages of RiskRuleRecord fields rg --type typescript "RiskRuleRecord"services/workflows-service/src/notion.service.ts (2)
29-49: The methodgetAllDatabaseRecordsValuesis robust and handles data transformation well. Ensure that the error handling is sufficient when the schema parsing fails.
71-112: The methodtransformNotionFieldToValuehandles various Notion field types. Ensure that all types are supported and correctly handled. Consider adding a default case in the switch statement.services/workflows-service/src/env.ts (1)
78-78: The addition ofNOTION_API_KEYtoserverEnvSchemais appropriate. Ensure that it is being used correctly across the application.services/workflows-service/package.json (3)
64-64: The addition of@notionhq/clientis necessary for the new Notion integration. Ensure that the version used is up-to-date and does not have known security vulnerabilities.
103-103: The addition ofstring-tsis appropriate for handling string transformations. Verify that it is used appropriately throughout the project.
77-77: Reintroduction ofballerine-nestjs-typeboxshould be validated to ensure that it is compatible with other dependencies and the current project structure.#!/bin/bash # Description: Verify compatibility of ballerine-nestjs-typebox with other dependencies. # Check for issues related to dependency compatibility npm audit
| @Injectable() | ||
| export class RiskRuleService { | ||
| constructor(private readonly notionService: NotionService) {} | ||
|
|
||
| public async findAll({ | ||
| databaseId, | ||
| source, | ||
| }: { | ||
| databaseId: string; | ||
| source: 'notion'; | ||
| }): Promise<RiskRuleRecord[]> { | ||
| if (source === 'notion') { | ||
| return await this.notionService.getAllDatabaseRecordsValues({ | ||
| databaseId, | ||
| schema: NotionRiskRuleRecordSchema, | ||
| }); | ||
| } | ||
|
|
||
| throw new Error('Unsupported source'); | ||
| } | ||
| } |
There was a problem hiding this comment.
The findAll method in RiskRuleService effectively utilizes the NotionService. However, consider adding error handling for potential API failures or unexpected data formats.
+ try {
return await this.notionService.getAllDatabaseRecordsValues({
databaseId,
schema: NotionRiskRuleRecordSchema,
});
+ } catch (error) {
+ // Log error and handle it appropriately
+ throw new Error('Failed to fetch data from Notion');
+ }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.
| @Injectable() | |
| export class RiskRuleService { | |
| constructor(private readonly notionService: NotionService) {} | |
| public async findAll({ | |
| databaseId, | |
| source, | |
| }: { | |
| databaseId: string; | |
| source: 'notion'; | |
| }): Promise<RiskRuleRecord[]> { | |
| if (source === 'notion') { | |
| return await this.notionService.getAllDatabaseRecordsValues({ | |
| databaseId, | |
| schema: NotionRiskRuleRecordSchema, | |
| }); | |
| } | |
| throw new Error('Unsupported source'); | |
| } | |
| } | |
| @Injectable() | |
| export class RiskRuleService { | |
| constructor(private readonly notionService: NotionService) {} | |
| public async findAll({ | |
| databaseId, | |
| source, | |
| }: { | |
| databaseId: string; | |
| source: 'notion'; | |
| }): Promise<RiskRuleRecord[]> { | |
| if (source === 'notion') { | |
| try { | |
| return await this.notionService.getAllDatabaseRecordsValues({ | |
| databaseId, | |
| schema: NotionRiskRuleRecordSchema, | |
| }); | |
| } catch (error) { | |
| // Log error and handle it appropriately | |
| throw new Error('Failed to fetch data from Notion'); | |
| } | |
| } | |
| throw new Error('Unsupported source'); | |
| } | |
| } |
| constructor( | ||
| private readonly configService: ConfigService, | ||
| private readonly logger: AppLoggerService, | ||
| ) { | ||
| if (!env.NOTION_API_KEY) { | ||
| throw new Error('NOTION_API_KEY is not set'); | ||
| } | ||
|
|
||
| this.client = new Client({ | ||
| auth: this.configService.get(env.NOTION_API_KEY), | ||
| }); | ||
| } |
There was a problem hiding this comment.
The constructor checks for the NOTION_API_KEY and initializes the Notion client. Ensure that the error message is informative and that the API key is being fetched correctly.
- throw new Error('NOTION_API_KEY is not set');
+ throw new Error('NOTION_API_KEY is not set. Please configure it in the environment variables.');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.
| constructor( | |
| private readonly configService: ConfigService, | |
| private readonly logger: AppLoggerService, | |
| ) { | |
| if (!env.NOTION_API_KEY) { | |
| throw new Error('NOTION_API_KEY is not set'); | |
| } | |
| this.client = new Client({ | |
| auth: this.configService.get(env.NOTION_API_KEY), | |
| }); | |
| } | |
| constructor( | |
| private readonly configService: ConfigService, | |
| private readonly logger: AppLoggerService, | |
| ) { | |
| if (!env.NOTION_API_KEY) { | |
| throw new Error('NOTION_API_KEY is not set. Please configure it in the environment variables.'); | |
| } | |
| this.client = new Client({ | |
| auth: this.configService.get(env.NOTION_API_KEY), | |
| }); | |
| } |
| private readonly configService: ConfigService, | ||
| private readonly logger: AppLoggerService, | ||
| ) { | ||
| if (!env.NOTION_API_KEY) { |
There was a problem hiding this comment.
Is it make sense to allow this variable as optional and once the service is initialized you will throw an exception?
There was a problem hiding this comment.
My solution is to create a provider that dynamically selects the appropriate service based on the configuration
something like:
export const dynamicServiceProvider: Provider = {
provide: NotionService,
useFactory: (configService: ConfigService) => {
const key = configService.get<string>('NOTION_KEY');
if (key) {
return new NotionService();
}
return new MockNotionService();
},
inject: [ConfigService],
};
MockNotionService can throw error when a concrete service api is called
There was a problem hiding this comment.
I prefer it to be required so in production if you will forget to set the env var it won't provision the pod. The alternative you suggested will notify only when a user tries to use the feature.
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
services/workflows-service/src/risk-rule.service.ts (1)
14-17: Consider simplifying the recursive definition inRuleSetSchema.The recursive definition using
z.lazy()is valid but can be complex to understand and maintain. If the nesting of rules isn't deeply required, consider flattening the structure or providing additional documentation on its intended use to aid in maintainability.Also applies to: 19-22
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 (4)
- services/workflows-service/package.json (3 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)
Files skipped from review as they are similar to previous changes (3)
- services/workflows-service/package.json
- services/workflows-service/src/env.ts
- services/workflows-service/src/notion.service.ts
| const isJsonString = (value: string) => { | ||
| try { | ||
| JSON.parse(value); | ||
|
|
||
| return true; | ||
| } catch (e) { | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Utility function isJsonString lacks clarity in purpose.
This function is used to validate JSON strings, but its placement and naming could be improved for better clarity and reusability. Consider moving it to a common utility module and renaming it to reflect its functionality more clearly, such as isValidJsonString.
| const NotionRiskRuleRecordSchema = z | ||
| .object({ | ||
| ID: z.string().min(1), | ||
| 'Rule set': z | ||
| .string() | ||
| .refine(isJsonString, 'Not a valid JSON string') | ||
| .transform(value => JSON.parse(value)) | ||
| .pipe(RuleSetSchema), | ||
| Domain: z.string().min(1), | ||
| Indicator: z.string().min(1), | ||
| 'Risk level': z.enum(['positive', 'moderate', 'critical']), | ||
| 'Base risk score': z.number().min(0).max(100), | ||
| 'Additional risk score': z.number().min(0).max(100), | ||
| 'Min risk score': z.number().min(0).max(100), | ||
| 'Max risk score': z.number().min(0).max(100), | ||
| }) | ||
| .transform(value => ({ | ||
| id: value.ID, | ||
| ruleSet: value['Rule set'], | ||
| domain: value.Domain, | ||
| indicator: value.Indicator, | ||
| baseRiskScore: value['Base risk score'], | ||
| additionalRiskScore: value['Additional risk score'], | ||
| minRiskScore: value['Min risk score'], | ||
| maxRiskScore: value['Max risk score'], | ||
| })); |
There was a problem hiding this comment.
Optimize the transformation logic in NotionRiskRuleRecordSchema.
The transformation logic in NotionRiskRuleRecordSchema is quite dense and mixes validation with transformation. This can be split into separate steps or methods to enhance readability and maintainability.
- .transform(value => ({
+ .transform(value => parseNotionRiskRuleRecord(value))And then define parseNotionRiskRuleRecord in a more modular fashion.
Committable suggestion was skipped due to low confidence.
| @Injectable() | ||
| export class RiskRuleService { | ||
| constructor( | ||
| private readonly notionService: NotionService, | ||
| private readonly logger: AppLoggerService, | ||
| ) {} | ||
|
|
||
| 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.
Enhance error handling and logging in findAll method.
The method handles errors locally but does not re-throw them, potentially hiding failures from callers. Consider re-throwing the errors or handling them in a way that callers can react appropriately.
+ } catch (error) {
+ this.logger.error('Failed to process records', { error });
+ throw error; // Re-throw to ensure the caller is aware of the failure.
+ }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.
| @Injectable() | |
| export class RiskRuleService { | |
| constructor( | |
| private readonly notionService: NotionService, | |
| private readonly logger: AppLoggerService, | |
| ) {} | |
| 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'); | |
| } | |
| } | |
| public async findAll({ databaseId, source }: { databaseId: string; source: 'notion' }) { | |
| try { | |
| 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'); | |
| } catch (error) { | |
| this.logger.error('Failed to process records', { error }); | |
| throw error; // Re-throw to ensure the caller is aware of the failure. | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- services/workflows-service/.env.example (1 hunks)
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)
| @@ -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.
Ensure NOTION_API_KEY is securely managed in production.
The static analysis tool flagged the API key as a potential security risk. It's crucial to ensure that in production, the API key is not hardcoded but securely managed, possibly using secrets management tools or environment-specific configurations.
Tools
Gitleaks
36-37: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Document the placeholder nature of NOTION_API_KEY.
The value secret for NOTION_API_KEY appears to be a placeholder. It would be beneficial to add a comment indicating that this value should be replaced with an actual API key in a real deployment.
- NOTION_API_KEY=secret
+ NOTION_API_KEY=secret # Replace with your actual Notion API keyCommittable 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=secret # Replace with your actual Notion API key |
Tools
Gitleaks
36-37: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
* feat: rule engine * feat: rule engine 2 * feat: rule engine - last year * fix: eslint * feat: new rule engine * feat: new rule engine v2 tests * feat: export RuleEngine * feat: prettier and eslint * RiskRuleService with Notion (#2468) * feat: risk-rule-service * fix: pr comments * fix: tests * feat: added parsing of rulegine * feat: generated risk rules plugin logic * fix: add company registered last year * added risk rules plugin to common plugins * feat: removed unnecessary objects in workflow service as they moved to comoon * feat: fixed eslint * feat: updated plugin params * fixed riskRule service * feat: updated invoked plugin and persistence logic * feat: added riskIndicatorsByDomain * fixed workflow service test * feat: updated rules plugin stracture * feat: add exists operator * Please enter the commit message for your changes. Lines starting * Changes to be committed: modified: ../../packages/workflow-core/src/lib/plugins/common-plugin/risk-rules-plugin.ts modified: src/rule-engine/risk-rule.service.ts * fix: exists * fix: import types * fix: backoffice mock data * fix: backoffice mock data * feat: bad merge fix * Added NOT EQUALS, IN and NOT_IN operations to rule engine (#2512) * feat: added NOT EQUALS operation to rule engine * feat: added IN and NOT_IN operations for the rule engine * feat: customizing for new codebase * feat: small fixes * fix: types * fix: types for notion service * fix: ci * fix: ci * feat: reworked ts service & updated schema (WIP) (#2495) * feat: reworked ts service & updated schema * feat: updated array insertion strategy * fix: fixed i18next initialization * fix: fixed tests & minor refactor * fix(conflicts): resolve --------- Co-authored-by: Alon Peretz <Alonp99@gmail.com> --------- Co-authored-by: Lior Zamir <liorz@ballerine.com> Co-authored-by: liorzam <6435752+liorzam@users.noreply.github.com> Co-authored-by: Matan Yadaev <matan.yed@gmail.com> Co-authored-by: Tomer Shvadron <tomers@ballerine.com> Co-authored-by: Illia Rudniev <cheskmr@gmail.com> Co-authored-by: Alon Peretz <Alonp99@gmail.com>
Summary by CodeRabbit
New Features
NotionServicefor retrieving and transforming database records.RiskRuleServicefor handling and retrieving risk rule records.Configuration
NOTION_API_KEYto environment configuration files.Dependencies
@notionhq/clientandstring-ts.@nestjs/serve-staticdependency.