avivijay19 commented on code in PR #6069:
URL: https://github.com/apache/fineract/pull/6069#discussion_r3633704469
##########
fineract-provider/src/test/java/org/apache/fineract/cob/loan/LoanCOBPartitionerTest.java:
##########
Review Comment:
It is no longer required. `JobOperator.stop(JobExecution)` declares only
`JobExecutionNotRunningException`; `NoSuchJobExecutionException` is declared by
the `long` overload, which this test no longer exercises. I have removed it
from the `throws` clause together with the import.
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/glaccount/jobs/updatetrialbalancedetails/UpdateTrialBalanceDetailsTasklet.java:
##########
@@ -77,7 +78,9 @@ private void insertTrialBalanceForDate(LocalDate tbGap) {
tb.setGlAccountId((Long) row[1]);
tb.setAmount((BigDecimal) row[2]);
tb.setEntryDate((LocalDate) row[3]);
- tb.setTransactionDate((LocalDate) row[4]);
+ // je.createdDate is the OffsetDateTime audit field; EclipseLink 5
materializes it as such in
+ // Object[] projections where EclipseLink 4 handed back a
date-castable value
+ tb.setTransactionDate(((OffsetDateTime) row[4]).toLocalDate());
Review Comment:
The column in the quoted changelog snippet is not the one this projection
reads.
`JournalEntry` extends `AbstractAuditableWithUTCDateTimeCustom`, in which
`createdDate` is declared as `OffsetDateTime` and mapped to the
`created_on_utc` column through
`AuditableFieldsConstants.CREATED_DATE_DB_FIELD`. That column is created as
`DATETIME` and, for PostgreSQL, `TIMESTAMP WITH TIME ZONE` in
`0025_add_audit_entries_to_journal_entry.xml`. The `created_date` column of
type `date` quoted above belongs to `m_trial_balance` in
`0001_initial_schema.xml`, which is the target table of this insert rather than
the source entity.
Because the JPQL projects the entity attribute `je.createdDate`, the
corresponding `Object[]` element carries the mapped Java type. This was
identified as an actual runtime failure rather than by inspection: the
`UPDATE_TRIAL_BALANCE_DETAILS` job failed during a full PostgreSQL integration
run on this branch. EclipseLink 4 returned a value for this element that
remained castable to `LocalDate`, whereas EclipseLink 5 materializes the mapped
attribute type.
Please advise if you would still prefer the change reversed, though the job
does not complete without it.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/filter/LoanCOBFilterHelperImpl.java:
##########
@@ -191,12 +188,14 @@ private List<Long>
getLoanIdsFromBatchApi(BodyCachingHttpServletRequestWrapper r
return loanIds;
}
- private Long getTopLevelLoanIdFromBatchRequest(BatchRequest batchRequest)
throws JsonProcessingException {
+ private Long getTopLevelLoanIdFromBatchRequest(BatchRequest batchRequest) {
String body = batchRequest.getBody();
if (StringUtils.isNotBlank(body)) {
JsonNode jsonNode = objectMapper.readTree(body);
if (jsonNode.has("loanId")) {
- return jsonNode.get("loanId").asLong();
Review Comment:
Unresolved references can occur at this point. This code executes in a
servlet filter against the raw `/batches` request body, before any resolution
performed by the batch layer, so a chained request still contains the literal
placeholder `"loanId": "$.loanId"`.
This was observed as a reproducible failure on this branch. Jackson 3
changed the no-argument `asLong()` to be strict, throwing `JsonNodeException`
for non-coercible values where Jackson 2 returned `0`. The exception was raised
inside the filter, before Jersey and before the batch error handling, so the
client received a container error page with HTTP 500 and no corresponding entry
in the application log. `BatchApiTest` returned to 40/40 passing once
`asLong(0)` was applied.
I agree that a `$.loanId` placeholder does not represent a valid loan
identifier. The pre-existing contract at this point is that no top-level loan
id is present and COB routing is skipped for that request, which `0` satisfies
since it matches no loan. If you would prefer this expressed explicitly, I can
replace it with a `canConvertToLong()` check returning `null`.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanAccrualsProcessingServiceImpl.java:
##########
@@ -205,6 +205,11 @@ public void addAccruals(@NonNull LocalDate tillDate)
throws JobExecutionExceptio
*/
@Override
public void reprocessExistingAccruals(@NonNull final Loan loan, final
boolean addEvent) {
+ if (loan.getId() == null) {
Review Comment:
There is one such scenario. `LoanAssemblerImpl.assembleFrom(..)` constructs
the loan through `Loan.newIndividualLoanApplication(..)` or
`Loan.newGroupLoanApplication(..)` and invokes
`reprocessExistingAccruals(loanApplication, false)` before returning, while the
caller in `LoanApplicationWritePlatformServiceJpaRepositoryImpl` performs
`saveAndFlush(loan)` only afterwards, annotated in the existing code as "Need
to flush to gather loan id". The entity is therefore still transient on the
loan application creation path.
This was previously harmless because `retrieveListOfAccrualTransactions`
simply returned an empty list. Under EclipseLink 5 the transient entity is
passed to `findNonReversedByLoanAndTypes`, the null identifier is bound as
VARCHAR, and PostgreSQL rejects it against the bigint foreign key column.
If you would prefer this handled elsewhere, for example by not invoking the
method from the assembler at all, I can make that change instead, but some form
of guard is required.
##########
fineract-provider/src/test/java/org/apache/fineract/cob/loan/LoanItemReaderStepDefinitions.java:
##########
@@ -68,7 +70,7 @@ public class LoanItemReaderStepDefinitions implements En {
public LoanItemReaderStepDefinitions() {
Given("/^The LoanItemReader.read method with loanIds (.*)$/", (String
loanIds) -> {
- JobExecution jobExecution = new JobExecution(1L);
+ JobExecution jobExecution = new JobExecution(1L, new
JobInstance(1L, "test"), new JobParameters());
ExecutionContext jobExecutionContext = new ExecutionContext();
jobExecution.setExecutionContext(jobExecutionContext);
StepExecution stepExecution = new StepExecution("test",
jobExecution);
Review Comment:
The deprecated constructor is the one that has been replaced here. `new
JobExecution(1L)` no longer exists in Spring Batch 6, and `JobExecution(long
id, JobInstance jobInstance, JobParameters jobParameters)` is the only
remaining public constructor; its Javadoc describes it as "the only valid one
from a modeling point of view". It is not deprecated.
Please let me know if you were referring to a different aspect of this
snippet.
##########
fineract-core/src/main/java/org/springframework/batch/core/scope/context/JobSynchronizationManager.java:
##########
@@ -53,8 +53,8 @@ public static JobContext register(JobExecution jobExecution) {
Enhancer enhancer = new Enhancer();
enhancer.setSuperclass(JobExecution.class);
enhancer.setCallback(new
TenantAwareEqualsHashCodeAdvice(jobExecution));
- return manager.register((JobExecution) enhancer.create(new Class[] {
JobInstance.class, Long.class, JobParameters.class },
- new Object[] { jobExecution.getJobInstance(),
jobExecution.getId(), jobExecution.getJobParameters() }));
+ return manager.register((JobExecution) enhancer.create(new Class[] {
long.class, JobInstance.class, JobParameters.class },
Review Comment:
The change is required by the constructor signature rather than being a
matter of preference. Spring Batch 5 declared `JobExecution(JobInstance, Long,
JobParameters)`. Spring Batch 6 declares a single public constructor,
`JobExecution(long id, JobInstance jobInstance, JobParameters jobParameters)`,
in which the identifier moved to the first position and became a primitive.
`Enhancer.create` resolves the constructor by exact signature, so both the
`Class[]` array and the argument array required reordering to match.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/service/retainedearning/listener/RetainedEarningJobListener.java:
##########
@@ -45,7 +45,7 @@ public class RetainedEarningJobListener implements
JobExecutionListener {
*/
@Override
public void beforeJob(JobExecution jobExecution) {
- log.info("Starting Retained Earning Job: {}", jobExecution.getJobId());
+ log.info("Starting Retained Earning Job: {}",
jobExecution.getJobInstanceId());
Review Comment:
They resolve to the same value. In Spring Batch 5, `JobExecution.getJobId()`
was implemented as `return jobInstance.getId()`. Spring Batch 6 removed
`getJobId()`, and `getJobInstanceId()` is its replacement, so the logged
identifier is unchanged.
--
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]
