Why you should be careful when using Lombok or other code generation tools

Look at the following code:



Will the testHashCode succeed or fail?

The answer it will fail, since the second assert: assertTrue(set.contains(myEntity)) will not find myEntity in the HashSet.
But it's there, right? It was never removed.

So what we have here is: 1. Business problem, since it's impossible to get object from set that is there. 2. Memory leak.

But how did it happen?

The problem is with Lombok's @Data annotation. It's a very convenient annotation that auto-generates all methods the java utility methods: equals, hashCode, toString as well as relevant constructors and getters.
Yes, it's very convenient, but a hidden problem is introduced: equals and hashCode include all fields and when value of a field changes, the hashCode returns a different value. Therefore, the object cannot be found in HashSet anymore.

This problem is not unique to Lombok. Exactly the same problem will occur if you write the method yourself by using mutable fields or if you use any other code-generation or reflection tools.
However, if you write code yourself, it's a bit more visible, while with Lombok it's kind of woodoo.

The best practices here are not related to Lombok or other library and are quite simple:
1. As much as possible try to make your class immutable.
2. Even if a class is mutable, are all fields mutable? Use only immutable fields in hashCode and equals and you will be safe.
3. If a class is completely mutable and you cannot rely on some immutable fields, reconsider if you need to override hashCode and equals at all. Is default implementation sufficient?
4. If none of above doesn't work for you - document. Put a HUGE WARNING in javadoc explaining why the users of the class must be careful, if they decide to store the instances in HashSet or as a key in a HashMap.

Deadlock in Jetty or Be Careful while Synchronizing

About nine months ago I reported a bug to the Jetty community that session timeout doesn't work properly. The bug was fixed quite quickly, but nine months later I have discovered that the fix leads to a deadlock in some scenarios.

Deadlock in Jetty illustrates an interesting coding guideline that you must follow while writing your code.

So what happened in Jetty?

Consider a class A that carries state and lives in a multi-threaded environment. Obviously this class must be synchronized.
Consider that you can subscribe to events of class A. So let's say class you must implement an interface I that will be notified when something important in class A happens.
Let's assume that the method in which class A invokes instances of I is synchronized (A carries state, remember?)
Let's also assume that your implementation of I also carries state and must be synchronized as well.

And now let's see what happens:

Thread-1: Some event on A occurs. It's wants to notify I, but first acquires LOCK_A and then invokes method of I. Method of I tries to change state of I, so it tries to acquire LOCK_I, but it was already acquired by Thread-2.

Thread-2: Runs on I. It changes the state of I, so it acquires the LOCK_I. During the change it needs some information of A. It tries to get it, but LOCK_A was already acquired by class A.

And here we have a deadlock.

So what is wrong here?
The most wrong part is of class A: it invokes method of some other class while it is locked. BAD! Finish the lock before calling someone else! And when I say "someone else" I include the other methods of the class! (What really happened in Jetty is that in class A method f1() was synchronized. Method f1() called to f2(), which called to f3(), which called to f4(), which called to I. It was clear in f4() that no synchronization is needed. But the mistake is actually in f1()!)
So you have some member to change? acquire the lock, change them and release the lock.

In addition, the situation could be a little improved if Read-Write lock was used instead of synchronized: most of the access to class are to read data. May be if LOCK_A was split to READ_LOCK_A and WRITE_LOCK_A; and LOCK_I was split to READ_LOCK_I and WRITE_LOCK_I, it was not causing the deadlock. But this is not about preventing the situation, but about improving.

Summary

The main point of my post is that when synchronizing, find the critical section and synchronize it only! Do not call other methods (even if they are of the same class) from the critical section: gather all information before and notify everyone else after.

Hibernate/JPA Best Practices

Here come some of Hibernate/JPA best practices.
Please notice that this guide does not intend to cover Hibernate/JPA at all, but only to provide some best practices. Personally I learned Hibernate using Hibernate in Action.

I really appreciate any comments saying what do you think about this practice, why it's wrong, and what additional practices should be added.

So let's start:

Override hashCode() and equals()

The Hibernate reference states that "It is recommended that you implement equals() and hashCode() to compare the natural key properties of the entity."
The reason is simple: different instances of the class may represent the same record in the database. Therefore, when comparing these two instances, you'd like to get equal result, while default equals implementation will return not-equal since it's not the same instance.

