Scott Leberknight's Weblog

If you have used Hibernate and mapped a one-to-many relationship you've probably come across the "delete orphan" feature. This feature offers cascade delete of objects orphaned by code like the following:

Preference pref = getSomePreference();
user.getPreferences().remove(pref);

In the above code, a specific Preference is removed from a User. With the delete orphan feature, and assuming there is an active transaction associated with a session, the preference that was removed from the user is automatically deleted from the database when the transaction commits. This feature is pretty handy, but can be tricky if you try to write clever code in you setter methods, e.g. something like this:

// Do not do this!
public void setPreferences(Set newPreferences) {
    this.preferences = newPreferences == null ? new HashSet<Preference>() : newPreferences;
}

Code like the above results in a HibernateException with the following message if you pass null into setPreferences and try to save the user object:

A collection with cascade="all-delete-orphan" was no longer referenced by the owning entity instance

What is happening here is that Hibernate requires complete ownership of the preferences collection in the User object. If you simply set it to a new object as in the above code sample, Hibernate is unable to track changes to that collection and thus has no idea how to apply the cascading persistence to your objects! The same error will occur if you passed in a different collection, e.g.:

user.setPreferences(new HashSet<Preference>());

So the point is that Hibernate's delete orphan abstraction is leaking into your domain model object. This is pretty much unavoidable but is a leaky abstraction nonetheless that developers need to be aware of lest they run into the error mentioned above.

So how can you avoid this problem? The only sure way that I know of is to make the setter method private, since passing any new collection or null results in the "owning entity" error. This way only Hibernate will use the setter method to load up user objects (it invokes the method reflectively after setting it accessible via the Reflection API). Then you could add a method addPreference to your code which is the public API for adding preferences. Anyone could of course use reflection to do the same thing Hibernate is doing, but then all bets are off as they are subverting your public API. For example:

public void addPreference(Preference p) {
    getPreferences().add(p);
    p.setUser(this);
}

This has the nice side effect of establishing the bi-directional relationship between user and preference, assuming your model allows bi-directional navigation. You could also add a null check if you are paranoid. Removing a preference from a user is equally simple. You can write a helper method removePreference or you could call the getter and then call remove as shown here:

user.getPreferences().remove(aPref);

Essentially, you can operate on the collection returned by getPreferences as a normal Java collection and Hibernate will do the right thing. Since you are operating on the collection maintained and observed by Hibernate, it is able to track your changes, whereas replacing the collection wholesale makes Hibernate really mad, since it believes it is the sole proprietor of the collection, not you! For example, if you want to remove all the user's preferences you could write the following:

user.getPreferences().clear();

Note that all the above discussion refers to one-to-many relationships that specify the delete orphan cascade type; usually you will specify both "all" and "delete-orphan." In cases where you are only using the "all" cascade option, the semantics are quite different. Assuming the normal case where the "many" side of the relationship owns the relationship -- i.e. you used inverse="true" in your mapping file or @OneToMany(mappedBy = "user") if using annotations - then you must explicitly delete the child objects as Hibernate will only track that side of the relationship. For example, if cascade is "all" and you remove a preference from a user and then save the user, nothing happens! You would need to explicitly delete the preference object, as shown here:

// Assume only using cascade="all" and an inverse="true" mapping in User.hbm.xml
user.getPreferences().remove(aPref);  // does not cause database delete
session.delete(aPref);                // this causes the database deletion

One last thing to note in the above is that you must remove aPref from the user, or else Hibernate will throw an exception stating that the preference would be re-saved upon cascade! So if the User object is in the Session, remember you need to undo both sides of the relationship for things to work properly.

For anyone who uses PMD, the title of this blog appears in their list of PMD errors if they don't declare their loggers static and final. Specifically, the LoggerIsNotStaticFinal rule simply says that a log should be declared static and final. I also like to make sure they are private as well. For example:

// Jakarta Commons Logging
private static final Log log = LogFactory.getLog(MyClass.class);

The above code also shows another good practice, which is to pass the Class object to the getLog() method, instead of a string. Why the java.util.logging.Logger class doesn't even provide a method accepting a Class object is simply beyond me. Why did the people who developed the java.util.logging package base their API on Log4j yet omit some of the most useful parts of it? Oh well.

Now to the point. Why it is good practice to declare loggers private, static, and final? A logger is an internal implementation detail, so it should be private. You only need one logger for all instances of a class, hence static. And a logger should not be able to be replaced, thus final. So if this is good, what's not so good (at least in my opinion)? Simple - any logger that is not private, static, final, and which doesn't pass in a Class object to getLog()! For example, consider this common bit of code, declared in some base class:

// Not so good logger declaration
protected final Log log = LogFactory.getLog(getClass());

Why is this bad? Well, it isn't static for one thing. For another, it uses getClass() to obtain the log. At first this seems efficient since now all subclasses automatically inherit a ready-made log of the correct runtime type. So what's the issue here? The biggest problem with loggers declared in this manner is that you now get all the logging from the superclass mixed in with the logging from the subclass, and it is impossible in the log output to discern which messages came from which class unless you look at the source. This is really annoying if the superclass has a lot of logging that you don't want to see, since you cannot filter it out.

Another problem is that your ability to set log levels differently goes away, for example if a subclass resides in a different package than the superclass. In that case, if you try to filter out logging from the superclass, you can't because the actual runtime class was used to obtain the logger.

Last, having a protected logger just seems to violate basic object-oriented principles. Why in the world should subclasses know about an internal implementation detail from a superclass that is a cross-cutting concern, no less? Anyway, though this is a silly little rant it really is annoying when you extend a superclass that declares a protected logger like this.