avivijay19 commented on code in PR #6069:
URL: https://github.com/apache/fineract/pull/6069#discussion_r3633704746
##########
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:
I think that snippet points at a different column. `JournalEntry` extends
`AbstractAuditableWithUTCDateTimeCustom`, where `createdDate` is an
`OffsetDateTime` mapped to `created_on_utc` via
`AuditableFieldsConstants.CREATED_DATE_DB_FIELD`. That column is created as
`DATETIME` (and `TIMESTAMP WITH TIME ZONE` on PostgreSQL) in
`0025_add_audit_entries_to_journal_entry.xml`. The `created_date date` column
you quoted is on `m_trial_balance` in `0001_initial_schema.xml`, so it's the
table we insert into, not `je`.
Since the JPQL projects the entity attribute, the `Object[]` slot holds the
mapped Java type. I didn't catch this by reading the code:
`UPDATE_TRIAL_BALANCE_DETAILS` actually failed during a full PostgreSQL
integration run on this branch. EclipseLink 4 gave back something still
castable to `LocalDate` here, EclipseLink 5 materializes the mapped type.
Happy to reverse it if you're still not convinced, but the job won't
complete without it.
##########
fineract-core/src/main/java/org/apache/fineract/infrastructure/businessdate/service/BusinessDateWritePlatformServiceImpl.java:
##########
@@ -52,6 +54,7 @@ public BusinessDateDTO updateBusinessDate(BusinessDateDTO
businessDateDto) {
}
@Override
+ @Transactional(propagation = Propagation.REQUIRES_NEW)
Review Comment:
Reverted, along with the retry in both tasklets and the unit tests for it.
Some context for the follow-up ticket: `INCREASE_BUSINESS_DATE_BY_1_DAY` and
`INCREASE_COB_DATE_BY_1_DAY` can run at the same time and both insert the
initial `m_business_date` row, so one of them dies on a duplicate key. We hit
it on this branch as a `SchedulerJobsTest` failure on the MySQL shard. It's a
pre-existing race though, not something Spring Boot 4 introduced, so I agree it
doesn't belong in this PR. I'll raise it separately and fix it with an upsert,
which is the better solution anyway.
Could you let me know which JIRA issue I should raise it under, or whether I
should create a new one?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/service/increasedateby1day/increasebusinessdateby1day/IncreaseBusinessDateBy1DayTasklet.java:
##########
@@ -39,7 +41,13 @@ public class IncreaseBusinessDateBy1DayTasklet implements
Tasklet {
@Override
public RepeatStatus execute(StepContribution contribution, ChunkContext
chunkContext) throws Exception {
if (configurationDomainService.isBusinessDateEnabled()) {
-
businessDateWritePlatformService.increaseDateByTypeByOneDay(BusinessDateType.BUSINESS_DATE);
Review Comment:
Same point as on `BusinessDateWritePlatformServiceImpl`: reverted here too,
and the duplicate-key race goes into the follow-up ticket.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/jobs/service/increasedateby1day/increasecobdateby1day/IncreaseCobDateBy1DayTasklet.java:
##########
@@ -39,7 +41,13 @@ public class IncreaseCobDateBy1DayTasklet implements Tasklet
{
@Override
public RepeatStatus execute(StepContribution contribution, ChunkContext
chunkContext) throws Exception {
if (configurationDomainService.isBusinessDateEnabled()) {
-
businessDateWritePlatformService.increaseDateByTypeByOneDay(BusinessDateType.COB_DATE);
+ try {
Review Comment:
Same here, reverted. This is the COB date side of the same race, so it goes
into the follow-up ticket as well.
##########
fineract-provider/src/test/java/org/apache/fineract/infrastructure/jobs/service/increasedateby1day/increasecobdateby1day/IncreaseCobDateBy1DayTaskletTest.java:
##########
@@ -77,4 +80,27 @@ public void
shouldBusinessDateJobBeProcessedWhenBusinessDateIsEnabled() throws E
assertEquals(RepeatStatus.FINISHED, repeatStatus);
}
+ @Test
+ public void shouldRetryOnceWhenConcurrentDateCreationFails() throws
Exception {
Review Comment:
Same here, these went with the retry logic.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/search/SavingsAccountTransactionsSearchServiceImpl.java:
##########
@@ -141,6 +142,7 @@ private static void addFromToFilter(@NonNull String column,
String fromValue, St
}
@Nullable
+ @SuppressFBWarnings("NP_BOOLEAN_RETURN_NULL")
Review Comment:
It's fallout from the JSpecify migration, not a new null return.
`org.springframework.lang.Nullable` was a JSR-305 `@TypeQualifierNickname` for
`@CheckForNull`, which SpotBugs understands and which kept this detector quiet.
`org.jspecify.annotations.Nullable` has no JSR-305 meta-annotation, so SpotBugs
now flags the tri-state `Boolean` that's been in this method all along.
The three states are deliberate (`true` = filter applied, `false` = no
filter, `null` = filter matches nothing) and the caller depends on them, so I
didn't want to change the behavior inside a dependency upgrade. Happy to raise
a separate ticket to replace the tri-state with something properly typed if
you'd like it cleaned up.
--
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]
