Update InboxQueryBuilder.java for ethopia staging #658
Conversation
ashish-egov
requested review from
talele08,
kavi-egov and
shashwat-egov
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces formatting improvements and code readability enhancements across multiple files in the inbox service. The changes primarily focus on standardizing code style, adding new utility methods in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
accelerators/inbox/src/main/resources/application.properties (1)
Line range hint
8-11: Security: Externalize sensitive configuration.Several sensitive configurations are hardcoded in the properties file:
- Database credentials
- Service endpoints
- Tenant IDs
Consider using environment variables or a secure configuration management system.
Example secure configuration:
spring.datasource.url=${POSTGRES_URL} spring.datasource.username=${POSTGRES_USER} spring.datasource.password=${POSTGRES_PASSWORD} parent.level.tenant.id=${PARENT_TENANT_ID} state.level.tenant.id=${STATE_TENANT_ID}Also applies to: 61-62, 105-105, 125-125
🧹 Nitpick comments (8)
accelerators/inbox/src/main/resources/application.properties (1)
Line range hint
1-126: Improve environment configuration management.The file contains a mix of development (localhost) and production URLs. Consider:
- Separating configurations by environment
- Using Spring profiles
- Implementing a configuration server
Example structure:
src/main/resources/ ├── application.properties (common properties) ├── application-dev.properties ├── application-staging.properties └── application-prod.propertiesaccelerators/inbox/src/main/java/org/egov/inbox/service/V2/InboxServiceV2.java (2)
Line range hint
90-107: Consider using Java 8 Stream API for better readability.The nested if conditions and loops can be simplified using Stream API for better maintainability.
private void hashParamsWhereverRequiredBasedOnConfiguration(Map<String, Object> moduleSearchCriteria, InboxQueryConfiguration inboxQueryConfiguration) { - inboxQueryConfiguration.getAllowedSearchCriteria().forEach(searchParam -> { - if (!ObjectUtils.isEmpty(searchParam.getIsHashingRequired()) && searchParam.getIsHashingRequired()) { - if (moduleSearchCriteria.containsKey(searchParam.getName())) { - if (moduleSearchCriteria.get(searchParam.getName()) instanceof List) { - List<Object> hashedParams = new ArrayList<>(); - ((List<?>) moduleSearchCriteria.get(searchParam.getName())).forEach(object -> { - hashedParams.add(hashService.getHashValue(object)); - }); - moduleSearchCriteria.put(searchParam.getName(), hashedParams); - } else { - Object hashedValue = hashService.getHashValue(moduleSearchCriteria.get(searchParam.getName())); - moduleSearchCriteria.put(searchParam.getName(), hashedValue); - } - } - } - }); + inboxQueryConfiguration.getAllowedSearchCriteria().stream() + .filter(searchParam -> !ObjectUtils.isEmpty(searchParam.getIsHashingRequired()) + && searchParam.getIsHashingRequired() + && moduleSearchCriteria.containsKey(searchParam.getName())) + .forEach(searchParam -> { + String paramName = searchParam.getName(); + Object paramValue = moduleSearchCriteria.get(paramName); + if (paramValue instanceof List) { + List<Object> hashedParams = ((List<?>) paramValue).stream() + .map(hashService::getHashValue) + .collect(Collectors.toList()); + moduleSearchCriteria.put(paramName, hashedParams); + } else { + moduleSearchCriteria.put(paramName, hashService.getHashValue(paramValue)); + } + });
Line range hint
387-403: Extract business service SLA count logic into a separate method.The loop body in the
getApplicationsNearingSlaCountmethod is complex and should be extracted into a separate method for better readability and maintainability.for (int i = 0; i < businessServices.size(); i++) { String businessService = businessServices.get(i); Long businessServiceSla = businessServiceSlaMap.get(businessService); - inboxRequest.getInbox().getProcessSearchCriteria() - .setStatus(businessServiceVsUuidsBasedOnSearchCriteria.get(businessService)); - Map<String, Object> finalQueryBody = queryBuilder.getNearingSlaCountQuery(inboxRequest, businessServiceSla); - StringBuilder uri = getURI(indexName, COUNT_PATH); - Map<String, Object> response = (Map<String, Object>) serviceRequestRepository.fetchResult(uri, - finalQueryBody); - Integer currentCount = 0; - if (response.containsKey(COUNT_CONSTANT)) { - currentCount = (Integer) response.get(COUNT_CONSTANT); - } else { - throw new CustomException("INBOX_COUNT_ERR", "Error occurred while executing ES count query"); - } - totalCount += currentCount; + totalCount += getBusinessServiceSlaCount(inboxRequest, indexName, businessService, + businessServiceSla, businessServiceVsUuidsBasedOnSearchCriteria); } +private Integer getBusinessServiceSlaCount(InboxRequest inboxRequest, String indexName, + String businessService, Long businessServiceSla, + Map<String, List<String>> businessServiceVsUuidsBasedOnSearchCriteria) { + inboxRequest.getInbox().getProcessSearchCriteria() + .setStatus(businessServiceVsUuidsBasedOnSearchCriteria.get(businessService)); + Map<String, Object> finalQueryBody = queryBuilder.getNearingSlaCountQuery(inboxRequest, businessServiceSla); + StringBuilder uri = getURI(indexName, COUNT_PATH); + Map<String, Object> response = (Map<String, Object>) serviceRequestRepository.fetchResult(uri, + finalQueryBody); + if (response.containsKey(COUNT_CONSTANT)) { + return (Integer) response.get(COUNT_CONSTANT); + } + throw new CustomException("INBOX_COUNT_ERR", "Error occurred while executing ES count query"); +}accelerators/inbox/src/main/java/org/egov/inbox/service/WorkflowService.java (5)
65-67: Enhance error handling with more context.The error message could be more descriptive by including the actual parsing error details.
- throw new CustomException(ErrorConstants.PARSING_ERROR, - "Failed to parse response of ProcessInstance Count"); + throw new CustomException(ErrorConstants.PARSING_ERROR, + String.format("Failed to parse response of ProcessInstance Count. Error: %s", e.getMessage()));
154-156: Extract FSM-specific logic for better modularity.The FSM-specific logic should be extracted to maintain consistency with the separation of concerns principle.
+ private void appendFSMAssignee(StringBuilder url, RequestInfo requestInfo) { + if (requestInfo.getUserInfo().getRoles().get(0).getCode().equals(FSMConstants.FSM_DSO)) { + url.append("&assignee=").append(requestInfo.getUserInfo().getUuid()); + } + } public ProcessInstanceResponse getProcessInstance(ProcessInstanceSearchCriteria criteria, RequestInfo requestInfo) { StringBuilder url = new StringBuilder(config.getWorkflowHost()); url.append(config.getProcessSearchPath()); url = this.buildWorkflowUrl(criteria, url, Boolean.FALSE); - if (requestInfo.getUserInfo().getRoles().get(0).getCode().equals(FSMConstants.FSM_DSO)) { - url.append("&assignee=").append(requestInfo.getUserInfo().getUuid()); - } + appendFSMAssignee(url, requestInfo);
210-243: Add null safety and documentation to mapping methods.The mapping methods could benefit from:
- Null checks for input collections
- JavaDoc documentation explaining the purpose and return values
- Immutable return types for better encapsulation
+ /** + * Creates a mapping of status UUIDs to their corresponding business service names. + * + * @param businessServices List of business services to process + * @return Immutable map of status UUID to business service name + * @throws CustomException if businessServices is null + */ public Map<String, String> getStatusIdToBusinessServiceMap(List<BusinessService> businessServices) { + if (businessServices == null) { + throw new CustomException(ErrorConstants.INVALID_INPUT, "Business services list cannot be null"); + } Map<String, String> statusIdToBusinessServiceMap = new HashMap<>(); businessServices.forEach(businessService -> { businessService.getStates().forEach(state -> { statusIdToBusinessServiceMap.put(state.getUuid(), businessService.getBusinessService()); }); }); - return statusIdToBusinessServiceMap; + return Collections.unmodifiableMap(statusIdToBusinessServiceMap); }
250-292: Refactor URL building using builder pattern.The URL building logic is complex and could be more maintainable using a builder pattern.
+ private static class WorkflowUrlBuilder { + private final StringBuilder url; + private final ProcessInstanceSearchCriteria criteria; + private final boolean noStatus; + + private WorkflowUrlBuilder(StringBuilder baseUrl, ProcessInstanceSearchCriteria criteria, boolean noStatus) { + this.url = baseUrl; + this.criteria = criteria; + this.noStatus = noStatus; + this.url.append("?tenantId=").append(criteria.getTenantId()); + } + + public WorkflowUrlBuilder appendStatus() { + if (!CollectionUtils.isEmpty(criteria.getStatus()) && !noStatus) { + url.append("&status=").append(StringUtils.arrayToDelimitedString(criteria.getStatus().toArray(), ",")); + } + return this; + } + + // Add similar methods for other parameters + + public StringBuilder build() { + return url; + } + } + private StringBuilder buildWorkflowUrl(ProcessInstanceSearchCriteria criteria, StringBuilder url, boolean noStatus) { - url.append("?tenantId=").append(criteria.getTenantId()); - if (!CollectionUtils.isEmpty(criteria.getStatus()) && noStatus == Boolean.FALSE) { - url.append("&status=").append(StringUtils.arrayToDelimitedString(criteria.getStatus().toArray(), ",")); - } - // ... rest of the conditions - return url; + return new WorkflowUrlBuilder(url, criteria, noStatus) + .appendStatus() + .appendBusinessIds() + .appendIds() + // ... chain other methods + .build(); }
326-366: Optimize role-based access control implementation.The role-based access control implementation could be improved by:
- Adding input validation
- Using Set instead of List for role lookups
- Caching the role mappings
+ @Cacheable(value = "actionableStatuses", key = "#requestInfo.userInfo.uuid") public HashMap<String, String> getActionableStatusesForRole(RequestInfo requestInfo, List<BusinessService> businessServices, ProcessInstanceSearchCriteria criteria) { + if (requestInfo == null || requestInfo.getUserInfo() == null) { + throw new CustomException(ErrorConstants.INVALID_INPUT, "Request info and user info are required"); + } String tenantId; - List<String> userRoleCodes; + Set<String> userRoleCodes; // Use Set for O(1) lookups Map<String, List<String>> tenantIdToUserRolesMap = getTenantIdToUserRolesMap(requestInfo); // ... rest of the method }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
accelerators/inbox/src/main/java/org/egov/inbox/repository/builder/V2/InboxQueryBuilder.java(9 hunks)accelerators/inbox/src/main/java/org/egov/inbox/service/V2/InboxServiceV2.java(7 hunks)accelerators/inbox/src/main/java/org/egov/inbox/service/WorkflowService.java(5 hunks)accelerators/inbox/src/main/resources/application.properties(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- accelerators/inbox/src/main/java/org/egov/inbox/repository/builder/V2/InboxQueryBuilder.java
🔇 Additional comments (2)
accelerators/inbox/src/main/resources/application.properties (1)
126-126: LGTM: Cache expiry configuration added.The 10-minute cache expiry duration is reasonable for an inbox service to balance between performance and data freshness.
accelerators/inbox/src/main/java/org/egov/inbox/service/V2/InboxServiceV2.java (1)
66-87: 🛠️ Refactor suggestionImprove null handling in getInboxResponse method.
The method should handle potential null values from
mdmsUtil.getConfigFromMDMS()to prevent NullPointerException.public InboxResponse getInboxResponse(InboxRequest inboxRequest) { validator.validateSearchCriteria(inboxRequest); InboxQueryConfiguration inboxQueryConfiguration = mdmsUtil.getConfigFromMDMS( inboxRequest.getInbox().getTenantId(), inboxRequest.getInbox().getProcessSearchCriteria().getModuleName()); + if (inboxQueryConfiguration == null) { + throw new CustomException("INVALID_CONFIG", + "No inbox configuration found for the given module"); + } hashParamsWhereverRequiredBasedOnConfiguration(inboxRequest.getInbox().getModuleSearchCriteria(), inboxQueryConfiguration);
accelerators/inbox/src/main/java/org/egov/inbox/service/V2/InboxServiceV2.java
Show resolved
Hide resolved
accelerators/inbox/src/main/java/org/egov/inbox/service/V2/InboxServiceV2.java
Show resolved
Hide resolved
accelerators/inbox/src/main/java/org/egov/inbox/service/V2/InboxServiceV2.java
Show resolved
Hide resolved
accelerators/inbox/src/main/java/org/egov/inbox/service/WorkflowService.java
Outdated
Show resolved
Hide resolved
accelerators/inbox/src/main/java/org/egov/inbox/service/WorkflowService.java
Show resolved
Hide resolved
Update WorkflowService.java
Update InboxServiceV2.java
Summary by CodeRabbit
Style
Chores
pbtopgNew Features
Note: These changes primarily focus on code maintenance and minor configuration updates with no significant user-facing impact.