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

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.

…ingWritePlatformServiceImpl

adamsaghy · 2026-07-16T15:52:48Z

        mavenBom 'io.github.resilience4j:resilience4j-bom:2.4.0'
        mavenBom 'org.testcontainers:testcontainers-bom:1.21.4'
        mavenBom 'org.glassfish.jersey:jersey-bom:3.1.11'
+        mavenBom 'org.springframework.modulith:spring-modulith-bom:1.4.11'


Is this relevant here?

It's only here to pin the spring-modulith-core version used by AccountingCrossFeatureBoundaryTest. If we drop that test (per your other comment), I'll remove this BOM entry too.

adamsaghy · 2026-07-16T15:53:24Z

+
+public interface AccountingChargeReadService {
+
+    List<ChargeData> findChargesByIds(Collection<Long> chargeIds);


Use List or Set, no Collection

adamsaghy · 2026-07-16T15:59:53Z

+import org.springframework.modulith.core.ApplicationModuleDependency;
+import org.springframework.modulith.core.ApplicationModules;
+
+class AccountingCrossFeatureBoundaryTest {


I am not too fond of this idea...

Any reason we want to do this?

The intent was to assert at the module level that fineract-accounting no longer reaches into other modules after this decoupling. But AllModulesCrossFeatureBoundaryTest (FINERACT-2645) already covers cross-feature boundaries globally, so we can drop this if you want. Let me know if you'd prefer that.

adamsaghy · 2026-07-16T16:00:16Z


 apply from: 'dependencies.gradle'

+test {


Any reason for this?

That block only configures the run for the new boundary test. It comes out along with AccountingCrossFeatureBoundaryTest if we drop it.

adamsaghy · 2026-07-16T16:00:37Z

    implementation(project(path: ':fineract-core'))
-    implementation(project(path: ':fineract-charge'))
+    testImplementation 'org.springframework.modulith:spring-modulith-core'
+    testImplementation("org.osgi:org.osgi.core:6.0.0")


Test-only. Spring-modulith-core's module scanning fails at runtime without an OSGi class on the classpath, so it's needed to run the boundary test.

adamsaghy · 2026-07-16T16:01:01Z

+    private final ChargeRepository chargeRepository;
+
+    @Override
+    public List<ChargeData> findChargesByIds(final Collection<Long> chargeIds) {


Dont use Collection. Its either a List or Set

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 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

Comment thread ...che/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java Outdated

adamsaghy requested changes Jul 13, 2026

View reviewed changes

vidakovic reviewed Jul 13, 2026

View reviewed changes

Comment thread ...che/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java Outdated

vidakovic reviewed Jul 13, 2026

View reviewed changes

Comment thread ...unting/producttoaccountmapping/service/ProductToGLAccountMappingReadPlatformServiceImpl.java Outdated

vidakovic reviewed Jul 13, 2026

View reviewed changes

Comment thread ...ounting/src/test/java/org/apache/fineract/accounting/AccountingCrossFeatureBoundaryTest.java

vidakovic reviewed Jul 13, 2026

View reviewed changes

Comment thread fineract-accounting/dependencies.gradle Outdated

mansi75 added 6 commits July 15, 2026 16:51

FINERACT-2646: Decouple accounting from charge via chargeId reference

62c71a7

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

ef40dd6

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

f13a015

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

cf2ddee

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

c0cfbdf

FINERACT-2646: Decouple accounting from charge via read contract

c12cd0f

mansi75 force-pushed the FINERACT-2646-decouple-accounting branch from 84fa296 to c12cd0f Compare July 15, 2026 11:33

FINERACT-2646: Fix Spotless import ordering in ProductToGLAccountMapp…

a47b44a

…ingWritePlatformServiceImpl

adamsaghy reviewed Jul 16, 2026

View reviewed changes

Comment thread ...unting/producttoaccountmapping/service/ProductToGLAccountMappingReadPlatformServiceImpl.java

adamsaghy reviewed Jul 16, 2026

View reviewed changes

Comment thread ...g/producttoaccountmapping/service/WorkingCapitalLoanProductAdvancedAccountingReadHelper.java

adamsaghy reviewed Jul 16, 2026

View reviewed changes


		public interface AccountingChargeReadService {

		List<ChargeData> findChargesByIds(Collection<Long> chargeIds);

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!

Uh oh!

adamsaghy left a comment

Choose a reason for hiding this comment

Uh oh!

vidakovic commented Jul 13, 2026

Uh oh!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants