FINERACT-2665: Fix NPE modifying a loan application when schedule fields are omitted

[oluexpert99]

• On modify, LoanScheduleAssembler.assembleLoanApplicationTermsFrom rebuilds the loan terms from the request
  JSON. A modify request may legitimately send only the fields being changed (LoanApplicationValidator
  .validateForModify already treats the rest as optional), but the recalculation read each schedule-driving
  field straight from the request: an omitted override-enabled enum hit XxxMethod.fromInt(null) and an omitted
  expectedDisbursementDate hit deriveFirstRepaymentDate(null), both throwing a NullPointerException (HTTP 500)
  once the change triggered a schedule recalculation. This completes FINERACT-2389, which applied null-override
  handling only to the grace/tolerance fields.
• Before recalculating, backfill any omitted schedule-driving field from the loan's own values
  (LoanAssemblerImpl.updateFrom -> backfillOmittedScheduleTerms) so the regenerated schedule matches what the
  loan records rather than a product default or null: interestType, amortizationType,
  interestCalculationPeriodType, repaymentEvery, numberOfRepayments, repaymentFrequencyType, loanTermFrequency,
  loanTermFrequencyType, interestRateFrequencyType, graceOnPrincipalPayment, graceOnInterestPayment,
  graceOnArrearsAgeing, principal, interestRatePerPeriod, expectedDisbursementDate, submittedOnDate and
  interestChargedFromDate. This mirrors validateForModify, so validation and recalculation stay consistent.
• Date fields are re-serialised with the request's own dateFormat/locale, exactly as loan creation requires
  them; if a date must be backfilled and either parameter is missing or malformed, fail with a clear
  validation error rather than defaulting.
• Add unit tests for the backfill helper, the date-formatter required-parameter handling, and the
  schedule-term backfill from a loan.

Description

Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)

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
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[adamsaghy]

I dont like this approach. No backfilling please!

If something is not provided, it should not be touched / updated!

[oluexpert99]

I dont like this approach. No backfilling please!

If something is not provided, it should not be touched / updated!

Thanks for the review. I agree with the principle — an omitted field must not be modified — and that's exactly what this preserves. "Backfill" is probably a misleading word for it; let me clarify.

The field updates already honor that rule: updateLoanApplicationAttributes only mutates a field when its parameter is present in the request. Nothing omitted is written or reset.

The NPE isn't in the field update — it's in the schedule recalculation. When a modify triggers a recalc, the schedule assembler rebuilds the loan terms from the request JSON. For a schedule-driving field the request omitted (interestType,
amortizationType, interestCalculationPeriodType, repaymentEvery), it reads null → InterestMethod.fromInt(null) → HTTP 500.

The assembler needs a complete set of inputs to recalc. This change supplies the omitted ones from the loan's own existing values (loan.getLoanProductRelatedDetail()) — not the product default, not a hardcoded default. So the regenerated schedule
matches what the loan already records, and the omitted field keeps its existing value. Net effect: the omitted field is untouched — your point exactly.

Concretely, on an overridden loan (interest method Declining; product default Flat): a modify omitting interestType returns 200, the header stays Declining, and the regenerated schedule equals an explicit Declining run — not the product's Flat. If
we instead don't supply the value, the recalc either NPEs (the current bug) or falls back to the product default and silently rewrites the schedule to disagree with the loan header — a worse, silent bug.

This also continues FINERACT-2389, which merged the same "omitted ⇒ keep existing value" idiom for the grace/tolerance fields; this closes the gap for the four schedule attributes it didn't cover.

If your concern is the mechanism (adding to the parsed JSON), I'm glad to instead read the loan's existing LoanProductRelatedDetail directly at recalc time — same outcome, no touching of the request. Which would you prefer?

[adamsaghy]

I dont like this approach. No backfilling please!
If something is not provided, it should not be touched / updated!

Thanks for the review. I agree with the principle — an omitted field must not be modified — and that's exactly what this preserves. "Backfill" is probably a misleading word for it; let me clarify.

The field updates already honor that rule: updateLoanApplicationAttributes only mutates a field when its parameter is present in the request. Nothing omitted is written or reset.

The NPE isn't in the field update — it's in the schedule recalculation. When a modify triggers a recalc, the schedule assembler rebuilds the loan terms from the request JSON. For a schedule-driving field the request omitted (interestType, amortizationType, interestCalculationPeriodType, repaymentEvery), it reads null → InterestMethod.fromInt(null) → HTTP 500.

The assembler needs a complete set of inputs to recalc. This change supplies the omitted ones from the loan's own existing values (loan.getLoanProductRelatedDetail()) — not the product default, not a hardcoded default. So the regenerated schedule matches what the loan already records, and the omitted field keeps its existing value. Net effect: the omitted field is untouched — your point exactly.

Concretely, on an overridden loan (interest method Declining; product default Flat): a modify omitting interestType returns 200, the header stays Declining, and the regenerated schedule equals an explicit Declining run — not the product's Flat. If we instead don't supply the value, the recalc either NPEs (the current bug) or falls back to the product default and silently rewrites the schedule to disagree with the loan header — a worse, silent bug.

This also continues FINERACT-2389, which merged the same "omitted ⇒ keep existing value" idiom for the grace/tolerance fields; this closes the gap for the four schedule attributes it didn't cover.

If your concern is the mechanism (adding to the parsed JSON), I'm glad to instead read the loan's existing LoanProductRelatedDetail directly at recalc time — same outcome, no touching of the request. Which would you prefer?

The principle to manipulate the json object which is the incoming request is wrong and should not be followed.

If something was not provided, just ignore it... we are doing it in many situations already:
command.hasParameter or fromJsonHelper.parameterExists

[oluexpert99]

The principle to manipulate the json object which is the incoming request is wrong and should not be followed.

If something was not provided, just ignore it... we are doing it in many situations already: command.hasParameter or fromJsonHelper.parameterExists

Hi @adamsaghy
I revamped this to ensure no more request-JSON manipulation. On modify, the schedule now recalculates from the loan's own state (buildScheduleGeneratorDTO(loan, null) + regenerateRepaymentSchedule(loan, dto)), exactly like the approval/reschedule/apply-holidays flows. An omitted field is simply ignored and its existing value flows through; nothing is written back to the request. Also, I dropped the redundant post-recalc recalculateAllCharges (regenerate already does it).

[adamsaghy]

@oluexpert99 Please check the failing test cases.

[adamsaghy]

I was wondering whether we could cover this changes with integration test / E2E test to ensure correct results?

[adamsaghy]

LGTM