Archive for December 4th, 2007

Don’t change the parameter value in a Hibernate setter

Tuesday, December 04th, 2007

A while back a co-worker asked for some help with a Hibernate problem – he thought he’d found a serious bug. He was an experienced database architect but new to Hibernate, so he’d written some basic test code. It was trivial – just reading objects from various HQL queries and displaying them to the console. The weird thing was, after reading an object the ‘name’ property was always null (even though the column wasn’t – the sample data had values for every column) and weirder still after reading a row, the column became null.

I looked at his mapping files and his environment but everything seemed fine. Then I noticed that his setter method looked something like this:

public void setName(String naem) {
  this.name = name;
}

So it was a classic typo – the parameter was misspelled, so the setter was just setting the field to itself (Eclipse has a check for this and I find it’s best to flag it as an error). Since the state at flush time was different from the original values that Hibernate caches for dirty detection, Hibernate dutifully pushed the “change” to the database.

I was reminded of this today because of a similar bug that was introduced last week. One of the developers was seeing an unexpected optimistic locking exception in an entity that wasn’t being updated concurrently – in fact there are no public setters in the class.

Since the class is effectively immutable, I added the mutable='false' attribute to the class element in the mapping file, removed the non-public setters only called by Hibernate during instantiation and switched to field access. I also removed the optimistic locking <version> element since there’s no need to check for concurrent updates, and the bug went away.

I felt guilty checking that in though since it felt like fixing the symptom and not the real problem – just setting up someone else the next time something similar triggered the root bug. Luckily I realized the real issue a little while later.

We have a custom type for storing dates independent of timezone (it converts to GMT on insert and we adjust for the current time zone on read) and it was implemented using java.sql.Timestamp. Apparently this caused problems when comparing equality with java.util.Date, so the custom type’s nullSafeGet was changed to read the database timestamp and convert to a java.util.Date. As with the earlier example – an apparent change of the value (Timestamp‘s equals method isn’t symmetric with Date‘s) caused Hibernate to detect the change and push the update to the database. Two users were concurrently reading the object and Hibernate was pushing both changes to the database, hence the optimistic locking exception.

So I’ll keep the entity class immutable – it matches the actual usage – but will fix the custom type to do the right thing.

The first time I burned myself like this was with mapped collections. I had a mapped List on an entity and when I wrote the unit tests for the DAO tier, I mistakenly created test SQL scripts with index column values that started at 1 instead of 0. So I was seeing that the lists always had N+1 elements, and the first was always null. I spent way too long banging on this and finally put in a hack where I created a copy of the List in the setter (a new ArrayList), keeping only non-null elements.

This was a side project I was working on from home, and I ended up getting too busy and leaving the project. I recently started working on it again after being away for a year, and was able to fix a lot of dumb mistakes I’d made (this was my first time using Spring and Hibernate a real project).

The folks on the team mentioned that the SQL logs showed lots of deletes and inserts every time the entity was loaded from the database, resulting in significant slowness, so they wanted me to take a look. It didn’t take long to realize that it was due to replacing the managed PersistentList with a new list. Since it’s a new instance, Hibernate doesn’t detect specific changes so it deletes all of the old rows and inserts all the new, even in a case like this where there was no net change. In the production code, there were no nulls and in fact no actual changes, so no actions were necessary at all. This realization lead to the discovery of the unit test bug, and I was able to remove the hack and just keep the List that Hibernate passed in the setter.

More fun with Generics – Class.cast()

Tuesday, December 04th, 2007

Continuing the discussion from here and here.

We’ve been making extensive use of generics at work and in the process have discovered a bug in the 1.6 JDK (more on that later). The problem is that we can’t cast to the actual type of some generic instance variables, and also can’t use instanceof. The annoying thing is that it works fine in Eclipse – it’s only an issue in Sun’s JDK, so it fails during a command-line compile. Also it works fine in 1.5, so being one of the few on the team to use 1.6, I end up needing to fix things since it doesn’t affect the rest of the team or the continuous build.

We’ve worked around it by replacing casts with the Class.cast() method:

public T cast(Object obj) {
   if (obj != null && !isInstance(obj))
      throw new ClassCastException();
   return (T) obj;
}

and replace instanceof checks with Class.isAssignableFrom().

But take a look at the cast method. If we were to take the optimistic approach assuming no improper usage and remove the null and isInstance checks, then the method becomes

public T cast(Object obj) {
   return (T) obj;
}

Huh. That’s odd – we’re just replacing a cast with a cast.

So I wrote a simple test method to see why this works:

@SuppressWarnings("unchecked")
public static <T> T cast(@SuppressWarnings("unused") final Class<T> clazz,
       final Object o) {
  return (T)o;
}

(It turns out that the clazz parameter isn’t necessary, but at first I thought it was to justify the T type.)

I wrote some test code:

public static void main(final String... args) {
  List<String> list = new ArrayList<String>();

  ArrayList<String> c1 = cast(ArrayList.class, list);
  System.out.println("c1: " + c1.getClass().getName());

  Object c2 = cast(String.class, list);
  System.out.println("c2: " + c2.getClass().getName());

  String c3 = cast(String.class, list);
  System.out.println("c3: " + c3.getClass().getName());
}

and was surprised that the cast using String.class compiled – I can cast to any class. Of course it fails at runtime – the cast isn’t legal, so the output is

c1: java.util.ArrayList
c2: java.util.ArrayList
Exception in thread "main" java.lang.ClassCastException:
 java.util.ArrayList cannot be cast to java.lang.String

Looking at the decompiled code makes it clear what’s going on – it’s just erasure. The type of T is only used by the compiler to ensure (to the extent possible) that the code is valid, but the runtime type still has to be valid:

public static Object cast(Class clazz, Object o) {
  return o;
}

public static void main(String[] args) {
  List list = new ArrayList();
  ArrayList c1 = (ArrayList)cast(ArrayList.class, list);
  System.out.println((new StringBuilder("c1: ")).append(
         c1.getClass().getName()).toString());
  Object c2 = cast(String.class, list);
  System.out.println((new StringBuilder("c2: ")).append(
         c2.getClass().getName()).toString());
  String c3 = (String)cast(String.class, list);
  System.out.println((new StringBuilder("c3: ")).append(
         c3.getClass().getName()).toString());
}

Of course the Class parameter isn’t necessary:

@SuppressWarnings("unchecked")
public static <T> T cast(final Object o) {
  return (T)o;
}

public static void main(final String... args) {
  List<String> list = new ArrayList<String>();

  ArrayList<String> c1 = cast(list);
  System.out.println("c1: " + c1.getClass().getName());

  Object c2 = cast(list);
  System.out.println("c2: " + c2.getClass().getName());

  String c3 = cast(list);
  System.out.println("c3: " + c3.getClass().getName());
}

would have worked fine.

Creative Commons License
This work is licensed under a Creative Commons Attribution 3.0 License.