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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
