Add unit tests for storage.controller.js#72
Conversation
|
@ayash911 is attempting to deploy a commit to the Yash Pouranik's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a comprehensive Jest test suite for storage controller handlers, introduces a path-traversal check in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive test suite for the storage controller, covering file uploads, deletions, and bulk deletions for both internal and external storage. The feedback suggests enhancing the tests by adding cases for path traversal and removal failures, and refining assertions for the storage list method to verify that correct project IDs and search parameters are used.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/public-api/src/__tests__/storage.controller.test.js (1)
257-307: Consider adding tests for external storage and remove failure indeleteAllFiles.The current test suite covers pagination and list failure, but two scenarios are untested:
External storage behavior: Verify that
Project.updateOneis not called whenisProjectStorageExternalreturnstrue(similar to thedeleteFileexternal test).Remove failure during bulk deletion: The controller calls
removeinside the pagination loop and throws onremoveError. A test asserting a 500 response would improve robustness.📋 Suggested test cases
+ test('returns 200 on external bulk deletion (skips storageUsed reset)', async () => { + isProjectStorageExternal.mockReturnValue(true); + mockStorageFrom.list + .mockResolvedValueOnce({ data: [{ name: 'file1.txt' }], error: null }) + .mockResolvedValueOnce({ data: [], error: null }); + mockStorageFrom.remove.mockResolvedValue({ data: [], error: null }); + + const req = { project: makeProject() }; + const res = makeRes(); + + await storageController.deleteAllFiles(req, res); + + expect(Project.updateOne).not.toHaveBeenCalled(); + expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ provider: 'external' })); + }); + + test('returns 500 when remove fails during bulk deletion', async () => { + isProjectStorageExternal.mockReturnValue(false); + mockStorageFrom.list.mockResolvedValue({ data: [{ name: 'file1.txt' }], error: null }); + mockStorageFrom.remove.mockResolvedValue({ data: null, error: new Error('Remove failed') }); + + const req = { project: makeProject() }; + const res = makeRes(); + + await storageController.deleteAllFiles(req, res); + + expect(res.status).toHaveBeenCalledWith(500); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public-api/src/__tests__/storage.controller.test.js` around lines 257 - 307, Add two tests for storageController.deleteAllFiles: (1) simulate isProjectStorageExternal.mockReturnValue(true) and assert that Project.updateOne is NOT called and the response indicates provider: 'external' (use makeProject(), makeRes(), and mockStorageFrom.list/remove as needed); (2) simulate a remove failure by having mockStorageFrom.list return items but mockStorageFrom.remove.mockResolvedValue({ data: null, error: new Error('Remove failed') }) and assert the controller responds with status 500 and an error JSON (verify mockStorageFrom.remove is called with the expected paths and that res.status/res.json reflect the failure).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/public-api/src/__tests__/storage.controller.test.js`:
- Around line 257-307: Add two tests for storageController.deleteAllFiles: (1)
simulate isProjectStorageExternal.mockReturnValue(true) and assert that
Project.updateOne is NOT called and the response indicates provider: 'external'
(use makeProject(), makeRes(), and mockStorageFrom.list/remove as needed); (2)
simulate a remove failure by having mockStorageFrom.list return items but
mockStorageFrom.remove.mockResolvedValue({ data: null, error: new Error('Remove
failed') }) and assert the controller responds with status 500 and an error JSON
(verify mockStorageFrom.remove is called with the expected paths and that
res.status/res.json reflect the failure).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e76247d-865c-4984-87aa-0bf52237b1af
📒 Files selected for processing (1)
apps/public-api/src/__tests__/storage.controller.test.js
…nd cover missing fail paths
|
@coderabbitai OK to merge? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13931 🏁 Script executed: Length of output: 26362
✅ What looks good:
If jest is confirmed available at the workspace root and you're comfortable with the one missing test being a follow-up, this is ready to merge. 🚀 |
|
@ayash911 solve this 2 things then I will review my self and then merge. |
|
@CodeRabbit all done?? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14628
❌ Item 1 — // current state — jest is absent from devDependencies
{
"scripts": { "test": "jest --testPathPatterns=src/" },
"jest": { "testEnvironment": "node" },
"dependencies": { ... }
// no devDependencies block at all
}
"devDependencies": {
"jest": "^29.7.0"
}❌ Item 2 — Missing test('skips storageUsed reset for external provider during deleteAllFiles', async () => {
isProjectStorageExternal.mockReturnValue(true);
mockStorageFrom.list
.mockResolvedValueOnce({ data: [{ name: 'file1.txt' }], error: null })
.mockResolvedValueOnce({ data: [], error: null });
mockStorageFrom.remove.mockResolvedValue({ data: [{}], error: null });
const req = { project: makeProject() };
const res = makeRes();
await storageController.deleteAllFiles(req, res);
expect(Project.updateOne).not.toHaveBeenCalled();
expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ provider: 'external' }));
});
Once |
ayash911
force-pushed
the
feature/storage-tests
branch
from
23d86b5 to
4dc7218
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/public-api/src/__tests__/storage.controller.test.js`:
- Around line 267-331: Add a test that pins the external-storage branch by
mocking isProjectStorageExternal(project) to return true, then have
mockStorageFrom.list return some files and mockStorageFrom.remove resolve
successfully; call storageController.deleteAllFiles(req, res) and assert that
mockStorageFrom.remove was called with the external file paths,
Project.updateOne was NOT called, and the response JSON contains success:true,
deleted:<n>, provider:'external' to prevent regressions in the deleteAllFiles
external branch (referencing storageController.deleteAllFiles,
isProjectStorageExternal, mockStorageFrom.list, mockStorageFrom.remove, and
Project.updateOne).
- Around line 1-40: The test runner 'jest' invoked by the test script ("jest
--testPathPatterns=src/") is not declared as a dependency for the public-api
package, which breaks isolated installs and CI; fix by adding 'jest' (and any
needed types like '@types/jest' if using TS) to the public-api package.json
devDependencies so the test script can run, or alternatively update the
repository documentation and CI config to explicitly state that Jest must always
be provided from the monorepo root; update the package.json that contains the
test script and ensure package-lock/yarn.lock is updated and CI uses the
declared dependency.
In `@apps/public-api/src/controllers/storage.controller.js`:
- Around line 102-103: The delete handler currently blocks any path containing
'..' but uploadFile still allows filenames like report..csv (it only trims
whitespace from file.originalname), causing uploads to succeed but deletes to
403; either sanitize/validate in uploadFile by rejecting or normalizing
file.originalname when building filePath (ensure names containing '..' are
rejected or escaped) or make the delete-time check stricter by normalizing the
incoming path (e.g., path.normalize) and checking path segments for a literal
'..' segment or ensuring the normalized path starts with `${project._id}/`;
update references in the uploadFile logic (file.originalname and filePath) or
the delete validation (path, project._id) accordingly so allowed filenames like
report..csv that are not path-traversal are not rejected while true traversal is
blocked.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 615e7826-e612-4c9f-b54d-77793d7f24be
📒 Files selected for processing (3)
apps/public-api/src/__tests__/storage.controller.test.jsapps/public-api/src/controllers/data.controller.jsapps/public-api/src/controllers/storage.controller.js
|
@ayash911 some more changes 🤏 |
- Add jest to devDependencies in public-api package.json - Add test for deleteAllFiles external provider path skipping quota reset - Narrow down path traversal check in deleteFile from path.includes to checking exact path segments
|
Finally OK to merge @coderabbitai ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3790
The PR is ready to merge. 🚀 |
|
@ayash911 CI is failing? @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5406 🏁 Script executed: Length of output: 39983
When Fix for # From the repo root
npm install
git add package-lock.json
git commit -m "chore: update package-lock.json after adding jest devDependency"
git push
|
|
@ayash911 follow what coderabbit says
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/public-api/package.json`:
- Around line 14-16: Remove the duplicated "devDependencies" block that contains
"jest": "^29.7.0" and keep only the existing "devDependencies" block that
currently contains "jest": "^30.3.0"; then update that remaining "jest" entry in
the kept devDependencies to the intended version "^29.7.0" so the file has a
single devDependencies object with "jest": "^29.7.0".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5313176-c653-4449-91c9-26f81b658a62
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
apps/public-api/package.json
| "devDependencies": { | ||
| "jest": "^29.7.0" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which Jest versions exist on npm
echo "=== Checking if jest@30.3.0 exists ==="
npm view jest@30.3.0 version 2>&1 || echo "Version 30.3.0 does not exist"
echo ""
echo "=== Latest Jest version ==="
npm view jest version
echo ""
echo "=== Available Jest 29.x versions ==="
npm view jest versions --json | jq '.[] | select(startswith("29"))' | tail -5
echo ""
echo "=== Available Jest 30.x versions (if any) ==="
npm view jest versions --json | jq '.[] | select(startswith("30"))' || echo "No 30.x versions found"Repository: yash-pouranik/urBackend
Length of output: 647
Duplicate devDependencies blocks will use the wrong Jest version.
The file contains two devDependencies blocks:
- Lines 14-16:
"jest": "^29.7.0"(newly added) - Lines 36-38:
"jest": "^30.3.0"(pre-existing)
JSON parsers use the last occurrence, so npm will resolve to jest@30.3.0 and ignore the intended ^29.7.0. This contradicts the PR objective.
Remove the first devDependencies block (lines 14-16) to keep only the second one, then update it to use the intended version:
"jest": {
"testEnvironment": "node"
},
- "devDependencies": {
- "jest": "^29.7.0"
- },
"dependencies": {
"@kiroo/sdk": "^0.1.2"
},
"devDependencies": {
- "jest": "^30.3.0"
+ "jest": "^29.7.0"
}
}🧰 Tools
🪛 Biome (2.4.9)
[error] 14-14: The key devDependencies was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/public-api/package.json` around lines 14 - 16, Remove the duplicated
"devDependencies" block that contains "jest": "^29.7.0" and keep only the
existing "devDependencies" block that currently contains "jest": "^30.3.0"; then
update that remaining "jest" entry in the kept devDependencies to the intended
version "^29.7.0" so the file has a single devDependencies object with "jest":
"^29.7.0".
|
Finally Merged! @ayash911 , thank you for the PR and dont forget to star the REPO |
2 participants
🚀 Pull Request Description
Addresses the lack of test coverage for the public-api storage controller.
Added a comprehensive Jest test suite (apps/public-api/src/tests/storage.controller.test.js) covering:
Also added
jestas a devDependency to thepublic-apiworkspace.🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
npm testin thebackend/directory and all tests passed (Verified withnpx jest src/__tests__/storage.controller.test.js).Frontend Verification:
npm run lintin thefrontend/directory.📸 Screenshots / Recordings (Optional)
Jest Test Output:
PASS src/__tests__/storage.controller.test.js storage.controller uploadFile ✓ returns 400 when no file is uploaded ✓ returns 413 when file exceeds MAX_FILE_SIZE ✓ returns 403 when internal storage quota is exceeded ✓ returns 201 and public URL on successful internal upload ✓ returns 201 and public URL on successful external upload (skips quota) ✓ rolls back quota and returns 500 when upload fails deleteFile ✓ returns 400 when file path is missing ✓ returns 403 when trying to delete file belonging to different project ✓ returns 200 on successful internal deletion ✓ returns 200 on successful external deletion (skips internal usage list) ✓ returns 500 when Supabase list fails ✓ returns 500 when Supabase remove fails deleteAllFiles ✓ returns 404 when project is not found ✓ deletes paginated files and resets internal storage to 0 ✓ returns 500 if pagination fetch fails Test Suites: 1 passed, 1 total Tests: 15 passed, 15 total✅ Checklist
Built with ❤️ for urBackend.
Summary by CodeRabbit
Bug Fixes
Tests