Java 7, TreeSet and NullPointerException.
Recently I tried to compile with java 7 a project developed with java 6. Lot of fun happened during tests execution, tests that in java 6 were running smoothly, with java 7, they were strangely failing! So, I had to understand why and this is what I discovered… The context first: In that project I have a simple Hibernate Entity more or less like the following.
package com.marco.test;
import javax.persistence.Column;
import javax.persistence.Entity;
import javax.persistence.Table;
import javax.persistence.UniqueConstraint;
import org.hibernate.validator.NotNull;
@Entity
@Table(...)
public class ABean {
...
private String name;
@Column(name = "name", nullable = false)
@NotNull
public String getName() {
return name;
}
public void setName(String name) {
this.name = name;
}
}note that the field “name” is nullable=false and marked with @NotNull. This to tell Hibernate to fail the validation in the case a user tries to create or update this column to Null. I also have a comparator for that Entity. This comparator uses the name field to compare the Entity ( this is just a simplified version of what I have in the project, of course I don’t order a bean based on the string length )
package com.marco.test;
import java.util.Comparator;
public class ABeanComparator implements Comparator<ABean> {
@Override
public int compare(ABean o1, ABean o2) {
if (o1.getName().length() > o2.getName().length()) {
return 1;
} else if (o1.getName().length() < o2.getName().length()) {
return -1;
} else {
return 0;
}
}
}note that there is no null check on the field name, in my project, Hibernate is already taking care of it. Now, I have a test that create one empty Entity and it stores it into a TreeSet, and then doees other stuff that we do not really care here. The beginning of the test is similar to the code below:
package com.marco.test;
import java.util.SortedSet;
import java.util.TreeSet;
public class SortedTestTest {
public static void main(String[] args) {
ABean aBean = new ABean();
SortedSet<ABean> sortedSet = new TreeSet<ABean>(new ABeanComparator());
sortedSet.add(aBean);
}
}If I run this with java 6, everything is OK. But, with java 7 I have a NullPointerException.
Exception in thread "main" java.lang.NullPointerException at com.marco.test.ABeanComparator.compare(ABeanComparator.java:9) at com.marco.test.ABeanComparator.compare(ABeanComparator.java:1) at java.util.TreeMap.compare(TreeMap.java:1188) at java.util.TreeMap.put(TreeMap.java:531) at java.util.TreeSet.add(TreeSet.java:255) at com.marco.test.SortedTestTest.main(SortedTestTest.java:14)
Why? This is why:
public V put(K key, V value) {
Entry<K,V> t = root;
if (t == null) {
compare(key, key); // type (and possibly null) check
root = new Entry<>(key, value, null);
size = 1;
modCount++;
return null;
}In java 7 when the first Object is added ( if (t== null) ) to a TreeSet, a compare against itself (compare(key,key)) is executed. The compare method will then call the comparator (if there is one) and we will have the NullPointerException on the name property.
// Little utilities
/**
* Compares two keys using the correct comparison method for this TreeMap.
*/
final int compare(Object k1, Object k2) {
return comparator==null ? ((Comparable<? super K>)k1).compareTo((K)k2)
: comparator.compare((K)k1, (K)k2);
}This raises more questions than answers:
- Why running a compare if you know that the Object in the TreeSet is the first and only one ?
- My guess is that what they wanted to do was running a simple Null check.
- Why not create a proper null check method ?
- No Answer
- Why wasting CPU and memory running a comparison that is not needed ?
- No Answer
- Why compare an object against itself (compare(key,key))??
- No Answer
This is the put method of the TreeSet in java 6 and as you can see the compare was commented out.
public V put(K key, V value) {
Entry<K, V> t = root;
if (t == null) {
// TBD:
// 5045147: (coll) Adding null to an empty TreeSet should
// throw NullPointerException
//
// compare(key, key); // type check
root = new Entry<K, V>(key, value, null);
size = 1;
modCount++;
return null;
}You see the comment? Adding null to an empty TreeSet should throw NullPointerException. So just check if key is null, don’t run a useless comparison! The conclusion? Always try to analyze the code you use, because even in the jdk there is bad code!
The compare call is to throw a ClassCastException with the first put().
Otherwise, with older JVM, we could put an Object, with success, then put an other Object and then have a ClassCastException.
Understood, but I’m convinced that the scope of a compare method should be to compare 2 Objects, not to to be used as Cast or Null check against a single Object.
It seems more to be a hack to fix a weird comportment of the JVM, so I’m ok with you :)
I actually find it quite reasonable when adding stuff into a SORTED set to make sure the first added item will be comparable to others. If you add an item, whose comparison to any other item (which you might be adding later) will throw a NPE, you will kind of block the set, because you won’t be able to add any more items then (the ‘add’ method must contain a call to ‘compare’ which will fail). Try it with java 6 and add two instances of ABean into the set, each of which will have null value as the name.… Read more »
The Hibernate annotations have effect in the context of the whole design of the application.
I don’t put null checks everywhere if I already know that Hibernate will taking care of the mandatory fields (the one in the post is just a simplified version, no need to show the EntityManager there).
I agree on the fact that if there is something wrong you should stop immediately the process, but using a compare method on a single Object to to this, I still feel is not right.
A comparator should be able to compare object A to object A. And if it is not able to, something is wrong, because *most likely* it will not be able to compare object B to object A, therefore not allowing you to add object B to the set later on after you added A. The check might not look very logical at first sight, but in my opinion it is actually quite useful and helps a programmer’s mistake show up earlier…