adamsaghy commented on PR #6059: URL: https://github.com/apache/fineract/pull/6059#issuecomment-4853055016
> > 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](https://issues.apache.org/jira/browse/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` -- 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]
