mardi 22 février 2011

Relations sans foreign keys en Hibernate


En Hibernate, une relation entre deux entités s'exprime en générale à l'aide d'une foreign key. Néanmoins, de nombreux développeurs s'imaginent à tort qu'Hibernate a besoin d'une contrainte de foreign key entre les tables pour mapper une relation.

Certes, c'est une bonne idée. Prenons par exemple le cas de deux tables (MASTER et DOG, tables utilisées dans un précédent article). DOG ne contient que deux colonnes: ID et NAME. MASTER en contient trois: ID, NAME et DOG_ID, cette dernière contenant un ID de la table DOG.

Un schéma classique qui se mappe comme suit (cf. toujours le même article):
@Entity
public class Dog {
     @Id @GeneratedValue(strategy=GenerationType.AUTO)
     private Integer id;

     private String name;

     //Setters, getters, equals, hashcode...   
}
et
@Entity
public class Master {

     @Id @GeneratedValue(strategy=GenerationType.AUTO)
     private Integer id;

     private String name;

     @OneToOne
     private Dog dog;

     //setters getters, equals, hashcode    
}
Si on laisse à Hibernate le soin de générer le schéma, il ajoutera une contrainte FK sur la colonne DOG_ID pour que les valeurs qu'elle contient soient toujours des clés primaires de DOG.

Sans contrainte

