[Add] mobile validation from master data #917
Conversation
Lokendra-egov
requested review from
GhanshyamRawat-eGov and
talele08
as code owners
WalkthroughAdds MDMS v2 DTOs and a Redis-backed ValidationRules cache, introduces MobileNumberValidator that queries MDMS-v2 with defaults, injects it into UserController flows, removes mobile Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant UC as UserController
participant MV as MobileNumberValidator
participant CACHE as ValidationRulesCacheRepository
participant MD as MDMS-v2
participant US as UserService
Client->>UC: Create/Update/Patch User (RequestInfo, user)
UC->>MV: validateMobileNumber(mobileNumber, tenantId, RequestInfo)
MV->>CACHE: getValidationRules(tenantKey)
alt Cache hit
CACHE-->>MV: ValidationRules
else Cache miss
MV->>MD: POST /mdms/v2/search (MdmsV2SearchRequest)
MD-->>MV: MdmsV2Response
MV->>MV: select active "defaultMobileValidation"
MV->>CACHE: cacheValidationRules(tenantKey, rules)
end
MV->>MV: if rules null -> use default rules
MV->>MV: validate length / pattern / allowed starts
alt Violations
MV-->>UC: throw CustomException(errors)
else Valid
MV-->>UC: return
end
UC->>US: proceed with business operation
US-->>UC: result
UC-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (2)
core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java (2)
121-130: Fix misleading comment and minor cleanup
- The comment says “first 2 characters” but code splits on '.' to get state-level tenant. Update the comment for clarity.
- // Extract state level tenant (first 2 characters) + // Extract state-level tenant (substring before '.'), e.g., "pb.amritsar" -> "pb"
23-25: Resilience and maintainability: DI, timeouts, and caching
- Prefer constructor injection with final fields for immutability and testability; avoid field injection.
- Ensure RestTemplate has connect/read timeouts; otherwise MDMS outages can stall requests. Alternatively, use a preconfigured bean with timeouts.
- Consider caching MDMS rules per state-level tenant (short TTL) to avoid per-request MDMS calls and reduce latency.
Possible direction (illustrative):
- Inject a preconfigured RestTemplate bean with timeouts.
- Add a lightweight cache (Spring Cache or Caffeine) for rules by tenant (keyed by state-level tenantId).
I can draft a small cache-backed provider (e.g., ValidationRulesProvider with @Cacheable) if you want.
Also applies to: 26-34, 143-145, 159-166
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2Data.java(1 hunks)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2Response.java(1 hunks)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2SearchCriteria.java(1 hunks)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2SearchRequest.java(1 hunks)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationData.java(1 hunks)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationRules.java(1 hunks)core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java(1 hunks)core-services/egov-user/src/main/java/org/egov/user/web/contract/UserRequest.java(1 hunks)core-services/egov-user/src/main/java/org/egov/user/web/contract/UserSearchRequest.java(0 hunks)core-services/egov-user/src/main/java/org/egov/user/web/controller/UserController.java(7 hunks)core-services/egov-user/src/main/resources/application.properties(1 hunks)
💤 Files with no reviewable changes (1)
- core-services/egov-user/src/main/java/org/egov/user/web/contract/UserSearchRequest.java
🧰 Additional context used
🧬 Code graph analysis (6)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2SearchCriteria.java (1)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2SearchRequest.java (1)
Data(10-21)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2SearchRequest.java (1)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2SearchCriteria.java (1)
Data(9-28)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationRules.java (2)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2Data.java (1)
Data(9-32)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationData.java (1)
Data(9-20)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2Data.java (3)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2Response.java (1)
Data(11-19)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationData.java (1)
Data(9-20)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationRules.java (1)
Data(11-37)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationData.java (3)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2Data.java (1)
Data(9-32)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2Response.java (1)
Data(11-19)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationRules.java (1)
Data(11-37)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2Response.java (5)
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2Data.java (1)
Data(9-32)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2SearchCriteria.java (1)
Data(9-28)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2SearchRequest.java (1)
Data(10-21)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationData.java (1)
Data(9-20)core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationRules.java (1)
Data(11-37)
🔇 Additional comments (9)
core-services/egov-user/src/main/resources/application.properties (1)
68-72: LGTM! MDMS v2 configuration properly added.The new MDMS v2 configuration properties are well-structured and align with the mobile validation integration objective.
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2Response.java (1)
11-18: LGTM! Standard response wrapper with proper annotations.The DTO structure correctly wraps the MDMS v2 data list with appropriate Lombok and Jackson mappings.
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2Data.java (1)
9-32: LGTM! Well-structured MDMS v2 data model.The DTO properly represents MDMS v2 data entries with appropriate field mappings and nested ValidationData structure.
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2SearchCriteria.java (1)
9-28: LGTM! Search criteria with sensible pagination defaults.The DTO provides appropriate search parameters with reasonable default values for limit (1000) and offset (0).
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationData.java (1)
9-20: LGTM! Clean validation data container.The DTO appropriately wraps ValidationRules with a validation name identifier.
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/MdmsV2SearchRequest.java (1)
10-21: LGTM! Request wrapper follows MDMS v2 API contract.The DTO properly wraps search criteria and request info. The capital-case JSON property names ("MdmsCriteria", "RequestInfo") likely match the MDMS v2 API specification.
core-services/egov-user/src/main/java/org/egov/user/domain/model/mdmsv2/ValidationRules.java (1)
11-37: LGTM! Comprehensive validation rule structure.The DTO provides a thorough set of validation constraints including pattern matching, length bounds, prefix/digit restrictions, and user-friendly error messages.
core-services/egov-user/src/main/java/org/egov/user/web/contract/UserRequest.java (1)
47-49: Mobile number validation enforced and handles null/empty correctly. MobileNumberValidator.validateMobileNumber is called in all create, update, and patch endpoints and explicitly skips null or empty values, ensuring validation before persistence.core-services/egov-user/src/main/java/org/egov/user/web/controller/UserController.java (1)
84-86: Enforce header‐based gating on mobile validation
- Wrap
mobileNumberValidator.validateMobileNumber(...)in
if (isMobileValidationRequired(headers)) { … }for:
•/users/_createnovalidate(lines 109–111)
•/users/_updatenovalidate(lines 173–175)
•/profile/_update(add@RequestHeader HttpHeaders headersand wrap lines 192–194)- For
/citizen/_create, decide if it should also honor this gating (addHttpHeaders headersand wrap) or remain always-on by design.
core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core-services/egov-user/src/main/java/org/egov/user/web/controller/UserController.java (1)
83-87: Extract duplicate validation logic into a helper method.The same validation pattern for both primary and alternate mobile numbers is repeated across four methods (
createCitizen,createUserWithoutValidation,updateUserWithoutValidation, andpatch). This violates the DRY principle and makes maintenance harder.Apply this diff to extract the common validation logic:
+ /** + * Validates both primary and alternate mobile numbers using MDMS-v2 validation rules. + * + * @param primaryMobile primary mobile number + * @param alternateMobile alternate mobile number (can be null) + * @param tenantId tenant identifier + * @param requestInfo request metadata + */ + private void validateMobileNumbers(String primaryMobile, String alternateMobile, + String tenantId, org.egov.common.contract.request.RequestInfo requestInfo) { + mobileNumberValidator.validateMobileNumber(primaryMobile, tenantId, requestInfo); + mobileNumberValidator.validateMobileNumber(alternateMobile, tenantId, requestInfo); + } + @PostMapping("/citizen/_create") public Object createCitizen(@RequestBody @Valid CreateUserRequest createUserRequest) { log.info("Received Citizen Registration Request " + createUserRequest); User user = createUserRequest.toDomain(true); - // Validate mobile number using MDMS-v2 - mobileNumberValidator.validateMobileNumber(user.getMobileNumber(), user.getTenantId(), createUserRequest.getRequestInfo()); - mobileNumberValidator.validateMobileNumber(user.getAlternateMobileNumber(), user.getTenantId(), createUserRequest.getRequestInfo()); + validateMobileNumbers(user.getMobileNumber(), user.getAlternateMobileNumber(), + user.getTenantId(), createUserRequest.getRequestInfo()); user.setOtpValidationMandatory(IsValidationMandatory);Repeat the same change pattern for
createUserWithoutValidation,updateUserWithoutValidation, andpatchmethods.Also applies to: 109-113, 174-178, 194-198
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core-services/egov-user/src/main/java/org/egov/user/web/controller/UserController.java(7 hunks)
🔇 Additional comments (2)
core-services/egov-user/src/main/java/org/egov/user/web/controller/UserController.java (2)
14-14: LGTM! Dependency injection properly implemented.The
MobileNumberValidatoris correctly imported, declared as a field, and injected via the constructor following standard Spring DI patterns.Also applies to: 50-50, 66-66, 69-69
86-86: Validation safely handles null/empty mobile numbers
StringUtils.isEmpty()returns true for null or empty strings, sovalidateMobileNumber()immediately returns.
| for (MdmsV2Data mdmsData : response.getMdms()) { | ||
| if (mdmsData.getData() != null | ||
| && Boolean.TRUE.equals(mdmsData.getIsActive()) | ||
| && "defaultMobileValidation".equals(mdmsData.getData().getValidationName())) { |
There was a problem hiding this comment.
Move "defaultMobileValidation" to constants
core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java
Show resolved
Hide resolved
| .build(); | ||
|
|
||
| log.info("Calling MDMS-v2 at: {}", url); | ||
| MdmsV2Response response = restTemplate.postForObject(url, searchRequest, MdmsV2Response.class); |
There was a problem hiding this comment.
We should think about caching instead of making network call for each request
There was a problem hiding this comment.
@talele08 We’re calling MDMS to fetch the rules. Shouldn’t the caching be handled at the MDMS level instead? MDMS can decide whether to retrieve the data from the database or the cache.
There was a problem hiding this comment.
@talele08 The cache logic should be implemented at the MDMS service level. The MDMS service should read from the cache and return the response. If caching is implemented separately in the User Service, any changes made at the MDMS level will not be reflected.
Ultimately, since MDMS is responsible for data creation and updation, the caching /reset caching should be managed within the MDMS service itself.
| @JsonProperty("rules") | ||
| private ValidationRules rules; | ||
|
|
||
| @JsonProperty("validationName") |
There was a problem hiding this comment.
Is validationName required? If it's only for default validation, can't we just initialise a default ValidationRule object and keep re-using it
There was a problem hiding this comment.
Currently, as per the data, this is required since there will be multiple active entries for this schema code. To fetch the correct rules, we need to check using both conditions (active and validationName).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java (2)
144-149: Hard-coded limit in MDMS search criteria.Line 147 hard-codes
limit(1000). If the MDMS schema contains more than 1000 validation configurations, some will be missed. Additionally, fetching 1000 records when typically only one is needed is inefficient.Consider either:
- Making the limit configurable via application properties
- Adding a filter in the MDMS query itself to retrieve only the specific validation configuration by name
- Implementing pagination if the MDMS API supports it
+ @Value("${egov.mdms.v2.search.limit:1000}") + private Integer mdmsSearchLimit; + MdmsV2SearchCriteria searchCriteria = MdmsV2SearchCriteria.builder() .tenantId(stateLevelTenantId) .schemaCode(validationSchemaCode) - .limit(1000) + .limit(mdmsSearchLimit) .offset(0) .build();
175-179: Overly broad exception handling masks errors.Lines 175-179 catch all exceptions and return
null, which causes the code to fall back to default validation rules. While this provides resilience, it also masks genuine errors such as:
- Network connectivity issues
- MDMS service downtime
- Authentication/authorization failures
- Malformed MDMS responses
Consider logging the exception type and message to help with debugging and monitoring:
} catch (Exception e) { - log.error("Error fetching validation rules from MDMS-v2", e); + log.error("Error fetching validation rules from MDMS-v2 for tenant {}: {} - {}", + tenantId, e.getClass().getSimpleName(), e.getMessage(), e); // Don't fail user creation if MDMS is down, just log the error return null; }Additionally, consider emitting metrics or alerts when MDMS calls fail, so operations teams can detect and respond to issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core-services/egov-user/src/main/java/org/egov/user/config/UserServiceConstants.java(1 hunks)core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: holashchand
Repo: egovernments/Digit-Core PR: 739
File: core-services/mdms-v2/src/main/java/org/egov/infra/mdms/producer/Producer.java:31-35
Timestamp: 2025-05-22T11:05:27.628Z
Learning: In the MDMS V2 service, tenant ID validation happens in the service and validator layers before reaching the Producer.push() method.
📚 Learning: 2025-05-22T11:05:27.628Z
Learnt from: holashchand
Repo: egovernments/Digit-Core PR: 739
File: core-services/mdms-v2/src/main/java/org/egov/infra/mdms/producer/Producer.java:31-35
Timestamp: 2025-05-22T11:05:27.628Z
Learning: In the MDMS V2 service, tenant ID validation happens in the service and validator layers before reaching the Producer.push() method.
Applied to files:
core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java
📚 Learning: 2025-05-22T11:05:57.893Z
Learnt from: holashchand
Repo: egovernments/Digit-Core PR: 739
File: core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/impl/SchemaDefinitionDbRepositoryImpl.java:70-77
Timestamp: 2025-05-22T11:05:57.893Z
Learning: In the MDMS V2 service, tenant ID validation is performed in the MultiStateInstanceUtil.replaceSchemaPlaceholder method, which checks that the tenant ID contains a dot and has sufficient length for schema extraction in a central instance environment.
Applied to files:
core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.javacore-services/egov-user/src/main/java/org/egov/user/config/UserServiceConstants.java
📚 Learning: 2025-05-22T11:05:27.628Z
Learnt from: holashchand
Repo: egovernments/Digit-Core PR: 739
File: core-services/mdms-v2/src/main/java/org/egov/infra/mdms/producer/Producer.java:31-35
Timestamp: 2025-05-22T11:05:27.628Z
Learning: In the MDMS V2 service, tenant ID validation happens elsewhere in the codebase before reaching the Producer.push() method.
Applied to files:
core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java
🔇 Additional comments (4)
core-services/egov-user/src/main/java/org/egov/user/config/UserServiceConstants.java (2)
73-73: LGTM! New constant is well-defined.The
DEFAULT_MOBILE_VALIDATION_NAMEconstant is appropriately named and will serve as a clear fallback for MDMS v2-based mobile validation rules.
64-64: Disregard the original review comment.The
PATTERN_MOBILEconstant is not obsolete—it serves as a critical fallback mechanism:
- Line 60: Used when MDMS v2 validation rules are unavailable
- Line 95: Fallback pattern when MDMS regex is invalid (PatternSyntaxException)
- Line 119: Default pattern for ValidationRules initialization
The code design intentionally preserves this constant for resilience. Removing it would break the fallback validation path. The transition to MDMS v2 does not eliminate the need for this safety mechanism.
Likely an incorrect or invalid review comment.
core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java (2)
84-103: Previous feedback addressed: invalid regex pattern handling.The pattern validation now correctly handles
PatternSyntaxExceptionby logging a warning and falling back to the default pattern. This prevents invalid MDMS regex from causing a 500 error.
161-168: Client-side filtering is currently necessary due to API model limitations.The MDMS-v2 API client model (
MdmsV2SearchCriteria) only exposes four fields:tenantId,schemaCode,limit, andoffset. There is no mechanism to pass filter criteria (likevalidationNameorisActive) through the search request. While the server-side model (MdmsCriteriaV2) includes filter andisActivefields, these are not exposed by the client library's search criteria class.The client-side iteration at lines 161-168 is not an optimization opportunity with the current API contract—it is a necessity. Eliminating it would require extending
MdmsV2SearchCriteriato support filtering (e.g., adding afiltersorfilterMapfield), which is outside the scope of this code change.
core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java
Show resolved
Hide resolved
core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java
Outdated
Show resolved
Hide resolved
core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core-services/egov-user/src/main/java/org/egov/user/EgovUserApplication.java(2 hunks)core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java(1 hunks)core-services/egov-user/src/main/java/org/egov/user/repository/ValidationRulesCacheRepository.java(1 hunks)core-services/egov-user/src/main/resources/application.properties(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core-services/egov-user/src/main/java/org/egov/user/domain/service/MobileNumberValidator.java
- core-services/egov-user/src/main/resources/application.properties
🔇 Additional comments (1)
core-services/egov-user/src/main/java/org/egov/user/EgovUserApplication.java (1)
109-112: Bean wiring aligns with existing Redis setup.Reusing the existing
JedisConnectionFactorykeeps all Redis access points consistent; looks good.
...ervices/egov-user/src/main/java/org/egov/user/repository/ValidationRulesCacheRepository.java
Outdated
Show resolved
Hide resolved
3 participants
Summary by CodeRabbit
New Features
Chores