Re: [PR] FINERACT-2332: Savings COB first business steps [fineract]

["adamsaghy (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org