You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "priyam99 (via GitHub)" <gi...@apache.org> on 2026/10/17 21:48:49 UTC
[PR] Prevent duplicate Fineract loan IDs from the mock adapter [fineract-loan-origination]
priyam99 opened a new pull request, #174:
URL: https://github.com/apache/fineract-loan-origination/pull/174
## Description
The mock Fineract adapter keeps its loan ID counter in memory, starting from 100000. Every restart resets it, so the next mock disbursement gets 100001 again even if an application already has that ID. The database accepted the duplicate without complaint, so two applications ended up pointing at the same "Fineract loan" and nothing flagged it.
I fixed this in two places.
First, before `MockFineractLoanApiClient` creates a loan, it now looks up the highest `fineract_loan_id` already stored and moves its counter past it. After a restart it carries on from where the data left off, so IDs are not reused. I read the value from `loan_application` rather than saving a separate "last ID" somewhere, so there is nothing extra to keep in sync, and it still works if the app was killed rather than shut down cleanly. The lookup is a small `MAX` query on `LoanApplicationRepository`. It returns only a number, so I left it unscoped by tenant and said so in its Javadoc.
Second, I added a unique constraint on `(tenant_id, fineract_loan_id)`, both on the entity and as migration `V10`, so a duplicate fails loudly instead of being saved. The issue asks for uniqueness on `fineract_loan_id`. I scoped it by tenant because each Fineract tenant has its own database and numbers its loans independently: loan 5 in one tenant and loan 5 in another are both legitimate. NULLs are still allowed more than once, so applications that haven't reached Fineract are unaffected.
One thing to be aware of: if a local database already contains duplicate IDs from the old behaviour, Hibernate's `ddl-auto: update` logs a warning and skips the constraint, and `V10` would fail once Flyway is running again (#158). Those rows need cleaning up first. Fresh databases are fine.
I also updated the mock adapter section in `docs/workflows/disbursement.adoc`.
Closes #157
## Verification
- [x] `./mvnw spotless:apply`
- [x] `./mvnw clean verify`: 207 unit tests and 13 Cucumber scenarios pass, `BUILD SUCCESS`
- [x] `./mvnw apache-rat:check`: passes
- [ ] Frontend changes: not applicable, this is a backend-only change
To check that the tests catch the bug, I removed both parts of the fix and ran the build again. The two restart-related unit tests and the restart Cucumber scenario failed with `expected: 100003L but was: 100001L`, and the duplicate scenario failed because the database accepted the second row. With the fix in place everything passes.
I also started the app against an existing local database that had no constraint. Hibernate added it on startup, and inserting a second application with the same tenant and loan ID was rejected:
```
los_db=> INSERT INTO loan_application SELECT * FROM demo_copy; -- tenant 'default', loan 100001
ERROR: duplicate key value violates unique constraint "uk_loan_application_tenant_fineract_loan_id"
DETAIL: Key (tenant_id, fineract_loan_id)=(default, 100001) already exists.
los_db=> INSERT INTO loan_application SELECT * FROM demo_copy; -- tenant 'other-tenant', loan 100001
INSERT 0 1
```
I couldn't run a disbursement end to end over HTTP locally, because staff login validates against a real Fineract instance. The Cucumber restart scenario covers that path against PostgreSQL instead.
New tests:
- with nothing stored yet, the mock still starts at 100001
- with 100002 already stored, a freshly started mock returns 100003
- consecutive loans stay unique even before the previous ID has been saved
- (Cucumber) after a restart, the mock does not reuse a stored loan ID
- (Cucumber) linking the same Fineract loan ID twice in one tenant is rejected
- (Cucumber) the same Fineract loan ID can be used in two different tenants
## AI assistance (optional)
- Tool / model: Claude Code (Claude Opus 5.5)
- How it was used: helped investigate the issue, draft the fix and tests, and run the build and the before/after checks. I reviewed every line and am responsible for the change.
## Checklist
- [x] This PR is one logical change, not a code dump.
- [x] I added or updated tests, or explained why tests are not needed.
- [x] New files carry the ASF license header.
- [x] Docs (and `AGENTS.md`, if commands, paths, or architecture rules changed) are updated
- [x] All commits are signed.
- [x] I followed the [AI Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy) and the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html).
--
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] Prevent duplicate Fineract loan IDs from the mock adapter [fineract-loan-origination]
Posted by "KRYSTALM7 (via GitHub)" <gi...@apache.org>.
KRYSTALM7 commented on PR #174:
URL: https://github.com/apache/fineract-loan-origination/pull/174#issuecomment-6317027167
@priyam99 I’ll need some time to review this PR properly, as it is indirectly related to the Fineract integration with the LOS. I want to understand the changes and their potential impact on the integration before sharing my feedback.
--
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] Prevent duplicate Fineract loan IDs from the mock adapter [fineract-loan-origination]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal commented on code in PR #174:
URL: https://github.com/apache/fineract-loan-origination/pull/174#discussion_r4359252593
##########
src/main/java/org/apache/fineract/los/domain/LoanApplication.java:
##########
@@ -55,7 +56,12 @@
* writer — preventing silent data corruption.
*/
@Entity
-@Table(name = "loan_application")
+@Table(
+ name = "loan_application",
+ uniqueConstraints =
+ @UniqueConstraint(
+ name = "uk_loan_application_tenant_fineract_loan_id",
Review Comment:
**The violation path needs handling in `DisbursementBridgeService.createLoan`.** The constraint is the right safety net, but it now fires at a bad moment. In `createLoan` the order is:
1. `fineractLoanApiClient.createLoan(request)`: the loan now **exists in Fineract**;
2. `application.setFineractLoanId(...)` / `setFineractIntegrationStatus(LOAN_CREATED)`;
3. `loanApplicationRepository.save(application)`: this is where a duplicate now throws `DataIntegrityViolationException`.
Nothing catches that exception. Only `RestClientException` is handled around the HTTP call, and `GlobalExceptionHandler` has no `DataIntegrityViolationException` handler, so it falls through to the generic `Exception` handler and becomes a 500. Worse, `fineractIntegrationStatus` stays `null`. So the next call to `executeFullDisbursement` goes into "Step 1: create loan" again and **creates a second loan in Fineract**, while the first one is orphaned with no LOS record pointing at it.
With the real adapter a duplicate ID "shouldn't" happen. But the point of the constraint is to catch it when it does, and then the recovery path matters. Suggested:
- Catch `DataIntegrityViolationException` around the save in `createLoan`.
- Log an error with the `applicationRef`, tenant and the returned Fineract `loanId`, so ops can reconcile the orphan.
- Set the status to `FAILED`, or a dedicated "needs reconciliation" state, so a retry doesn't call `POST /loans` again.
- Throw a typed exception that maps to 409 instead of 500.
- Add a unit test in `DisbursementBridgeServiceTest` where `save` throws after `createLoan` succeeds, asserting that no second create happens on retry.
##########
src/main/java/org/apache/fineract/los/bridge/MockFineractLoanApiClient.java:
##########
@@ -55,8 +57,18 @@ public class MockFineractLoanApiClient implements FineractLoanApiClient {
private final AtomicLong sequence = new AtomicLong(MOCK_ID_SEED);
+ private final LoanApplicationRepository loanApplicationRepository;
+
+ public MockFineractLoanApiClient(final LoanApplicationRepository loanApplicationRepository) {
+ this.loanApplicationRepository = loanApplicationRepository;
+ }
+
@Override
public FineractLoanCreateResponse createLoan(final FineractLoanCreateRequest request) {
+ // The counter lives in memory and resets on restart, so move it past the IDs already stored.
+ final long highestStoredId =
+ loanApplicationRepository.findMaxFineractLoanId().orElse(MOCK_ID_SEED);
+ sequence.accumulateAndGet(highestStoredId, Math::max);
Review Comment:
This works for a single instance; the `AtomicLong` keeps IDs unique within the JVM even before they're saved, and `keepsIdsUniqueBeforeTheyAreStored` covers that. Two notes, neither blocking:
- **Multiple instances still collide.** Two mock-enabled instances against one database (e.g. a two-replica dev deployment) both read the same `MAX` and hand out the same next ID. The new constraint now makes that fail loudly, which is fine for a mock, but please say so in the class Javadoc ("unique per JVM; multiple instances against one DB will collide and the second save is rejected"), so nobody reads "unique across restarts" as "unique across instances". A Postgres sequence would remove the caveat if it ever matters.
- **Cost.** `MAX(fineract_loan_id)` across all tenants can't use the new `(tenant_id, fineract_loan_id)` index, so it's a sequential scan on every mock disbursement. That's irrelevant at dev volumes, but since it only runs in mock mode, a one-line comment saying so would stop someone copying the pattern into the real adapter.
##########
src/main/java/org/apache/fineract/los/repository/LoanApplicationRepository.java:
##########
@@ -136,4 +137,16 @@ Page<LoanApplication> findAllByStatusAndTenantId(
* @return total number of applications for the tenant
*/
long countByTenantId(String tenantId);
+
+ /**
+ * Returns the highest Fineract loan ID stored for any application.
+ *
+ * <p>Used only by {@code MockFineractLoanApiClient} so its generated loan IDs continue after the
+ * IDs already stored instead of restarting from a fixed seed. It returns a single number and no
+ * application data, so it is intentionally not scoped by tenant.
+ *
+ * @return the highest stored Fineract loan ID, or empty if no application has one
+ */
+ @Query("SELECT MAX(a.fineractLoanId) FROM LoanApplication a")
+ Optional<Long> findMaxFineractLoanId();
Review Comment:
Nit: this is a tenant-unscoped query on a repository where every other finder takes a `tenantId`, and the only thing preventing misuse is the Javadoc. It returns just a number, as you say, but it does reveal the highest Fineract loan ID across all tenants to any caller.
Since only the mock needs it, consider moving it off the shared repository, e.g. a small `@Profile`/`@ConditionalOnProperty("los.fineract.mock-enabled")` query bean next to `MockFineractLoanApiClient`, or a native query there. Then a future feature can't pick it up by accident. Non-blocking.
##########
src/main/resources/db/migration/V10__add_unique_fineract_loan_id_per_tenant.sql:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+-- A Fineract loan can be linked to only one LOS application per tenant.
+-- NULLs are allowed more than once, so applications not yet sent to Fineract are unaffected.
+ALTER TABLE loan_application
+ ADD CONSTRAINT uk_loan_application_tenant_fineract_loan_id
+ UNIQUE (tenant_id, fineract_loan_id);
Review Comment:
You flagged that existing duplicates make this fail (and make `ddl-auto: update` silently skip the constraint). Since anyone who ran the mock and restarted has those duplicates locally, a short cleanup note would save confusion.
Please add a SQL snippet to `disbursement.adoc`, or as a comment here, that finds them, e.g.
```sql
SELECT tenant_id, fineract_loan_id, count(*)
FROM loan_application
WHERE fineract_loan_id IS NOT NULL
GROUP BY 1, 2 HAVING count(*) > 1;
```
plus a line on what to do with them (null out the later rows' `fineract_loan_id` in a dev DB). Please also cross-reference #158, so whoever re-enables Flyway knows V10 depends on that cleanup.
--
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