Wednesday, January 20, 2010

Parent/Child Hibernate pitfall

Lets say we have these two interfaces:
package com.codeark.parentchild;

import java.util.Set;

public interface Parent {
Set<Child> getChildren();
}

And
package com.codeark.parentchild;

public interface Child {
Parent getParent();
}

And here are their implementations, annotated for persistence:
package com.codeark.parentchild;

import java.util.HashSet;
import java.util.Set;

import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
import javax.persistence.OneToMany;
import javax.persistence.Table;

@Entity
@Table(name="parents")
public class ParentImpl implements Parent {

private Set<Child> children;
private Long id;

public ParentImpl() {
this.children = new HashSet<Child>();
}

@Override
@OneToMany(targetEntity=ChildImpl.class, mappedBy="parent", cascade=CascadeType.ALL)
public Set<Child> getChildren() {
return children;
}

public void setChildren(Set<Child> children) {
this.children = children;
}

@Id
@GeneratedValue(strategy = GenerationType.AUTO)
public Long getId() {
return id;
}

public void setId(Long id) {
this.id = id;
}
} 


And the child implementation:
package com.codeark.parentchild;

import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
import javax.persistence.ManyToOne;
import javax.persistence.Table;

@Entity
@Table(name="children")
public class ChildImpl implements Child {

private Parent parent;
private Long id;

public ChildImpl() {

}

@Override
@ManyToOne(targetEntity=ParentImpl.class)
public Parent getParent() {
return parent;
}

public void setParent(Parent parent) {
this.parent = parent;
}

@Id
@GeneratedValue(strategy = GenerationType.AUTO)
public Long getId() {
return id;
}

public void setId(Long id) {
this.id = id;
}
}
So, where is the pitfall? Well, there are probably a few lying about, but the one I ran into will become apparent shortly. First, consider this code snippet:
Parent p = new ParentImpl();
Child c = new ChildImpl();

p.getChildren().add(c);
((ChildImpl)c).setParent(p);

// persist to database
DAOFactory.getParentDAO().persist(p);

Now, another programmer joined the show, and decided to have his own Child implementation:
package com.codeark.parentchild;

public class AnotherChildImpl implements Child {

private Parent parent;

public AnotherChildImpl() {

}

protected void someOtherNewMethods() {

}

@Override
public Parent getParent() {
return parent;
}

public void setParent(Parent p) {
this.parent = p;
}
}

A perfectly legal implementation, lets have a look at another code snippet:
Parent p = new ParentImpl();
Child c = new ChildImpl();
Child c1 = new AnotherChildImpl();

p.getChildren().add(c);
p.getChildren().add(c1);
((ChildImpl)c).setParent(p);
((AnotherChildImpl)c1).setParent(p);

// persist to database
DAOFactory.getParentDAO().persist(p);

What do you think will happen now? This code will compile, but during runtime Hibernate will hit us with an IllegalArgumentException in class: com.codeark.parentchild.ChildImpl, getter method of property: id
org.hibernate.PropertyAccessException: IllegalArgumentException occurred calling getter of com.codeark.parentchild.ChildImpl.id...

This is a result of me telling hibernate to expect a ChildImpl (targetEntity) while in practice that collection contained AnotherChildImpl class, which hibernate knows nothing about.

Why is this so bad?

This exception will not occur until the program tries to persist a ParentImpl with an AnotherChildImpl bomb waiting to explode in its children collection. That event can happen far into the (runtime) future. Although I doubt such a bug will make it to production if you have decent integration and QA, its still within the realm of possibilities; it would be best if we do what ever we can to prevent that eventuality.

Here is what I did to prevent this...

First I changed the parent Interface:
package com.codeark.parentchild;

import java.util.Set;

public interface Parent {
Set<? extends Child> getChildren();
}
Note that our children collection is now of an unknown type bounded by Child, instead of just Child.

Then I changed ParentImpl:
package com.codeark.parentchild;

import java.util.HashSet;
import java.util.Set;

import javax.persistence.CascadeType;
import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
import javax.persistence.OneToMany;
import javax.persistence.Table;

@Entity
@Table(name="parents")
public final class ParentImpl implements Parent {

private Set<ChildImpl> children;
private Long id;

public ParentImpl() {
this.children = new HashSet<ChildImpl>();
}

@Override
@OneToMany(mappedBy="parent", cascade=CascadeType.ALL)
public Set<ChildImpl> getChildren() {
return children;
}

public void setChildren(Set<ChildImpl> children) {
this.children = children;
}

@Id
@GeneratedValue(strategy = GenerationType.AUTO)
public Long getId() {
return id;
}

public void setId(Long id) {
this.id = id;
}
}
Our HashSet collection is now of ChildImpl type instead of just Child, I've changed that throughout the class's internals and external API. Our class still complies with Parent's contract, since we've changed Parent.getChildren() from Set<Child> to Set<? extends Child>

As an added bonus, we no longer need the targetEntity=ChildImpl.class annotation for ParentImpl.getChildren().

Last small caveat, I've marked the class as "final" since subclassing it without proper annotations/mapping might expose us to similar problems.

Another important change; in ChildImpl class:
package com.codeark.parentchild;

import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
import javax.persistence.ManyToOne;
import javax.persistence.Table;

@Entity
@Table(name="children")
public final class ChildImpl implements Child {

private ParentImpl parent;
private Long id;

public ChildImpl() {

}

@Override
@ManyToOne
public ParentImpl getParent() {
return parent;
}

public void setParent(ParentImpl parent) {
this.parent = parent;
}

@Id
@GeneratedValue(strategy = GenerationType.AUTO)
public Long getId() {
return id;
}

public void setId(Long id) {
this.id = id;
}
}
All the Parent references were changed to ParentImpl, so I wouldn't face a similar problem if someone tries to add a non persistent parent to ChildImpl.

Lets look at our buggy code again:
ParentImpl p = new ParentImpl();
ChildImpl c = new ChildImpl();
Child c1 = new AnotherChildImpl();

p.getChildren().add(c);
p.getChildren().add(c1);  // A Compile time error here!
Now the "other" programmer will get a compile time error, while trying to add his new (and improved) class to your parent Implementation, and the world is a slightly better place now.

No comments:

Post a Comment