fix: Filter pra types to get DE and TEA [DHIS2-17243]

[enricocolasante]

I am asking @dhis2/platform-backend review because I am changing a store method and using nativeSynchronizedQuery

[david-mackessy]

Hi @enricocolasante, one of the concerns about using the native query, and it's mentioned in the javadoc, is that it should only target the same table of the store, in this case programrule.
Because these 2 updated queries use joins (dataelement & trakedentityattribute), and also the main select is to programruleaction, my understanding is that this will cause Hibernate to flush all DataElement, TrackedEntityAttribute & ProgramRuleAction from the cache before executing the query. This could potentially have negative impact on the query.
If there was a performance test for this endpoint it would help guide a decision.

[enricocolasante]

Hi @enricocolasante, one of the concerns about using the native query, and it's mentioned in the javadoc, is that it should only target the same table of the store, in this case programrule. Because these 2 updated queries use joins (dataelement & trakedentityattribute), and also the main select is to programruleaction, my understanding is that this will cause Hibernate to flush all DataElement, TrackedEntityAttribute & ProgramRuleAction from the cache before executing the query. This could potentially have negative impact on the query. If there was a performance test for this endpoint it would help guide a decision.

Changed to use nativeQuery

[david-mackessy]

Hi @enricocolasante, one of the concerns about using the native query, and it's mentioned in the javadoc, is that it should only target the same table of the store, in this case programrule. Because these 2 updated queries use joins (dataelement & trakedentityattribute), and also the main select is to programruleaction, my understanding is that this will cause Hibernate to flush all DataElement, TrackedEntityAttribute & ProgramRuleAction from the cache before executing the query. This could potentially have negative impact on the query. If there was a performance test for this endpoint it would help guide a decision.

Changed to use nativeQuery

This still hits the native query force cache issue, and is probably worse now. Is there a specific reason to use a native query?
Would using this query method work instead?
So it would look something like this:

return getQuery(sql, String.class)
        .setParameter("types", serverSupportedTypes)
        .getResultList();

Using that method does not hit the Hibernate cache issue as Hibernate is fully aware of the objects involved in the query. So no flush is triggered.

[david-mackessy]

Also, any fix should usually be accompanied with a test, proving the absence of the bug being fixed.

[jbee]

FYI: If you look at the implementation of nativeSynchronizedQuery it might become more clear what the "synchronized" part entails. It adds the table of the store as a table to "synchronize" with, meaning "make sure all changes affecting that table are flushed and caches cleared". If you use a join the proper usage for a native query is to add the joined tables to the query as well as those tables also need to be "in sync" to give you the results you want. At least most likely this is what you want.

As David pointed out, if you use HQL that part is done for you. But there are other reasons to prefer SQL over HQL so I also think it is wrong to advocate for HQL purely because hibernate is asking you to do this manually. One could even get the idea they did that on purpose to make users dependent. Because they could have easily offered an option to run the SQL on the same basis as the HQL. It is not that this would be hard to figure out based on the hibernate mappings.

[enricocolasante]

Thanks for the comments, it was a quite confusing PR as it is extracted from a bigger one just to try to make it smaller but it was very clarifying for me. I thought I was confused about the different query option that we have but I found out that I was more than expected.
I just went with @david-mackessy suggestion on this one and I am using hql and everything is working as expected

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[david-mackessy]

thanks for adding the tests :)