You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "oluexpert99 (via GitHub)" <gi...@apache.org> on 2026/06/30 22:26:22 UTC
[PR] FINERACT-2668: Stop bulk-import loan-repayment template loading the entire tenant [fineract]
oluexpert99 opened a new pull request, #6071:
URL: https://github.com/apache/fineract/pull/6071
- Downloading the loan-repayment template (GET /v1/loans/repayments/downloadtemplate) eagerly fetched every
client (ClientReadPlatformService.retrieveAll(null) applies no LIMIT) and every active loan, embedding them
as a tenant-wide Clients sheet, a per-loan lookup table and a 3000-row x 5 VLOOKUP block. The cost is
O(tenant size) -- multi-MB .xls, slow Excel open, and server heap pressure / OOM risk under concurrent
downloads -- for an operation that is conceptually O(rows entered). The import handler never reads the
Clients/Offices sheets; it resolves the loan from the typed account number
(LoanRepaymentImportHandler -> retrieveLoanIdByAccountNumber), so the embedded dataset is discarded at import.
- Add an optional lean template, selected by a new LookupMode {AUTO, INCLUDE, EXCLUDE}.
BulkImportWorkbookPopulatorService gains a 5-arg getTemplate(..., LookupMode); the existing 4-arg signature
becomes a default delegating with AUTO, so all other callers and templates are unchanged.
- LoansApiResource#getLoanRepaymentTemplate exposes ?includeLookups={true|false}, mapped to a LookupMode
(null -> AUTO, true -> INCLUDE, false -> EXCLUDE). INCLUDE keeps the previous unbounded behaviour (the caller
accepts the full cost). EXCLUDE always produces a lean template. AUTO bounds the client/loan fetch with a SQL
LIMIT and degrades to lean when either set exceeds fineract.bulk-import.template.max-lookup-rows
(default 10000), surfaced through FineractProperties.
- LoanRepaymentWorkbookPopulator gains a lean mode (static lean()/full() factories) that omits the
client/office sheets, the per-loan lookup table and the 3000-row VLOOKUP block, while keeping the data-entry
column layout intact (the import handler parses by fixed column index) and retaining the payment-type
dropdown sourced from the small Extras sheet (extracted to a shared addPaymentTypeValidation()).
- Add unit tests: LookupMode mapping; the max-lookup-rows cap boundary; and lean-mode output asserting the
Clients/Offices sheets and per-loan lookup rows are omitted while the data-column headers are preserved.
## 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](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
- [ ] 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](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [ ] Follow our [coding conventions](https://cwiki.apache.org/confluence/display/FINERACT/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"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
--
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
Re: [PR] FINERACT-2668: Stop bulk-import loan-repayment template loading the entire tenant [fineract]
Posted by "oluexpert99 (via GitHub)" <gi...@apache.org>.
oluexpert99 commented on PR #6071:
URL: https://github.com/apache/fineract/pull/6071#issuecomment-4852888306
> I have some concerns about this functionality. Specifically, I’m worried about limiting the number of fetched data and the data that can be retrieved. What if I need all 20,000 clients or loans? Perhaps I don’t fully understand how these `templates`, `downloadTemplates`, and `bulk-import` work. Could you please explain their exact purpose and usage?
Thanks — happy to explain, and your instinct about "limiting data" is worth addressing head-on.
What these templates are. Bulk import is two phases:
1. Download template (downloadtemplate) — the server generates a pre-formatted .xls to help a user data-enter transactions. Besides the data-entry sheet, it embeds hidden Clients/Offices sheets plus cascading Excel dropdowns so the user can pick
Office → Client → Account instead of typing.
2. Upload (uploadtemplate) — the user fills rows and uploads; the import handler posts each row.
The key point: the import handler never reads the Clients/Offices sheets. For loan repayment, it resolves the loan straight from the typed account number (retrieveLoanIdByAccountNumber). So the embedded client/office dataset is purely a data-entry
convenience for the downloaded file — it is discarded at import.
This PR does not limit what you can import. You can still upload and import 20,000 (or 200,000) rows — the upload/import path is completely unchanged.
And it does not permanently limit the download either. ?includeLookups=true fetches all clients/loans, unbounded — exactly today's behaviour. If you want all 20,000 in the dropdowns, that flag gives you all 20,000.
What changed is only the default for very large tenants: today, retrieveAll(null) loads every client with no LIMIT and writes them all (plus a 3000-row VLOOKUP block) into the workbook — a multi-MB file that's slow to open and can OOM the server
under concurrent downloads, built from data the importer ignores. So AUTO (default) degrades to a lean template above a configurable threshold; the lean template still imports everything — the user just types the account number instead of picking
from a 20,000-row dropdown.
If you'd prefer zero change to the default behaviour, I'm glad to flip it so the default stays the full (unbounded) template and the lean form is purely opt-in via ?includeLookups=false — that keeps this PR strictly additive. Happy to go whichever way.
--
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
Re: [PR] FINERACT-2668: Stop bulk-import loan-repayment template loading the entire tenant [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6071:
URL: https://github.com/apache/fineract/pull/6071#issuecomment-4852976969
> > I have some concerns about this functionality. Specifically, I’m worried about limiting the number of fetched data and the data that can be retrieved. What if I need all 20,000 clients or loans? Perhaps I don’t fully understand how these `templates`, `downloadTemplates`, and `bulk-import` work. Could you please explain their exact purpose and usage?
>
> Thanks — happy to explain, and your instinct about "limiting data" is worth addressing head-on.
>
> What these templates are. Bulk import is two phases:
>
> 1. Download template (downloadtemplate) — the server generates a pre-formatted .xls to help a user data-enter transactions. Besides the data-entry sheet, it embeds hidden Clients/Offices sheets plus cascading Excel dropdowns so the user can pick
> Office → Client → Account instead of typing.
> 2. Upload (uploadtemplate) — the user fills rows and uploads; the import handler posts each row.
>
> The key point: the import handler never reads the Clients/Offices sheets. For loan repayment, it resolves the loan straight from the typed account number (retrieveLoanIdByAccountNumber). So the embedded client/office dataset is purely a data-entry convenience for the downloaded file — it is discarded at import.
>
> This PR does not limit what you can import. You can still upload and import 20,000 (or 200,000) rows — the upload/import path is completely unchanged.
>
> And it does not permanently limit the download either. ?includeLookups=true fetches all clients/loans, unbounded — exactly today's behaviour. If you want all 20,000 in the dropdowns, that flag gives you all 20,000.
>
> What changed is only the default for very large tenants: today, retrieveAll(null) loads every client with no LIMIT and writes them all (plus a 3000-row VLOOKUP block) into the workbook — a multi-MB file that's slow to open and can OOM the server under concurrent downloads, built from data the importer ignores. So AUTO (default) degrades to a lean template above a configurable threshold; the lean template still imports everything — the user just types the account number instead of picking from a 20,000-row dropdown.
>
> If you'd prefer zero change to the default behaviour, I'm glad to flip it so the default stays the full (unbounded) template and the lean form is purely opt-in via ?includeLookups=false — that keeps this PR strictly additive. Happy to go whichever way.
Thank you for the explanation.
I understand the lean vs not lean approach and i am okay with that:
- Lean should not fetch any of the client / loan / etc, but only the small entry size selectors (payment type, transaction type, etc.).
What i dont get is `max-lookup-rows`: We should either read all entries or none... reading some number of entries are incorrect, misleading and lead towards weird situations where dropdown list is incomplete...
What do you think?
--
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
Re: [PR] FINERACT-2668: Stop bulk-import loan-repayment template loading the entire tenant [fineract]
Posted by "oluexpert99 (via GitHub)" <gi...@apache.org>.
oluexpert99 commented on code in PR #6071:
URL: https://github.com/apache/fineract/pull/6071#discussion_r3504795167
##########
fineract-core/src/main/java/org/apache/fineract/infrastructure/core/config/FineractProperties.java:
##########
@@ -448,6 +449,22 @@ public static class FineractTemplateProperties {
private List<String> regexWhitelist;
}
+ @Getter
+ @Setter
+ public static class FineractBulkImportProperties {
+
+ private FineractBulkImportTemplateProperties template;
+ }
+
+ @Getter
+ @Setter
+ public static class FineractBulkImportTemplateProperties {
+
+ // FINERACT-2668: max tenant-wide lookup rows (clients/loans) embedded in a download template before it
+ // auto-degrades to a lean form (typed keys only). Honoured by templates that opt in (loan repayment).
+ private int maxLookupRows = 10000;
Review Comment:
Thanks @adamsaghy, an oversight. It has been addressed
--
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
Re: [PR] FINERACT-2668: Stop bulk-import loan-repayment template loading the entire tenant [fineract]
Posted by "oluexpert99 (via GitHub)" <gi...@apache.org>.
oluexpert99 commented on PR #6071:
URL: https://github.com/apache/fineract/pull/6071#issuecomment-4853532954
> > > I have some concerns about this functionality. Specifically, I’m worried about limiting the number of fetched data and the data that can be retrieved. What if I need all 20,000 clients or loans? Perhaps I don’t fully understand how these `templates`, `downloadTemplates`, and `bulk-import` work. Could you please explain their exact purpose and usage?
> >
> >
> > Thanks — happy to explain, and your instinct about "limiting data" is worth addressing head-on.
> > What these templates are. Bulk import is two phases:
> >
> > 1. Download template (downloadtemplate) — the server generates a pre-formatted .xls to help a user data-enter transactions. Besides the data-entry sheet, it embeds hidden Clients/Offices sheets plus cascading Excel dropdowns so the user can pick
> > Office → Client → Account instead of typing.
> > 2. Upload (uploadtemplate) — the user fills rows and uploads; the import handler posts each row.
> >
> > The key point: the import handler never reads the Clients/Offices sheets. For loan repayment, it resolves the loan straight from the typed account number (retrieveLoanIdByAccountNumber). So the embedded client/office dataset is purely a data-entry convenience for the downloaded file — it is discarded at import.
> > This PR does not limit what you can import. You can still upload and import 20,000 (or 200,000) rows — the upload/import path is completely unchanged.
> > And it does not permanently limit the download either. ?includeLookups=true fetches all clients/loans, unbounded — exactly today's behaviour. If you want all 20,000 in the dropdowns, that flag gives you all 20,000.
> > What changed is only the default for very large tenants: today, retrieveAll(null) loads every client with no LIMIT and writes them all (plus a 3000-row VLOOKUP block) into the workbook — a multi-MB file that's slow to open and can OOM the server under concurrent downloads, built from data the importer ignores. So AUTO (default) degrades to a lean template above a configurable threshold; the lean template still imports everything — the user just types the account number instead of picking from a 20,000-row dropdown.
> > If you'd prefer zero change to the default behaviour, I'm glad to flip it so the default stays the full (unbounded) template and the lean form is purely opt-in via ?includeLookups=false — that keeps this PR strictly additive. Happy to go whichever way.
>
> Thank you for the explanation.
>
> I understand the lean vs not lean approach and i am okay with that:
>
> * Lean should not fetch any of the client / loan / etc, but only the small entry size selectors (payment type, transaction type, etc.).
>
> What i dont get is `max-lookup-rows`: We should either read all entries or none... reading some number of entries are incorrect, misleading and lead towards weird situations where dropdown list is incomplete...
>
> What do you think?
Agreed @adamsaghy I dropped max-lookup-rows and the auto-degrade entirely. It's now all-or-nothing: the default (includeLookups absent/true) fetches every client/account exactly as before, and ?includeLookups=false fetches none and returns a lean template
--
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
Re: [PR] FINERACT-2668: Stop bulk-import loan-repayment template loading the entire tenant [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #6071:
URL: https://github.com/apache/fineract/pull/6071#discussion_r3504710657
##########
fineract-core/src/main/java/org/apache/fineract/infrastructure/core/config/FineractProperties.java:
##########
@@ -448,6 +449,22 @@ public static class FineractTemplateProperties {
private List<String> regexWhitelist;
}
+ @Getter
+ @Setter
+ public static class FineractBulkImportProperties {
+
+ private FineractBulkImportTemplateProperties template;
+ }
+
+ @Getter
+ @Setter
+ public static class FineractBulkImportTemplateProperties {
+
+ // FINERACT-2668: max tenant-wide lookup rows (clients/loans) embedded in a download template before it
+ // auto-degrades to a lean form (typed keys only). Honoured by templates that opt in (loan repayment).
+ private int maxLookupRows = 10000;
Review Comment:
No need for hardcoded value
--
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
Re: [PR] FINERACT-2668: Stop bulk-import loan-repayment template loading the entire tenant [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6071:
URL: https://github.com/apache/fineract/pull/6071#issuecomment-4852764100
I am having some concerns... limiting the number of fetched data, limiting the data you got... What if you need all 20k clients or loan? Maybe i dont fully understand this functionality... can you please help me to understand what these `templates`, `downloadTemplates`, `bulk-import` is doing exactly and used for what purpose?
--
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