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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
