Release 3.6.0 into main #128
Conversation
* AMM-1217 * removed unused imports * removed unused imports * AMM-1252 * swagger changes * bug(Identity-API) Edit family tagging null condition added * Jwttoken implementation and Crossorigin changes. * code rabbit comments addressed * coderabbitai comments addressed * coderabbit commments addressed * code optimised * redis runtime issuefixed * Coderabbit changes * removed unused method * Coderabbit comments addressed
Fix the Bengen Id
* fix: Handled Improper session managemant (#108) * AMM-1217 * removed unused imports * removed unused imports * AMM-1252 * swagger changes * bug(Identity-API) Edit family tagging null condition added * Jwttoken implementation and Crossorigin changes. * code rabbit comments addressed * coderabbitai comments addressed * coderabbit commments addressed * code optimised * redis runtime issuefixed * Coderabbit changes * removed unused method * Coderabbit comments addressed * fix: authentication issue * fix: removed db_iemr * fix: add env. to the variable to fetch the values from env file --------- Co-authored-by: ravishanigarapu <133210792+ravishanigarapu@users.noreply.github.com>
* fix: add van Id while import to local * fix: removed unwanted code
Redis correction
fix: amm-1927 security fix to prevent res headers for unallowed origins
Release 3.4.0
Release 3.5.0
📝 WalkthroughWalkthroughThis PR bumps the project version to 3.6.0, enhances CORS security with stricter header validation and dynamic origin pattern matching, adds VanID support to the beneficiary import flow, implements phone number normalization in identity creation, and qualifies the User entity with an explicit database schema. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~27 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/common/identity/service/IdentityService.java (1)
1796-1811: Critical: SQL query parameter mismatch.The SQL INSERT statement has 6 placeholders but the code attempts to populate 7 values in
objArr:
- Line 1796-1797: Query defines 6 columns:
BenRegId, BeneficiaryID, Provisioned, CreatedDate, CreatedBy, Reserved- Line 1803: Array initialized with
new Object[7]- Lines 1805-1811: Seven values are assigned (indices 0-6), including
VanIDat index 6This mismatch will cause a runtime error when executing the batch update.
🐛 Add VanID to SQL query
String query = " INSERT INTO m_beneficiaryregidmapping(BenRegId, BeneficiaryID, " - + " Provisioned, CreatedDate, CreatedBy, Reserved) VALUES (?,?,?,?,?,?) "; + + " Provisioned, CreatedDate, CreatedBy, Reserved, VanID) VALUES (?,?,?,?,?,?,?) ";
🤖 Fix all issues with AI agents
In @src/main/java/com/iemr/common/identity/service/IdentityService.java:
- Around line 1800-1802: The logger calls are malformed: replace the comma usage
and missing spacing by concatenating strings properly; update the call using the
logger instance (logger.info) where the first call currently reads
logger.info("inside for check->",obj) to concatenate the object (or its
toString) to the message, and adjust the second call inside
importBenIdToLocalServer that currently does logger.info("In for loop of
importBenIdToLocalServer"+obj.getVanID()) to include a separating space before
the concatenated obj.getVanID(); ensure both use a single string argument (or
use parameterized logging if available) and reference the same logger and
obj.getVanID() symbols when making the fix.
In
@src/main/java/com/iemr/common/identity/utils/http/HTTPRequestInterceptor.java:
- Around line 93-99: In HTTPRequestInterceptor (the Origin header handling
block), the closing brace and the subsequent statement are merged (`} status
= false;`); separate them by placing `status = false;` on its own properly
indented line immediately after the closing brace of the Origin-handling if/else
so the block reads with the brace on its own line followed by `status = false;`.
🧹 Nitpick comments (3)
src/main/java/com/iemr/common/identity/config/CorsConfig.java (1)
20-22: Consider consolidating redundant header case variations.The allowed headers list includes four case variations of
serverAuthorization:
serverAuthorizationServerAuthorizationserverauthorizationServerauthorizationHTTP headers are case-insensitive per RFC 7230. Spring's CORS handling already performs case-insensitive matching, so maintaining all four variations is unnecessary and adds maintenance overhead.
♻️ Simplify to single case
.allowedMethods("GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS") .allowedHeaders("Authorization", "Content-Type", "Accept", "Jwttoken", - "serverAuthorization", "ServerAuthorization", "serverauthorization", "Serverauthorization") + "ServerAuthorization") .exposedHeaders("Authorization", "Jwttoken").allowCredentials(true).maxAge(3600);src/main/java/com/iemr/common/identity/utils/http/HTTPRequestInterceptor.java (1)
130-143: Consider extracting duplicated origin validation logic.The
isOriginAllowedmethod logic is duplicated acrossHTTPRequestInterceptorandJwtUserIdValidationFilter. This duplication increases maintenance burden and the risk of inconsistent validation behavior if one implementation is updated without the other.♻️ Extract to shared utility
Create a shared CORS utility class:
public class CorsUtil { public static boolean isOriginAllowed(String origin, String allowedOrigins) { if (origin == null || allowedOrigins == null || allowedOrigins.trim().isEmpty()) { return false; } return Arrays.stream(allowedOrigins.split(",")) .map(String::trim) .anyMatch(pattern -> { String regex = pattern .replace(".", "\\.") .replace("*", ".*"); return origin.matches(regex); }); } }Then reference this utility in both
HTTPRequestInterceptorandJwtUserIdValidationFilter.src/main/java/com/iemr/common/identity/service/IdentityService.java (1)
1134-1163: Refactor: Extract repeated phone number cleaning into a loop.The phone number normalization logic applies
cleanPhoneNumberto 8 different fields with identical null checks. This repetitive pattern increases code volume and makes maintenance harder.♻️ Proposed refactor using helper method
logger.info("IdentityService.createIdentity - saving Contacts"); MBeneficiarycontact mContc = identityDTOToMBeneficiarycontact(identity); - - if (mContc.getEmergencyContactNum() != null) { - mContc.setEmergencyContactNum(cleanPhoneNumber(mContc.getEmergencyContactNum())); - } - - if (mContc.getPhoneNum1() != null) { - mContc.setPhoneNum1(cleanPhoneNumber(mContc.getPhoneNum1())); - } - - if (mContc.getPhoneNum2() != null) { - mContc.setPhoneNum2(cleanPhoneNumber(mContc.getPhoneNum2())); - } - if (mContc.getPhoneNum3() != null) { - mContc.setPhoneNum3(cleanPhoneNumber(mContc.getPhoneNum3())); - } - - if (mContc.getPhoneNum4() != null) { - mContc.setPhoneNum4(cleanPhoneNumber(mContc.getPhoneNum4())); - } - if (mContc.getPhoneNum5() != null) { - mContc.setPhoneNum5(cleanPhoneNumber(mContc.getPhoneNum5())); - } - if (mContc.getPreferredSMSPhoneNum() != null) { - mContc.setPreferredSMSPhoneNum(cleanPhoneNumber(mContc.getPreferredSMSPhoneNum())); - } if (mContc.getPreferredPhoneNum() != null) { - mContc.setPreferredPhoneNum(cleanPhoneNumber(mContc.getPreferredPhoneNum())); - } - - - - + + // Clean all phone numbers + mContc.setEmergencyContactNum(cleanPhoneNumber(mContc.getEmergencyContactNum())); + mContc.setPhoneNum1(cleanPhoneNumber(mContc.getPhoneNum1())); + mContc.setPhoneNum2(cleanPhoneNumber(mContc.getPhoneNum2())); + mContc.setPhoneNum3(cleanPhoneNumber(mContc.getPhoneNum3())); + mContc.setPhoneNum4(cleanPhoneNumber(mContc.getPhoneNum4())); + mContc.setPhoneNum5(cleanPhoneNumber(mContc.getPhoneNum5())); + mContc.setPreferredSMSPhoneNum(cleanPhoneNumber(mContc.getPreferredSMSPhoneNum())); + mContc.setPreferredPhoneNum(cleanPhoneNumber(mContc.getPreferredPhoneNum())); +Since
cleanPhoneNumberalready handles null/empty inputs gracefully (returns unchanged), the null checks are redundant.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pom.xmlsrc/main/java/com/iemr/common/identity/config/CorsConfig.javasrc/main/java/com/iemr/common/identity/controller/IdentityController.javasrc/main/java/com/iemr/common/identity/domain/User.javasrc/main/java/com/iemr/common/identity/dto/BenIdImportDTO.javasrc/main/java/com/iemr/common/identity/mapper/BenIdImportMapper.javasrc/main/java/com/iemr/common/identity/service/IdentityService.javasrc/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.javasrc/main/java/com/iemr/common/identity/utils/http/HTTPRequestInterceptor.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-17T07:29:05.121Z
Learnt from: ravishanigarapu
Repo: PSMRI/Identity-API PR: 85
File: src/main/java/com/iemr/common/identity/repo/BenMappingRepo.java:125-138
Timestamp: 2025-02-17T07:29:05.121Z
Learning: In the Identity-API project, tables involved in beneficiary mapping queries (MBeneficiaryregidmapping, MBeneficiarycontact, MBeneficiarydetail, MBeneficiaryaddress, MBeneficiaryconsent, MBeneficiaryImage, MBeneficiaryAccount, MBeneficiaryidentity, MBeneficiaryfamilymapping, MBeneficiaryservicemapping) have indexes created on join columns (vanID, vanSerialNo) to optimize query performance.
Applied to files:
src/main/java/com/iemr/common/identity/dto/BenIdImportDTO.javasrc/main/java/com/iemr/common/identity/service/IdentityService.javasrc/main/java/com/iemr/common/identity/mapper/BenIdImportMapper.java
🔇 Additional comments (8)
pom.xml (1)
8-8: Verify the version jump from 3.4.0 to 3.6.0.The version increment skips 3.5.0. Confirm this is intentional and aligns with your versioning strategy.
src/main/java/com/iemr/common/identity/dto/BenIdImportDTO.java (1)
33-33: LGTM! VanID field addition is well-structured.The vanID field and its accessors are properly implemented and align with the mapper changes. Based on learnings, vanID is used as an indexed join column in beneficiary mapping queries, making this addition performance-conscious.
Also applies to: 67-73
src/main/java/com/iemr/common/identity/domain/User.java (2)
18-18: LGTM! Explicit schema qualification improves clarity.Adding
schema = "db_iemr"to the@Tableannotation explicitly qualifies the table location, which is a best practice for applications that may interact with multiple database schemas.
20-20: Annotation placement is acceptable.Moving
@JsonIgnorePropertiesafter@Datadoes not affect behavior, as both annotations are processed independently by their respective frameworks (Lombok and Jackson).src/main/java/com/iemr/common/identity/controller/IdentityController.java (1)
27-27: Import additions noted.The new imports (
ArrayList,JsonArray,JsonSyntaxException) have been added. Their usage is not visible in the provided code segments, which may indicate they're used in controller logic that extends beyond the changed lines shown.Also applies to: 48-48, 54-54
src/main/java/com/iemr/common/identity/mapper/BenIdImportMapper.java (1)
42-42: LGTM! VanID mapping completes the integration.The
vanIDmapping properly connects the DTO field to the target entity field, completing the end-to-end support for vanID in the beneficiary import flow.src/main/java/com/iemr/common/identity/utils/JwtUserIdValidationFilter.java (1)
137-156: LGTM! Solid origin validation logic.The origin validation implementation is thorough with good defensive checks:
- Handles null/empty cases appropriately
- Escapes regex special characters correctly
- Provides clear logging for debugging
Note: This method duplicates logic from
HTTPRequestInterceptor.isOriginAllowed()(see comment on that file about extracting to a shared utility).src/main/java/com/iemr/common/identity/service/IdentityService.java (1)
1336-1351: Review phone number normalization edge cases.The
cleanPhoneNumberlogic handles common cases but has potential edge cases:
Line 1345-1347: The condition
cleaned.startsWith("91") && cleaned.length() == 12assumes that any 12-digit number starting with "91" should have "91" stripped. This could incorrectly process phone numbers from other countries (e.g., "919876543210" could be a different country's number).No validation: The method doesn't validate that the resulting number is a valid 10-digit Indian phone number after stripping the prefix.
Leading zeros: If a phone number has leading zeros after stripping the prefix, they will be preserved, which may or may not be desired.
Consider adding validation to ensure the cleaned phone number matches expected patterns (e.g., 10-digit numbers for India):
private String cleanPhoneNumber(String phoneNumber) { if (phoneNumber == null || phoneNumber.trim().isEmpty()) { return phoneNumber; } String cleaned = phoneNumber.trim(); // Remove +91 or 91 prefix if (cleaned.startsWith("+91")) { cleaned = cleaned.substring(3); } else if (cleaned.startsWith("91") && cleaned.length() == 12) { cleaned = cleaned.substring(2); } cleaned = cleaned.trim(); // Optional: Validate 10-digit number if (!cleaned.isEmpty() && !cleaned.matches("\\d{10}")) { logger.warn("Phone number after cleaning is not 10 digits: {}", cleaned); } return cleaned; }Do you want validation for 10-digit Indian phone numbers, or should the method accept various formats?
| logger.info("inside for check->",obj); | ||
|
|
||
| logger.info("In for loop of importBenIdToLocalServer"+obj.getVanID()); |
There was a problem hiding this comment.
Fix malformed logger statements.
Lines 1800 and 1802 have syntax errors in the logger calls:
- Line 1800:
logger.info("inside for check->",obj);- comma should be+for concatenation - Line 1802:
logger.info("In for loop of importBenIdToLocalServer"+obj.getVanID());- missing space before concatenation
These will cause compilation errors or unexpected log output.
🔧 Fix logger syntax
- logger.info("inside for check->",obj);
+ logger.info("inside for check-> {}", obj);
- logger.info("In for loop of importBenIdToLocalServer"+obj.getVanID());
+ logger.info("In for loop of importBenIdToLocalServer: {}", obj.getVanID());🤖 Prompt for AI Agents
In @src/main/java/com/iemr/common/identity/service/IdentityService.java around
lines 1800 - 1802, The logger calls are malformed: replace the comma usage and
missing spacing by concatenating strings properly; update the call using the
logger instance (logger.info) where the first call currently reads
logger.info("inside for check->",obj) to concatenate the object (or its
toString) to the message, and adjust the second call inside
importBenIdToLocalServer that currently does logger.info("In for loop of
importBenIdToLocalServer"+obj.getVanID()) to include a separating space before
the concatenated obj.getVanID(); ensure both use a single string argument (or
use parameterized logging if available) and reference the same logger and
obj.getVanID() symbols when making the fix.
| String origin = request.getHeader("Origin"); | ||
| if (origin != null && isOriginAllowed(origin)) { | ||
| response.setHeader("Access-Control-Allow-Origin", origin); | ||
| response.setHeader("Access-Control-Allow-Credentials", "true"); | ||
| } else if (origin != null) { | ||
| logger.warn("CORS headers NOT added for error response | Unauthorized origin: {}", origin); | ||
| } status = false; |
There was a problem hiding this comment.
Fix formatting: missing newline before statement.
Line 99 has } status = false; which appears to be a formatting issue. The closing brace and the statement should be on separate lines for better readability.
🔧 Proposed formatting fix
} else if (origin != null) {
logger.warn("CORS headers NOT added for error response | Unauthorized origin: {}", origin);
- } status = false;
+ }
+ status = false;📝 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 origin = request.getHeader("Origin"); | |
| if (origin != null && isOriginAllowed(origin)) { | |
| response.setHeader("Access-Control-Allow-Origin", origin); | |
| response.setHeader("Access-Control-Allow-Credentials", "true"); | |
| } else if (origin != null) { | |
| logger.warn("CORS headers NOT added for error response | Unauthorized origin: {}", origin); | |
| } status = false; | |
| String origin = request.getHeader("Origin"); | |
| if (origin != null && isOriginAllowed(origin)) { | |
| response.setHeader("Access-Control-Allow-Origin", origin); | |
| response.setHeader("Access-Control-Allow-Credentials", "true"); | |
| } else if (origin != null) { | |
| logger.warn("CORS headers NOT added for error response | Unauthorized origin: {}", origin); | |
| } | |
| status = false; |
🤖 Prompt for AI Agents
In
@src/main/java/com/iemr/common/identity/utils/http/HTTPRequestInterceptor.java
around lines 93 - 99, In HTTPRequestInterceptor (the Origin header handling
block), the closing brace and the subsequent statement are merged (`} status
= false;`); separate them by placing `status = false;` on its own properly
indented line immediately after the closing brace of the Origin-handling if/else
so the block reads with the brace on its own line followed by `status = false;`.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.