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.

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

Updated the code for immediate children search #1054

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
2 changes: 1 addition & 1 deletion health-services/libraries/health-services-models/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<modelVersion>4.0.0</modelVersion>
<groupId>org.egov.common</groupId>
<artifactId>health-services-models</artifactId>
<version>1.0.21-SNAPSHOT</version>
<version>1.0.21-impel-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Version change requires coordination across multiple services

The version change to 1.0.21-impel-SNAPSHOT requires attention as:

  • Most health services are currently using version 1.0.20-SNAPSHOT
  • Only the project service is using the new 1.0.21-impel-SNAPSHOT version
  • Some services are using even older versions (product: 1.0.7-SNAPSHOT, egov-hrms: 1.0.12-SNAPSHOT)

This inconsistency in versions across services needs to be addressed:

  • Either roll back to 1.0.21-SNAPSHOT to maintain consistency
  • Or coordinate version updates across all dependent services if the impel qualifier is necessary
🔗 Analysis chain

Verify the rationale for version qualifier change

The version change from 1.0.21-SNAPSHOT to 1.0.21-impel-SNAPSHOT seems unrelated to the PR's objective of updating code for immediate children search. Please clarify:

  1. Why is the 'impel' qualifier being added?
  2. Are there any breaking changes that require this version distinction?
  3. Have all dependent services been tested with this new version?
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any other services that might be affected by this version change
rg -l "health-services-models.*1\.0\.21-SNAPSHOT" 

Length of output: 51


Script:

#!/bin/bash
# Let's try a broader search for dependencies on health-services-models
rg -l "health-services-models"

# Also search for any pom.xml files that might reference this dependency
fd "pom.xml" --exec grep -l "health-services-models" {}

Length of output: 1094


Script:

#!/bin/bash
# Let's check the version references in the dependent projects' pom files
for pom in $(fd "pom.xml" --exclude "health-services-models"); do
    echo "=== $pom ==="
    grep -A 2 -B 2 "health-services-models" "$pom" || true
done

Length of output: 6007

<properties>
<java.version>17</java.version>
<maven.compiler.source>${java.version}</maven.compiler.source>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public class Field {

@JsonProperty("value")
@NotNull
@Size(min = 1, max = 10000)
@Size(max = 10000)
private String value = null;


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,21 @@ public enum TaskStatus {
* This status signifies that the task has not been processed or
* handled yet.
*/
NOT_ADMINISTERED("NOT_ADMINISTERED");
NOT_ADMINISTERED("NOT_ADMINISTERED"),

/**
* Indicates that the beneficiary is ineligible.
* This status means that the individual or entity for whom the task
* was intended is ineligible
*/
BENEFICIARY_INELIGIBLE("BENEFICIARY_INELIGIBLE"),

/**
* Indicates that the beneficiary is ineligible.
* This status means that the individual or entity for whom the task
* was intended was referred to some institution
*/
Comment on lines +65 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

Fix copy-paste error in JavaDoc comment

The JavaDoc comment for BENEFICIARY_REFERRED incorrectly states "Indicates that the beneficiary is ineligible" which appears to be copied from the previous enum value.

Apply this diff to fix the documentation:

    /**
-     * Indicates that the beneficiary is ineligible.
+     * Indicates that the beneficiary has been referred.
      * This status means that the individual or entity for whom the task
      * was intended was referred to some institution
      */
📝 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
/**
* Indicates that the beneficiary is ineligible.
* This status means that the individual or entity for whom the task
* was intended was referred to some institution
*/
/**
* Indicates that the beneficiary has been referred.
* This status means that the individual or entity for whom the task
* was intended was referred to some institution
*/

BENEFICIARY_REFERRED("BENEFICIARY_REFERRED");

// The string value associated with the task status.
private String value;
Expand Down
2 changes: 1 addition & 1 deletion health-services/project/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
<dependency>
<groupId>org.egov.common</groupId>
<artifactId>health-services-models</artifactId>
<version>1.0.21-SNAPSHOT</version>
<version>1.0.21-impel-SNAPSHOT</version>
<scope>compile</scope>
</dependency>
<dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public ProjectRepository(Producer producer, NamedParameterJdbcTemplate namedPara
}


public List<Project> getProjects(ProjectRequest project, Integer limit, Integer offset, String tenantId, Long lastChangedSince, Boolean includeDeleted, Boolean includeAncestors, Boolean includeDescendants, Long createdFrom, Long createdTo) {
public List<Project> getProjects(ProjectRequest project, Integer limit, Integer offset, String tenantId, Long lastChangedSince, Boolean includeDeleted, Boolean includeAncestors, Boolean includeDescendants, Boolean includeImmediateChildren ,Long createdFrom, Long createdTo) {

//Fetch Projects based on search criteria
List<Project> projects = getProjectsBasedOnSearchCriteria(project.getProjects(), limit, offset, tenantId, lastChangedSince, includeDeleted, createdFrom, createdTo);
Expand All @@ -86,14 +86,13 @@ public List<Project> getProjects(ProjectRequest project, Integer limit, Integer
projectIds.addAll(ancestorProjectIds);
}
}
//Get Project descendants if includeDescendants flag is true
if (includeDescendants) {
if (includeImmediateChildren) {
descendants = getProjectImmediateDescendants(projects);
} else if (includeDescendants) {
descendants = getProjectDescendants(projects);
if (descendants != null && !descendants.isEmpty()) {
List<String> descendantsProjectIds = descendants.stream().map(Project :: getId).collect(Collectors.toList());
projectIds.addAll(descendantsProjectIds);
}
}
List<String> descendantsProjectIds = descendants == null || descendants.isEmpty() ? new ArrayList<>() : descendants.stream().map(Project::getId).collect(Collectors.toList());
projectIds.addAll(descendantsProjectIds);

//Fetch targets based on Project Ids
List<Target> targets = getTargetsBasedOnProjectIds(projectIds);
Expand Down Expand Up @@ -176,6 +175,14 @@ private List<Project> getProjectsDescendantsBasedOnProjectIds(List<String> proje
return projects;
}

/* Fetch Project descendants based on Project ids */
private List<Project> getProjectsImmediateDescendantsBasedOnProjectIds(List<String> projectIds, List<Object> preparedStmtListDescendants) {
String query = queryBuilder.getProjectImmediateDescendantsSearchQueryBasedOnIds(projectIds, preparedStmtListDescendants);
List<Project> projects = jdbcTemplate.query(query, addressRowMapper, preparedStmtListDescendants.toArray());
log.info("Fetched project immediate descendants list based on given Project Ids");
return projects;
}

Comment on lines +178 to +185
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)

Add input validation for projectIds.

The method should validate the input parameters to prevent potential issues with null or empty lists.

Consider adding validation:

     private List<Project> getProjectsImmediateDescendantsBasedOnProjectIds(List<String> projectIds, List<Object> preparedStmtListDescendants) {
+        if (projectIds == null || projectIds.isEmpty()) {
+            log.debug("No project IDs provided for fetching immediate descendants");
+            return new ArrayList<>();
+        }
         String query = queryBuilder.getProjectImmediateDescendantsSearchQueryBasedOnIds(projectIds, preparedStmtListDescendants);
         List<Project> projects = jdbcTemplate.query(query, addressRowMapper, preparedStmtListDescendants.toArray());
         log.info("Fetched project immediate descendants list based on given Project Ids");
         return projects;
     }
📝 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
/* Fetch Project descendants based on Project ids */
private List<Project> getProjectsImmediateDescendantsBasedOnProjectIds(List<String> projectIds, List<Object> preparedStmtListDescendants) {
String query = queryBuilder.getProjectImmediateDescendantsSearchQueryBasedOnIds(projectIds, preparedStmtListDescendants);
List<Project> projects = jdbcTemplate.query(query, addressRowMapper, preparedStmtListDescendants.toArray());
log.info("Fetched project immediate descendants list based on given Project Ids");
return projects;
}
/* Fetch Project descendants based on Project ids */
private List<Project> getProjectsImmediateDescendantsBasedOnProjectIds(List<String> projectIds, List<Object> preparedStmtListDescendants) {
if (projectIds == null || projectIds.isEmpty()) {
log.debug("No project IDs provided for fetching immediate descendants");
return new ArrayList<>();
}
String query = queryBuilder.getProjectImmediateDescendantsSearchQueryBasedOnIds(projectIds, preparedStmtListDescendants);
List<Project> projects = jdbcTemplate.query(query, addressRowMapper, preparedStmtListDescendants.toArray());
log.info("Fetched project immediate descendants list based on given Project Ids");
return projects;
}

/* Fetch targets based on Project Ids */
private List<Target> getTargetsBasedOnProjectIds(Set<String> projectIds) {
List<Object> preparedStmtListTarget = new ArrayList<>();
Expand Down Expand Up @@ -226,6 +233,16 @@ private List<Project> getProjectDescendants(List<Project> projects) {
return getProjectsDescendantsBasedOnProjectIds(projectIds, preparedStmtListDescendants);
}

/* Fetch projects where project parent for projects in db contains project ID of requested project.*/
private List<Project> getProjectImmediateDescendants(List<Project> projects) {
List<String> requestProjectIds = projects.stream().map(Project::getId).collect(Collectors.toList());

List<Object> preparedStmtListDescendants = new ArrayList<>();
log.info("Fetching immediate descendant projects");

return getProjectsImmediateDescendantsBasedOnProjectIds(requestProjectIds, preparedStmtListDescendants);
}

Comment on lines +236 to +245
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)

Enhance logging for better debugging.

The current logging could be more informative by including the number of projects being processed and retrieved.

Consider enhancing the logging:

     private List<Project> getProjectImmediateDescendants(List<Project> projects) {
         List<String> requestProjectIds = projects.stream().map(Project::getId).collect(Collectors.toList());
 
         List<Object> preparedStmtListDescendants = new ArrayList<>();
-        log.info("Fetching immediate descendant projects");
+        log.info("Fetching immediate descendant projects for {} parent projects", projects.size());
 
-        return getProjectsImmediateDescendantsBasedOnProjectIds(requestProjectIds, preparedStmtListDescendants);
+        List<Project> descendants = getProjectsImmediateDescendantsBasedOnProjectIds(requestProjectIds, preparedStmtListDescendants);
+        log.info("Found {} immediate descendant projects", descendants.size());
+        return descendants;
     }
📝 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
/* Fetch projects where project parent for projects in db contains project ID of requested project.*/
private List<Project> getProjectImmediateDescendants(List<Project> projects) {
List<String> requestProjectIds = projects.stream().map(Project::getId).collect(Collectors.toList());
List<Object> preparedStmtListDescendants = new ArrayList<>();
log.info("Fetching immediate descendant projects");
return getProjectsImmediateDescendantsBasedOnProjectIds(requestProjectIds, preparedStmtListDescendants);
}
/* Fetch projects where project parent for projects in db contains project ID of requested project.*/
private List<Project> getProjectImmediateDescendants(List<Project> projects) {
List<String> requestProjectIds = projects.stream().map(Project::getId).collect(Collectors.toList());
List<Object> preparedStmtListDescendants = new ArrayList<>();
log.info("Fetching immediate descendant projects for {} parent projects", projects.size());
List<Project> descendants = getProjectsImmediateDescendantsBasedOnProjectIds(requestProjectIds, preparedStmtListDescendants);
log.info("Found {} immediate descendant projects", descendants.size());
return descendants;
}

/* Constructs Project Objects with fetched projects, targets and documents using Project id and return list of Projects */
private List<Project> buildProjectSearchResult(List<Project> projects, List<Target> targets, List<Document> documents, List<Project> ancestors, List<Project> descendants) {
for (Project project: projects) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,18 @@ public String getProjectDescendantsSearchQueryBasedOnIds(List<String> projectIds

return queryBuilder.toString();
}

/* Returns query to search for projects where project parent contains project Ids */
public String getProjectImmediateDescendantsSearchQueryBasedOnIds(List<String> projectIds, List<Object> preparedStmtListDescendants) {
StringBuilder queryBuilder = new StringBuilder(FETCH_PROJECT_ADDRESS_QUERY);
for (String projectId : projectIds) {
addConditionalClause(preparedStmtListDescendants, queryBuilder);
queryBuilder.append(" ( prj.parent = ? )");
preparedStmtListDescendants.add(projectId);
}
Comment on lines +370 to +376
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)

Add input validation for projectIds.

Consider adding input validation to handle null or empty projectIds list more gracefully.

Here's a suggested improvement:

     public String getProjectImmediateDescendantsSearchQueryBasedOnIds(List<String> projectIds, List<Object> preparedStmtListDescendants) {
         StringBuilder queryBuilder = new StringBuilder(FETCH_PROJECT_ADDRESS_QUERY);
+        if (projectIds == null || projectIds.isEmpty()) {
+            return queryBuilder.toString();
+        }
         for (String projectId : projectIds) {
+            if (StringUtils.isBlank(projectId)) {
+                continue;
+            }
             addConditionalClause(preparedStmtListDescendants, queryBuilder);
             queryBuilder.append(" ( prj.parent = ? )");
             preparedStmtListDescendants.add(projectId);
         }
📝 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
public String getProjectImmediateDescendantsSearchQueryBasedOnIds(List<String> projectIds, List<Object> preparedStmtListDescendants) {
StringBuilder queryBuilder = new StringBuilder(FETCH_PROJECT_ADDRESS_QUERY);
for (String projectId : projectIds) {
addConditionalClause(preparedStmtListDescendants, queryBuilder);
queryBuilder.append(" ( prj.parent = ? )");
preparedStmtListDescendants.add(projectId);
}
public String getProjectImmediateDescendantsSearchQueryBasedOnIds(List<String> projectIds, List<Object> preparedStmtListDescendants) {
StringBuilder queryBuilder = new StringBuilder(FETCH_PROJECT_ADDRESS_QUERY);
if (projectIds == null || projectIds.isEmpty()) {
return queryBuilder.toString();
}
for (String projectId : projectIds) {
if (StringUtils.isBlank(projectId)) {
continue;
}
addConditionalClause(preparedStmtListDescendants, queryBuilder);
queryBuilder.append(" ( prj.parent = ? )");
preparedStmtListDescendants.add(projectId);
}


return queryBuilder.toString();
}
Comment on lines +369 to +379
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)

Add method documentation for consistency.

The implementation looks good, but for consistency with other methods in the class, consider adding Javadoc documentation.

Add the following documentation:

+    /**
+     * Constructs SQL query to find immediate child projects based on parent project IDs.
+     *
+     * @param projectIds                  The list of parent project IDs to search for
+     * @param preparedStmtListDescendants The list to which prepared statement parameters will be added
+     * @return                           The constructed SQL query string
+     */
     public String getProjectImmediateDescendantsSearchQueryBasedOnIds(List<String> projectIds, List<Object> preparedStmtListDescendants) {
📝 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
/* Returns query to search for projects where project parent contains project Ids */
public String getProjectImmediateDescendantsSearchQueryBasedOnIds(List<String> projectIds, List<Object> preparedStmtListDescendants) {
StringBuilder queryBuilder = new StringBuilder(FETCH_PROJECT_ADDRESS_QUERY);
for (String projectId : projectIds) {
addConditionalClause(preparedStmtListDescendants, queryBuilder);
queryBuilder.append(" ( prj.parent = ? )");
preparedStmtListDescendants.add(projectId);
}
return queryBuilder.toString();
}
/**
* Constructs SQL query to find immediate child projects based on parent project IDs.
*
* @param projectIds The list of parent project IDs to search for
* @param preparedStmtListDescendants The list to which prepared statement parameters will be added
* @return The constructed SQL query string
*/
public String getProjectImmediateDescendantsSearchQueryBasedOnIds(List<String> projectIds, List<Object> preparedStmtListDescendants) {
StringBuilder queryBuilder = new StringBuilder(FETCH_PROJECT_ADDRESS_QUERY);
for (String projectId : projectIds) {
addConditionalClause(preparedStmtListDescendants, queryBuilder);
queryBuilder.append(" ( prj.parent = ? )");
preparedStmtListDescendants.add(projectId);
}
return queryBuilder.toString();
}


/* Returns query to get total projects count based on project search params */
public String getSearchCountQueryString(List<Project> projects, String tenantId, Long lastChangedSince, Boolean includeDeleted, Long createdFrom, Long createdTo, List<Object> preparedStatement) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ public List<Project> searchProject(
Boolean includeDeleted,
Boolean includeAncestors,
Boolean includeDescendants,
Boolean includeImmediateChildren,
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)

Add JavaDoc for the new parameter.

The new includeImmediateChildren parameter should be documented to clarify its purpose and behavior, especially its interaction with other inclusion flags (includeAncestors, includeDescendants).

Add parameter documentation:

    /**
     * Search for projects based on various criteria
     * @param project Project request containing search criteria
     * ...
+    * @param includeImmediateChildren When true, includes only the immediate children of matched projects
+    *                                This parameter takes precedence over includeDescendants
     * ...
     * @return List of matching projects
     */

Also applies to: 105-105

Long createdFrom,
Long createdTo
) {
Expand All @@ -101,6 +102,7 @@ public List<Project> searchProject(
includeDeleted,
includeAncestors,
includeDescendants,
includeImmediateChildren,
createdFrom,
createdTo
);
Expand All @@ -125,7 +127,7 @@ public ProjectRequest updateProject(ProjectRequest request) {
List<Project> projectsFromDB = searchProject(
getSearchProjectRequest(request.getProjects(), request.getRequestInfo(), false),
projectConfiguration.getMaxLimit(), projectConfiguration.getDefaultOffset(),
request.getProjects().get(0).getTenantId(), null, false, false, false, null, null
request.getProjects().get(0).getTenantId(), null, false, false, false, false,null, null
);
log.info("Fetched projects for update request");

Expand Down Expand Up @@ -279,6 +281,7 @@ private void checkAndEnrichCascadingProjectDates(ProjectRequest request, Project
false,
true,
true,
false,
null,
null
);
Expand All @@ -301,7 +304,7 @@ private List<Project> getParentProjects(ProjectRequest projectRequest) {
List<Project> parentProjects = null;
List<Project> projectsForSearchRequest = projectRequest.getProjects().stream().filter(p -> StringUtils.isNotBlank(p.getParent())).collect(Collectors.toList());
if (projectsForSearchRequest.size() > 0) {
parentProjects = searchProject(getSearchProjectRequest(projectsForSearchRequest, projectRequest.getRequestInfo(), true), projectConfiguration.getMaxLimit(), projectConfiguration.getDefaultOffset(), projectRequest.getProjects().get(0).getTenantId(), null, false, false, false, null, null);
parentProjects = searchProject(getSearchProjectRequest(projectsForSearchRequest, projectRequest.getRequestInfo(), true), projectConfiguration.getMaxLimit(), projectConfiguration.getDefaultOffset(), projectRequest.getProjects().get(0).getTenantId(), null, false, false, false, false, null, null);
}
log.info("Fetched parent projects from DB");
return parentProjects;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ public ResponseEntity<ProjectResponse> searchProject(
@ApiParam(value = "Used in search APIs to specify if (soft) deleted records should be included in search results.", defaultValue = "false") @Valid @RequestParam(value = "includeDeleted", required = false, defaultValue = "false") Boolean includeDeleted,
@ApiParam(value = "Used in project search API to specify if response should include project elements that are in the preceding hierarchy of matched projects.", defaultValue = "false") @Valid @RequestParam(value = "includeAncestors", required = false, defaultValue = "false") Boolean includeAncestors,
@ApiParam(value = "Used in project search API to specify if response should include project elements that are in the following hierarchy of matched projects.", defaultValue = "false") @Valid @RequestParam(value = "includeDescendants", required = false, defaultValue = "false") Boolean includeDescendants,
@ApiParam(value = "Used in project search API to limit the search results to only those projects whose parent matched with ProjectId", defaultValue = "false") @Valid @RequestParam(value = "includeImmediateChildren", required = false, defaultValue = "false") Boolean includeImmediateChildren,
@ApiParam(value = "Used in project search API to limit the search results to only those projects whose creation date is after the specified 'createdFrom' date", defaultValue = "false") @Valid @RequestParam(value = "createdFrom", required = false) Long createdFrom,
@ApiParam(value = "Used in project search API to limit the search results to only those projects whose creation date is before the specified 'createdTo' date", defaultValue = "false") @Valid @RequestParam(value = "createdTo", required = false) Long createdTo
) {
Expand All @@ -495,6 +496,7 @@ public ResponseEntity<ProjectResponse> searchProject(
includeDeleted,
includeAncestors,
includeDescendants,
includeImmediateChildren,
createdFrom,
createdTo
);
Expand Down
Loading