FINERACT-2022: Rewrite account domain, services and jobs using QueryDSL.

[mariiaKraievska]

Description

JIRA ticket - https://issues.apache.org/jira/browse/FINERACT-2022

In this PR, I implemented QueryDSL to generate code for type-safe SQL queries for the Account entity. I used QueryDSL for complex JPA queries, removed all custom "@Query" annotations from repository classes, and refused to use JdbcTemplate.

I'd like to explain the reason for using the @Transactional(propagation = Propagation.REQUIRES_NEW) annotation for methods that use QueryDSL for "select" queries.

Previously, using JdbcTemplate to work with the database, we did not catch errors related to optimistic locking. This was due to the fact that JdbcTemplate works directly with SQL and does not take into account the optimistic locking mechanisms built into JPA. But they were still present and it could be seen in the logs.

After switching to QueryDSL, we started to face OptimisticLockException on read operations. This indicated that we had concurrent changes in the database and these changes were being checked at the time of the read requests, which are now performed via JPA. To avoid such cases and ensure data consistency, I applied the REQUIRES_NEW annotation. This allows each read request to be executed in a new transaction, isolating it from other operations that may be modifying the data at the same time.

Please let me know if you have any questions or suggestions.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests

• [ ] 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 coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/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

• [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

[vidakovic]

@mariiaKraievska ... I'll check it out as soon as possible... will ping you... FYI

[vidakovic]

Just a quick note... QueryDSL (the project) seems to have hit a bit of a slow down due to members occupied with work and studies... the good news: it seems that the people from OpenFeign jumped in and already made quite some significant improvements. Also: there is also a better way to generate the metadata sources with the Infobip extensions.

I have contributed some improvements to both projects and hope they get accepted. The one for the Infobip extensions is a fairly small one, just trying to convince them to switch from com.querydsl:* to io.github.openfeign.querydsl:*; package names etc. are all the same.

The other PR on OpenFeign's QueryDSL fork is not relevant for what we need, just supporting a newer geo library (JTS) and the latest Postgis driver extensions.

The PR that would be interesting for us is the Infobip one (replacing com.querydsl:* to io.github.openfeign.querydsl:*).

FYI

https://github.com/infobip/infobip-spring-data-querydsl/pull/101 https://github.com/OpenFeign/querydsl/pull/416

[adamsaghy]

@mariiaKraievska Please kindly check @vidakovic recommendation and resolve the conflicting files and squash the commits! Thank you! ;)

[adamsaghy]

@mariiaKraievska Kindly asking you to squash your commits and resolve the conflicts. Feel free to reach out if you are facing any issues with that.

[mariiaKraievska]

@adamsaghy I'm sorry for the delay. (I thought I could quickly deal with the problem and then write back, but no). I'm a bit stuck with these two tests right now: ClientLoanIntegrationTest - testLoanCharges_INSTALLATION_FEE. SchedulerJobsTestResults - testInterestTransferForSavings.

They started to fail for me after this commit https://github.com/apache/fineract/pull/3854/commits/c267bfd61902c9313c2a1c4dc5f598104d5c3188, they do not pass without an annotation @Transactional(propagation = Propagation.REQUIRES_NEW) The first one does not pass without this annotation on the fetchPostInterestTransactionIds method in the AccountTransfersReadPlatformServiceImpl class. And the second without it on the retrieveLoanLinkedAssociation method in the AccountAssociationsCustomRepositoryImpl class.

I will work further and try to solve this ASAP.

[borikas]

@adamsaghy I'm sorry for the delay. (I thought I could quickly deal with the problem and then write back, but no). I'm a bit stuck with these two tests right now: ClientLoanIntegrationTest - testLoanCharges_INSTALLATION_FEE. SchedulerJobsTestResults - testInterestTransferForSavings.

