use HibernateOpenmrsObjectDAO in HibernateDAOs

@lluismf showed me HibernateOpenmrsObjectDAO which I didnt know existed :slight_smile:

(see the discussion from replace occurences of vector with arraylist: )

he suggested to

Make all the Hibernate*DAO extend HibernateOpenmrsObjectDAO Remove all the sessionFactory.getCurrentSession() prefixes. Invoke get/saveOrUpdate/… of the parent. Remove the sessionFactory instance variable because it won’t be used anymore

I just wanted some more discussion on this. Why was this class introduced and not used? Was it a requirement from a module?

Do you also think this should be done? Any thoughts?

It’s intended to expose a generic DAO which can be used to do basic CRUD operations for just about any persistent domain object, this would help to reduce a lot of the duplicated DAO code which would also speed up module development for modules that require a DAO.

In theory, something like HibernateLocationDAO wouldn’t need to have CRUD operation methods like saveLocation(Location), getLocation(Integer), getLocationByUuid(String), getAllLocations(), deleteLocation(Location) meaning the relevant LocationServiceImpl methods would instead delegate to the appropriate methods on the HiberateOpenmrsObjectDAO

1 Like

We probably need to clean up our existing DAOs to use it, probably not big of a priority but and for sure going forward future services that have DAOs will have to use it.

The short answer is that it was introduced so we could refactor and standardize the codebase, but then we got derailed by thinking about whether an even huger refactor to Spring Data would be better, and the effort died.

The full history is in TRUNK-1824 and TRUNK-3782.

My opinion is that we should ignore the Spring Data option, and do the straightforward refactoring/standardization work based on HibernateOpenmrsObjectDAO. (It’s possible that the already-existing sub-tickets of TRUNK-1824 cover this.) It seems like a nicely-parallelizable stream of work for a hackathon.


I agree (I started working on this a few years ago).

BTW having the methods in the superclass has a nice advantage, we can add before/after code (using Spring or not) to do things like measure performance, logging etc.

so for example code in

will be replaced by a call to getById

so the change will only be in the implementation of the Hibernate*DAOs without making any changes to their DAO interfaces, right?

yes, labeled it with the hackathon label. thanks @lluismf for bringing it up!

Just getById

of course!

Strictly speaking our DAOs aren’t part of our public APIs, so it’s theoretically okay to change the interfaces. But in practice it’s not a good idea without a major version bump.

How about we go a bit further:

  1. Deprecate LocationDAO.getLocation(Integer) and comment to use getById instead
  2. Change implementation of HibernateLocationDAO.getLocation(Integer) to call getById(Integer)
  3. Change all usages of LocationDAO.getLocation(Integer) to instead call LocationDAO.getById(Integer)

Later (maybe when we release platform 3.0.0) we remove the deprecated methods. So eventually we actually do cut down on the amount of unnecessarily-duplicated code.

What do others think about this?

Fine for me !

I agree with this too!

I’d vote for removing Vs deprecating code, what’s the benefit of deprecating over removing?

The reason I suggested deprecating is that I imagine there’s some module code out there that directly uses DAOs. I’m not sure we’ve ever explicitly said this is wrong.

For example✓&q=org%3Aopenmrs+ConceptDAO+org.openmrs.module&type=Code returns a couple results outside of openmrs-core.

I do agree with your approach since the DAOs are public we do not know where they have been used.

Shall I integrate this into ?

I had one more thought.

While I don’t think we should try to deal with Spring Data now, it’s a really cool tech that I hope we can leverage in the future.

What if we made some changes to method names in HibernateOpenmrsObjectDAO so that whatever methods we are now standardizing on are done exactly as they would be if they were implemented via Spring Data? That way, we could theoretically move to Spring Data seamlessly in the future.

That would mean, for example:

// rename this to "find"
T getById(Serializable id)

// rename this to "findByUuid"
T getByUuid(String uuid)


I was hoping that OpenMRS would adopt Spring Data and am happy if the community agrees that this is something we want to reach in the future.

I think keeping this in mind when doing this refactoring is a good idea, but I wonder if the renaming of these two methods isnt adding confusion. I mean generally we should stick to one word for a specific action and this would lead to having a mix of get and find for retrieving things from the DB, at least until the change to spring data happens.

I agree with Darius’ view, to keep the scope of the ticket small and not have to switch to Spring data but just implement it in a way that aligns with Spring data

I we’re going to break compatibility now, better change the methods to align with Spring data, I agree. If OTOH we decide to keep the same methods in the interfaces and just change the implementation we can wait.

Yes, this is my point.

If we are going to deprecate existing DAO methods for new ones and we plan to later move to spring data, let’s align the new method names with Spring Data.

(Otherwise we will end up having to deprecate the new methods too.)

-Darius (by phone)