FINERACT-2646: Decouple accounting from charge via chargeId reference by mansi75 · Pull Request #6106 · apache/fineract

mansi75 · 2026-07-08T19:58:37Z

Description

This PR addresses FINERACT-2646 by decoupling the accounting module from the charge domain dependency.

The accounting code previously depended on charge domain objects directly. This change updates the accounting side to use the 'chargeId' reference instead, so accounting can identify and process charge-related mappings without requiring a direct dependency on the charge package/module.

This helps improve module boundaries and supports the ongoing Fineract modularization effort by reducing cross-feature coupling between accounting and charge. FINERACT-2646 is listed as a sub-task for decoupling the accounting package/module from third-party domains. :contentReference[oaicite:0]{index=0}

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.

Copilot

Pull request overview

This PR implements FINERACT-2646 by removing accounting’s dependency on charge domain objects, switching accounting-side charge-related mappings to use chargeId references and (where needed) direct SQL reads of charge metadata. It also introduces a Modulith-based boundary test to prevent future cross-feature coupling regressions.

Changes:

Replaced ProductToGLAccountMapping’s Charge association with a scalar chargeId and updated related repository queries and mapping helpers.
Reworked fee/penalty-to-income mapping reads to join m_charge via JdbcTemplate (to avoid charge-domain navigation and EclipseLink native query binding issues).
Added an accounting cross-feature dependency boundary test (Spring Modulith) and adjusted accounting module test/build dependencies.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
fineract-loan/src/main/java/org/apache/fineract/accounting/productaccountmapping/service/LoanProductToGLAccountMappingHelper.java	Aligns constructor/super wiring with removal of charge-domain dependency.
fineract-accounting/src/test/java/org/apache/fineract/accounting/AccountingCrossFeatureBoundaryTest.java	Adds Modulith-based boundary test to detect cross-feature type references from accounting.
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/WorkingCapitalLoanProductAdvancedAccountingReadHelper.java	Reads charge name via SQL join instead of charge domain; maps to accounting DTOs.
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ShareProductToGLAccountMappingHelper.java	Removes charge-domain constructor dependency.
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/SavingsProductToGLAccountMappingHelper.java	Removes charge-domain constructor dependency and related comment block.
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingReadPlatformServiceImpl.java	Reworks charge-income mapping reads via JDBC and updates mapping behavior accordingly.
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java	Stores `chargeId` instead of a `Charge` reference when persisting mappings.
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/domain/ProductToGLAccountMappingRepository.java	Updates JPQL to use `chargeId` column and removes charge-domain-specific queries.
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/domain/ProductToGLAccountMapping.java	Replaces `Charge` relation with `Long chargeId` field.
fineract-accounting/dependencies.gradle	Adds Modulith test dependency/version; currently still declares `:fineract-charge` implementation dependency.
fineract-accounting/build.gradle	Increases test JVM heap/metaspace limits for the accounting module.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

adamsaghy · 2026-07-13T09:22:26Z

+        // The charge name lives on m_charge, not on ProductToGLAccountMapping (which only stores charge_id), and the
+        // accounting module is decoupled from the charge domain, so we read the charge -> income-account rows via
+        // JdbcTemplate rather than a JPA native query (EclipseLink does not bind Spring Data ":name" markers in native
+        // queries). Column order: [0] gl_account_id, [1] charge id, [2] charge name. The penalty flag is known from the
+        // method argument, so it is inlined as a boolean literal (never user input) rather than bound.


Copilot is right, also, see above: this is not the way...

That would be a regression, agreed.

Got it. I'll rework on it

Aman-Mittal · 2026-07-13T06:27:31Z

+        // markers to reach the database. JdbcTemplate uses plain JDBC positional binding and works on all supported
+        // databases. The GL account fields are selected via a join on acc_gl_account so we avoid a per-row lookup.
+        // Column order: [0] gl_account_id, [1] gl_account_name, [2] gl_code, [3] charge_id, [4] charge_name.
+        final String sql = "select ga.id, ga.name, ga.gl_code, c.id, c.name " + "from acc_product_mapping apm "


You can use java textblock for static multiline string

I don’t like this idea. You say we are decoupling the charge, but in reality, it doesn’t happen. Just because you are using a workaround to use native SQL queries it still means the responsibility remains here: Fetch the ChargeData via the charge service (the interface can and the DTO can remain in the accounting module, while the implementation can go into the charge module).

Let’s decide whether the accounting module depends on the charge or the charge depends on the accounting. One of them must, because there is common logic.

