Conversation
tarinagarwal
left a comment
tarinagarwal
left a comment
There was a problem hiding this comment.
Great work on the Labs CRUD implementation! The core functionality is solid. A few improvements needed:
1. Use existing Prisma instance
Replace const prisma = new PrismaClient(); with import prisma from "../db.js"; to use the existing connection.
2. Add input validation
- Validate required fields (title, language, difficulty, etc.)
- Validate enum values for difficulty ("beginner", "intermediate", "advanced")
- Validate visibility ("private", "public", "link")
3. Add filters to GET /api/labs
Add query parameter support for filtering by language, difficulty, creator, etc.
4. Minor fixes
- Add newline at end of schema.prisma file
- Add basic validation for empty/invalid ObjectIds
5. Error handling
Add validation for invalid ObjectId format in route parameters.
The MongoDB adaptations and ObjectId usage are perfect for this setup. Core CRUD logic and auth checks look excellent!
|
Hello, @tarinagarwal |
|
Hello @tarinagarwal please review the changes |
|
@Priyamanjare54 going thru it |
|
Hello any changes required? |
![tarin-lgtm[bot]](https://avatars.githubusercontent.com/in/3092461?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes Requested 🐈
This PR implements full CRUD functionality for Labs with JWT authentication and authorization. Review found critical performance issue due to missing pagination on GET /api/labs, multiple high-severity documentation gaps, and security concerns on unauthenticated GET endpoints. Additional improvements in validation, error handling, and code structure are recommended.
There are a few things I'd like to see addressed before we merge this:
Before merging
- Add pagination and limit to GET /api/labs endpoint to prevent performance degradation.
- Provide comprehensive API documentation for all Labs routes including methods, parameters, responses, authentication, and error handling.
- Implement authentication and authorization checks on GET endpoints and sanitize user inputs to mitigate security risks.
Findings breakdown (33 total)
1 critical / 6 high / 8 medium / 11 low / 7 info
Confidence: 95%
🔗 View Full Review Report — detailed findings, severity breakdown, and agent analysis
Reviewed by Looks Good To Meow — AI-powered code review
💬 You can interact with me directly in this PR:
@tarin-lgtm fix [any constraints]@tarin-lgtm explain [your question]@tarin-lgtm improve [focus area]@tarin-lgtm test [what to focus on]
| }); | ||
|
|
||
| // --- GET ALL Labs --- | ||
| router.get("/", async (req, res) => { |
There was a problem hiding this comment.
🚨 Critical — The GET /api/labs endpoint fetches all labs without any pagination or limit, potentially returning a very large dataset which can degrade performance and increase memory usage.
Implement pagination by accepting page and limit query parameters and use Prisma's skip and take options to limit the number of labs returned per request.
performance
| @@ -0,0 +1,199 @@ | |||
| import express from "express"; | |||
There was a problem hiding this comment.
Add comprehensive JSDoc or API documentation comments above each route handler describing the endpoint's purpose, HTTP method, URL path, expected request parameters and body schema, response format, authentication requirements, and possible error responses.
documentation
| const ALLOWED_VISIBILITY = ["private", "public", "link"]; | ||
|
|
||
| // --- CREATE Lab --- | ||
| router.post("/", authenticateToken, async (req, res) => { |
There was a problem hiding this comment.
Add JSDoc or API documentation for this endpoint specifying it is a POST request to create a lab, detailing required and optional fields in the request body, authentication via JWT, the structure of the success response, and possible error responses with status codes.
documentation
| }); | ||
|
|
||
| // --- GET ALL Labs --- | ||
| router.get("/", async (req, res) => { |
There was a problem hiding this comment.
Add documentation describing this GET endpoint, including optional query parameters (language, difficulty, creator), the structure of the returned labs array, and error responses.
documentation
| }); | ||
|
|
||
| // --- GET SINGLE Lab --- | ||
| router.get("/:id", async (req, res) => { |
There was a problem hiding this comment.
Add documentation specifying this GET endpoint accepts a lab ID as a path parameter, returns the lab object with related data, and possible error responses for invalid ID or not found.
documentation
| return res.status(400).json({ error: "Invalid visibility value" }); | ||
| } | ||
|
|
||
| const lab = await prisma.lab.update({ |
There was a problem hiding this comment.
💡 Suggestion — The PUT /:id route does not handle the case where no fields are provided for update, which may cause Prisma to update with undefined values.
Filter out undefined fields from the update data before passing to Prisma to avoid overwriting fields with undefined.
best-practices
|
|
||
| if (existing.creatorId !== req.user.id) return res.status(403).json({ error: "Forbidden" }); | ||
|
|
||
| await prisma.lab.delete({ where: { id } }); |
There was a problem hiding this comment.
💡 Suggestion — The DELETE /:id route returns a JSON message on success but does not set an explicit HTTP status code. The default 200 is acceptable but 204 No Content is more conventional for successful deletes.
Consider returning status 204 No Content with no body for successful DELETE requests to align with REST conventions.
best-practices
| }); | ||
|
|
||
| // --- UPDATE Lab --- | ||
| router.put("/:id", authenticateToken, async (req, res) => { |
There was a problem hiding this comment.
💡 Suggestion — The PUT /api/labs/:id endpoint updates lab fields without sanitizing or validating string inputs such as title, description, content, tasks, testCases, and solution. This could lead to stored XSS if these fields are rendered in a client without proper escaping.
Implement input sanitization or escaping for user-supplied string fields before storing them in the database. Additionally, consider validating the structure and content of complex fields like tasks and testCases.
security
| visibility, | ||
| } = req.body; | ||
|
|
||
| if (difficulty && !ALLOWED_DIFFICULTIES.includes(difficulty)) { |
There was a problem hiding this comment.
💡 Suggestion — In the UPDATE Lab route, the difficulty validation returns a generic error without listing allowed values, unlike the CREATE Lab route which provides allowed values for better client feedback.
Include the allowedValues array in the error response for consistency and better client-side validation feedback.
bugs
| return res.status(400).json({ error: "Invalid difficulty value" }); | ||
| } | ||
|
|
||
| if (visibility && !ALLOWED_VISIBILITY.includes(visibility)) { |
There was a problem hiding this comment.
💡 Suggestion — In the UPDATE Lab route, the visibility validation returns a generic error without listing allowed values, unlike the CREATE Lab route which provides allowed values for better client feedback.
Include the allowedValues array in the error response for consistency and better client-side validation feedback.
bugs
2 participants
📝 Description
Implemented full Labs CRUD functionality with authentication and authorization.Fixes #58
Key highlights:
This completes the Labs module backend functionality and prepares it for frontend integration.
🔗 Related Issue
Closes: N/A
🏷️ Type of Change
📸 Screenshots (if applicable)
N/A (Backend-only changes)
✅ Checklist
🧪 Testing
How I tested:
Generated JWT token via login
Tested protected endpoints using Authorization header
Verified ownership checks for update/delete
Confirmed Prisma works with MongoDB replica set
Tested API endpoints (Postman)
Tested on Chrome
Tested on Firefox
Tested on mobile
📋 Additional Notes
201,403,404, etc.).SWOC 2026 Participant ✅
Please add the
swoc2026label to this PR 🎉