[HCMPRE-2741] Central instance support for MDMS V2 service #739
Conversation
WalkthroughThis update introduces tenant-aware functionality to the MDMS service. It modifies repository and producer classes to handle tenant-specific topic publishing and schema resolution, adds error handling for invalid tenant IDs, and updates SQL queries to support dynamic schema prefixes. The migration script is enhanced to support multi-schema Flyway migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MdmsDataRepositoryImpl
participant MultiStateInstanceUtil
participant Producer
participant DB
Client->>MdmsDataRepositoryImpl: create(MdmsRequest)
MdmsDataRepositoryImpl->>Producer: push(tenantId, topic, value)
Producer->>MultiStateInstanceUtil: resolveTopic(tenantId, topic)
MultiStateInstanceUtil-->>Producer: tenantSpecificTopic
Producer-->>MdmsDataRepositoryImpl: (message sent)
MdmsDataRepositoryImpl-->>Client: (acknowledgement)
Client->>MdmsDataRepositoryImpl: search(criteria)
MdmsDataRepositoryImpl->>MultiStateInstanceUtil: replaceSchemaPlaceholder(query, tenantId)
alt valid tenantId
MultiStateInstanceUtil-->>MdmsDataRepositoryImpl: queryWithSchema
MdmsDataRepositoryImpl->>DB: execute(queryWithSchema)
DB-->>MdmsDataRepositoryImpl: results
MdmsDataRepositoryImpl-->>Client: results
else invalid tenantId
MultiStateInstanceUtil-->>MdmsDataRepositoryImpl: throw InvalidTenantIdException
MdmsDataRepositoryImpl-->>Client: CustomException(INVALID_TENANT_ID_ERR_CODE)
end
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 5
🧹 Nitpick comments (3)
core-services/mdms-v2/src/main/resources/db/migrate.sh (1)
3-6: Quote variables and validate required inputs
Currently debug statements and assignments are unquoted and may break if values contain spaces or are unset. Consider quoting and validatingSCHEMA_NAME. Remove or gate debug echo behind a verbose flag if not needed in production.-echo "the baseurl : $DB_URL" -schemasetter="?currentSchema=" -schemas=$SCHEMA_NAME -echo "the schemas : $schemas" +echo "Base URL: ${baseurl}" +schemasetter="?currentSchema=" +schemas="${SCHEMA_NAME:?Environment variable SCHEMA_NAME must be set}" +echo "Schemas to migrate: ${schemas}"core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/impl/MdmsDataRepositoryImpl.java (2)
82-88: Duplicate try/catch blocks – extract to a helper
searchV2andsearchcontain identical try/catch logic for schema substitution. Duplicated code increases maintenance cost and risk of divergence.-private String replaceSchema(String query, String tenantId){ - try{ - return multiStateInstanceUtil.replaceSchemaPlaceholder(query, tenantId); - }catch(InvalidTenantIdException e){ - throw new CustomException(INVALID_TENANT_ID_ERR_CODE, e.getMessage()); - } -}Then invoke:
query = replaceSchema(query, mdmsCriteriaV2.getTenantId());
98-105: Consider lowering query log level & include parameters separatelyLogging the full, resolved SQL at
INFOcan expose multi-tenant schema names and potentially sensitive literals in prod logs. A safer pattern is:log.debug("MDMS query: {} | params: {}", query, preparedStmtList);This keeps prod logs clean while retaining valuable data for debugging when
DEBUGis enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core-services/mdms-v2/pom.xml(1 hunks)core-services/mdms-v2/src/main/java/org/egov/infra/mdms/errors/ErrorCodes.java(1 hunks)core-services/mdms-v2/src/main/java/org/egov/infra/mdms/producer/Producer.java(1 hunks)core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/impl/MdmsDataRepositoryImpl.java(5 hunks)core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/impl/SchemaDefinitionDbRepositoryImpl.java(4 hunks)core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/querybuilder/MdmsDataQueryBuilder.java(1 hunks)core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/querybuilder/MdmsDataQueryBuilderV2.java(1 hunks)core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/querybuilder/SchemaDefinitionQueryBuilder.java(2 hunks)core-services/mdms-v2/src/main/resources/db/migrate.sh(1 hunks)
🔇 Additional comments (10)
core-services/mdms-v2/pom.xml (1)
90-90:❓ Verification inconclusive
Ensure the new snapshot version is published and compatible
Double-check thatorg.egov.services:services-common:2.9.0-SNAPSHOTis published to your snapshot Nexus repository and resolves correctly without introducing breaking changes in downstream modules.
🏁 Script executed:
#!/usr/bin/env bash # Verify artifact existence in Nexus snapshots curl -sI https://nexus-repo.digit.org/nexus/content/repositories/snapshots/org/egov/services/services-common/2.9.0-SNAPSHOT/Length of output: 336
Snapshot Artifact Verification Completed
The Nexus snapshot repository returns HTTP 200 fororg.egov.services:services-common:2.9.0-SNAPSHOT, so the artifact is indeed published.Please proceed to validate compatibility with downstream modules by:
- Running local builds or integration tests against this new snapshot.
- Checking for any API changes in
services-commonthat might impact consumers.core-services/mdms-v2/src/main/java/org/egov/infra/mdms/errors/ErrorCodes.java (1)
28-28: LGTM: Addition of error code for invalid tenant IDThe new error code constant follows the existing naming conventions and will be used for tenant validation in repository implementations.
core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/querybuilder/SchemaDefinitionQueryBuilder.java (2)
12-13: LGTM: Added import for schema placeholderAppropriate import for the dynamic schema replacement constant.
22-22: LGTM: Replaced hardcoded schema with dynamic placeholderGood implementation of tenant-aware schema resolution by replacing the hardcoded schema prefix with the dynamic placeholder.
core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/querybuilder/MdmsDataQueryBuilderV2.java (2)
13-14: LGTM: Added import for schema placeholderAppropriate import for the dynamic schema replacement constant.
22-22: LGTM: Replaced hardcoded schema with dynamic placeholderGood implementation of tenant-aware schema resolution by replacing the hardcoded schema reference with the dynamic placeholder.
core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/querybuilder/MdmsDataQueryBuilder.java (2)
9-10: LGTM: Added import for schema placeholderAppropriate import for the dynamic schema replacement constant.
15-15: LGTM: Replaced hardcoded schema with dynamic placeholderGood implementation of tenant-aware schema resolution by replacing the hardcoded schema reference with the dynamic placeholder.
core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/impl/SchemaDefinitionDbRepositoryImpl.java (1)
56-58: Propagation of tenant ID increatelooks good
The call now includes the tenant while publishing, matching the newProducer.pushsignature.core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/impl/MdmsDataRepositoryImpl.java (1)
63-72: Good alignment with tenant-aware producer
Bothcreateandupdatenow passtenantId. This prevents cross-tenant data on Kafka.
| #!/bin/bash | ||
| baseurl=$DB_URL |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce strict mode and improve portability
Add a portable shebang and enable strict error handling at the top of the script to fail fast and catch unbound variables.
Apply this diff:
-#!/bin/bash
-baseurl=$DB_URL
+#!/usr/bin/env bash
+set -euo pipefail
+# Base URL for Flyway migrations (fail if unset)
+baseurl="${DB_URL:?Environment variable DB_URL must be set}"📝 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.
| #!/bin/bash | |
| baseurl=$DB_URL | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| # Base URL for Flyway migrations (fail if unset) | |
| baseurl="${DB_URL:?Environment variable DB_URL must be set}" |
| for schemaname in ${schemas//,/ } | ||
| do | ||
| echo "the schema name : ${baseurl}${schemasetter}${schemaname}" | ||
| flyway -url=${baseurl}${schemasetter}${schemaname} -table=$SCHEMA_TABLE -user=$FLYWAY_USER -password=$FLYWAY_PASSWORD -locations=$FLYWAY_LOCATIONS -baselineOnMigrate=true -outOfOrder=true migrate | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote parameters in the loop and Flyway command
To avoid word-splitting and globbing issues, split the comma-separated list in a robust way, quote all expansions, and format the Flyway command for readability and safety.
-for schemaname in ${schemas//,/ }
-do
- echo "the schema name : ${baseurl}${schemasetter}${schemaname}"
- flyway -url=${baseurl}${schemasetter}${schemaname} -table=$SCHEMA_TABLE -user=$FLYWAY_USER -password=$FLYWAY_PASSWORD -locations=$FLYWAY_LOCATIONS -baselineOnMigrate=true -outOfOrder=true migrate
-done
+# Split schemas on comma into an array
+IFS=',' read -r -a schema_array <<< "${schemas}"
+for schemaname in "${schema_array[@]}"; do
+ target_url="${baseurl}${schemasetter}${schemaname}"
+ echo "Migrating schema '${schemaname}' with URL: '${target_url}'"
+ flyway \
+ -url="${target_url}" \
+ -table="${SCHEMA_TABLE}" \
+ -user="${FLYWAY_USER}" \
+ -password="${FLYWAY_PASSWORD}" \
+ -locations="${FLYWAY_LOCATIONS}" \
+ -baselineOnMigrate=true \
+ -outOfOrder=true \
+ migrate
+done
core-services/mdms-v2/src/main/java/org/egov/infra/mdms/producer/Producer.java
Outdated
Show resolved
Hide resolved
| public void push(String tenantId, String topic, Object value) { | ||
| String updatedTopic = multiStateInstanceUtil.getStateSpecificTopicName(tenantId, topic); | ||
| log.info("The Kafka topic for the tenantId : {} is : {}", tenantId, updatedTopic); | ||
| this.kafkaTemplate.send(updatedTopic, value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden the push contract (null / blank checks & async error handling)
tenantIdandtopicare accepted blindly. If either arrives asnull/blank,getStateSpecificTopicNamemay throw or silently produce an invalid topic.kafkaTemplate.send(..)is async and returns aListenableFuture. At present, failures will be swallowed.
Consider:
+Assert.hasText(tenantId, "tenantId must not be blank");
+Assert.hasText(topic, "topic must not be blank");
+
+ListenableFuture<SendResult<String, Object>> future =
+ kafkaTemplate.send(updatedTopic, value);
+future.addCallback(
+ success -> log.debug("Message sent to {}", updatedTopic),
+ ex -> log.error("Failed to publish to {}", updatedTopic, ex));This keeps the producer robust and surfaces publish failures upstream.
Committable suggestion skipped: line range outside the PR's diff.
| String query = schemaDefinitionQueryBuilder.getSchemaSearchQuery(schemaDefCriteria, preparedStatementList); | ||
| log.info("Schema definition search query: " + query); | ||
| try { | ||
| query = multiStateInstanceUtil.replaceSchemaPlaceholder(query, schemaDefCriteria.getTenantId()); | ||
| } catch (InvalidTenantIdException e) { | ||
| throw new CustomException(INVALID_TENANT_ID_ERR_CODE, e.getMessage()); | ||
| } | ||
| log.info("Schema definition search query: {}", query); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify placeholder replacement & guard against SQL-injection
replaceSchemaPlaceholder performs a string substitution of the schema name. If tenantId can be user-controlled, this opens the door to SQL–injection via schema names (e.g., foo; DROP TABLE X;). Unless MultiStateInstanceUtil rigorously validates against a strict whitelist/regex, we should either:
- Restrict
tenantIdto[a-zA-Z0-9_]+before substitution, or - Use JDBC
setCatalog()/setSchema()or a prepared-statement param rather than string concat.
Also, logging the fully-rendered query (log.info) may leak internal schema names in higher log levels. Consider downgrading to debug.
🏁 Script executed:
#!/bin/bash
# Locate the MultiStateInstanceUtil class and its implementation
rg -n "class MultiStateInstanceUtil" -A20 -C10
# Locate the replaceSchemaPlaceholder method to inspect tenantId validation
rg -n "replaceSchemaPlaceholder" -A10 -B5Length of output: 66168
🏁 Script executed:
#!/bin/bash
# Display the implementation of replaceSchemaPlaceholder in MultiStateInstanceUtil
rg -n "replaceSchemaPlaceholder" -A20 -B5 core-services/libraries/services-common/src/main/java/org/egov/common/utils/MultiStateInstanceUtil.javaLength of output: 1138
Sanitize tenantId before schema substitution & lower log level
The current replaceSchemaPlaceholder implementation (MultiStateInstanceUtil.java:51–66) does an unchecked regex replace of "{schema}" using a substring of any tenantId. A malicious tenantId like
foo; DROP TABLE users; --.tenantx
could inject SQL into the query. At minimum, enforce a strict whitelist (e.g. [A-Za-z0-9_]+) on the schema segment before replacement, or—preferably—use JDBC’s setSchema()/setCatalog() API or a parameterized approach rather than string replacement.
Additionally, logging the fully rendered query at INFO level leaks internal schema names; change this to DEBUG in:
• core-services/mdms-v2/src/main/java/org/egov/infra/mdms/repository/impl/SchemaDefinitionDbRepositoryImpl.java (line 76).
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores