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

Created API for get count of any fields and with field value search #328

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,42 @@ static CompletionStage<BackEnd.GetConfigurationResponse> getConfiguration(
return stage.thenApply(response -> response);
}

static CompletionStage<BackEnd.GetAgeGroupCountResponse> getAgeGroupCount(
final ActorSystem<Void> actorSystem,
final ActorRef<BackEnd.Event> backEnd,
final ApiModels.SearchAgeCountFields searchAgeCountFields) {
final CompletionStage<BackEnd.GetAgeGroupCountResponse> stage = AskPattern
.ask(backEnd,
replyTo -> new BackEnd.GetAgeGroupCountRequest(replyTo, searchAgeCountFields),
java.time.Duration.ofSeconds(GlobalConstants.TIMEOUT_DGRAPH_QUERY_SECS),
actorSystem.scheduler());
return stage.thenApply(response -> response);
}

static CompletionStage<BackEnd.GetAllListResponse> getAllList(
final ActorSystem<Void> actorSystem,
final ActorRef<BackEnd.Event> backEnd,
final ApiModels.AllList allList) {
final CompletionStage<BackEnd.GetAllListResponse> stage = AskPattern
.ask(backEnd,
replyTo -> new BackEnd.GetAllListRequest(replyTo, allList),
java.time.Duration.ofSeconds(GlobalConstants.TIMEOUT_DGRAPH_QUERY_SECS),
actorSystem.scheduler());
return stage.thenApply(response -> response);
}

static CompletionStage<BackEnd.GetFieldCountResponse> getFieldCount(
final ActorSystem<Void> actorSystem,
final ActorRef<BackEnd.Event> backEnd,
final ApiModels.CountFields countFields) {
final CompletionStage<BackEnd.GetFieldCountResponse> stage = AskPattern
.ask(backEnd,
replyTo -> new BackEnd.GetFieldCountRequest(replyTo, countFields),
java.time.Duration.ofSeconds(GlobalConstants.TIMEOUT_DGRAPH_QUERY_SECS),
actorSystem.scheduler());
return stage.thenApply(response -> response);
}

static CompletionStage<BackEnd.GetFieldsConfigurationResponse> getFieldsConfiguration(
final ActorSystem<Void> actorSystem,
final ActorRef<BackEnd.Event> backEnd) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@
import java.sql.SQLException;
import java.sql.Timestamp;
import java.time.LocalDateTime;
import java.time.Period;
import java.util.*;
import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeParseException;

public final class BackEnd extends AbstractBehavior<BackEnd.Event> {

Expand Down Expand Up @@ -165,6 +169,9 @@ public Receive<Event> actor() {
.onMessage(PostUploadCsvFileRequest.class, this::postUploadCsvFileHandler)
.onMessage(SQLDashboardDataRequest.class, this::getSqlDashboardDataHandler)
.onMessage(GetConfigurationRequest.class, this::getConfigurationHandler)
.onMessage(GetFieldCountRequest.class, this::getFieldCountHandler)
.onMessage(GetAgeGroupCountRequest.class, this::getAgeGroupCountHandler)
.onMessage(GetAllListRequest.class, this::getAllListHandler)
.onMessage(PostConfigurationRequest.class, this::postConfigurationHandler)
.onMessage(GetFieldsConfigurationRequest.class, this::getFieldsConfigurationHandler)
.build();
Expand Down Expand Up @@ -514,6 +521,69 @@ private Behavior<Event> getConfigurationHandler(final GetConfigurationRequest re
return Behaviors.same();
}

private Behavior<Event> getFieldCountHandler(final GetFieldCountRequest request) {
String getCount = null;
try {
getCount = libMPI.getFieldCount(request.countFields);
request.replyTo.tell(new GetFieldCountResponse(getCount));
} catch (Exception e) {
LOGGER.error(e.getLocalizedMessage(), e);
LOGGER.error("libMPI.findExpandedGoldenRecord failed for goldenId: {} with error: {}", e.getMessage());
Comment on lines +530 to +531
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

Incorrect error message and mismatched logging arguments in 'getFieldCountHandler'

The error message references libMPI.findExpandedGoldenRecord and uses two placeholders {} but only provides one argument e.getMessage(). This is incorrect for this method and will cause logging issues.

Apply this diff to fix the error message and placeholders:

- LOGGER.error("libMPI.findExpandedGoldenRecord failed for goldenId: {} with error: {}", e.getMessage());
+ LOGGER.error("libMPI.getFieldCount failed with error: {}", e.getMessage());
📝 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
LOGGER.error(e.getLocalizedMessage(), e);
LOGGER.error("libMPI.findExpandedGoldenRecord failed for goldenId: {} with error: {}", e.getMessage());
LOGGER.error(e.getLocalizedMessage(), e);
LOGGER.error("libMPI.getFieldCount failed with error: {}", e.getMessage());

}
return Behaviors.same();
}
Comment on lines +524 to +534
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 error message and improve error logging

The error message in the catch block is incorrect. It mentions "findExpandedGoldenRecord" which is not related to this method. Also, the error logging can be improved by using placeholders.

Apply this diff to fix the error message and improve logging:

 private Behavior<Event> getFieldCountHandler(final GetFieldCountRequest request) {
     String getCount = null;
     try {
         getCount = libMPI.getFieldCount(request.countFields);
         request.replyTo.tell(new GetFieldCountResponse(getCount));
     } catch (Exception e) {
         LOGGER.error(e.getLocalizedMessage(), e);
-        LOGGER.error("libMPI.findExpandedGoldenRecord failed for goldenId: {} with error: {}", e.getMessage());
+        LOGGER.error("libMPI.getFieldCount failed with error: {}", e.getMessage());
     }
     return Behaviors.same();
 }
📝 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 Behavior<Event> getFieldCountHandler(final GetFieldCountRequest request) {
String getCount = null;
try {
getCount = libMPI.getFieldCount(request.countFields);
request.replyTo.tell(new GetFieldCountResponse(getCount));
} catch (Exception e) {
LOGGER.error(e.getLocalizedMessage(), e);
LOGGER.error("libMPI.findExpandedGoldenRecord failed for goldenId: {} with error: {}", e.getMessage());
}
return Behaviors.same();
}
private Behavior<Event> getFieldCountHandler(final GetFieldCountRequest request) {
String getCount = null;
try {
getCount = libMPI.getFieldCount(request.countFields);
request.replyTo.tell(new GetFieldCountResponse(getCount));
} catch (Exception e) {
LOGGER.error(e.getLocalizedMessage(), e);
LOGGER.error("libMPI.getFieldCount failed with error: {}", e.getMessage());
}
return Behaviors.same();
}


private Behavior<Event> getAgeGroupCountHandler(final GetAgeGroupCountRequest request) {
long getCount = 0;
try {
String startDob = request.searchAgeCountFields.startDate();
String endDob = request.searchAgeCountFields.endDate();
LOGGER.info("startDob: {}, endDob: {}", startDob, endDob);
getCount = libMPI.getAgeGroupCount(request.searchAgeCountFields);
request.replyTo.tell(new GetAgeGroupCountResponse(getCount));
} catch (Exception e) {
LOGGER.error(e.getLocalizedMessage(), e);
LOGGER.error("libMPI.findExpandedGoldenRecord failed for goldenId: {} with error: {}", e.getMessage());
Comment on lines +545 to +546
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

Incorrect error message and mismatched logging arguments in 'getAgeGroupCountHandler'

The error message references libMPI.findExpandedGoldenRecord and uses two placeholders {} but only provides one argument e.getMessage(). This is incorrect for this method and will cause logging issues.

Apply this diff to fix the error message and placeholders:

- LOGGER.error("libMPI.findExpandedGoldenRecord failed for goldenId: {} with error: {}", e.getMessage());
+ LOGGER.error("libMPI.getAgeGroupCount failed with error: {}", e.getMessage());
📝 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
LOGGER.error(e.getLocalizedMessage(), e);
LOGGER.error("libMPI.findExpandedGoldenRecord failed for goldenId: {} with error: {}", e.getMessage());
LOGGER.error(e.getLocalizedMessage(), e);
LOGGER.error("libMPI.getAgeGroupCount failed with error: {}", e.getMessage());

}
return Behaviors.same();
}
Comment on lines +536 to +549
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 error message and improve error logging

The error message in the catch block is incorrect. It mentions "findExpandedGoldenRecord" which is not related to this method. Also, the error logging can be improved by using placeholders.

Apply this diff to fix the error message and improve logging:

 private Behavior<Event> getAgeGroupCountHandler(final GetAgeGroupCountRequest request) {
     long getCount = 0;
     try {
         String startDob = request.searchAgeCountFields.startDate();
         String endDob = request.searchAgeCountFields.endDate();
         LOGGER.info("startDob: {}, endDob: {}", startDob, endDob);
         getCount = libMPI.getAgeGroupCount(request.searchAgeCountFields);
         request.replyTo.tell(new GetAgeGroupCountResponse(getCount));
     } catch (Exception e) {
         LOGGER.error(e.getLocalizedMessage(), e);
-        LOGGER.error("libMPI.findExpandedGoldenRecord failed for goldenId: {} with error: {}", e.getMessage());
+        LOGGER.error("libMPI.getAgeGroupCount failed with error: {}", e.getMessage());
     }
     return Behaviors.same();
 }
📝 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 Behavior<Event> getAgeGroupCountHandler(final GetAgeGroupCountRequest request) {
long getCount = 0;
try {
String startDob = request.searchAgeCountFields.startDate();
String endDob = request.searchAgeCountFields.endDate();
LOGGER.info("startDob: {}, endDob: {}", startDob, endDob);
getCount = libMPI.getAgeGroupCount(request.searchAgeCountFields);
request.replyTo.tell(new GetAgeGroupCountResponse(getCount));
} catch (Exception e) {
LOGGER.error(e.getLocalizedMessage(), e);
LOGGER.error("libMPI.findExpandedGoldenRecord failed for goldenId: {} with error: {}", e.getMessage());
}
return Behaviors.same();
}
private Behavior<Event> getAgeGroupCountHandler(final GetAgeGroupCountRequest request) {
long getCount = 0;
try {
String startDob = request.searchAgeCountFields.startDate();
String endDob = request.searchAgeCountFields.endDate();
LOGGER.info("startDob: {}, endDob: {}", startDob, endDob);
getCount = libMPI.getAgeGroupCount(request.searchAgeCountFields);
request.replyTo.tell(new GetAgeGroupCountResponse(getCount));
} catch (Exception e) {
LOGGER.error(e.getLocalizedMessage(), e);
LOGGER.error("libMPI.getAgeGroupCount failed with error: {}", e.getMessage());
}
return Behaviors.same();
}


private Behavior<Event> getAllListHandler(final GetAllListRequest request) {
List<String> dobList = new ArrayList<>();
try {
dobList = libMPI.getAllList(request.allListRequest);
LOGGER.info("dobList size: {}", dobList.size());
// double allList = calculateAvarageAge(dobList);
request.replyTo.tell(new GetAllListResponse(dobList));
} catch (Exception e) {
LOGGER.error(e.getLocalizedMessage(), e);
LOGGER.error("libMPI.getAllList failed for allListRequest: {} with error: {}", request.allListRequest, e.getMessage());
}
return Behaviors.same();
}

public static double calculateAvarageAge(final List<String> dobList) {
LocalDate today = LocalDate.now(); // Get today's date
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyyMMdd"); // DOB format in YYYYMMDD
int totalAge = 0;
int count = 0;
// Iterate through the list of DOBs and calculate the age for each
for (String dob : dobList) {
if (!dob.isEmpty()) {
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

Add null check for 'dob' before calling 'isEmpty()'

There's a potential for a NullPointerException if dob is null. It's safer to check for null before using isEmpty().

Apply this diff:

- if (!dob.isEmpty()) {
+ if (dob != null && !dob.isEmpty()) {
📝 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 (!dob.isEmpty()) {
if (dob != null && !dob.isEmpty()) {

try {
LocalDate birthDate = LocalDate.parse(dob, formatter); // Try to convert DOB to LocalDate
int age = Period.between(birthDate, today).getYears(); // Calculate age in years
totalAge += age;
count++;
} catch (DateTimeParseException e) {
LOGGER.error("Invalid date format for dob: " + dob + ". Skipping this record.");
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

Use placeholders in logging instead of string concatenation

In the logging statement, string concatenation is used, which can lead to unnecessary object creation even if the log level is not enabled. It's better to use placeholders for efficiency.

Apply this diff:

- LOGGER.error("Invalid date format for dob: " + dob + ". Skipping this record.");
+ LOGGER.error("Invalid date format for dob: {}. Skipping this record.", dob);
📝 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
LOGGER.error("Invalid date format for dob: " + dob + ". Skipping this record.");
LOGGER.error("Invalid date format for dob: {}. Skipping this record.", dob);

}
}
}
// Calculate and return average age
return count > 0 ? (double) totalAge / count : 0;
}
Comment on lines +565 to +585
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

Improve method name, use constant for date format, and enhance error logging

There are several improvements that can be made to this method:

  1. The method name has a typo: "Avarage" should be "Average".
  2. The date format string should be a constant.
  3. Error logging for invalid date formats can be improved.

Apply this diff to implement these improvements:

-    public static double calculateAvarageAge(final List<String> dobList) {
+    private static final DateTimeFormatter DOB_FORMATTER = DateTimeFormatter.ofPattern("yyyyMMdd");
+
+    public static double calculateAverageAge(final List<String> dobList) {
         LocalDate today = LocalDate.now();  // Get today's date
-        DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyyMMdd");  // DOB format in YYYYMMDD
         int totalAge = 0;
         int count = 0;
         // Iterate through the list of DOBs and calculate the age for each
         for (String dob : dobList) {
             if (!dob.isEmpty()) {
                try {
-                LocalDate birthDate = LocalDate.parse(dob, formatter);  // Try to convert DOB to LocalDate
+                LocalDate birthDate = LocalDate.parse(dob, DOB_FORMATTER);  // Try to convert DOB to LocalDate
                 int age = Period.between(birthDate, today).getYears();  // Calculate age in years
                 totalAge += age;
                 count++;
                } catch (DateTimeParseException e) {
-                  LOGGER.error("Invalid date format for dob: " + dob + ". Skipping this record.");
+                  LOGGER.error("Invalid date format for dob: {}. Skipping this record.", dob);
                }
             }
         }
         // Calculate and return average age
         return count > 0 ? (double) totalAge / count : 0;
     }
📝 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 static double calculateAvarageAge(final List<String> dobList) {
LocalDate today = LocalDate.now(); // Get today's date
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyyMMdd"); // DOB format in YYYYMMDD
int totalAge = 0;
int count = 0;
// Iterate through the list of DOBs and calculate the age for each
for (String dob : dobList) {
if (!dob.isEmpty()) {
try {
LocalDate birthDate = LocalDate.parse(dob, formatter); // Try to convert DOB to LocalDate
int age = Period.between(birthDate, today).getYears(); // Calculate age in years
totalAge += age;
count++;
} catch (DateTimeParseException e) {
LOGGER.error("Invalid date format for dob: " + dob + ". Skipping this record.");
}
}
}
// Calculate and return average age
return count > 0 ? (double) totalAge / count : 0;
}
private static final DateTimeFormatter DOB_FORMATTER = DateTimeFormatter.ofPattern("yyyyMMdd");
public static double calculateAverageAge(final List<String> dobList) {
LocalDate today = LocalDate.now(); // Get today's date
int totalAge = 0;
int count = 0;
// Iterate through the list of DOBs and calculate the age for each
for (String dob : dobList) {
if (!dob.isEmpty()) {
try {
LocalDate birthDate = LocalDate.parse(dob, DOB_FORMATTER); // Try to convert DOB to LocalDate
int age = Period.between(birthDate, today).getYears(); // Calculate age in years
totalAge += age;
count++;
} catch (DateTimeParseException e) {
LOGGER.error("Invalid date format for dob: {}. Skipping this record.", dob);
}
}
}
// Calculate and return average age
return count > 0 ? (double) totalAge / count : 0;
}


private Behavior<Event> getFieldsConfigurationHandler(final GetFieldsConfigurationRequest request) {
final var separator = FileSystems.getDefault().getSeparator();
final String configDir = System.getenv("SYSTEM_CONFIG_DIRS");
Expand Down Expand Up @@ -617,6 +687,18 @@ public record GetConfigurationRequest(ActorRef<GetConfigurationResponse> replyTo

public record GetConfigurationResponse(Configuration configuration) implements EventResponse { }

public record GetFieldCountRequest(ActorRef<GetFieldCountResponse> replyTo, ApiModels.CountFields countFields) implements Event { }

public record GetFieldCountResponse(String genderCount) implements EventResponse { }

public record GetAgeGroupCountRequest(ActorRef<GetAgeGroupCountResponse> replyTo, ApiModels.SearchAgeCountFields searchAgeCountFields) implements Event { }

public record GetAgeGroupCountResponse(long ageGroupCount) implements EventResponse { }

public record GetAllListRequest(ActorRef<GetAllListResponse> replyTo, ApiModels.AllList allListRequest) implements Event { }

public record GetAllListResponse(List<String> records) implements EventResponse { }

public record GetFieldsConfigurationRequest(ActorRef<GetFieldsConfigurationResponse> replyTo) implements Event { }

public record GetFieldsConfigurationResponse(List<FieldsConfiguration.Field> fields) implements EventResponse { }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import akka.http.javadsl.marshalling.Marshaller;
import akka.http.javadsl.model.*;
import akka.http.javadsl.server.Route;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.jembi.jempi.libmpi.MpiServiceError;
Expand Down Expand Up @@ -548,6 +550,53 @@ public static Route getConfiguration(
});
}

public static Route getFieldCount(
final ActorSystem<Void> actorSystem,
final ActorRef<BackEnd.Event> backEnd) {
return entity(Jackson.unmarshaller(ApiModels.CountFields.class),
request -> onComplete(Ask.getFieldCount(actorSystem, backEnd, request),
result -> {
if (!result.isSuccess()) {
return handleError(result.failed().get());
}
ObjectMapper objectMapper = new ObjectMapper();
JsonNode jsonResponse = null;
try {
jsonResponse = objectMapper.readTree(result.get().genderCount());
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
Comment on lines +565 to +568
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 JsonProcessingException without throwing RuntimeException

Throwing a RuntimeException in the catch block is not recommended as it may lead to unhandled exceptions and obscure the original error context. Instead, consider handling the exception gracefully by logging the error and returning an appropriate error response using handleError.

Apply this diff to handle the exception properly:

  } catch (JsonProcessingException e) {
-     throw new RuntimeException(e);
+     LOGGER.error("Failed to parse JSON response in getFieldCount", e);
+     return handleError(e);
  }
📝 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
jsonResponse = objectMapper.readTree(result.get().genderCount());
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
jsonResponse = objectMapper.readTree(result.get().genderCount());
} catch (JsonProcessingException e) {
LOGGER.error("Failed to parse JSON response in getFieldCount", e);
return handleError(e);
}

// Complete the request with JSON response
return complete(StatusCodes.OK, jsonResponse, JSON_MARSHALLER);
Comment on lines +562 to +570
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

Optimize ObjectMapper usage and improve exception handling

  1. Instead of creating a new ObjectMapper instance, reuse the existing OBJECT_MAPPER from AppUtils.
  2. Handle the JsonProcessingException more gracefully instead of throwing a RuntimeException.
  3. Consider returning the genderCount directly without parsing it to JsonNode for consistency with other methods.

Apply this diff to address these issues:

- ObjectMapper objectMapper = new ObjectMapper();
  JsonNode jsonResponse = null;
  try {
-     jsonResponse = objectMapper.readTree(result.get().genderCount());
-  } catch (JsonProcessingException e) {
-     throw new RuntimeException(e);
-  }
-  // Complete the request with JSON response
-  return complete(StatusCodes.OK, jsonResponse, JSON_MARSHALLER);
+  return complete(StatusCodes.OK, result.get().genderCount(), JSON_MARSHALLER);

This change simplifies the code, improves efficiency by reusing the existing OBJECT_MAPPER, and maintains consistency with other methods in the class.

Committable suggestion was skipped due to low confidence.

}));
Comment on lines +553 to +571
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

Reuse existing ObjectMapper instance instead of creating a new one

In the getFieldCount method, a new ObjectMapper instance is created at line 562. Since OBJECT_MAPPER is already available (imported from AppUtils), it's better to reuse it to avoid unnecessary object creation and maintain consistency throughout the codebase.

Apply this diff to reuse the existing OBJECT_MAPPER:

- ObjectMapper objectMapper = new ObjectMapper();
  JsonNode jsonResponse = null;
  try {
-     jsonResponse = objectMapper.readTree(result.get().genderCount());
+     jsonResponse = OBJECT_MAPPER.readTree(result.get().genderCount());
  } catch (JsonProcessingException e) {
      throw new RuntimeException(e);
  }
📝 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 static Route getFieldCount(
final ActorSystem<Void> actorSystem,
final ActorRef<BackEnd.Event> backEnd) {
return entity(Jackson.unmarshaller(ApiModels.CountFields.class),
request -> onComplete(Ask.getFieldCount(actorSystem, backEnd, request),
result -> {
if (!result.isSuccess()) {
return handleError(result.failed().get());
}
ObjectMapper objectMapper = new ObjectMapper();
JsonNode jsonResponse = null;
try {
jsonResponse = objectMapper.readTree(result.get().genderCount());
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
// Complete the request with JSON response
return complete(StatusCodes.OK, jsonResponse, JSON_MARSHALLER);
}));
public static Route getFieldCount(
final ActorSystem<Void> actorSystem,
final ActorRef<BackEnd.Event> backEnd) {
return entity(Jackson.unmarshaller(ApiModels.CountFields.class),
request -> onComplete(Ask.getFieldCount(actorSystem, backEnd, request),
result -> {
if (!result.isSuccess()) {
return handleError(result.failed().get());
}
JsonNode jsonResponse = null;
try {
jsonResponse = OBJECT_MAPPER.readTree(result.get().genderCount());
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
// Complete the request with JSON response
return complete(StatusCodes.OK, jsonResponse, JSON_MARSHALLER);
}));

}

public static Route getAgeGroupCount(
final ActorSystem<Void> actorSystem,
final ActorRef<BackEnd.Event> backEnd) {
return entity(Jackson.unmarshaller(ApiModels.SearchAgeCountFields.class),
request -> onComplete(Ask.getAgeGroupCount(actorSystem, backEnd, request),
result -> {
if (!result.isSuccess()) {
return handleError(result.failed().get());
}
return complete(StatusCodes.OK, result.get().ageGroupCount(), JSON_MARSHALLER);
}));
}
Comment on lines +574 to +585
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

Ensure consistent exception handling across methods

In the getAgeGroupCount method, exceptions that may occur during processing (e.g., JsonProcessingException) are not caught or handled. This contrasts with getFieldCount, where exceptions are explicitly handled. For consistency and robustness, ensure that exceptions are properly handled in all methods.

Consider adding exception handling similar to getFieldCount:

return entity(Jackson.unmarshaller(ApiModels.SearchAgeCountFields.class),
    request -> onComplete(Ask.getAgeGroupCount(actorSystem, backEnd, request),
        result -> {
            if (!result.isSuccess()) {
                return handleError(result.failed().get());
            }
            try {
                return complete(StatusCodes.OK, result.get().ageGroupCount(), JSON_MARSHALLER);
            } catch (Exception e) {
                LOGGER.error("Error processing ageGroupCount response", e);
                return handleError(e);
            }
        }));

Alternatively, ensure that any potential exceptions are handled within the called methods or that appropriate error handling is in place.


private static Route getDataList(
final ActorSystem<Void> actorSystem,
final ActorRef<BackEnd.Event> backEnd) {
return entity(Jackson.unmarshaller(ApiModels.AllList.class),
request -> onComplete(Ask.getAllList(actorSystem, backEnd, request),
result -> {
if (!result.isSuccess()) {
return handleError(result.failed().get());
}
return complete(StatusCodes.OK, result.get(), JSON_MARSHALLER);
}));
}

private static Route getFieldsConfiguration(
final ActorSystem<Void> actorSystem,
final ActorRef<BackEnd.Event> backEnd) {
Expand Down Expand Up @@ -661,6 +710,12 @@ public static Route createCoreAPIRoutes(
() -> Routes.getNotifications(actorSystem, backEnd)),
path(GlobalConstants.SEGMENT_POST_GOLDEN_RECORD,
() -> Routes.updateGoldenRecord(actorSystem, backEnd)),
path(GlobalConstants.SEGMENT_GET_FIELD_COUNT,
() -> Routes.getFieldCount(actorSystem, backEnd)),
path(GlobalConstants.SEGMENT_GET_AGE_GROUP_COUNT,
() -> Routes.getAgeGroupCount(actorSystem, backEnd)),
path(GlobalConstants.SEGMENT_GET_DATA_LIST,
() -> Routes.getDataList(actorSystem, backEnd)),
Comment on lines +713 to +718
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

Consider using GET method for retrieval operations

The new routes for getFieldCount, getAgeGroupCount, and getDataList are currently added to the POST route group. However, these operations are typically used for retrieving data rather than modifying it. According to RESTful principles, GET requests are more appropriate for such operations.

Consider moving these routes to the GET method group for better alignment with RESTful practices. Here's a suggested change:

-post(() -> concat(
+get(() -> concat(
     // ... existing GET routes ...
     path(GlobalConstants.SEGMENT_GET_FIELDS_CONFIGURATION,
          () -> Routes.getFieldsConfiguration(actorSystem, backEnd)),
+    path(GlobalConstants.SEGMENT_GET_FIELD_COUNT,
+         () -> Routes.getFieldCount(actorSystem, backEnd)),
+    path(GlobalConstants.SEGMENT_GET_AGE_GROUP_COUNT,
+         () -> Routes.getAgeGroupCount(actorSystem, backEnd)),
+    path(GlobalConstants.SEGMENT_GET_DATA_LIST,
+         () -> Routes.getDataList(actorSystem, backEnd))
 )))

This change would make the API more intuitive and consistent with RESTful conventions.

Committable suggestion was skipped due to low confidence.

path(GlobalConstants.SEGMENT_POST_FILTER_GIDS_WITH_INTERACTION_COUNT,
() -> Routes.postFilterGidsWithInteractionCount(actorSystem, backEnd)),
path(GlobalConstants.SEGMENT_PROXY_POST_CR_UPDATE_FIELDS,
Expand All @@ -677,7 +732,8 @@ public static Route createCoreAPIRoutes(
path(GlobalConstants.SEGMENT_GET_CONFIGURATION,
() -> Routes.getConfiguration(actorSystem, backEnd)),
path(GlobalConstants.SEGMENT_GET_FIELDS_CONFIGURATION,
() -> Routes.getFieldsConfiguration(actorSystem, backEnd)))));
() -> Routes.getFieldsConfiguration(actorSystem, backEnd))
)));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,21 @@ public Either<MpiGeneralError, List<GoldenRecord>> findGoldenRecords(final List<
return Either.right(results.get().data());
}

public String getFieldCount(final ApiModels.CountFields countFields) {
client.connect();
return client.getFieldCount(countFields);
}
Comment on lines +152 to +155
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

Consider revising the return type and adding exception handling

The getFieldCount method could be improved in the following ways:

  1. Change the return type from String to a numeric type like long or int, which is more appropriate for a count operation.
  2. Add exception handling to manage potential errors from the client method call.

Here's a suggested improvement:

-public String getFieldCount(final ApiModels.CountFields countFields) {
+public long getFieldCount(final ApiModels.CountFields countFields) throws ClientException {
     client.connect();
-    return client.getFieldCount(countFields);
+    try {
+        return Long.parseLong(client.getFieldCount(countFields));
+    } catch (Exception e) {
+        throw new ClientException("Error getting field count", e);
+    }
 }

This change assumes that ClientException is a custom exception class that you'd need to define. Also, ensure that the client method returns a string that can be parsed into a long.

Committable suggestion was skipped due to low confidence.


public long getAgeGroupCount(final ApiModels.SearchAgeCountFields searchAgeCountFields) {
client.connect();
return client.getAgeGroupCount(searchAgeCountFields);
}
Comment on lines +157 to +160
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

Add exception handling and consider using more specific date types

While the return type is appropriate for a count operation, the method lacks exception handling. Additionally, consider using more specific date types if the SearchAgeCountFields includes date parameters.

Consider the following improvements:

  1. Add exception handling.
  2. If applicable, use java.time.LocalDate instead of String for date parameters in SearchAgeCountFields.

Here's a suggested implementation:

-public long getAgeGroupCount(final ApiModels.SearchAgeCountFields searchAgeCountFields) {
+public long getAgeGroupCount(final ApiModels.SearchAgeCountFields searchAgeCountFields) throws ClientException {
     client.connect();
-    return client.getAgeGroupCount(searchAgeCountFields);
+    try {
+        return client.getAgeGroupCount(searchAgeCountFields);
+    } catch (Exception e) {
+        throw new ClientException("Error getting age group count", e);
+    }
 }

This change assumes that ClientException is a custom exception class that you'd need to define.

Committable suggestion was skipped due to low confidence.


public List<String> getAllList(final ApiModels.AllList allListRequest) {
client.connect();
return client.getAllList(allListRequest);
}
Comment on lines +162 to +165
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

Improve method naming and add exception handling

The current method name "getAllList" is somewhat redundant and not very descriptive. Additionally, the method lacks exception handling for potential errors from the client call.

Here are the suggested improvements:

  1. Rename the method to be more specific (e.g., getAllItemsList or a more descriptive name based on what it actually retrieves).
  2. Add exception handling.

Here's a suggested implementation:

-public List<String> getAllList(final ApiModels.AllList allListRequest) {
+public List<String> getAllItemsList(final ApiModels.AllList allListRequest) throws ClientException {
     client.connect();
-    return client.getAllList(allListRequest);
+    try {
+        return client.getAllList(allListRequest);
+    } catch (Exception e) {
+        throw new ClientException("Error retrieving all items list", e);
+    }
 }

This change assumes that ClientException is a custom exception class that you'd need to define. Also, consider updating the client method name if you change this method name.

📝 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 List<String> getAllList(final ApiModels.AllList allListRequest) {
client.connect();
return client.getAllList(allListRequest);
}
public List<String> getAllItemsList(final ApiModels.AllList allListRequest) throws ClientException {
client.connect();
try {
return client.getAllList(allListRequest);
} catch (Exception e) {
throw new ClientException("Error retrieving all items list", e);
}
}


public ExpandedGoldenRecord findExpandedGoldenRecord(final String goldenId) {
client.connect();
final var results = client.findExpandedGoldenRecords(List.of(goldenId));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ List<ExpandedSourceId> findExpandedSourceIdList(

PaginatedResultSet<ExpandedGoldenRecord> findExpandedGoldenRecords(List<String> goldenIds);

String getFieldCount(ApiModels.CountFields countFields);
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

Consider revising the return type and adding documentation for getFieldCount.

The method getFieldCount returns a String, which seems inconsistent with its name suggesting a count operation. Consider the following points:

  1. If the method is intended to return a count, it would be more appropriate to use a numeric return type like int or long.
  2. If a String return type is necessary (e.g., for a formatted result), please add a comment explaining the expected format and why a String is used.
  3. Add Javadoc comments to describe the method's purpose, parameters, and return value.

Here's a suggested revision:

/**
 * Counts the occurrences of specified fields.
 *
 * @param countFields The fields to be counted, specified using ApiModels.CountFields
 * @return The count of the specified fields
 */
long getFieldCount(ApiModels.CountFields countFields);

If a String return type is necessary, please explain why in a comment.


long getAgeGroupCount(ApiModels.SearchAgeCountFields searchAgeCountFields);

List<String> getAllList(ApiModels.AllList allListRequest);
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

Clarify the purpose of getAllList and add documentation.

The method getAllList is currently vague and lacks clarity. Consider the following improvements:

  1. Rename the method to more accurately reflect its specific purpose. For example, if it's retrieving a list of all users, it could be named getAllUsers.
  2. Add Javadoc comments to describe the method's purpose, parameters, and return value.
  3. Consider using a more specific return type if possible, rather than List<String>.

Here's a suggested revision (assuming it's retrieving a list of users as an example):

/**
 * Retrieves a list of all users based on the specified criteria.
 *
 * @param allListRequest The criteria for retrieving the user list
 * @return A list of user identifiers matching the specified criteria
 */
List<String> getAllUsers(ApiModels.AllList allListRequest);

If the method is intended for a different purpose, please adjust the name and documentation accordingly to clearly reflect its functionality.


List<String> findGoldenIds();

List<String> fetchGoldenIds(
Expand Down
Loading