hrms-bangkok/hrms-api-org
7
0
Fork 0
Code Issues Pull requests Projects Releases Packages 1 Wiki Activity Actions
hrms-api-org/reports/batch-13-controllers-121-130-analysis.md
DESKTOP-1R2VSQH\Lenovo ThinkPad E490 85e9be08f6 report: Controllers
2026-05-08 18:15:03 +07:00

25 KiB
Raw Blame History

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 - 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:

// 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
});

// 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:

// 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 - 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:

// 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
});

// 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:

// 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 - 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:

// Line 76-79 - No error handling for stored procedure call
const position = await AppDataSource.query("CALL GetProfileSalaryPosition(?, ?)", [
  x.id,
  _currentDate,
]);

Recommended Fix:

// 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 - 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:

// 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:

@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 - 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:

// Line 608-611 - DANGEROUS
salaryList.forEach(async (p, i) => {
  p.order = i + 1;
  await this.salaryRepo.save(p);  // Unhandled rejection
});

Recommended Fix:

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 - 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 - 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:

// Line 1493-1498 - Insufficient error handling
} catch (error) {
  await queryRunner.rollbackTransaction();
  throw error;  // No logging, no context
} finally {
  await queryRunner.release();
}

Recommended Fix:

} 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 - 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:

// Line 1425-1431 - Bulk insert without error handling
if (salaryRows.length) {
  await queryRunner.manager.insert(
    ProfileSalary,
    salaryRows.map(({ id, ...data }) => ({
      ...data,
      ...metaCreated,
    })),
  );
}

Recommended Fix:

// 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 - 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:

// 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:

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