adamsaghy commented on PR #5951:
URL: https://github.com/apache/fineract/pull/5951#issuecomment-4874430338
> > @alberto-art3ch Please consider the below recommendations:
> > ### PR #5951 (Savings COB) — does it match Loan COB?
> > Short answer: **no, not in the way you want.** The Spring Batch _step
topology_ is a faithful port, but the PR does **not**reuse the generic COB
framework that Loan COB already sits on — it re-introduces monolithic,
copy‑pasted implementations under `Savings*` names. On top of that, several
pieces are missing or wrong enough that the job can't compile, start, or
actually do anything.
> > #### The core problem: it duplicates instead of reusing the existing
generic layer
> > Loan COB was already refactored into entity‑agnostic bases in the
**`fineract-cob`** module, and the Loan classes are thin (30–60 line)
subclasses. The PR ignored all of them and re-implemented the pre‑refactor
monoliths. Verified mappings:
> > PR's new Savings class | Existing generic base it should extend (in
fineract-cob) | What Loan does -- | -- | -- SavingsCOBPartitioner (~107 lines)
| cob.common.CommonPartitioner | LoanCOBPartitioner extends CommonPartitioner
(58 lines) SavingsLockingServiceImpl (~117 lines) |
cob.domain.AbstractLockingService | LoanLockingServiceImpl extends
AbstractLockingService (57 lines) ApplySavingsLockTasklet (~108 lines) |
cob.tasklet.ApplyCommonLockTasklet | ApplyLoanLockTasklet extends
ApplyCommonLockTasklet (47 lines) UnlockProcessedSavingsTasklet |
cob.tasklet.UnlockProcessedAccountsTasklet+ AccountLockService |
UnlockProcessedLoansTasklet extends UnlockProcessedAccountsTasklet<>
ChunkProcessingSavingsItemListener(~103 lines) |
cob.listener.AbstractLoanItemListener(already generic) |
ChunkProcessingLoanItemListener extends AbstractLoanItemListener (39 lines)
SavingsItemReader | cob.service.BeforeStepLockingItemReaderHelper |
LoanItemReader delegates to it (44 lines) new S
avingsLockOwner enum | existing cob.domain.LockOwner | reuses LockOwner
> > The uniform approach you're after already exists. Savings COB should
shrink to ~6 thin subclasses + supply entity-specific
table/SQL/lock-owner/parameter keys, exactly like Loan and Working‑Capital‑Loan
do. Where a base is "Loan"‑named but already generic
(`AbstractLoanItemListener<T>`, `AbstractLoanItemReader<T>`), rename it to a
neutral `Abstract*`/`Common*` as part of this PR.
> > #### Verified findings, most severe first
> >
> > 1. **`UnlockProcessedSavingsTasklet:44` won't compile** — calls
`savingsAccountLockRepository.removeOrphanedLocksForProcessedAccounts()`, but
that method does not exist on `SavingsAccountLockRepository` (it only has
`removeLockByOwner()` / `existsBy…`). The generic
`AccountLockService.removeOrphanedLocksForProcessedAccounts()` that Loan uses
_does_ exist — reuse it. (Note `removeLockByOwner` deletes rows where `error IS
NOT NULL` — the opposite of orphan cleanup.)
> > 2. **Lock table is never created** — `SavingsAccountLock` maps to
`@Table(name = "m_savings_account_locks")`, but no Liquibase changelog creates
that table (`grep` finds none; Loan ships `0039_add_loan_account_locks.xml`).
First `applyLock`→ SQL error on every tenant.
> > 3. **`batchJdbcTransactionTemplate` bean is undefined** — injected with
`@Qualifier("batchJdbcTransactionTemplate")` in
`SavingsCOBWorkerConfiguration`, `ApplySavingsLockTasklet:54`,
`ChunkProcessingSavingsItemListener`, but no such bean exists anywhere. Loan
uses the existing `requiresNewTransactionJdbcTemplate`
(`JdbcTransactionConfig:51`, `PROPAGATION_REQUIRES_NEW`). →
`NoSuchBeanDefinitionException` at startup.
> > 4. **Retry contract broken in `ApplySavingsLockTasklet`** — gates
retries on `contribution.getStepExecution().getCommitCount()`(diff line 729)
instead of the persisted attempt counter `ApplyCommonLockTasklet` uses
(`executionContext.getLong(getApplyLockAttemptsKey())`, lines 88–99).
`getCommitCount()` is committed-chunk count, not failed-lock attempts, so `> 3`
is typically never true → tasklet returns `CONTINUABLE` and can spin instead of
failing after 3 attempts. Disappears entirely if you extend the base.
> > 5. **Stayed-locked detection is a no-op** —
`RetrieveSavingsIdServiceImpl.findAllStayedLockedByCobBusinessDate` returns
`List.of()` with comment _"implemented when we add the query… lock table
doesn't exist yet"_. So `StayedLockedSavingsTasklet` can never emit
`SavingsAccountsStayedLockedBusinessEvent`.
> > 6. **Job has no business steps → stops immediately** — there is zero
`implements SavingsCOBBusinessStep` and no `m_batch_business_steps` seed for
`SAVINGS_CLOSE_OF_BUSINESS`. `SavingsCOBPartitioner` calls `stopJobExecution()`
when steps are empty, so every run stops without processing anything.
> > 7. **No `job` row → never scheduled** — no migration inserts a "Savings
COB" `ScheduledJobDetail` (Loan ships `0037_add_loan_cob_job_data.xml`). The
bean exists but the scheduler never triggers it and it's invisible in the
Scheduler Jobs API.
> > 8. **`PartitionedJob` enum not extended** — still only `LOAN_COB`. A
stuck `SAVINGS_COB` execution fails `existsByJobName`, so
`StuckJobExecutorServiceImpl` treats it as a plain tasklet job — stuck-job
recovery diverges from Loan.
> >
> > Also worth fixing:
`ApplySavingsLockTasklet`/`ChunkProcessingSavingsItemListener` mutate a shared
`TransactionTemplate` via `setPropagationBehavior(...)` per call (diff 450,
766) under the multi-threaded worker step — a data race the Loan path avoids by
using a pre-configured bean; and `AbstractSavingsItemReader` throws raw
`RuntimeException` instead of a typed not-found exception (loses meaning in the
recorded lock stacktrace).
> > #### Recommendation
> > Reframe the PR around the existing `fineract-cob` generics rather than
parallel copies: extend `CommonPartitioner`, `AbstractLockingService`,
`ApplyCommonLockTasklet`, `UnlockProcessedAccountsTasklet<T>`,
`AbstractLoanItemListener<T>` (rename it), `BeforeStepLockingItemReaderHelper`,
and reuse `LockOwner`/`AccountLockService`. Then add the missing artifacts Loan
ships as standard — the lock-table migration, the `job` row, the business-step
seed, at least one `SavingsCOBBusinessStep` impl, and the `PartitionedJob` enum
entry. That gets you the uniform, single-maintenance-point COB you're
describing, and every future COB fix applies to both account types at once.
>
> Thanks @adamsaghy for the detailed review. We are applying the next updates
>
> **Reuse over duplication** (the main point): Savings COB now sits on the
generic fineract-cob framework instead of re-implementing the pre-refactor
monoliths. _SavingsCOBPartitioner_, _SavingsLockingServiceImpl_,
_ApplySavingsLockTasklet_, _ChunkProcessingSavingsItemListener_ and the readers
are now thin subclasses of _CommonPartitioner_, _AbstractLockingService_,
_ApplyCommonLockTasklet_, _AbstractItemListener_ (renamed from
_AbstractLoanItemListener_) and _AbstractAccountItemReader_ (extracted from
_AbstractLoanItemReader_). _SavingsLockOwner_/_SavingsLockingService_ were
dropped in favor of the shared _LockOwner_/_LockingService_. Net ~-110 LOC and
a single maintenance point for both account types.
>
> **Bugs fixed** for free by the reuse: the retry contract (now uses the
persisted ExecutionContext attempt counter, not getCommitCount()), the
shared-TransactionTemplate data race (pre-configured
requiresNewTransactionJdbcTemplate), the typed not-found exception in the
reader, and PartitionedJob.SAVINGS_COB so stuck-job recovery matches Loan.
>
> **One deliberate exception**: _UnlockProcessedSavingsTasklet_ still uses
the repository directly rather than _UnlockProcessedAccountsTasklet_ — folding
_SavingsAccountLock_ into the loan_id-keyed _AccountLock_ hierarchy is a
larger, riskier entity refactor I'd prefer to do separately.
**Main architectural concern**
The PR re-implements the COB framework standalone for savings instead of
reusing the abstractions the codebase already generalized. fineract-cob already
provides shared bases — AbstractAccountLockService / AccountLockService /
UnlockProcessedAccountsTasklet, CommonPartitioner, AbstractLoanItemListener,
ApplyCommonLockTasklet — and there are already two consumers of them: loan and
working-capital-loan (WorkingCapitalAccountLockRepository).
Working-capital-loan is the template for "add a new account type the right way."
Savings instead hand-writes parallel copies: SavingsLockingServiceImpl,
UnlockProcessedSavingsTasklet, SavingsCOBPartitioner (dup of
CommonPartitioner), AbstractSavingsItemReader (dup of AbstractLoanItemReader),
ChunkProcessingSavingsItemListener (dup of AbstractLoanItemListener),
ApplySavingsLockTasklet (dup of ApplyCommonLockTasklet). Every future bug fix
or account type now has to be applied in N copies — which is exactly what went
wrong below.
**Major**
fineract-savings/.../cob/domain/SavingsAccountLockRepository.java:58 —
orphaned-lock cleanup drops the "was actually processed" guard. Savings'
removeOrphanedLocksForProcessedAccounts() deletes every chunk-processing lock
where error is null. The loan/WC shared query
(deleteOrphanedLocksForProcessedAccounts) additionally requires EXISTS (SELECT
l ... WHERE l.lastClosedBusinessDate = lck.lockPlacedOnCobBusinessDate).
Failure scenario: an account is locked but never advanced and no error gets
recorded on its lock (e.g. a rollback path where the error-recording
REQUIRES_NEW write also doesn't land) → this query silently unlocks it as if
COB completed, and it is not reported as stayed-locked. The loan EXISTS check
exists precisely to prevent this. This is a direct consequence of not reusing
the shared query.
SavingsAccountLockRepository.java:48,57 — bulk deletes use
@Modifying(flushAutomatically = true) where loan uses clearAutomatically =
true. After a bulk JPQL delete, the persistence context still holds the
now-deleted SavingsAccountLock entities; loan clears them. Low runtime impact
here (last job step), but it's a latent staleness bug and an unnecessary
divergence.
fineract-provider/.../cob/savings/AbstractSavingsItemReader.java:50 — throws
a bare RuntimeException("SavingsAccount not found: ...") instead of the typed
SavingsAccountNotFoundException (loan uses LoanNotFoundException). It's wrapped
in LockedReadException regardless, so low severity, but it loses typed handling
and consistency.
**Minor**
The SAVINGS_INLINE_COB_PROCESSING lock owner, removeLockByOwner(), and the
doc's references to an "inline catch-up COB triggered by a write API" describe
an inline path that isn't implemented in this PR — harmless scaffolding, but
worth a note so reviewers don't assume inline COB works yet. (The rest of
savings-cob.adoc is accurate — it correctly states only
POST_INTEREST_FOR_SAVINGS ships.)
Recommendation
Refactor to extend the existing shared fineract-cob abstractions the way
working-capital-loan does, rather than duplicating them. If the standalone
approach must stay for now, at minimum port the EXISTS(lastClosedBusinessDate =
lockPlacedOnCobBusinessDate) guard into the orphaned-lock delete and switch the
bulk deletes to clearAutomatically = true.
--
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]
