alberto-art3ch commented on PR #5951:
URL: https://github.com/apache/fineract/pull/5951#issuecomment-4931041171
> **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 wha
t 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 p
ersistence 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.
Most of these were already addressed in the latest push:
- Duplication: Savings now extends the shared fineract-cob bases rather than
copying them — SavingsCOBPartitioner extends CommonPartitioner,
ChunkProcessingSavingsItemListener extends AbstractItemListener,
ApplySavingsLockTasklet extends ApplyCommonLockTasklet,
SavingsLockingServiceImpl extends AbstractLockingService, SavingsItemReader
extends AbstractAccountItemReader. This push also generalized the loan-specific
bases (AbstractLoanItemReader→AbstractAccountItemReader,
AbstractLoanItemListener→AbstractItemListener) so all account types share one
path.
- Orphaned-lock guard: Fixed — removeOrphanedLocksForProcessedAccounts now
carries the same EXISTS (… lastClosedBusinessDate =
lockPlacedOnCobBusinessDate) guard as the loan/WC query.
- Bulk deletes: Fixed — both @Modifying deletes now use clearAutomatically =
true, matching the loan/WC repositories.
- Typed exception: Fixed — the reader now throws
SavingsAccountNotFoundException instead of a bare RuntimeException.
- Inline COB: The inline job (INLINE_SAVINGS_COB) and its lock owner are
wired and invocable, but there is no write-API servlet filter (no
SavingsCOBApiFilter) yet — I've updated savings-cob.adoc with a NOTE making
that explicit so it isn't assumed to auto-trigger on writes.
--
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]
