Skip to content

FINERACT-2779: Remove restassured from feign loan helper - #6321

Open
DeathGun44 wants to merge 15 commits into
apache:developfrom
DeathGun44:FINERACT-2779/remove-restassured-from-feign-loan-helper
Open

FINERACT-2779: Remove restassured from feign loan helper#6321
DeathGun44 wants to merge 15 commits into
apache:developfrom
DeathGun44:FINERACT-2779/remove-restassured-from-feign-loan-helper

Conversation

@DeathGun44

@DeathGun44 DeathGun44 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

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.
  • If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

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

@DeathGun44 DeathGun44 changed the title FINERACT-2779: remove restassured from feign loan helper FINERACT-2779: Remove restassured from feign loan helper Aug 24, 2026
@DeathGun44 DeathGun44 mentioned this pull request Aug 24, 2026
Open
7 tasks
adamsaghy
adamsaghy reviewed Aug 26, 2026
View reviewed changes
Comment thread ...sts/src/test/java/org/apache/fineract/integrationtests/ExternalIdSupportIntegrationTest.java Outdated
.withFeeAndPenaltyAssetAccount(assetFeeAndPenaltyAccount).build(null);
final Integer loanProductID = getLoanProductId(loanProductJSON);
.withFeeAndPenaltyAssetAccount(assetFeeAndPenaltyAccount).buildRequest(null);
final Integer loanProductID = getLoanProductId(loanProductRequest);

@adamsaghy adamsaghy Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Long, no?

@DeathGun44 DeathGun44 Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. getLoanProductId only existed to downcast the Long that createLoanProduct already returns, and applyForLoanApplication converted both ids straight back. Both are Long now, which also removes the clientId.intValue() at 19 call sites. It turned up a real bug: one test had a private applyForLoanApplication(Long, Long, String) whose third argument is a principal, while the base method's third argument is an external id. On Integer ids the clash was invisible. Renamed it applyForLoanWithPrincipal.

adamsaghy
adamsaghy reviewed Aug 26, 2026
View reviewed changes
Comment thread ...sts/src/test/java/org/apache/fineract/integrationtests/ExternalIdSupportIntegrationTest.java Outdated
.withInArrearsTolerance("1001").withMultiDisburse().withDisallowExpectedDisbursements(true).build(null);
final Integer loanProductID = getLoanProductId(loanProductJSON);
.withInArrearsTolerance("1001").withMultiDisburse().withDisallowExpectedDisbursements(true).buildRequest(null);
final Integer loanProductID = getLoanProductId(loanProductRequest);

@adamsaghy adamsaghy Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Long, no?

@DeathGun44 DeathGun44 Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

adamsaghy
adamsaghy reviewed Aug 26, 2026
View reviewed changes
Comment thread ...sts/src/test/java/org/apache/fineract/integrationtests/ExternalIdSupportIntegrationTest.java Outdated

@adamsaghy adamsaghy Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be using feign, no?

@DeathGun44 DeathGun44 Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looked into it, It did go through Feign, but the base class was throwing the response away: it kept only the id and rebuilt a PostLoansResponse by hand, with a second GET to fetch the external id back. POST /loans already returns all of it. It now returns the client's response directly. The loan() test asserts the external id right after applying and passes, so the extra GET was never needed.

adamsaghy
adamsaghy requested changes Aug 26, 2026
View reviewed changes

@adamsaghy adamsaghy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please kindly review my concenrs

@DeathGun44
FINERACT-2779: fix loan swagger DTOs and retire the REST-assured comm…
27c1117 
…and paths

FeignLoanHelper drove the server through REST-assured for six loan commands,
so every test taking those paths was Feign in name only. The gaps that forced
them are fixed at source rather than worked around:

  PostLoansRequest             += calendarId, syncDisbursementWithMeeting,
                                  createStandingInstructionAtDisbursement,
                                  interestChargedFromDate
  PostLoansRequest.repaymentsStartingFromDate         LocalDate -> String
  PostLoansLoanIdDisbursementData.expectedDisbursementDate  LocalDate -> String
  PostCreateRescheduleLoansRequest += recalculateInterest
  PostProvisioningCriteriaRequest  += definitions, locale

