845 lines
25 KiB
Markdown
845 lines
25 KiB
Markdown
|
|
# Batch 13 Controllers Analysis (Controllers 121-130)
|
||
|
|
|
||
|
|
## Controllers in this batch:
|
||
|
|
1. ProfileOtherEmployeeController
|
||
|
|
2. ProfileOtherEmployeeTempController
|
||
|
|
3. ProfileSalaryController
|
||
|
|
4. ProfileSalaryEmployeeController
|
||
|
|
5. ProfileSalaryEmployeeTempController
|
||
|
|
6. ProfileSalaryTempController
|
||
|
|
7. ProfileTrainingController
|
||
|
|
8. ProfileTrainingEmployeeController
|
||
|
|
9. ProfileTrainingEmployeeTempController
|
||
|
|
10. ProvinceController
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Critical Issues Found
|
||
|
|
|
||
|
|
### 1. **ProfileSalaryTempController** - Multiple Unhandled forEach Async Operations
|
||
|
|
|
||
|
|
**File & Location:** [ProfileSalaryTempController.ts](src/controllers/ProfileSalaryTempController.ts) - Methods: `listSalary()`, `confirmDoneSalary()`, `changeSortEditGenAll()`, `changeSortEdit()`
|
||
|
|
|
||
|
|
**Problem Type:** 1. Unhandled Exception / 2. Missing Error Handle
|
||
|
|
|
||
|
|
**Root Cause:**
|
||
|
|
Multiple methods use `forEach()` with async operations without proper error handling or awaiting. When errors occur in these async callbacks, they become unhandled rejections that can crash the Node.js process.
|
||
|
|
|
||
|
|
**Affected Code Locations:**
|
||
|
|
- Line 1058-1061: `salaryOld.forEach(async (p, i) => { ... })` in `deleteSalary()`
|
||
|
|
- Line 1115-1118: `salaryList.forEach(async (p, i) => { ... })` in `deleteSalary()`
|
||
|
|
- Line 202-205: `salaryOld.forEach((item: any, i) => { ... })` in `listSalary()` (sync operations but no error handling)
|
||
|
|
- Line 1729-1741: `for await` loop with database operations without error handling in `changeSortEditGenAll()`
|
||
|
|
- Line 1763-1766: `salaryOld.forEach()` in `changeSortEdit()`
|
||
|
|
|
||
|
|
**Code Examples:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Line 1115-1118 - DANGEROUS: async forEach without error handling
|
||
|
|
salaryList.forEach(async (p, i) => {
|
||
|
|
p.order = i + 1;
|
||
|
|
await this.salaryRepo.save(p); // If this fails, error is unhandled
|
||
|
|
});
|
||
|
|
```
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Line 1729-1741 - DANGEROUS: for await without try-catch
|
||
|
|
for await (const item of profiles) {
|
||
|
|
let salaryOld = await this.salaryOldRepo.find({
|
||
|
|
where: { profileId: item.id },
|
||
|
|
order: { commandDateAffect: "ASC", order: "ASC" },
|
||
|
|
});
|
||
|
|
|
||
|
|
salaryOld.forEach((item: any, i) => {
|
||
|
|
item.order = i + 1;
|
||
|
|
});
|
||
|
|
num = num + 1;
|
||
|
|
console.log(num);
|
||
|
|
await this.salaryOldRepo.save(salaryOld); // If this fails, entire operation crashes
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**Recommended Fix:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// For deleteSalary() - Use Promise.all with error handling
|
||
|
|
try {
|
||
|
|
await Promise.all(
|
||
|
|
salaryList.map(async (p, i) => {
|
||
|
|
p.order = i + 1;
|
||
|
|
await this.salaryRepo.save(p);
|
||
|
|
})
|
||
|
|
);
|
||
|
|
} catch (error) {
|
||
|
|
console.error('Error updating salary order:', error);
|
||
|
|
// Optionally throw a more specific error or handle gracefully
|
||
|
|
throw new HttpError(HttpStatus.INTERNAL_SERVER_ERROR, 'Failed to update salary order');
|
||
|
|
}
|
||
|
|
|
||
|
|
// For changeSortEditGenAll() - Add error handling per iteration
|
||
|
|
try {
|
||
|
|
const profiles = await this.profileRepo.find();
|
||
|
|
let num = 1;
|
||
|
|
|
||
|
|
for await (const item of profiles) {
|
||
|
|
try {
|
||
|
|
let salaryOld = await this.salaryOldRepo.find({
|
||
|
|
where: { profileId: item.id },
|
||
|
|
order: { commandDateAffect: "ASC", order: "ASC" },
|
||
|
|
});
|
||
|
|
|
||
|
|
salaryOld.forEach((item: any, i) => {
|
||
|
|
item.order = i + 1;
|
||
|
|
});
|
||
|
|
|
||
|
|
await this.salaryOldRepo.save(salaryOld);
|
||
|
|
num = num + 1;
|
||
|
|
console.log(num);
|
||
|
|
} catch (error) {
|
||
|
|
console.error(`Error processing profile ${item.id}:`, error);
|
||
|
|
// Continue with next profile instead of crashing
|
||
|
|
continue;
|
||
|
|
}
|
||
|
|
}
|
||
|
|
|
||
|
|
return new HttpSuccess();
|
||
|
|
} catch (error) {
|
||
|
|
console.error('Error in changeSortEditGenAll:', error);
|
||
|
|
throw new HttpError(HttpStatus.INTERNAL_SERVER_ERROR, 'Failed to process profiles');
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 2. **ProfileSalaryController** - Unhandled forEach Async Operations
|
||
|
|
|
||
|
|
**File & Location:** [ProfileSalaryController.ts](src/controllers/ProfileSalaryController.ts) - Methods: `deleteSalary()`, `Registry()`, `RegistryEmployee()`
|
||
|
|
|
||
|
|
**Problem Type:** 1. Unhandled Exception / 2. Missing Error Handle
|
||
|
|
|
||
|
|
**Root Cause:**
|
||
|
|
Multiple critical methods use `forEach()` with async database operations. When database operations fail within these callbacks, the Promise rejection is unhandled and can crash the service.
|
||
|
|
|
||
|
|
**Affected Code Locations:**
|
||
|
|
- Line 1115-1118: `salaryList.forEach(async (p, i) => { ... })` in `deleteSalary()`
|
||
|
|
- Line 362-373: Complex async operations in `Registry()` without error handling
|
||
|
|
- Line 383-395: Complex async operations in `RegistryEmployee()` without error handling
|
||
|
|
- Line 412-427: `record.map(async (r) => { ... })` with `Promise.all()` but no error handling
|
||
|
|
- Line 463-477: Similar pattern in `getSalaryPositionUser()`
|
||
|
|
- Line 497-512: Similar pattern in `getSalary()`
|
||
|
|
|
||
|
|
**Code Examples:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Line 1115-1118 - CRITICAL: async forEach without error handling
|
||
|
|
salaryList.forEach(async (p, i) => {
|
||
|
|
p.order = i + 1;
|
||
|
|
await this.salaryRepo.save(p); // Unhandled rejection
|
||
|
|
});
|
||
|
|
```
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Line 412-427 - Promise.all without error handling
|
||
|
|
const result = await Promise.all(
|
||
|
|
record.map(async (r) => {
|
||
|
|
let _command = null;
|
||
|
|
if (r.commandId) {
|
||
|
|
_command = await this.commandRepository.findOne({
|
||
|
|
where: { id: r.commandId },
|
||
|
|
relations: ["commandType"],
|
||
|
|
});
|
||
|
|
}
|
||
|
|
return {
|
||
|
|
...r,
|
||
|
|
commandType: _command && _command?.commandType ? _command?.commandType.code : null,
|
||
|
|
};
|
||
|
|
}),
|
||
|
|
);
|
||
|
|
```
|
||
|
|
|
||
|
|
**Recommended Fix:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// For deleteSalary() - Proper error handling
|
||
|
|
try {
|
||
|
|
await Promise.all(
|
||
|
|
salaryList.map(async (p, i) => {
|
||
|
|
p.order = i + 1;
|
||
|
|
await this.salaryRepo.save(p);
|
||
|
|
})
|
||
|
|
);
|
||
|
|
} catch (error) {
|
||
|
|
console.error('Error updating salary order:', error);
|
||
|
|
throw new HttpError(HttpStatus.INTERNAL_SERVER_ERROR, 'Failed to update salary order');
|
||
|
|
}
|
||
|
|
|
||
|
|
// For Promise.all operations - Add error boundary
|
||
|
|
try {
|
||
|
|
const result = await Promise.all(
|
||
|
|
record.map(async (r) => {
|
||
|
|
try {
|
||
|
|
let _command = null;
|
||
|
|
if (r.commandId) {
|
||
|
|
_command = await this.commandRepository.findOne({
|
||
|
|
where: { id: r.commandId },
|
||
|
|
relations: ["commandType"],
|
||
|
|
});
|
||
|
|
}
|
||
|
|
return {
|
||
|
|
...r,
|
||
|
|
commandType: _command && _command?.commandType ? _command?.commandType.code : null,
|
||
|
|
};
|
||
|
|
} catch (error) {
|
||
|
|
console.error(`Error loading command for salary ${r.id}:`, error);
|
||
|
|
return {
|
||
|
|
...r,
|
||
|
|
commandType: null,
|
||
|
|
};
|
||
|
|
}
|
||
|
|
}),
|
||
|
|
);
|
||
|
|
return new HttpSuccess(result);
|
||
|
|
} catch (error) {
|
||
|
|
console.error('Error processing salary records:', error);
|
||
|
|
throw new HttpError(HttpStatus.INTERNAL_SERVER_ERROR, 'Failed to load salary data');
|
||
|
|
}
|
||
|
|
|
||
|
|
// For Registry() - Add comprehensive error handling
|
||
|
|
try {
|
||
|
|
await this.registryRepo.clear();
|
||
|
|
|
||
|
|
const allRegis = await AppDataSource.getRepository(viewRegistryOfficer)
|
||
|
|
.createQueryBuilder("registryOfficer")
|
||
|
|
.getMany();
|
||
|
|
|
||
|
|
const profileIds = new Set((await this.profileRepo.find()).map((p) => p.id));
|
||
|
|
|
||
|
|
const mapData = allRegis
|
||
|
|
.filter((x) => profileIds.has(x.profileId))
|
||
|
|
.map((x) => ({
|
||
|
|
...x,
|
||
|
|
isProbation: Boolean(x.isProbation),
|
||
|
|
isLeave: Boolean(x.isLeave),
|
||
|
|
isRetirement: Boolean(x.isRetirement),
|
||
|
|
Educations: x.Educations ? JSON.stringify(x.Educations) : "",
|
||
|
|
}));
|
||
|
|
|
||
|
|
if (mapData.length > 0) {
|
||
|
|
// Save in batches to avoid overwhelming the database
|
||
|
|
const batchSize = 100;
|
||
|
|
for (let i = 0; i < mapData.length; i += batchSize) {
|
||
|
|
const batch = mapData.slice(i, i + batchSize);
|
||
|
|
await this.registryRepo.save(batch);
|
||
|
|
}
|
||
|
|
}
|
||
|
|
|
||
|
|
return new HttpSuccess();
|
||
|
|
} catch (error) {
|
||
|
|
console.error('Error in Registry cronjob:', error);
|
||
|
|
throw new HttpError(HttpStatus.INTERNAL_SERVER_ERROR, 'Failed to sync registry data');
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 3. **ProfileSalaryController** - Raw SQL Queries Without Error Handling
|
||
|
|
|
||
|
|
**File & Location:** [ProfileSalaryController.ts](src/controllers/ProfileSalaryController.ts) - Multiple methods using `AppDataSource.query()`
|
||
|
|
|
||
|
|
**Problem Type:** 2. Missing Error Handle
|
||
|
|
|
||
|
|
**Root Cause:**
|
||
|
|
Multiple stored procedure calls (`CALL GetProfile...()`) are executed without try-catch blocks. If these stored procedures fail or the database is unavailable, the errors will propagate unhandled.
|
||
|
|
|
||
|
|
**Affected Code Locations:**
|
||
|
|
- Line 76-79: `CALL GetProfileSalaryPosition(?, ?)` in `cronjobTenurePositionOfficer()`
|
||
|
|
- Line 126-129: Similar in `cronjobTenurePositionEmployee()`
|
||
|
|
- Line 176-179: `CALL GetProfileSalaryLevel(?, ?)` in `cronjobTenureLevelOfficer()`
|
||
|
|
- Line 236-239: Similar in `cronjobTenureLevelEmployee()`
|
||
|
|
- Line 317-320: `CALL GetProfileSalaryExecutive(?, ?)` in `cronjobTenureExecutivePositionOfficer()`
|
||
|
|
- Line 588-591, 622-625, 662-665: Multiple calls in `getPositionTenureUser()`
|
||
|
|
- Line 722-725, 760-763, 803-806: Multiple calls in `getPositionTenure()`
|
||
|
|
|
||
|
|
**Code Examples:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Line 76-79 - No error handling for stored procedure call
|
||
|
|
const position = await AppDataSource.query("CALL GetProfileSalaryPosition(?, ?)", [
|
||
|
|
x.id,
|
||
|
|
_currentDate,
|
||
|
|
]);
|
||
|
|
```
|
||
|
|
|
||
|
|
**Recommended Fix:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Wrap all database query calls in try-catch
|
||
|
|
try {
|
||
|
|
const position = await AppDataSource.query("CALL GetProfileSalaryPosition(?, ?)", [
|
||
|
|
x.id,
|
||
|
|
_currentDate,
|
||
|
|
]);
|
||
|
|
|
||
|
|
const _position = position.length > 0 ? position[0] : [];
|
||
|
|
const mapPosition =
|
||
|
|
_position.length > 1
|
||
|
|
? _position.slice(1).map((curr: any, index: number) => ({
|
||
|
|
days_diff: curr.days_diff,
|
||
|
|
positionName: _position[index]?.positionName,
|
||
|
|
}))
|
||
|
|
: [];
|
||
|
|
|
||
|
|
const calDayDiff = mapPosition
|
||
|
|
.filter((curr: any) => curr.positionName == x.position)
|
||
|
|
.reduce(
|
||
|
|
(acc: any, curr: any) => {
|
||
|
|
acc.days_diff += Number(curr.days_diff) || 0;
|
||
|
|
acc.positionName = curr.positionName;
|
||
|
|
return acc;
|
||
|
|
},
|
||
|
|
{ days_diff: 0, positionName: null },
|
||
|
|
);
|
||
|
|
|
||
|
|
const { year, month, day } = calculateTenure(calDayDiff.days_diff);
|
||
|
|
const mapData: any = {
|
||
|
|
profileId: x.id,
|
||
|
|
positionName: calDayDiff.positionName,
|
||
|
|
days_diff: calDayDiff.days_diff,
|
||
|
|
Years: year,
|
||
|
|
Months: month,
|
||
|
|
Days: day,
|
||
|
|
};
|
||
|
|
data.push(mapData);
|
||
|
|
} catch (error) {
|
||
|
|
console.error(`Error processing position tenure for profile ${x.id}:`, error);
|
||
|
|
// Add default/error entry or skip this profile
|
||
|
|
const mapData: any = {
|
||
|
|
profileId: x.id,
|
||
|
|
positionName: null,
|
||
|
|
days_diff: 0,
|
||
|
|
Years: 0,
|
||
|
|
Months: 0,
|
||
|
|
Days: 0,
|
||
|
|
error: true,
|
||
|
|
};
|
||
|
|
data.push(mapData);
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 4. **ProfileTrainingController** - Multiple Database Operations Without Error Handling
|
||
|
|
|
||
|
|
**File & Location:** [ProfileTrainingController.ts](src/controllers/ProfileTrainingController.ts) - Methods: `deleteAllTraining()`, `deleteById()`
|
||
|
|
|
||
|
|
**Problem Type:** 2. Missing Error Handle
|
||
|
|
|
||
|
|
**Root Cause:**
|
||
|
|
Multiple sequential delete operations without transaction or error handling. If intermediate operations fail, the database can be left in an inconsistent state.
|
||
|
|
|
||
|
|
**Affected Code Locations:**
|
||
|
|
- Line 238-259: `deleteAllTraining()` - Multiple delete operations without transaction
|
||
|
|
- Line 274-339: `deleteById()` - Multiple delete operations without transaction
|
||
|
|
|
||
|
|
**Code Examples:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Line 238-259 - No error handling or transaction
|
||
|
|
const trainings = await this.trainingRepo.find({
|
||
|
|
select: { id: true },
|
||
|
|
where: { developmentId: reqBody.developmentId },
|
||
|
|
});
|
||
|
|
if (trainings.length > 0) {
|
||
|
|
const trainingIds = trainings.map((x) => x.id);
|
||
|
|
await this.trainingHistoryRepo.delete({
|
||
|
|
profileTrainingId: In(trainingIds),
|
||
|
|
});
|
||
|
|
await this.trainingRepo.delete({
|
||
|
|
developmentId: reqBody.developmentId,
|
||
|
|
});
|
||
|
|
}
|
||
|
|
|
||
|
|
await this.developmentHistoryRepo.delete({
|
||
|
|
kpiDevelopmentId: reqBody.developmentId,
|
||
|
|
});
|
||
|
|
await this.developmentRepo.delete({
|
||
|
|
kpiDevelopmentId: reqBody.developmentId
|
||
|
|
});
|
||
|
|
```
|
||
|
|
|
||
|
|
**Recommended Fix:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
@Post("delete-all")
|
||
|
|
public async deleteAllTraining(
|
||
|
|
@Body() reqBody: { developmentId: string },
|
||
|
|
@Request() req: RequestWithUser
|
||
|
|
) {
|
||
|
|
const queryRunner = AppDataSource.createQueryRunner();
|
||
|
|
await queryRunner.connect();
|
||
|
|
await queryRunner.startTransaction();
|
||
|
|
|
||
|
|
try {
|
||
|
|
const trainings = await queryRunner.manager.find(ProfileTraining, {
|
||
|
|
select: { id: true },
|
||
|
|
where: { developmentId: reqBody.developmentId },
|
||
|
|
});
|
||
|
|
|
||
|
|
if (trainings.length > 0) {
|
||
|
|
const trainingIds = trainings.map((x) => x.id);
|
||
|
|
|
||
|
|
await queryRunner.manager.delete(ProfileTrainingHistory, {
|
||
|
|
profileTrainingId: In(trainingIds),
|
||
|
|
});
|
||
|
|
|
||
|
|
await queryRunner.manager.delete(ProfileTraining, {
|
||
|
|
developmentId: reqBody.developmentId,
|
||
|
|
});
|
||
|
|
}
|
||
|
|
|
||
|
|
await queryRunner.manager.delete(ProfileDevelopmentHistory, {
|
||
|
|
kpiDevelopmentId: reqBody.developmentId,
|
||
|
|
});
|
||
|
|
|
||
|
|
await queryRunner.manager.delete(ProfileDevelopment, {
|
||
|
|
kpiDevelopmentId: reqBody.developmentId
|
||
|
|
});
|
||
|
|
|
||
|
|
await queryRunner.commitTransaction();
|
||
|
|
return new HttpSuccess();
|
||
|
|
} catch (error) {
|
||
|
|
await queryRunner.rollbackTransaction();
|
||
|
|
console.error('Error deleting training data:', error);
|
||
|
|
throw new HttpError(
|
||
|
|
HttpStatus.INTERNAL_SERVER_ERROR,
|
||
|
|
'Failed to delete training data'
|
||
|
|
);
|
||
|
|
} finally {
|
||
|
|
await queryRunner.release();
|
||
|
|
}
|
||
|
|
}
|
||
|
|
|
||
|
|
// Similar fix for deleteById()
|
||
|
|
@Post("delete-byId")
|
||
|
|
public async deleteById(
|
||
|
|
@Body() reqBody: {
|
||
|
|
type: string;
|
||
|
|
profileId: string;
|
||
|
|
developmentId: string;
|
||
|
|
},
|
||
|
|
@Request() req: RequestWithUser
|
||
|
|
) {
|
||
|
|
const queryRunner = AppDataSource.createQueryRunner();
|
||
|
|
await queryRunner.connect();
|
||
|
|
await queryRunner.startTransaction();
|
||
|
|
|
||
|
|
try {
|
||
|
|
const type = reqBody.type?.trim().toUpperCase();
|
||
|
|
|
||
|
|
// 1. validate profile
|
||
|
|
if (type === "OFFICER") {
|
||
|
|
const profile = await queryRunner.manager.findOne(Profile, {
|
||
|
|
where: { id: reqBody.profileId }
|
||
|
|
});
|
||
|
|
if (!profile) {
|
||
|
|
throw new HttpError(HttpStatus.BAD_REQUEST, "ไม่พบ profile ดังกล่าว");
|
||
|
|
}
|
||
|
|
} else {
|
||
|
|
const profile = await queryRunner.manager.findOne(ProfileEmployee, {
|
||
|
|
where: { id: reqBody.profileId }
|
||
|
|
});
|
||
|
|
if (!profile) {
|
||
|
|
throw new HttpError(HttpStatus.BAD_REQUEST, "ไม่พบ profile ดังกล่าว");
|
||
|
|
}
|
||
|
|
}
|
||
|
|
|
||
|
|
const profileField = type === "OFFICER" ? "profileId" : "profileEmployeeId";
|
||
|
|
|
||
|
|
// 2. Find and delete ProfileTraining
|
||
|
|
const trainings = await queryRunner.manager.find(ProfileTraining, {
|
||
|
|
select: { id: true },
|
||
|
|
where: {
|
||
|
|
developmentId: reqBody.developmentId,
|
||
|
|
[profileField]: reqBody.profileId,
|
||
|
|
},
|
||
|
|
});
|
||
|
|
|
||
|
|
if (trainings.length > 0) {
|
||
|
|
const trainingIds = trainings.map(x => x.id);
|
||
|
|
|
||
|
|
await queryRunner.manager.delete(ProfileTrainingHistory, {
|
||
|
|
profileTrainingId: In(trainingIds),
|
||
|
|
});
|
||
|
|
|
||
|
|
await queryRunner.manager.delete(ProfileTraining, {
|
||
|
|
id: In(trainingIds),
|
||
|
|
});
|
||
|
|
}
|
||
|
|
|
||
|
|
// 3. Find and delete ProfileDevelopment
|
||
|
|
const developments = await queryRunner.manager.find(ProfileDevelopment, {
|
||
|
|
select: { id: true },
|
||
|
|
where: {
|
||
|
|
kpiDevelopmentId: reqBody.developmentId,
|
||
|
|
[profileField]: reqBody.profileId,
|
||
|
|
},
|
||
|
|
});
|
||
|
|
|
||
|
|
if (developments.length > 0) {
|
||
|
|
const devIds = developments.map(x => x.id);
|
||
|
|
|
||
|
|
await queryRunner.manager.delete(ProfileDevelopmentHistory, {
|
||
|
|
profileDevelopmentId: In(devIds),
|
||
|
|
});
|
||
|
|
|
||
|
|
await queryRunner.manager.delete(ProfileDevelopment, {
|
||
|
|
id: In(devIds),
|
||
|
|
});
|
||
|
|
}
|
||
|
|
|
||
|
|
await queryRunner.commitTransaction();
|
||
|
|
return new HttpSuccess();
|
||
|
|
} catch (error) {
|
||
|
|
await queryRunner.rollbackTransaction();
|
||
|
|
console.error('Error deleting by ID:', error);
|
||
|
|
if (error instanceof HttpError) {
|
||
|
|
throw error;
|
||
|
|
}
|
||
|
|
throw new HttpError(
|
||
|
|
HttpStatus.INTERNAL_SERVER_ERROR,
|
||
|
|
'Failed to delete data'
|
||
|
|
);
|
||
|
|
} finally {
|
||
|
|
await queryRunner.release();
|
||
|
|
}
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 5. **ProfileSalaryEmployeeController** - forEach Async Operations Without Error Handling
|
||
|
|
|
||
|
|
**File & Location:** [ProfileSalaryEmployeeController.ts](src/controllers/ProfileSalaryEmployeeController.ts) - Method: `deleteSalaryEmployee()`
|
||
|
|
|
||
|
|
**Problem Type:** 1. Unhandled Exception / 2. Missing Error Handle
|
||
|
|
|
||
|
|
**Root Cause:**
|
||
|
|
Similar to ProfileSalaryController, uses `forEach()` with async operations without proper error handling.
|
||
|
|
|
||
|
|
**Affected Code Locations:**
|
||
|
|
- Line 608-611: `salaryList.forEach(async (p, i) => { ... })` in `deleteSalaryEmployee()`
|
||
|
|
|
||
|
|
**Code Example:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Line 608-611 - DANGEROUS
|
||
|
|
salaryList.forEach(async (p, i) => {
|
||
|
|
p.order = i + 1;
|
||
|
|
await this.salaryRepo.save(p); // Unhandled rejection
|
||
|
|
});
|
||
|
|
```
|
||
|
|
|
||
|
|
**Recommended Fix:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
try {
|
||
|
|
await Promise.all(
|
||
|
|
salaryList.map(async (p, i) => {
|
||
|
|
p.order = i + 1;
|
||
|
|
await this.salaryRepo.save(p);
|
||
|
|
})
|
||
|
|
);
|
||
|
|
} catch (error) {
|
||
|
|
console.error('Error updating salary order:', error);
|
||
|
|
throw new HttpError(HttpStatus.INTERNAL_SERVER_ERROR, 'Failed to update salary order');
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 6. **ProfileSalaryEmployeeTempController** - forEach Async Operations Without Error Handling
|
||
|
|
|
||
|
|
**File & Location:** [ProfileSalaryEmployeeTempController.ts](src/controllers/ProfileSalaryEmployeeTempController.ts) - Method: `deleteSalaryEmployee()`
|
||
|
|
|
||
|
|
**Problem Type:** 1. Unhandled Exception / 2. Missing Error Handle
|
||
|
|
|
||
|
|
**Root Cause:**
|
||
|
|
Same pattern as above - `forEach()` with async operations.
|
||
|
|
|
||
|
|
**Affected Code Locations:**
|
||
|
|
- Line 202-205: `salaryList.forEach(async (p, i) => { ... })` in `deleteSalaryEmployee()`
|
||
|
|
|
||
|
|
**Recommended Fix:** Same as above - use `Promise.all()` with error handling.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 7. **ProfileSalaryTempController** - confirmDoneSalary() Transaction Handling Issues
|
||
|
|
|
||
|
|
**File & Location:** [ProfileSalaryTempController.ts](src/controllers/ProfileSalaryTempController.ts) - Method: `confirmDoneSalary()`
|
||
|
|
|
||
|
|
**Problem Type:** 2. Missing Error Handle
|
||
|
|
|
||
|
|
**Root Cause:**
|
||
|
|
While this method uses transactions, there are several potential issues:
|
||
|
|
1. Line 1686: Empty `catch` block that swallows all errors
|
||
|
|
2. Line 1493-1497: Error is re-thrown without proper logging or context
|
||
|
|
3. Multiple complex operations within transaction that could fail
|
||
|
|
|
||
|
|
**Affected Code Locations:**
|
||
|
|
- Line 1493-1498: `catch` block re-throws error without logging
|
||
|
|
- Line 1685: Empty `catch` block in `returnEdit()`
|
||
|
|
|
||
|
|
**Code Examples:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Line 1493-1498 - Insufficient error handling
|
||
|
|
} catch (error) {
|
||
|
|
await queryRunner.rollbackTransaction();
|
||
|
|
throw error; // No logging, no context
|
||
|
|
} finally {
|
||
|
|
await queryRunner.release();
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**Recommended Fix:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
} catch (error) {
|
||
|
|
await queryRunner.rollbackTransaction();
|
||
|
|
console.error('Error in confirmDoneSalary:', {
|
||
|
|
profileId: body.profileId,
|
||
|
|
type: body.type,
|
||
|
|
error: error instanceof Error ? error.message : error,
|
||
|
|
stack: error instanceof Error ? error.stack : undefined,
|
||
|
|
});
|
||
|
|
|
||
|
|
// Provide more specific error message
|
||
|
|
if (error instanceof HttpError) {
|
||
|
|
throw error;
|
||
|
|
}
|
||
|
|
|
||
|
|
throw new HttpError(
|
||
|
|
HttpStatus.INTERNAL_SERVER_ERROR,
|
||
|
|
'Failed to confirm salary data. Please try again.'
|
||
|
|
);
|
||
|
|
} finally {
|
||
|
|
await queryRunner.release();
|
||
|
|
}
|
||
|
|
|
||
|
|
// For returnEdit() - Proper error handling
|
||
|
|
try {
|
||
|
|
if (profile) {
|
||
|
|
profile.statusCheckEdit = "PENDING";
|
||
|
|
await this.profileRepo.save(profile);
|
||
|
|
} else if (profileEmployee) {
|
||
|
|
profileEmployee.statusCheckEdit = "PENDING";
|
||
|
|
await this.profileEmployeeRepo.save(profileEmployee);
|
||
|
|
}
|
||
|
|
|
||
|
|
const history: PositionSalaryEditHistory = Object.assign(
|
||
|
|
new PositionSalaryEditHistory(),
|
||
|
|
body,
|
||
|
|
);
|
||
|
|
|
||
|
|
if (profile) {
|
||
|
|
history.profileId = profileId;
|
||
|
|
} else if (profileEmployee) {
|
||
|
|
history.profileEmployeeId = profileId;
|
||
|
|
}
|
||
|
|
|
||
|
|
history.returnedDate = new Date();
|
||
|
|
history.examinerName = req.user.name;
|
||
|
|
history.createdFullName = req.user.name;
|
||
|
|
history.lastUpdateFullName = req.user.name;
|
||
|
|
|
||
|
|
await this.positionSalaryEditHistoryRepo.save(history);
|
||
|
|
|
||
|
|
return new HttpSuccess();
|
||
|
|
} catch (error) {
|
||
|
|
console.error('Error in returnEdit:', error);
|
||
|
|
throw new HttpError(
|
||
|
|
HttpStatus.INTERNAL_SERVER_ERROR,
|
||
|
|
'Failed to process return edit request'
|
||
|
|
);
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 8. **ProfileSalaryTempController** - Bulk Operations Without Error Handling
|
||
|
|
|
||
|
|
**File & Location:** [ProfileSalaryTempController.ts](src/controllers/ProfileSalaryTempController.ts) - Methods: `listSalary()`, `confirmDoneSalary()`
|
||
|
|
|
||
|
|
**Problem Type:** 2. Missing Error Handle
|
||
|
|
|
||
|
|
**Root Cause:**
|
||
|
|
Bulk insert operations without error handling for individual records. If one record fails, the entire operation may fail or data may be partially inserted.
|
||
|
|
|
||
|
|
**Affected Code Locations:**
|
||
|
|
- Line 1058-1061: `salaryOld.forEach()` without error handling
|
||
|
|
- Line 1098-1101: Similar pattern
|
||
|
|
- Line 1425-1431: Bulk insert without error handling
|
||
|
|
|
||
|
|
**Code Example:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Line 1425-1431 - Bulk insert without error handling
|
||
|
|
if (salaryRows.length) {
|
||
|
|
await queryRunner.manager.insert(
|
||
|
|
ProfileSalary,
|
||
|
|
salaryRows.map(({ id, ...data }) => ({
|
||
|
|
...data,
|
||
|
|
...metaCreated,
|
||
|
|
})),
|
||
|
|
);
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**Recommended Fix:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Implement batch processing with error handling
|
||
|
|
if (salaryRows.length) {
|
||
|
|
const batchSize = 100; // Process in batches
|
||
|
|
for (let i = 0; i < salaryRows.length; i += batchSize) {
|
||
|
|
const batch = salaryRows.slice(i, i + batchSize);
|
||
|
|
|
||
|
|
try {
|
||
|
|
await queryRunner.manager.insert(
|
||
|
|
ProfileSalary,
|
||
|
|
batch.map(({ id, ...data }) => ({
|
||
|
|
...data,
|
||
|
|
...metaCreated,
|
||
|
|
}))
|
||
|
|
);
|
||
|
|
} catch (error) {
|
||
|
|
console.error(`Error inserting salary batch ${i / batchSize + 1}:`, error);
|
||
|
|
// Log which records failed
|
||
|
|
const failedIds = batch.map(b => b.id);
|
||
|
|
console.error('Failed record IDs:', failedIds);
|
||
|
|
throw new HttpError(
|
||
|
|
HttpStatus.INTERNAL_SERVER_ERROR,
|
||
|
|
`Failed to insert salary records (batch ${i / batchSize + 1})`
|
||
|
|
);
|
||
|
|
}
|
||
|
|
}
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
### 9. **ProvinceController** - Try-Catch With Generic Error Handling
|
||
|
|
|
||
|
|
**File & Location:** [ProvinceController.ts](src/controllers/ProvinceController.ts) - Method: `Delete()`
|
||
|
|
|
||
|
|
**Problem Type:** 2. Missing Error Handle
|
||
|
|
|
||
|
|
**Root Cause:**
|
||
|
|
While there is a try-catch block, it catches all errors without logging or differentiation. This makes debugging difficult and may mask underlying issues.
|
||
|
|
|
||
|
|
**Affected Code Locations:**
|
||
|
|
- Line 168-175: Generic catch block
|
||
|
|
|
||
|
|
**Code Example:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
// Line 168-175 - Generic error handling
|
||
|
|
let result: any;
|
||
|
|
try {
|
||
|
|
result = await this.provinceRepository.delete({ id: id });
|
||
|
|
} catch {
|
||
|
|
throw new HttpError(
|
||
|
|
HttpStatusCode.NOT_FOUND,
|
||
|
|
"ไม่สามารถลบได้เนื่องจากมีการใช้งานข้อมูลจังหวัดนี้อยู่",
|
||
|
|
);
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**Recommended Fix:**
|
||
|
|
|
||
|
|
```typescript
|
||
|
|
let result: any;
|
||
|
|
try {
|
||
|
|
result = await this.provinceRepository.delete({ id: id });
|
||
|
|
} catch (error) {
|
||
|
|
console.error('Error deleting province:', {
|
||
|
|
id,
|
||
|
|
error: error instanceof Error ? error.message : error,
|
||
|
|
stack: error instanceof Error ? error.stack : undefined,
|
||
|
|
});
|
||
|
|
|
||
|
|
// Check for foreign key constraint error
|
||
|
|
if (error instanceof Error && error.message.includes('foreign key constraint')) {
|
||
|
|
throw new HttpError(
|
||
|
|
HttpStatusCode.CONFLICT, // Use 409 instead of 404
|
||
|
|
"ไม่สามารถลบได้เนื่องจากมีการใช้งานข้อมูลจังหวัดนี้อยู่",
|
||
|
|
);
|
||
|
|
}
|
||
|
|
|
||
|
|
throw new HttpError(
|
||
|
|
HttpStatusCode.INTERNAL_SERVER_ERROR,
|
||
|
|
"เกิดข้อผิดพลาดในการลบข้อมูลจังหวัด",
|
||
|
|
);
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Summary Statistics
|
||
|
|
|
||
|
|
**Total Critical Issues Found:** 9
|
||
|
|
|
||
|
|
**Breakdown by Type:**
|
||
|
|
- **Unhandled Exception (forEach with async):** 6 instances
|
||
|
|
- **Missing Error Handling (DB operations):** 8 instances
|
||
|
|
- **Transaction Issues:** 2 instances
|
||
|
|
- **Generic Error Handling:** 1 instance
|
||
|
|
|
||
|
|
**Controllers with Issues:**
|
||
|
|
1. ProfileSalaryTempController - 4 critical issues
|
||
|
|
2. ProfileSalaryController - 3 critical issues
|
||
|
|
3. ProfileSalaryEmployeeController - 1 critical issue
|
||
|
|
4. ProfileSalaryEmployeeTempController - 1 critical issue
|
||
|
|
5. ProfileTrainingController - 2 critical issues
|
||
|
|
6. ProvinceController - 1 minor issue
|
||
|
|
|
||
|
|
**Risk Level: HIGH**
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Priority Recommendations
|
||
|
|
|
||
|
|
### Immediate Actions Required:
|
||
|
|
|
||
|
|
1. **Replace all `forEach()` with async operations** - Use `Promise.all()` or `for...of` loops with proper error handling
|
||
|
|
2. **Add error boundaries** around all database operations
|
||
|
|
3. **Implement proper logging** for all errors
|
||
|
|
4. **Use transactions** for multi-step database operations
|
||
|
|
5. **Add circuit breakers** for external dependencies (database, stored procedures)
|
||
|
|
|
||
|
|
### Graceful Recovery Strategies:
|
||
|
|
|
||
|
|
1. **Implement request-level error boundaries** - Catch errors at the controller level and return appropriate HTTP responses
|
||
|
|
2. **Add database operation timeouts** - Prevent indefinite hangs
|
||
|
|
3. **Implement retry logic** for transient database errors
|
||
|
|
4. **Add health checks** - Monitor database connectivity
|
||
|
|
5. **Use connection pooling** with proper error handling
|
||
|
|
|
||
|
|
### Long-term Improvements:
|
||
|
|
|
||
|
|
1. **Implement a centralized error handling middleware**
|
||
|
|
2. **Add structured logging** (e.g., Winston, Pino)
|
||
|
|
3. **Implement request tracing** for debugging
|
||
|
|
4. **Add metrics/monitoring** for error rates
|
||
|
|
5. **Implement graceful shutdown** procedures
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Testing Recommendations
|
||
|
|
|
||
|
|
1. **Test database failure scenarios** - Disconnect database during operations
|
||
|
|
2. **Test with large datasets** - Ensure forEach operations don't cause memory issues
|
||
|
|
3. **Test transaction rollback** - Verify data consistency on errors
|
||
|
|
4. **Test concurrent requests** - Ensure race conditions don't cause crashes
|
||
|
|
5. **Test stored procedure failures** - Simulate SP errors
|