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

mariiaKraievska · 2024-04-08T18:34:40Z

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.

…s-account-services # Conflicts: # build.gradle # fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanTransaction.java # fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/reaging/LoanReAgeParameter.java # fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/transactionprocessor/impl/AdvancedPaymentScheduleTransactionProcessor.java # fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanApplicationWritePlatformServiceJpaRepositoryImpl.java # fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java # fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/reaging/LoanReAgingServiceImpl.java # fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/starter/LoanAccountConfiguration.java # fineract-provider/src/main/resources/db/changelog/tenant/changelog-tenant.xml # integration-tests/dependencies.gradle # integration-tests/src/test/java/org/apache/fineract/integrationtests/AdvancedPaymentAllocationLoanRepaymentScheduleTest.java # integration-tests/src/test/java/org/apache/fineract/integrationtests/BaseLoanIntegrationTest.java # integration-tests/src/test/java/org/apache/fineract/integrationtests/FixedLengthLoanProductIntegrationTest.java # integration-tests/src/test/java/org/apache/fineract/integrationtests/LoanRepaymentScheduleWithDownPaymentTest.java # integration-tests/src/test/java/org/apache/fineract/integrationtests/loan/reaging/LoanReAgingIntegrationTest.java # integration-tests/src/test/java/org/apache/fineract/integrationtests/loan/reamortization/LoanReAmortizationIntegrationTest.java # oauth2-tests/dependencies.gradle # twofactor-tests/dependencies.gradle

vidakovic · 2024-04-09T07:27:25Z

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

adamsaghy · 2024-04-10T09:48:57Z

...t/portfolio/account/jobs/executestandinginstructions/ExecuteStandingInstructionsTasklet.java

@@ -113,8 +120,12 @@ public RepeatStatus execute(StepContribution contribution, ChunkContext chunkCon
                final boolean transferCompleted = transferAmount(errors, accountTransferDTO, data.getId());

                if (transferCompleted) {
-                    final String updateQuery = "UPDATE m_account_transfer_standing_instructions SET last_run_date = ? where id = ?";
-                    jdbcTemplate.update(updateQuery, transactionDate, data.getId());
+                    final JPAQueryFactory queryFactory = new JPAQueryFactory(this.entityManager);


I would move this whole logic into an AccountTransferStandingInstruction repository. We can change our repository interfaces to an impl where queryDSL is used.

adamsaghy · 2024-04-10T09:52:15Z

...t/portfolio/account/jobs/executestandinginstructions/ExecuteStandingInstructionsTasklet.java

+        newHistory.setErrorLog(String.valueOf(errorLog));
+
+        entityManager.persist(newHistory);
+        entityManager.flush();


I dont think we need explicit flush.

You right, done

adamsaghy · 2024-04-10T09:52:49Z

...t/portfolio/account/jobs/executestandinginstructions/ExecuteStandingInstructionsTasklet.java

-        updateQuery.append("'").append(errorLog).append("')");
-        jdbcTemplate.update(updateQuery.toString());
+
+        final AccountTransferStandingInstructionsHistory newHistory = new AccountTransferStandingInstructionsHistory();


I would move this whole logic into a repository

adamsaghy · 2024-04-10T11:30:10Z

...rg/apache/fineract/portfolio/account/service/AccountAssociationsReadPlatformServiceImpl.java


+    @Transactional(propagation = Propagation.REQUIRES_NEW)


I dont really understand the new transaction for reading... I believe you are referring to Optimistic read locking but I am afraid you are failing to sort the explained problem by this.

If I understand the problem you are explaining in the description of the PR is the following:
Transaction isolation= READ COMMITTED
TX1 started

Read Account (by name) -> Account (id=1, name=Adam, version=1, ref=1234)
TX2 started

Read Account (by id) -> Account (id=1, name=Adam, version=1, ref=1234)

Modify Account name -> Account (id=1, name=Josh, version=2, ref=3456)
TX2 ended
TX1:

Read Accounts -> [Account(id=1, name=Josh, version=2)] -> JPA realize with the same ID a modified entry it has in its persistence context, so it is not in sync with the DB anymore and trigger Optimistic lock!

Imagine if you are reading the entity in new transactions when the read completed, the read entity became detached and removed from the persistence context and the JPA optimistic locking would only be triggered if this detached entity to be written back to the DB, but not when it was read again...

What do you think?

Thank you for your comments and concerns regarding the use of @transactional with Propagation.REQUIRES_NEW for read-only operations. Initially, this approach was adopted to manage specific issues with OptimisticLockException observed when using QueryDSL instead of JdbcTemplate. However, improvements made in the handling of OptimisticLockException by the recent updates #3761 have resolved these issues effectively. As a result, the additional transactional annotation is no longer necessary, and our tests confirm that the system performs well without it. I appreciate your input and agree that adhering to these updated practices ensures cleaner and more efficient code.

adamsaghy · 2024-04-10T11:30:49Z

...rg/apache/fineract/portfolio/account/service/AccountAssociationsReadPlatformServiceImpl.java

-            }
-        } catch (final EmptyResultDataAccessException e) {
-            log.debug("Linking account is not configured");
+        final QAccountAssociations qAccountAssociations = QAccountAssociations.accountAssociations;


It would be better to have for this a Repository

adamsaghy

I am really happy to see we are moving away from building queries "manually" toward QueryDSL!!

In the other hand please take a look on my review comments and let me know about your thoughts!

…ES_NEW), add custom repositories

