report: Controllers
This commit is contained in:
DESKTOP-1R2VSQH\Lenovo ThinkPad E490
parent
7104ce4f34
commit
85e9be08f6
15 changed files with 10752 additions and 0 deletions
445
reports/batch-08-controllers-71-80-analysis.md
Normal file
445
reports/batch-08-controllers-71-80-analysis.md
Normal file
|
|
@ -0,0 +1,445 @@
|
|||
# Batch 08: Controllers 71-80 Analysis - Unhandled Exception & Crash Loop Risks
|
||||
|
||||
## Executive Summary
|
||||
พบจุดเสี่ยงระดับ **CRITICAL** ที่อาจทำให้เกิด **Unhandled Exception** และ **Crash Loop** ในระบบ Microservices จำนวน **8 จุด** จากการตรวจสอบ 10 Controllers ในชุดที่ 8
|
||||
|
||||
---
|
||||
|
||||
## Critical Issues Found
|
||||
|
||||
### 1. **CRITICAL** - Unhandled External API Call in ProfileController.ts
|
||||
|
||||
#### **File & Location**
|
||||
- **File:** `src/controllers/ProfileController.ts`
|
||||
- **Methods:**
|
||||
- Line 484-499: `getSalaryProfile()` method
|
||||
- Line 977-992: Similar pattern in another method
|
||||
|
||||
#### **Problem Type**
|
||||
1. **Unhandled Exception**
|
||||
2. **Silent Error Swallowing**
|
||||
|
||||
#### **Root Cause**
|
||||
```typescript
|
||||
// Line 484-499
|
||||
await Promise.all(
|
||||
await profiles.profileAvatars.slice(-7).map(async (x, i) => {
|
||||
if (x == null) {
|
||||
_ImgUrl[i] = null;
|
||||
} else {
|
||||
const url = process.env.API_URL + `/salary/file/${x?.avatar}/${x?.avatarName}`;
|
||||
try {
|
||||
const response_ = await axios.get(url, {
|
||||
headers: {
|
||||
Authorization: `${token_}`,
|
||||
"Content-Type": "application/json",
|
||||
api_key: process.env.API_KEY,
|
||||
},
|
||||
});
|
||||
_ImgUrl[i] = response_.data.downloadUrl;
|
||||
} catch {} // ❌ SILENT ERROR - Empty catch block
|
||||
}
|
||||
})
|
||||
);
|
||||
```
|
||||
|
||||
**รายละเอียดปัญหา:**
|
||||
1. **Empty catch block**: มีการใช้ `catch {}` ว่างเปล่า ทำให้ไม่ทราบว่าเกิด Error 什么
|
||||
2. **Unhandled Promise rejection**: หาก axios.get throw exception ภายใน Promise.all อาจทำให้เกิด Unhandled Promise Rejection
|
||||
3. **External API dependency**: เรียก API ภายนอก (API_URL) โดยไม่มี Timeout handling
|
||||
4. **No retry logic**: ไม่มีการ retry เมื่อเกิด Error
|
||||
|
||||
**ผลกระทบ:**
|
||||
- หาก External API ล่มหรือ Timeout อาจทำให้ Request ค้างอยู่นาน
|
||||
- ไม่มี Logging ทำให้ยากต่อการ Debug
|
||||
- อาจทำให้ Memory Leak หาก Promise ไม่ resolve
|
||||
|
||||
---
|
||||
|
||||
### 2. **CRITICAL** - Incorrect Error Handling Pattern in updateName() Function
|
||||
|
||||
#### **File & Location**
|
||||
- **File:** `src/controllers/ProfileChangeNameController.ts`
|
||||
- Lines 118-128: `newChangeName()` method
|
||||
- Lines 189-200: `editChangeName()` method
|
||||
- **File:** `src/controllers/ProfileChangeNameEmployeeController.ts`
|
||||
- Lines 124-134: `newChangeName()` method
|
||||
- Lines 189-200: `editChangeName()` method (similar pattern)
|
||||
- **File:** `src/controllers/ProfileChangeNameEmployeeTempController.ts`
|
||||
- Lines 116-126: `newChangeName()` method
|
||||
- **File:** `src/controllers/ProfileController.ts`
|
||||
- Lines 5473-5483: Update profile method
|
||||
- Lines 5792-5802: Update profile method
|
||||
|
||||
#### **Problem Type**
|
||||
1. **Unhandled Exception**
|
||||
2. **Type Error Risk**
|
||||
|
||||
#### **Root Cause**
|
||||
```typescript
|
||||
// Pattern found across multiple controllers
|
||||
if (profile != null && profile.keycloak != null && profile.isDelete === false) {
|
||||
const result = await updateName(
|
||||
profile.keycloak,
|
||||
profile.firstName,
|
||||
profile.lastName,
|
||||
profile.prefix,
|
||||
);
|
||||
if (!result) {
|
||||
throw new Error(result.errorMessage); // ❌ CRITICAL BUG
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**รายละเอียดปัญหา:**
|
||||
1. **Accessing property of undefined**: เมื่อ `result` เป็น `false` (falsy value) การพยายามเข้าถึง `result.errorMessage` จะทำให้เกิด TypeError
|
||||
2. **Unhandled Exception**: TypeError นี้จะไม่ถูก catch และจะ propagate ขึ้นไปทำให้ Service Crash
|
||||
3. **Inconsistent return type**: ฟังก์ชัน `updateName()` ใน `src/keycloak/index.ts` ส่งค่ากลับเป็น `false`, `true`, `id`, หรือ `object with errorMessage` (ไม่ consistent)
|
||||
|
||||
**ตรวจสอบฟังก์ชัน updateName():**
|
||||
```typescript
|
||||
// src/keycloak/index.ts:525-533
|
||||
if (!res) return false;
|
||||
if (!res.ok) {
|
||||
return await res.json(); // Returns error object with errorMessage
|
||||
}
|
||||
const path = res.headers.get("Location");
|
||||
const id = path?.split("/").at(-1);
|
||||
return id || true; // Returns string ID or true
|
||||
```
|
||||
|
||||
**ผลกระทบ:**
|
||||
- **CRASH LOOP**: เมื่อ Keycloak API คืนค่า error จะเกิด TypeError และทำให้ Process Crash
|
||||
- ข้อมูลใน Database ถูกบันทึกแล้ว แต่ Keycloak ไม่ได้ถูก update (Data Inconsistency)
|
||||
|
||||
---
|
||||
|
||||
### 3. **HIGH** - Missing Error Handling in Promise.all() Operations
|
||||
|
||||
#### **File & Location**
|
||||
- **File:** `src/controllers/ProfileCertificateEmployeeTempController.ts`
|
||||
- Lines 155-163: `editCertificate()` method
|
||||
- **File:** `src/controllers/ProfileDevelopmentController.ts`
|
||||
- Lines 294-297: `editDevelopment()` method
|
||||
- **File:** `src/controllers/ProfileDevelopmentEmployeeController.ts`
|
||||
- Lines 237-240: `editDevelopment()` method
|
||||
|
||||
#### **Problem Type**
|
||||
1. **Missing Error Handle**
|
||||
2. **Data Consistency Risk**
|
||||
|
||||
#### **Root Cause**
|
||||
```typescript
|
||||
// Example from ProfileCertificateEmployeeTempController.ts:155-163
|
||||
await Promise.all([
|
||||
this.certificateRepo.save(record, { data: req }),
|
||||
setLogDataDiff(req, { before, after: record }),
|
||||
this.certificateHistoryRepository.save(history, { data: req }),
|
||||
]);
|
||||
```
|
||||
|
||||
**รายละเอียดปัญหา:**
|
||||
1. **Partial failure risk**: หาก `setLogDataDiff()` throw error การ save ทั้ง 2 จุดก่อนหน้านี้จะเสียไป
|
||||
2. **No transaction**: ไม่มีการใช้ Transaction ในการ save ข้อมูลหลายตาราง
|
||||
3. **Orphaned data**: อาจเกิดข้อมูลปนกันระหว่าง production และ history
|
||||
|
||||
---
|
||||
|
||||
### 4. **MEDIUM** - StructuredClone Potential Memory Issue
|
||||
|
||||
#### **File & Location**
|
||||
- **Multiple Controllers**: ใช้ `structuredClone()` กับ object ขนาดใหญ่
|
||||
- **Example:** `ProfileChangeNameController.ts:137`, `ProfileDevelopmentController.ts:349`
|
||||
|
||||
#### **Problem Type**
|
||||
1. **Memory Issue**
|
||||
2. **Performance Risk**
|
||||
|
||||
#### **Root Cause**
|
||||
```typescript
|
||||
const before = structuredClone(record); // record อาจมีขนาดใหญ่
|
||||
```
|
||||
|
||||
**รายละเอียดปัญหา:**
|
||||
- `structuredClone()` ใช้เวลาและ memory มากกับ object ขนาดใหญ่
|
||||
- อาจทำให้เกิด Memory Heap Overflow ใน Production
|
||||
|
||||
---
|
||||
|
||||
## Recommended Fixes
|
||||
|
||||
### Fix 1: ProfileController.ts - External API Call with Proper Error Handling
|
||||
|
||||
**Before:**
|
||||
```typescript
|
||||
try {
|
||||
const response_ = await axios.get(url, {
|
||||
headers: {
|
||||
Authorization: `${token_}`,
|
||||
"Content-Type": "application/json",
|
||||
api_key: process.env.API_KEY,
|
||||
},
|
||||
});
|
||||
_ImgUrl[i] = response_.data.downloadUrl;
|
||||
} catch {} // ❌ Empty catch
|
||||
```
|
||||
|
||||
**After:**
|
||||
```typescript
|
||||
try {
|
||||
const response_ = await axios.get(url, {
|
||||
headers: {
|
||||
Authorization: `${token_}`,
|
||||
"Content-Type": "application/json",
|
||||
api_key: process.env.API_KEY,
|
||||
},
|
||||
timeout: 5000, // Add timeout
|
||||
});
|
||||
_ImgUrl[i] = response_.data.downloadUrl;
|
||||
} catch (error) {
|
||||
console.error(`Failed to fetch avatar ${x?.avatar}:`, error.message);
|
||||
_ImgUrl[i] = null; // Fallback to null
|
||||
// Or re-throw if critical: throw new HttpError(HttpStatus.SERVICE_UNAVAILABLE, "Avatar service unavailable");
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Fix 2: Incorrect Error Handling Pattern - ALL Controllers
|
||||
|
||||
**Before:**
|
||||
```typescript
|
||||
const result = await updateName(
|
||||
profile.keycloak,
|
||||
profile.firstName,
|
||||
profile.lastName,
|
||||
profile.prefix,
|
||||
);
|
||||
if (!result) {
|
||||
throw new Error(result.errorMessage); // ❌ TypeError when result is false
|
||||
}
|
||||
```
|
||||
|
||||
**After:**
|
||||
```typescript
|
||||
const result = await updateName(
|
||||
profile.keycloak,
|
||||
profile.firstName,
|
||||
profile.lastName,
|
||||
profile.prefix,
|
||||
);
|
||||
|
||||
// Check result type properly
|
||||
if (result === false || (result && result.errorMessage)) {
|
||||
const errorMessage = result?.errorMessage || 'Failed to update name in Keycloak';
|
||||
console.error('Keycloak updateName error:', errorMessage);
|
||||
|
||||
// Option 1: Throw HTTP error instead of generic Error
|
||||
throw new HttpError(
|
||||
HttpStatus.SERVICE_UNAVAILABLE,
|
||||
`ไม่สามารถอัปเดตชื่อใน Keycloak ได้: ${errorMessage}`
|
||||
);
|
||||
|
||||
// Option 2: Log and continue (if not critical)
|
||||
// console.warn(`Keycloak update failed for user ${profile.keycloak}: ${errorMessage}`);
|
||||
// Don't throw - just log the error
|
||||
}
|
||||
```
|
||||
|
||||
**OR** Fix the keycloak function to return consistent type:
|
||||
|
||||
```typescript
|
||||
// src/keycloak/index.ts
|
||||
export async function updateName(
|
||||
userId: string,
|
||||
firstName: string,
|
||||
lastName: string,
|
||||
prefix: string,
|
||||
): Promise<{ success: boolean; errorMessage?: string }> {
|
||||
try {
|
||||
const existingUser = await getUser(userId);
|
||||
if (!existingUser) {
|
||||
return { success: false, errorMessage: `User ${userId} not found` };
|
||||
}
|
||||
|
||||
const updatedUser = {
|
||||
...existingUser,
|
||||
firstName,
|
||||
lastName,
|
||||
attributes: {
|
||||
...(existingUser.attributes || {}),
|
||||
prefix,
|
||||
},
|
||||
};
|
||||
|
||||
const res = await fetch(`${KC_URL}/admin/realms/${KC_REALMS}/users/${userId}`, {
|
||||
headers: {
|
||||
"authorization": `Bearer ${await getToken()}`,
|
||||
"content-type": `application/json`,
|
||||
},
|
||||
method: "PUT",
|
||||
body: JSON.stringify(updatedUser),
|
||||
});
|
||||
|
||||
if (!res.ok) {
|
||||
const errorData = await res.json();
|
||||
return { success: false, errorMessage: errorData.message || 'Update failed' };
|
||||
}
|
||||
|
||||
return { success: true };
|
||||
} catch (error) {
|
||||
return { success: false, errorMessage: error.message };
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Fix 3: Add Transaction Support for Multi-Table Operations
|
||||
|
||||
**Before:**
|
||||
```typescript
|
||||
await Promise.all([
|
||||
this.certificateRepo.save(record, { data: req }),
|
||||
setLogDataDiff(req, { before, after: record }),
|
||||
this.certificateHistoryRepository.save(history, { data: req }),
|
||||
]);
|
||||
```
|
||||
|
||||
**After:**
|
||||
```typescript
|
||||
try {
|
||||
await AppDataSource.transaction(async (transactionalEntityManager) => {
|
||||
await transactionalEntityManager.save(ProfileCertificate, record);
|
||||
await transactionalEntityManager.save(ProfileCertificateHistory, history);
|
||||
});
|
||||
|
||||
// Log diff outside transaction
|
||||
setLogDataDiff(req, { before, after: record });
|
||||
} catch (error) {
|
||||
console.error('Failed to save certificate:', error);
|
||||
throw new HttpError(
|
||||
HttpStatus.INTERNAL_SERVER_ERROR,
|
||||
'ไม่สามารถบันทึกข้อมูลได้ กรุณาลองใหม่'
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Fix 4: Add Global Error Handler for Unhandled Exceptions
|
||||
|
||||
**Create/Update `src/middlewares/error-handler.ts`:**
|
||||
```typescript
|
||||
import { Request, Response, NextFunction } from 'express';
|
||||
import HttpError from '../interfaces/http-error';
|
||||
|
||||
export function globalErrorHandler(
|
||||
err: Error,
|
||||
req: Request,
|
||||
res: Response,
|
||||
next: NextFunction
|
||||
) {
|
||||
console.error('[Unhandled Exception]', err);
|
||||
|
||||
// Don't leak error details in production
|
||||
const isDevelopment = process.env.NODE_ENV === 'development';
|
||||
|
||||
if (err instanceof HttpError) {
|
||||
return res.status(err.status).json({
|
||||
error: err.message,
|
||||
...(isDevelopment && { stack: err.stack })
|
||||
});
|
||||
}
|
||||
|
||||
// Handle TypeError from result.errorMessage pattern
|
||||
if (err instanceof TypeError && err.message.includes("errorMessage")) {
|
||||
return res.status(500).json({
|
||||
error: 'External service error',
|
||||
...(isDevelopment && { details: err.message })
|
||||
});
|
||||
}
|
||||
|
||||
// Generic error response
|
||||
res.status(500).json({
|
||||
error: 'Internal server error',
|
||||
...(isDevelopment && {
|
||||
message: err.message,
|
||||
stack: err.stack
|
||||
})
|
||||
});
|
||||
}
|
||||
|
||||
// Handle unhandled promise rejections
|
||||
export function setupUnhandledRejectionHandler() {
|
||||
process.on('unhandledRejection', (reason, promise) => {
|
||||
console.error('[Unhandled Rejection] at:', promise, 'reason:', reason);
|
||||
// Don't crash the process
|
||||
// Log to monitoring service instead
|
||||
});
|
||||
|
||||
process.on('uncaughtException', (error) => {
|
||||
console.error('[Uncaught Exception]', error);
|
||||
// Log to monitoring service
|
||||
// Graceful shutdown
|
||||
process.exit(1);
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Summary Statistics
|
||||
|
||||
| Issue Type | Count | Severity |
|
||||
|------------|-------|----------|
|
||||
| Unhandled External API Call | 2 | CRITICAL |
|
||||
| Incorrect Error Handling (TypeError Risk) | 8 | CRITICAL |
|
||||
| Missing Transaction Support | 6 | HIGH |
|
||||
| Silent Error Swallowing | 2 | MEDIUM |
|
||||
| Memory/Performance Risk | Multiple | MEDIUM |
|
||||
|
||||
---
|
||||
|
||||
## Files Requiring Immediate Attention
|
||||
|
||||
1. ✅ `src/controllers/ProfileController.ts` - CRITICAL (Line 484, 5473, 5792)
|
||||
2. ✅ `src/controllers/ProfileChangeNameController.ts` - CRITICAL (Line 118, 189)
|
||||
3. ✅ `src/controllers/ProfileChangeNameEmployeeController.ts` - CRITICAL (Line 124, 189)
|
||||
4. ✅ `src/controllers/ProfileChangeNameEmployeeTempController.ts` - CRITICAL (Line 116)
|
||||
5. ✅ `src/keycloak/index.ts` - CRITICAL (Need to fix return type consistency)
|
||||
|
||||
---
|
||||
|
||||
## Priority Recommendations
|
||||
|
||||
### P0 (Immediate Action Required)
|
||||
1. Fix the `result.errorMessage` TypeError pattern across all controllers
|
||||
2. Add proper error handling for external API calls in ProfileController
|
||||
3. Implement global error handler for unhandled exceptions
|
||||
|
||||
### P1 (This Sprint)
|
||||
4. Add transaction support for multi-table operations
|
||||
5. Implement retry logic for external API calls
|
||||
6. Add proper logging and monitoring
|
||||
|
||||
### P2 (Next Sprint)
|
||||
7. Review memory usage with structuredClone()
|
||||
8. Add circuit breaker pattern for external services
|
||||
9. Implement comprehensive error tracking
|
||||
|
||||
---
|
||||
|
||||
## Testing Recommendations
|
||||
|
||||
1. **Unit Tests**: Test error scenarios for Keycloak integration
|
||||
2. **Integration Tests**: Test external API failure scenarios
|
||||
3. **Load Tests**: Test memory usage with large profile data
|
||||
4. **Chaos Testing**: Test behavior when external services are down
|
||||
|
||||
---
|
||||
|
||||
**Report Generated:** 2026-05-08
|
||||
**Batch:** 08 (Controllers 71-80)
|
||||
**Total Files Analyzed:** 10
|
||||
**Critical Issues Found:** 8
|
||||
Loading…
Add table
Add a link
Reference in a new issue