hrms-api-org/reports/batch-13-controllers-121-130-analysis.md

845 lines
25 KiB
Markdown
Raw Normal View History

2026-05-08 18:15:03 +07:00
# 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