fix/fe-be/notifications #1079
fix/fe-be/notifications #1079
Conversation
…geSpeedMonitorSlice
WalkthroughThe changes in this pull request focus on the 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
Server/db/mongo/modules/monitorModule.js (1)
Line range hint
292-307: Yo! Let's beef up that error handling! 🍝The error handling could use some love for notification-specific failures. Also, we should validate the notification data before updating the monitor.
Consider adding specific error handling:
try { const monitor = await Monitor.findById(monitorId); if (monitor === null || monitor === undefined) { const error = new Error(errorMessages.DB_FIND_MONITOR_BY_ID(monitorId)); error.status = 404; throw error; } // Get notifications + let notifications; + try { notifications = await Notification.find({ monitorId: monitorId, }); + } catch (notificationError) { + const error = new Error(`Failed to fetch notifications: ${notificationError.message}`); + error.status = 500; + throw error; + } + + // Validate notifications + if (!Array.isArray(notifications)) { + const error = new Error('Invalid notifications data structure'); + error.status = 500; + throw error; + }
| const updatedMonitor = await Monitor.findByIdAndUpdate( | ||
| monitorId, | ||
| { notifications }, | ||
| { new: true } | ||
| ).populate("notifications"); |
There was a problem hiding this comment.
Yo! The current implementation might still have concurrency issues, dawg! 🍝
The separate find and findByIdAndUpdate operations create a potential race condition. If notifications are modified between these operations, the monitor could end up with stale data.
Consider using MongoDB's aggregation pipeline for atomic operations:
-const notifications = await Notification.find({
- monitorId: monitorId,
-});
-const updatedMonitor = await Monitor.findByIdAndUpdate(
- monitorId,
- { notifications },
- { new: true }
-).populate("notifications");
+const updatedMonitor = await Monitor.aggregate([
+ { $match: { _id: monitorId } },
+ {
+ $lookup: {
+ from: "notifications",
+ localField: "_id",
+ foreignField: "monitorId",
+ as: "notifications"
+ }
+ },
+ { $limit: 1 }
+]).exec();
+
+if (!updatedMonitor[0]) {
+ const error = new Error(errorMessages.DB_FIND_MONITOR_BY_ID(monitorId));
+ error.status = 404;
+ throw error;
+}
+
+return updatedMonitor[0];Alternatively, if you need to stick with the current approach, wrap it in a transaction:
const session = await Monitor.startSession();
session.startTransaction();
try {
const notifications = await Notification.find({ monitorId }).session(session);
const updatedMonitor = await Monitor.findByIdAndUpdate(
monitorId,
{ notifications },
{ new: true, session }
).populate("notifications");
await session.commitTransaction();
return updatedMonitor;
} catch (error) {
await session.abortTransaction();
throw error;
} finally {
session.endSession();
}
There was a concurrency error in way in which notificaitons were being added to monitors on the back end. This PR rectifies that issue.