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
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.
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.
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:
Deprecate LocationDAO.getLocation(Integer) and comment to use getById instead
Change implementation of HibernateLocationDAO.getLocation(Integer) to call getById(Integer)
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.
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.
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.
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.)
