OpenMRS Style Tip: standardizing our DAOs, and avoiding data-level services

We've developed a new best-practice way of organizing services and DAOs in OpenMRS, and I wanted to share that here.

1. We should have one DAO class for every domain object, and the boilerplate code should be encapsulated in an abstract base class

2. A service can contain multiple DAOs, for related domain functionality:

3. The service methods should expose higher-level functionality, rather than just CRUD

Below are (long) code samples:

First, we implement some helper classes to take care of the boilerplate code that should be in every OpenMRS DAO. (We did this in the EMR module, but we should get this into the OpenMRS core, or perhaps some devutils module.)

public interface SingleClassDAO<T> {
        T getById(Integer id);
        List<T> getAll();
        T saveOrUpdate(T object);
        T update(T object);
        void delete(T object);
}

public abstract class HibernateSingleClassDAO<T> implements SingleClassDAO<T> {

    protected SessionFactory sessionFactory;
    protected Class<T> mappedClass;

    /**
     * Marked private because you *must* provide the class at runtime when instantiating one of
     * these, using the next constructor
     */
    @SuppressWarnings("unused")
    private HibernateSingleClassDAO() {
    }

    /**
     * You must call this before using any of the data access methods, since it's not actually
     * possible to write them all with compile-time class information.
     *
     * @param mappedClass
     */
    protected HibernateSingleClassDAO(Class<T> mappedClass) {
        this.mappedClass = mappedClass;
    }

    public void setSessionFactory(SessionFactory sessionFactory) {
        this.sessionFactory = sessionFactory;
    }

    @SuppressWarnings("unchecked")
    @Override
    @Transactional(readOnly = true)
    public T getById(Integer id) {
        return (T) sessionFactory.getCurrentSession().get(mappedClass, id);
    }

    @SuppressWarnings("unchecked")
    @Override
    @Transactional(readOnly = true)
    public List<T> getAll() {
        return (List<T>) sessionFactory.getCurrentSession().createCriteria(mappedClass).list();
    }

    @Override
    @Transactional
    public T saveOrUpdate(T object) {
        sessionFactory.getCurrentSession().saveOrUpdate(object);
        return object;
    }

    @Override
    @Transactional
    public T update(T object) {
        sessionFactory.getCurrentSession().update(object);
        return object;
    }

    @Override
    @Transactional
    public void delete(T object) {
        sessionFactory.getCurrentSession().delete(object);
    }

}

Each one of our DAOs will extend these base ones (and cover just a single domain object). So the basic CRUD operations are already taken care of for us, but we still need to implement specific queries for the domain object:

public interface PaperRecordRequestDAO extends SingleClassDAO {

    /**
     * Returns all the paper record requests with the specified status
     *
     * @param status
     * @return the paper record requests with the specified status
     */
    List findPaperRecordRequests(PaperRecordRequest.Status status);

    // ...
}

public class HibernatePaperRecordRequestDAO extends HibernateSingleClassDAO implements PaperRecordRequestDAO {

    public HibernatePaperRecordRequestDAO() {
        super(PaperRecordRequest.class);
    }

    @Override
    public List findPaperRecordRequests(PaperRecordRequest.Status status) {

        Criteria criteria = createPaperRecordCriteria();
        addStatusRestriction(criteria, status);
        addOrderByDate(criteria);

        return (List) criteria.list();
    }

    // ...
}

Our service uses that DAO. If our service needs to access another domain object, we'll do another SingleClassDAO for that one:

public class PaperRecordServiceImpl extends BaseOpenmrsService implements PaperRecordService {

    private PaperRecordRequestDAO paperRecordRequestDAO;
    // more DAOs go here when we need them

    // ...
}

Our service layer defines methods that match business-level functionality, not data-level functionality. In this example we have multiple methods to allow for fetching requests in different statuses, since this is meaningful at a business level. They are all one-line calls to the same DAO method that takes a status argument, but we don't expose that method in the service. This helps us avoid letting business logic creep into our controllers, and keeps it in the service where it belongs. Example:

public interface PaperRecordService extends OpenmrsService {

    /**
     * Retrieves all records that are open (ie, have yet to be assigned to an archivist for retrieval)
     * and need to be pulled (ie, already exist and just need to be pulled, not created)
     *
     * @return the list of all open paper record requests that need to be pulled
     */
    @Authorized(EmrConstants.PRIVILEGE_PAPER_RECORDS_MANAGE_REQUESTS)
    List getOpenPaperRecordRequestsToPull();

    /**
     * Retrieves all records that have been assigned and need to be pulled
     *
     * @return the list of all assigned paper record requests that need to be pulled
     */
    @Authorized(EmrConstants.PRIVILEGE_PAPER_RECORDS_MANAGE_REQUESTS)
    List getAssignedPaperRecordRequestsToPull();

    // ...
}

Comments welcome! (Also, please copy this approach!)