'Re: Proposed new best practice for SingleClassDAOs and business-level services'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openmrs-dev
Subject:    Re: Proposed new best practice for SingleClassDAOs and business-level services
From:       Lluis Martinez <lluismf () gmail ! com>
Date:       2012-11-28 19:56:45
Message-ID: CAAmjNgbANSPTLjg1SWiEw3uXv6ZhoPCTRAZTs4z_BoeRea2TNw () mail ! gmail ! com
[Download RAW message or body]

I think the session factory which is injected into the DAOs is always
the same, having it in the DAO superclass means less instance
variables but it should not affect performance.

On Wed, Nov 28, 2012 at 7:07 PM, Friedman, Roger (CDC/CGH/DGHA) (CTR)
<rdf4@cdc.gov> wrote:
> +1 for Daniel's additions, don't understand Jeremy's objections.  Maybe we
> need to have a "throws NotSupportedException" or just a more liberal use of
> "throws Exception".
>
>
>
> Would it improve performance to allow the service to get a single Hibernate
> session which it would pass to the Hibernate concrete DAO's constructor,
> rather than having each DAO have its own as we do currently?
>
>
>
> From: dev@openmrs.org [mailto:dev@openmrs.org] On Behalf Of Jeremy Keiper
> Sent: Monday, November 26, 2012 10:47 AM
> To: dev@openmrs.org
> Subject: Re: Proposed new best practice for SingleClassDAOs and
> business-level services
>
>
>
> I think those are realistic to include, as long as this generic DAO is only
> for OpenmrsMetadata.  You may want to enforce that, e.g. SingleClassDAO<T
> extends OpenmrsMetadata>.
>
>
>
> Jeremy Keiper
> OpenMRS Core Developer
> AMPATH / IU-Kenya Support
>
> On Mon, Nov 26, 2012 at 8:18 AM, Daniel Kayiwa <kayiwadaniel@gmail.com>
> wrote:
>
> Thanks Darius and others for sharing.
> Committed at:
> https://github.com/openmrs/openmrs-module-appointment/commit/fcbc77574160b0ccbe7bacc28e277182996a915e
>
> Found myself adding these methods to the SingleClassDAO
>
> T getByUuid(String uuid);
> List<T> getAll(boolean includeRetired);
> List<T> getAll(String fuzzySearchPhrase);
>
> Or should they belong to another base subclass of SingleClassDAO (For the
> sake of those who may not need them)?
>
>
>
> On Mon, Nov 26, 2012 at 2:29 PM, Wesley Brown <wesbrown1@gmail.com> wrote:
>
> This seems like a really good idea!  If a given entity doesn't have any
> custom data methods a generic DAO class should be able to handle the core
> functionality.
>
>
>
> -Wes
>
>
>
> On Monday, November 26, 2012 9:45:33 AM UTC+3, Rowan Seymour wrote:
>
> Looking at the HTML source of Darius' blog bost I can see now that the
> generic class parameters (<T>) are there but aren't displayed as they're
> being treated like HTML tags. Now it all makes much more sense!
>
>
>
> Does this mean that for simple DAO requirements, one could avoid defining a
> new DAO altogether and just use the generic version? e.g.
>
>
>
> <bean class="org.openmrs.module.example.api.ExampleServiceImpl">
>
>   <property name="dao" />
>
>     <bean class="org.openmrs.api.db.hibernate.HibernateSingleClassDAO">
>
>       <constructor-arg value="org.openmrs.module.example.Example" />
>
>     </bean>
>
>   </property>
>
> </bean>
>
>
>
> -Rowan
>
>
>
> On 25 November 2012 23:27, Lluis Martinez <llu...@gmail.com> wrote:
>
> This is exactly the approach to follow in
> https://tickets.openmrs.org/browse/TRUNK-1824
>
>
> On Sun, Nov 25, 2012 at 2:14 PM, Rowan Seymour <rowans...@gmail.com> wrote:
>> Shouldn't the first interface be completely generic, i.e.
>>
>
>
>
>> public interface SingleClassDAO<T> {
>>   T getById(Integer id);
>>   ...
>> }
>>
>> Otherwise T is undefined... And then instead of passing the class as a
>> constructor parameter to HibernateSingleClassDAO, pass it as a generic
>> class
>> parameter (as Jeremy suggests), e.g.
>>
>> public abstract class HibernateSingleClassDAO<T> implements
>> SingleClassDAO<T> { ... }
>>
>> public interface PaperRecordRequestDAO extends
>> SingleClassDAO<PaperRecordRequest> {
>>
>> public class HibernatePaperRecordRequestDAO extends
>> HibernateSingleClassDAO<PaperRecordRequest> implements
>> PaperRecordRequestDAO
>> { ... }
>>
>> I'm only using my head as a compiler... but with the code as it is, aren't
>> you lacking type safety on the DAO methods?
>>
>> -Rowan
>>
>
>> On 24 November 2012 16:57, Jeremy Keiper <jer...@openmrs.org> wrote:
>>>
>>> Wes, I agree that it feels strange to be repeating ourselves a lot with
>>> methods in both the service and dao layers, but this abstraction is on
>>> purpose.  If you consider that criteria are hibernate specific and you
>>> want
>>> the ability to later implement a dao using a different orm, the criteria
>>> code makes sense to keep in the dao impl.
>>>
>>> Darius, when I started working with spring, the recommended approach at
>>> the time was to use a generic dao like the one you wrote, except use a
>>> generic parameter in the class definition. E.g. PatientDao extends
>>> GenericDao<Patient>.
>>>
>
>>> On Nov 24, 2012 6:18 AM, "Wesley Brown" <wesb...@gmail.com> wrote:
>>>>
>>>> Darius,
>>>>
>>>> Thanks for posting this!  We have been working on something similar but
>>>> came up with a very different solution; I was hoping to discuss it on an
>>>> upcoming design/dev forum but perhaps it's just easier to mention it
>>>> here.
>>>>
>>>> When creating our services for the soon-to-be-released Cashier Module we
>>>> quickly noticed that our services ended up being mostly direct calls to
>>>> our
>>>> DAO layer. IE:
>>>>
>>>>> public class BillServiceImpl ... {
>>>>>     private IBillDAO dao;
>>>>>     public Bill save(Bill bill) {
>>>>>         return dao.save(bill);
>>>>>     }
>>>>>
>>>>>     public void purge(Bill bill) {
>>>>>         dao.purge(bill);
>>>>>     }
>>>>>
>>>>>     ...
>>>>>
>>>>> }
>>>>
>>>>
>>>> I think that this happens because our business services are really data
>>>> services; however we use a design that assumes another logical layer
>>>> between
>>>> the DAO and controller.  Look, for example, at the
>>>> org.openmrs.api.impl.VisitServiceImpl class; almost all the methods
>>>> simply
>>>> call an identically named DAO method.  I think we both agree that this
>>>> is
>>>> not an optimal solution. (Note that this also means that mocking the DAO
>>>> in
>>>> the services for unit tests often results in completely useless tests)
>>>>
>>>> Our solution differs from yours in that we decided to embrace the fact
>>>> that our services are data services and therefore make them responsible
>>>> for
>>>> interacting with the database. We did this because most objects that
>>>> necessitate a service need to have the core save, delete, and select
>>>> methods.  Also, rather than having a DAO class per model we created a
>>>> single
>>>> DAO class which handles data operations for all model types (in our
>>>> codebase
>>>> all model types eventually inherit from BaseOpenmrsObject). This DAO
>>>> class
>>>> is quite simple:
>>>>
>>>>> public interface IGenericHibernateDAO {
>>>>>     <E extends BaseOpenmrsObject> Criteria createCriteria(Class<E>
>>>>> cls);
>>>>>     <E extends BaseOpenmrsObject> E save(E entity) throws APIException;
>>>>>     <E extends BaseOpenmrsObject> void delete(E entity) throws
>>>>> APIException;
>>>>>     <T> T selectValue(Criteria criteria);
>>>>>     <E extends BaseOpenmrsObject> E selectSingle(Class<E> cls,
>>>>> Serializable id) throws APIException;
>>>>>     <E extends BaseOpenmrsObject> E selectSingle(Class<E> cls, Criteria
>>>>> criteria) throws APIException;
>>>>>     <E extends BaseOpenmrsObject> List<E> select(Class<E> cls) throws
>>>>> APIException;
>>>>>     <E extends BaseOpenmrsObject> List<E> select(Class<E> cls, Criteria
>>>>> criteria) throws APIException;
>>>>> }
>>>>
>>>>
>>>> Our data services then use this generic DAO class to perform whatever
>>>> data methods are needed and expose them to the controllers and such:
>>>>
>>>>> public class BillServiceImpl extends ... {
>>>>>     @Override
>>>>>     @Authorized( { CashierPrivilegeConstants.VIEW_BILLS } )
>>>>>     @Transactional(readOnly = true)
>>>>>     public Bill getBillByReceiptNumber(String receiptNumber) throws
>>>>> APIException {
>>>>>
>>>>>         Criteria criteria = dao.createCriteria(getEntityClass());
>>>>>
>>>>>         criteria.add(Restrictions.eq("receiptNumber", receiptNumber));
>>>>>
>>>>>         return dao.selectSingle(getEntityClass(), criteria);
>>>>>
>>>>>     }
>>>>> }
>>>>
>>>>
>>>> You may notice that we have allowed the hibernate API to "leak" into the
>>>> service layer here (through the Criteria) but I argue that this is a
>>>> non-issue.  Designing an API that supports switching ORM's solves a
>>>> problem
>>>> that doesn't actually exist in the real world at the expense of a more
>>>> simple overall design.  This also makes it trivial to create an overall
>>>> service architecture, taking into account the differences between
>>>> BaseOpenmrsObject, BaseOpenmrsData, and BaseOpenmrsMetadata.  We have
>>>> posted
>>>> that service architecture API in our openhmis.commons repository and
>>>> will be
>>>> creating documentation on the wiki shortly.
>>>>
>>>> I'd love to feedback from the community on this.
>>>>
>>>> -Wes
>>>>
>>>> On Saturday, November 24, 2012 12:47:16 AM UTC+3, Darius Jazayeri wrote:
>>>>>
>>>>> Hi Daniel and Yonetan,
>>>>>
>>>>> I noticed the commit around implementing the DAOs for the Account
>>>>> module, and I wanted to share our new best-practice suggestion from our
>>>>> Mirebalais work.
>>>>>
>>>>> I written it up in a blog post here:
>>>>>
>>>>>
>>>>> http://mirebalaisemr.blogspot.com/2012/11/openmrs-style-tip-standardizing-our.html
>>>>>
>>>>> In short, I suggest that you copy the SingleClassDAO and
>>>>> HibernateSingleClassDAO from the emr module, and build your DAO layer
>>>>> based
>>>>> on that pattern.
>>>>>
>>>>> -Darius
>>>>
>>>> --
>>>> OpenMRS Developers: http://go.openmrs.org/dev
>
>>>> Post: d...@openmrs.org
>>>> Unsubscribe: dev+uns...@openmrs.org
>
>
>>>> Manage your OpenMRS subscriptions at https://id.openmrs.org/
>>>>
>>>>
>>>
>>> --
>>> OpenMRS Developers: http://go.openmrs.org/dev
>
>>> Post: d...@openmrs.org
>>> Unsubscribe: dev+uns...@openmrs.org
>
>
>>> Manage your OpenMRS subscriptions at https://id.openmrs.org/
>>>
>>>
>>
>>
>>
>>
>> --
>> Rowan Seymour
>> tel: +250 783835665
>>
>> --
>> OpenMRS Developers: http://go.openmrs.org/dev
>
>> Post: d...@openmrs.org
>> Unsubscribe: dev+uns...@openmrs.org
>
>
>> Manage your OpenMRS subscriptions at https://id.openmrs.org/
>>
>>
>
> --
> OpenMRS Developers: http://go.openmrs.org/dev
>
> Post: d...@openmrs.org
> Unsubscribe: dev+uns...@openmrs.org
>
>
> Manage your OpenMRS subscriptions at https://id.openmrs.org/
>
>
>
>
>
> --
> Rowan Seymour
> tel: +250 783835665
>
> --
> OpenMRS Developers: http://go.openmrs.org/dev
> Post: dev@openmrs.org
> Unsubscribe: dev+unsubscribe@openmrs.org
> Manage your OpenMRS subscriptions at https://id.openmrs.org/
>
>
>
>
>
> --
> If we keep uppermost in our minds the unkind and unjust acts of others, we
> shall find it impossible to love them as Christ has loved us; but if our
> thoughts dwell upon the wondrous love and pity of Christ for us, the same
> spirit will flow out to others.
>
>
>
> --
> OpenMRS Developers: http://go.openmrs.org/dev
> Post: dev@openmrs.org
> Unsubscribe: dev+unsubscribe@openmrs.org
> Manage your OpenMRS subscriptions at https://id.openmrs.org/
>
>
>
>
>
> --
> OpenMRS Developers: http://go.openmrs.org/dev
> Post: dev@openmrs.org
> Unsubscribe: dev+unsubscribe@openmrs.org
> Manage your OpenMRS subscriptions at https://id.openmrs.org/
>
>
>
> --
> OpenMRS Developers: http://go.openmrs.org/dev
> Post: dev@openmrs.org
> Unsubscribe: dev+unsubscribe@openmrs.org
> Manage your OpenMRS subscriptions at https://id.openmrs.org/
>
>

-- 
OpenMRS Developers: http://go.openmrs.org/dev
Post: dev@openmrs.org
Unsubscribe: dev+unsubscribe@openmrs.org
Manage your OpenMRS subscriptions at https://id.openmrs.org/


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic