Separated out the prod-dev data based on the flag and made files name configurable #117
Conversation
Lokendra-egov
requested review from
Hari-egov,
jagankumar-egov,
pradeepkumarcm-egov,
subhashini-egov and
vinothrallapalli-eGov
as code owners
WalkthroughThis pull request introduces configuration-driven data loading for the default-data-handler service. It replaces hardcoded resource paths with configurable data file paths for MDMS, user, employee, and localization data. It adds a development mode flag that conditionally loads dev-specific datasets, includes new API endpoints for tenant creation, and provides separate production and development data files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as DataHandlerController
participant Service as DataHandlerService
participant MDMSUtil as MdmsBulkLoader
participant LocalizationUtil
participant Config as ServiceConfiguration
participant DB as Database/Backend
Client->>Controller: POST /tenant/new<br/>(NewTenantRequest)
activate Controller
Controller->>Service: loadNewTenantProductionData<br/>(newTenantRequest)
activate Service
Note over Service: Create user context<br/>(dummy user if needed)
Service->>Service: ensureUserContext()
Note over Service: Load production MDMS schema
Service->>MDMSUtil: createMdmSchemaFromFile()
activate MDMSUtil
MDMSUtil->>DB: Create schemas
deactivate MDMSUtil
Note over Service: Load production MDMS data
Service->>Config: getDefaultMdmsDataPath()
Config-->>Service: mdms data path
Service->>MDMSUtil: loadAllMdmsData<br/>(tenantId, requestInfo, mdmsDataPath)
activate MDMSUtil
MDMSUtil->>DB: Load MDMS records
deactivate MDMSUtil
Note over Service: Load production localization
Service->>Config: getDefaultLocalizationDataPath()
Config-->>Service: localization path
Service->>LocalizationUtil: upsertLocalizationFromFile<br/>(defaultDataRequest, localizationPath)
activate LocalizationUtil
LocalizationUtil->>DB: Upsert localization messages
deactivate LocalizationUtil
Note over Service: Create tenant config
Service->>DB: Save tenant config
Service-->>Controller: Success
deactivate Service
Controller->>Controller: Build DataSetupResponse<br/>(targetTenantId)
Controller-->>Client: HTTP 202 Accepted<br/>(DataSetupResponse)
deactivate Controller
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
utilities/default-data-handler/src/main/java/org/egov/handler/config/ServiceConfiguration.java (1)
11-15: Optional: Simplify Lombok annotations.The class uses
@Data(line 11) which already generates@Getter,@Setter,@ToString,@EqualsAndHashCode, and@RequiredArgsConstructor. The explicit@Setter(line 14) and@Getter(line 15) annotations are redundant.Consider this simplification:
@Component @Data @NoArgsConstructor @AllArgsConstructor -@Setter -@Getter public class ServiceConfiguration {devops/deploy-as-code/charts/urban/default-data-handler/values.yaml (1)
128-129: Consider making this configurable via Helm values.The flag is currently hardcoded to
"true". For better flexibility across different environments (dev, staging, production), consider extracting this to a configurable Helm value.Add a new value in the values section:
# Feature flags loadUserEmployeeDataEnabled: "true"Then reference it in the env section:
- name: DEFAULT_DATA_LOAD_USER_EMPLOYEE_ENABLED - value: "true" + value: {{ index .Values "loadUserEmployeeDataEnabled" | quote }}utilities/default-data-handler/src/main/java/org/egov/StartupUserAndEmployeeInitializer.java (1)
101-106: Add logging for observability.While the conditional logic is correct, consider adding logging to indicate whether user/employee data loading was executed or skipped. This aids troubleshooting and makes the flag's effect visible in logs.
// Load user, employee, and workflow config data only if enabled if (serviceConfig.isLoadUserEmployeeDataEnabled()) { + System.out.println("[INFO] Loading user, employee, and workflow config data (flag enabled)"); dataHandlerService.createUserFromFile(tenantRequest); dataHandlerService.createPgrWorkflowConfig(tenantRequest.getTenant().getCode()); dataHandlerService.createEmployeeFromFile(defaultDataRequest.getRequestInfo()); + } else { + System.out.println("[INFO] Skipping user, employee, and workflow config data loading (flag disabled)"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
devops/deploy-as-code/charts/urban/default-data-handler/values.yaml(1 hunks)utilities/default-data-handler/src/main/java/org/egov/StartupUserAndEmployeeInitializer.java(1 hunks)utilities/default-data-handler/src/main/java/org/egov/handler/config/ServiceConfiguration.java(1 hunks)utilities/default-data-handler/src/main/resources/application.properties(1 hunks)
🔇 Additional comments (2)
utilities/default-data-handler/src/main/java/org/egov/handler/config/ServiceConfiguration.java (1)
135-136: LGTM! Feature flag properly configured.The new configuration property is correctly wired with an appropriate property name that follows Spring Boot conventions.
utilities/default-data-handler/src/main/resources/application.properties (1)
87-88: LGTM! Configuration property properly added.The property name is consistent with the ServiceConfiguration field, and the default value of
truemaintains existing behavior. The comment clearly explains the purpose.
… with a dev enabled flag
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
utilities/default-data-handler/src/main/java/org/egov/handler/util/LocalizationUtil.java (2)
87-87: Remove commented code.The commented exception throw should be removed if it's no longer needed, or uncommented if error handling should fail fast.
} catch (Exception e) { log.error("Error creating Tenant localization data for {} : {}", tenantId, e.getMessage()); -// throw new CustomException("TENANT", "Failed to create localization data for " + tenantId + " : " + e.getMessage()); }
110-112: Consider whether silent failure is appropriate.The method returns an empty list when the path pattern fails to resolve. If an invalid path is provided due to misconfiguration, this could silently fail without making the issue obvious.
Consider whether you should throw an exception for configuration errors vs. simply missing files:
} catch (IOException e) { log.error("Failed to scan localization directories: {}", e.getMessage()); if (messages.isEmpty()) { log.warn("No localization messages loaded from path: {}", localizationPath); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
utilities/default-data-handler/src/main/resources/localisations-dev/localizations (default).xlsxis excluded by!**/*.xlsxutilities/default-data-handler/src/main/resources/localisations-dev/localizations (en_IN).xlsxis excluded by!**/*.xlsxutilities/default-data-handler/src/main/resources/localisations-dev/localizations (hi_IN).xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (15)
devops/deploy-as-code/charts/urban/default-data-handler/values.yaml(1 hunks)utilities/default-data-handler/src/main/java/org/egov/StartupSchemaAndMasterDataInitializer.java(1 hunks)utilities/default-data-handler/src/main/java/org/egov/StartupUserAndEmployeeInitializer.java(2 hunks)utilities/default-data-handler/src/main/java/org/egov/handler/config/ServiceConfiguration.java(1 hunks)utilities/default-data-handler/src/main/java/org/egov/handler/service/DataHandlerService.java(3 hunks)utilities/default-data-handler/src/main/java/org/egov/handler/util/LocalizationUtil.java(2 hunks)utilities/default-data-handler/src/main/java/org/egov/handler/util/MdmsBulkLoader.java(1 hunks)utilities/default-data-handler/src/main/resources/HRMS-dev.json(1 hunks)utilities/default-data-handler/src/main/resources/HRMS.json(0 hunks)utilities/default-data-handler/src/main/resources/User-dev.json(1 hunks)utilities/default-data-handler/src/main/resources/User.json(0 hunks)utilities/default-data-handler/src/main/resources/application.properties(1 hunks)utilities/default-data-handler/src/main/resources/localisations-dev/en_IN/egov-hrms.json(1 hunks)utilities/default-data-handler/src/main/resources/localisations-dev/en_IN/egov-user.json(1 hunks)utilities/default-data-handler/src/main/resources/localisations-dev/en_IN/rainmaker-hr.json(1 hunks)
💤 Files with no reviewable changes (2)
- utilities/default-data-handler/src/main/resources/User.json
- utilities/default-data-handler/src/main/resources/HRMS.json
✅ Files skipped from review due to trivial changes (4)
- utilities/default-data-handler/src/main/resources/localisations-dev/en_IN/egov-hrms.json
- utilities/default-data-handler/src/main/resources/localisations-dev/en_IN/egov-user.json
- utilities/default-data-handler/src/main/resources/localisations-dev/en_IN/rainmaker-hr.json
- utilities/default-data-handler/src/main/resources/HRMS-dev.json
🚧 Files skipped from review as they are similar to previous changes (3)
- utilities/default-data-handler/src/main/resources/application.properties
- devops/deploy-as-code/charts/urban/default-data-handler/values.yaml
- utilities/default-data-handler/src/main/java/org/egov/handler/config/ServiceConfiguration.java
🔇 Additional comments (1)
utilities/default-data-handler/src/main/java/org/egov/handler/util/LocalizationUtil.java (1)
98-99: LGTM!The dynamic path loading with PathMatchingResourcePatternResolver is well-implemented, and the logging provides good visibility.
utilities/default-data-handler/src/main/java/org/egov/handler/util/LocalizationUtil.java
Show resolved
Hide resolved
utilities/default-data-handler/src/main/java/org/egov/handler/util/LocalizationUtil.java
Show resolved
Hide resolved
utilities/default-data-handler/src/main/java/org/egov/handler/util/LocalizationUtil.java
Outdated
Show resolved
Hide resolved
utilities/default-data-handler/src/main/java/org/egov/StartupUserAndEmployeeInitializer.java
Show resolved
Hide resolved
utilities/default-data-handler/src/main/resources/User-dev.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
utilities/default-data-handler/src/main/java/org/egov/handler/util/LocalizationUtil.java (2)
53-55: Good refactoring; consider adding parameter validation.The signature change to accept
localizationPathenables the configurable loading approach described in the PR objectives. Past reviews confirmed that callers provide valid paths from configuration.Consider adding defensive null/empty validation for the
localizationPathparameter to fail fast with a clear error message if misconfigured:public void upsertLocalizationFromFile(DefaultDataRequest defaultDataRequest, String localizationPath){ + if (localizationPath == null || localizationPath.trim().isEmpty()) { + throw new IllegalArgumentException("localizationPath must not be null or empty"); + } List<Message> messageList = addMessagesFromFile(defaultDataRequest, localizationPath);
91-99: Return type fix looks good; consistent parameter validation recommended.The parameterized return type
List<Message>properly addresses the raw type concern from past reviews. The dynamic path usage and logging are appropriate.For consistency with defensive coding practices, consider adding the same null/empty validation here:
public List<Message> addMessagesFromFile(DefaultDataRequest defaultDataRequest, String localizationPath){ + if (localizationPath == null || localizationPath.trim().isEmpty()) { + throw new IllegalArgumentException("localizationPath must not be null or empty"); + } List<Message> messages = new ArrayList<>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utilities/default-data-handler/src/main/java/org/egov/handler/util/LocalizationUtil.java(2 hunks)
Lokendra-egov
changed the title
…hema excel validator
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)
utilities/default-data-handler/src/main/java/org/egov/handler/service/DataHandlerService.java (1)
193-196: Security concern: Hardcoded password should be externalized.The password
"eGov@123"is hardcoded in source code. Even for development/provisioning, this exposes a known credential and makes rotation difficult. Consider externalizing this to configuration.- // Set password - updatedUser.put("password", "eGov@123"); + // Set password from configuration + updatedUser.put("password", serviceConfig.getDefaultEmployeePassword());
🧹 Nitpick comments (4)
utilities/default-data-handler/src/main/resources/schema/tenant.json (1)
416-469: Inconsistent schema validation: missingadditionalProperties: false.The
localization.masterschema is missingadditionalProperties: false, unliketenant.master(line 349) andcommon.master(line 413). This inconsistency allows extra properties in localization data while rejecting them in other schemas."description": "Translated message in the specified locale" } - } + }, + "additionalProperties": false } } ]utilities/default-data-handler/src/main/java/org/egov/handler/web/models/NewTenantRequest.java (1)
23-26: Remove unnecessary@Validand consider@NotBlankfor stronger validation.
@Validon aStringfield has no effect—it's meant for cascading validation into nested objects. Also,@NotNullallows empty strings; use@NotBlankto reject both null and empty/whitespace-only values.@JsonProperty("targetTenantId") - @NotNull - @Valid + @NotBlank private String targetTenantId;Add the import:
import jakarta.validation.constraints.NotBlank;utilities/default-data-handler/src/main/java/org/egov/handler/service/DataHandlerService.java (2)
147-149: Error message references hardcoded filename instead of the configurable path.The error message mentions "User.json" but the file path is now configurable via
userFilePath. This could mislead debugging efforts.} catch (Exception e) { - log.error("Error creating users from User.json for tenant {}: {}", tenantCode, e.getMessage(), e); + log.error("Error creating users from {} for tenant {}: {}", userFilePath, tenantCode, e.getMessage(), e); throw new CustomException("USER_CREATION_FAILED", "Failed to create users for tenant: " + tenantCode); }
644-660: Consider using ASCII-safe log messages for broader environment compatibility.Unicode symbols like
✓and✓✓✓in log output may render incorrectly in some log aggregators, terminals, or file encodings. Consider using ASCII alternatives like[OK]or[SUCCESS].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
utilities/default-data-handler/src/main/java/org/egov/handler/service/DataHandlerService.java(5 hunks)utilities/default-data-handler/src/main/java/org/egov/handler/web/controller/DataHandlerController.java(2 hunks)utilities/default-data-handler/src/main/java/org/egov/handler/web/models/NewTenantRequest.java(1 hunks)utilities/default-data-handler/src/main/resources/schema/tenant.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
utilities/default-data-handler/src/main/java/org/egov/handler/web/models/NewTenantRequest.java (1)
utilities/default-data-handler/src/main/java/org/egov/handler/web/models/Message.java (1)
Builder(10-27)
🔇 Additional comments (1)
utilities/default-data-handler/src/main/java/org/egov/handler/web/controller/DataHandlerController.java (1)
49-59: LGTM!The new
/tenant/newendpoint follows the existing controller patterns, uses appropriate HTTP 202 status for the potentially long-running tenant provisioning operation, and properly delegates to the service layer.
| } else { | ||
| requestInfo.getUserInfo().setTenantId(targetTenantId); | ||
| } |
There was a problem hiding this comment.
Mutating the caller's RequestInfo object may cause unintended side effects.
Calling requestInfo.getUserInfo().setTenantId(targetTenantId) modifies the object passed by the caller. If the caller reuses this RequestInfo for other operations, they may get unexpected behavior. Consider creating a copy or documenting this mutation as intentional.
| Tenant newTenant = new Tenant(); | ||
| newTenant.setCode(targetTenantId); | ||
| newTenant.setName(targetTenantId); | ||
| newTenant.setEmail("admin@" + targetTenantId + ".com"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check NewTenantRequest and Tenant classes structure
rg -n "class NewTenantRequest|class Tenant" --type=java -A 25 | head -100Repository: egovernments/Citizen-Complaint-Resolution-System
Length of output: 11045
🏁 Script executed:
# Check the context around lines 664-667 in DataHandlerService.java
sed -n '650,680p' utilities/default-data-handler/src/main/java/org/egov/handler/service/DataHandlerService.javaRepository: egovernments/Citizen-Complaint-Resolution-System
Length of output: 1596
🏁 Script executed:
# Check if there are configuration properties or defaults being used elsewhere
rg -n "defaultTenantName|defaultTenantEmail|admin@" --type=java -B 2 -A 2 | head -40Repository: egovernments/Citizen-Complaint-Resolution-System
Length of output: 822
Add tenant name and email to NewTenantRequest or implement configuration defaults.
The code hardcodes placeholder values (name = targetTenantId and email = "admin@" + targetTenantId + ".com") when creating a new Tenant configuration. These placeholders persist permanently in the Tenant Management System. Since NewTenantRequest currently provides only targetTenantId and RequestInfo, consider either:
- Adding
nameandemailfields toNewTenantRequestto accept proper tenant metadata from the caller, or - Implementing configurable defaults (e.g., via
ServiceConfiguration) instead of hardcoding placeholder values.
The Tenant model enforces @NotNull on name and @Email validation on email, making these required fields that should be meaningful for production tenants.
🤖 Prompt for AI Agents
In
utilities/default-data-handler/src/main/java/org/egov/handler/service/DataHandlerService.java
around lines 664 to 667, the code currently assigns placeholder tenant name and
email (name = targetTenantId, email = "admin@...") which violates model
validation and persists meaningless values; update the flow so
newTenant.setName(...) and newTenant.setEmail(...) pull real values instead of
hardcoded ones: either extend NewTenantRequest to include name and email and use
those fields here (validate non-null and email format) or read configurable
defaults from ServiceConfiguration (e.g., defaultTenantName and
defaultTenantEmail) and apply them; ensure the chosen source is validated before
setting on Tenant and update callers/tests accordingly.
| { | ||
| "tenantId": "{tenantid}", | ||
| "code": "localization.master", | ||
| "description": "Localization Master Schema - Used for Excel validation in Jupyter notebooks. Validates multi-language translation data with locale codes (e.g., en_IN, hi_IN, ta_IN)", | ||
| "isActive": true, | ||
| "definition": { | ||
| "type": "object", | ||
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "required": [ | ||
| "module", | ||
| "code", | ||
| "locale" | ||
| ], | ||
| "x-unique": [ | ||
| "code", | ||
| "locale" | ||
| ], | ||
| "properties": { | ||
| "module": { | ||
| "type": "string", | ||
| "minLength": 1, | ||
| "maxLength": 100, | ||
| "description": "Module name (e.g., rainmaker-pgr, rainmaker-common)" | ||
| }, | ||
| "code": { | ||
| "type": "string", | ||
| "minLength": 1, | ||
| "maxLength": 200, | ||
| "description": "Translation key (e.g., PGR_COMPLAINT_TYPE, CORE_COMMON_SAVE)" | ||
| }, | ||
| "englishMessage": { | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ], | ||
| "maxLength": 1000, | ||
| "description": "English translation message" | ||
| }, | ||
| "locale": { | ||
| "type": "string", | ||
| "pattern": "^[a-z]{2}_[A-Z]{2}$", | ||
| "description": "Locale code in format: language_COUNTRY (e.g., en_IN, hi_IN, ta_IN)" | ||
| }, | ||
| "translation": { | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ], | ||
| "maxLength": 1000, | ||
| "description": "Translated message in the specified locale" | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add additionalProperties: false to the localization.master schema for consistency.
The tenant.master and common.master schemas both include "additionalProperties": false to enforce strict validation and reject unexpected properties. The localization.master schema is missing this constraint, which creates an inconsistency and may allow unintended properties during validation.
Apply this diff to add the missing constraint:
"translation": {
"type": [
"string",
"null"
],
"maxLength": 1000,
"description": "Translated message in the specified locale"
}
}
+ "additionalProperties": false
}
]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
utilities/default-data-handler/src/main/resources/schema/excel-validation-schemas.json
around lines 166 to 219: the localization.master schema is missing
"additionalProperties": false on its "definition" object causing inconsistent
validation behavior; add "additionalProperties": false directly inside the
"definition" object (next to "type" and "$schema") so the schema rejects
unexpected properties just like tenant.master and common.master.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.