First thing that jumps at you is the string concatenation (just to confirm @Aman-Mittal 's observation). Even if that particular snippet doesn't seem to contain any of the usual SQL injection issues... at the very least we should use multiline strings (still beware of any use of unchecked variables!); if it's a simple query then use Spring Data JDBC repository functions (findByXXX).

@vidakovic Got it

Lets not use native sql queries and jdbctemplate calls. Whenever a module needs information from an another module, have a service for that.

@adamsaghy I agree. The native SQL moves the coupling to the schema rather than removing it.
'financialAccountType' already distinguishes these on our own row, and the update path in 'ProductToGLAccountMappingHelper' already uses it so the read path can too, no charge navigation. Only the charge name needs the charge module.
I'd propose charge depends on accounting, interface in accounting, impl in charge (ChargeData is already in core). Is this right to proceed with?

adamsaghy · 2026-07-13T09:16:52Z

+        // The accounting module no longer loads the Charge entity (it is decoupled from the charge domain), so the
+        // existence check that loading the entity used to provide is restored here with a lightweight JDBC lookup.
+        // This keeps acc_product_mapping.charge_id from persisting orphaned references without reintroducing a
+        // compile-time dependency on the charge module.
        // TODO Vishwas: Need to validate if given charge is fee or Penalty
        // based on input condition
+        final Integer chargeCount = this.jdbcTemplate.queryForObject("select count(*) from m_charge where id = ?", Integer.class, chargeId);
+        if (chargeCount == null || chargeCount == 0) {
+            throw new ResourceNotFoundException("error.msg.charge.id.invalid", "Charge with identifier " + chargeId + " does not exist",
+                    new Object[] { chargeId });
+        }



I don’t believe we need this validation here. Before providing the chargeId to this helper, we should validate it. We shouldn’t mix the responsibilities here.

I'll remove it. Charge id validation belongs upstream, before the id reaches the mapping helper.

adamsaghy

Please review my concerns and lets find a different approach...

vidakovic · 2026-07-13T13:25:34Z

Just a general suggestion: maybe let's go easy with the use of Copilot here... I don't think that the code base is in any shape to have predictable results here (which is anyway not really possible with these tools). And especially with this task (defining proper domain boundaries) I think Copilot might not be that useful, because we have to surgically make the needed changes.

vidakovic · 2026-07-13T13:29:13Z

            final PortfolioProductType portfolioProductType, final boolean isPenalty) {
-        final Charge charge = this.chargeRepositoryWrapper.findOneWithNotFoundDetection(chargeId);

+        // The accounting module no longer loads the Charge entity (it is decoupled from the charge domain), so the


Is this something inserted by Copilot? If not: the code itself is maybe not the proper place to put documentation like this. I know, it's there are not really hard criteria on how much you should or could comment, but this here looks rather large and in fact the changed code should speak for itself... if it doesn't then maybe something is not entirely clear in general.

vidakovic · 2026-07-13T13:30:04Z

-            chargeToGLAccountMappers.add(chargeToGLAccountMapper);
-        }
-        return chargeToGLAccountMappers;
+        // The accounting module is decoupled from the charge domain, so we cannot navigate a Charge association in


Same here, see comment about lengthy explanations in sources.

vidakovic · 2026-07-13T13:33:15Z

+            }
+        });
+
+        LOG.info("==== Accounting cross-feature dependency report ====");


Use Lombok annotation @Slf4j... these uppercase LOG.xxx(...) will become lowercase log.xxx(...). Just to stay consistent with the rest of the codebase.

vidakovic · 2026-07-13T13:38:00Z

 * under the License.
 */
-
+ext['spring-modulith.version'] = '1.4.11'


We should probably put this (dependency management) where we manage all dependencies: see https://github.com/apache/fineract/blob/develop/buildSrc/src/main/groovy/org.apache.fineract.dependencies.gradle

FINERACT-2646: Decouple accounting from charge via chargeId reference

39f6ef1

Copilot AI review requested due to automatic review settings July 8, 2026 19:58

Copilot started reviewing on behalf of mansi75 July 8, 2026 19:59 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

mansi75 added 4 commits July 9, 2026 01:57

FINERACT-2646: fix StringSplitter error prone failure in boundry test

66c1e06

FINERACT-2646: Address review feedback on accounting/charge decoupling

3fa6264

FINERACT-2646: fix SLF4J SpotBugs violations in accounting boundary test

a1dce7f

FINERACT-2646: Auto enable report test based on the flag

84fa296

mansi75 force-pushed the FINERACT-2646-decouple-accounting branch from fd16395 to 84fa296 Compare July 9, 2026 20:06

Aman-Mittal reviewed Jul 13, 2026

View reviewed changes

adamsaghy reviewed Jul 13, 2026

View reviewed changes

adamsaghy requested changes Jul 13, 2026

View reviewed changes

vidakovic reviewed Jul 13, 2026

View reviewed changes

Uh oh!

Conversation

mansi75 commented Jul 8, 2026

Description

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

vidakovic commented Jul 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants