[GitHub] [fineract] galovics commented on a diff in pull request #3108: FINERACT-1724-Regular-loan-cob-job-having-catch-up-true

["galovics (via GitHub)" <gi...@apache.org>]

galovics commented on code in PR #3108:
URL: https://github.com/apache/fineract/pull/3108#discussion_r1162376333


##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/domain/CustomJobParameterRepository.java:
##########
@@ -20,4 +20,4 @@
 
 import org.springframework.data.jpa.repository.JpaRepository;
 
-public interface CustomJobParameterRepository extends JpaRepository<CustomJobParameter, Long> {}
+public interface CustomJobParameterRepository extends JpaRepository<CustomJobParameter, Long>, JobCustomParameterRepository {}

Review Comment:
   I think we should get rid of the JpaRepository interface here completely cause it's not working properly (the save method for example, since that's why you have the custom repository implementation).



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/database/DatabaseSpecificSQLGenerator.java:
##########
@@ -168,4 +168,14 @@ public String currentSchema() {
             throw new IllegalStateException("Database type is not supported for current schema " + databaseTypeResolver.databaseType());
         }
     }
+
+    public String jsonAppender() {
+        if (databaseTypeResolver.isMySQL()) {
+            return "";
+        } else if (databaseTypeResolver.isPostgreSQL()) {
+            return " ::json";
+        } else {
+            throw new IllegalStateException("Database type is not supported for current schema " + databaseTypeResolver.databaseType());

Review Comment:
   Copy paste typo on the error msg.



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/domain/JobCustomParameterRepository.java:
##########
@@ -0,0 +1,24 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.jobs.domain;
+
+public interface JobCustomParameterRepository {
+
+    Long saveCustomJobParameter(String jsonString);

Review Comment:
   Should be called save simply (after removing the JpaRepository)



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/domain/JobCustomParameterRepositoryImpl.java:
##########
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.jobs.domain;
+
+import lombok.RequiredArgsConstructor;
+import org.apache.fineract.infrastructure.core.service.database.DatabaseSpecificSQLGenerator;
+import org.springframework.jdbc.core.namedparam.MapSqlParameterSource;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
+import org.springframework.jdbc.core.namedparam.SqlParameterSource;
+import org.springframework.stereotype.Component;
+
+@RequiredArgsConstructor
+@Component
+public class JobCustomParameterRepositoryImpl implements JobCustomParameterRepository {
+
+    private final NamedParameterJdbcTemplate namedParameterJdbcTemplate;
+    private final DatabaseSpecificSQLGenerator databaseSpecificSQLGenerator;
+
+    @Override
+    public Long saveCustomJobParameter(String jsonString) {
+        final StringBuilder insertSqlStatementBuilder = new StringBuilder(500);
+        insertSqlStatementBuilder.append("INSERT INTO batch_custom_job_parameters (parameter_json) VALUES (:jsonString"
+                + databaseSpecificSQLGenerator.jsonAppender() + ")");

Review Comment:
   This would look much better with the approach I mentioned in the generator class (like castChar).



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/database/DatabaseSpecificSQLGenerator.java:
##########
@@ -168,4 +168,14 @@ public String currentSchema() {
             throw new IllegalStateException("Database type is not supported for current schema " + databaseTypeResolver.databaseType());
         }
     }
+
+    public String jsonAppender() {

Review Comment:
   I think based on the castChar method, this should be castJson.



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/domain/JobCustomParameterRepositoryImpl.java:
##########
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.jobs.domain;
+
+import lombok.RequiredArgsConstructor;
+import org.apache.fineract.infrastructure.core.service.database.DatabaseSpecificSQLGenerator;
+import org.springframework.jdbc.core.namedparam.MapSqlParameterSource;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
+import org.springframework.jdbc.core.namedparam.SqlParameterSource;
+import org.springframework.stereotype.Component;
+
+@RequiredArgsConstructor
+@Component
+public class JobCustomParameterRepositoryImpl implements JobCustomParameterRepository {
+
+    private final NamedParameterJdbcTemplate namedParameterJdbcTemplate;
+    private final DatabaseSpecificSQLGenerator databaseSpecificSQLGenerator;
+
+    @Override
+    public Long saveCustomJobParameter(String jsonString) {
+        final StringBuilder insertSqlStatementBuilder = new StringBuilder(500);
+        insertSqlStatementBuilder.append("INSERT INTO batch_custom_job_parameters (parameter_json) VALUES (:jsonString"
+                + databaseSpecificSQLGenerator.jsonAppender() + ")");
+        SqlParameterSource parameters = new MapSqlParameterSource("jsonString", jsonString);
+        namedParameterJdbcTemplate.update(insertSqlStatementBuilder.toString(), parameters);
+        final Long customParameterId = namedParameterJdbcTemplate.getJdbcTemplate()
+                .queryForObject("SELECT " + databaseSpecificSQLGenerator.lastInsertId(), Long.class);

Review Comment:
   Formatting here pls. Bonus point if you put this into a constant so this is cached. (note: not necessarily a constant into this class but somewhere else where anybody can reuse it)



##########
fineract-provider/src/main/resources/db/changelog/tenant/parts/0103_modify_parameter_json_column_custom_job_parameters.xml:
##########
@@ -0,0 +1,30 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+    Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements. See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership. The ASF licenses this file
+    to you under the Apache License, Version 2.0 (the
+    "License"); you may not use this file except in compliance
+    with the License. You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+    Unless required by applicable law or agreed to in writing,
+    software distributed under the License is distributed on an
+    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+    KIND, either express or implied. See the License for the
+    specific language governing permissions and limitations
+    under the License.
+
+-->
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+                   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+                   xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.1.xsd">
+    <changeSet id="1" author="fineract">
+        <modifyDataType columnName="parameter_json"

Review Comment:
   Does this modification work if you already had string values in the table?



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/domain/JobExecutionRepository.java:
##########
@@ -180,4 +193,32 @@ bje.JOB_INSTANCE_ID NOT IN (
                 """, 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 current schema " + databaseTypeResolver.databaseType());

Review Comment:
   Error msg.



##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/domain/JobCustomParameterRepositoryImpl.java:
##########
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.jobs.domain;
+
+import lombok.RequiredArgsConstructor;
+import org.apache.fineract.infrastructure.core.service.database.DatabaseSpecificSQLGenerator;
+import org.springframework.jdbc.core.namedparam.MapSqlParameterSource;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
+import org.springframework.jdbc.core.namedparam.SqlParameterSource;
+import org.springframework.stereotype.Component;
+
+@RequiredArgsConstructor
+@Component
+public class JobCustomParameterRepositoryImpl implements JobCustomParameterRepository {
+
+    private final NamedParameterJdbcTemplate namedParameterJdbcTemplate;
+    private final DatabaseSpecificSQLGenerator databaseSpecificSQLGenerator;
+
+    @Override
+    public Long saveCustomJobParameter(String jsonString) {
+        final StringBuilder insertSqlStatementBuilder = new StringBuilder(500);
+        insertSqlStatementBuilder.append("INSERT INTO batch_custom_job_parameters (parameter_json) VALUES (:jsonString"

Review Comment:
   Let's try to use formatting, makes it much easier to read.
   ```
   "INSERT INTO ... VALUES (%s)".formatted(generator.castJson(":jsonString"))
   ```



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