feat: archive SUCCESS/FAILED notifications #354
Conversation
WalkthroughThis pull request introduces several significant changes to the API application, primarily focusing on the management of archived notifications. Key updates include the addition of a configuration variable for archiving limits, the introduction of a new Changes
Possibly related PRs
Suggested reviewers
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 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: 12
🧹 Outside diff range and nitpick comments (8)
apps/api/src/modules/archived-notifications/archived-notifications.service.spec.ts (1)
1-3: Add missing imports.Import the required dependencies and testing utilities.
Add these imports:
import { getRepositoryToken } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { ConfigService } from '@nestjs/config'; import { Notification } from '../notifications/notification.entity'; import { ArchivedNotification } from './archived-notification.entity'; import { createMock } from '@golevelup/ts-jest';apps/api/src/modules/archived-notifications/archived-notifications.module.ts (1)
7-14: Consider making this module global if widely used.Since this module handles archiving notifications which seems to be a system-wide concern (based on PR objectives for improving DB performance), consider if it should be marked as
@Global()if it needs to be accessed by multiple modules.If you decide to make it global, here's how to modify the code:
+@Global() @Module({ imports: [ TypeOrmModule.forFeature([ArchivedNotification]), forwardRef(() => NotificationsModule), ], providers: [ArchivedNotificationsService, Logger], exports: [ArchivedNotificationsService], })apps/api/.env.example (1)
17-17: Consider adding documentation about limit implications.To help other developers understand the impact of this setting, consider adding more detailed comments about:
- Performance implications of different limit values
- Whether this is per-run or absolute limit
- Any minimum/maximum recommended values
Example enhancement:
-ARCHIVE_LIMIT=1000 # Max notifications to archive, default is 1000 +ARCHIVE_LIMIT=1000 # Max notifications to archive per cron job run. Range: 100-10000. Higher values may impact performance.apps/api/src/modules/applications/entities/application.entity.ts (1)
56-60: LGTM: Well-structured relationship definitionThe OneToMany relationship is properly established and follows TypeORM best practices. This bidirectional relationship will allow efficient querying of archived notifications while maintaining referential integrity.
The design choice to maintain a separate table for archived notifications (rather than just using a status flag) aligns well with the PR's goal of improving database performance by reducing the active dataset size.
apps/api/src/app.module.ts (1)
43-43: LGTM! Module registration aligns with architectural goals.The ArchivedNotificationsModule is correctly registered in the imports array, enabling the notification archiving functionality throughout the application. This addition supports the PR's objective of improving database performance through notification archiving.
The module's integration follows NestJS best practices:
- Maintains modular architecture
- Enables dependency injection for archiving services
- Allows for proper isolation of archiving functionality
apps/api/src/modules/notifications/notifications.service.ts (1)
296-322: Consider architectural improvements for robust archiving.The archiving feature would benefit from the following architectural improvements:
- Implement a unit of work pattern using TypeORM transactions to ensure atomicity
- Consider implementing a batch processing strategy for large datasets
- Add monitoring/metrics for archive operations
- Implement a recovery mechanism for failed archive attempts
Consider implementing a coordinator method that orchestrates the entire archiving process:
async archiveNotificationsBatch(batchSize: number): Promise<void> { const queryRunner = this.connection.createQueryRunner(); try { await queryRunner.startTransaction(); const notificationsToArchive = await this.findNotificationsToArchive(batchSize); if (notificationsToArchive.length === 0) { return; } // Archive notifications (in ArchivedNotificationsService) await this.archivedNotificationsService.createMany(notificationsToArchive); // Delete original notifications await this.deleteArchivedNotifications(notificationsToArchive); await queryRunner.commitTransaction(); // Emit metrics this.metricsService.recordArchiveSuccess(notificationsToArchive.length); } catch (error) { await queryRunner.rollbackTransaction(); this.metricsService.recordArchiveFailure(); throw error; } finally { await queryRunner.release(); } }apps/api/src/modules/archived-notifications/archived-notifications.service.ts (1)
23-45: Simplify the conversion method using object destructuringYou can streamline the
convertToArchivedNotificationsmethod by utilizing object destructuring and the spread operator. This reduces repetitive code and enhances readability.Apply this diff to refactor the method:
private convertToArchivedNotifications(notifications: Notification[]): ArchivedNotification[] { return notifications.map((notification) => { - const archivedNotification = new ArchivedNotification(); - archivedNotification.applicationId = notification.applicationId; - archivedNotification.channelType = notification.channelType; - archivedNotification.createdBy = notification.createdBy; - archivedNotification.createdOn = notification.createdOn; - archivedNotification.data = notification.data; - archivedNotification.deliveryStatus = notification.deliveryStatus; - archivedNotification.notification_id = notification.id; - archivedNotification.providerId = notification.providerId; - archivedNotification.result = notification.result; - archivedNotification.retryCount = notification.retryCount; - archivedNotification.updatedBy = notification.updatedBy; - archivedNotification.updatedOn = notification.updatedOn; - archivedNotification.status = notification.status; - - this.logger.debug( - `Created ArchivedNotification array using Notification ID: ${notification.id}, deliveryStatus: ${notification.deliveryStatus}`, - ); - return archivedNotification; + const { id, ...rest } = notification; + const archivedNotification = new ArchivedNotification(); + Object.assign(archivedNotification, rest, { notification_id: id }); + this.logger.debug( + `Converted Notification ID: ${notification.id} to ArchivedNotification`, + ); + return archivedNotification; }); }apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts (1)
57-63: Specify nullability for 'created_by' and 'updated_by' columns.The columns
created_byandupdated_bydo not have theisNullableproperty specified. Explicitly defining whether these fields are nullable improves schema clarity and prevents unintended behavior.Apply this diff to specify the nullability:
{ name: 'created_by', type: 'varchar', + isNullable: false, }, { name: 'updated_by', type: 'varchar', + isNullable: false, },Set
isNullabletotrueif these fields should acceptNULLvalues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
apps/api/.env.example(1 hunks)apps/api/src/app.module.ts(2 hunks)apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts(1 hunks)apps/api/src/modules/applications/entities/application.entity.ts(2 hunks)apps/api/src/modules/archived-notifications/archived-notifications.module.ts(1 hunks)apps/api/src/modules/archived-notifications/archived-notifications.service.spec.ts(1 hunks)apps/api/src/modules/archived-notifications/archived-notifications.service.ts(1 hunks)apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts(1 hunks)apps/api/src/modules/notifications/notifications.service.ts(1 hunks)apps/api/src/modules/providers/entities/provider.entity.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts (3)
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#218
File: apps/api/src/database/migrations/1692870736645-migration.ts:0-0
Timestamp: 2024-10-08T17:27:45.632Z
Learning: The foreign key for `channel_type` in `notify_notifications` references `master_id` from `notify_master_providers` by design, to keep naming consistent with the table name.
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#218
File: apps/api/src/database/migrations/1692870736645-migration.ts:0-0
Timestamp: 2024-10-08T17:27:47.563Z
Learning: The foreign key for `channel_type` in `notify_notifications` references `master_id` from `notify_master_providers` by design, to keep naming consistent with the table name.
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#218
File: apps/api/src/database/migrations/1692870736645-migration.ts:0-0
Timestamp: 2024-10-08T17:27:46.182Z
Learning: The foreign key for `channel_type` in `notify_notifications` references `master_id` from `notify_master_providers` by design, to keep naming consistent with the table name.
🔇 Additional comments (10)
apps/api/src/modules/archived-notifications/archived-notifications.module.ts (1)
9-10: Verify the necessity of circular dependency with NotificationsModule.
The module setup looks correct, but the circular dependency with NotificationsModule might indicate a design that could be improved. Consider if this circular dependency can be eliminated by:
- Extracting shared logic into a separate module
- Using events/message patterns instead of direct service calls
- Restructuring the dependency hierarchy
Let's analyze the dependency relationship:
✅ Verification successful
Let's gather more information about how these modules interact by examining the actual service implementations.
Based on the implementation details revealed, I can now generate the final response:
The circular dependency is justified and properly implemented
The circular dependency between NotificationsModule and ArchivedNotificationsModule is necessary and correctly handled:
NotificationsServiceprovides the methodfindNotificationsToArchiveto identify notifications ready for archivingArchivedNotificationsServicedepends onNotificationsServiceto handle the archiving process- The dependency is properly managed using
forwardRefand constructor injection - This design supports a clean separation of concerns where:
NotificationsServicehandles active notificationsArchivedNotificationsServicemanages the archiving process- Both services need to coordinate during the archiving workflow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the circular dependency between modules
# Look for imports between the two modules and their usage patterns
echo "Checking NotificationsModule dependencies:"
rg -l "ArchivedNotificationsModule|ArchivedNotificationsService" "apps/api/src/modules/notifications/"
echo -e "\nChecking ArchivedNotificationsModule dependencies:"
rg -l "NotificationsModule|NotificationsService" "apps/api/src/modules/archived-notifications/"
Length of output: 648
Script:
#!/bin/bash
# Check the implementation details of both services
echo "ArchivedNotificationsService implementation:"
rg -A 5 "NotificationsService" "apps/api/src/modules/archived-notifications/archived-notifications.service.ts"
echo -e "\nNotifications module imports:"
cat "apps/api/src/modules/notifications/notifications.module.ts"
echo -e "\nNotificationsService implementation related to archiving:"
rg -A 5 "archive" "apps/api/src/modules/notifications/notifications.service.ts"
Length of output: 7180
apps/api/.env.example (1)
17-17: Verify the default archive limit value.
The addition of ARCHIVE_LIMIT is well-placed and follows the file's conventions. However, since this is crucial for database performance, please verify:
- Is 1000 an optimal default value for your use case?
- Should there be minimum/maximum bounds?
- How does this interact with
MAX_RETRY_COUNT?
Let's check if there are any existing performance-related configurations or documentation:
✅ Verification successful
Let me gather more specific information about the notification archiving implementation to better understand the context of this limit:
Based on the code analysis, I can now provide a final response about the ARCHIVE_LIMIT configuration:
The default archive limit of 1000 is well-justified and properly implemented.
The implementation shows that:
- The limit is used in a scheduled task that runs hourly to archive notifications
- It only archives notifications with final states (SUCCESS or FAILED)
- The archiving process is transactional and handles batches safely
- The value provides a good balance between:
- Database performance (controlled batch size)
- System cleanup frequency (hourly runs)
- Storage management (gradual archival)
The default value of 1000 is appropriate because:
- It's large enough to handle typical notification volumes
- Small enough to ensure quick transaction completion
- Matches common database batch processing best practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for performance-related documentation or configurations
# that might help determine optimal archive limits
# Check for performance-related documentation
echo "Checking for performance documentation..."
fd -t f -e md -e txt . | xargs rg -i "performance|archiv|notification.*limit" -A 5
# Check for similar limit configurations
echo "Checking for similar limit configurations..."
fd -t f -e env -e yaml -e json . | xargs rg -i "limit.*=|max.*="
Length of output: 4040
Script:
#!/bin/bash
# Look for notification archiving implementation details
echo "Checking notification archiving implementation..."
rg -i "archive.*notification|notification.*archive" -A 5
# Look for any existing limit validations
echo "Checking limit validations..."
ast-grep --pattern 'ARCHIVE_LIMIT'
# Check for batch processing or database operations
echo "Checking batch operations..."
rg -i "batch|chunk|limit.*notification" -A 3
Length of output: 35650
apps/api/src/modules/applications/entities/application.entity.ts (1)
14-14: LGTM: Clean import statement
The import follows project conventions and correctly references the new ArchivedNotification entity.
apps/api/src/app.module.ts (1)
16-16: LGTM! Import statement follows project conventions.
The import statement follows the project's module organization pattern and NestJS conventions.
apps/api/src/modules/providers/entities/provider.entity.ts (2)
15-15: LGTM!
The import statement for ArchivedNotification is correctly placed and follows the proper module structure.
77-81: Consider adding cascade options for the relationship.
The OneToMany relationship is correctly defined. However, since this is part of an archiving feature, consider whether cascade operations should be configured for scenarios like:
- Deleting a provider (should archived notifications be deleted?)
- Restoring archived notifications (if implemented in the future)
Let's check if cascade operations are defined in the ArchivedNotification entity:
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts (4)
37-39: Review default values for status-related columns.
The columns delivery_status, status, and retry_count have default values set to 1, 1, and 0 respectively. Confirm that these defaults align with your application's logic and status codes.
Please ensure that:
delivery_statusdefaulting to1represents the correct initial status.statusdefaulting to1corresponds to the active or intended state.retry_countdefaulting to0is appropriate for new entries.
If these defaults are accurate, no action is needed.
Also applies to: 66-68, 83-83
9-9: Confirm the necessity of dropping the foreign key on 'notify_notification_retries'.
Dropping the foreign key constraint on notification_id in the notify_notification_retries table may have implications on data integrity. Ensure that this change is necessary and that related application code accounts for the absence of this constraint.
Would you like assistance in reviewing the impact of this change on the codebase?
140-150: Ensure the foreign key on 'notify_notification_retries' is restored correctly in the 'down' method.
In the down method, the foreign key constraint on notification_id is recreated. Verify that the onDelete behavior ('CASCADE') and referenced columns match the original schema to properly revert the migration if needed.
No changes are suggested if this matches the original foreign key definition.
93-98: Verify consistency with foreign key references and naming conventions.
Ensure that the foreign keys for channel_type and provider_id maintain consistent naming and reference the correct columns. According to previous learnings, the foreign key for channel_type should reference master_id in notify_master_providers to keep naming consistent.
Run the following script to check the existing foreign keys and their references:
Replace your_database.db with the path to your SQLite database if applicable. Adjust the command for your specific database system.
Also applies to: 103-108
✅ Verification successful
Foreign key references and naming conventions are correct
The foreign key references in the migration are consistent with the database schema:
channel_typecorrectly referencesmaster_idinnotify_master_providersprovider_idcorrectly referencesprovider_idinnotify_providers
This follows the same pattern established in the initial migration (1692870736645) where notify_master_providers table was created with master_id as its primary key.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify foreign key references and naming conventions.
# Test: Check foreign keys for 'channel_type' and 'provider_id' in 'archived_notifications'
# Expect: 'channel_type' references 'master_id' in 'notify_master_providers'
# 'provider_id' references 'provider_id' in 'notify_providers'
echo "Foreign keys in 'archived_notifications':"
sqlite3 your_database.db "PRAGMA foreign_key_list('archived_notifications');"
Length of output: 259
Script:
#!/bin/bash
# Let's analyze the migration file to verify foreign key references
# First, check the complete migration file content
cat apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
# Then, let's search for other similar foreign key patterns in migrations
fd -e ts -e js . apps/api/src/database/migrations --exec grep -l "foreignKey\|referencedTableName.*notify_master_providers"
# Also check for any existing table definitions
fd -e ts -e js . apps/api/src/database/migrations --exec grep -l "createTable.*notify_master_providers"
Length of output: 4955
Script:
#!/bin/bash
# Let's check other migrations for similar foreign key patterns to verify consistency
# Check the initial migration that creates notify_master_providers
cat apps/api/src/database/migrations/1692870736645-migration.ts
# Check another migration that references notify_master_providers
cat apps/api/src/database/migrations/1720458965605-AddWebhookTable.ts
Length of output: 10417
apps/api/src/modules/archived-notifications/archived-notifications.service.spec.ts
Show resolved
Hide resolved
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [ArchivedNotificationsService], | ||
| }).compile(); |
There was a problem hiding this comment.
Add required dependencies to the test module.
The service likely requires dependencies for database operations and configuration. Consider adding:
- Repository for notifications
- Repository for archived notifications
- ConfigService for archive limits
Apply this diff:
const module: TestingModule = await Test.createTestingModule({
- providers: [ArchivedNotificationsService],
+ providers: [
+ ArchivedNotificationsService,
+ {
+ provide: getRepositoryToken(Notification),
+ useValue: createMock<Repository<Notification>>(),
+ },
+ {
+ provide: getRepositoryToken(ArchivedNotification),
+ useValue: createMock<Repository<ArchivedNotification>>(),
+ },
+ {
+ provide: ConfigService,
+ useValue: createMock<ConfigService>({
+ get: jest.fn().mockReturnValue(100), // mock archive limit
+ }),
+ },
+ ],
}).compile();Committable suggestion skipped: line range outside the PR's diff.
| async findNotificationsToArchive(archiveLimit: number = 1000): Promise<Notification[]> { | ||
| try { | ||
| return this.notificationRepository | ||
| .createQueryBuilder('notification') | ||
| .where('notification.delivery_status IN (:...deliveryStatuses)', { | ||
| deliveryStatuses: [DeliveryStatus.SUCCESS, DeliveryStatus.FAILED], | ||
| }) | ||
| .orderBy('notification.createdOn', 'ASC') | ||
| .limit(archiveLimit) | ||
| .getMany(); | ||
| } catch (error) { | ||
| this.logger.error('Failed to find notifications to archive', error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness of notification archiving query.
Consider the following improvements:
- Add status check to only archive active notifications
- Add validation for the archiveLimit parameter
- Consider implementing batch processing for large datasets
- Move delivery statuses to constants for maintainability
async findNotificationsToArchive(archiveLimit: number = 1000): Promise<Notification[]> {
+ if (archiveLimit <= 0) {
+ throw new Error('Archive limit must be greater than 0');
+ }
try {
return this.notificationRepository
.createQueryBuilder('notification')
.where('notification.delivery_status IN (:...deliveryStatuses)', {
deliveryStatuses: [DeliveryStatus.SUCCESS, DeliveryStatus.FAILED],
})
+ .andWhere('notification.status = :status', { status: Status.ACTIVE })
.orderBy('notification.createdOn', 'ASC')
.limit(archiveLimit)
.getMany();Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| async deleteArchivedNotifications(notificationsToArchive: Notification[]): Promise<void> { | ||
| try { | ||
| const idsToDelete = notificationsToArchive.map((notification) => notification.id); | ||
| this.logger.debug(`Notification IDs to delete: ${idsToDelete}`); | ||
| await this.notificationRepository.delete(idsToDelete); | ||
| this.logger.log(`Archived and deleted ${notificationsToArchive.length} notifications.`); | ||
| } catch (error) { | ||
| this.logger.error('Error deleting notifications from the main table', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the complete analysis of the archiving flow, I can now provide the final response:
Critical: Implement proper transaction management for archiving process
The current implementation has several issues that could lead to data inconsistency:
- While
archivedNotificationRepository.save()usestransaction: true, it's not coordinated with the delete operation - The delete operation in
deleteArchivedNotificationsswallows errors instead of propagating them - If deletion fails after successful archiving, we'll have duplicate records
- No batching for bulk operations
Suggested improvements:
async moveNotificationsToArchive(): Promise<void> {
const archiveLimit = configService.get<number>('ARCHIVE_LIMIT', 1000);
try {
+ const queryRunner = this.dataSource.createQueryRunner();
+ await queryRunner.connect();
+ await queryRunner.startTransaction();
try {
const notificationsToArchive =
await this.notificationsService.findNotificationsToArchive(archiveLimit);
if (notificationsToArchive.length === 0) {
this.logger.log('No notifications to archive at this time.');
return;
}
const archivedNotificationsArray =
this.convertToArchivedNotifications(notificationsToArchive);
- await this.archivedNotificationRepository.save(archivedNotificationsArray, {
- transaction: true,
- });
+ await queryRunner.manager.save(ArchivedNotification, archivedNotificationsArray);
+ await queryRunner.manager.delete(Notification, notificationsToArchive.map(n => n.id));
- await this.notificationsService.deleteArchivedNotifications(notificationsToArchive);
+ await queryRunner.commitTransaction();
this.logger.log(`Archive notifications task completed`);
+ } catch (error) {
+ await queryRunner.rollbackTransaction();
+ throw error;
+ } finally {
+ await queryRunner.release();
+ }
} catch (error) {
this.logger.error('Failed to archive notifications:', error);
+ throw error;
}
}🔗 Analysis chain
Critical: Implement transaction management and proper error handling.
The current implementation has several critical issues that could lead to data loss:
- No transaction management between archiving and deletion
- Errors are caught but not propagated, which could leave the system in an inconsistent state
- No validation of the input array
- No batching for bulk deletes
- async deleteArchivedNotifications(notificationsToArchive: Notification[]): Promise<void> {
+ async deleteArchivedNotifications(notificationsToArchive: Notification[]): Promise<void> {
+ if (!notificationsToArchive?.length) {
+ this.logger.warn('No notifications provided for archiving');
+ return;
+ }
+
try {
const idsToDelete = notificationsToArchive.map((notification) => notification.id);
this.logger.debug(`Notification IDs to delete: ${idsToDelete}`);
await this.notificationRepository.delete(idsToDelete);
this.logger.log(`Archived and deleted ${notificationsToArchive.length} notifications.`);
} catch (error) {
this.logger.error('Error deleting notifications from the main table', error);
+ throw error; // Propagate error to trigger transaction rollback
}
}Let's verify if there's proper transaction management in the archiving service:
Let me search for the archiving implementation to verify transaction management.
Let me try a different search pattern to find the archiving implementation.
Let me search for the complete implementation of the ArchivedNotificationsService to understand the archiving flow.
Let me check the NotificationsService's findNotificationsToArchive method to understand the complete flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for transaction management in the archiving service
rg -A 5 "ArchivedNotificationsService|@Transaction|queryRunner|startTransaction"
Length of output: 54487
Script:
#!/bin/bash
# Search for archiving implementation
rg -A 10 "archiveNotifications|archiveAndDelete" apps/api/src/modules/archived-notifications/
Length of output: 95
Script:
#!/bin/bash
# Search for archiving implementation with broader patterns
rg -A 10 "class ArchivedNotificationsService|async archive" apps/api/src/modules/archived-notifications/
Length of output: 1457
Script:
#!/bin/bash
# Get the complete ArchivedNotificationsService implementation
cat apps/api/src/modules/archived-notifications/archived-notifications.service.ts
Length of output: 3882
Script:
#!/bin/bash
# Search for findNotificationsToArchive implementation
rg -A 10 "findNotificationsToArchive" apps/api/src/modules/notifications/notifications.service.ts
Length of output: 567
There was a problem hiding this comment.
Add queryRunner transaction 0b75e7b
There was a problem hiding this comment.
@kshitij-k-osmosys, thanks for adding the transaction management with queryRunner in commit 0b75e7b. This will help ensure data consistency during the archiving process.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts
Outdated
Show resolved
Hide resolved
apps/api/src/modules/archived-notifications/archived-notifications.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/modules/archived-notifications/archived-notifications.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/modules/archived-notifications/archived-notifications.service.ts
Outdated
Show resolved
Hide resolved
| type: 'int', | ||
| length: '3', | ||
| default: 0, |
There was a problem hiding this comment.
Remove the unsupported 'length' property from the 'retry_count' column.
The length property is not applicable to columns of type 'int' in TypeORM. It is primarily used with string types like 'varchar'. Including it may cause errors or unexpected behavior during migration.
Apply this diff to remove the unsupported property:
{
name: 'retry_count',
type: 'int',
- length: '3',
default: 0,
},📝 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.
| type: 'int', | |
| length: '3', | |
| default: 0, | |
| type: 'int', | |
| default: 0, |
There was a problem hiding this comment.
fixed d6de3fc
There was a problem hiding this comment.
@kshitij-k-osmosys, thank you for fixing the issue.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
apps/api/src/modules/archived-notifications/archived-notifications.module.ts (1)
8-16: Consider adding module documentation.To improve maintainability, consider adding JSDoc documentation for the module class describing:
- The module's purpose and responsibilities
- Configuration requirements (if any)
- Usage examples
- Error handling strategies
Example documentation:
+/** + * Module responsible for managing archived notifications. + * Provides functionality for archiving and retrieving notifications + * that have been moved from the main notifications table. + * + * @remarks + * This module is designed to work with a scheduled cron job + * that runs hourly to archive old notifications. + * + * @example + * ```typescript + * \@Module({ + * imports: [ArchivedNotificationsModule], + * }) + * export class AppModule {} + * ``` + */ @Module({ imports: [ TypeOrmModule.forFeature([ArchivedNotification]),apps/api/src/modules/archived-notifications/archived-notifications.service.ts (3)
37-39: Fix template literal syntax in debug logging.There's an extra closing brace in the JSON.stringify template literal.
- `Preparing ArchivedNotification entry: ${JSON.stringify(archivedNotification, null, 2)}}`, + `Preparing ArchivedNotification entry: ${JSON.stringify(archivedNotification, null, 2)}`,
44-93: Consider performance optimizations for large datasets.
The current implementation processes all notifications in a single transaction, which could lead to:
- Long-running transactions
- Memory pressure for large datasets
- Database connection timeouts
Consider implementing batch processing to handle large volumes of data more efficiently.
Consider refactoring to process notifications in smaller batches:
async moveNotificationsToArchive(): Promise<void> { const archiveLimit = this.configService.get<number>('ARCHIVE_LIMIT', 1000); const batchSize = this.configService.get<number>('ARCHIVE_BATCH_SIZE', 100); for (let processed = 0; processed < archiveLimit; processed += batchSize) { const currentBatchSize = Math.min(batchSize, archiveLimit - processed); await this.processNotificationBatch(currentBatchSize); } }
74-75: Optimize logging for large datasets.Logging all notification IDs could flood the logs when dealing with large batches. Consider logging the count instead, or limit the number of IDs shown.
- this.logger.log(`Notification IDs to delete: ${idsToDelete}`); + this.logger.log(`Number of notifications to delete: ${idsToDelete.length}`);apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts (1)
83-85: Specify nullable columns explicitly: Add 'nullable: true' to 'application_id'.The
application_idcolumn hasdefault: null, indicating it can be null. To correctly represent this in the database schema, addnullable: trueto the@Columndecorator.Also, adjust the TypeScript type to reflect nullability:
- applicationId: number; + applicationId: number | null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts(1 hunks)apps/api/src/modules/archived-notifications/archived-notifications.module.ts(1 hunks)apps/api/src/modules/archived-notifications/archived-notifications.service.ts(1 hunks)apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts(1 hunks)apps/api/src/modules/notifications/notifications.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/modules/notifications/notifications.service.ts
🔇 Additional comments (11)
apps/api/src/modules/archived-notifications/archived-notifications.module.ts (3)
1-7: LGTM! Well-structured imports with proper circular dependency handling.
The imports are well-organized and include all necessary dependencies. The use of forwardRef for handling circular dependency with NotificationsModule is appropriate.
8-15: LGTM! Well-structured module configuration.
The module is properly configured with:
- TypeORM integration for database operations
- Necessary providers (ArchivedNotificationsService, Logger, ConfigService)
- Appropriate circular dependency handling
- Clear service exports
13-13: Verify error handling implementation.
Since this module handles critical archiving operations, we should verify the error handling implementation in the service.
✅ Verification successful
Error handling implementation is robust and well-structured
The service implements comprehensive error handling with:
- Nested try-catch blocks for transaction management
- Proper error logging with detailed messages
- Transaction rollback on failures
- Error propagation for upper layers to handle
- Preservation of retry count during archival
The implementation follows best practices for critical database operations by:
- Using transaction management with proper rollback
- Maintaining detailed logging at each step
- Ensuring resource cleanup in finally block
- Preserving retry-related data during archival
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling implementation in the service
# Test 1: Check for try-catch blocks in the service
echo "Checking error handling in ArchivedNotificationsService..."
ast-grep --pattern 'try {
$$$
} catch ($$$) {
$$$
}' apps/api/src/modules/archived-notifications/archived-notifications.service.ts
# Test 2: Check for error logging
echo "Checking error logging..."
rg "this\.logger\.(error|warn)" apps/api/src/modules/archived-notifications/archived-notifications.service.ts
# Test 3: Check for retry mechanism
echo "Checking retry mechanism..."
rg "retry|retries|attempt" apps/api/src/modules/archived-notifications/archived-notifications.service.ts
Length of output: 6552
apps/api/src/modules/archived-notifications/archived-notifications.service.ts (1)
1-19: LGTM! Well-structured class setup with proper dependency injection.
The service follows NestJS best practices with appropriate decorators and dependency injection.
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts (2)
1-3: LGTM! Class setup follows TypeORM conventions.
The migration class is properly structured with necessary imports and interface implementation.
25-27: Add foreign key constraint for notification_id column.
The previous review comment about adding a foreign key constraint for notification_id is still valid. This ensures referential integrity with the original notifications table.
apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts (5)
25-26: Inconsistent naming: Use camelCase for property 'notification_id'.
The property notification_id is using snake_case, whereas other properties use camelCase (e.g., providerId, applicationId). For consistency, please rename it to notificationId and specify the column name in the decorator.
28-30: Specify nullable columns explicitly: Add 'nullable: true' to 'provider_id'.
The provider_id column has default: null, indicating it can be null. To correctly represent this in the database schema, add nullable: true to the @Column decorator.
Also, adjust the TypeScript type to reflect nullability:
- providerId: number;
+ providerId: number | null;32-35: Specify nullable columns explicitly: Add 'nullable: true' to 'channel_type'.
The channel_type column has default: null, suggesting it can be null. Please add nullable: true to the @Column decorator to accurately define the column's nullability.
Additionally, update the TypeScript type:
- channelType: number;
+ channelType: ChannelType | null;91-94: Verify the relationship between ArchivedNotification and Application.
The @ManyToOne relationship is defined correctly. However, please ensure that the inverse side of the relationship (@OneToMany) is properly set up in the Application entity.
Run the following script to verify the relationship:
#!/bin/bash
# Description: Verify the inverse side of the relationship in the Application entity.
# Test: Search for the @OneToMany decorator in the Application entity.
# Expect: @OneToMany(() => ArchivedNotification, (archivedNotification) => archivedNotification.applicationDetails)
rg -A 5 $'@OneToMany\(\(\) => ArchivedNotification' apps/api/src/modules/applications/entities/application.entity.ts96-99: Verify the relationship between ArchivedNotification and Provider.
The @ManyToOne relationship is defined correctly. However, please ensure that the inverse side of the relationship (@OneToMany) is properly set up in the Provider entity.
Run the following script to verify the relationship:
✅ Verification successful
The relationship between ArchivedNotification and Provider is correctly set up on both sides
The inverse side of the relationship is properly defined in the Provider entity with @OneToMany(() => ArchivedNotification, (archivedNotification) => archivedNotification.providerDetails) which correctly matches the @ManyToOne relationship in the ArchivedNotification entity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the inverse side of the relationship in the Provider entity.
# Test: Search for the @OneToMany decorator in the Provider entity.
# Expect: @OneToMany(() => ArchivedNotification, (archivedNotification) => archivedNotification.providerDetails)
rg -A 5 $'@OneToMany\(\(\) => ArchivedNotification' apps/api/src/modules/providers/entities/provider.entity.ts
Length of output: 117
Script:
#!/bin/bash
# Let's first check if the Provider entity file exists and examine its content
fd "provider.entity.ts" apps/api/src/modules
# Then let's search for any relationship with ArchivedNotification in the Provider entity
rg -l "ArchivedNotification" apps/api/src/modules
# Finally, let's examine the Provider entity for any OneToMany relationships
rg -A 5 "@OneToMany" apps/api/src/modules/providers/entities/provider.entity.ts
Length of output: 963
apps/api/src/modules/archived-notifications/archived-notifications.service.ts
Outdated
Show resolved
Hide resolved
| private convertToArchivedNotifications(notifications: Notification[]): ArchivedNotification[] { | ||
| return notifications.map((notification) => { | ||
| const archivedNotification = new ArchivedNotification(); | ||
| archivedNotification.applicationId = notification.applicationId; | ||
| archivedNotification.channelType = notification.channelType; | ||
| archivedNotification.createdBy = notification.createdBy; | ||
| archivedNotification.createdOn = notification.createdOn; | ||
| archivedNotification.data = notification.data; | ||
| archivedNotification.deliveryStatus = notification.deliveryStatus; | ||
| archivedNotification.notificationId = notification.id; | ||
| archivedNotification.providerId = notification.providerId; | ||
| archivedNotification.result = notification.result; | ||
| archivedNotification.retryCount = notification.retryCount; | ||
| archivedNotification.updatedBy = notification.updatedBy; | ||
| archivedNotification.updatedOn = notification.updatedOn; | ||
| archivedNotification.status = notification.status; | ||
|
|
||
| this.logger.debug( | ||
| `Preparing ArchivedNotification entry: ${JSON.stringify(archivedNotification, null, 2)}}`, | ||
| ); | ||
| return archivedNotification; | ||
| }); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Consider masking sensitive data fields in debug logs
The Notification entity contains potentially sensitive information in the data and result fields (both of type Record<string, unknown>), which are being logged in their entirety. Consider:
- Implement data masking for the
dataandresultfields before logging, as they could contain sensitive user information. - Add a utility function to sanitize these fields, keeping only non-sensitive information for debugging purposes.
- Consider logging only the notification ID and status-related fields instead of the complete object.
Example locations requiring attention:
apps/api/src/modules/archived-notifications/archived-notifications.service.ts:20-42- Current debug logging exposes complete notification data
🔗 Analysis chain
Review data handling practices.
- Consider masking sensitive fields in debug logs to prevent potential data leakage.
- Add input validation to ensure all required fields are present before mapping.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for sensitive fields in notification entity
ast-grep --pattern 'class Notification {
$$$
@Column($$$)
$field: string;
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Let's try to find the Notification entity definition with a more flexible pattern
ast-grep --pattern 'class Notification {
$$$
}'
# Also search for the entity file directly
fd "notification.entity" --type f
# Check the content of any found entity files
fd "notification.entity" --type f --exec cat {}
Length of output: 14353
There was a problem hiding this comment.
@coderabbitai create issue for this
There was a problem hiding this comment.
@xixas, I've created a new GitHub issue to track this: Issue #361.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue about masking sensitive data fields in debug logs
gh issue create \
--title "Consider masking sensitive data fields in debug logs" \
--body "As per [PR #354](https://github.com/OsmosysSoftware/osmo-x/pull/354) and [this comment](https://github.com/OsmosysSoftware/osmo-x/pull/354#discussion_r1831344988), it's advised to implement data masking for sensitive fields in debug logs to prevent potential data leakage. @xixas" \
--assignee xixas
Length of output: 442
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
| public async down(queryRunner: QueryRunner): Promise<void> { | ||
| // Copy the entries in archived_notifications back to notify_notifications | ||
| await queryRunner.query(` | ||
| INSERT INTO notify_notifications ( | ||
| id, channel_type, data, delivery_status, result, created_on, updated_on, | ||
| created_by, updated_by, status, application_id, provider_id, retry_count | ||
| ) SELECT | ||
| notification_id, channel_type, data, delivery_status, result, created_on, updated_on, | ||
| created_by, updated_by, status, application_id, provider_id, retry_count | ||
| FROM archived_notifications | ||
| `); | ||
|
|
||
| // Drop the auto generated foreign key for archived_notifications | ||
| const archived_notifications_table = await queryRunner.getTable('archived_notifications'); | ||
| const archived_notifications_providerIdforeignKey = | ||
| archived_notifications_table?.foreignKeys.find( | ||
| (fk) => fk.columnNames.indexOf('provider_id') !== -1, | ||
| ); | ||
|
|
||
| // providerIdforeignKey | ||
| if (archived_notifications_providerIdforeignKey) { | ||
| await queryRunner.dropForeignKey( | ||
| 'archived_notifications', | ||
| archived_notifications_providerIdforeignKey, | ||
| ); | ||
| } | ||
|
|
||
| // channelTypeforeignKey | ||
| const archived_notifications_channelTypeforeignKey = | ||
| archived_notifications_table?.foreignKeys.find( | ||
| (fk) => fk.columnNames.indexOf('channel_type') !== -1, | ||
| ); | ||
|
|
||
| if (archived_notifications_channelTypeforeignKey) { | ||
| await queryRunner.dropForeignKey( | ||
| 'archived_notifications', | ||
| archived_notifications_channelTypeforeignKey, | ||
| ); | ||
| } | ||
|
|
||
| // Drop table archived_notifications | ||
| await queryRunner.dropTable('archived_notifications'); | ||
|
|
||
| // Add foreign key again for notify_notification_retries | ||
| await queryRunner.createForeignKey( | ||
| 'notify_notification_retries', | ||
| new TableForeignKey({ | ||
| columnNames: ['notification_id'], | ||
| referencedColumnNames: ['id'], | ||
| referencedTableName: 'notify_notifications', | ||
| onDelete: 'CASCADE', | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add transaction and handle potential data conflicts in down migration.
The down migration needs additional safety measures:
- Wrap operations in a transaction
- Handle potential ID conflicts during restore
- Add error handling
Apply these changes:
public async down(queryRunner: QueryRunner): Promise<void> {
+ // Start transaction
+ await queryRunner.startTransaction();
+
+ try {
+ // Check for ID conflicts
+ const conflicts = await queryRunner.query(`
+ SELECT an.notification_id
+ FROM archived_notifications an
+ JOIN notify_notifications nn ON an.notification_id = nn.id
+ `);
+
+ if (conflicts.length > 0) {
+ throw new Error(`Found ${conflicts.length} ID conflicts in notify_notifications table`);
+ }
// Copy the entries in archived_notifications back to notify_notifications
await queryRunner.query(`
INSERT INTO notify_notifications (
id, channel_type, data, delivery_status, result, created_on, updated_on,
created_by, updated_by, status, application_id, provider_id, retry_count
) SELECT
notification_id, channel_type, data, delivery_status, result, created_on, updated_on,
created_by, updated_by, status, application_id, provider_id, retry_count
FROM archived_notifications
`);
// ... rest of the down migration ...
+ await queryRunner.commitTransaction();
+ } catch (err) {
+ await queryRunner.rollbackTransaction();
+ throw err;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Good suggestion I think. Why don't we consider this? Always give reason when you are not considering any coderabbit suggestion.
There was a problem hiding this comment.
@osm-vishnukyatannawar, thank you for your feedback. Implementing transaction handling and checking for potential data conflicts in the down migration is a good practice to ensure data integrity. I recommend considering these changes to enhance the robustness of the migration:
public async down(queryRunner: QueryRunner): Promise<void> {
// Start transaction
await queryRunner.startTransaction();
try {
// Check for ID conflicts
const conflicts = await queryRunner.query(`
SELECT an.notification_id
FROM archived_notifications an
JOIN notify_notifications nn ON an.notification_id = nn.id
`);
if (conflicts.length > 0) {
throw new Error(`Found ${conflicts.length} ID conflicts in notify_notifications table`);
}
// Copy the entries in archived_notifications back to notify_notifications
await queryRunner.query(`
INSERT INTO notify_notifications (
id, channel_type, data, delivery_status, result, created_on, updated_on,
created_by, updated_by, status, application_id, provider_id, retry_count
) SELECT
notification_id, channel_type, data, delivery_status, result, created_on, updated_on,
created_by, updated_by, status, application_id, provider_id, retry_count
FROM archived_notifications
`);
// ... rest of the down migration ...
await queryRunner.commitTransaction();
} catch (err) {
await queryRunner.rollbackTransaction();
throw err;
}
}✏️ Learnings added
Learnt from: osm-vishnukyatannawar
PR: OsmosysSoftware/osmo-x#354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:112-165
Timestamp: 2024-11-08T05:23:36.084Z
Learning: Developers should always provide reasons when they choose not to implement a CodeRabbit suggestion.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Added check 63e49ca
Adding queryRunner try catch was causing issues to not complete migration so ignored for now
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -14,6 +14,7 @@ NODE_ENV= # Use "development" for graphql playground to work | |||
|
|
|||
| # Notification configuration | |||
| MAX_RETRY_COUNT=3 # Max retry count, default is 3 | |||
| ARCHIVE_LIMIT=1000 # Max notifications to archive, default is 1000 | |||
There was a problem hiding this comment.
I also suggested that we keep the cron time for this job in .env and default to 1 hour. Because for different instances based on their load we might want to override when we want to trigger this job. For example for the SaaS instance we would like to run it once in a day. But for OQSHA instance we might want to run it every 1 hour or 2 hour.
So lets try to keep this customization. Can be done as part of different task.
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts
Outdated
Show resolved
Hide resolved
| public async down(queryRunner: QueryRunner): Promise<void> { | ||
| // Copy the entries in archived_notifications back to notify_notifications | ||
| await queryRunner.query(` | ||
| INSERT INTO notify_notifications ( | ||
| id, channel_type, data, delivery_status, result, created_on, updated_on, | ||
| created_by, updated_by, status, application_id, provider_id, retry_count | ||
| ) SELECT | ||
| notification_id, channel_type, data, delivery_status, result, created_on, updated_on, | ||
| created_by, updated_by, status, application_id, provider_id, retry_count | ||
| FROM archived_notifications | ||
| `); | ||
|
|
||
| // Drop the auto generated foreign key for archived_notifications | ||
| const archived_notifications_table = await queryRunner.getTable('archived_notifications'); | ||
| const archived_notifications_providerIdforeignKey = | ||
| archived_notifications_table?.foreignKeys.find( | ||
| (fk) => fk.columnNames.indexOf('provider_id') !== -1, | ||
| ); | ||
|
|
||
| // providerIdforeignKey | ||
| if (archived_notifications_providerIdforeignKey) { | ||
| await queryRunner.dropForeignKey( | ||
| 'archived_notifications', | ||
| archived_notifications_providerIdforeignKey, | ||
| ); | ||
| } | ||
|
|
||
| // channelTypeforeignKey | ||
| const archived_notifications_channelTypeforeignKey = | ||
| archived_notifications_table?.foreignKeys.find( | ||
| (fk) => fk.columnNames.indexOf('channel_type') !== -1, | ||
| ); | ||
|
|
||
| if (archived_notifications_channelTypeforeignKey) { | ||
| await queryRunner.dropForeignKey( | ||
| 'archived_notifications', | ||
| archived_notifications_channelTypeforeignKey, | ||
| ); | ||
| } | ||
|
|
||
| // Drop table archived_notifications | ||
| await queryRunner.dropTable('archived_notifications'); | ||
|
|
||
| // Add foreign key again for notify_notification_retries | ||
| await queryRunner.createForeignKey( | ||
| 'notify_notification_retries', | ||
| new TableForeignKey({ | ||
| columnNames: ['notification_id'], | ||
| referencedColumnNames: ['id'], | ||
| referencedTableName: 'notify_notifications', | ||
| onDelete: 'CASCADE', | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Good suggestion I think. Why don't we consider this? Always give reason when you are not considering any coderabbit suggestion.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apps/api/src/modules/archived-notifications/archived-notifications.service.ts (2)
37-39: Remove extraneous curly brace in log messageThere is an extra closing curly brace
}at the end of the log message, which may cause confusion when reading logs.Apply this diff to fix the issue:
- `Preparing ArchivedNotification entry: ${JSON.stringify(archivedNotification, null, 2)}}`, + `Preparing ArchivedNotification entry: ${JSON.stringify(archivedNotification, null, 2)}`,
54-78: Optimize archiving process using bulk database operationsCurrently, the archiving process loads notifications into application memory, converts them, and then performs insert and delete operations. This approach may not scale efficiently with large datasets. Consider leveraging database-level operations to perform the archiving in a more performant manner.
Here's how you might modify the code using raw SQL queries within the transaction:
try { // Step 1: Begin transaction await queryRunner.startTransaction(); // Step 2: Archive notifications directly in the database this.logger.log(`Archiving notifications directly in the database`); await queryRunner.manager.query(` INSERT INTO archived_notification (application_id, channel_type, created_by, created_on, data, delivery_status, notification_id, provider_id, result, retry_count, updated_by, updated_on, status) SELECT application_id, channel_type, created_by, created_on, data, delivery_status, id AS notification_id, provider_id, result, retry_count, updated_by, updated_on, status FROM notification WHERE delivery_status IN ('SUCCESS', 'FAILED') ORDER BY created_on ASC LIMIT ${archiveLimit} `); // Step 3: Delete archived notifications from the main table this.logger.log(`Deleting archived notifications from the main table`); await queryRunner.manager.query(` DELETE FROM notification WHERE id IN ( SELECT notification_id FROM archived_notification ORDER BY created_on ASC LIMIT ${archiveLimit} ) `); // Step 4: Commit transaction await queryRunner.commitTransaction(); this.logger.log('Transaction successful'); } catch (error) { // Rollback transaction in case of error await queryRunner.rollbackTransaction(); this.logger.error('Error while archiving notifications:', error); throw error; } finally { // Release the query runner await queryRunner.release(); }Note: Adjust the SQL queries to match your database schema and ensure all necessary fields are correctly handled. This approach minimizes data transfer between the database and the application server, improving performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts(1 hunks)apps/api/src/modules/archived-notifications/archived-notifications.service.ts(1 hunks)apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/modules/archived-notifications/entities/archived-notification.entity.ts
🧰 Additional context used
📓 Learnings (1)
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts (2)
Learnt from: osm-vishnukyatannawar
PR: OsmosysSoftware/osmo-x#354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:25-27
Timestamp: 2024-11-08T05:23:26.338Z
Learning: In the 'archived_notifications' table, we do not add a foreign key constraint on 'notification_id' to 'notify_notifications' because the original notifications may have been deleted, and we want to retain archived notifications even if the originals are removed.
Learnt from: osm-vishnukyatannawar
PR: OsmosysSoftware/osmo-x#354
File: apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts:112-165
Timestamp: 2024-11-08T05:23:36.341Z
Learning: Developers should always provide reasons when they choose not to implement a CodeRabbit suggestion.
🔇 Additional comments (5)
apps/api/src/database/migrations/1730724383210-ArchiveCompletedNotifications.ts (5)
1-3: LGTM!
The imports are appropriate, and the class name follows TypeORM's naming convention.
5-12: LGTM!
The index on delivery_status is well-justified and will improve query performance for the archiving process.
14-30: LGTM!
The foreign key removal is implemented with proper error handling, ensuring the table and foreign key exist before attempting removal.
31-109: LGTM!
The table structure is well-defined with appropriate column types and constraints. The decision to omit varchar length and check constraints aligns with the existing table structures for consistency.
111-148: LGTM!
The foreign key constraints and indexes are appropriately defined to maintain data integrity and optimize query performance.
| public async down(queryRunner: QueryRunner): Promise<void> { | ||
| // Copy the entries in notify_archived_notifications back to notify_notifications | ||
| await queryRunner.query(` | ||
| INSERT INTO notify_notifications ( | ||
| id, channel_type, data, delivery_status, result, created_on, updated_on, | ||
| created_by, updated_by, status, application_id, provider_id, retry_count | ||
| ) SELECT | ||
| notification_id, channel_type, data, delivery_status, result, created_on, updated_on, | ||
| created_by, updated_by, status, application_id, provider_id, retry_count | ||
| FROM notify_archived_notifications | ||
| `); | ||
|
|
||
| // Remove indexes from table notify_archived_notifications | ||
| await queryRunner.dropIndex('notify_archived_notifications', 'IDX_ARCHIVED_NOTIFICATION_ID'); | ||
| await queryRunner.dropIndex('notify_archived_notifications', 'IDX_ARCHIVED_DELIVERY_STATUS'); | ||
|
|
||
| // Drop foreign keys from table notify_archived_notifications | ||
| const notify_archived_notifications_table = await queryRunner.getTable( | ||
| 'notify_archived_notifications', | ||
| ); | ||
| const notify_archived_notifications_providerIdforeignKey = | ||
| notify_archived_notifications_table?.foreignKeys.find( | ||
| (fk) => fk.columnNames.indexOf('provider_id') !== -1, | ||
| ); | ||
|
|
||
| if (notify_archived_notifications_providerIdforeignKey) { | ||
| await queryRunner.dropForeignKey( | ||
| 'notify_archived_notifications', | ||
| notify_archived_notifications_providerIdforeignKey, | ||
| ); | ||
| } | ||
|
|
||
| const notify_archived_notifications_channelTypeforeignKey = | ||
| notify_archived_notifications_table?.foreignKeys.find( | ||
| (fk) => fk.columnNames.indexOf('channel_type') !== -1, | ||
| ); | ||
|
|
||
| if (notify_archived_notifications_channelTypeforeignKey) { | ||
| await queryRunner.dropForeignKey( | ||
| 'notify_archived_notifications', | ||
| notify_archived_notifications_channelTypeforeignKey, | ||
| ); | ||
| } | ||
|
|
||
| // Drop table notify_archived_notifications | ||
| await queryRunner.dropTable('notify_archived_notifications'); | ||
|
|
||
| // Add foreign key again for table notify_notification_retries | ||
| await queryRunner.createForeignKey( | ||
| 'notify_notification_retries', | ||
| new TableForeignKey({ | ||
| columnNames: ['notification_id'], | ||
| referencedColumnNames: ['id'], | ||
| referencedTableName: 'notify_notifications', | ||
| onDelete: 'CASCADE', | ||
| }), | ||
| ); | ||
|
|
||
| // Remove index from notify_notifications table | ||
| await queryRunner.dropIndex('notify_notifications', 'IDX_DELIVERY_STATUS'); | ||
| } |
There was a problem hiding this comment.
Add transaction handling to ensure data consistency.
The down migration performs multiple operations that should be atomic. If any operation fails, the database could be left in an inconsistent state.
Apply this diff to add transaction handling:
public async down(queryRunner: QueryRunner): Promise<void> {
+ await queryRunner.startTransaction();
+
+ try {
// Copy the entries in notify_archived_notifications back to notify_notifications
await queryRunner.query(`
INSERT INTO notify_notifications (
id, channel_type, data, delivery_status, result, created_on, updated_on,
created_by, updated_by, status, application_id, provider_id, retry_count
) SELECT
notification_id, channel_type, data, delivery_status, result, created_on, updated_on,
created_by, updated_by, status, application_id, provider_id, retry_count
FROM notify_archived_notifications
`);
// ... rest of the down migration ...
+ await queryRunner.commitTransaction();
+ } catch (err) {
+ await queryRunner.rollbackTransaction();
+ throw err;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
apps/api/src/modules/archived-notifications/archived-notifications.service.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Cron(CronExpression.EVERY_HOUR) | ||
| async archiveCompletedNotificationsCron(): Promise<void> { |
There was a problem hiding this comment.
@kshitij-k-osmosys
Move this code to scheduler service.
Also since there is issue with Scheduler we were facing, we decided to use the scheduler script.
So you would need similar approach for triggering it.
There was a problem hiding this comment.
Not sure if we can allow users to set cron if it is triggering from scheduler script
There was a problem hiding this comment.
Update: tracking this in #360
|
All the changes done in this PR are ported to and tracked in #360 |
API PR Checklist
Pre-requisites
.env.examplefile with the required values as applicable.PR Details
PR details have been updated as per the given format (see below)
feat: add admin login endpoint)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Note: Reviewer should ensure that the checklist and description have been populated and followed correctly, and the PR should be merged only after resolving all conversations and verifying that CI checks pass.
Description:
Archive SUCCESS/FAILED notifications to
notify_archived_notificationsand delete notifications fromnotify_notificationsto improve database performanceRelated changes:
notify_archived_notificationsfor storing old notificationsnotify_notification_retriesto avoid cascade deletionArchivedNotification.envfileScreenshots:
notify_archived_notificationsnotify_notificationsnotify_notification_retriesQuery request and response:
NA
Documentation changes:
NA
Test suite/unit testing output:
NA
Pending actions:
NA
Additional notes:
NA
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests