Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch HH from matview #1025

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fetch HH from matview #1025

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
19482c0
Fetch households from matview and other minor changes
saiprakash-eGov Oct 21, 2024
29e951b
Merge branch 'master' into hh-matview-fetch
saiprakash-eGov Oct 21, 2024
0ad9a94
change to boolean from string
saiprakash-eGov Oct 21, 2024
94fcd8c
change to the boolean variable and count issue fixed
saiprakash-eGov Oct 21, 2024
d348299
redis caching enabled for downsync
saiprakash-eGov Nov 20, 2024
18fecab
matview downsync changes
vishal-egov Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ...ent/src/main/java/org/egov/referralmanagement/config/ReferralManagementConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,7 @@ public class ReferralManagementConfiguration {
@Value("${egov.mdms.search.endpoint}")
private String mdmsSearchUrl;

@Value("${egov.enable.matview.search}")
private boolean enableMatviewSearch;

}
39 changes: 39 additions & 0 deletions ...lmanagement/src/main/java/org/egov/referralmanagement/repository/HouseholdRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package org.egov.referralmanagement.repository;

import org.egov.common.ds.Tuple;
import org.egov.common.models.household.Household;
import org.egov.referralmanagement.repository.rowmapper.HouseholdRowMapper;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
import org.springframework.stereotype.Repository;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

@Repository
public class HouseholdRepository {

@Autowired
private NamedParameterJdbcTemplate namedParameterJdbcTemplate;

@Autowired
private HouseholdRowMapper householdRowMapper;

public Tuple<Long, List<Household>> findByView (String localityCode, Integer limit, Integer offset, String tenantId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.



String query = "select * from household_address_mv where localitycode=:localitycode and rank between :start and :end ";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


Comment on lines +26 to +27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Potential issues with pagination using rank column

Using rank BETWEEN :start AND :end for pagination may not be reliable if the rank values are not continuous or have gaps. This could result in missing or duplicated records in different pages. Consider using LIMIT and OFFSET for pagination.

Apply this diff to modify the pagination approach:

- String query = "select * from household_address_mv where localitycode=:localitycode and rank between :start and :end ";
+ String query = "select * from household_address_mv where localitycode=:localitycode ORDER BY rank LIMIT :limit OFFSET :offset";

And adjust the parameters accordingly:

- paramsMap.put("start", offset);
- paramsMap.put("end", offset+limit);
+ paramsMap.put("limit", limit);
+ paramsMap.put("offset", offset);

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);
Comment on lines +28 to +35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Long totalCount = maxRank == null ? 0L : Long.valueOf(maxRank);
Comment on lines +35 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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);

return new Tuple<>(totalCount, this.namedParameterJdbcTemplate.query(query, paramsMap, householdRowMapper));
}
}
76 changes: 76 additions & 0 deletions ...nt/src/main/java/org/egov/referralmanagement/repository/rowmapper/HouseholdRowMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package org.egov.referralmanagement.repository.rowmapper;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import digit.models.coremodels.AuditDetails;
import org.egov.common.models.core.AdditionalFields;
import org.egov.common.models.household.Address;
import org.egov.common.models.household.AddressType;
import org.egov.common.models.core.Boundary;
import org.egov.common.models.household.Household;
import org.springframework.jdbc.core.RowMapper;
import org.springframework.stereotype.Component;

import java.sql.ResultSet;
import java.sql.SQLException;

@Component
public class HouseholdRowMapper implements RowMapper<Household> {
private final ObjectMapper objectMapper = new ObjectMapper();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Prefer Dependency Injection for ObjectMapper

It is recommended to inject the ObjectMapper provided by Spring to leverage shared configurations and better manage resources, instead of creating a new instance with new ObjectMapper().

Apply this change to use dependency injection:

- private final ObjectMapper objectMapper = new ObjectMapper();
+ @Autowired
+ private ObjectMapper objectMapper;

Ensure that the class is configured for component scanning and that an ObjectMapper bean is available in the Spring context.

📝 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.

Suggested change
private final ObjectMapper objectMapper = new ObjectMapper();
@Autowired
private ObjectMapper objectMapper;


@Override
public Household mapRow(ResultSet resultSet, int i) throws SQLException {
try {
AuditDetails auditDetails = AuditDetails.builder()
.createdBy(resultSet.getString("createdBy"))
.createdTime(resultSet.getLong("createdTime"))
.lastModifiedBy(resultSet.getString("lastModifiedBy"))
.lastModifiedTime(resultSet.getLong("lastModifiedTime"))
.build();
AuditDetails clientAuditDetails = AuditDetails.builder()
.createdTime(resultSet.getLong("clientCreatedTime"))
.createdBy(resultSet.getString("clientCreatedBy"))
.lastModifiedTime(resultSet.getLong("clientLastModifiedTime"))
.lastModifiedBy(resultSet.getString("clientLastModifiedBy"))
.build();
Household household = Household.builder()
.id(resultSet.getString("id"))
.rowVersion(resultSet.getInt("rowVersion"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle Potential Null Values for Integer Fields

Using ResultSet.getInt() returns 0 when the column value is NULL, which might not be the intended behavior if 0 is not a valid default. Consider using ResultSet.getObject() to correctly handle NULL values.

Apply this diff to handle null values appropriately:

- .rowVersion(resultSet.getInt("rowVersion"))
- .memberCount(resultSet.getInt("numberOfMembers"))
+ .rowVersion(resultSet.getObject("rowVersion") != null ? resultSet.getInt("rowVersion") : null)
+ .memberCount(resultSet.getObject("numberOfMembers") != null ? resultSet.getInt("numberOfMembers") : null)

Also, update the data types in the Household class:

- private int rowVersion;
- private int memberCount;
+ private Integer rowVersion;
+ private Integer memberCount;

Also applies to: 41-41

.isDeleted(resultSet.getBoolean("isDeleted"))
.tenantId(resultSet.getString("tenantId"))
.memberCount(resultSet.getInt("numberOfMembers"))
.clientReferenceId(resultSet.getString("clientReferenceId"))
.auditDetails(auditDetails)
.clientAuditDetails(clientAuditDetails)
.additionalFields(resultSet.getString("additionalDetails") == null ? null : objectMapper.readValue(resultSet
.getString("additionalDetails"), AdditionalFields.class))
Comment on lines +45 to +46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
.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)

.address(Address.builder()
.id(resultSet.getString("aid"))
.clientReferenceId(resultSet.getString("aclientreferenceid"))
.tenantId(resultSet.getString("atenantid"))
.doorNo(resultSet.getString("doorNo"))
.latitude(resultSet.getDouble("latitude"))
.longitude(resultSet.getDouble("longitude"))
.locationAccuracy(resultSet.getDouble("locationAccuracy"))
Comment on lines +52 to +54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
.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)

.type(AddressType.fromValue(resultSet.getString("type")))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
.type(AddressType.fromValue(resultSet.getString("type")))
.type(resultSet.getString("type") != null ? AddressType.fromValue(resultSet.getString("type")) : null)

.addressLine1(resultSet.getString("addressLine1"))
.addressLine2(resultSet.getString("addressLine2"))
.landmark(resultSet.getString("landmark"))
.city(resultSet.getString("city"))
.pincode(resultSet.getString("pinCode"))
.buildingName(resultSet.getString("buildingName"))
.street(resultSet.getString("street"))
.locality(resultSet.getString("localityCode") != null ?
Boundary.builder().code(resultSet.getString("localityCode")).build() : null)
.build())
.build();
if (household.getAddress().getId() == null) {
household.setAddress(null);
}
Comment on lines +67 to +69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
if (household.getAddress().getId() == null) {
household.setAddress(null);
}
if (household.getAddress() == null || household.getAddress().getId() == null) {
household.setAddress(null);
}

return household;
} catch (JsonProcessingException e) {
throw new SQLException(e);
}
Comment on lines +71 to +73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Log Exceptions Before Rethrowing

Currently, JsonProcessingException is caught and wrapped as an SQLException without logging. Logging the exception can aid in debugging.

Consider logging the exception:

} catch (JsonProcessingException e) {
+     // Log the exception
+     logger.error("Error processing JSON for additional fields", e);
      throw new SQLException(e);
}

Ensure that a logger is initialized in the class:

private static final Logger logger = LoggerFactory.getLogger(HouseholdRowMapper.class);


}
}
Loading
Loading