Fetch HH from matview #1025
Fetch HH from matview #1025
Conversation
saiprakash-eGov
requested review from
kavi-egov and
sathishp-eGov
as code owners
WalkthroughThe pull request introduces several changes to the referral management service, including the addition of a new configuration property in Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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
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: 16
🧹 Outside diff range comments (3)
health-services/referralmanagement/src/main/resources/application.properties (1)
Line range hint
156-159: Remove unnecessary blank lines at the end of the file.While the formatting changes around
project.mdms.moduleandegov.location.hierarchy.typeare fine, it's best to remove the extra blank lines at the end of the file to adhere to best practices for property files.Consider removing the extra blank lines at the end of the file.
health-services/referralmanagement/src/main/java/org/egov/referralmanagement/service/DownsyncService.java (2)
Line range hint
289-311: Avoid SQL injection by preventing dynamic table and field names in queries.Using dynamic table names and field names in SQL queries can introduce SQL injection vulnerabilities. The
tableNameandidListFieldNameparameters ingetPrimaryIdsshould not be directly inserted into the query without validation.Consider refactoring to use an enumeration or a mapping of allowed table and field names to ensure only valid, expected values are used. For example:
// Define enums or constants for table and field names enum TableName { HOUSEHOLD_MEMBER("HOUSEHOLD_MEMBER"), PROJECT_BENEFICIARY("PROJECT_BENEFICIARY"), PROJECT_TASK("PROJECT_TASK"), SIDE_EFFECT("SIDE_EFFECT"); private final String name; TableName(String name) { this.name = name; } public String getName() { return name; } } // Use the enum in the method private List<String> getPrimaryIds(List<String> idList, String idListFieldName, TableName table, Long lastChangedSince) { // Validate idListFieldName against allowed field names // Proceed with query construction using validated table.getName() }
Line range hint
492-508: Use URI builder for constructing URLs to ensure proper encoding.In
appendUrlParams, URL parameters are concatenated as strings, which may lead to encoding issues with special characters. Using a URI builder or utilities provided by the HTTP client library can help prevent such issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- health-services/referralmanagement/src/main/java/org/egov/referralmanagement/config/ReferralManagementConfiguration.java (1 hunks)
- health-services/referralmanagement/src/main/java/org/egov/referralmanagement/repository/HouseholdRepository.java (1 hunks)
- health-services/referralmanagement/src/main/java/org/egov/referralmanagement/repository/rowmapper/HouseholdRowMapper.java (1 hunks)
- health-services/referralmanagement/src/main/java/org/egov/referralmanagement/service/DownsyncService.java (10 hunks)
- health-services/referralmanagement/src/main/resources/application.properties (1 hunks)
🧰 Additional context used
📓 Learnings (1)
health-services/referralmanagement/src/main/resources/application.properties (2)
Learnt from: Priyanka-eGov PR: egovernments/health-campaign-services#934 File: health-services/plan-service/src/main/resources/application.properties:54-54 Timestamp: 2024-10-03T10:08:20.045Z Learning: In the 'health-campaign-services' project, it is acceptable to keep the old MDMS endpoint `/egov-mdms-service/v1/_search` in the following files, and updating them is not required: - `health-services/referralmanagement/src/main/resources/application.properties` - `health-services/transformer/src/main/resources/application.properties` - `health-services/stock/src/main/resources/application.properties` - `health-services/resource-estimation-service/src/main/resources/application.properties` - `health-services/individual/src/main/resources/application.properties` - `health-services/product/src/main/resources/application.properties` - `health-services/project/src/main/resources/application.properties` - `health-services/household/src/main/resources/application.properties` - `health-services/facility/src/main/resources/application.properties` - `core-services/pgr-services/src/main/resources/application.properties` - `core-services/egov-hrms/src/main/resources/application.properties` - `core-services/dashboard-analytics/src/main/resources/application.properties`Learnt from: Priyanka-eGov PR: egovernments/health-campaign-services#934 File: health-services/plan-service/src/main/resources/application.properties:54-54 Timestamp: 2024-10-08T20:11:12.539Z Learning: In the 'health-campaign-services' project, it is acceptable to keep the old MDMS endpoint `/egov-mdms-service/v1/_search` in the following files, and updating them is not required: - `health-services/referralmanagement/src/main/resources/application.properties` - `health-services/transformer/src/main/resources/application.properties` - `health-services/stock/src/main/resources/application.properties` - `health-services/resource-estimation-service/src/main/resources/application.properties` - `health-services/individual/src/main/resources/application.properties` - `health-services/product/src/main/resources/application.properties` - `health-services/project/src/main/resources/application.properties` - `health-services/household/src/main/resources/application.properties` - `health-services/facility/src/main/resources/application.properties` - `core-services/pgr-services/src/main/resources/application.properties` - `core-services/egov-hrms/src/main/resources/application.properties` - `core-services/dashboard-analytics/src/main/resources/application.properties`
🔇 Additional comments (7)
health-services/referralmanagement/src/main/java/org/egov/referralmanagement/config/ReferralManagementConfiguration.java (1)
118-119: 🧹 Nitpick (assertive)Consider using a boolean type and add documentation for the new property.
The new
enableMatviewSearchproperty seems to be a feature flag. However, there are a few points to consider:
- Using a
Stringtype for what appears to be a boolean flag might not be the most appropriate choice. Consider using abooleantype instead.- There's no documentation explaining the purpose or possible values of this property. Adding a comment would improve clarity.
Consider applying the following changes:
- @Value("${egov.enable.matview.search}") - private String enableMatviewSearch; + /** + * Flag to enable or disable materialized view search. + * Set to true to use materialized view, false to use regular search. + */ + @Value("${egov.enable.matview.search}") + private boolean enableMatviewSearch;To ensure this property is properly set in the application properties, run the following command:
Could you please clarify the intended use of this property and confirm if a boolean type would be more appropriate?
✅ Verification successful
Verified:
egov.enable.matview.searchUsage
- The
enableMatviewSearchproperty is defined inReferralManagementConfiguration.javaand is not used elsewhere in the codebase.- Changing its type from
Stringtobooleanis appropriate and safe.- Adding documentation for this property will enhance clarity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the new property is defined in application.properties grep -n "egov.enable.matview.search" $(find . -name "application.properties" -type f)Length of output: 1361
Script:
#!/bin/bash # Search for all usages of `enableMatviewSearch` in the codebase rg "enableMatviewSearch" --context 2Length of output: 729
health-services/referralmanagement/src/main/java/org/egov/referralmanagement/service/DownsyncService.java (6)
14-14: Import ofTupleclass is appropriate.The addition of
import org.egov.common.ds.Tuple;is necessary for handling tuples returned from the repository.
46-46: Import ofHouseholdRepositoryis appropriate.The
HouseholdRepositoryimport is required for accessing household data from the materialized view.
68-68: Declaration ofhouseholdRepositoryis correct.The private member
householdRepositoryis properly declared for use within the service.
78-87: Constructor updated to includehouseholdRepository.The
HouseholdRepositoryis correctly injected via the constructor and assigned to the class member.
532-538: Utility methodsplitListis well implemented.The
splitListmethod efficiently splits a list into sublists of the specified size, which helps in handling large datasets without overwhelming the system.
246-275: Potential mismatch in field names affecting query results.In
searchMembers, the field name"householdClientReferenceId"is used in thegetPrimaryIdsmethod. Ensure that this field name matches the actual column name in theHOUSEHOLD_MEMBERtable to prevent incorrect query results.Run the following script to verify the column name in the
HOUSEHOLD_MEMBERtable:
| @@ -152,6 +152,7 @@ project.document.id.verification.required=false | |||
|
|
|||
| project.mdms.module=HCM-PROJECT-TYPES | |||
| egov.location.hierarchy.type=ADMIN | |||
| egov.enable.matview.search=false | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM. Consider adding documentation for the new property.
The addition of egov.enable.matview.search=false is a good practice for introducing a new feature. Setting it to false by default ensures backward compatibility.
Consider adding a comment above this line to explain the purpose and impact of this property, e.g.:
# Controls whether to use materialized view for search operations. Set to true to enable.
egov.enable.matview.search=false| @Autowired | ||
| private HouseholdRowMapper householdRowMapper; | ||
|
|
||
| public Tuple<Long, List<Household>> findByView (String localityCode, Integer limit, Integer offset, String tenantId) { |
There was a problem hiding this comment.
Unused parameter tenantId in method findByView
The parameter tenantId is declared but not used within the method. In a multitenant system, it's important to include tenantId in your query to ensure data isolation. Consider removing it if unnecessary or incorporating it into the SQL queries.
| public Tuple<Long, List<Household>> findByView (String localityCode, Integer limit, Integer offset, String tenantId) { | ||
|
|
||
|
|
||
| String query = "select * from household_address_mv where localitycode=:localitycode and rank between :start and :end "; |
There was a problem hiding this comment.
tenantId is not included in SQL queries
The SQL queries do not filter by tenantId, which could lead to data from other tenants being accessed. Ensure that tenantId is included in the WHERE clause of your queries to maintain proper data segregation.
Also applies to: 35-35
🛠️ Refactor suggestion
Avoid using SELECT * in SQL queries
Using SELECT * can lead to fetching unnecessary data, impacting performance. Specify only the columns that are required for the operation.
Modify the query to include specific columns:
- String query = "select * from household_address_mv where localitycode=:localitycode ORDER BY rank LIMIT :limit OFFSET :offset";
+ String query = "select column1, column2, column3 from household_address_mv where localitycode=:localitycode ORDER BY rank LIMIT :limit OFFSET :offset";Replace column1, column2, column3 with the actual column names you need.
Committable suggestion was skipped due to low confidence.
| Map<String, Object> paramsMap = new HashMap<>(); | ||
| paramsMap.put("start", offset); | ||
| paramsMap.put("end", offset+limit); | ||
| paramsMap.put("localitycode", localityCode); | ||
|
|
||
| Map<String, Object> paramsMapCount = new HashMap<>(); | ||
| paramsMapCount.put("localitycode", localityCode); | ||
| Integer maxRank = namedParameterJdbcTemplate.queryForObject("select max(rank) from household_address_mv where localitycode=:localitycode", paramsMapCount, Integer.class); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Combine parameter maps to reduce redundancy
Currently, two parameter maps are used (paramsMap and paramsMapCount), both containing localitycode. Combining them simplifies the code and reduces redundancy.
Here's how you can adjust the code:
Map<String, Object> paramsMap = new HashMap<>();
paramsMap.put("localitycode", localityCode);
- paramsMap.put("start", offset);
- paramsMap.put("end", offset+limit);
+ paramsMap.put("limit", limit);
+ paramsMap.put("offset", offset);
- Map<String, Object> paramsMapCount = new HashMap<>();
- paramsMapCount.put("localitycode", localityCode);
- Long totalCount = namedParameterJdbcTemplate.queryForObject("select count(*) from household_address_mv where localitycode=:localitycode", paramsMapCount, Long.class);
+ Long totalCount = namedParameterJdbcTemplate.queryForObject("select count(*) from household_address_mv where localitycode=:localitycode", paramsMap, Long.class);Now, paramsMap is used for both queries.
Committable suggestion was skipped due to low confidence.
| Integer maxRank = namedParameterJdbcTemplate.queryForObject("select max(rank) from household_address_mv where localitycode=:localitycode", paramsMapCount, Integer.class); | ||
| Long totalCount = maxRank == null ? 0L : Long.valueOf(maxRank); |
There was a problem hiding this comment.
Using MAX(rank) may not accurately represent total count
Relying on MAX(rank) assumes that ranks are sequential and without gaps. If there are missing ranks, MAX(rank) will not reflect the actual total number of records. Consider using COUNT(*) to get an accurate total count of records matching the criteria.
Apply this diff to correct the total count calculation:
- Integer maxRank = namedParameterJdbcTemplate.queryForObject("select max(rank) from household_address_mv where localitycode=:localitycode", paramsMapCount, Integer.class);
- Long totalCount = maxRank == null ? 0L : Long.valueOf(maxRank);
+ Long totalCount = namedParameterJdbcTemplate.queryForObject("select count(*) from household_address_mv where localitycode=:localitycode", paramsMapCount, Long.class);📝 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.
| Integer maxRank = namedParameterJdbcTemplate.queryForObject("select max(rank) from household_address_mv where localitycode=:localitycode", paramsMapCount, Integer.class); | |
| Long totalCount = maxRank == null ? 0L : Long.valueOf(maxRank); | |
| Long totalCount = namedParameterJdbcTemplate.queryForObject("select count(*) from household_address_mv where localitycode=:localitycode", paramsMapCount, Long.class); |
| .additionalFields(resultSet.getString("additionalDetails") == null ? null : objectMapper.readValue(resultSet | ||
| .getString("additionalDetails"), AdditionalFields.class)) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Handle Null Values for AdditionalFields
If additionalDetails in the result set is NULL, objectMapper.readValue() might not be necessary. The current null check could be simplified for clarity.
Consider simplifying the code:
- .additionalFields(resultSet.getString("additionalDetails") == null ? null : objectMapper.readValue(resultSet
- .getString("additionalDetails"), AdditionalFields.class))
+ .additionalFields(resultSet.getString("additionalDetails") != null ?
+ objectMapper.readValue(resultSet.getString("additionalDetails"), AdditionalFields.class) : null)📝 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.
| .additionalFields(resultSet.getString("additionalDetails") == null ? null : objectMapper.readValue(resultSet | |
| .getString("additionalDetails"), AdditionalFields.class)) | |
| .additionalFields(resultSet.getString("additionalDetails") != null ? | |
| objectMapper.readValue(resultSet.getString("additionalDetails"), AdditionalFields.class) : null) |
| .latitude(resultSet.getDouble("latitude")) | ||
| .longitude(resultSet.getDouble("longitude")) | ||
| .locationAccuracy(resultSet.getDouble("locationAccuracy")) | ||
| .type(AddressType.fromValue(resultSet.getString("type"))) |
There was a problem hiding this comment.
Check for Null Before Using AddressType.fromValue
If resultSet.getString("type") returns null, calling AddressType.fromValue(null) could throw an exception. Add a null check to prevent this.
Apply this diff to handle potential null values:
- .type(AddressType.fromValue(resultSet.getString("type")))
+ .type(resultSet.getString("type") != null ? AddressType.fromValue(resultSet.getString("type")) : null)📝 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.
| .type(AddressType.fromValue(resultSet.getString("type"))) | |
| .type(resultSet.getString("type") != null ? AddressType.fromValue(resultSet.getString("type")) : null) |
| if (household.getAddress().getId() == null) { | ||
| household.setAddress(null); | ||
| } |
There was a problem hiding this comment.
Prevent Potential NullPointerException When Checking Address ID
If household.getAddress() returns null, calling getId() on it will cause a NullPointerException. Given that the address is always set during the build, verify if the address can be null and adjust the check accordingly.
Apply this diff to ensure safety:
- if (household.getAddress().getId() == null) {
+ if (household.getAddress() == null || household.getAddress().getId() == null) {Alternatively, if household.getAddress() cannot be null, confirm this assumption and document it to clarify the behavior.
📝 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 (household.getAddress().getId() == null) { | |
| household.setAddress(null); | |
| } | |
| if (household.getAddress() == null || household.getAddress().getId() == null) { | |
| household.setAddress(null); | |
| } |
| .latitude(resultSet.getDouble("latitude")) | ||
| .longitude(resultSet.getDouble("longitude")) | ||
| .locationAccuracy(resultSet.getDouble("locationAccuracy")) |
There was a problem hiding this comment.
Properly Handle Null Values for Double Fields
Using ResultSet.getDouble() returns 0.0 when the column value is NULL. This might lead to confusion between a genuine 0.0 value and a NULL. Use ResultSet.getObject() to differentiate between them.
Apply this diff to handle null values correctly:
- .latitude(resultSet.getDouble("latitude"))
- .longitude(resultSet.getDouble("longitude"))
- .locationAccuracy(resultSet.getDouble("locationAccuracy"))
+ .latitude(resultSet.getObject("latitude") != null ? resultSet.getDouble("latitude") : null)
+ .longitude(resultSet.getObject("longitude") != null ? resultSet.getDouble("longitude") : null)
+ .locationAccuracy(resultSet.getObject("locationAccuracy") != null ? resultSet.getDouble("locationAccuracy") : null)Also, update the corresponding fields in the Address class to use Double instead of double:
- private double latitude;
- private double longitude;
- private double locationAccuracy;
+ private Double latitude;
+ private Double longitude;
+ private Double locationAccuracy;📝 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.
| .latitude(resultSet.getDouble("latitude")) | |
| .longitude(resultSet.getDouble("longitude")) | |
| .locationAccuracy(resultSet.getDouble("locationAccuracy")) | |
| .latitude(resultSet.getObject("latitude") != null ? resultSet.getDouble("latitude") : null) | |
| .longitude(resultSet.getObject("longitude") != null ? resultSet.getDouble("longitude") : null) | |
| .locationAccuracy(resultSet.getObject("locationAccuracy") != null ? resultSet.getDouble("locationAccuracy") : null) |
| @@ -63,21 +65,26 @@ public class DownsyncService { | |||
|
|
|||
| private MasterDataService masterDataService; | |||
|
|
|||
| private HouseholdRepository householdRepository; | |||
|
|
|||
| private static final Integer SEARCH_MAX_COUNT = 1000; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using int instead of Integer for SEARCH_MAX_COUNT.
Since SEARCH_MAX_COUNT is a constant and will not be null, using the primitive type int is more efficient.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range comments (1)
health-services/referralmanagement/src/main/java/org/egov/referralmanagement/service/DownsyncService.java (1)
Line range hint
466-489: Avoid injecting parameters directly into SQL queries to prevent SQL injection risks.In the
getPrimaryIdsmethod,tableNameandidListFieldNameare directly injected into the SQL query. This practice can lead to SQL injection vulnerabilities if these variables are not strictly controlled.Consider refactoring the code to use predefined constants for table and field names or to validate the inputs thoroughly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- health-services/referralmanagement/src/main/java/org/egov/referralmanagement/service/DownsyncService.java (10 hunks)
🧰 Additional context used
| @@ -104,16 +111,16 @@ public Downsync prepareDownsyncData(DownsyncRequest downsyncRequest) { | |||
| LinkedHashMap<String, Object> projectType = masterDataService.getProjectType(downsyncRequest); | |||
|
|
|||
| /* search household */ | |||
| householdIds = searchHouseholds(downsyncRequest, downsync); | |||
| households = searchHouseholds(downsyncRequest, downsync); | |||
| householdClientRefIds = households.stream().map(Household::getClientReferenceId).collect(Collectors.toList()); | |||
There was a problem hiding this comment.
Possible NullPointerException when households is null.
At line 115, householdClientRefIds is assigned by calling households.stream(). If households is null, this will result in a NullPointerException.
Please add a null check for households before calling stream().
Apply this diff to fix the potential issue:
+ if (households != null) {
householdClientRefIds = households.stream().map(Household::getClientReferenceId).collect(Collectors.toList());
+ } else {
+ householdClientRefIds = Collections.emptyList();
+ }📝 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.
| householdClientRefIds = households.stream().map(Household::getClientReferenceId).collect(Collectors.toList()); | |
| if (households != null) { | |
| householdClientRefIds = households.stream().map(Household::getClientReferenceId).collect(Collectors.toList()); | |
| } else { | |
| householdClientRefIds = Collections.emptyList(); | |
| } |
| List<Individual> individuals = new ArrayList<>(); | ||
| List<List<String>> subLists = splitList(individualClientRefIds, SEARCH_MAX_COUNT); |
There was a problem hiding this comment.
Add null check for individualClientRefIds before splitting the list.
In the searchIndividuals method, individualClientRefIds might be null or empty, which could cause a NullPointerException when passing it to splitList.
Please add a null or empty check before calling splitList.
Apply this diff to fix the issue:
+ if (CollectionUtils.isEmpty(individualClientRefIds)) {
+ return Collections.emptyList();
+ }
List<Individual> individuals = new ArrayList<>();
List<List<String>> subLists = splitList(individualClientRefIds, SEARCH_MAX_COUNT);📝 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.
| List<Individual> individuals = new ArrayList<>(); | |
| List<List<String>> subLists = splitList(individualClientRefIds, SEARCH_MAX_COUNT); | |
| if (CollectionUtils.isEmpty(individualClientRefIds)) { | |
| return Collections.emptyList(); | |
| } | |
| List<Individual> individuals = new ArrayList<>(); | |
| List<List<String>> subLists = splitList(individualClientRefIds, SEARCH_MAX_COUNT); |
| for (int i = 0; i < list.size(); i += size) { | ||
| subLists.add(list.subList(i, Math.min(i + size, list.size()))); |
There was a problem hiding this comment.
Handle possible zero or negative size in splitList method.
The splitList method does not handle cases where size is zero or negative, which could lead to an infinite loop or an exception.
Consider adding validation to ensure that size is greater than zero.
Apply this diff to add validation:
private <T> List<List<T>> splitList(List<T> list, int size) {
+ if (size <= 0) {
+ throw new IllegalArgumentException("Size must be greater than zero");
+ }
List<List<T>> subLists = new ArrayList<>();
for (int i = 0; i < list.size(); i += size) {
subLists.add(list.subList(i, Math.min(i + size, list.size())));
}
return subLists;
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
health-services/referralmanagement/src/main/java/org/egov/referralmanagement/service/DownsyncService.java(10 hunks)
🔇 Additional comments (4)
health-services/referralmanagement/src/main/java/org/egov/referralmanagement/service/DownsyncService.java (4)
53-53: Consider usingintinstead ofIntegerforSEARCH_MAX_COUNT.
Using a primitive type is more efficient for constants that will never benull.
112-112: Possible NullPointerException whenhouseholdsisnull.
IfsearchHouseholdsunexpectedly returnsnullinstead of an empty list, callingstream()will throw an NPE. Please add a null check or ensuresearchHouseholdsnever returnsnull.
213-214: Add a null check forindividualClientRefIdsbefore splitting the list.
WhenindividualClientRefIdsisnull, passing it tosplitListwill cause a NullPointerException.
534-535: ValidatesizeinsplitListto prevent invalid ranges.
Ifsizeis zero or negative, the loop can lead to an infinite iteration or an out-of-bounds error. Ensuresize > 0before proceeding.
| String key = downsyncCriteria.getLocality() + downsyncCriteria.getOffset() + downsyncCriteria.getLimit(); | ||
|
|
||
| Object obj = getFromCache(key); | ||
| if (null != obj) { | ||
| return (Downsync) obj; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Incorporate additional parameters into the cache key to avoid collisions.
Right now, the cache key is derived only from locality, offset, and limit. If multiple queries share these same three parameters but differ in others (e.g., tenantId, lastSyncedTime, or matview toggles), they may overwrite each other in the cache. Consider including more fields from downsyncCriteria in the key for uniqueness.
| // removing incremental downsync for matview flow | ||
| if (configs.isEnableMatviewSearch()) { | ||
| downsyncCriteria.setLastSyncedTime(null); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Review the decision to reset lastSyncedTime when isEnableMatviewSearch is true.
Setting lastSyncedTime to null might skip incremental downsync for matview-based searches, which may be unexpected if partial sync is needed. Consider clarifying or adjusting this logic if incremental sync from matview is a requirement.
Fetch HH from matview and sea
Summary by CodeRabbit
New Features
Improvements
Documentation