Refactor request validation and parsing using utility functions#168
Refactor request validation and parsing using utility functions#168Shweta-281 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR improves request validation and JSON parsing across backend functions by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (2)
functions/create-room/src/utils.js (1)
25-25: Minor: missing semicolon for consistency.Other utils.js files end
parseBodywith a semicolon. Add one here for consistency.-} +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/create-room/src/utils.js` at line 25, The file ends the parseBody function without a trailing semicolon; update the closing brace of the parseBody function in functions/create-room/src/utils.js (the parseBody function) to include a semicolon for consistency with other utils.js files.functions/delete-room/src/utils.js (1)
1-25: Code duplication: identical utilities copied across 4 files.The
throwIfMissingandparseBodyfunctions are duplicated verbatim in:
functions/delete-room/src/utils.jsfunctions/join-room/src/utils.jsfunctions/database-cleaner/src/utils.jsfunctions/create-room/src/utils.jsConsider extracting these to a shared module (e.g.,
functions/shared/utils.jsor a workspace package) to avoid maintaining identical code in multiple places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/delete-room/src/utils.js` around lines 1 - 25, The utilities throwIfMissing and parseBody are duplicated across four function packages; extract them into a single shared module (e.g., functions/shared/utils.js or a workspace package) and replace the local copies with imports from that shared module in delete-room, join-room, database-cleaner, and create-room; ensure the shared file exports throwIfMissing and parseBody with identical behavior and update all imports/usages (functions named throwIfMissing and parseBody) so tests/build use the single source of truth and remove the duplicate files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@functions/database-cleaner/src/main.js`:
- Around line 29-34: cleanParticipantsCollection currently uses
participantDocs.documents.forEach with async callbacks so individual deletion
errors don't propagate and the try/catch around its caller won't catch failures;
refactor the async forEach into a Promise array (map participantDocs.documents
to deletion promises) and await Promise.all on that array inside
cleanParticipantsCollection (similar to the other cleanup methods), and only
resolve/return after all deletions complete so the caller's try/catch and the
success log in the caller reflect actual outcome.
- Around line 52-55: Change the unconditional success response to return an
aggregated result: during each cleanup phase push any caught error messages into
an errors array (instead of only logging), compute success = errors.length ===
0, and then call res.json with success, a message that reflects full/partial
success (e.g., "Database cleanup completed" vs "Completed with errors"), and
include the errors array (or error count) alongside the existing "expired OTPs
cleared" info; update the code path that currently calls res.json({ success:
true, message: "Database cleanup completed and expired OTPs cleared" }) to use
these aggregated values.
In `@functions/database-cleaner/src/utils.js`:
- Around line 15-25: Remove the unused parseBody export from utils.js: locate
the parseBody function declaration and export (function name parseBody) and
delete the entire function block and its export so it's no longer part of the
module's public API; ensure no other code references parseBody (remove any
leftover imports/usages if present) and run tests/lint to confirm no missing
symbols remain.
---
Nitpick comments:
In `@functions/create-room/src/utils.js`:
- Line 25: The file ends the parseBody function without a trailing semicolon;
update the closing brace of the parseBody function in
functions/create-room/src/utils.js (the parseBody function) to include a
semicolon for consistency with other utils.js files.
In `@functions/delete-room/src/utils.js`:
- Around line 1-25: The utilities throwIfMissing and parseBody are duplicated
across four function packages; extract them into a single shared module (e.g.,
functions/shared/utils.js or a workspace package) and replace the local copies
with imports from that shared module in delete-room, join-room,
database-cleaner, and create-room; ensure the shared file exports throwIfMissing
and parseBody with identical behavior and update all imports/usages (functions
named throwIfMissing and parseBody) so tests/build use the single source of
truth and remove the duplicate files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad19a469-ad8e-4ad3-8e74-c6e76df1124d
📒 Files selected for processing (7)
functions/create-room/src/main.jsfunctions/create-room/src/utils.jsfunctions/database-cleaner/src/main.jsfunctions/database-cleaner/src/utils.jsfunctions/delete-room/src/main.jsfunctions/delete-room/src/utils.jsfunctions/join-room/src/utils.js
| try { | ||
| await appwrite.cleanParticipantsCollection(); | ||
| log("[CLEANUP] Participants collection cleaned"); | ||
| } catch (e) { | ||
| context.error(String(e)); | ||
| error("[CLEANUP_ERROR] Participants: " + String(e)); | ||
| } |
There was a problem hiding this comment.
Try/catch ineffective for cleanParticipantsCollection errors.
Per the relevant code snippet at functions/database-cleaner/src/appwrite.js:31-46, cleanParticipantsCollection uses forEach with async callbacks:
participantDocs.documents.forEach(async (participantDoc) => {
// errors here won't propagate to the caller
});Errors from individual document deletions won't propagate to this try/catch, and the success log on line 31 will print even if deletions fail silently. Consider refactoring cleanParticipantsCollection to use Promise.all like the other cleanup methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/database-cleaner/src/main.js` around lines 29 - 34,
cleanParticipantsCollection currently uses participantDocs.documents.forEach
with async callbacks so individual deletion errors don't propagate and the
try/catch around its caller won't catch failures; refactor the async forEach
into a Promise array (map participantDocs.documents to deletion promises) and
await Promise.all on that array inside cleanParticipantsCollection (similar to
the other cleanup methods), and only resolve/return after all deletions complete
so the caller's try/catch and the success log in the caller reflect actual
outcome.
| return res.json({ | ||
| success: true, | ||
| message: "Database cleanup completed and expired OTPs cleared", | ||
| }); |
There was a problem hiding this comment.
Success response returned even when individual cleanup steps fail.
The function returns success: true unconditionally, even if one or more cleanup phases encountered errors (logged but swallowed). Consider tracking failures and returning a partial success indicator or including error details in the response.
💡 Suggested approach
+ const errors = [];
+
// Cleanup Participants
try {
await appwrite.cleanParticipantsCollection();
log("[CLEANUP] Participants collection cleaned");
} catch (e) {
error("[CLEANUP_ERROR] Participants: " + String(e));
+ errors.push("Participants");
}
// ... similar for other phases
return res.json({
- success: true,
+ success: errors.length === 0,
message: "Database cleanup completed and expired OTPs cleared",
+ ...(errors.length > 0 && { failedPhases: errors }),
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/database-cleaner/src/main.js` around lines 52 - 55, Change the
unconditional success response to return an aggregated result: during each
cleanup phase push any caught error messages into an errors array (instead of
only logging), compute success = errors.length === 0, and then call res.json
with success, a message that reflects full/partial success (e.g., "Database
cleanup completed" vs "Completed with errors"), and include the errors array (or
error count) alongside the existing "expired OTPs cleared" info; update the code
path that currently calls res.json({ success: true, message: "Database cleanup
completed and expired OTPs cleared" }) to use these aggregated values.
| export const parseBody = (body) => { | ||
| if (!body) { | ||
| throw new Error("Request body is empty"); | ||
| } | ||
|
|
||
| try { | ||
| return JSON.parse(body); | ||
| } catch (err) { | ||
| throw new Error("Invalid JSON body"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if parseBody is imported/used in database-cleaner/main.js
rg -n "parseBody" functions/database-cleaner/src/Repository: AOSSIE-Org/Resonate-Backend
Length of output: 150
🏁 Script executed:
# Check imports and usage of parseBody in database-cleaner main.js
rg -n "parseBody\|import.*parseBody" functions/database-cleaner/src/main.jsRepository: AOSSIE-Org/Resonate-Backend
Length of output: 53
🏁 Script executed:
# Check the main.js file to understand what the database-cleaner function does
cat -n functions/database-cleaner/src/main.jsRepository: AOSSIE-Org/Resonate-Backend
Length of output: 1991
🏁 Script executed:
# Verify if parseBody is called anywhere within utils.js itself
rg -n "parseBody(" functions/database-cleaner/src/utils.jsRepository: AOSSIE-Org/Resonate-Backend
Length of output: 141
🏁 Script executed:
# Check if parseBody is called within utils.js (fixing regex)
rg -n "parseBody\s*\(" functions/database-cleaner/src/utils.jsRepository: AOSSIE-Org/Resonate-Backend
Length of output: 53
🏁 Script executed:
# Also check the full utils.js to see what functions are defined and exported
cat -n functions/database-cleaner/src/utils.jsRepository: AOSSIE-Org/Resonate-Backend
Length of output: 1110
Remove unused parseBody function.
The parseBody export at lines 15-25 is never imported or used in this module. Since database-cleaner is a scheduled cleanup function that doesn't parse HTTP request bodies, this function is dead code and should be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/database-cleaner/src/utils.js` around lines 15 - 25, Remove the
unused parseBody export from utils.js: locate the parseBody function declaration
and export (function name parseBody) and delete the entire function block and
its export so it's no longer part of the module's public API; ensure no other
code references parseBody (remove any leftover imports/usages if present) and
run tests/lint to confirm no missing symbols remain.
|
Hi 👋 @M4dhav I’ve worked on improving request validation and JSON parsing across backend functions. Key Improvements:
These changes aim to improve backend reliability and prevent runtime errors due to invalid input. Would love your feedback and suggestions 🙌 |
1 participant
Addressed Issues:
Fixes #167
✨ Changes
throwIfMissingutility to correctly handle falsy valuesparseBodyutility for safe JSON parsingcreate-roomfunction to:🧩 Problem Solved
🚀 Impact
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
Bug Fixes
Improvements
success/messagefields).