Both date fields were declared LocalDate while the same request body declares
dateFormat "dd MMMM yyyy", so a generated client serialised ISO and the server
rejected it. Their response-side twins were already String; the schema example
"[2012, 4, 3]" was the bug frozen into the spec. Only the format changes, so
the wire payload is unchanged and swagger-brake reports no breaking change.

Adding the fields made two workaround subclasses redundant -
RescheduleRequestWithRecalculateInterest and ApplyLoanWithLegacyDates, both of
which existed solely because "the generated OpenAPI model omits this field".
Error Prone found them via MissingOverride once the real setters appeared.

disburseToSavings, disburseLoanFromJson and the reschedule create now call the
typed client. The JSON builders they used silently injected a note and a
netDisbursalAmount, so those are set explicitly at the call sites to keep the
payload identical.

createRescheduleRequestWithFullResponse returned an untyped HashMap and took an
expected status code; it is now FeignCalls.fail(...) with the status asserted at
the call site. The specific error code lives in errors[0], not the top-level
userMessageGlobalisationCode, so the assertion goes through
FeignLoanTestBase.extractErrorGlobalisationCode.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: build loan applications with PostLoansRequest instead …
60a52a1 
…of JSON

Converts the first three applyForLoanFromJson callers to the typed model, now
that PostLoansRequest carries the fields they need.

LoanApplicationTestBuilder.build() is not the visible .withX() chain: it always
emits maxOutstandingLoanBalance "36000", collateral [], a default
transactionProcessingStrategyCode, loanType and locale "en_GB" - none of which
appear at the call sites. Each conversion reproduces them, so the request body
is unchanged.

The jlg case in LoanReschedulingWithinCenterTest also stops round-tripping its
tranches through HashMap, since the parameter was already typed. Its collateral
list becomes PostLoansRequestCollateralData, which names the field quantity
where the map said amount. That mismatch was harmless only because
LoanApplicationValidator applies collateral to individual accounts alone and
skips it for jlg; the typed name is what the server would read if that guard
ever widens, and the helper says so.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: type the advanced-payment-allocation loan applications
2c58bcc 
Three more applyForLoanFromJson callers move to PostLoansRequest. Each keeps
the defaults LoanApplicationTestBuilder.build() applied invisibly, including
the amortizationType and interestCalculationPeriodType the chains never set,
and strips the thousands separator from the 15,000.00 principal.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: type the single-repayment loan applications
9730d15 
Four more applyForLoanFromJson callers move to PostLoansRequest. All four are
the same one-month, one-repayment flat-balance shape, so they differ only in
principal, rate and external id.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: share the legacy loan application shape and type four …
53f1474 
…more callers

The four client-loan tests built the same equal-installment declining-balance
application, so LoanRequestBuilders.legacyIndividualApplication carries it once,
including the fields build() emitted invisibly. It takes the principal as a
String and strips the grouping separator, because the JSON builder sent amounts
like 12,000.00 for the server to parse under en_GB.

The tranche variant also sends the fixedEmiAmount that build() attached
whenever disbursementData was present, and its tranche details are now
PostLoansDisbursementData rather than HashMap.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: share the days-based loan application shape
cd2863c 
Four advanced-payment-allocation tests built the same days-based equal-principal
application, so legacyDaysBasedApplication carries it alongside the monthly one.
Interest type defaults to flat and the reverse-replay test overrides it, which
is the only way the four differed beyond dates and strategy.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: type the down-payment loan applications
1d82cc7 
Three more callers reuse legacyIndividualApplication, overriding the interest
and amortization types where they differ from its equal-installment declining
balance default.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: type the down-payment repayment schedule applications
f1f03ef 
Converts the last four applyForLoanFromJson sites in the file. Also gives
legacyDaysBasedApplication the default transactionProcessingStrategyCode that
build() always emitted; its earlier callers all set a strategy explicitly, so
the omission only surfaced here as a mandatory-parameter 400.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: type the tranche reschedule loan application
ea621cb 
Drops the HashMap round-trip for the tranche details, which the method already
received as PostLoansDisbursementData. The empty fixedEmiAmount the JSON
builder sent has no typed equivalent and is omitted; the schedule assertions
are unchanged.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: retire applyForLoanFromJson and the last REST-assured …
2877731 
…plumbing