vidakovic · 2024-05-01T13:21:46Z

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

infobip/infobip-spring-data-querydsl#101
OpenFeign/querydsl#416

…-sql-queries-account-services # Conflicts: # fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/AccountTransfersReadPlatformServiceImpl.java # fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/StandingInstructionHistoryReadPlatformServiceImpl.java # fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/StandingInstructionReadPlatformServiceImpl.java

adamsaghy · 2024-05-07T09:52:55Z

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

…-sql-queries-account-services

adamsaghy · 2024-05-16T11:37:47Z

@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 · 2024-05-23T09:44:36Z

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

borikas · 2024-06-04T11:48:12Z

@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 · 2024-06-04T11:52:37Z

@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 · 2024-06-04T12:40:52Z

@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 · 2024-06-16T21:49:36Z

@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 · 2024-06-17T09:17:19Z

@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 · 2024-06-17T21:20:33Z

@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

mariiaKraievska added 2 commits April 8, 2024 21:13

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

9047c88

vidakovic self-requested a review April 9, 2024 07:26

FINERACT-2022: fix format in LoanAccountConfiguration

6b376c1

adamsaghy reviewed Apr 10, 2024

View reviewed changes

adamsaghy requested changes Apr 10, 2024

View reviewed changes

FINERACT-2022: remove @transactional(propagation = Propagation.REQUIR…

c267bfd

…ES_NEW), add custom repositories

mariiaKraievska added 3 commits May 13, 2024 15:10

FINERACT-2022: use io.github.openfeign instead of com.querydsl

a260e28

Merge branch 'refs/heads/develop' into FINERACT-2022/type-safe-native…

8116875

…-sql-queries-account-services

FINERACT-2022: fix check-style issues

3eee9db

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mariiaKraievska commented Apr 8, 2024 •

edited

Loading

vidakovic commented Apr 9, 2024

adamsaghy Apr 10, 2024

mariiaKraievska Apr 30, 2024

adamsaghy Apr 10, 2024

mariiaKraievska Apr 30, 2024

adamsaghy Apr 10, 2024

mariiaKraievska Apr 30, 2024

adamsaghy Apr 10, 2024 •

edited

Loading

mariiaKraievska Apr 30, 2024

adamsaghy Apr 10, 2024

mariiaKraievska Apr 30, 2024

adamsaghy left a comment

vidakovic commented May 1, 2024

adamsaghy commented May 7, 2024 •

edited

Loading

adamsaghy commented May 16, 2024

mariiaKraievska commented May 23, 2024

borikas commented Jun 4, 2024

adamsaghy commented Jun 4, 2024

borikas commented Jun 4, 2024

mariiaKraievska commented Jun 16, 2024

adamsaghy commented Jun 17, 2024 •

edited

Loading

vidakovic commented Jun 17, 2024

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

Are you sure you want to change the base?

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

Conversation

mariiaKraievska commented Apr 8, 2024 • edited Loading

Description

Checklist

vidakovic commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsaghy Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsaghy left a comment

Choose a reason for hiding this comment

vidakovic commented May 1, 2024

adamsaghy commented May 7, 2024 • edited Loading

adamsaghy commented May 16, 2024

mariiaKraievska commented May 23, 2024

borikas commented Jun 4, 2024

adamsaghy commented Jun 4, 2024

borikas commented Jun 4, 2024

mariiaKraievska commented Jun 16, 2024

adamsaghy commented Jun 17, 2024 • edited Loading

vidakovic commented Jun 17, 2024

mariiaKraievska commented Apr 8, 2024 •

edited

Loading

adamsaghy Apr 10, 2024 •

edited

Loading

adamsaghy commented May 7, 2024 •

edited

Loading

adamsaghy commented Jun 17, 2024 •

edited

Loading