Accueil > Non classé > HibernateGenericDAO considered harmful

HibernateGenericDAO considered harmful

La factorisation de code est une des grandes promesses de la programmation orientée objet. Ainsi, dès que l’on a passé le niveau “novice”, on meurt d’envie de factoriser du code et l’on tombe dans le premier piège de la POO : abuser de l’héritage. La désillusion n’en sera que plus grande dès que l’on passera au niveau “confirmé” :). Mais ce n’est pas le sujet de ce billet. Non, dans ce billet je vous propose de voir un effet plus pervers de ce problème, induit par le mode de fonctionnement d’Hibernate, et la paresse humaine.

Plantons le décor

Tout d’abord, expliquons ce que l’on entend par HibernateGenericDAO. Les projets classiques découpent leur architecture en 3 couches (présentation, métier et accès aux données) et cette dernière est souvent réalisée avec le pattern DataAccessObject, ou DAO. Celui-ci est caractérisé par l’utilisation d’une instance sans état d’une classe d’accès, pour chaque type d’entité métier (un UserDAO permet de manipuler des User, un SubscriptionDAO permet de manipuler des … Subscription). Pour la plupart de ces objets, on réalise assez rapidement que les fameuses opérations CRUD sont nécessaires, et l’on en vient à factoriser innocemment le code en créant une classe mère commune, HibernateGenericDAO, mettant à disposition toutes ces opérations :


1
2
3
4
5
6
7
8
public abstract class HibernateGenericDAO<T, ID extends Serializable> implements IGenericDAO<T, ID> {
    protected Class<T> persistentType;
    protected HibernateGenericDAO(Class<T> persistentType) {/*...*/}
    public ID makePersistent(T entity) {/*...*/}
    public void makeNotPersistent(T entity) {/*...*/}
    public T loadById(ID id) {/*...*/}
    public List<T> findAll() {/*...*/}
}

Quelque chose dans le genre. Mon argumentaire est d’autant plus cruel que ce code a l’air génial : utilisation des generics, type de retour covariant. Je vous ai épargné la mastication intellectuelle consistant à borner T par <T extends IPersistent<T>> qui semble parfaite une fois que ça finit enfin par compiler. Au passage, j’attire votre attention sur un autre problème de ce pattern qui est que l’on ne réalise par forcément que les objets manipulés sont dans l’état “persistent”, d’où mon choix de nommage de méthode “makePersistent” plus explicite que “save” par exemple.

Mais revenons à nos moutons. Le vrai problème de ce code réside non pas dans l’emploi de generics, mais dans le non-dit des deux méthodes loadById() et findAll(). A partir du moment où vous mettez à disposition ces méthodes (vu que tous vos DAOs vont hériter du squelette de classe présenté plus haut), vous pouvez être sûr qu’elles vont être utilisées. Mais ces méthodes ne sont pas assez spécifiques. En effet, quel est le rôle d’un développeur sur un projet informatique ? De développer des applications bien sûr ! Ou plus exactement de réaliser l’implémentation de use-cases. Et nous touchons là au cœur du problème : chaque use-case est différent. Chaque use-case A nécessite de travailler sur des données légèrement différentes de celles sur lesquelles travaille le use-case B. Par exemple, même si les use-cases A et B se focalisent sur une commande, peut-être que le use-case A nécessite de présenter les articles présents dans la commande, alors que le use-case B nous montre les détails de facturation.

Où l’on parle de lazyness

Et malheureusement, parce qu’Hibernate utilise la technique du lazy-loading, et que d’autres frameworks la favorisent, vous vous retrouverez vite avec le fameux problème du 1+N requêtes (voire du 1 + N x M). Entendons nous bien : je ne remets pas en cause le principe du lazy-loading, utile parfois. Ce qui me semble problématique ici, c’est la facilité avec laquelle on peut produire le code de nos use-cases A et B en utilisant OrderDAO.loadById(), sans réfléchir. Et soyez certains que quand il s’agit d’être lazy, il n’y a pas qu’Hibernate qui répond présent. La nature humaine est telle qu’avec l’absence d’encadrement, une documentation pas tout à fait à jour ou un fort turnover des équipes sur le projet, ces méthodes finiront par être utilisées dans un cadre qui ne convient pas (mais finalement, si l’on suit mon raisonnement jusqu’au bout, seul le CRUD basique – qui pourrait être généré automatiquement – convient à l’utilisation de ces méthodes).

Ainsi donc, dans le use-case A mentionné ci-dessous, on est amené à manipuler order.articles tandis que dans le use-case B, c’est order.paymentInfo qui nous intéresse. Si l’on n’a pas pris soin de ramener ces objets dans notre appel de DAO, ils seront ramenés a posteriori par Hibernate (c’est tout l’intérêt du lazy-loading), produisant des requêtes supplémentaires. Evidemment, si par exemple paymentInfo référence un objet complexe de type Address, une autre requête est susceptible d’être déclenchée lorsque l’on présentera les détails de l’adresse de facturation, et ainsi de suite.

Mettre à disposition une méthode du type findAll(), a fortiori pour un coût nul en la factorisant dans une super-classe, revient donc à mon sens à se tirer une balle dans le pied. Evidemment, je ne prétend pas avoir évité ce piège (tout comme je n’affirme pas qu’une méthode de ce type n’a jamais sa place dans vos sources, malgré un choix de titre sulfureux), mais l’expérience m’a montré que sur tous les projets souffrant de problèmes de performance Hibernate, une incompréhension ou une méconnaissance du lazy-loading était à l’origine du problème.

Pour l’abolition des méthodes sans saveur

La solution à ce problème technique est très simple : préciser dans chaque méthode (et dans sa documentation) du DAO les données qui seront ramenées (pré-fetchées), par exemple1 :


1
2
3
4
5
6
7
8
9
10
11
12
13
14
...
    public Order loadByIdFetchingArticles(Long id) {
        String hql = "select o from Order o left join fetch o.articles where o.id = :pId";
        ...
    }

    public Order loadByIdFetchingPaymentInfo(Long id) {
        String hql = "select o from Order o left join fetch o.paymentInfo where o.id = :pId";
        ...
    }

    public List<Order> findAllFetchingXXX() {
        ...
    }

Du coup, comme mentionné plus haut, les simples méthodes findAll() ou loadById() n’ont plus d’intérêt. A minima, si vous tenez absolument à les conserver (mais avec le recul, je pense sincèrement que c’est une erreur), renommez-les en loadByIdFetchingNothing() histoire d’attirer l’attention des développeurs les plus grégaires. Finalement, si la solution technique est assez simple, le vrai problème n’est pas technique mais culturel : comment s’assurer que tout un chacun réalise bien l’impact d’une méthode aussi banale que findAll() sur les performances de son application ?

PS : les lecteurs avertis auront bien sûr réalisé que modifier le mapping hibernate afin de forcer un join partout (lazy="false"), est une contre-solution.

1 idéalement, vos requêtes ne sont pas redéfinies dans des Strings à chaque fois. C’est le nommage de la méthode qui m’intéresse ici.

Share
  1. anonym
    25/11/2009 à 14:31 | #1

    C’est GenericDAO mais pas AllImaginableUseCaseDao!!!

  1. 29/01/2010 à 07:16 | #1
  2. 20/11/2014 à 03:21 | #2