Conversation
…nclude relationships
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds a Notes feature with file attachments to the NestJS backend, including new REST endpoints, Prisma models, and configuration for local file storage.
Changes:
- Introduces Notes CRUD endpoints/services and DTOs.
- Adds File upload/list/download/update/delete endpoints/services for note attachments.
- Extends Prisma schema with
NoteandFilemodels and wires new modules intoAppModule.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/notes/notes.service.ts | Note data access + authorization checks (currently has DI, correctness, and delete semantics issues). |
| backend/src/notes/notes.controller.ts | Exposes notes endpoints (route design ambiguity to resolve). |
| backend/src/notes/notes.module.ts | Registers notes controller/service. |
| backend/src/files/files.service.ts | Implements local storage persistence + DB metadata (missing-file handling, blocking FS ops, env validation, filename validation). |
| backend/src/files/files.controller.ts | Exposes file endpoints (upload/list/download/metadata update/delete). |
| backend/src/files/files.module.ts | Registers files controller/service. |
| backend/src/dto/notes.dto.ts | Adds Create/Update note DTOs (Update DTO validation currently breaks partial PATCH). |
| backend/src/dto/index.ts | Re-exports notes DTOs. |
| backend/src/app.module.ts | Registers NotesModule and FilesModule. |
| backend/prisma/schema.prisma | Adds Note and File models + relations. |
| backend/package.json | Adds @types/multer for file upload typing. |
| backend/package-lock.json | Locks @types/multer. |
| .env.example | Adds UPLOAD_DIR/MAX_FILE_SIZE example (inline comment format needs adjustment). |
Files not reviewed (1)
- backend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async getNotesBySubject(subjectId: string, userId: string) { | ||
| const notes = await this.prisma.note.findMany({ | ||
| where: { subjectId }, | ||
| }); | ||
| if (!notes) { | ||
| throw new HttpException('Subject not found', 404); | ||
| } |
There was a problem hiding this comment.
findMany returns an empty array when there are no notes, so if (!notes) will never be true. As written, an unknown subjectId will fall through and typically return 403 (access denied) instead of 404 (subject not found). Check that the subject exists (e.g., subject.findUnique) before querying notes / performing the membership check, and remove the ineffective !notes check.
| @ApiResponse({ status: 200, description: 'Return notes by subject ID' }) | ||
| @ApiResponse({ status: 403, description: 'Forbidden' }) | ||
| @ApiResponse({ status: 404, description: 'Subject not found' }) | ||
| @Get('/:subjectId') |
There was a problem hiding this comment.
The NotesController routes use the same path shape for different resource IDs: GET /notes/:subjectId vs PATCH/DELETE /notes/:noteId. This is ambiguous for API consumers and makes the :id segment semantically inconsistent. Consider using a distinct route for listing by subject (e.g., GET /subjects/:subjectId/notes or GET /notes?subjectId=...).
| @Get('/:subjectId') | |
| @Get('/subject/:subjectId') |
| export class UpdateNoteDto { | ||
| @IsString() | ||
| @ApiProperty({ | ||
| description: 'Name of the note', | ||
| example: 'Math Homework 1', | ||
| }) | ||
| name?: string; | ||
| @IsString() | ||
| @ApiProperty({ | ||
| description: 'Content of the note', | ||
| example: | ||
| 'This is the content of the note. JSON format can be used for rich text.', | ||
| }) | ||
| content?: string; |
There was a problem hiding this comment.
UpdateNoteDto marks fields optional with ?, but the validators (@IsString) will still run on undefined values unless @IsOptional() is used. This will cause PATCH requests that update only one field (or none) to fail validation. Add @IsOptional() (and typically @IsNotEmpty() if empty strings aren’t allowed) to optional fields.
| const fileDir = path.join( | ||
| process.cwd(), | ||
| `../${process.env.UPLOAD_DIR}/${note.subjectId}/${noteId}`, | ||
| ); |
There was a problem hiding this comment.
UPLOAD_DIR is used to build storage paths but isn’t validated or defaulted. If it’s missing/misconfigured, files may be written under an unintended ../undefined/... directory. Validate process.env.UPLOAD_DIR at startup (or default it safely) and fail fast if it’s not set.
| async updateFileMetadata(fileId: string, userId: string, name: string) { | ||
| const file = await this.prisma.file.findUnique({ | ||
| where: { id: fileId }, | ||
| select: { | ||
| originalName: true, | ||
| }, | ||
| }); | ||
| if (!file) { | ||
| throw new HttpException('File not found', 404); | ||
| } | ||
|
|
||
| const isInClass = await this.prisma.classUser.findFirst({ | ||
| where: { | ||
| userId, | ||
| class: { | ||
| subjects: { | ||
| some: { | ||
| notes: { | ||
| some: { | ||
| files: { | ||
| some: { id: fileId }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| if (!isInClass) { | ||
| throw new HttpException('Access to file denied', 403); | ||
| } | ||
|
|
||
| const updatedFile = await this.prisma.file.update({ | ||
| where: { id: fileId }, | ||
| data: { originalName: name + path.extname(file.originalName) }, | ||
| }); | ||
| return updatedFile; |
There was a problem hiding this comment.
name is persisted into originalName and later used as the download filename. Without validation/sanitization, a client could provide values containing path separators or control characters, which can cause problematic filenames and potentially unsafe headers. Validate name (length + allowed characters, disallow \r/\n and path separators) before saving.
| VITE_DEBUG=true | ||
|
|
||
| UPLOAD_DIR=./uploads | ||
| MAX_FILE_SIZE=5 # in MB No newline at end of file |
There was a problem hiding this comment.
Inline comments on env var lines aren’t reliably supported by dotenv parsers. MAX_FILE_SIZE=5 # in MB may be loaded as the literal string "5 # in MB", which can lead to confusing behavior/error messages. Move the comment to its own line and keep the value strictly numeric.
| MAX_FILE_SIZE=5 # in MB | |
| # MAX_FILE_SIZE is in MB | |
| MAX_FILE_SIZE=5 |
| } | ||
| await this.prisma.note.delete({ | ||
| where: { id: noteId }, | ||
| }); |
There was a problem hiding this comment.
deleteNote doesn’t return any response body (it resolves to undefined), but the controller documents a successful deletion response. This is also inconsistent with other delete endpoints (e.g., assignments/files) that return a message object. Consider returning a { message: 'Note deleted successfully' } (or the deleted note) for a consistent API contract.
| }); | |
| }); | |
| return { message: 'Note deleted successfully' }; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.