[HCMPRE-2739] central instance changes for expense service #1916
Conversation
WalkthroughThe changes introduce multi-tenant support across the expense backend by dynamically resolving schema names in SQL queries and Kafka topics based on tenant IDs. This is achieved by injecting a utility for schema/topic resolution, updating repository and producer classes, and modifying service methods to pass tenant IDs. The Flyway migration script is also updated to support migrations for multiple schemas. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Service Layer (Bill/Payment/Notification)
participant Producer as ExpenseProducer
participant Util as MultiStateInstanceUtil
participant Kafka as Kafka
Service->>Service: Extract tenantId from request
Service->>Producer: push(tenantId, topic, payload)
Producer->>Util: getStateSpecificTopicName(tenantId, topic)
Util-->>Producer: stateSpecificTopic
Producer->>Kafka: Send payload to stateSpecificTopic
sequenceDiagram
participant Repository as Bill/PaymentRepository
participant Util as MultiStateInstanceUtil
participant DB as Database
Repository->>Repository: Build SQL with schema placeholder
Repository->>Util: replaceSchemaPlaceholder(query, tenantId)
alt Valid tenantId
Util-->>Repository: queryWithSchema
Repository->>DB: Execute queryWithSchema
else Invalid tenantId
Util-->>Repository: throw InvalidTenantIdException
Repository->>Repository: Throw CustomException("INVALID_TENANT_ID")
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java (2)🧬 Code Graph Analysis (1)backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java (2)
🔇 Additional comments (4)
✨ 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: 7
♻️ Duplicate comments (1)
backend/expense/src/main/java/org/egov/digit/expense/config/Constants.java (1)
112-115: Duplicate: dynamic schema injection in COUNT query
Same as above, this COUNT query also injectsSCHEMA_REPLACE_STRING. Ensure the placeholder is sanitized and consider applying the same refactor (query builder or parameterized approach).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
backend/expense/pom.xml(1 hunks)backend/expense/src/main/java/org/egov/digit/expense/config/Constants.java(3 hunks)backend/expense/src/main/java/org/egov/digit/expense/kafka/ExpenseProducer.java(1 hunks)backend/expense/src/main/java/org/egov/digit/expense/repository/BillRepository.java(2 hunks)backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java(2 hunks)backend/expense/src/main/java/org/egov/digit/expense/service/BillService.java(5 hunks)backend/expense/src/main/java/org/egov/digit/expense/service/NotificationService.java(2 hunks)backend/expense/src/main/java/org/egov/digit/expense/service/PaymentService.java(4 hunks)backend/expense/src/main/resources/db/migrate.sh(1 hunks)
🔇 Additional comments (19)
backend/expense/pom.xml (1)
111-115: Addition of services-common dependency looks goodThis change properly adds the services-common library that provides multi-tenant support through utilities like
MultiStateInstanceUtil, which is essential for the schema/topic resolution based on tenant IDs.backend/expense/src/main/java/org/egov/digit/expense/service/NotificationService.java (4)
41-41: Good extraction of tenant ID for multi-tenant supportThis change correctly extracts the tenant ID from the bill request for use in the notification processing pipeline.
64-64: Properly updated producer call with tenant IDThe
expenseProducer.pushmethod call now correctly includes the tenant ID as the first parameter, which aligns with the multi-tenant Kafka topic resolution approach.
69-69: Good extraction of tenant ID for multi-tenant supportSimilar to the previous method, correctly extracting tenant ID for multi-tenant notification handling in supervision bills.
77-77: Properly updated producer call with tenant IDThe producer call correctly includes the tenant ID, ensuring notifications are sent to the appropriate tenant-specific Kafka topic.
backend/expense/src/main/java/org/egov/digit/expense/service/BillService.java (6)
67-67: Good extraction of tenant ID for multi-tenant supportThis change correctly extracts the tenant ID from the bill object for use in tenant-specific processing.
94-94: Properly updated producer call with tenant IDThe
expenseProducer.pushmethod call now includes the tenant ID parameter, enabling proper resolution of tenant-specific Kafka topics.
113-113: Good extraction of tenant ID for multi-tenant supportConsistent with the create method, this correctly extracts the tenant ID for the update operation.
136-136: Properly updated producer call with tenant IDThe producer call correctly includes the tenant ID for topic resolution during bill updates.
204-204: Good extraction of tenant ID for batch processingCorrectly extracts tenant ID for use in the batch processing of bill details.
211-211: Properly updated producer call with tenant ID for batch processingThe producer call within the batch processing loop correctly includes the tenant ID parameter, ensuring consistent multi-tenant handling.
backend/expense/src/main/java/org/egov/digit/expense/service/PaymentService.java (6)
62-62: Good extraction of tenant ID for multi-tenant supportThis change correctly extracts the tenant ID from the payment object for use in payment processing.
66-66: Properly updated producer call with tenant IDThe producer call now correctly includes the tenant ID, enabling proper resolution of tenant-specific Kafka topics for payment creation.
80-80: Good extraction of tenant ID for payment updatesConsistent tenant ID extraction for the payment update operation.
87-87: Properly updated producer call with tenant IDThe producer call correctly includes the tenant ID for payment updates.
118-118: Good extraction of tenant ID for bill back-updatesCorrectly extracts tenant ID for use in the bill back-update process after payment processing.
187-187: Properly updated producer call with tenant IDThe producer call for bill updates in the back-update logic correctly includes the tenant ID.
backend/expense/src/main/java/org/egov/digit/expense/config/Constants.java (2)
10-10: Static import of SCHEMA_REPLACE_STRING is correctly added
Using a static import here keeps the SQL constants cleaner and aligns with how the schema placeholder is used throughout the file.
100-109:❓ Verification inconclusive
Verify and sanitize the dynamic schema placeholder in your SQL
You now injectSCHEMA_REPLACE_STRINGdirectly into raw SQL; please confirm thatMultiStateInstanceUtilstrictly validates and escapes tenant‐derived schema names to prevent any risk of SQL injection.
Additionally, consider using a query builder or parameterized access layer for dynamic schema resolution to improve maintainability.
🏁 Script executed:
#!/bin/bash # Locate the SCHEMA_REPLACE_STRING constant declaration rg -n "SCHEMA_REPLACE_STRING" . # Find the MultiStateInstanceUtil class and any references to schema validation rg -n "MultiStateInstanceUtil" .Length of output: 9259
Verify and Sanitize
SCHEMA_REPLACE_STRINGUsage
Please ensure that your dynamic schema placeholder cannot be abused for SQL injection and consider a more robust approach:
- Confirm that
MultiStateInstanceUtil.SCHEMA_REPLACE_STRINGstrictly validates and escapes tenant-derived schema names before substitution.- Review all usages in backend/expense/src/main/java/org/egov/digit/expense/config/Constants.java (e.g., lines 100–109, 112–114, 145–151, 153–158) to ensure consistency.
- For long-term maintainability and safety, evaluate using a query builder or a parameterized access layer that supports dynamic schema resolution.
backend/expense/src/main/java/org/egov/digit/expense/kafka/ExpenseProducer.java
Show resolved
Hide resolved
backend/expense/src/main/java/org/egov/digit/expense/repository/BillRepository.java
Show resolved
Hide resolved
backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java
Outdated
Show resolved
Hide resolved
backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java
Outdated
Show resolved
Hide resolved
backend/expense/src/main/java/org/egov/digit/expense/config/Constants.java
Show resolved
Hide resolved
backend/expense/src/main/java/org/egov/digit/expense/config/Constants.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
backend/expense/src/main/java/org/egov/digit/expense/kafka/ExpenseProducer.java (1)
31-35: 🛠️ Refactor suggestionAdd null-safety & failure handling around topic resolution
The
pushmethod still forwardstenantIdandtopicdirectly tomultiStateInstanceUtil.getStateSpecificTopicName. Anullor empty value for either argument will bubble up as an NPE or an unchecked exception from the util, taking down the calling thread. Guard-clauses plus a try/catch would make this producer safer (same concern flagged in an earlier review).+ if (tenantId == null || tenantId.isBlank() || topic == null || topic.isBlank()) { + log.error("Kafka push aborted – tenantId/topic is blank. tenantId: {}, topic: {}", tenantId, topic); + throw new IllegalArgumentException("tenantId and topic must be non-blank"); + } + String updatedTopic = multiStateInstanceUtil.getStateSpecificTopicName(tenantId, topic); log.info("The Kafka topic for the tenantId : {} is : {}", tenantId, updatedTopic); - this.kafkaTemplate.send(updatedTopic, value); + try { + this.kafkaTemplate.send(updatedTopic, value); + } catch (Exception ex) { + log.error("Failed to publish to topic {} for tenant {}", updatedTopic, tenantId, ex); + throw ex; + }backend/expense/src/main/java/org/egov/digit/expense/repository/BillRepository.java (1)
42-48: 🧹 Nitpick (assertive)Extract repeated schema-replacement logic into a private helper
The
try-catchblock that callsmultiStateInstanceUtil.replaceSchemaPlaceholderis duplicated in bothsearchandsearchCount. Encapsulating it in aprivate String applySchemaReplacement(String query, String tenantId)(or moving it to a common base/utility class shared by repositories) will eliminate duplication and make it easier to evolve error handling in one place.Also applies to: 54-59
backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java (1)
44-50: 🧹 Nitpick (assertive)Centralise schema-placeholder replacement for maintainability
As with
BillRepository, the identicaltry-catchblock appears in bothsearchandcount. Extracting it into a helper will reduce repeated code and the risk of diverging behaviour between methods or repositories.Also applies to: 56-61
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
backend/expense/src/main/java/org/egov/digit/expense/kafka/ExpenseProducer.java(1 hunks)backend/expense/src/main/java/org/egov/digit/expense/repository/BillRepository.java(2 hunks)backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java (2)
backend/expense/src/main/java/org/egov/digit/expense/repository/BillRepository.java (1)
Repository(19-62)backend/expense/src/main/java/org/egov/digit/expense/config/Constants.java (1)
Constants(12-161)
backend/expense/src/main/java/org/egov/digit/expense/repository/BillRepository.java
Show resolved
Hide resolved
backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java
Show resolved
Hide resolved
backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java (2)
Learnt from: holashchand
PR: egovernments/DIGIT-Works#1916
File: backend/expense/src/main/java/org/egov/digit/expense/repository/BillRepository.java:42-47
Timestamp: 2025-05-22T10:39:15.595Z
Learning: In the DIGIT-Works project's expense service, tenant ID validation is performed upstream before reaching the repository layer, so null checks on tenant ID in the repository methods are not necessary.
Learnt from: holashchand
PR: egovernments/DIGIT-Works#1916
File: backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java:45-49
Timestamp: 2025-05-22T10:39:45.956Z
Learning: In the DIGIT-Works application, tenant ID validation occurs at a higher level (controller/API layer) before requests reach the service/repository layer, so null checks for tenant ID in repositories like PaymentRepository are unnecessary.
🔇 Additional comments (4)
backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java (4)
8-9: LGTM!The imports are correctly added to support multi-tenant schema resolution functionality.
Also applies to: 14-14, 19-20
30-31: LGTM!The field declaration follows the established pattern and is properly encapsulated.
44-50: LGTM!The schema replacement implementation is correct with proper error handling. The approach is consistent with the application's architecture where tenant validation occurs upstream.
56-61: LGTM!The schema replacement implementation in the count method is consistent with the search method and follows the same error handling pattern.
backend/expense/src/main/java/org/egov/digit/expense/repository/PaymentRepository.java
Outdated
Show resolved
Hide resolved
|
|
closing as already merged in another pr 1935 |
Summary by CodeRabbit