added cache while project create for fetching parent project ids #1576
Conversation
WalkthroughRedis caching was integrated into the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProjectService
participant Redis
participant Database
participant Kafka
Client->>ProjectService: createProject(ProjectRequest)
ProjectService->>ProjectService: Validate and enrich request
ProjectService->>Kafka: Push project request
loop For each project
ProjectService->>Redis: Cache projectHierarchy with TTL
Redis-->>ProjectService: Acknowledge caching
end
ProjectService-->>Client: Return result
sequenceDiagram
participant Client
participant ProjectService
participant Redis
participant Database
Client->>ProjectService: getParentProjects(ProjectRequest)
ProjectService->>Redis: Fetch parent projectHierarchies
alt Hierarchy found in Redis
Redis-->>ProjectService: Return cached hierarchy
else Hierarchy not in Redis
Redis-->>ProjectService: Return miss
ProjectService->>Database: Query for missing parents
Database-->>ProjectService: Return parent projects
ProjectService->>Redis: Cache fetched hierarchies
end
ProjectService-->>Client: Return combined parent projects
sequenceDiagram
participant Validator
participant Redis
participant Database
Validator->>Redis: Check project ID keys
alt IDs found in Redis
Redis-->>Validator: Return valid IDs
else Some IDs missing or Redis error
Validator->>Database: Validate missing IDs
Database-->>Validator: Return valid IDs
end
Validator-->>Caller: Return combined valid project IDs
Suggested reviewers
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: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
health-services/project/src/main/java/org/egov/project/service/ProjectService.java(5 hunks)
🔇 Additional comments (2)
health-services/project/src/main/java/org/egov/project/service/ProjectService.java (2)
4-5: LGTM! Appropriate imports for Redis caching functionality.The new imports correctly support the Redis caching implementation with Duration for TTL management and Collections for empty list handling.
Also applies to: 24-24
54-65: LGTM! Constructor follows good dependency injection practices.The constructor properly injects dependencies and inline ObjectMapper initialization is appropriate for a stateless utility.
health-services/project/src/main/java/org/egov/project/service/ProjectService.java
Show resolved
Hide resolved
| /* Search for parent projects based on "parent" field and returns parent projects */ | ||
| private List<Project> getParentProjects(ProjectRequest projectRequest) { | ||
| List<Project> parentProjects = new ArrayList<>(); | ||
|
|
||
| List<Project> projectsWithParent = projectRequest.getProjects().stream() | ||
| .filter(p -> StringUtils.isNotBlank(p.getParent())) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| if (projectsWithParent.isEmpty()) { | ||
| return Collections.emptyList(); | ||
| } | ||
|
|
||
| List<String> missingParentIds = new ArrayList<>(); | ||
|
|
||
| for (Project project : projectsWithParent) { | ||
| String parentId = project.getParent(); | ||
| String redisKey = "project-create-cache-" + parentId; | ||
|
|
||
| String cachedHierarchy = redisTemplate.opsForValue().get(redisKey); | ||
| if (StringUtils.isNotBlank(cachedHierarchy)) { | ||
| log.info("Parent project hierarchy for {} fetched from Redis", parentId); | ||
|
|
||
| Project parent = new Project(); | ||
| parent.setId(parentId); | ||
| parent.setProjectHierarchy(cachedHierarchy); | ||
| parentProjects.add(parent); | ||
| } else { | ||
| missingParentIds.add(parentId); | ||
| } | ||
| } | ||
|
|
||
| if (!missingParentIds.isEmpty()) { | ||
| List<Project> dbQueryProjects = missingParentIds.stream() | ||
| .map(id -> Project.builder().id(id) | ||
| .tenantId(projectRequest.getProjects().get(0).getTenantId()).build()) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| ProjectRequest searchRequest = getSearchProjectRequest(dbQueryProjects, | ||
| projectRequest.getRequestInfo(), false); | ||
|
|
||
| List<Project> dbProjects = searchProject( | ||
| searchRequest, | ||
| projectConfiguration.getMaxLimit(), | ||
| projectConfiguration.getDefaultOffset(), | ||
| searchRequest.getProjects().get(0).getTenantId(), | ||
| null, false, false, false, null, null, false | ||
| ); | ||
|
|
||
| for (Project parent : dbProjects) { | ||
| String redisKey = "project-create-cache-" + parent.getId(); | ||
| redisTemplate.opsForValue().set(redisKey, parent.getProjectHierarchy(), Duration.ofDays(1)); | ||
| parentProjects.add(parent); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| return parentProjects; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider refactoring for better maintainability.
The caching logic is well-designed but needs resilience and could be more maintainable.
Critical Issue: Add error handling for Redis operations
String cachedHierarchy = redisTemplate.opsForValue().get(redisKey);Should be wrapped in try-catch to handle Redis failures gracefully:
+ try {
String cachedHierarchy = redisTemplate.opsForValue().get(redisKey);
if (StringUtils.isNotBlank(cachedHierarchy)) {
log.info("Parent project hierarchy for {} fetched from Redis", parentId);
// ... existing logic
} else {
missingParentIds.add(parentId);
}
+ } catch (Exception e) {
+ log.warn("Failed to fetch from Redis for parent {}: {}", parentId, e.getMessage());
+ missingParentIds.add(parentId);
+ }Refactoring Suggestion: Break down the method
Consider extracting helper methods:
fetchFromCache(List<String> parentIds)fetchFromDatabase(List<String> missingIds, ProjectRequest request)cacheProjects(List<Project> projects)
This would improve readability and testability.
📝 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.
| /* Search for parent projects based on "parent" field and returns parent projects */ | |
| private List<Project> getParentProjects(ProjectRequest projectRequest) { | |
| List<Project> parentProjects = new ArrayList<>(); | |
| List<Project> projectsWithParent = projectRequest.getProjects().stream() | |
| .filter(p -> StringUtils.isNotBlank(p.getParent())) | |
| .collect(Collectors.toList()); | |
| if (projectsWithParent.isEmpty()) { | |
| return Collections.emptyList(); | |
| } | |
| List<String> missingParentIds = new ArrayList<>(); | |
| for (Project project : projectsWithParent) { | |
| String parentId = project.getParent(); | |
| String redisKey = "project-create-cache-" + parentId; | |
| String cachedHierarchy = redisTemplate.opsForValue().get(redisKey); | |
| if (StringUtils.isNotBlank(cachedHierarchy)) { | |
| log.info("Parent project hierarchy for {} fetched from Redis", parentId); | |
| Project parent = new Project(); | |
| parent.setId(parentId); | |
| parent.setProjectHierarchy(cachedHierarchy); | |
| parentProjects.add(parent); | |
| } else { | |
| missingParentIds.add(parentId); | |
| } | |
| } | |
| if (!missingParentIds.isEmpty()) { | |
| List<Project> dbQueryProjects = missingParentIds.stream() | |
| .map(id -> Project.builder().id(id) | |
| .tenantId(projectRequest.getProjects().get(0).getTenantId()).build()) | |
| .collect(Collectors.toList()); | |
| ProjectRequest searchRequest = getSearchProjectRequest(dbQueryProjects, | |
| projectRequest.getRequestInfo(), false); | |
| List<Project> dbProjects = searchProject( | |
| searchRequest, | |
| projectConfiguration.getMaxLimit(), | |
| projectConfiguration.getDefaultOffset(), | |
| searchRequest.getProjects().get(0).getTenantId(), | |
| null, false, false, false, null, null, false | |
| ); | |
| for (Project parent : dbProjects) { | |
| String redisKey = "project-create-cache-" + parent.getId(); | |
| redisTemplate.opsForValue().set(redisKey, parent.getProjectHierarchy(), Duration.ofDays(1)); | |
| parentProjects.add(parent); | |
| } | |
| } | |
| return parentProjects; | |
| } | |
| /* Search for parent projects based on "parent" field and returns parent projects */ | |
| private List<Project> getParentProjects(ProjectRequest projectRequest) { | |
| List<Project> parentProjects = new ArrayList<>(); | |
| List<Project> projectsWithParent = projectRequest.getProjects().stream() | |
| .filter(p -> StringUtils.isNotBlank(p.getParent())) | |
| .collect(Collectors.toList()); | |
| if (projectsWithParent.isEmpty()) { | |
| return Collections.emptyList(); | |
| } | |
| List<String> missingParentIds = new ArrayList<>(); | |
| for (Project project : projectsWithParent) { | |
| String parentId = project.getParent(); | |
| String redisKey = "project-create-cache-" + parentId; | |
| + try { | |
| String cachedHierarchy = redisTemplate.opsForValue().get(redisKey); | |
| if (StringUtils.isNotBlank(cachedHierarchy)) { | |
| log.info("Parent project hierarchy for {} fetched from Redis", parentId); | |
| Project parent = new Project(); | |
| parent.setId(parentId); | |
| parent.setProjectHierarchy(cachedHierarchy); | |
| parentProjects.add(parent); | |
| } else { | |
| missingParentIds.add(parentId); | |
| } | |
| + } catch (Exception e) { | |
| + log.warn("Failed to fetch from Redis for parent {}: {}", parentId, e.getMessage()); | |
| + missingParentIds.add(parentId); | |
| + } | |
| } | |
| if (!missingParentIds.isEmpty()) { | |
| List<Project> dbQueryProjects = missingParentIds.stream() | |
| .map(id -> Project.builder().id(id) | |
| .tenantId(projectRequest.getProjects().get(0).getTenantId()).build()) | |
| .collect(Collectors.toList()); | |
| ProjectRequest searchRequest = getSearchProjectRequest(dbQueryProjects, | |
| projectRequest.getRequestInfo(), false); | |
| List<Project> dbProjects = searchProject( | |
| searchRequest, | |
| projectConfiguration.getMaxLimit(), | |
| projectConfiguration.getDefaultOffset(), | |
| searchRequest.getProjects().get(0).getTenantId(), | |
| null, false, false, false, null, null, false | |
| ); | |
| for (Project parent : dbProjects) { | |
| String redisKey = "project-create-cache-" + parent.getId(); | |
| redisTemplate.opsForValue().set(redisKey, parent.getProjectHierarchy(), Duration.ofDays(1)); | |
| parentProjects.add(parent); | |
| } | |
| } | |
| return parentProjects; | |
| } |
🤖 Prompt for AI Agents
In
health-services/project/src/main/java/org/egov/project/service/ProjectService.java
between lines 329 and 386, add try-catch blocks around all Redis operations to
handle potential Redis failures gracefully without crashing the method. Refactor
the existing getParentProjects method by extracting three helper methods:
fetchFromCache to retrieve parent projects from Redis cache given a list of
parent IDs, fetchFromDatabase to query missing parent projects from the database
using the ProjectRequest, and cacheProjects to store retrieved projects back
into Redis with appropriate expiration. Replace the corresponding code blocks in
getParentProjects with calls to these helper methods to improve readability,
maintainability, and testability.
health-services/project/src/main/java/org/egov/project/service/ProjectService.java
Outdated
Show resolved
Hide resolved
# Conflicts: # health-services/project/src/main/java/org/egov/project/service/ProjectService.java
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
health-services/project/src/main/java/org/egov/project/validator/facility/PfProjectIdValidator.java (1)
63-63: 🧹 Nitpick (assertive)Use a shared constant for Redis key pattern.
Similar to
PsProjectIdValidator, the Redis key pattern should be extracted as a constant. Consider defining it in a shared constants class to ensure consistency.health-services/project/src/main/java/org/egov/project/service/ProjectService.java (3)
50-51: 🛠️ Refactor suggestionUse constructor injection instead of field injection for RedisTemplate.
Field injection makes testing harder and hides dependencies. Constructor injection is the preferred approach.
88-103: 🛠️ Refactor suggestionConsider implementing cache invalidation for data consistency.
The caching implementation is robust with proper error handling. However, cached data may become stale when projects are updated.
333-400: 🧹 Nitpick (assertive)Refactor getParentProjects method for better maintainability.
The method is complex and handles multiple responsibilities. Breaking it down would improve readability and testability.
Consider extracting these helper methods:
private Map<String, String> fetchFromCache(List<String> parentIds) { Map<String, String> cachedHierarchies = new HashMap<>(); List<String> missingIds = new ArrayList<>(); for (String parentId : parentIds) { String redisKey = PROJECT_CACHE_KEY_PREFIX + parentId; try { String hierarchy = redisTemplate.opsForValue().get(redisKey); if (StringUtils.isNotBlank(hierarchy)) { cachedHierarchies.put(parentId, hierarchy); } else { missingIds.add(parentId); } } catch (Exception ex) { log.error("Redis error for parent {}: {}", parentId, ex.getMessage()); missingIds.add(parentId); } } return cachedHierarchies; } private void cacheProjects(List<Project> projects) { for (Project project : projects) { String redisKey = PROJECT_CACHE_KEY_PREFIX + project.getId(); try { redisTemplate.opsForValue().set(redisKey, project.getProjectHierarchy(), Duration.ofDays(1)); } catch (Exception ex) { log.error("Failed to cache project {}: {}", project.getId(), ex.getMessage()); } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
health-services/project/src/main/java/org/egov/project/service/ProjectService.java(5 hunks)health-services/project/src/main/java/org/egov/project/validator/facility/PfProjectIdValidator.java(3 hunks)health-services/project/src/main/java/org/egov/project/validator/staff/PsProjectIdValidator.java(3 hunks)
🔇 Additional comments (1)
health-services/project/src/main/java/org/egov/project/validator/staff/PsProjectIdValidator.java (1)
55-76: Well-implemented Redis caching with proper error handling!The caching logic is properly implemented with:
- Graceful fallback to database on Redis failures
- Clear separation between cache hits and misses
- Appropriate logging for debugging
| List<String> cacheMissIds = new ArrayList<>(); | ||
|
|
||
| for (String id : entityIds) { | ||
| String redisKey = "project-create-cache-" + id; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Extract Redis key pattern as a constant for better maintainability.
The Redis key pattern is hardcoded. Consider extracting it as a constant to maintain consistency across the codebase.
Add a constant at the class level:
public class PsProjectIdValidator implements Validator<ProjectStaffBulkRequest, ProjectStaff> {
+ private static final String PROJECT_CACHE_KEY_PREFIX = "project-create-cache-";
private final ProjectRepository projectRepository;
private final RedisTemplate<String,String> redisTemplate;Then update line 59:
- String redisKey = "project-create-cache-" + id;
+ String redisKey = PROJECT_CACHE_KEY_PREFIX + id;📝 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.
| String redisKey = "project-create-cache-" + id; | |
| public class PsProjectIdValidator implements Validator<ProjectStaffBulkRequest, ProjectStaff> { | |
| private static final String PROJECT_CACHE_KEY_PREFIX = "project-create-cache-"; | |
| private final ProjectRepository projectRepository; | |
| private final RedisTemplate<String, String> redisTemplate; | |
| // existing constructors and methods... | |
| @Override | |
| public void validate(ProjectStaffBulkRequest request, Errors errors) { | |
| String id = request.getStaff().get(0).getId(); | |
| - String redisKey = "project-create-cache-" + id; | |
| + String redisKey = PROJECT_CACHE_KEY_PREFIX + id; | |
| // existing caching logic... | |
| } | |
| } |
🤖 Prompt for AI Agents
In
health-services/project/src/main/java/org/egov/project/validator/staff/PsProjectIdValidator.java
at line 59, the Redis key pattern is hardcoded as a string literal. Extract this
pattern into a private static final constant at the class level to improve
maintainability and consistency. Replace the hardcoded string on line 59 with
this constant concatenated with the variable id.
| List<String> existingProjectIds = new ArrayList<>(); | ||
| List<String> cacheMissIds = new ArrayList<>(); | ||
|
|
||
| for (String id : entityIds) { | ||
| String redisKey = "project-create-cache-" + id; | ||
| try { | ||
| if (Boolean.TRUE.equals(redisTemplate.hasKey(redisKey))) { | ||
| existingProjectIds.add(id); | ||
| } else { | ||
| cacheMissIds.add(id); | ||
| } | ||
| } catch (Exception ex) { | ||
| log.error("Redis error while checking key: {}", redisKey, ex); | ||
| cacheMissIds.add(id); // safely fallback to DB | ||
| } | ||
| } | ||
| if (!cacheMissIds.isEmpty()) { | ||
| List<String> dbValidIds = projectRepository.validateIds(cacheMissIds, | ||
| getIdFieldName(idMethod)); | ||
| List<ProjectFacility> invalidEntities = validEntities.stream().filter(notHavingErrors()).filter(entity -> | ||
| existingProjectIds.addAll(dbValidIds); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider extracting common Redis caching logic.
The Redis caching logic here is nearly identical to PsProjectIdValidator. Consider creating a shared utility class or base validator to reduce code duplication.
Example approach:
- Create a
RedisProjectCacheUtilclass with methods likegetCachedProjectIds()andcacheProjectIds() - Or create an abstract base validator class with the common caching logic
- This would ensure consistent caching behavior and make maintenance easier
🤖 Prompt for AI Agents
In
health-services/project/src/main/java/org/egov/project/validator/facility/PfProjectIdValidator.java
around lines 59 to 79, the Redis caching logic is duplicated and similar to that
in PsProjectIdValidator. To fix this, extract the common Redis caching code into
a shared utility class such as RedisProjectCacheUtil with methods like
getCachedProjectIds() and cacheProjectIds(), or create an abstract base
validator class encapsulating this logic. Then refactor both validators to use
this shared code, reducing duplication and improving maintainability.
health-services/project/src/main/java/org/egov/project/service/ProjectService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
health-services/project/src/main/java/org/egov/project/service/ProjectService.java (2)
88-88: Remove the emoji comment.This comment doesn't add value and should be replaced with a more descriptive comment.
333-400: 🛠️ Refactor suggestionConsider cache invalidation strategy and method refactoring.
The cache-first approach is well-implemented with proper error handling and fallback to database queries. However, there are architectural considerations to address:
Cache invalidation: As noted in previous reviews, this caching strategy lacks invalidation when projects are updated, which could lead to stale data.
Method complexity: The method handles multiple concerns (cache lookup, database queries, cache population) which could benefit from extraction into smaller, focused methods.
The current implementation is functional but could be improved for maintainability and data consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
health-services/project/src/main/java/org/egov/project/service/ProjectService.java(5 hunks)
🔇 Additional comments (6)
health-services/project/src/main/java/org/egov/project/service/ProjectService.java (6)
4-5: LGTM! Clean import additions.The new imports for
DurationandCollectionsare appropriately added and necessary for the Redis caching implementation.
24-24: LGTM! Appropriate RedisTemplate import.The RedisTemplate import is correctly added for the caching functionality.
50-51: LGTM! Proper field declaration with constructor injection.The RedisTemplate field is correctly declared as final and is injected via constructor, following best practices for dependency injection.
53-65: LGTM! Constructor injection properly implemented.The constructor correctly includes the RedisTemplate parameter and assigns it to the field, following dependency injection best practices.
89-103: LGTM! Well-implemented caching with proper error handling.The caching logic is well-structured with:
- Proper validation of projectHierarchy before caching
- Comprehensive error handling with try-catch blocks
- Appropriate logging for both success and failure cases
- Reasonable TTL of 1 day
The implementation handles Redis failures gracefully without disrupting the main project creation flow.
370-396: LGTM! Efficient fallback mechanism with cache population.The fallback logic for missing cache entries is well-implemented:
- Efficiently groups missing parent IDs for batch database query
- Properly constructs search request with required fields
- Includes error handling for cache population
- Maintains consistency between cached and database-fetched projects
This ensures optimal performance while maintaining data integrity.
health-services/project/src/main/java/org/egov/project/service/ProjectService.java
Show resolved
Hide resolved
1 participant
Summary by CodeRabbit