Service layer woes
Posted by Peter Veentjer at around evening time: July 18, 2007
Service layers are one of the most important parts of an enterprise system in my opinion, for a lot of reasons:
- defines what your system can and can't do
- a good location for transaction demarcation
- a good location for security
And although a lot has been written about the service layer (Patterns of Enterprise Applications from Martin Fowler is one of the most famous books about this subject), there are a lot of ways to implement it. In this post I'll walk through a few examples that are improved step by step.
An example of some bad code I have seen too often:
[code]
class BonusController{
EmployeeDao employeeDao;
EmployeeService employeeService;
void someAction(Request request){
long id = new Long(request.get("id"));
Employee employee = employeeDao.findById(id);
employee.setSalary(employee.getSalary()+100);
employeeService.saveOrUpdate(employee);
}
}
[/code]
There is a lot wrong with this code. The most important issues are:
- the bonus logic is hard to reuse because it is integrated inside the controller, so duplication is not uncommon in such systems.
- it is hard to maintain/extend because the bonus logic is overshadowed by all the plumbing.
- it is very easy to bypass any form of validation because the database is exposed by the saveOrUpdate method. If the employeeService is used inside a remoting layer, and you have 'deserialized' an employee, god knows what kind of properties are placed in the database when it is saved.
A better, but still bad solution, is the following:
[code]
class BonusController{
EmployeeDao employeeDao;
EmployeeService employeeService;
void someAction(Request request){
long id = new Long(request.get("id"));
Employee employee = employeeDao.findById(id);
employeeService.giveBonus(employee);
}
}
class EmployeeService{
EmployeeDao employeeDao;
void giveBonus(Employee employee){
...execute logic for bonus
employeeDao.saveOrUpdate(employee);
}
}
[/code]
The horrific saveOrUpdate method has been removed and the bonus logic has been moved inside the service. But we are not out of the woods yet:
- I'm still able to inject an employee that bypasses all checks
- Transactional behavior is vague at best and erroneous at worst. If multiple transactions are working on the same employee, and if there is no form of offline locking, then one transaction is able to overwrite the changes of the other: this problem is called the lost update problem. The consequence of a lost update is that we could loose a bonus! Beside the lost update there are other problems: Imagine what happens when one transaction has deleted the employee and the second one commits. If all logic is executed inside a single transaction, you can make use of standard solutions to prevent these problems (optimistic/pessimistic-locks, isolation levels etc).
Another issue I find smelly, is that no explicit transaction is used to access the employeeDao inside the controller. Most databases are friendly enough to create a transaction if one is missing, and in the previous example it is quite clear that an employeeDao is used inside a controller, but I have seen my share of deeply tucked away dao's inside presentationlayer support objects like Spring editors. I think this is a good example of misuse of technology.
The simplest solution to the problems above is the following:
[code]
class BonusController{
void someAction(Request request){
long id = new Long(request.get("id"));
employeeService.giveBonus(id);
}
}
class EmployeeService{
void giveBonus(long employeeId){
Employee employee = employeeDao.findById(employeeId);
....bonus logic
}
}
[/code]
All previous mentioned problems are solved, and even the controller has been simplified. This approach even is agnostic to the technique you use to realize the bonus logic. You could create a transaction script:
[code]
class EmployeeService{
void giveBonus(long employeeId){
Employee employee = employeeDao.findById(employeeId);
employee.setSalary(employee.getSalary()+100);
}
}
[/code]
You could place it inside the domain object of you have a richer domain model (something I'm for).
[code]
class EmployeeService{
EmployeeDao employeeDao;
void giveBonus(long employeeId){
Employee employee = employeeDao.findById(employeeId);
employee.giveBonus();
}
}
[/code]
And if your bonus logic is very complex, you could even decide to use a domain service (see Domain Driven Design from Eric Evans, chapter 5, subject: Services for more information) that contains the actual logic:
[code]
class EmployeeService{
EmployeeDao employeeDao;
BonusDomainService bonusDomainService;
void giveBonus(long employeeId){
Employee employee = employeeDao.findById(employeeId);
bonusDomainService.giveBonus(employee);
}
}
[/code]
For those that have watched carefully, the EmployeeService provides another very important function: it provides a layer of abstraction. The outside world is not coupled to the implementation, and this means that changes in the implementation won't effect the clients (presentationlayer, remoting layers).
Filed under:
July 18th, 2007 at 8:14 pm
[…] http://blog.xebia.com/2007/07/18/service-layer-woes/ […]
July 19th, 2007 at 10:25 am
The design is still somewhat flawed with a locking issue because you are retrieving the employee just before you are giving him his bonus. It could be that another manager had already given him his bonus at the same time, thus getting the latest ‘version’ of that employee.
You need to cache the version of the employee somehow to avoid these kind of issues.
July 19th, 2007 at 7:57 pm
Hi Fox, I agree that the design is feasible for improvement.
The goal of this blogpost however was not to provide a solution for concurrency problems, but to show the importance of a well defined (application)service layer.
I had a discussion with a very smart colleague of mine about isolation problems in the current version. What if 2 different managers want to give you a bonus at the end of the year based on your billability. They give you the bonus based on the assumption that you were not given a bonus already. With the current approach it could happen that you will receive a bonus twice.
A solution to this approach would be to use an optimistic lock. But having a version next to the id doesn’t feel good (feels technical)… or does it? We came to the conclusion that it would be an idea to introduce a tuple that contains the id and the version of the entity. This tuple can identify an entity over time.
How do it work?
Controller pseudo code:
long id = request.get(”id”)
long version = request.get(”version”)
VersionedId versionedId = new VersionedId(id,version)
employeeService.giveBonus(versionedId);
giveBonus pseudo code:
Employee employee = employeeDao.find(versionedId)
if(employee == null)
throw new AlreadyChangedException()
employee.giveBonus();
If a different manager already has committed his change, the find of the dao would return null because the expected version of that object doesn’t exist anymore. And eventually the manager that comitted last, will get a message that some else already changed the information.
Now to get back to your suggestion.
It is correct that some form of locking needs to be applied inside the giveBonus function to prevent lost updates . This problem can be solved in different ways and the most popular once are:
-use a repeatable read isolation level (not always the best choice performance wise)
-use a select for update when loading the employee from the database
-rely on the optimistic locking functionality that already is being used in the pseudocode above. As soon as the object is flushed to db, you will get an optimistic locking failure.
I don’t completely understand your cache solution. Can you elaborate on that?
July 19th, 2007 at 8:11 pm
Or am I explaining what you meant?
July 19th, 2007 at 8:54 pm
First of all, nice reply
The options you give to prevent lost updates look viable solutions to me.
But you need, as you acknowledge, a way to differentiate the ‘detached’ object in a way so that you can verify if it was already updated. Hibernate allows this kind of optmistic locking by its version stamp.
What I meant with the cache is that you could keep the version of the last retrieved object in the session (associated with its id), available in some kind of cache (or the whole object itself in the Request or HttpSession), to allow checking of a stale version. But you could see these kind of tricks as workarounds for the problem.
July 27th, 2007 at 6:16 pm
Service Layers, are great, but replacing the the actual domain object with just an ID is not always the best way. A better solution is to use the JPA which allows the use of binding data to protected fields. Thus, for someone to change a password a call has to made to userDAO.setPassword(user,pasw), since the user will neither have a public getPassword() nor a setPassword() method. Sure someone may use reflection to make the protected field accessible, but the above scheme will prevent someone from casually changing a field on a database.
Thus we can keep high level domain objects without fearing an em.update() will allow someone to ca usually slip in a change on the database.