Release 3.6.0 #99
Release 3.6.0 #99
Conversation
Bulk registration
Bulk registration
# Conflicts: # src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java
remove unwanted line
WalkthroughAdds XML-driven bulk user ingestion with per-row validation, error export, new signature activation endpoint, repository/service lookups for Aadhaar/contact, JWT ID-type change to Integer, tightened CORS/origin handling, new XML/Excel dependencies, and related model/service additions. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Controller as BulkRegistrationController
participant Service as BulkRegistrationServiceImpl
participant Xml as EmployeeXmlService
participant Repo as EmployeeMasterInter
participant DB as Database
User->>Controller: POST /bulkRegistration (XML, auth)
Controller->>Service: registerBulkUser(xml, auth, userName, serviceProviderID)
Service->>Xml: parseXml(xml)
Xml-->>Service: EmployeeList
loop each Employee
Service->>Service: validate fields (Aadhaar, email, DOB, contact, etc.)
alt valid
Service->>Repo: saveBulkUserEmployee(M_User1)
Repo->>DB: persist
DB-->>Repo: OK
Repo-->>Service: saved entity
else invalid
Service->>Service: record BulkRegistrationError
end
end
Service-->>Controller: summary (total, registered, errors)
Controller-->>User: JSON response
Note over User,Controller: Error export
User->>Controller: GET /download-error-sheet
Controller->>Service: insertErrorLog()
Service->>Service: generate Excel bytes
Service-->>Controller: byte[]
Controller-->>User: Excel file (application/octet-stream)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40–70 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| logger.info("JWT Token From Cookie: " + jwtToken); | ||
| logger.info("M_user Request: " + m_user); |
Check notice
Code scanning / SonarCloud
Logging should not be vulnerable to injection attacks Low
| mUser.setEmployeeID(employee.getUserName()); | ||
| mUser.setServiceProviderID(showuserRepo.findByUserName(createdBy).getServiceProviderID()); | ||
| mUser.setPassword(generateStrongPassword(employee.getPassword())); | ||
| logger.info("Register_user:" + mUser); |
Check notice
Code scanning / SonarCloud
Logging should not be vulnerable to injection attacks Low
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (18)
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.java (7)
38-38: Remove unused field.The
errorResponsefield is declared but never used in the controller.
41-41: Thread safety: Shared mutable state across requests.The class-level
responsemap is shared across all requests, creating thread-safety issues and potential data leaks between concurrent requests. Use a local variable instead.
59-63: CRITICAL: Do not log authentication credentials.Lines 59, 62-63 log the JWT token and Authorization header. These are sensitive credentials that must never be logged. If logs are compromised, attackers can use these tokens to impersonate users.
60-60: Avoid logging user-controlled data.Logging the raw
m_userpayload may expose PII and is vulnerable to log injection attacks.
65-65: Duplicate: Avoid logging user-controlled data.This is a duplicate log of
m_user(also logged at line 60) and carries the same risks.
66-88: Thread safety: Direct field manipulation violates encapsulation.Directly accessing and clearing public fields on
BulkRegistrationServiceImpl(lines 66, 72-79) is not thread-safe. Multiple concurrent requests could interfere with each other's data. Refactor the service to return an immutable result object.
90-109: Race condition in error sheet download.Clearing
bulkRegistrationErrorsafter file generation (lines 98-100) can cause issues with concurrent requests. One request might clear errors while another is still reading them. Consider atomically retrieving and clearing errors within the service method.src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (10)
306-306: Extract complex condition to a helper method.This 16-part conditional statement is difficult to read and maintain. Extract to a descriptive helper method like
hasAllRequiredFields(Employee).
46-69: Encapsulation: Public mutable fields violate thread safety.Public mutable collection fields (
bulkRegistrationErrors,errorLogs,m_bulkUser,m_UserDemographics,totalEmployeeListSize) break encapsulation and are not thread-safe. Make these private with synchronized access or return result objects.
80-80: Do not log PII.Logging the full employee list with
toString()exposes sensitive PII including contact numbers, addresses, Aadhaar numbers, and PAN numbers.Based on learnings
114-114: Do not log PII.Logging the full employee object exposes sensitive PII including contact numbers, addresses, Aadhaar, PAN, and email.
Based on learnings
228-231: Logic error: Inverted Aadhaar validation.Since
isValidAadharhas inverted logic (line 745-751), this condition adds an error when the Aadhaar is actually valid. After fixingisValidAadharto return true for valid numbers, negate this condition.
329-332: Logic error: Inverted Aadhaar condition.The condition negates
isValidAadharwhich is already inverted. After fixingisValidAadhar, remove the negation and add null-safety for the lookup result.
478-494: Hard-coded year will become obsolete.The year validation is hard-coded to 2025, making this code obsolete after that year. Use a dynamic value like
LocalDate.now().getYear()or a configurable maximum year.
507-538: Remove PII from debug logs.The
showLoggermethod logs extensive sensitive PII including contact numbers, email, Aadhaar, PAN, and addresses. Either remove this method entirely or sanitize all PII fields.Based on learnings
594-597: Implement proper designation lookup.The method returns a hard-coded value of 20 regardless of input. Implement proper lookup logic or document why this is intentionally hard-coded.
745-751: CRITICAL: Inverted Aadhaar validation logic.The method returns
truewhen the pattern does NOT match, which is backwards. This will accept invalid Aadhaar numbers and reject valid ones.Apply this diff:
public static boolean isValidAadhar(String aadharNumber) { + if (aadharNumber == null || aadharNumber.isEmpty()) { + return false; + } - if (!Pattern.matches("\\d{12}", aadharNumber)) { - return true; // Must be a 12-digit number - } - return false; + return Pattern.matches("\\d{12}", aadharNumber); }src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationService.java (1)
4-4: Method signature needs improvement.The method signature has formatting and design issues:
- Missing space after the first comma
- Parameter name
userdoesn't convey that it's an XML payloadvoidreturn type makes per-row error handling opaque to callersConsider returning a structured result (e.g.,
List<BulkRegistrationError>) so callers can inspect validation failures.
🧹 Nitpick comments (2)
src/main/java/com/iemr/admin/repo/employeemaster/V_ShowuserRepo.java (1)
26-26: Remove unused import.The
java.util.Listimport is not used anywhere in this file.Apply this diff:
-import java.util.List;src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (1)
107-107: Fix parameter spacing.Missing space after the comma between
authorizationandString createdBy.Apply this diff:
- private void saveUserUser(Employee employee, Integer row, String authorization,String createdBy) throws Exception { + private void saveUserUser(Employee employee, Integer row, String authorization, String createdBy) throws Exception {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.java(1 hunks)src/main/java/com/iemr/admin/repo/employeemaster/V_ShowuserRepo.java(2 hunks)src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationService.java(1 hunks)src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-13T06:01:56.090Z
Learnt from: SauravBizbRolly
Repo: PSMRI/Admin-API PR: 69
File: src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java:0-0
Timestamp: 2025-03-13T06:01:56.090Z
Learning: Sensitive personal identifiable information (PII) such as contact numbers, addresses, PAN numbers, and other personal details should not be logged in this Java codebase to maintain privacy and security standards.
Applied to files:
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.javasrc/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
🧬 Code graph analysis (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (1)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java (1)
Service(110-1240)
🔇 Additional comments (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (1)
346-347: Audit fields correctly use parameter.The audit fields now correctly use the
createdByparameter passed from the caller, which is good practice.Also applies to: 369-369
| @Query("SELECT u FROM V_Showuser u WHERE u.userName = :userName ORDER BY u.userName") | ||
| V_Showuser findByUserName(@Param("userName") String userName); |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure callers handle null returns.
The method returns a single V_Showuser entity. Callers must handle null if no user is found.
Verify that all call sites check for null before using the result:
🏁 Script executed:
#!/bin/bash
# Search for usages of findByUserName and check if null is handled
rg -nP -C5 '\.findByUserName\s*\(' --type=javaLength of output: 1870
Fix NullPointerException risk at BulkRegistrationServiceImpl line 350.
The method findByUserName() returns a non-optional entity that may be null. At line 350 in BulkRegistrationServiceImpl.java, the result is dereferenced without null checking:
mUser.setServiceProviderID(showuserRepo.findByUserName(createdBy).getServiceProviderID());If the user identified by createdBy does not exist, this will throw a NullPointerException. Add a null check or use Optional to handle this case before accessing getServiceProviderID().
🤖 Prompt for AI Agents
In src/main/java/com/iemr/admin/repo/employeemaster/V_ShowuserRepo.java around
lines 48-49, the repository method returns a possibly null V_Showuser causing a
NPE when dereferenced in BulkRegistrationServiceImpl at line 350; update the
repo method signature to return Optional<V_Showuser> (e.g. Optional<V_Showuser>
findByUserName(@Param("userName") String userName)) and update the query mapping
accordingly, then modify BulkRegistrationServiceImpl to call
showuserRepo.findByUserName(createdBy).ifPresentOrElse(u ->
mUser.setServiceProviderID(u.getServiceProviderID()), () ->
handleMissingUserScenario()) or use orElseThrow with a clear exception or
fallback behavior to avoid the null dereference.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (8)
114-114: Remove PII from logs.Logging the full employee object via
toString()exposes sensitive PII including contact numbers, Aadhaar, PAN, email, and addresses.Based on learnings
Apply this diff:
- logger.info("employee_list after for loop" + employee.toString()); + logger.info("Processing employee at row " + (row + 1) + " with username: " + employee.getUserName());
745-751: Critical: Inverted Aadhaar validation logic.The method returns
truewhen the Aadhaar pattern does not match, which is backwards. This causes valid Aadhaar numbers to be rejected and invalid ones to be accepted.Fix the inverted logic:
public static boolean isValidAadhar(String aadharNumber) { - if (!Pattern.matches("\\d{12}", aadharNumber)) { - return true; // Must be a 12-digit number + if (aadharNumber == null || aadharNumber.isEmpty()) { + return false; } - return false; + // Returns true if aadharNumber is exactly 12 digits + return Pattern.matches("\\d{12}", aadharNumber); }
594-597: Critical: Stub method returns hard-coded designation ID.The method always returns
20and ignores thedesignationStringparameter, causing all users to be assigned the same designation regardless of their actual role.Implement proper lookup logic:
public int getDesignationId(String designationString) { - return 20; + if (designationString == null || designationString.isEmpty()) { + logger.warn("Designation string is null or empty, using default ID 20"); + return 20; + } + + // Implement lookup via repository or service + // Example (adjust based on your actual repository): + // return designationRepo.findByName(designationString) + // .map(Designation::getId) + // .orElseThrow(() -> new IllegalArgumentException("Unknown designation: " + designationString)); + + logger.warn("Designation lookup not implemented for: " + designationString + ", using default ID 20"); + return 20; }
80-80: Remove PII from logs.Logging the full employee list via
toString()exposes sensitive PII including contact numbers, addresses, Aadhaar numbers, PAN numbers, and emails.Based on learnings
Apply this diff:
- logger.info("employee_list" + employeeList.getEmployees().toString()); + logger.info("Processing bulk registration with " + employeeList.getEmployees().size() + " employees");
46-69: Critical: Public mutable fields create race conditions in concurrent requests.These public mutable collection fields (
bulkRegistrationErrors,errorLogs,m_bulkUser,m_UserDemographics,totalEmployeeListSize) are shared across all requests because this is a singleton Spring service. Concurrent bulk registration calls will corrupt each other's data, leading to incorrect error reports and data loss.Make these fields request-scoped by either:
- Returning a result object from
registerBulkUsercontaining these collections, or- Using
ThreadLocalvariables (with proper cleanup), or- Making them method-local variables passed through the call chain
Apply this diff:
- public Integer totalEmployeeListSize = 0; - public static final String FILE_PATH = "error_log.xlsx"; - public List<BulkRegistrationError> bulkRegistrationErrors = new ArrayList<>(); - public ArrayList<String> errorLogs = new ArrayList<>(); - public ArrayList<M_User1> m_bulkUser = new ArrayList<>(); - public ArrayList<M_UserDemographics> m_UserDemographics = new ArrayList<>(); + private static final String FILE_PATH = "error_log.xlsx"; + + // Create a result class to hold per-request state + public static class BulkRegistrationResult { + private final List<BulkRegistrationError> bulkRegistrationErrors = new ArrayList<>(); + private final List<String> errorLogs = new ArrayList<>(); + private final List<M_User1> savedUsers = new ArrayList<>(); + private final List<M_UserDemographics> savedDemographics = new ArrayList<>(); + private int totalEmployeeListSize = 0; + // Add getters + }Then modify
registerBulkUserto returnBulkRegistrationResultand pass it through the call chain.
507-538: Critical: Debug method logs extensive PII.The
showLoggermethod logs sensitive PII including email (line 512), contact numbers (lines 513-514), Aadhaar (line 528), PAN (line 529), full addresses (lines 526-527), and dates of birth/joining (lines 535-536).Based on learnings
Either remove this method entirely if it's only for debugging, or sanitize all PII:
private void showLogger(Employee employee) { - logger.info("Title: " + employee.getTitle()); - logger.info("Title Id: " + getTitleId(employee.getTitle())); - logger.info("First Name: " + employee.getFirstName()); - logger.info("Last Name: " + employee.getLastName()); - logger.info("Email: " + employee.getEmail()); - logger.info("Contact No: " + employee.getContactNo()); - logger.info("Emergency Contact No: " + employee.getEmergencyContactNo()); - // ... (all other PII fields) + logger.debug("Processing employee - Username: " + employee.getUserName() + + ", Title: " + employee.getTitle() + + ", HasAadhaar: " + !employee.getAadhaarNo().isEmpty() + + ", HasPAN: " + !employee.getPan().isEmpty()); }Or simply remove the method if it's not needed.
329-332: Critical: Inverted Aadhaar validation with incorrect negation.Due to the inverted logic in
isValidAadhar(lines 745-751), this condition uses!isValidAadhar()which compounds the error. After fixingisValidAadharto returntruefor valid Aadhaar numbers, this negation must be removed.After fixing
isValidAadhar, update this condition:- if (!isValidAadhar(employee.getAadhaarNo()) && employeeMasterInter.FindEmployeeAadhaar(employee.getAadhaarNo()).equalsIgnoreCase("aadhaarnotexist")) { + if (isValidAadhar(employee.getAadhaarNo()) && employeeMasterInter.FindEmployeeAadhaar(employee.getAadhaarNo()).equalsIgnoreCase("aadhaarnotexist")) { mUser.setAadhaarNo(String.valueOf(employee.getAadhaarNo())); }
228-231: Critical: Inverted Aadhaar validation logic.The
isValidAadharmethod (lines 745-751) has inverted logic: it returnstruewhen the Aadhaar is invalid andfalsewhen valid. This condition at line 228 will incorrectly add an error for valid Aadhaar numbers.After fixing
isValidAadhar(see separate comment), update this condition:- if(isValidAadhar(employee.getAadhaarNo())){ + if(!isValidAadhar(employee.getAadhaarNo())){ validationErrors.add("Aadhaar number is invalid"); }
🧹 Nitpick comments (5)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (5)
306-306: Extract complex conditional to helper method.This 16-part conditional spanning multiple required fields reduces readability.
Extract to a descriptive helper method:
- if (!employee.getTitle().isEmpty() && !employee.getFirstName().isEmpty() && !employee.getLastName().isEmpty() && !employee.getContactNo().isEmpty() && !employee.getEmergencyContactNo().isEmpty() && !employee.getDob().isEmpty() && !employee.getUserName().isEmpty() && !employee.getPassword().isEmpty() && !employee.getState().isEmpty() && !employee.getDistrict().isEmpty() && !employee.getPermanentState().isEmpty() && !employee.getPermanentDistrict().isEmpty() && !employee.getGender().isEmpty() && !employee.getQualification().isEmpty() && isValidDate(convertStringIntoDate(employee.getDob()).toString()) && isValidDate(convertStringIntoDate(employee.getDateOfJoining()).toString())) { + if (hasAllRequiredFieldsValid(employee) && validationErrors.isEmpty()) {Add helper method:
private boolean hasAllRequiredFieldsValid(Employee employee) { return !employee.getTitle().isEmpty() && !employee.getFirstName().isEmpty() && !employee.getLastName().isEmpty() && !employee.getContactNo().isEmpty() && !employee.getEmergencyContactNo().isEmpty() && !employee.getDob().isEmpty() && !employee.getUserName().isEmpty() && !employee.getPassword().isEmpty() && !employee.getState().isEmpty() && !employee.getDistrict().isEmpty() && !employee.getPermanentState().isEmpty() && !employee.getPermanentDistrict().isEmpty() && !employee.getGender().isEmpty() && !employee.getQualification().isEmpty() && isValidDate(convertStringIntoDate(employee.getDob()).toString()) && isValidDate(convertStringIntoDate(employee.getDateOfJoining()).toString()); }
358-358: Hard-coded role ID reduces flexibility.The hard-coded
roleIDvalue of 122 makes the code inflexible and harder to maintain across environments.Make this configurable via
application.properties:@Value("${bulk.registration.default.roleId:122}") private Integer defaultRoleId;Then use:
- m_userServiceRoleMapping.setRoleID(122); + m_userServiceRoleMapping.setRoleID(defaultRoleId);
107-472: Method is too long and violates single responsibility principle.The
saveUserUsermethod is 365 lines long and handles validation, user creation, demographics mapping, role mapping, and persistence. This makes it difficult to test, understand, and maintain.Break this into smaller, focused methods:
private void saveUserUser(Employee employee, Integer row, String authorization, String createdBy) throws Exception { List<String> validationErrors = validateEmployee(employee); if (!validationErrors.isEmpty()) { recordValidationErrors(employee, row, validationErrors); return; } if (!hasAllRequiredFieldsValid(employee)) { return; } try { M_User1 user = createUser(employee, createdBy); M_UserDemographics demographics = createDemographics(user, employee, createdBy); M_UserServiceRoleMapping2 roleMapping = createRoleMapping(user, createdBy); persistUserData(user, demographics, roleMapping, authorization); } catch (Exception e) { recordPersistenceError(employee, row, e); } } private List<String> validateEmployee(Employee employee) { // All validation logic (lines 121-291) } private void recordValidationErrors(Employee employee, int row, List<String> errors) { // Error recording logic } private M_User1 createUser(Employee employee, String createdBy) { // User creation logic (lines 309-353) } private M_UserDemographics createDemographics(M_User1 user, Employee employee, String createdBy) { // Demographics creation logic (lines 359-413) } private M_UserServiceRoleMapping2 createRoleMapping(M_User1 user, String createdBy) { // Role mapping logic (lines 355-358) } private void persistUserData(M_User1 user, M_UserDemographics demographics, M_UserServiceRoleMapping2 roleMapping, String authorization) { // Persistence calls (lines 414-417) }
478-494: Hard-coded year 2025 will become obsolete.The hard-coded maximum year of 2025 requires annual maintenance and will cause valid dates to be rejected in the future.
Make the year dynamic:
private boolean isValidDate(String dateStr) { try { String[] parts = dateStr.split("-"); int year = Integer.parseInt(parts[0]); + int currentYear = LocalDate.now().getYear(); - if (year > 2025) { - return false; // Year should not be greater than 2025 + if (year > currentYear + 1) { + return false; // Allow dates up to next year } DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd"); LocalDate.parse(dateStr, formatter); return true; } catch (Exception e) { return false; } }
767-767: Remove unused variable.The variable
rowIndex2is declared but never used.Apply this diff:
int rowIndex = 1; // Start from row 1 (row 0 is header) - int rowIndex2 = 1; // Start from row 1 (row 0 is header) logger.info("Error Size" + bulkRegistrationErrors.size());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-13T06:01:56.090Z
Learnt from: SauravBizbRolly
Repo: PSMRI/Admin-API PR: 69
File: src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java:0-0
Timestamp: 2025-03-13T06:01:56.090Z
Learning: Sensitive personal identifiable information (PII) such as contact numbers, addresses, PAN numbers, and other personal details should not be logged in this Java codebase to maintain privacy and security standards.
Applied to files:
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
🧬 Code graph analysis (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (3)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java (1)
Service(110-1240)src/main/java/com/iemr/admin/service/bulkRegistration/EmployeeXmlService.java (1)
Service(8-21)src/main/java/com/iemr/admin/service/rolemaster/Role_Master_ServiceImpl.java (1)
Service(48-387)
| mUser.setEmployeeID(employee.getUserName()); | ||
| mUser.setServiceProviderID(showuserRepo.findByUserName(createdBy).getServiceProviderID()); | ||
| mUser.setPassword(generateStrongPassword(employee.getPassword())); | ||
| logger.info("Register_user:" + mUser); |
There was a problem hiding this comment.
Potential PII logging.
Logging the full mUser object may expose PII fields such as contact numbers, Aadhaar, PAN, and email.
Apply this diff:
- logger.info("Register_user:" + mUser);
+ logger.info("Registering user with ID: " + mUser.getUserID() + " and username: " + mUser.getUserName());🤖 Prompt for AI Agents
In
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
around line 352, the logger.info call currently prints the entire mUser object
which may contain PII (contact numbers, Aadhaar, PAN, email); replace this by
logging only non-sensitive identifiers (e.g., userId, username) and/or
explicitly masked versions of any fields you must show (mask digits of
contact/Aadhaar/PAN and partially mask email), or remove the log entirely;
update the log message to include clear context (e.g., "Register_user id=...,
username=...") and ensure no full object or unmasked sensitive fields are
logged.
| public static Date convertStringIntoDate(String date) { | ||
|
|
||
| final long MILLISECONDS_PER_DAY = 86400000L; | ||
| final long EPOCH_OFFSET = 2209161600000L; | ||
|
|
||
| // Calculate milliseconds since epoch | ||
| long javaMillis = (long) (Double.parseDouble(date) * MILLISECONDS_PER_DAY - EPOCH_OFFSET); | ||
|
|
||
| return new Date(javaMillis); | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
Add error handling for date conversion.
The method assumes the input is an Excel serial number (numeric string) but doesn't validate this assumption. If the input is in a different format (e.g., "2025-01-15"), it will throw NumberFormatException.
Add validation and error handling:
public static Date convertStringIntoDate(String date) {
+ if (date == null || date.isEmpty()) {
+ throw new IllegalArgumentException("Date string cannot be null or empty");
+ }
+
+ try {
final long MILLISECONDS_PER_DAY = 86400000L;
final long EPOCH_OFFSET = 2209161600000L;
long javaMillis = (long) (Double.parseDouble(date) * MILLISECONDS_PER_DAY - EPOCH_OFFSET);
return new Date(javaMillis);
-
+ } catch (NumberFormatException e) {
+ throw new IllegalArgumentException("Invalid date format. Expected Excel serial number, got: " + date, e);
+ }
}🤖 Prompt for AI Agents
In
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
around lines 702 to 713, the convertStringIntoDate method assumes the input is
an Excel serial numeric string and directly parses it to a double which causes
NumberFormatException for other formats; update the method to first check for
null/empty, then attempt parsing the string as a double inside a try-catch to
convert Excel serial to milliseconds, and if NumberFormatException is thrown
fall back to parsing common date string formats (e.g., ISO_LOCAL_DATE and
ISO_LOCAL_DATE_TIME) using java.time (LocalDate/LocalDateTime -> Date), and if
all parsing attempts fail throw a clear IllegalArgumentException or return null
as per project convention, ensuring any caught exceptions are logged with
contextual information before rethrowing or returning.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (8)
338-351: Consider making default IDs configurable.Hard-coded values for
maritalStatusID,statusID, andserviceProviderIDreduce flexibility. If these defaults change or vary by environment/deployment, the code will require modification.Extract these to application properties or constructor parameters:
// In application.properties: // bulk.registration.default.maritalStatusID=1 // bulk.registration.default.statusID=2 // bulk.registration.default.serviceProviderID=13 @Value("${bulk.registration.default.maritalStatusID}") private Integer defaultMaritalStatusID; @Value("${bulk.registration.default.statusID}") private Integer defaultStatusID; @Value("${bulk.registration.default.serviceProviderID}") private Integer defaultServiceProviderID;Then use these fields instead of literals.
356-359: Remove or document commented-out code.Commented code for role mapping is present but unused. If this feature is planned, add a TODO comment; otherwise, remove it to reduce clutter.
-// m_userServiceRoleMapping.setUserID(bulkUserID.getUserID()); -// m_userServiceRoleMapping.setServiceProviderID(bulkUserID.getServiceProviderID()); -// m_userServiceRoleMapping.setCreatedBy(createdBy); -// m_userServiceRoleMapping.setRoleID(122); + // TODO: Add role mapping support in future iterationOr remove entirely if not needed.
542-557: Use logger instead of System.out.println for consistency.Lines 551 and 553 use
System.out.printlnwhile the rest of the class useslogger. This creates inconsistent logging output.if (genderId == 0) { - System.out.println("Gender name not found: " + genderString); + logger.warn("Gender name not found: " + genderString); } else { - System.out.println("Gender ID: " + genderId); + logger.debug("Gender ID: " + genderId); }
559-566: Add null safety to isNumeric.The method will throw
NullPointerExceptionifstris null. Add a null/empty check for robustness.public static boolean isNumeric(String str) { + if (str == null || str.isEmpty()) { + return false; + } try { Integer.parseInt(str); return true; } catch (NumberFormatException e) { return false; } }
660-676: Use logger instead of System.out.println.Line 671 uses
System.out.printlninstead of the class logger, creating inconsistent logging.if (stateId == 0) { logger.info("State name not found: " + stateName); } else { - System.out.println("State ID: " + stateId); + logger.debug("State ID: " + stateId); m_districts = locationMasterServiceInter.getAllDistrictByStateId(stateId); }
683-692: Remove or replace debug print statement.Line 685 contains a debug
System.out.printlnthat should be removed or replaced with proper logging.private boolean isValidPhoneNumber(String phoneNumber) { - // Correct regex: Phone number must start with 6, 7, 8, or 9 and be exactly 10 digits - System.out.println("Phone Number" + phoneNumber); String regex = "^[6789]\\d{9}$";
479-495: Add null safety to isValidDate.The method will throw
NullPointerExceptionifdateStris null. Add validation at the start of the method.private boolean isValidDate(String dateStr) { + if (dateStr == null || dateStr.isEmpty()) { + return false; + } try { String[] parts = dateStr.split("-");
755-796: Use logger instead of System.out.println.Line 788 uses
System.out.printlnfor a success message. Use the class logger for consistency.workbook.write(baos); workbook.close(); - System.out.println("Error log generated successfully!"); + logger.info("Error log generated successfully!"); } catch (IOException e) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-13T06:01:56.090Z
Learnt from: SauravBizbRolly
Repo: PSMRI/Admin-API PR: 69
File: src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java:0-0
Timestamp: 2025-03-13T06:01:56.090Z
Learning: Sensitive personal identifiable information (PII) such as contact numbers, addresses, PAN numbers, and other personal details should not be logged in this Java codebase to maintain privacy and security standards.
Applied to files:
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
🧬 Code graph analysis (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (3)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java (1)
Service(110-1240)src/main/java/com/iemr/admin/service/bulkRegistration/EmployeeXmlService.java (1)
Service(8-21)src/main/java/com/iemr/admin/service/rolemaster/Role_Master_ServiceImpl.java (1)
Service(48-387)
🔇 Additional comments (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (1)
101-104: Verify XML escaping is complete.The method only escapes
&characters. If the XML input contains unescaped<,>,", or'in text content, they will not be handled and may cause parsing errors or security issues.Confirm whether the XML source guarantees that other special characters are already escaped, or add escaping for all XML special characters:
public static String escapeXmlSpecialChars(String xml) { - // Only escape & that are not already part of valid XML entities - return xml.replaceAll("&(?!amp;|lt;|gt;|apos;|quot;|#\\d+;)", "&"); + // Escape all XML special characters + return xml.replaceAll("&(?!amp;|lt;|gt;|apos;|quot;|#\\d+;)", "&") + .replace("<", "<") + .replace(">", ">") + .replace("\"", """) + .replace("'", "'"); }
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.java (1)
43-88: Previous review comments still apply.The critical issues identified in past reviews remain valid:
- Unused field at line 38
- Security vulnerability with authorization token logging (marked as addressed but should be verified)
- Thread safety issues with shared mutable state (lines 41, 66-79, 86)
- Log injection vulnerabilities (lines 60, 65)
Based on learnings
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (3)
224-227: Inverted Aadhaar validation logic.The
isValidAadharmethod has inverted logic (returns true when invalid). This causes line 224 to add an error when Aadhaar is actually valid.This issue was flagged in previous reviews and remains unresolved. Lines 325-328 and 742-748 have the same problem.
76-76: Remove PII from logs.Logging the full employee list exposes sensitive information including contact numbers, Aadhaar, and PAN.
Based on learnings
Replace with a non-PII summary as recommended in previous reviews.
110-110: Remove PII from logs.Logging the full employee object exposes sensitive PII.
Based on learnings
This issue was marked as addressed in commit 67a4240 but the problematic logging remains in the current code.
🧹 Nitpick comments (5)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (5)
334-334: Hard-coded marital status reduces flexibility.The marital status is always set to 1 regardless of the employee data.
Consider adding a maritalStatus field to the Employee XML schema and mapping it here, or make this value configurable if it's intentionally a default.
330-333: PAN validation method defined but never used.The
isValidPANmethod exists (lines 674-677) but PAN numbers are accepted without validation.Apply this diff to validate PAN when provided:
if (!employee.getPan().isEmpty()) { + if (!isValidPAN(employee.getPan())) { + validationErrors.add("Invalid PAN format"); + } mUser.setpAN(employee.getPan()); }
346-346: Redundant assignment of employeeID.EmployeeID is set twice at lines 310 and 346 with the same value.
Remove the duplicate assignment at line 346.
352-355: Commented-out role mapping code suggests incomplete feature.The role mapping logic is commented out, indicating the feature may be incomplete or intentionally disabled.
Do you want me to help implement the role mapping feature or open an issue to track this work?
547-547: Use logger instead of System.out.println.Console output at lines 547 and 667 should use the configured logger for consistency and proper log management.
Replace
System.out.printlnwithlogger.debugorlogger.info.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.java(1 hunks)src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationService.java(1 hunks)src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationService.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-13T06:01:56.090Z
Learnt from: SauravBizbRolly
Repo: PSMRI/Admin-API PR: 69
File: src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java:0-0
Timestamp: 2025-03-13T06:01:56.090Z
Learning: Sensitive personal identifiable information (PII) such as contact numbers, addresses, PAN numbers, and other personal details should not be logged in this Java codebase to maintain privacy and security standards.
Applied to files:
src/main/java/com/iemr/admin/controller/bulkRegistration/BulkRegistrationController.javasrc/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
🧬 Code graph analysis (1)
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java (3)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java (1)
Service(110-1240)src/main/java/com/iemr/admin/service/bulkRegistration/EmployeeXmlService.java (1)
Service(8-21)src/main/java/com/iemr/admin/service/rolemaster/Role_Master_ServiceImpl.java (1)
Service(48-387)
| mUser.setUserName(employee.getContactNo()); | ||
| mUser.setdOB(convertStringIntoDate(employee.getDob())); | ||
| mUser.setEmployeeID(employee.getUserName()); |
There was a problem hiding this comment.
Username and EmployeeID assignment appears reversed.
Line 308 sets userName to the contact number, while line 310 sets employeeID to the username field. This seems backwards—typically userName should be the user's username, not their phone number.
Consider this fix:
- mUser.setUserName(employee.getContactNo());
+ mUser.setUserName(employee.getUserName());
mUser.setdOB(convertStringIntoDate(employee.getDob()));
- mUser.setEmployeeID(employee.getUserName());
+ mUser.setEmployeeID(employee.getContactNo());🤖 Prompt for AI Agents
In
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
around lines 308 to 310, the assignments are swapped: userName is being set to
employee.getContactNo() and employeeID is set to employee.getUserName(), which
is likely reversed. Change the assignments so
mUser.setUserName(employee.getUserName()) and
mUser.setEmployeeID(employee.getEmployeeID() or employee.getEmpId() — if
employee has no explicit employeeID field use the appropriate identifier field),
keeping the DOB line as-is; ensure you pick the correct source field for
employeeID from the Employee object.
| } | ||
| mUserDemographics.setIsPermanent(false); | ||
| if (!employee.getPermanentPincode().isEmpty()) { | ||
| mUserDemographics.setPermPinCode(Integer.valueOf(employee.getPermanentPincode())); |
There was a problem hiding this comment.
Inconsistent pincode type handling.
Permanent pincode is stored as Integer (line 382), but current pincode is stored as String (line 408). This inconsistency may cause issues downstream.
Standardize on one type. If the database field is String, apply this diff:
if (!employee.getPermanentPincode().isEmpty()) {
- mUserDemographics.setPermPinCode(Integer.valueOf(employee.getPermanentPincode()));
+ mUserDemographics.setPermPinCode(employee.getPermanentPincode());
}🤖 Prompt for AI Agents
In
src/main/java/com/iemr/admin/service/bulkRegistration/BulkRegistrationServiceImpl.java
around lines 382 and 408, the permanent pincode is being set as an Integer while
the current pincode is set as a String; standardize both to the same type to
avoid downstream issues—if the DB field is a String, change the permanent
pincode assignment to use the String value (remove Integer.valueOf and pass
employee.getPermanentPincode()), and ensure the mUserDemographics
model/getter/setter uses String for permPincode; alternatively, if the DB
expects Integer, convert the current pincode handling to Integer and update the
model accordingly so both use Integer consistently.
Release 3.6.0
fix: amm-1927 send headers only if the request is from the allowed origin
Release 3.5.0
Release 3.6.1
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
157-161: Critical: Duplicate error logging and response - will cause IllegalStateException.Lines 160-161 duplicate lines 157-158. The second
response.sendError()call will throwIllegalStateExceptionbecause the response is already committed after the firstsendError().logger.warn("No valid authentication token found"); response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Unauthorized: Invalid or missing token"); - - logger.warn("No valid authentication token found"); - response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Unauthorized: Invalid or missing token");
🧹 Nitpick comments (3)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (3)
40-46: Consider reducing debug log verbosity for security.Logging all allowed origins in every request (line 46) could expose security configuration in logs. Consider logging this once at startup instead, or removing it from per-request logging.
logger.debug("Incoming Origin: {}", origin); logger.debug("Request Method: {}", method); logger.debug("Request URI: {}", uri); - logger.debug("Allowed Origins Configured: {}", allowedOrigins);
48-66: Origin validation logic is sound but creates redundancy with later checks.The early validation at lines 48-66 correctly blocks unauthorized origins. However, this means the
elseblock at lines 82-88 (which handles OPTIONS with invalid origin) is unreachable—if origin is invalid, we never reach line 72 because we already returned at line 52 or 57.This redundancy isn't incorrect but adds unnecessary code complexity.
82-89: Unreachable code for OPTIONS with invalid origin.Since lines 48-58 already return early for OPTIONS requests with null or invalid origins, this
elseblock handling OPTIONS at line 85-88 will never execute. Consider removing the dead code:} else { logger.warn("Origin [{}] is NOT allowed. CORS headers NOT added.", origin); - - if ("OPTIONS".equalsIgnoreCase(method)) { - response.sendError(HttpServletResponse.SC_FORBIDDEN, "Origin not allowed for OPTIONS request"); - return; - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pom.xml(5 hunks)src/main/java/com/iemr/admin/config/CorsConfig.java(1 hunks)src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java(2 hunks)src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java(5 hunks)src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java
- pom.xml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T11:55:39.499Z
Learnt from: 5Amogh
Repo: PSMRI/Admin-API PR: 110
File: src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java:153-166
Timestamp: 2025-11-17T11:55:39.499Z
Learning: In the Admin-API Java codebase, small self-contained utility methods like `isOriginAllowed` can be tolerated as duplicates across classes (e.g., HTTPRequestInterceptor and JwtUserIdValidationFilter) if it improves local readability and avoids unnecessary abstraction.
Applied to files:
src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.javasrc/main/java/com/iemr/admin/config/CorsConfig.javasrc/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java
🔇 Additional comments (5)
src/main/java/com/iemr/admin/config/CorsConfig.java (1)
22-24: LGTM - Explicit headers and PATCH method are good security improvements.Replacing wildcard
*with an explicit header list and addingPATCHaligns with least-privilege CORS practices. The four case variations ofserverAuthorizationappear intentional for compatibility with clients that may not normalize header case, which is acceptable defensive coding.src/main/java/com/iemr/admin/utils/http/HTTPRequestInterceptor.java (3)
47-48: Field injection for CORS configuration is appropriate.The
@Valueinjection forallowedOriginsproperly externalizes the configuration and enables environment-specific CORS policies.
112-118: LGTM - Conditional CORS headers on error responses.Good security practice to validate the origin before adding CORS headers on error responses, preventing unauthorized origins from receiving credentialed responses.
153-167: Origin validation logic looks correct.The regex transformation handles wildcard patterns appropriately, and the localhost port restriction (
\\d+) is a good security measure. Per learnings, duplication of this helper acrossHTTPRequestInterceptorandJwtUserIdValidationFilteris acceptable for local readability.src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (1)
169-176: LGTM - Clean CORS header helper.The
addCorsHeadersmethod properly encapsulates CORS header setting and correctly uses the validated origin instead of a wildcard.
| }); | ||
| } | ||
|
|
||
| private boolean isMobileClient(String userAgent) { | ||
| } private boolean isMobileClient(String userAgent) { |
There was a problem hiding this comment.
Formatting error: Missing newline between methods.
The closing brace of isOriginAllowed and the declaration of isMobileClient are on the same line (line 195), which violates standard Java formatting and hinders readability.
- } private boolean isMobileClient(String userAgent) {
+ }
+
+ private boolean isMobileClient(String userAgent) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }); | |
| } | |
| private boolean isMobileClient(String userAgent) { | |
| } private boolean isMobileClient(String userAgent) { | |
| }); | |
| } | |
| private boolean isMobileClient(String userAgent) { |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java around
lines 194 to 195, the closing brace of isOriginAllowed and the declaration of
isMobileClient are on the same line; fix by placing the closing brace on its own
line and starting the private boolean isMobileClient(...) declaration on the
next line (i.e., add a newline between the methods) to restore standard Java
formatting and readability.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java (1)
78-90: Avoid raw JSON Strings in services; add validation and domain-level error handling.This method still takes a raw JSON string, parses it with
JSONObject, and throws a genericIllegalArgumentExceptionon not-found; it also doesn’t handle malformed/partial JSON or null input, which can easily turn bad client data into 500s instead of clear 4xx responses. Prefer a typed request/DTO at the controller layer (with bean validation) and a service method taking strongly-typed arguments (e.g.,Long userId, boolean active), and use a domain-specific not-found exception and/or consistent audit updates.
🧹 Nitpick comments (2)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java (2)
24-30: Remove duplicateJSONObjectimport.
org.json.JSONObjectis imported twice (lines 24 and 30); keep a single import to avoid clutter.
43-48: Clean up commented‑out legacy code.The large commented block in
uploadSignatureis dead code and was also flagged by Sonar; consider removing it or replacing it with a brief explanatory comment if the logic is still relevant historically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java (1)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeMasterServiceImpl.java (1)
Service(110-1240)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java
[warning] 82-82: This block of commented-out lines of code should be removed.
🔇 Additional comments (2)
src/main/java/com/iemr/admin/service/employeemaster/EmployeeSignatureServiceImpl.java (2)
67-70: Formatting-only change inexistSignaturelooks fine.The comparison using
> 0is clear and preserves existing behavior.
72-74:isSignatureActiveimplementation is consistent with repo semantics.Using
countByUserIDAndSignatureNotNullAndDeletedFalse(...) > 0correctly reflects “has at least one non-deleted signature”; this looks good and aligns with the intended status check.
📋 Description
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.