Copilot commented on code in PR #6106:
URL: https://github.com/apache/fineract/pull/6106#discussion_r3546919607
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingHelper.java:
##########
@@ -613,7 +610,6 @@ private void savePaymentChannelToFundSourceMapping(final
Long productId, final L
*/
private void saveChargeToFundSourceMapping(final Long productId, final
Long chargeId, final Long incomeAccountId,
final PortfolioProductType portfolioProductType, final boolean
isPenalty) {
- final Charge charge =
this.chargeRepositoryWrapper.findOneWithNotFoundDetection(chargeId);
// TODO Vishwas: Need to validate if given charge is fee or Penalty
// based on input condition
Review Comment:
`saveChargeToFundSourceMapping` no longer verifies that the provided
`chargeId` exists (previously this was implicitly validated by loading the
`Charge` entity). Because `acc_product_mapping.charge_id` has no FK constraint
to `m_charge`, this can persist orphaned mappings that will never be returned
by the join-based read queries, and may lead to confusing downstream behavior.
Consider validating `chargeId` (and optionally `m_charge.is_penalty` vs
`isPenalty`) via a lightweight JDBC lookup, or by delegating to an existing
charge read contract that does not reintroduce a domain dependency.
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingReadPlatformServiceImpl.java:
##########
@@ -257,16 +262,27 @@ public List<ChargeToGLAccountMapper>
fetchPenaltyToIncomeAccountMappingsForSavin
private List<ChargeToGLAccountMapper>
fetchChargeToIncomeAccountMappings(final PortfolioProductType
portfolioProductType,
final Long loanProductId, final boolean penalty) {
- final List<ProductToGLAccountMapping> mappings = penalty
- ?
productToGLAccountMappingRepository.findAllPenaltyMappings(loanProductId,
portfolioProductType.getValue())
- :
productToGLAccountMappingRepository.findAllFeeMappings(loanProductId,
portfolioProductType.getValue());
-
- List<ChargeToGLAccountMapper> chargeToGLAccountMappers =
mappings.isEmpty() ? null : new ArrayList<>();
- for (final ProductToGLAccountMapping mapping : mappings) {
- final GLAccountData gLAccountData = new
GLAccountData().setId(mapping.getGlAccount().getId())
-
.setName(mapping.getGlAccount().getName()).setGlCode(mapping.getGlAccount().getGlCode());
- final ChargeData chargeData =
ChargeData.builder().id(mapping.getCharge().getId()).name(mapping.getCharge().getName())
- .penalty(mapping.getCharge().isPenalty()).build();
+ // The accounting module is decoupled from the charge domain, so we
cannot navigate a Charge association in
+ // JPQL. We read the charge -> income-account rows via JdbcTemplate
instead of a JPA native query: EclipseLink
+ // does not bind Spring Data ":name" parameters in native queries (it
expects "#name"), which caused the raw
+ // markers to reach the database. JdbcTemplate uses plain JDBC
positional binding and works on all supported
+ // databases. 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.
Review Comment:
This JDBC implementation does an extra JPA lookup per row
(`glAccountRepository.findById` inside the loop), creating an N+1 query pattern
and additional transaction overhead. Since all required GL account fields come
from `acc_gl_account`, consider selecting the GL account name/code directly in
the SQL and mapping straight to `GLAccountData`; this also lets you bind the
`penalty` flag as a parameter instead of concatenating it into the SQL string.
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/ProductToGLAccountMappingReadPlatformServiceImpl.java:
##########
@@ -312,6 +328,7 @@ private List<ClassificationToGLAccountData>
fetchClassificationMappings(final Po
:
productToGLAccountMappingRepository.findAllBuyDownFeeClassificationsMappings(loanProductId,
portfolioProductType.getValue());
+
productToGLAccountMappingRepository.findAllChargeOffReasonsMappings(loanProductId,
portfolioProductType.getValue());
Review Comment:
This call to `findAllChargeOffReasonsMappings(...)` is unused and triggers
an extra database query on every classification mapping fetch. It looks like a
leftover line and should be removed to avoid unnecessary load and confusion.
##########
fineract-accounting/dependencies.gradle:
##########
@@ -25,6 +25,7 @@ dependencies {
// implementation dependencies are directly used (compiled against) in
src/main (and src/test)
//
implementation(project(path: ':fineract-core'))
+ testImplementation 'org.springframework.modulith:spring-modulith-core'
implementation(project(path: ':fineract-charge'))
Review Comment:
After the code changes in this PR, `fineract-accounting` no longer imports
any types from the `fineract-charge` module (only `ChargeData`, which is in
`fineract-core`). Keeping `implementation(project(path: ':fineract-charge'))`
preserves a build-time coupling that appears contrary to the PR’s stated goal
of decoupling accounting from charge. If nothing else in this module depends on
`:fineract-charge`, consider removing this dependency.
##########
fineract-accounting/src/main/java/org/apache/fineract/accounting/producttoaccountmapping/service/WorkingCapitalLoanProductAdvancedAccountingReadHelper.java:
##########
@@ -74,17 +79,26 @@ public List<AdvancedMappingToExpenseAccountData>
fetchWriteOffReasonMappings(fin
}
private List<ChargeToGLAccountMapper> fetchChargeToIncomeMappings(final
Long wcLoanProductId, final boolean penalty) {
- final List<ProductToGLAccountMapping> mappings = penalty
- ?
productToGLAccountMappingRepository.findAllPenaltyMappings(wcLoanProductId,
- PortfolioProductType.WORKING_CAPITAL_LOAN.getValue())
- :
productToGLAccountMappingRepository.findAllFeeMappings(wcLoanProductId,
- PortfolioProductType.WORKING_CAPITAL_LOAN.getValue());
+ // 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.
Review Comment:
Same as in `ProductToGLAccountMappingReadPlatformServiceImpl`: this method
performs an extra JPA lookup per mapping row (`glAccountRepository.findById`),
which can become an N+1 pattern. Prefer selecting the needed GL account fields
directly in the SQL (joining `acc_gl_account`) and mapping straight to
`GLAccountData`; also bind the `penalty` flag as a JDBC parameter instead of
concatenating it.
##########
fineract-accounting/build.gradle:
##########
@@ -48,6 +48,11 @@ configurations {
apply from: 'dependencies.gradle'
+test {
+ maxHeapSize = '2g'
+ jvmArgs += ['-XX:MaxMetaspaceSize=1g']
+}
Review Comment:
This increases the JVM limits for *all* accounting tests (`maxHeapSize =
'2g'` and `MaxMetaspaceSize=1g`). If this was added solely to support the new
Modulith/ArchUnit boundary test, consider scoping the higher memory settings to
just that test (e.g., a dedicated Gradle `Test` task or a property-controlled
switch) to avoid inflating resource usage across the entire test suite and on
developer machines.
--
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]
