FINERACT-2593: when loan product is null

[mansi75]

Description

JIRA: https://issues.apache.org/jira/browse/FINERACT-2593

Problem

DelinquencyReadPlatformServiceImpl.calculateLoanCollectionData contained two bugs that caused an unhandled NullPointerException (HTTP 500) when loan.getLoanProduct() returns null:

Bug 1 — Inverted null guard (pre-approval / pending state branch)
The condition loan.getLoanProduct() == null || used the wrong operator. When getLoanProduct() returns null, the left-hand operand evaluates to true, short-circuiting the || and routing execution directly into calculateAvailableDisbursementAmountWithOverApplied(loan). The helper immediately dereferences loanProduct with no null check, producing an NPE. The intended operator is != null &&.

Bug 2 — No null guard on active-loan branch
The active (open) loan branch called calculateAvailableDisbursementAmountWithOverApplied(loan) unconditionally with no null check on getLoanProduct() at all, giving a second independent crash surface from the same root cause.

Fix

• Changed == null || to != null && in the pre-approval branch so the helper is only invoked when a LoanProduct is present and over-apply is enabled.
• Wrapped the active-loan branch call with a loan.getLoanProduct() != null guard.
• Added four missing @Mock field declarations in DelinquencyReadPlatformServiceImplTest (ConfigurationDomainService, LoanTransactionRepository, PossibleNextRepaymentCalculationServiceDiscovery, LoanDelinquencyActionRepository) that were causing silent Mockito injection failure on any new tests written for this path.
• Added unit tests covering both crash surfaces to prevent regression.

Files Changed

File	Change
DelinquencyReadPlatformServiceImpl.java	Fix inverted operator + add missing null guard
DelinquencyReadPlatformServiceImplTest.java	Add 4 missing @Mock declarations + new regression tests

---

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes — Not applicable, no API contract changes. This fix corrects internal null handling only.
• This PR must not be a "code dump". This is a minimal targeted fix — 2 lines changed in the implementation, 4 mock declarations and regression tests added.

[Copilot]

Pull request overview

Fixes FINERACT-2593 by preventing NullPointerException in delinquency collection calculations when loan.getLoanProduct() returns null, and updates the unit test setup to improve Mockito injection for added dependencies.

Changes:

• Fix inverted null guard in the pre-approval/pending/cancelled branch.
• Add a null guard before computing availableDisbursementAmountWithOverApplied on the active-loan branch.
• Add missing @Mock fields in DelinquencyReadPlatformServiceImplTest to support constructor injection.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
fineract-loan/src/main/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyReadPlatformServiceImpl.java	Corrects null-check logic and prevents NPE when loan product is missing.
fineract-provider/src/test/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyReadPlatformServiceImplTest.java	Adds mocks for newly required constructor dependencies (but still needs one more mock for reliable injection) and has minor formatting issues.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[AshharAhmadKhan]

Good work @mansi75 ! The core fixes on lines 146 and 163 are correct and address the two most visible crash surfaces.

Two things worth addressing before merge:

1. Helper still has no null guard (line 202)

calculateAvailableDisbursementAmountWithOverApplied dereferences loanProduct without a null check. This matters because LoanReadPlatformServiceImpl calls it directly at lines 648 and 663 with no guard at either call site — verified on develop. Those two surfaces are still live crashes.

Fix:
```java
// before
if (loanProduct.isAllowApprovedDisbursedAmountsOverApplied()) {
// after
if (loanProduct != null && loanProduct.isAllowApprovedDisbursedAmountsOverApplied()) {
```

2. Tests assert null but don't cover the helper directly

Asserting getAvailableDisbursementAmountWithOverApplied() is null only confirms the field was never set. Since the helper is a public interface method and callable independently, it needs direct coverage — null product path, flag false, and the percentage calculation path.

[San-43]

Hi @mansi75, you may want to update the PR title to use uppercase FINERACT-. I believe Jira only references the PR correctly when the issue key is uppercase. I ran into the same issue recently, where the PR was not linked to the Jira ticket because the key was not uppercase.

[Aman-Mittal]

@mansi75 Please review failing e2e test cases

Then Loan has availableDisbursementAmountWithOverApplied field with value: 150 # org.apache.fineract.test.stepdef.loan.LoanStepDef.checkLoanDetailsAvailableDisbursementAmountWithOverAppliedField(double)
org.opentest4j.AssertionFailedError: [Wrong value in LoanDetails/availableDisbursementAmountWithOverApplied.
Actual value is: 0.0
Expected Value is: 150.0]
expected: 150.0
but was: 0.0
at org.apache.fineract.test.stepdef.loan.LoanStepDef.checkLoanDetailsAvailableDisbursementAmountWithOverAppliedField(LoanStepDef.java:3484)
at ✽.Loan has availableDisbursementAmountWithOverApplied field with value: 150(file:///home/runner/work/fineract/fineract/fineract-e2e-tests-runner/src/test/resources/features/LoanUpdateAvailableDisbursementAmount.feature:280)

[Execute tests for shard 10](https://github.com/apache/fineract/actions/runs/25077326923/job/73538174734#annotation:12:16062)

Test failed for src/test/resources/features/LoanUpdateApprovedAmount.feature

Execute tests for shard 9
Test failed for src/test/resources/features/LoanCapitalizedIncome-Part2.feature

[mansi75]

@San-43 Thank you for informing. I will update accordingly.

[adamsaghy]

@mansi75 Please review the failing test cases!

[adamsaghy]

@mansi75
Please review the failing checks:

Please run:
./gradlew spotlessApply spotbugsMain spotbugsTest checkstyleMain checkstyleTest
./gradlew --no-daemon build -x test -x cucumber -x doc

Before any PR or changes, please always run these two commands and make sure there is green build!

[mansi75]

@adamsaghy Got it! I am working on the failing tests

[Aman-Mittal]

Can you please explain when loanProduct is required in calculateAvailableDisbursementAmountWithOverApplied function ? When loanProduct is not present what part of loan product it utilize when calculating AvailableDisburementAmountWithOverApplied

[Aman-Mittal]

Based on Adam's review I have one more concern. If loan product is non nullable and giving NPE shouldn't we use fail fast approach in this? As you have applied the npe handling in calculation wouldn't it cause logical issues? Even through all tests are passing.

[Aman-Mittal]

Seems ok to merge what i can see it Logic was fixed where || is causing problem but curious about AvailableDisbursementAmountWithOverapplied Logic

[adamsaghy]

@mansi75 I dont see a use case where loan product is null. Can you please give me an example?

[mansi75]

@adamsaghy Correct me if I got it wrong. In the codebase, getLoanProduct() should never return null because loanProduct is a mandatory @manytoone relationship on the Loan entity but if a Loan object is loaded in one Hibernate session and then getLoanProduct() is called outside that session, hibernate will return null as it can't initialize the proxy

[adamsaghy]

@adamsaghy Correct me if I got it wrong. In the codebase, getLoanProduct() should never return null because loanProduct is a mandatory @manytoone relationship on the Loan entity but if a Loan object is loaded in one Hibernate session and then getLoanProduct() is called outside that session, hibernate will return null as it can't initialize the proxy

Technically maybe but:

• loan product on loan is eagerly loaded (if i remember correctly)
• all write operations are inside transaction boundary way: no such thing loan is loaded and txn ended but still there are write / read operations.

How did you come to the conclusion this is an issue?

Have you noticed NPE somewhere?

[mansi75]

@adamsaghy Thank you for the correction. After checking the actual Loan.java source:

@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "product_id", nullable = false)
private LoanProduct loanProduct;

loanProduct IS lazily loaded but it cannot be null in practice since nullable = false and DelinquencyReadPlatformServiceImpl is annotated @transactional, so it is open for entire method call. So I agree there is no real-world scenario where getLoanProduct() returns null and I have not noticed NPE anywhere. I will revert the unnecessary null guards and keep only the single operator fix.
Sorry for the confusion

[AshharAhmadKhan]

LGTM

[Aman-Mittal]

Please address my concern.