You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "b0c1 (via GitHub)" <gi...@apache.org> on 2023/05/04 20:47:47 UTC
[GitHub] [fineract] b0c1 opened a new pull request, #3154: Fineract-1724 - CatchUp Job Custom parameter fix
b0c1 opened a new pull request, #3154:
URL: https://github.com/apache/fineract/pull/3154
- [x] Move from JobParameter to CustomJobParameter
- [x] Improve CustomJobParameterResolver to raise custom exception and work with StepExecution
- [x] Improve tests
## Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update unit or integration tests for verifying the changes made.
- [ ] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ruchiD commented on a diff in pull request #3154: Fineract-1724 - CatchUp Job Custom parameter fix
Posted by "ruchiD (via GitHub)" <gi...@apache.org>.
ruchiD commented on code in PR #3154:
URL: https://github.com/apache/fineract/pull/3154#discussion_r1185762428
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/domain/JobExecutionRepository.java:
##########
@@ -190,35 +188,13 @@ bje.JOB_INSTANCE_ID NOT IN (
AND
bji.JOB_NAME = :jobName
)
- """, Map.of("statuses", List.of(STARTED.name(), STARTING.name()), "jobName", jobName, "completedStatus", COMPLETED.name(),
- "parameterKeyName", parameterKeyName, "parameterValue", parameterValue), Long.class);
- }
-
- public List<Long> getRunningJobsIdsByExecutionParameter(String jobName, String jobCustomParamKeyName, String parameterKeyName,
- String parameterValue) {
- final StringBuilder sqlStatementBuilder = new StringBuilder();
- String jsonString = gson.toJson(new JobParameterDTO(parameterKeyName, parameterValue));
- sqlStatementBuilder.append(
- "SELECT bje.JOB_EXECUTION_ID FROM BATCH_JOB_INSTANCE bji INNER JOIN BATCH_JOB_EXECUTION bje ON bji.JOB_INSTANCE_ID = bje.JOB_INSTANCE_ID INNER JOIN BATCH_JOB_EXECUTION_PARAMS bjep ON bje.JOB_EXECUTION_ID = bjep.JOB_EXECUTION_ID"
- + " WHERE bje.STATUS IN (:statuses) AND bji.JOB_NAME = :jobName AND bjep.KEY_NAME = :jobCustomParamKeyName AND bjep.LONG_VAL IN ("
- + getSubQueryForCustomJobParameters()
- + ") AND bje.JOB_INSTANCE_ID NOT IN (SELECT bje.JOB_INSTANCE_ID FROM BATCH_JOB_INSTANCE bji INNER JOIN BATCH_JOB_EXECUTION bje ON bji.JOB_INSTANCE_ID = bje.JOB_INSTANCE_ID"
- + " WHERE bje.STATUS = :completedStatus AND bji.JOB_NAME = :jobName)");
- return namedParameterJdbcTemplate.queryForList(
- sqlStatementBuilder.toString(), Map.of("statuses", List.of(STARTED.name(), STARTING.name()), "jobName", jobName,
- "completedStatus", COMPLETED.name(), "jobCustomParamKeyName", jobCustomParamKeyName, "jsonString", jsonString),
- Long.class);
- }
-
- private String getSubQueryForCustomJobParameters() {
- if (databaseTypeResolver.isMySQL()) {
- return "SELECT cjp.id FROM batch_custom_job_parameters cjp WHERE JSON_CONTAINS(cjp.parameter_json,:jsonString)";
- } else if (databaseTypeResolver.isPostgreSQL()) {
- return "SELECT cjp.id FROM (SELECT id,json_array_elements(parameter_json) AS json_data FROM batch_custom_job_parameters) AS cjp WHERE (cjp.json_data ::jsonb @> :jsonString ::jsonb)";
- } else {
- throw new IllegalStateException("Database type is not supported for json query " + databaseTypeResolver.databaseType());
- }
-
Review Comment:
We should analyze performance implications of this change of processing json in application code vs native query/json functions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] galovics commented on a diff in pull request #3154: Fineract-1724 - CatchUp Job Custom parameter fix
Posted by "galovics (via GitHub)" <gi...@apache.org>.
galovics commented on code in PR #3154:
URL: https://github.com/apache/fineract/pull/3154#discussion_r1187412071
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/LoanCatchUpSupport.java:
##########
@@ -18,16 +18,15 @@
*/
package org.apache.fineract.cob.loan;
-import org.springframework.batch.core.StepContribution;
+import org.apache.fineract.cob.common.CustomJobParameterResolver;
+import org.apache.fineract.cob.exceptions.CustomJobParameterNotFoundException;
import org.springframework.batch.core.StepExecution;
public interface LoanCatchUpSupport {
- default boolean isCatchUp(StepContribution contribution) {
- return isCatchUp(contribution.getStepExecution());
- }
-
- default boolean isCatchUp(StepExecution execution) {
- return "true".equalsIgnoreCase(execution.getExecutionContext().getString(LoanCOBConstant.IS_CATCH_UP_PARAMETER_NAME, "false"));
+ default boolean isCatchUp(CustomJobParameterResolver resolver, StepExecution execution) {
+ return resolver.getCustomJobParameterSet(execution, CustomJobParameterNotFoundException::new).stream()
+ .filter(parameter -> LoanCOBConstant.IS_CATCH_UP_PARAMETER_NAME.equals(parameter.getParameterName())).findFirst()
+ .map(jobParameter -> "true".equalsIgnoreCase(jobParameter.getParameterValue())).orElse(false);
Review Comment:
I think checking for "true" as a string is not necessarily the best idea. why don't we convert the value to a boolean and then check against the bool true value?
##########
fineract-provider/src/main/java/org/apache/fineract/cob/common/CustomJobParameterResolver.java:
##########
@@ -50,20 +56,45 @@ public void afterPropertiesSet() throws Exception {
public void resolve(StepContribution contribution, ChunkContext chunkContext, String customJobParameterKey,
String parameterNameInExecutionContext) {
- Set<JobParameterDTO> jobParameterDTOList = getCustomJobParameterSet(chunkContext);
+ Set<JobParameterDTO> jobParameterDTOList = getCustomJobParameterSet(chunkContext.getStepContext().getStepExecution(),
+ CustomJobParameterNotFoundException::new);
JobParameterDTO businessDateParameter = jobParameterDTOList.stream()
.filter(jobParameterDTO -> customJobParameterKey.equals(jobParameterDTO.getParameterName())) //
.findFirst().orElseThrow(() -> new CustomJobParameterNotFoundException(customJobParameterKey));
contribution.getStepExecution().getExecutionContext().put(parameterNameInExecutionContext,
businessDateParameter.getParameterValue());
}
- private Set<JobParameterDTO> getCustomJobParameterSet(ChunkContext chunkContext) {
- Long customJobParameterId = (Long) chunkContext.getStepContext().getJobParameters()
- .get(SpringBatchJobConstants.CUSTOM_JOB_PARAMETER_ID_KEY);
+ /**
+ * Get parameter set from custom job parameter table
+ *
+ * @param stepExecution
+ * @param keyNotFoundExceptionResolver
+ * @return
+ */
+ public Set<JobParameterDTO> getCustomJobParameterSet(StepExecution stepExecution,
+ LongFunction<RuntimeException> keyNotFoundExceptionResolver) {
Review Comment:
I think with passing the exception function, you're actually hiding a very important line between layers.
If a custom job parameter is not found, the appropriate exception shall be thrown. In other layers where this resolver is used, you could of course catch this CustomJobParameterNotFound exception and transform it to the layer's appropriate one (for example I saw a LoanNotFoundException somewhere).
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/LoanCatchUpSupport.java:
##########
@@ -18,16 +18,15 @@
*/
package org.apache.fineract.cob.loan;
-import org.springframework.batch.core.StepContribution;
+import org.apache.fineract.cob.common.CustomJobParameterResolver;
+import org.apache.fineract.cob.exceptions.CustomJobParameterNotFoundException;
import org.springframework.batch.core.StepExecution;
public interface LoanCatchUpSupport {
Review Comment:
Btw why interface and why don't we use a simple bean instead of this? That way we don't even need to pass the resolver as a param.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] galovics merged pull request #3154: Fineract-1724 - CatchUp Job Custom parameter fix
Posted by "galovics (via GitHub)" <gi...@apache.org>.
galovics merged PR #3154:
URL: https://github.com/apache/fineract/pull/3154
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org