Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TRUNK-6202 Replace Hibernate Criteria API with JPA for HibernateProviderDAO #4501

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

k4pran
Copy link
Contributor

@k4pran k4pran commented Dec 20, 2023

Description of what I changed

This task is to migrate the deprecated hibernate criteria to jpa criteria for the HibernateProviderDAO class. This class had a little more complexity than previous migrations and could do with a closer look during the review.

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-6202

Checklist: I completed these to help reviewers :)

  • My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)

    No? -> write tests and add them to this commit git add . && git commit --amend

  • I ran mvn clean package right before creating this pull request and
    added all formatting changes to my commit.

    No? -> execute above command

  • All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

  • My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@k4pran k4pran force-pushed the TRUNK-6202-HibernateProviderDAO branch from d85eda4 to efc1bc2 Compare December 20, 2023 15:16
@k4pran k4pran marked this pull request as ready for review December 22, 2023 13:32
if (!includeRetired) {
criteria.add(Restrictions.eq("retired", false));
cq.where(personPredicate, cb.isFalse(root.get("retired")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't the order by clause outside of the if (!includeRetired)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, but the logic should be the same, I just restructured it a little to avoid new lists. I updated the code though to make it closer to the original structure

@k4pran k4pran force-pushed the TRUNK-6202-HibernateProviderDAO branch from efc1bc2 to 73e56bb Compare December 31, 2023 09:27
CriteriaQuery<Provider> cq = cb.createQuery(Provider.class);
Root<Provider> root = cq.from(Provider.class);

List<Predicate> predicates = prepareProviderCriteria(cb, root, name, includeRetired);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an easy way of doing distinct(true) from within prepareProviderCriteria?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is while keeping the method flexible enough for reuse. In some of the first migrations I tried doing it by passing the CriteriaQuery object into the method to allow me to do this, but then when we want to do certain things outside the method like counts it doesn't work because the query object specifies the return type of the result

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK 😊

@dkayiwa dkayiwa merged commit 1e86f5f into openmrs:master Jan 3, 2024
6 checks passed
Wandji69 pushed a commit to Wandji69/openmrs-core that referenced this pull request Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants