Merge pull request #154 from SELab-2/fix/databank-laat-toevoegen-van-meerdere-studenten-met-dezelfde-username-toe-#153

fix: Databank laat toevoegen van meerdere studenten of leerkrachten met dezelfde username toe (#153) + error handling

backend/src/app.ts

@@ -9,6 +9,7 @@ import { EnvVars, getNumericEnvVar } from './util/envvars.js';
 import apiRouter from './routes/router.js';
 import swaggerMiddleware from './swagger.js';
 import swaggerUi from 'swagger-ui-express';
+import { errorHandler } from './middleware/error-handling/error-handler.js';
 
 const logger: Logger = getLogger();
 
@@ -26,6 +27,8 @@ app.use('/api', apiRouter);
 // Swagger
 app.use('/api-docs', swaggerUi.serve, swaggerMiddleware);
 
+app.use(errorHandler);
+
 async function startServer() {
     await initORM();
 

backend/src/controllers/learning-objects.ts

@@ -4,9 +4,9 @@ import { FilteredLearningObject, LearningObjectIdentifier, LearningPathIdentifie
 import learningObjectService from '../services/learning-objects/learning-object-service.js';
 import { EnvVars, getEnvVar } from '../util/envvars.js';
 import { Language } from '../entities/content/language.js';
-import { BadRequestException } from '../exceptions.js';
 import attachmentService from '../services/learning-objects/attachment-service.js';
 import { NotFoundError } from '@mikro-orm/core';
+import { BadRequestException } from '../exceptions/bad-request-exception.js';
 
 function getLearningObjectIdentifierFromRequest(req: Request): LearningObjectIdentifier {
     if (!req.params.hruid) {

backend/src/controllers/learning-paths.ts

@@ -2,13 +2,14 @@ import { Request, Response } from 'express';
 import { themes } from '../data/themes.js';
 import { FALLBACK_LANG } from '../config.js';
 import learningPathService from '../services/learning-paths/learning-path-service.js';
-import { BadRequestException, NotFoundException } from '../exceptions.js';
 import { Language } from '../entities/content/language.js';
 import {
     PersonalizationTarget,
     personalizedForGroup,
     personalizedForStudent,
 } from '../services/learning-paths/learning-path-personalization-util.js';
+import { BadRequestException } from '../exceptions/bad-request-exception.js';
+import { NotFoundException } from '../exceptions/not-found-exception.js';
 
 /**
  * Fetch learning paths based on query parameters.

backend/src/data/dwengo-entity-repository.ts

@@ -1,10 +1,12 @@
 import { EntityRepository, FilterQuery } from '@mikro-orm/core';
+import { EntityAlreadyExistsException } from '../exceptions/entity-already-exists-exception.js';
 
 export abstract class DwengoEntityRepository<T extends object> extends EntityRepository<T> {
-    public async save(entity: T) {
-        const em = this.getEntityManager();
-        em.persist(entity);
-        await em.flush();
+    public async save(entity: T, options?: { preventOverwrite?: boolean }): Promise<void> {
+        if (options?.preventOverwrite && (await this.findOne(entity))) {
+            throw new EntityAlreadyExistsException(`A ${this.getEntityName()} with this identifier already exists.`);
+        }
+        await this.getEntityManager().persistAndFlush(entity);
     }
     public async deleteWhere(query: FilterQuery<T>) {
         const toDelete = await this.findOne(query);

backend/src/entities/users/student.entity.ts

@@ -13,12 +13,4 @@ export class Student extends User {
 
     @ManyToMany(() => Group)
     groups!: Collection<Group>;
-
-    constructor(
-        public username: string,
-        public firstName: string,
-        public lastName: string
-    ) {
-        super();
-    }
 }

backend/src/entities/users/teacher.entity.ts

@@ -7,12 +7,4 @@ import { TeacherRepository } from '../../data/users/teacher-repository.js';
 export class Teacher extends User {
     @ManyToMany(() => Class)
     classes!: Collection<Class>;
-
-    constructor(
-        public username: string,
-        public firstName: string,
-        public lastName: string
-    ) {
-        super();
-    }
 }

backend/src/exceptions.ts

@@ -1,42 +0,0 @@
-/**
- * Exception for HTTP 400 Bad Request
- */
-export class BadRequestException extends Error {
-    public status = 400;
-
-    constructor(error: string) {
-        super(error);
-    }
-}
-
-/**
- * Exception for HTTP 401 Unauthorized
- */
-export class UnauthorizedException extends Error {
-    status = 401;
-    constructor(message: string = 'Unauthorized') {
-        super(message);
-    }
-}
-
-/**
- * Exception for HTTP 403 Forbidden
- */
-export class ForbiddenException extends Error {
-    status = 403;
-
-    constructor(message: string = 'Forbidden') {
-        super(message);
-    }
-}
-
-/**
- * Exception for HTTP 404 Not Found
- */
-export class NotFoundException extends Error {
-    public status = 404;
-
-    constructor(error: string) {
-        super(error);
-    }
-}

backend/src/exceptions/bad-request-exception.ts

@@ -0,0 +1,10 @@
+import { ExceptionWithHttpState } from './exception-with-http-state.js';
+
+/**
+ * Exception for HTTP 400 Bad Request
+ */
+export class BadRequestException extends ExceptionWithHttpState {
+    constructor(error: string) {
+        super(400, error);
+    }
+}

backend/src/exceptions/conflict-exception.ts

@@ -0,0 +1,12 @@
+import { ExceptionWithHttpState } from './exception-with-http-state.js';
+
+/**
+ * Exception for HTTP 409 Conflict
+ */
+export class ConflictException extends ExceptionWithHttpState {
+    public status = 409;
+
+    constructor(error: string) {
+        super(409, error);
+    }
+}

backend/src/exceptions/entity-already-exists-exception.ts

@@ -0,0 +1,7 @@
+import { ConflictException } from './conflict-exception.js';
+
+export class EntityAlreadyExistsException extends ConflictException {
+    constructor(message: string) {
+        super(message);
+    }
+}

backend/src/exceptions/exception-with-http-state.ts

@@ -0,0 +1,11 @@
+/**
+ * Exceptions which are associated with a HTTP error code.
+ */
+export abstract class ExceptionWithHttpState extends Error {
+    constructor(
+        public status: number,
+        public error: string
+    ) {
+        super(error);
+    }
+}

backend/src/exceptions/forbidden-exception.ts

@@ -0,0 +1,12 @@
+import { ExceptionWithHttpState } from './exception-with-http-state.js';
+
+/**
+ * Exception for HTTP 403 Forbidden
+ */
+export class ForbiddenException extends ExceptionWithHttpState {
+    status = 403;
+
+    constructor(message: string = 'Forbidden') {
+        super(403, message);
+    }
+}

backend/src/exceptions/not-found-exception.ts

@@ -0,0 +1,12 @@
+import { ExceptionWithHttpState } from './exception-with-http-state.js';
+
+/**
+ * Exception for HTTP 404 Not Found
+ */
+export class NotFoundException extends ExceptionWithHttpState {
+    public status = 404;
+
+    constructor(error: string) {
+        super(404, error);
+    }
+}

backend/src/exceptions/unauthorized-exception.ts

@@ -0,0 +1,10 @@
+import { ExceptionWithHttpState } from './exception-with-http-state.js';
+
+/**
+ * Exception for HTTP 401 Unauthorized
+ */
+export class UnauthorizedException extends ExceptionWithHttpState {
+    constructor(message: string = 'Unauthorized') {
+        super(401, message);
+    }
+}

backend/src/interfaces/student.ts

@@ -1,4 +1,5 @@
 import { Student } from '../entities/users/student.entity.js';
+import { getStudentRepository } from '../data/repositories.js';
 
 export interface StudentDTO {
     id: string;
@@ -23,7 +24,9 @@ export function mapToStudentDTO(student: Student): StudentDTO {
 }
 
 export function mapToStudent(studentData: StudentDTO): Student {
-    const student = new Student(studentData.username, studentData.firstName, studentData.lastName);
-
-    return student;
+    return getStudentRepository().create({
+        username: studentData.username,
+        firstName: studentData.firstName,
+        lastName: studentData.lastName,
+    });
 }

backend/src/interfaces/teacher.ts

@@ -1,4 +1,5 @@
 import { Teacher } from '../entities/users/teacher.entity.js';
+import { getTeacherRepository } from '../data/repositories.js';
 
 export interface TeacherDTO {
     id: string;
@@ -22,8 +23,10 @@ export function mapToTeacherDTO(teacher: Teacher): TeacherDTO {
     };
 }
 
-export function mapToTeacher(TeacherData: TeacherDTO): Teacher {
-    const teacher = new Teacher(TeacherData.username, TeacherData.firstName, TeacherData.lastName);
-
-    return teacher;
+export function mapToTeacher(teacherData: TeacherDTO): Teacher {
+    return getTeacherRepository().create({
+        username: teacherData.username,
+        firstName: teacherData.firstName,
+        lastName: teacherData.lastName,
+    });
 }

backend/src/middleware/auth/auth.ts

@@ -6,7 +6,8 @@ import * as express from 'express';
 import * as jwt from 'jsonwebtoken';
 import { AuthenticatedRequest } from './authenticated-request.js';
 import { AuthenticationInfo } from './authentication-info.js';
-import { ForbiddenException, UnauthorizedException } from '../../exceptions.js';
+import { UnauthorizedException } from '../../exceptions/unauthorized-exception.js';
+import { ForbiddenException } from '../../exceptions/forbidden-exception.js';
 
 const JWKS_CACHE = true;
 const JWKS_RATE_LIMIT = true;

backend/src/middleware/error-handling/error-handler.ts

@@ -0,0 +1,15 @@
+import { NextFunction, Request, Response } from 'express';
+import { getLogger, Logger } from '../../logging/initalize.js';
+import { ExceptionWithHttpState } from '../../exceptions/exception-with-http-state.js';
+
+const logger: Logger = getLogger();
+
+export function errorHandler(err: unknown, req: Request, res: Response, _: NextFunction): void {
+    if (err instanceof ExceptionWithHttpState) {
+        logger.warn(`An error occurred while handling a request: ${err} (-> HTTP ${err.status})`);
+        res.status(err.status).json(err);
+    } else {
+        logger.error(`Unexpected error occurred while handing a request: ${JSON.stringify(err)}`);
+        res.status(500).json(err);
+    }
+}

backend/src/mikro-orm.config.ts

@@ -49,6 +49,7 @@ function config(testingMode: boolean = false): Options {
             dbName: getEnvVar(EnvVars.DbName),
             subscribers: [new SqliteAutoincrementSubscriber()],
             entities: entities,
+            persistOnCreate: false, // Do not implicitly save entities when they are created via `create`.
             // EntitiesTs: entitiesTs,
 
             // Workaround: vitest: `TypeError: Unknown file extension ".ts"` (ERR_UNKNOWN_FILE_EXTENSION)
@@ -65,6 +66,7 @@ function config(testingMode: boolean = false): Options {
         user: getEnvVar(EnvVars.DbUsername),
         password: getEnvVar(EnvVars.DbPassword),
         entities: entities,
+        persistOnCreate: false, // Do not implicitly save entities when they are created via `create`.
         // EntitiesTs: entitiesTs,
 
         // Logging

backend/src/services/students.ts

@@ -1,6 +1,4 @@
 import { getClassRepository, getGroupRepository, getStudentRepository, getSubmissionRepository } from '../data/repositories.js';
-import { Class } from '../entities/classes/class.entity.js';
-import { Student } from '../entities/users/student.entity.js';
 import { AssignmentDTO } from '../interfaces/assignment.js';
 import { ClassDTO, mapToClassDTO } from '../interfaces/class.js';
 import { GroupDTO, mapToGroupDTO, mapToGroupDTOId } from '../interfaces/group.js';
@@ -28,15 +26,9 @@ export async function getStudent(username: string): Promise<StudentDTO | null> {
 export async function createStudent(userData: StudentDTO): Promise<StudentDTO | null> {
     const studentRepository = getStudentRepository();
 
-    try {
-        const newStudent = studentRepository.create(mapToStudent(userData));
-        await studentRepository.save(newStudent);
-
-        return mapToStudentDTO(newStudent);
-    } catch (e) {
-        console.log(e);
-        return null;
-    }
+    const newStudent = mapToStudent(userData);
+    await studentRepository.save(newStudent, { preventOverwrite: true });
+    return mapToStudentDTO(newStudent);
 }
 
 export async function deleteStudent(username: string): Promise<StudentDTO | null> {
@@ -87,9 +79,7 @@ export async function getStudentAssignments(username: string, full: boolean): Pr
     const classRepository = getClassRepository();
     const classes = await classRepository.findByStudent(student);
 
-    const assignments = (await Promise.all(classes.map(async (cls) => await getAllAssignments(cls.classId!, full)))).flat();
-
-    return assignments;
+    return (await Promise.all(classes.map(async (cls) => await getAllAssignments(cls.classId!, full)))).flat();
 }
 
 export async function getStudentGroups(username: string, full: boolean): Promise<GroupDTO[]> {

backend/src/services/teachers.ts

@@ -1,18 +1,9 @@
-import {
-    getClassRepository,
-    getLearningObjectRepository,
-    getQuestionRepository,
-    getStudentRepository,
-    getTeacherRepository,
-} from '../data/repositories.js';
-import { Teacher } from '../entities/users/teacher.entity.js';
+import { getClassRepository, getLearningObjectRepository, getQuestionRepository, getTeacherRepository } from '../data/repositories.js';
 import { ClassDTO, mapToClassDTO } from '../interfaces/class.js';
 import { getClassStudents } from './classes.js';
 import { StudentDTO } from '../interfaces/student.js';
 import { mapToQuestionDTO, mapToQuestionId, QuestionDTO, QuestionId } from '../interfaces/question.js';
-import { mapToUser } from '../interfaces/user.js';
 import { mapToTeacher, mapToTeacherDTO, TeacherDTO } from '../interfaces/teacher.js';
-import { teachersOnly } from '../middleware/auth/auth.js';
 
 export async function getAllTeachers(full: boolean): Promise<TeacherDTO[] | string[]> {
     const teacherRepository = getTeacherRepository();
@@ -34,15 +25,10 @@ export async function getTeacher(username: string): Promise<TeacherDTO | null> {
 export async function createTeacher(userData: TeacherDTO): Promise<TeacherDTO | null> {
     const teacherRepository = getTeacherRepository();
 
-    try {
-        const newTeacher = teacherRepository.create(mapToTeacher(userData));
-        await teacherRepository.save(newTeacher);
+    const newTeacher = mapToTeacher(userData);
+    await teacherRepository.save(newTeacher, { preventOverwrite: true });
 
-        return mapToTeacherDTO(newTeacher);
-    } catch (e) {
-        console.log(e);
-        return null;
-    }
+    return mapToTeacherDTO(newTeacher);
 }
 
 export async function deleteTeacher(username: string): Promise<TeacherDTO | null> {

backend/tests/data/users/students.test.ts

@@ -1,5 +1,4 @@
 import { setupTestApp } from '../../setup-tests.js';
-import { Student } from '../../../src/entities/users/student.entity.js';
 import { describe, it, expect, beforeAll } from 'vitest';
 import { StudentRepository } from '../../../src/data/users/student-repository.js';
 import { getStudentRepository } from '../../../src/data/repositories.js';
@@ -30,7 +29,7 @@ describe('StudentRepository', () => {
     });
 
     it('should return the queried student after he was added', async () => {
-        await studentRepository.insert(new Student(username, firstName, lastName));
+        await studentRepository.insert(studentRepository.create({ username, firstName, lastName }));
 
         const retrievedStudent = await studentRepository.findByUsername(username);
         expect(retrievedStudent).toBeTruthy();

backend/tests/data/users/teachers.test.ts

@@ -2,7 +2,6 @@ import { describe, it, expect, beforeAll } from 'vitest';
 import { TeacherRepository } from '../../../src/data/users/teacher-repository';
 import { setupTestApp } from '../../setup-tests';
 import { getTeacherRepository } from '../../../src/data/repositories';
-import { Teacher } from '../../../src/entities/users/teacher.entity';
 
 const username = 'testteacher';
 const firstName = 'John';
@@ -30,7 +29,7 @@ describe('TeacherRepository', () => {
     });
 
     it('should return the queried teacher after he was added', async () => {
-        await teacherRepository.insert(new Teacher(username, firstName, lastName));
+        await teacherRepository.insert(teacherRepository.create({ username, firstName, lastName }));
 
         const retrievedTeacher = await teacherRepository.findByUsername(username);
         expect(retrievedTeacher).toBeTruthy();