fix(core): patch storage quota evasion loophole in update pipeline - #224
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds per-project storage quota enforcement to ChangesData Controller Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/public-api/src/controllers/data.controller.js`:
- Around line 535-553: The code path inside the non-external DB block returns
raw JSON errors (res.status(404).json({ error: ... }) and res.status(403).json({
error: ... })) which breaks the controller contract; replace those returns with
the standard error handling using the AppError class and the controller's error
middleware (e.g., import AppError and call next(new AppError("Document not
found.", 404)) for the 404 and next(new AppError("Storage quota exceeded. Please
upgrade your plan.", 403)) for the quota check) so errors follow the { success,
data, message } response shape and no raw DB details are leaked; update
references in this block (the existingDoc check and the quota branch that uses
sizeDelta / project.databaseUsed/project.databaseLimit) accordingly.
- Around line 532-571: The current flow does findOne → local quota check →
findOneAndUpdate → separate Project.findByIdAndUpdate which allows race
conditions and never decrements storage for shrink updates; fix by performing
the document read, quota check, document update and Project.$inc inside a single
MongoDB transaction/session so the quota check and the $inc are atomic: use
mongoose.startSession(), session.startTransaction(), perform
Model.findOne({queryFilter}).session(session) to get existingDoc (abort if
null), compute sizeDelta = newSize - oldSize, if sizeDelta>0 verify
(project.databaseUsed || 0) + sizeDelta <= project.databaseLimit else allow
negative deltas, then run Model.findOneAndUpdate(queryFilter, { $set:
sanitizedData }, { new: true, runValidators: true, session }) and
Project.findByIdAndUpdate(project._id, { $inc: { databaseUsed: sizeDelta } }, {
session }), commitTransaction on success and abort/rollback and return
appropriate error on failure; ensure you always apply the computed sizeDelta
(can be negative) so shrinks free quota and avoid separate non-atomic updates
via Project.findByIdAndUpdate outside a transaction.
- Around line 288-299: The code clamps limit only in responseMeta but still uses
the raw parsed value when paginating; compute a single clampedLimit (e.g. const
limit = Math.max(1, Math.min(parseInt(req.query.limit, 10) || 100, 100))) before
any pagination logic and use that variable wherever slicing/pagination occurs
(cursor handling, nextCursor calculation, or array.slice calls) and also
reference the same limit variable in responseMeta; update any places using the
raw parsed value so nextCursor and returned metadata are consistent.
- Around line 200-206: In bulkInsertData (and the related bulk insert error
branches) the AppError constructor is being called with the arguments reversed;
AppError expects (statusCode, message). Update each call in data.controller.js
(notably the branches using isDuplicateKeyError and the final bulk insert
failure path) to call new AppError with the status code first and the error
message second (e.g., new AppError(409, "Duplicate value violates unique
constraint.") and new AppError(500, "Failed to insert bulk data")). Ensure all
occurrences in bulkInsertData use this corrected argument order.
🪄 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: 4c63cc70-0213-4c62-8850-291eb63b2fba
📒 Files selected for processing (1)
apps/public-api/src/controllers/data.controller.js
|
Hi @yash-pouranik, I just pushed the updates to resolve the CodeRabbit review (implemented the MongoDB atomic transaction for the quota check and fixed the existing AppError argument bugs). It looks like the CodeRabbit bot hit its hourly rate limit so it couldn't generate the final summary, but the code is fully updated and ready for your review whenever you have time! |
|
Hi @yash-pouranik, just a gentle ping on this one whenever you have a free moment! The CodeRabbit checks are all green and the atomic transaction logic is good to go. Absolutely no rush at all on my end just wanted to keep it on your radar. Thank You!! |
|
really sry, will surelly try to merge it, this night, else tomorrow eve. |
yash-pouranik
left a comment
yash-pouranik
left a comment
There was a problem hiding this comment.
just Answer one thing, else is good
yash-pouranik
added
GSSOC'26
gssoc:approved
level:advanced
quality:exceptional
type:performance
enhancement
|
@ShreyasPatil3105 |
2 participants
Pull Request Description
Fixes #216
Summary of Changes:
Implemented the
BSON.calculateObjectSize()validation step within theupdateSingleDatapipeline indata.controller.js. By calculating the size delta between the existing document and the incoming payload, the system now enforces thedatabaseLimitstorage quota on document updates prior to database persistence, resolving the identified quota evasion loophole. Additionally, integrated a non-blocking background update tracking loop for thedatabaseUsedmetric.Type of Change
Testing & Validation
Backend Verification:
npm testin thebackend/directory and all tests passed.Frontend Verification:
npm run lintin thefrontend/directory.Checklist
Summary by CodeRabbit
New Features
Bug Fixes