The floating-rate test was the only caller that could not be expressed typed:
it stripped interestRatePerPeriod and added interestRateDifferential and
isFloatingInterestRate, neither of which was on PostLoansRequest. Both are
accepted by LoanApplicationValidator, so they are added to the DTO rather than
left as a reason to keep the JSON path.

With the last caller converted, applyForLoanFromJson, jsonRequestSpec,
APPLY_LOAN_URL and the io.restassured imports are deleted. FeignLoanHelper no
longer references REST-assured at all.

getLoanIdFromApplication, which built a PostLoansResponse from a bare loan id,
becomes applyForLoanResponse taking a typed request.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: build loan products with PostLoanProductsRequest inste…
62c0575 
…ad of JSON

LoanProductTestBuilder only spoke JSON, so every Feign test that needed a
loan product handed a string to createLoanProductFromJson, which parsed it
straight back into PostLoanProductsRequest. Add buildRequest(), the typed
counterpart of build(), so callers can skip the round trip.

Six of the map's keys have no counterpart on the request model:
syncExpectedWithDisbursementDate, mandatoryGuarantee,
minimumGuaranteeFromGuarantor, minimumGuaranteeFromOwnFunds, minimumGap and
maximumGap. The JSON path dropped them as unknown properties, so leaving
them unset sends the same body and needs no schema change.

LoanProductTestBuilderParityTest pins the two builders together: 31 builder
configurations are serialised and deserialised the way the JSON path did,
and the result must equal the typed request.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: type the loan product creations in the Feign loan tests
ec00c9e 
Replace every createLoanProductFromJson / getLoanProductError /
getLoanProductId call that took a hand-built JSON string with the typed
request from LoanProductTestBuilder.buildRequest(). The Utils.convertToJson
and Gson().toJson wrappers around the builder's map go with them.

Three helpers now return what they actually build: loanProductJson() ->
loanProductRequest(), buildLoanProductJson() -> buildLoanProductRequest(),
createLoanJSON() -> createLoanProductRequest(), and
loanProductTestBuilder() -> customizedLoanProduct(), which returns a
request rather than a builder.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: retire the JSON loan product paths
7b45fae 
With no callers left, drop createLoanProductFromJson from FeignLoanHelper
and FeignLoanTestBase, and take PostLoanProductsRequest on
getLoanProductError and getLoanProductId. That removes the last places a
Feign loan test parsed a JSON string back into a request model, along with
the "silences unknown property errors" warning that came with it.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
@DeathGun44
FINERACT-2779: return the loan application response the server sent
cd8250d 
applyForLoanResponse kept only the id from the Feign response and rebuilt a
PostLoansResponse by hand, paying for a second GET to recover the external id. POST /loans
already answers a full response, so the synthetic object dropped every other field for no
gain. Return the client's response.

Type the loan-product id as Long while here: getLoanProductId existed only to downcast
createLoanProduct, which already returns Long, and applyForLoanApplication converted both
ids straight back with longValue().
@DeathGun44
FINERACT-2779: carry loan ids as Long through the loan tests
ac60619 
Follows the base helpers to Long: the loan-product locals lose their Integer declarations
and nineteen call sites lose the clientId.intValue() downcast.

Renaming one method is not cosmetic. ClientLoanChargeExternalIntegrationTest declared a
private applyForLoanApplication(Long, Long, String) whose third argument is a principal,
while the base method of the same shape takes an external id. On Integer ids the signatures
differed and the collision was invisible; it is now applyForLoanWithPrincipal.

Also drops a redundant String cast on getResourceExternalId, which the generated model has
returned as String for a while.
@DeathGun44
DeathGun44 force-pushed the FINERACT-2779/remove-restassured-from-feign-loan-helper branch from ecd772f to ac60619 Compare August 26, 2026 18:15
@DeathGun44

DeathGun44 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

@adamsaghy ready for re review when you have a moment ,the failing test seems flaky

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adamsaghy adamsaghy adamsaghy requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@DeathGun44 @adamsaghy