fix(injectModel): fix pre-existing index rollback and required-field duplicate precheck#75
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…duplicate precheck Agent-Logs-Url: https://github.com/yash-pouranik/urBackend/sessions/a4ebb02d-93e3-4686-9938-d19a6344dc1a Co-authored-by: yash-pouranik <172860064+yash-pouranik@users.noreply.github.com>
yash-pouranik
temporarily deployed
to
copilot/fix-pre-existing-index-rollback - urBackend-frankfrut PR #75
Render
Destroyed
There was a problem hiding this comment.
Pull request overview
This PR fixes two edge cases in the dynamic model injection utilities to prevent unsafe index rollback and improve duplicate detection before creating MongoDB unique indexes.
Changes:
- Make unique-index rollback safe by tracking only indexes newly created during the current
createUniqueIndexesrun. - Adjust required-field duplicate precheck to include documents missing the field (aligning with MongoDB unique-index behavior for missing values).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| throw new Error( | ||
| `Cannot create unique index on '${normalizedKey}'. ${duplicates.length} duplicate values exist.${examples ? ` Examples: ${examples}` : ""}`, | ||
| const existingIndexes = await Model.collection.indexes(); |
There was a problem hiding this comment.
Model.collection.indexes() will throw a NamespaceNotFound (code 26 / "ns not found") error when the collection doesn’t exist yet. createUniqueIndexes is called immediately after compiling a model for newly-created collections (before any inserts), so this can cause schema/collection creation to fail before any indexes are created. Consider wrapping the indexes() call in a try/catch and treating NamespaceNotFound as an empty index list (or ensuring the collection exists via Model.createCollection() / Model.init() before calling indexes()).
| const existingIndexes = await Model.collection.indexes(); | |
| let existingIndexes = []; | |
| try { | |
| existingIndexes = await Model.collection.indexes(); | |
| } catch (err) { | |
| const isNamespaceNotFound = | |
| err && | |
| (err.code === 26 || | |
| err.codeName === "NamespaceNotFound" || | |
| err.message === "ns not found" || | |
| (typeof err.message === "string" && | |
| err.message.toLowerCase().includes("ns not found"))); | |
| if (!isNamespaceNotFound) { | |
| throw err; | |
| } | |
| } |
|
@coderabbitai merge?? |
|
Tip For best results, initiate chat on the files or code changes.
However, if you'd like me to approve this PR, just say 🧠 Learnings used |
|
Bro @coderabbitai I mean is this PR OK to merge? please review? |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughModified Changes
Possibly related issues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
packages/common/src/utils/injectModel.js (1)
224-228: Consider logging rollback failures for debugging visibility.The current
.catch(() => {})pattern is fine for ensuring cleanup continues, but silently swallowed errors may complicate debugging if rollback partially fails.💡 Optional: Add minimal logging for rollback failures
} catch (err) { for (const indexName of createdIndexes) { - await Model.collection.dropIndex(indexName).catch(() => {}); + await Model.collection.dropIndex(indexName).catch((dropErr) => { + console.warn(`Failed to rollback index ${indexName}:`, dropErr.message); + }); } throw err; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/utils/injectModel.js` around lines 224 - 228, The rollback catch currently swallows dropIndex errors; update the cleanup loop in the catch block that iterates over createdIndexes and calls Model.collection.dropIndex(indexName) so that failures are logged (e.g., include indexName and the error) instead of being silently ignored—use a minimal logger (console.warn or the module's existing logger) inside the .catch handler for Model.collection.dropIndex to record rollback failures while still allowing the loop to continue and rethrow the original err at the end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/common/src/utils/injectModel.js`:
- Around line 224-228: The rollback catch currently swallows dropIndex errors;
update the cleanup loop in the catch block that iterates over createdIndexes and
calls Model.collection.dropIndex(indexName) so that failures are logged (e.g.,
include indexName and the error) instead of being silently ignored—use a minimal
logger (console.warn or the module's existing logger) inside the .catch handler
for Model.collection.dropIndex to record rollback failures while still allowing
the loop to continue and rethrow the original err at the end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4990db98-8d40-4f9c-9499-23643ea5140b
📒 Files selected for processing (1)
packages/common/src/utils/injectModel.js
Two edge-case bugs in
createUniqueIndexesandgetUniqueFieldFilterthat could cause silent data loss and missed duplicate detection on live collections.Changes
createUniqueIndexes): Snapshot existing index names before the loop; only push indexes intocreatedIndexesif they weren't already present. The rollback path now exclusively drops indexes created by the current operation, preventing accidental destruction of pre-existing indexes when a later field fails.getUniqueFieldFilter): For required fields, return{}instead of{ [fieldKey]: { $exists: true } }. MongoDB's unique index treats all missing-field documents as duplicatenulls, so the old filter could pass the precheck whilecreateIndexstill throws error 11000.Summary by CodeRabbit