They started to fail for me after this commit c267bfd, they do not pass without an annotation @transactional(propagation = Propagation.REQUIRES_NEW) The first one does not pass without this annotation on the fetchPostInterestTransactionIds method in the AccountTransfersReadPlatformServiceImpl class. And the second without it on the retrieveLoanLinkedAssociation method in the AccountAssociationsCustomRepositoryImpl class.

I will work further and try to solve this ASAP.

@adamsaghy It looks like we have a problem with caching sql queries when migrating from jdbcTemplate to queryDSL, because queryDSL uses EntityManager Cache, at the same time as jdbcTemplate uses Spring Cache.

[adamsaghy]

@adamsaghy I'm sorry for the delay. (I thought I could quickly deal with the problem and then write back, but no). I'm a bit stuck with these two tests right now: ClientLoanIntegrationTest - testLoanCharges_INSTALLATION_FEE. SchedulerJobsTestResults - testInterestTransferForSavings. They started to fail for me after this commit c267bfd, they do not pass without an annotation @transactional(propagation = Propagation.REQUIRES_NEW) The first one does not pass without this annotation on the fetchPostInterestTransactionIds method in the AccountTransfersReadPlatformServiceImpl class. And the second without it on the retrieveLoanLinkedAssociation method in the AccountAssociationsCustomRepositoryImpl class. I will work further and try to solve this ASAP.

@adamsaghy It looks like we have a problem with caching sql queries when migrating from jdbcTemplate to queryDSL, because queryDSL uses EntityManager Cache, at the same time as jdbcTemplate uses Spring Cache.

I might not yet fully understand the issue. Are we talking about the prepared statement cache? Or the entity cache, rather when something was already fetched, it does not go to the database to fetch again?

[borikas]

@adamsaghy I'm sorry for the delay. (I thought I could quickly deal with the problem and then write back, but no). I'm a bit stuck with these two tests right now: ClientLoanIntegrationTest - testLoanCharges_INSTALLATION_FEE. SchedulerJobsTestResults - testInterestTransferForSavings. They started to fail for me after this commit c267bfd, they do not pass without an annotation @transactional(propagation = Propagation.REQUIRES_NEW) The first one does not pass without this annotation on the fetchPostInterestTransactionIds method in the AccountTransfersReadPlatformServiceImpl class. And the second without it on the retrieveLoanLinkedAssociation method in the AccountAssociationsCustomRepositoryImpl class. I will work further and try to solve this ASAP.

@adamsaghy It looks like we have a problem with caching sql queries when migrating from jdbcTemplate to queryDSL, because queryDSL uses EntityManager Cache, at the same time as jdbcTemplate uses Spring Cache.

I might not yet fully understand the issue. Are we talking about the prepared statement cache? Or the entity cache, rather when something was already fetched, it does not go to the database to fetch again?

This problem is similar to an incorrect sequence of sql queries within the same transaction

[mariiaKraievska]

@adamsaghy Related to the problem of having in the same transaction queries that use jdbcTemplate and new, rewritten queries that use QueryDSL'. I propose to go the way of rewriting within transactions or as now within modules (as in this PR account domain), but duplicating methods related to other modules, written with jdbcTemplate, rewriting them with QueryDSL`. Or the third option, which I am inclined to and have already started working on, is to take another module, more independent from other modules, and present PR on its example. What do you think?

[adamsaghy]

@adamsaghy Related to the problem of having in the same transaction queries that use jdbcTemplate and new, rewritten queries that use QueryDSL'. I propose to go the way of rewriting within transactions or as now within modules (as in this PR account domain), but duplicating methods related to other modules, written with jdbcTemplate, rewriting them with QueryDSL`. Or the third option, which I am inclined to and have already started working on, is to take another module, more independent from other modules, and present PR on its example. What do you think?

I like the idea to do it within module boundaries in this case. I wont mind small steps towards the final goal ;)

@vidakovic Thoughts?

[vidakovic]

@mariiaKraievska ... maybe some inspiration: https://github.com/vidakovic/fineract/tree/feature/APACHE_EU_2024/custom/apachecon/note ... if you want we can talk this through

[github-actions[bot]]

This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.