Cependant, cette contrainte n'est absolument pas nécessaire et cela n'empêchera pas Hibernate de fonctionner (de la même manière, une propriété annotée @Id ne doit pas forcément correspondre à une clé primaire, mais c'est une autre histoire).

Curieusement, ce qui ressemble à une mauvaise pratique est plus courant qu'on ne le croit, même si elle ne se justifie souvent que par le poids de l'héritage (legacy).

Cela pose quand même un problème. Imaginons que notre DB contiennent les informations suivantes:
  • dans DOG, une ligne ID=1, NAME=Brutus
  • dans MASTER, une ligne ID=2, NAME=Toto, DOG_ID=1 et une ligne ID=3, NAME=Totor, DOG_ID=7

Nous avons donc une ligne master dont le DOG_ID ne référence aucun DOG.

Que va-t-il se passer avec le code suivant?
public class TestContrainte {
    public static void main(String[] args) {
        Configuration cfg = newAnnotationConfiguration().configure(); 
        SessionFactory sf = cfg.buildSessionFactory();
        Session session = sf.openSession();

        Master maitre = (Master)session.get(Maitre.class, 3);

        System.out.println(maitre.getNom());
        if(maitre.getChien() == null){ 
                System.out.println("Chien introuvable");
        } else {
                System.out.println("Chien: "+maitre.getChien().getNom());
        }

        session.close();
    }
} 

En fait, le code lance une exception:
Exception in thread "main" org.hibernate.ObjectNotFoundException: No row with the given identifier exists: [entities.Chien#7]

Evidemment, si on avait demandé l'id 2, on aurait obtenu la réponse:
Toto
Chien: Brutus

La solution

Le fait est qu'Hibernate considère que la référence vers Dog DOIT exister si une “FK” existe. Une exception est donc lancée. Ce comportement par défaut est généralement correct, mais dans les cas où la contrainte de foreign key n'existe pas, le code risque de planter.

Heusement, Hibernate propose un moyen de s'en sortir par le biais de ses annotations propres (hibernate-annotations).

Voici comment modifier la référence dans Master vers Dog:
@OneToOne
@NotFound(action=NotFoundAction.IGNORE)
private Dog dog;
L'annotation @org.hibernate.annotations.NotFound prend l'enum org.hibernate.annotations.NotFoundAction en paramètre. La valeur prise par défaut est NotFoundAction.EXCEPTION, ce qui correspond au comportement généralement observé. Mais quand "action" est en "IGNORE", comme dans ces quelques lignes, si Hibernate rencontre une "foreign key" ne pointant vers aucune ligne, il ne lancera pas d'exception, mais se contentera mettre la référence à null.

Ainsi donc, le test donné ci-dessus donnera comme résultat:
Totor
Chien introuvable

Ce qui n'est pas faux...

samedi 19 février 2011

NonUniqueObjectException, cascade et evict

Notre équipe d'architectes (Yannick et moi-même) a volé à la rescousse par un développeur qui ne savait plus à quel Saint se vouer.

Son code, à base d'Hibernate et de Spring, lançait une org.hibernate.NonUniqueObjectException et il n'en comprenait pas l'origine et pouvait encore moins s'en débarrasser.

Je l’avoue, dans les standards de développement mis en place (et que certains appellent à tort "architecture"), c’est la première fois que je rencontre ce cas.

L’API d’Hibernate décrit l'exception comme suit:
This exception is thrown when an operation would break session-scoped identity. This occurs if the user tries to associate two different instances of the same Java class with a particular identifier, in the scope of a single Session.

Reproduction simple

L'erreur est facile à reproduire. Utilisons pour l'exemple une entité Dog, simpliste:
@Entity
public class Dog {
     @Id @GeneratedValue(strategy=GenerationType.AUTO)
     private Integer id;

     private String name;

     //Setters, getters, equals, hashcode...   
}

Dans la base de données, il y au moins un entrée d’id 1 et de name "Bill". Voici comment reproduire l’erreur:
public class NonUniqueChien {
     public static void main(String[] args) {
          SessionFactory sf = new AnnotationConfiguration().configure().buildSessionFactory();
          Session session = sf.openSession();
          Transaction tx = session.beginTransaction();

          Dog oldDog = (Dog) session.get(Dog.class, 1);

          Dog dog = new Dog();
          dog.setName("Totor");
          dog.setId(1);

          session.saveOrUpdate(dog);

          tx.commit();
          session.close();
          sf.close();
     }
}

L’exécution de ce script provoque la org.hibernate.NonUniqueObjectException.

Explications:
  1. A la ligne 7, le code va rechercher l’entité Dog correspondant à l’id 1 (ce brave Bill). La session contient donc une entité Dog d’id 1.
  2. De 9 à 11, le code construit ensuite un objet Dog transient, avec un autre nom, mais le même id.
  3. A la ligne 13, il appelle saveOrUpdate. Dans la mesure où il y a un id, déclaré comme étant auto-généré, Hibernate part du principe que c’est un update. Par sécurité, le framework vérifie si l’objet existe déjà en session, ce qui est le cas (il s'agit de Bill). Ce n’est donc pas la même référence. Du point de vue d’Hibernate, il y a un risque: deux objets, représentant la même entité, peuvent potentiellement contenir des champs différents (ce qui est le cas ici). Quelle entité persister? Le chien qui s’appelle “Bill” ou celui qui s’appelle “Totor”? Ne pouvant décider, Hibernate lance la NonUniqueObjectException.

A noter que le problème se pose aussi avec "update" (bien sûr), mais pas avec "save" (Hibernate considère qu’il s’agit d’une nouvelle entité et génère un index à la place de celui fourni), ni avec merge évidemment.

Le problème est apparemment simple, mais détecter où il se produit est plus difficile. Le développeur, débutant en Spring et en Hibernate, a perdu le contrôle sur son application. Le modèle est complexe, avec des références vers de nombreuses autres entités dans des relations ToMany ou ToOne, toujours bidirectionnelles et du cascading CascadeType.ALL sur toutes les relations. C’est un cauchemar. Les méthodes s’enchaînent, passent d’un service ou d’un DAO à l’autre, sautant du transactionnel au non transactionnel...

Reproduction du problème complexe


Après un long moment de debugging, le noeud du problème est enfin trouvé.

Le développeur, parce qu'il ne maîtrise pas les transactions, le dirty checking et le lazy-loading, a fait beaucoup d'erreurs. Pour tenter de résoudre une des difficultés rencontrées, il a utilisé à plusieurs reprises des "evict" sur la session.

Une petite parenthèse s'impose ici: que ne nous a-t-il pas appelé à l'aide plus tôt, dés qu'il a eu des soucis! Au lieu de ça, il s'est enfoncé dans du bricolage fait du bricolage, pour obtenir au final un code spaghetti, non maintenable, d'une grande fragilité... et qui ne fonctionne pas. Fin de la parenthèse.

Voici comment on peut reproduire le problème tel qu'il l'a, mais d'une manière hautement simplifiée...

Une deuxième entité est nécessaire: le maître du chien. Présentation donc de Master:

@Entity
public class Master {

     @Id @GeneratedValue(strategy=GenerationType.AUTO)
     private Integer id;

     private String name;

     @OneToOne(cascade=javax.persistence.CascadeType.ALL)
     private Dog dog;

     //setters getters, equals, hashcode    
}


En DB, une ligne est créée dans chacune des tables. Nous avons maintenant notre Dog d'id=1 (Bill) et un Master d'id=2 (Boule), fier maître de Bill grâce à sa foreignkey vers Dog valant 1.

Le code suivant va provoquer l’erreur:

public class NonUniqueObjectTest {
    public static void main(String[] args) {
        SessionFactory sf = new AnnotationConfiguration().configure().buildSessionFactory();
        Session session = sf.openSession();
        Transaction tx = session.beginTransaction();
       
        Master boule = (Master) session.get(Master.class,2);
        session.evict(boule.getDog());
       
        Dog bill = (Dog) session.get(Dog.class, 1);
       
        tx.commit();
        session.close();
        sf.close();
    }
}


Explications
La cause du problème est due à une combinaison du "evict" et du cascading. Des deux, seul le "evict" est réellement erroné. Si le cascading était supprimé, il n'y aurait plus d'erreur, mais la logique du code resterait douteuse.
  1. A la ligne 7, l'entité Master (Boule) est récupérée. Elle contient une référence vers Dog (Bill).
  2. A la ligne 8, l'entité Dog (Bill) est détachée de la session avec "evict".
  3. A la ligne 10, l'entité Dog (Bill) est récupérée à nouveau. Il y a alors deux objets Dog (Bill): un est référencé par la propriété dog de Master (Bill), l'autre par la variable bill.
  4. A la ligne 12, la transaction est commitée. La session est flushée. La cascading amène Hibernate à vérifier le statut de l'objet référencé par dog. Voyant qu'il n'est pas attaché, il détecte qu'il est détaché (et non transient) et vérifie que la session ne contient pas déjà cette entité. Or, comme nous l'avons rechargée à la ligne 10, elle la contient. Hibernate, ne pouvant décider quel objet est le bon, lance une exception. L'ironie dans le cas présent est que les deux objets sont rigoureusement identiques...

Solution

Malheureusement pour le développeur, il n'y a pas de solution miracle. Même si dans le code qui précède, on pourrait résoudre le problème de différentes manières (retirer le "evict", retirer le cascading, limiter le cascading pour ne pas considérer les updates...), ce n'est pas aussi simple dans la réalité.

Le code développé est fragile, les effets de bord sont nombreux. S'il y a un evict, c'est parce qu'il en a eu besoin à ce moment-là. Même chose pour le cascading.

Pour lui, une seule solution, tout recommencer et mieux maîtriser la succession des opérations.

Pour les autres, une règle de base: éviter le evict. C'est rarement utile.

vendredi 18 février 2011

"Si ça marche, ce n'est pas une erreur"

Un des points qui me tiennent à coeur, c'est la qualité des développements. Et une grande partie de ma fonction consiste justement à veiller à cette qualité. La question toutefois est de savoir ce qu'est la qualité d'une application.

Un incident récent suggère que pour certains, il suffit que l'application fasse ce qu'on lui demande. Je suis d'accord que c'est un point essentiel, mais il ne suffit cependant pas à attribuer un label de qualité à une application.

La preuve avec un exemple bien réel.

L'analyse quotidienne faite par Sonar révèle un problème potentiel sur une application. Voici le code :
Integer i = Integer.valueOf(0);
Integer j = Integer.valueOf(1)
//Opérations sur i et j
if(i == j){
    //la suite du code
Le problème, c'est le "==" entre deux objets. Sonar soupçonne, avec raison, que c'est l'égalité des objets qui est testée et non l'égalité des références. Le code correct est donc:
if(i.equals(j))
C'est la correction la plus directe. Néanmoins, dans le cas présent, une meilleur approche est de transformer les Integer en int car ils ne se justifient pas et entraînent une perte de performance.

L'erreur est signifiée au développeur qui hausse les épaules et répond: "pourtant, ça marche".

Le fait est que la classe Integer, pour des valeurs comprises entre -128 et 127 utilise un cache, à condition de passer par valueOf (ce qui est le cas ici, mais aussi des opérations de boxing/unboxing).
Dans ces conditions, plusieurs Integer encapsulant le même "int" compris entre ces valeurs auront la même référence. Or, dans les opérations effectuées ci-dessus avec i et j, leurs valeurs sont comprises entre 0 et environ 10. Conséquence: bien qu'incorrect d'un point de vue OO, le test fonctionne.

Le développeur se tourne vers son chef d'équipe qui rétorque: "Si ça fonctionne, je ne vois pas où est le problème. De toute façon, on n'a pas le temps (sic)."

Au-delà de l'incident, on est sans doute tenté de se poser la question "pourquoi modifier ce code puisque ça marche?".

Voici quelques réponses:
  1. parce que si les valeurs des Integer dépassent les limites du cache, le code ne fonctionnera plus
  2. parce que si les valueOf sont remplacés par des new Integer(), ça ne fonctionnera plus
  3. parce que c'est incorrect
Les objections à ces arguments sont d'intéressantes.

En ce qui concerne le premier point, le développeur fait remarquer qu'il est "peu probable" que les valeurs dépasseront 10 ou 12. De son côté, le chef d'équipe explique que, dans une approche "pragmatique", lorsque ça ne fonctionnera plus, la correction sera apportée.

Je ne peux être d'accord avec ces deux objections. "Peu probable" me semble "peu rassurant". Quant à l'approche qui consiste à réparer lorsque ça posera problème, elle risque d'entraîner une perte de temps pour retrouver où ça ne marche pas.

En ce qui concerne le deuxième point, c'est simple. Puisque nous (la cellule d'architecture) recommandons l'usage de valueOf plutôt que de new Integer(), la problème ne se présentera pas.

Quant à la troisième réponse, l'objection est simple: en quoi est-ce une erreur, puisque ça marche?

Le plus terrible dans cette histoire, c'est que la correction aurait pris, commit sur Subversion compris, une trentaine de secondes. Notre discussion a duré un quart d'heure.

Alors, c'est quoi finalement la qualité? C'est un sujet sur lequel j'aurai l'occasion de revenir.