This becomes really important, when working with collections, especially with Sets. You don't want the same object to appear twice in set, right?

Overriding the hashCode() and equals() is not a very complex goal, but you must be very careful:
1. Remember that two equal objects must return the same hash code. Therefore, you cannot use auto-generated Hibernate id in hashCode() - this value is not assigned for the newly created objects. After persistence occurs, the value is assigned, so the hashCode will change, when the object actually wasn't changed!
2. Changing the fields that participate in hashCode() will change the hash code value. So if your object is stored in a Set (or it's key of a Map), you won't be able to retrieve it from the set anymore - one hash was used for insert and another one was used for retrieve.
So basically you'll need to remember not to change objects that are stored in sets! And this is really important!
So you may ask: how will I know who stored my object in a set?
My answer is simple: you cannot know this, unless you don't give your objects out. So you are the only person who is using these objects, so you know how they are kept, right?
Storing the objects without giving them out is not so weird idea: keep the persistence layer away from the business logic and return a copy of object when required.
Another option: return immutable objects to the business tier. So the business tier won't be able to change them. When the change is required - provide a special API. Thus the objects won't change accidentally.

Try to Make All Object Immutable

This may sound weird, how exactly the persistent objects can be immutable. But in the previous part I described why it's important. And actually it can be quite easily achieved:
1. Make all setters private. Thus it will be impossible to call them without using reflection. (Hibernate will use reflection and populate the properties during the object retrieval)
2. When returning collections, wrap them with Collections.unmodifiable. So the user won't be able to modify your collections.
3. Allow changes only via special methods.

Return Copy of Persistent Objects to the Business Tier

So no accidental change in hash code may occur.
Additionally when filling the business objects, a lot of potential problems may be resolved. Consider that the persistent object contains a lazy collection. If the object is returned as is to the business tier, the lazy elements in the collections can be accessed after the transaction was closed, therefore the query to the database will fail and user will get an exception.

h3. Change Data in Collections Only via Special Methods
When having associations, take care of this association via special method: for example, Parent class will have method addChild(Child child).
When returning the values of collections wrap them using Collections.unmodifiable to prevent accidental changes.
This is useful both to handle bidirectional associations correctly and to prevent accidental changes in hash code.

Summary

1. Override hashCode() and equals() of the entities using the natural key properties of the entity.
2. Don't compare auto-generated id in hashCode().
3. Don't change the properties that participate in the hashCode() for objects stored in collections that use hash code (especially Sets or Maps). If such a change must occur, reinsert object into collection. Remember that remove must occur before the property is updated.
4. Keep the persistence tier away from the business logic as much as possible. Don't pass the persistence objects to the business at all.
5. Make your objects immutable (or semi-immutable): make all setters private, return collection values only wrapped with Collections.unmodifiable, make changes in collections only via special methods.

---

Recommended Reading

1. Hibernate in Action (In Action series)
2. Java Persistence with Hibernate
3.The Best Software Writing I: Selected and Introduced by Joel Spolsky (v. 1)

---

Calling non-private methods from constructors

Every time I join a new project there is usually a discussion about the Coding Guidelines. Usually it includes stupid guidelines, like "use underscore prefix with the fields" or "end constants with "CN".
In extreme stupid cases the guidelines require surrounding methods with "try-catch" blocks, while catch does something like "log.error(exception); return null;" and later it becomes impossible to understand from the log what actually has happened.

Usually I insist on adding the following guideline: "Never call methods that can be overridden by a subclass from the constructor." It means that if you decide to call a method from the constructor, it should be either private, or static, or final. It's also possible to declare the whole class final.

Why?

Run the following example and see what happens.

public class Base {

public static class A {

public A() {
printHello();     // calling method from constructor
}

protected void printHello() {
System.out.println("Hello!");
}
}

public static class B extends A {

private String helloWorld = "Hello world!";

public B() {

}

// override printHello
protected void printHello() {
System.out.println(helloWorld.length());    // Null pointer exception will be thrown here.
}
}

public static void main(String[] args) {
B b = new B();
}

}

---

Recommended Reading

1. How Would You Move Mount Fuji? Microsoft's Cult of the Puzzle - How the World's Smartest Company Selects the Most Creative Thinkers
2. User Interface Design for Programmers
3. Code Complete: A Practical Handbook of Software Construction

---

Tips regarding Spring's PropertyPlaceholderConfigurer

Most of the Spring users are familiar with the PropertyPlaceholderConfigurer that is used to replace placeholders defined in the context xml with the actual values. In this post I want to describe some of its less known features.

Ignore Unresolvable Placeholders

By default a PropertyPlaceholderConfigurer throws an exception, when it completes resolving the placeholders, and there are still unresolved placeholders left. However, suppose that your application is built from the several modules, which are combined to the same spring context. Each module has its own PropertyPlaceholderConfigurer and simply cannot resolve the others. You won't like it to fail the whole application loading, right?

Setting property ignoreUnresolvablePlaceholders to true will tell the PropertyPlaceholderConfigurer to ignore the unresolvable placeholders.

In general, I would suggest for all modules to set this property to true to ensure that none of the modules fails the whole system.

Placing validating PropertyPlaceholderConfigurer

"But" - you may say - "I still want to know that the context is incorrect. How can I do it, if all the PropertyPlaceholderConfigurers are set to ignore the unresolvable placeholders?"

To do so, you'll need to place a validating PropertyPlaceholderConfigurer, which task will be to run after all PropertyPlaceholderConfigurers completed their job and check, if all the properties were resolved. Its definition is very simple and won't include any special configuration.

"But" - you should ask now - "I don't know the order in which the PropertyPlaceholderConfigurers run".

Actually it's untrue and you know the order, you just don't know about it:

Setting the Order of PropertyPlaceholderConfigurers

PropertyPlaceholderConfigurer is a BeanFactoryPostProcessor that implements PriorityOrdered interface. This cause it to run before the BeanFactoryPostProcessors that implement Ordered interface and they run before the BeanFactoryPostProcessors that don't implement these interfaces at all.
Both PriorityOrdered and Ordered objects implement a method getOrder that returns an order in which the objects should be applied. Zero means a highest priority. Integer.MAX_VALUE means the lowest priority.
By default the order is set to Integer.MAX_VALUE.
And what happens if two PropertyPlaceholderConfigurer have a same priority? They run in the order of their definition in the context xml file.

Overriding Properties Defined in the PropertyPlaceholderConfigurer

Consider the following situating: you are using some module, which has defined its own PropertyPlaceholderConfigurer and you want to override a property. Without "order" and "ignoreUnresolvablePlaceholders" properties it can be quite cumbersome. However, when using "order" and "ignoreUnresolvablePlaceholders" it becomes very simple:

1. Define a PropertyPlaceholderConfigurer with order set to a lower order (higher priority) then a PropertyPlaceholderConfigurer in the module.
2. Set ignoreUnresolvablePlaceholders to true, since you don't want to override all the properties. Moreover, there are may be additional modules, who have their own placeholders...
3. Define the properties you want to override.

Summary

When using a PropertyPlaceholderConfigurer you need to identify if your product is a stand alone application or a module in a bigger system. The default PropertyPlaceholderConfigurer is good for a stand alone application. But if you are writing the module make sure that:

1. The order property should be set between 1 and Integer.MAX_VALUE. I would suggest setting it somewhere between 10000 and Integer.MAX_VALUE-10000 to ensure that anyone can insert a PropertyPlaceholderConfigurer both before and after yours. I assume that none system would have more then 10000 PropertyPlaceholderConfigurers.
2. Set ignoreUnresolvablePlaceholders to true, to ensure that your module won't fail because of others placeholders. Your placeholder is still responsible to fill the placeholders in your module and I would recommend to create a unit test, which will hold a validating PropertyPlaceholderConfigurer and fail if someone forgets to resolve a placeholder.
3. The main system should include the validating PropertyPlaceholderConfigurer to ensure that all modules have filled their placeholders.
4. Include module name and version in the properties names. This will reduce conflicts and backward compatibility problems. Example: if module has name "example" and version is "4.1", make all properties to start with "example.4.1" or "example_4_1". Meaning "debug" propery would be called "example.4.1.debug" or "example_4_1_debug".

---

Recommended Reading

1. Spring in Action
2. Effective Java
3. Joel on Software: And on Diverse and Occasionally Related Matters That Will Prove of Interest to Software Developers, Designers, and Managers, and to Those Who, Whether by Good Fortune or Ill Luck, Work with Them in Some Capacity
4. Small Gods

---