Central Instance changes for MUKTASoft #842
Conversation
1. Estimate Service changes
WalkthroughThe changes introduce multi-state and tenant-aware capabilities to the estimate service. Kafka consumers and producers are updated to use tenant-specific topic patterns and message routing. SQL queries now support dynamic schema resolution based on tenant ID. New configuration properties and constants are added, and migration scripts are enhanced to support multi-schema Flyway migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant Kafka as Kafka Broker
participant Consumer as DenormalizeAndEnrichEstimateConsumer
participant Producer as EstimateProducer
participant Repo as EstimateRepository
participant MultiUtil as MultiStateInstanceUtil
Kafka->>Consumer: Receive message (topic pattern)
Consumer->>Consumer: Check topic matches configured save/update
Consumer->>Consumer: Set tenant ID in MDC
Consumer->>Producer: push(tenantId, topic, message)
Producer->>MultiUtil: Resolve topic for tenant
Producer->>Kafka: Send message to tenant-specific topic
Consumer->>Repo: (if needed) Query with checkCentralInstance
Repo->>MultiUtil: Replace {schema} with tenant schema
Poem
✨ 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
backend/estimates/src/main/java/org/egov/config/EstimateServiceConfiguration.java(1 hunks)backend/estimates/src/main/java/org/egov/consumer/DenormalizeAndEnrichEstimateConsumer.java(3 hunks)backend/estimates/src/main/java/org/egov/producer/EstimateProducer.java(1 hunks)backend/estimates/src/main/java/org/egov/repository/EstimateRepository.java(4 hunks)backend/estimates/src/main/java/org/egov/repository/rowmapper/EstimateQueryBuilder.java(1 hunks)backend/estimates/src/main/java/org/egov/service/EstimateService.java(3 hunks)backend/estimates/src/main/java/org/egov/service/NotificationService.java(3 hunks)backend/estimates/src/main/java/org/egov/util/EstimateServiceConstant.java(1 hunks)backend/estimates/src/main/resources/application.properties(1 hunks)backend/estimates/src/main/resources/db/migrate.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
backend/estimates/src/main/resources/db/migrate.sh
[warning] 7-7: In POSIX sh, string replacement is undefined.
(SC3060)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (22)
backend/estimates/src/main/java/org/egov/util/EstimateServiceConstant.java (1)
87-87: LGTM! Well-implemented constant addition.The constant follows proper naming conventions and aligns with the multi-tenant MDC logging pattern being introduced.
backend/estimates/src/main/java/org/egov/config/EstimateServiceConfiguration.java (1)
63-64: LGTM! Proper configuration property addition.The new
estimateTopicPatternproperty follows established patterns and correctly supports the tenant-aware Kafka topic handling being introduced.backend/estimates/src/main/java/org/egov/repository/rowmapper/EstimateQueryBuilder.java (1)
30-51: LGTM! Consistent multi-tenant schema pattern implementation.The systematic addition of
{schema}placeholders to all table references enables proper tenant-specific database schema resolution. The pattern is applied consistently across both fetch and count queries.backend/estimates/src/main/java/org/egov/service/EstimateService.java (3)
73-73: LGTM! Consistent tenant-aware message routing.The addition of tenant ID as the first parameter to
producer.pushenables proper tenant-specific Kafka topic resolution.
130-130: LGTM! Consistent tenant-aware message routing.The tenant ID parameter addition maintains consistency with the multi-tenant messaging pattern.
150-150: LGTM! Consistent tenant-aware message routing.The tenant ID parameter is correctly propagated for the previous estimate workflow status update.
backend/estimates/src/main/resources/application.properties (3)
134-136: Central instance configuration looks correct.The central instance properties are properly configured with:
state.level.tenantid.length=2for 2-character state codesis.environment.central.instance=trueto enable central instance modestate.level.tenant.id=odfor Odisha state identification
129-129: SMS additional field configuration added.The
sms.isAdditonalFieldRequired=trueproperty enables additional fields in SMS processing, which aligns with the notification service changes.
131-131: Verify the Kafka topic pattern regex for correctness.The regex pattern
((^[a-zA-Z]+-)?save-estimate|(^[a-zA-Z]+-)?update-estimate)should be validated to ensure it matches the intended topic naming convention. The pattern allows optional alphabetic prefixes followed by hyphens before the base topic names.#!/bin/bash # Test the regex pattern against various topic names to ensure it matches correctly echo "Testing Kafka topic pattern regex..." # Test cases for the pattern pattern="((^[a-zA-Z]+-)?save-estimate|(^[a-zA-Z]+-)?update-estimate)" test_topics=( "save-estimate" "update-estimate" "od-save-estimate" "od-update-estimate" "punjab-save-estimate" "punjab-update-estimate" "123-save-estimate" # Should not match (numeric prefix) "save-estimate-extra" # Should not match (extra suffix) "other-topic" # Should not match ) for topic in "${test_topics[@]}"; do if echo "$topic" | grep -qE "^${pattern}$"; then echo "✓ $topic matches" else echo "✗ $topic does not match" fi donebackend/estimates/src/main/java/org/egov/repository/EstimateRepository.java (4)
33-34: Autowired dependency injection is correct.The
MultiStateInstanceUtilis properly injected using@Autowiredannotation.
78-86: Error handling in checkCentralInstance method is appropriate.The method properly catches
InvalidTenantIdExceptionand converts it to aCustomExceptionwith a descriptive error message. The schema placeholder replacement logic is correctly implemented.
58-58: Query execution with tenant-aware schema resolution.The
getEstimatemethod correctly applies schema placeholder replacement before executing the query, ensuring tenant-specific database schema access.
75-75: Count query execution with tenant-aware schema resolution.The
getEstimateCountmethod consistently applies the same schema placeholder replacement pattern as the main query method.backend/estimates/src/main/java/org/egov/producer/EstimateProducer.java (1)
15-16: MultiStateInstanceUtil dependency injection is correct.The
MultiStateInstanceUtilis properly injected using@Autowiredannotation for tenant-aware topic routing.backend/estimates/src/main/java/org/egov/service/NotificationService.java (4)
106-106: Tenant ID parameter correctly passed to notification method.The
checkAdditionalFieldAndPushONSmsTopicmethod call correctly includes the tenant ID extracted from the estimate object.
131-131: Consistent tenant ID parameter passing in approve action.The tenant ID is consistently passed to the notification method in the approve action path, maintaining consistency with the reject action.
291-291: Method signature updated to include tenant ID parameter.The
checkAdditionalFieldAndPushONSmsTopicmethod signature correctly includes thetenantIdparameter for tenant-aware message routing.
298-298: Producer calls updated with tenant ID for both SMS types.Both SMS request types (with and without additional fields) correctly include the tenant ID as the first parameter when calling the producer, ensuring consistent tenant-aware message routing.
Also applies to: 303-303
backend/estimates/src/main/java/org/egov/consumer/DenormalizeAndEnrichEstimateConsumer.java (4)
31-32: State-level tenant ID configuration injection is correct.The
@Valueannotation properly injects thestate.level.tenant.idconfiguration property for use in MDC context.
58-59: Topic pattern listening replaces specific topic listening.The change from specific topics to topic pattern listening enables multi-tenant topic consumption. The commented-out line shows the previous implementation for reference.
66-75: Topic matching logic and MDC context management.The implementation correctly checks if the received topic matches the configured save or update topics, sets the MDC context with the tenant ID, and processes the message accordingly. The conditional logic ensures only relevant topics are processed.
71-71: Producer call updated with tenant ID from estimate.The producer call correctly includes the tenant ID extracted from the estimate object, ensuring tenant-aware message routing for the enriched estimate.
| 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 -ignoreMissingMigrations=true migrate | ||
| done |
There was a problem hiding this comment.
Fix shell compatibility and quoting issues.
The script has several issues that should be addressed:
- POSIX compatibility: The string replacement
${schemas//,/ }is bash-specific and won't work in POSIX sh - Missing quotes: Multiple variables lack proper quoting which can cause word splitting and globbing issues
Apply this diff to fix the issues:
-#!/bin/sh
+#!/bin/bash
baseurl=$DB_URL
echo "the baseurl : $DB_URL"
schemasetter="?currentSchema="
schemas=$SCHEMA_NAME
echo "the schemas : $schemas"
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 -ignoreMissingMigrations=true migrate
+ flyway -url="${baseurl}${schemasetter}${schemaname}" -table="$SCHEMA_TABLE" -user="$FLYWAY_USER" -password="$FLYWAY_PASSWORD" -locations="$FLYWAY_LOCATIONS" -baselineOnMigrate=true -outOfOrder=true -ignoreMissingMigrations=true migrate
doneAlternatively, if you must use POSIX sh, replace the string substitution with a different approach:
#!/bin/sh
baseurl=$DB_URL
echo "the baseurl : $DB_URL"
schemasetter="?currentSchema="
-schemas=$SCHEMA_NAME
-echo "the schemas : $schemas"
-for schemaname in ${schemas//,/ }
+echo "the schemas : $SCHEMA_NAME"
+IFS=','
+for schemaname in $SCHEMA_NAME
do
+ IFS=' '
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 -ignoreMissingMigrations=true migrate
+ flyway -url="${baseurl}${schemasetter}${schemaname}" -table="$SCHEMA_TABLE" -user="$FLYWAY_USER" -password="$FLYWAY_PASSWORD" -locations="$FLYWAY_LOCATIONS" -baselineOnMigrate=true -outOfOrder=true -ignoreMissingMigrations=true migrate
done🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 7-7: In POSIX sh, string replacement is undefined.
(SC3060)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
In backend/estimates/src/main/resources/db/migrate.sh around lines 7 to 11, the
script uses bash-specific string replacement ${schemas//,/ } which breaks POSIX
sh compatibility, and variables are unquoted causing potential word splitting
and globbing issues. To fix, replace the string substitution with a
POSIX-compatible method such as using IFS and read or a loop to split the
schemas variable by commas, and add double quotes around all variable expansions
to prevent word splitting and globbing.
| public void push(String tenantId, String topic, Object value) { | ||
| //kafkaTemplate.send(topic, value); | ||
| String updatedTopic = centralInstanceUtil.getStateSpecificTopicName(tenantId, topic); | ||
| kafkaTemplate.send(updatedTopic, value); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove commented code and improve method documentation.
The method signature change to include tenantId is correct for multi-tenant support. However, the commented-out code should be removed for cleaner code.
public void push(String tenantId, String topic, Object value) {
- //kafkaTemplate.send(topic, value);
String updatedTopic = centralInstanceUtil.getStateSpecificTopicName(tenantId, topic);
kafkaTemplate.send(updatedTopic, value);
}Consider adding JavaDoc to document the new parameter:
+/**
+ * Pushes a message to a tenant-specific Kafka topic
+ * @param tenantId The tenant ID for state-specific topic routing
+ * @param topic The base topic name
+ * @param value The message payload
+ */
public void push(String tenantId, String topic, Object value) {
String updatedTopic = centralInstanceUtil.getStateSpecificTopicName(tenantId, topic);
kafkaTemplate.send(updatedTopic, value);
}🤖 Prompt for AI Agents
In backend/estimates/src/main/java/org/egov/producer/EstimateProducer.java
around lines 23 to 27, remove the commented-out kafkaTemplate.send(topic, value)
line to clean up the code. Additionally, add JavaDoc comments to the push method
to describe its purpose and parameters, including the new tenantId parameter, to
improve code documentation and clarity.
| EstimateRequest enrichedEstimateRequest = denormalizeAndEnrichEstimateService.denormalizeAndEnrich(estimateRequest); | ||
| producer.push(serviceConfiguration.getEnrichEstimateTopic(), enrichedEstimateRequest); | ||
|
|
||
| if (topic.contains(serviceConfiguration.getSaveEstimateTopic()) || topic.contains(serviceConfiguration.getUpdateEstimateTopic())){ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using String.equals() for topic comparison.
The current implementation uses contains() which could potentially match unintended topics. Consider using exact string matching or more precise pattern matching for better reliability.
-if (topic.contains(serviceConfiguration.getSaveEstimateTopic()) || topic.contains(serviceConfiguration.getUpdateEstimateTopic())){
+if (topic.equals(serviceConfiguration.getSaveEstimateTopic()) ||
+ topic.equals(serviceConfiguration.getUpdateEstimateTopic()) ||
+ topic.endsWith("-" + serviceConfiguration.getSaveEstimateTopic()) ||
+ topic.endsWith("-" + serviceConfiguration.getUpdateEstimateTopic())){This approach would be more precise for tenant-prefixed topics while avoiding false matches.
📝 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.
| if (topic.contains(serviceConfiguration.getSaveEstimateTopic()) || topic.contains(serviceConfiguration.getUpdateEstimateTopic())){ | |
| if (topic.equals(serviceConfiguration.getSaveEstimateTopic()) || | |
| topic.equals(serviceConfiguration.getUpdateEstimateTopic()) || | |
| topic.endsWith("-" + serviceConfiguration.getSaveEstimateTopic()) || | |
| topic.endsWith("-" + serviceConfiguration.getUpdateEstimateTopic())) { |
🤖 Prompt for AI Agents
In
backend/estimates/src/main/java/org/egov/consumer/DenormalizeAndEnrichEstimateConsumer.java
at line 66, replace the use of topic.contains(...) with exact string matching
using String.equals() to avoid unintended matches. Update the condition to
compare the topic string exactly with
serviceConfiguration.getSaveEstimateTopic() and
serviceConfiguration.getUpdateEstimateTopic() using equals() or
equalsIgnoreCase() as appropriate, ensuring more precise and reliable topic
matching.
Central Instance changes for MUKTASoft
Summary by CodeRabbit
New Features
Enhancements
Chores