AMM-1048 | Assam Pre Prod in MO Save and Frequency is not working #77
Conversation
pull latest code
latest pull
WalkthroughThe changes in this pull request involve significant updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
src/main/java/com/iemr/common/identity/controller/IdentityController.java (1)
700-720: Overall implementation is more robust and maintainable.The refactored method shows several improvements:
- Better type safety with direct array input
- Proper input validation
- Efficient stream-based transformation
- Clear logging
- Consistent error handling
However, consider adding:
- Request size limits to prevent memory issues
- Detailed error messages in the response
Consider adding a max size limit:
@PostMapping(path = "/getByBenRegIdList", headers = "Authorization") public String getBeneficiariesByBenRegIds( - @RequestBody Long[] benRegIds) { + @RequestBody Long[] benRegIds) { + final int MAX_IDS = 1000; // Adjust based on your requirements + if (benRegIds != null && benRegIds.length > MAX_IDS) { + return getErrorResponseString( + String.format("Too many IDs. Maximum allowed: %d", MAX_IDS), + 400, "error", "Too many IDs"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/com/iemr/common/identity/controller/IdentityController.java(1 hunks)src/main/java/com/iemr/common/identity/service/IdentityService.java(2 hunks)
🔇 Additional comments (4)
src/main/java/com/iemr/common/identity/controller/IdentityController.java (3)
700-701: Improved method signature for better type safety and clarity.
The change from accepting a JSON string to a strongly-typed Long[] array is a good improvement that:
- Reduces parsing overhead
- Provides compile-time type safety
- Makes the API contract clearer
703-706: Good defensive programming with input validation.
The null and empty array checks provide proper error handling with a descriptive message.
708-711: Consider handling potential overflow for very large IDs.
While the stream operation to convert Long to BigInteger is elegant, there's a potential risk when dealing with very large numbers.
Consider adding a validation check for the ID size:
BigInteger[] benRegIdArray = Arrays.stream(benRegIds)
+ .peek(id -> {
+ if (id > Long.MAX_VALUE/2) {
+ throw new IllegalArgumentException("Beneficiary ID too large: " + id);
+ }
+ })
.map(BigInteger::valueOf)
.toArray(BigInteger[]::new);src/main/java/com/iemr/common/identity/service/IdentityService.java (1)
1587-1587: Verify the impact of switching to getBeneficiariesDTONewForList
The change to use getBeneficiariesDTONewForList looks appropriate, but we should verify that all beneficiary data is correctly retrieved with the new method.
Run the following script to check for any differences in data retrieval between the old and new methods:
✅ Verification successful
The change to getBeneficiariesDTONewForList is safe and appropriate
After comparing both implementations, the new method getBeneficiariesDTONewForList is functionally equivalent to getBeneficiariesDTONew with one improvement: it handles the type conversion for registration ID more safely using BigInteger.valueOf(((Long) benMapArr[5]).longValue()) instead of direct casting ((BigInteger) benMapArr[5]).
Both methods:
- Accept the same input parameter type (Object[] benMapArr)
- Perform the same validation checks on array length and null values
- Use identical mappingRepo.getMapping calls with same parameters
- Set the same fields on MBeneficiarymapping object
- Handle RMNCH beneficiary details in the same way
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all usages of getBeneficiariesDTONew and getBeneficiariesDTONewForList
# to ensure consistent behavior across the codebase
# Search for all usages of both methods
echo "Searching for method usages..."
rg -A 2 "getBeneficiariesDTONew[^a-zA-Z]"
# Search for any error handling related to these methods
echo "Searching for related error handling..."
rg "catch.*getBeneficiariesDTO"
Length of output: 2967
Script:
#!/bin/bash
# Let's search for the implementation and usage of getBeneficiariesDTONewForList
# to compare it with getBeneficiariesDTONew
# Search for the implementation
echo "Searching for getBeneficiariesDTONewForList implementation..."
ast-grep --pattern 'private MBeneficiarymapping getBeneficiariesDTONewForList($$$) { $$$ }'
# Search for all usages
echo "Searching for all usages of getBeneficiariesDTONewForList..."
rg -A 3 "getBeneficiariesDTONewForList"
# Search for the context of the change
echo "Searching for the context of getBeneficiariesDeatilsByBenRegIdList..."
rg -A 10 "getBeneficiariesDeatilsByBenRegIdList"
Length of output: 7173
Script:
#!/bin/bash
# Let's compare the implementation of both methods to ensure they handle data similarly
# Search for getBeneficiariesDTONew implementation
echo "Searching for getBeneficiariesDTONew implementation..."
rg -A 25 "private MBeneficiarymapping getBeneficiariesDTONew\(Object\[\] benMapArr\)"
# Search for the mappingRepo.getMapping usage in both methods
echo "Searching for mappingRepo.getMapping usage..."
rg -B 2 -A 2 "mappingRepo.getMapping"
# Search for any test cases related to these methods
echo "Searching for test cases..."
rg -i "test.*getBeneficiariesDTO"
Length of output: 4660
| private MBeneficiarymapping getBeneficiariesDTONewForList(Object[] benMapArr) { | ||
| MBeneficiarymapping benMapOBJ = new MBeneficiarymapping(); | ||
| if (benMapArr != null && benMapArr.length == 12 && benMapArr[8] != null && benMapArr[9] != null) { | ||
| benMapOBJ.setBenMapId(getBigIntegerValueFromObject(benMapArr[0])); | ||
| benMapOBJ.setCreatedBy(String.valueOf(benMapArr[10])); | ||
| benMapOBJ.setCreatedDate((Timestamp) benMapArr[11]); | ||
| benMapOBJ = mappingRepo.getMapping(getBigIntegerValueFromObject(benMapArr[9]), (Integer) benMapArr[8]); | ||
|
|
||
| BigInteger regId = BigInteger.valueOf(((Long) benMapArr[5]).longValue()); | ||
| RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(regId); | ||
|
|
||
| // RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(((BigInteger) benMapArr[5])); | ||
| if (obj != null) { | ||
| if (obj.getHouseoldId() != null) | ||
| benMapOBJ.setHouseHoldID(obj.getHouseoldId()); | ||
| if (obj.getGuidelineId() != null) | ||
| benMapOBJ.setGuideLineID(obj.getGuidelineId()); | ||
| if (obj.getRchid() != null) | ||
| benMapOBJ.setRchID(obj.getRchid()); | ||
| } | ||
|
|
||
| } | ||
| return benMapOBJ; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add method documentation and improve error handling
The new method needs documentation and better error handling:
- Missing javadoc explaining the method's purpose and parameters
- Unsafe type casting from Long to BigInteger
- Insufficient null checks for RMNCH object properties
- Contains commented out code
Apply these improvements:
+ /**
+ * Creates a MBeneficiarymapping object from an array of beneficiary data
+ * @param benMapArr Array containing beneficiary mapping data
+ * @return MBeneficiarymapping object with populated data
+ * @throws IllegalArgumentException if required data is missing
+ */
private MBeneficiarymapping getBeneficiariesDTONewForList(Object[] benMapArr) {
MBeneficiarymapping benMapOBJ = new MBeneficiarymapping();
- if (benMapArr != null && benMapArr.length == 12 && benMapArr[8] != null && benMapArr[9] != null) {
+ if (benMapArr == null || benMapArr.length != 12 || benMapArr[8] == null || benMapArr[9] == null) {
+ throw new IllegalArgumentException("Invalid or incomplete beneficiary mapping data");
+ }
+ try {
benMapOBJ.setBenMapId(getBigIntegerValueFromObject(benMapArr[0]));
benMapOBJ.setCreatedBy(String.valueOf(benMapArr[10]));
benMapOBJ.setCreatedDate((Timestamp) benMapArr[11]);
benMapOBJ = mappingRepo.getMapping(getBigIntegerValueFromObject(benMapArr[9]), (Integer) benMapArr[8]);
- BigInteger regId = BigInteger.valueOf(((Long) benMapArr[5]).longValue());
+ BigInteger regId = getBigIntegerValueFromObject(benMapArr[5]);
RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(regId);
-
- // RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(((BigInteger) benMapArr[5]));
if (obj != null) {
- if (obj.getHouseoldId() != null)
- benMapOBJ.setHouseHoldID(obj.getHouseoldId());
- if (obj.getGuidelineId() != null)
- benMapOBJ.setGuideLineID(obj.getGuidelineId());
- if (obj.getRchid() != null)
- benMapOBJ.setRchID(obj.getRchid());
+ benMapOBJ.setHouseHoldID(obj.getHouseoldId());
+ benMapOBJ.setGuideLineID(obj.getGuidelineId());
+ benMapOBJ.setRchID(obj.getRchid());
}
- }
+ } catch (ClassCastException e) {
+ throw new IllegalArgumentException("Invalid data type in beneficiary mapping array", e);
+ }
return benMapOBJ;
}Committable suggestion skipped: line range outside the PR's diff.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return response; | ||
| @RequestBody Long[] benRegIds) { // Accepting an array of Longs directly | ||
| String logMessage = String.format("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = %s", Arrays.toString(benRegIds)) ; | ||
| logger.info(logMessage); |
Check notice
Code scanning / SonarCloud
Logging should not be vulnerable to injection attacks
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/main/java/com/iemr/common/identity/controller/IdentityController.java (1)
704-707: Robust input validationGood addition of null and empty array checks. However, we could enhance the validation further.
Consider adding a size limit check:
if (benRegIds == null || benRegIds.length == 0) { return getErrorResponseString("No beneficiary registration IDs provided", 400, "error", "Array is empty"); } +if (benRegIds.length > 1000) { // Adjust limit as needed + return getErrorResponseString("Too many beneficiary registration IDs", 400, "error", "Array size exceeds limit"); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/com/iemr/common/identity/controller/IdentityController.java(1 hunks)src/main/java/com/iemr/common/identity/service/IdentityService.java(2 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud
src/main/java/com/iemr/common/identity/controller/IdentityController.java
[notice] 702-702: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
🔇 Additional comments (3)
src/main/java/com/iemr/common/identity/controller/IdentityController.java (2)
700-700: Improved method signature for better type safety
The change from accepting a JSON string to a strongly-typed Long[] array is a good improvement that:
- Eliminates the need for JSON parsing
- Provides compile-time type checking
- Reduces potential runtime errors
709-713: Efficient array conversion using streams
Good use of Java streams for converting Long[] to BigInteger[]. The code is concise and readable.
src/main/java/com/iemr/common/identity/service/IdentityService.java (1)
1586-1586: Verify the impact of switching to getBeneficiariesDTONewForList
The change from getBeneficiariesDTONew to getBeneficiariesDTONewForList needs verification to ensure it maintains the same functionality.
Run the following script to verify the usage and impact:
✅ Verification successful
The change to getBeneficiariesDTONewForList is safe and appropriate
The only difference between the two methods is in how they handle the registration ID:
getBeneficiariesDTONewForListexplicitly casts the registration ID usingBigInteger.valueOf(((Long) benMapArr[5]).longValue())getBeneficiariesDTONewdirectly casts toBigInteger
This change appears to be a type-safety improvement for handling the registration ID when processing lists of beneficiaries. The rest of the functionality remains identical, including the mapping repository calls and RMNCH beneficiary details processing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other calls to getBeneficiariesDTONew that might need similar updates
# Search for other usages of getBeneficiariesDTONew
ast-grep --pattern 'getBeneficiariesDTONew($$$)'
# Search for test coverage of the changed method
rg -l "getBeneficiariesDeatilsByBenRegIdList" --type java --glob "*Test.java"
Length of output: 277
Script:
#!/bin/bash
# Let's try a different approach to search for the methods
# Search for getBeneficiariesDTONew method calls
rg "getBeneficiariesDTONew" --type java -A 2 -B 2
# Search for getBeneficiariesDTONewForList method calls
rg "getBeneficiariesDTONewForList" --type java -A 2 -B 2
# Search for getBeneficiariesDeatilsByBenRegIdList method
rg "getBeneficiariesDeatilsByBenRegIdList" --type java -A 2 -B 2
Length of output: 9315
Script:
#!/bin/bash
# Let's examine the implementation differences between these methods
# Get the implementation of both methods to compare
rg -A 30 "private MBeneficiarymapping getBeneficiariesDTONew\(" --type java
rg -A 30 "private MBeneficiarymapping getBeneficiariesDTONewForList\(" --type java
Length of output: 7283
| String logMessage = String.format("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = %s", Arrays.toString(benRegIds)) ; | ||
| logger.info(logMessage); |
There was a problem hiding this comment.
Security: Potential log injection vulnerability
The current logging implementation directly interpolates user input into the log message, which could be exploited for log injection attacks.
Apply this diff to sanitize the log message:
-String logMessage = String.format("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = %s", Arrays.toString(benRegIds)) ;
-logger.info(logMessage);
+logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = {}",
+ Arrays.toString(benRegIds.length > 100 ?
+ Arrays.copyOf(benRegIds, 100) : benRegIds));This change:
- Uses parameterized logging to prevent injection
- Limits the array size in logs to prevent excessive logging
📝 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.
| String logMessage = String.format("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = %s", Arrays.toString(benRegIds)) ; | |
| logger.info(logMessage); | |
| logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = {}", | |
| Arrays.toString(benRegIds.length > 100 ? | |
| Arrays.copyOf(benRegIds, 100) : benRegIds)); |
🧰 Tools
🪛 GitHub Check: SonarCloud
[notice] 702-702: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
| private MBeneficiarymapping getBeneficiariesDTONewForList(Object[] benMapArr) { | ||
| MBeneficiarymapping benMapOBJ = new MBeneficiarymapping(); | ||
| if (benMapArr != null && benMapArr.length == 12 && benMapArr[8] != null && benMapArr[9] != null) { | ||
| benMapOBJ.setBenMapId(getBigIntegerValueFromObject(benMapArr[0])); | ||
| benMapOBJ.setCreatedBy(String.valueOf(benMapArr[10])); | ||
| benMapOBJ.setCreatedDate((Timestamp) benMapArr[11]); | ||
| benMapOBJ = mappingRepo.getMapping(getBigIntegerValueFromObject(benMapArr[9]), (Integer) benMapArr[8]); | ||
|
|
||
| BigInteger regId = BigInteger.valueOf(((Long) benMapArr[5]).longValue()); | ||
| RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(regId); | ||
|
|
||
| if (obj != null) { | ||
| if (obj.getHouseoldId() != null) | ||
| benMapOBJ.setHouseHoldID(obj.getHouseoldId()); | ||
| if (obj.getGuidelineId() != null) | ||
| benMapOBJ.setGuideLineID(obj.getGuidelineId()); | ||
| if (obj.getRchid() != null) | ||
| benMapOBJ.setRchID(obj.getRchid()); | ||
| } | ||
|
|
||
| } | ||
| return benMapOBJ; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and reduce code duplication
The method has several issues that need to be addressed:
- Unsafe type casting from Object to Long to BigInteger could throw ClassCastException
- Nested null checks can be simplified
- Similar implementation to
getBeneficiariesDTONew- consider refactoring to reduce duplication
Consider applying these improvements:
+ /**
+ * Creates a MBeneficiarymapping object from an array of beneficiary data
+ * @param benMapArr Array containing beneficiary mapping data
+ * @return MBeneficiarymapping object with populated data
+ * @throws IllegalArgumentException if required data is missing or invalid
+ */
private MBeneficiarymapping getBeneficiariesDTONewForList(Object[] benMapArr) {
MBeneficiarymapping benMapOBJ = new MBeneficiarymapping();
- if (benMapArr != null && benMapArr.length == 12 && benMapArr[8] != null && benMapArr[9] != null) {
+ if (benMapArr == null || benMapArr.length != 12 || benMapArr[8] == null || benMapArr[9] == null) {
+ throw new IllegalArgumentException("Invalid or incomplete beneficiary mapping data");
+ }
+
+ try {
benMapOBJ.setBenMapId(getBigIntegerValueFromObject(benMapArr[0]));
benMapOBJ.setCreatedBy(String.valueOf(benMapArr[10]));
benMapOBJ.setCreatedDate((Timestamp) benMapArr[11]);
benMapOBJ = mappingRepo.getMapping(getBigIntegerValueFromObject(benMapArr[9]), (Integer) benMapArr[8]);
- BigInteger regId = BigInteger.valueOf(((Long) benMapArr[5]).longValue());
+ BigInteger regId = getBigIntegerValueFromObject(benMapArr[5]);
RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(regId);
if (obj != null) {
- if (obj.getHouseoldId() != null)
- benMapOBJ.setHouseHoldID(obj.getHouseoldId());
- if (obj.getGuidelineId() != null)
- benMapOBJ.setGuideLineID(obj.getGuidelineId());
- if (obj.getRchid() != null)
- benMapOBJ.setRchID(obj.getRchid());
+ benMapOBJ.setHouseHoldID(obj.getHouseoldId());
+ benMapOBJ.setGuideLineID(obj.getGuidelineId());
+ benMapOBJ.setRchID(obj.getRchid());
}
- }
+ } catch (ClassCastException e) {
+ throw new IllegalArgumentException("Invalid data type in beneficiary mapping array", e);
+ }
return benMapOBJ;
}Additionally, consider extracting common code between this method and getBeneficiariesDTONew into a shared utility method to reduce duplication.
📝 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 MBeneficiarymapping getBeneficiariesDTONewForList(Object[] benMapArr) { | |
| MBeneficiarymapping benMapOBJ = new MBeneficiarymapping(); | |
| if (benMapArr != null && benMapArr.length == 12 && benMapArr[8] != null && benMapArr[9] != null) { | |
| benMapOBJ.setBenMapId(getBigIntegerValueFromObject(benMapArr[0])); | |
| benMapOBJ.setCreatedBy(String.valueOf(benMapArr[10])); | |
| benMapOBJ.setCreatedDate((Timestamp) benMapArr[11]); | |
| benMapOBJ = mappingRepo.getMapping(getBigIntegerValueFromObject(benMapArr[9]), (Integer) benMapArr[8]); | |
| BigInteger regId = BigInteger.valueOf(((Long) benMapArr[5]).longValue()); | |
| RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(regId); | |
| if (obj != null) { | |
| if (obj.getHouseoldId() != null) | |
| benMapOBJ.setHouseHoldID(obj.getHouseoldId()); | |
| if (obj.getGuidelineId() != null) | |
| benMapOBJ.setGuideLineID(obj.getGuidelineId()); | |
| if (obj.getRchid() != null) | |
| benMapOBJ.setRchID(obj.getRchid()); | |
| } | |
| } | |
| return benMapOBJ; | |
| } | |
| /** | |
| * Creates a MBeneficiarymapping object from an array of beneficiary data | |
| * @param benMapArr Array containing beneficiary mapping data | |
| * @return MBeneficiarymapping object with populated data | |
| * @throws IllegalArgumentException if required data is missing or invalid | |
| */ | |
| private MBeneficiarymapping getBeneficiariesDTONewForList(Object[] benMapArr) { | |
| MBeneficiarymapping benMapOBJ = new MBeneficiarymapping(); | |
| if (benMapArr == null || benMapArr.length != 12 || benMapArr[8] == null || benMapArr[9] == null) { | |
| throw new IllegalArgumentException("Invalid or incomplete beneficiary mapping data"); | |
| } | |
| try { | |
| benMapOBJ.setBenMapId(getBigIntegerValueFromObject(benMapArr[0])); | |
| benMapOBJ.setCreatedBy(String.valueOf(benMapArr[10])); | |
| benMapOBJ.setCreatedDate((Timestamp) benMapArr[11]); | |
| benMapOBJ = mappingRepo.getMapping(getBigIntegerValueFromObject(benMapArr[9]), (Integer) benMapArr[8]); | |
| BigInteger regId = getBigIntegerValueFromObject(benMapArr[5]); | |
| RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(regId); | |
| if (obj != null) { | |
| benMapOBJ.setHouseHoldID(obj.getHouseoldId()); | |
| benMapOBJ.setGuideLineID(obj.getGuidelineId()); | |
| benMapOBJ.setRchID(obj.getRchid()); | |
| } | |
| } catch (ClassCastException e) { | |
| throw new IllegalArgumentException("Invalid data type in beneficiary mapping array", e); | |
| } | |
| return benMapOBJ; | |
| } |
There was a problem hiding this comment.
@srishtigrp78 As discussed in call Pls remove the new method to avoid duplicate code.
And make the changes like I suggested.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/com/iemr/common/identity/controller/IdentityController.java(1 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud
src/main/java/com/iemr/common/identity/controller/IdentityController.java
[notice] 702-702: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
🔇 Additional comments (3)
src/main/java/com/iemr/common/identity/controller/IdentityController.java (3)
700-701: LGTM: Improved method signature
The change from accepting a JSON string to directly accepting a Long[] array simplifies the API and reduces unnecessary JSON parsing overhead.
703-706: LGTM: Added proper input validation
Good addition of null and empty array checks with a clear error message.
708-711: Consider handling potential overflow during type conversion
While the conversion from Long to BigInteger is generally safe, extremely large numbers could potentially cause issues.
Consider adding a validation check for the maximum allowed value:
BigInteger[] benRegIdArray = Arrays.stream(benRegIds)
+ .filter(id -> id <= Long.MAX_VALUE && id > 0)
.map(BigInteger::valueOf)
.toArray(BigInteger[]::new);| logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = " + benRegIds.length); | ||
|
|
There was a problem hiding this comment.
Security: Fix log injection vulnerability
The current logging implementation directly interpolates user input into the log message, which could be exploited for log injection attacks.
Apply this diff to sanitize the log message:
-logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = " + benRegIds.length);
+logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList size = {}", benRegIds.length);📝 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.
| logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList = " + benRegIds.length); | |
| logger.info("IdentityController.getBeneficiariesByBenRegIds - start. benRegIdList size = {}", benRegIds.length); |
🧰 Tools
🪛 GitHub Check: SonarCloud
[notice] 702-702: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
| List<BeneficiariesDTO> list = svc.getBeneficiariesDeatilsByBenRegIdList(Arrays.asList(benRegIdArray)); | ||
| list.removeIf(Objects::isNull); | ||
| Collections.sort(list); | ||
|
|
||
| String response = getSuccessResponseString(list, 200, "success", "getBeneficiariesByBenRegIds"); | ||
|
|
||
| logger.info("IdentityController.getBeneficiariesByBenRegIds - end : "); | ||
| return response; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for service layer exceptions
The service call and response handling could benefit from more robust error handling.
Apply this diff to improve error handling:
- List<BeneficiariesDTO> list = svc.getBeneficiariesDeatilsByBenRegIdList(Arrays.asList(benRegIdArray));
- list.removeIf(Objects::isNull);
- Collections.sort(list);
-
- String response = getSuccessResponseString(list, 200, "success", "getBeneficiariesByBenRegIds");
-
- logger.info("IdentityController.getBeneficiariesByBenRegIds - end : ");
- return response;
+ try {
+ List<BeneficiariesDTO> list = svc.getBeneficiariesDeatilsByBenRegIdList(Arrays.asList(benRegIdArray));
+ if (list == null) {
+ return getErrorResponseString("No beneficiaries found", 404, "not_found", "getBeneficiariesByBenRegIds");
+ }
+ list.removeIf(Objects::isNull);
+ Collections.sort(list);
+
+ String response = getSuccessResponseString(list, 200, "success", "getBeneficiariesByBenRegIds");
+ logger.info("IdentityController.getBeneficiariesByBenRegIds - end");
+ return response;
+ } catch (Exception e) {
+ logger.error("Error retrieving beneficiaries: {}", e.getMessage(), e);
+ return getErrorResponseString("Error retrieving beneficiaries", 500, "error", "getBeneficiariesByBenRegIds");
+ }📝 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.
| List<BeneficiariesDTO> list = svc.getBeneficiariesDeatilsByBenRegIdList(Arrays.asList(benRegIdArray)); | |
| list.removeIf(Objects::isNull); | |
| Collections.sort(list); | |
| String response = getSuccessResponseString(list, 200, "success", "getBeneficiariesByBenRegIds"); | |
| logger.info("IdentityController.getBeneficiariesByBenRegIds - end : "); | |
| return response; | |
| try { | |
| List<BeneficiariesDTO> list = svc.getBeneficiariesDeatilsByBenRegIdList(Arrays.asList(benRegIdArray)); | |
| if (list == null) { | |
| return getErrorResponseString("No beneficiaries found", 404, "not_found", "getBeneficiariesByBenRegIds"); | |
| } | |
| list.removeIf(Objects::isNull); | |
| Collections.sort(list); | |
| String response = getSuccessResponseString(list, 200, "success", "getBeneficiariesByBenRegIds"); | |
| logger.info("IdentityController.getBeneficiariesByBenRegIds - end"); | |
| return response; | |
| } catch (Exception e) { | |
| logger.error("Error retrieving beneficiaries: {}", e.getMessage(), e); | |
| return getErrorResponseString("Error retrieving beneficiaries", 500, "error", "getBeneficiariesByBenRegIds"); | |
| } |
|
| private MBeneficiarymapping getBeneficiariesDTONewForList(Object[] benMapArr) { | ||
| MBeneficiarymapping benMapOBJ = new MBeneficiarymapping(); | ||
| if (benMapArr != null && benMapArr.length == 12 && benMapArr[8] != null && benMapArr[9] != null) { | ||
| benMapOBJ.setBenMapId(getBigIntegerValueFromObject(benMapArr[0])); | ||
| benMapOBJ.setCreatedBy(String.valueOf(benMapArr[10])); | ||
| benMapOBJ.setCreatedDate((Timestamp) benMapArr[11]); | ||
| benMapOBJ = mappingRepo.getMapping(getBigIntegerValueFromObject(benMapArr[9]), (Integer) benMapArr[8]); | ||
|
|
||
| BigInteger regId = BigInteger.valueOf(((Long) benMapArr[5]).longValue()); | ||
| RMNCHBeneficiaryDetailsRmnch obj = rMNCHBeneficiaryDetailsRmnchRepo.getByRegID(regId); | ||
|
|
||
| if (obj != null) { | ||
| if (obj.getHouseoldId() != null) | ||
| benMapOBJ.setHouseHoldID(obj.getHouseoldId()); | ||
| if (obj.getGuidelineId() != null) | ||
| benMapOBJ.setGuideLineID(obj.getGuidelineId()); | ||
| if (obj.getRchid() != null) | ||
| benMapOBJ.setRchID(obj.getRchid()); | ||
| } | ||
|
|
||
| } | ||
| return benMapOBJ; | ||
| } |
There was a problem hiding this comment.
@srishtigrp78 As discussed in call Pls remove the new method to avoid duplicate code.
And make the changes like I suggested.
📋 Description
JIRA ID:
AMM-1048 | Assam Pre Prod in MO Save and Frequency is not working
Linked issues- AMM-1059/AMM-1060
✅ 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
Bug Fixes
Documentation