10

I have been using a DAO pattern to provide access to my persistence tier in an application that I have been building.

One of the things that I have implemented is a 'wrapper' around my DAO implementations for purposes of validation. The wrapper takes in an instance of my DAO as a constructor parameter, and implements a similar interface as the DAO, with the exception being the types of exceptions thrown.

For example:

Business Logic Interface

public interface UserBLInt {

   private void assignRightToUser(int userId, int rightId) throws SomeAppException;

}

DAO Interface

public interface UserDAOInt {

   private void assignRightToUser(int userId, int rightId) throws SomeJPAExcption;

}

Business Logic Implementation

public class UserBLImpl implements  UserBLInt {

   private UserDAOInt userDAO;

   public UserBLImpl(UserDAOInt userDAO){
      this.userDAO = userDAO;
   }

   @Override
   private void assignRightToUser(int userId, int rightId) throws SomeAppException{
      if(!userExists(userId){
         //throw an exception
      }
      try{
         userDAO.assignRightToUser(userId, rightId);
      } catch(SomeJpAException e){
        throw new SomeAppException(some message);
      }
   } 

}

DAO Implementation

public class UserDAOImpl implements UserDAOInt {
      //......
      @Override
      public void assignRightToUser(int userId, int rightId){
         em.getTransaction().begin();
         User userToAssignRightTo = em.find(User.class, userId);
         userToAssignRightTo.getRights().add(em.find(Right.class, rightId));
         em.getTransaction().commit();
      }
}

This is just a quick example, but my question is, it seems 'redundant' within the DAO implementation to do another check to be sure that the User isn't null before adding a Right, but, as a programmer, I see an opportunity for a null pointer.

Obviously I could add a null check after calling find on the entity manager and throw an exception if null is returned, but that's the whole purpose of having the DAO wrapped in a business logic implementation, to do all the validation work beforehand, so that the DAO code is clean and doesn't have to do much in the way of null checks or much logic at all. Since I have the wrapper around the DAO, is it still a good idea to do the null checking anyway in the DAO? I know in theory the object could be deleted between the business logic call and the dao call, it's just unlikely, and checking for null seems like duplicated work. What is the best practice for a situation like this?

EDIT:

Does this look like a suitable DAO modification?

public EntityManager beginTransaction(){
    EntityManager entityManager = entityManagerFactory.createEntityManager();
    EntityTransaction entityTransaction = entityManager.getTransaction();
    entityTransaction.begin();
    return entityManager;
}

public void rollback(EntityManager entityManager){
    entityManager.getTransaction().rollback();
    entityManager.close();
}

public void commit(EntityManager entityManager){
    entityManager.getTransaction().commit();
    entityManager.close();
}
user1154644
  • 4,491
  • 16
  • 59
  • 102

3 Answers3

8

DAOs, although a bit of a now generic and overused term, are (usually) meant to abtract the Data layer (so among other benefits, it could be changed without having to touch the rest of the app).

It seems, though, your DAO is actually doing a lot more than just abstracting the data layer. In:

public class UserDAOImpl implements UserDAOInt {
  ......
  @Override
  public void assignRightToUser(int userId, int rightId){
     em.getTransaction().begin();
     User userToAssignRightTo = em.find(User.class, userId);
     userToAssignRightTo.getRights().add(em.find(Right.class, rightId));
     em.getTransaction().commit();
  }
}

Your DAO knows business logic. It knows that assigning a right to auser is adding a right to the list of rights. (It may seem obvious that assigning a right is just adding it to a list, but imagine this could get, in the future, much more complicated, with side-effects to other users and rights and so on.)

So this assigning does not belong in the DAO. It should be in the business layer. Your DAO should have just something like userDAO.save(user) that the business layer would call once it is done setting the rights and stuff.


Another problem: Your transaction is too localized. It is almost not a transaction.

Remember the transaction is a business unit, something you do atomic (a "batch" of) business work within, not just something you open because the EntityManager makes you.

What I mean, in terms of code, is that the business layer should have the initiative of opening the transaction, not the DAO (actually, the DAO should have "open a transaction" as a service -- method -- that would be called).

Consider, then, opening the transaction in the Business Layer:

public class UserBLImpl implements  UserBLInt {
   ...
   @Override
   private void assignRightToUser(int userId, int rightId) throws SomeAppException{
      userDAO.beginTransaction(); // or .beginUnitOfWork(), if you wanna be fancy

      if(!userExists(userId){
         //throw an exception
         // DON'T FORGET TO ROLLBACK!!! before throwing the exception
      }
      try{
         userDAO.assignRightToUser(userId, rightId);
      } catch(SomeJpAException e){
        throw new SomeAppException(some message);
      }

      userDAO.commit();
   } 
}

Now, to your question: that risk of the DB changing between the userExists() if and the userDAO persisting it still exists... but you have options:

(1) Lock the user until the transaction is over; or (2) Let it be.

1: If the risk of the user being messed up with between those two commands is high (say your system has a lot of concurrent users) and if that problem happened it would be big deal, then consider locking the user for the whole transaction; that is, if I begin working with it, no other transaction may change it.

  • Another (better) solution to that if your system has tons os concurrent users is to "design the problem away", that is, rework your design so that what is changed in a business transaction has a more strict (smaller) scope -- the scope of your example is small enough, so this advice may not make so much sense, but just consider your business transaction did a whole lot more (then making it do a whole lot less, each in its turn, could be the solution). This is a whole topic all by itself, so I won't go into more details here, just keep your mind open to that.

2: Another possibility, and you will figure this is the most common approach, if you are dealing with a SQL DB that has constraint checks, such as UNIQUE, is just to let the DAO exception blow away. I mean, it will be such a rare and near-impossible thing to happen that you may as well deal with it by just accepting it can happen and your system will just display a nice message like "Something went wrong, please try again" -- this is just basic costs vs. benefits weighting.


Update:

Programatic transaction handling can be tricky (no wonder declarative alternatives, such as Spring and EJB/CDI, are so used). Still, we don't always have the luxury of using them (maybe you are adapting a legacy system, who knows). So here's a suggestion: https://gist.github.com/acdcjunior/94363ea5cdbf4cae41c7

acdcjunior
  • 132,397
  • 37
  • 331
  • 304
  • I see your point about the saveUser operation, but from the perspective of the entity manager, what operation would be called to 'update' the user in the database? Would the business logic layer add the role to the user, and the dao layer would simply call merge? – user1154644 Apr 09 '15 at 12:30
  • Yes; actually the `saveUser()` would update or insert, the DAO should be able to figure out what the user needs. An example of this [can be found in the Petclinic app](https://github.com/spring-projects/spring-petclinic/blob/master/src/main/java/org/springframework/samples/petclinic/repository/jpa/JpaPetRepositoryImpl.java#L55). – acdcjunior Apr 09 '15 at 14:47
  • About declarative transaction management (aka using `@Transactional`), I didn't go into it as you clearly weren't using EJB/CDI/Spring, but using the declarative style, if possible, is always better than using the programatic (i.e. calling `beginTransaction()` "manually") way, as the declarative way handles the commits/rollbacks and else much more cleanly (less boilerplate code). But that's just it. I mean, anything the `@Transactional` would do you can do programatically, all it takes is more attention. – acdcjunior Apr 09 '15 at 14:51
  • You can create a `.beginTransaction()` method in your DAO that would create the `EntityManager`, get a transaction, begin it and return. Then two other methods: a `.rollback()` and a `.commit()` which would call the `EntityManager`'s transaction's methods of the same name and then should close the `EntityManager`. Makes sense? – acdcjunior Apr 13 '15 at 19:14
  • `EntityManager`s are not thread-safe, so you really must have only one per thread. If you have one per DAO and multiple threads access the same DAO instance, then you are certainly get into (nasty) trouble. Here's what I suggest: https://gist.github.com/acdcjunior/94363ea5cdbf4cae41c7 – acdcjunior Apr 25 '15 at 03:33
  • Besides, I would advise against returning the `EntityManager` to the business layer (classes). That is implementation detail of the data acces layer leaking. – acdcjunior Apr 25 '15 at 03:41
  • Thanks for the example, I'm working on implementing this.For the BL Impl, do you have an example of what calling a simple method like findUsersByName() would look like? Creating the UnitOfWork begins a transaction, but the findUsersByName method doesnt require a transaction – user1154644 Apr 25 '15 at 04:08
  • OK, good point. I updated the gist to address calling methods outside transactions. Check it out. (For exactly what has changed, check [the last Revision on the Revisions tab](https://gist.github.com/acdcjunior/94363ea5cdbf4cae41c7/revisions).) – acdcjunior Apr 25 '15 at 17:10
  • thanks so much for the example, I really really appreciate it – user1154644 Apr 26 '15 at 21:10
  • Not a problem at all. Let me know if any problem arises again, so we can adapt this model. Also, for future projects, I suggest you take a look at declarative transaction frameworks like Spring or CDI(EJB), if that's a possibility for your projects, of course, as they make that transaction handling simpler (less boilerplate code). – acdcjunior Apr 27 '15 at 15:32
3

The DAO has too much logic here. The role of a DAO is not to assign rights to a user. That is business logic. The role of a DAO is to find users or find rights. The code that is in the DAO should be in the service:

interface UserDAO {
    User findById(int userId);
    //...
}

interface RightDAO {
    Right findById(int rightId);
    // ...
}

public class UserBLImpl implements  UserBLInt {

    private UserDAO userDAO;
    private RightDAO rightDAO;

    // ...

    @Override
    public void assignRightToUser(int userId, int rightId) {
        User u = userDAO.findById(userId);
        Right right = rightDAO.findById(rightId);
        // do some null checks and throw exception
        user.addRight(right);
    } 
}

This also shows a fundamental problem in your design: the transactions must not be started at the DAO layer. They must be started at the service layer. That's what allows DAO methods to be reusable and services methods to use several DAOs in a single transaction. Not doing so will lead to anemic services that will call DAO methods containing the whole business logic, as your code shows.

And that's why frameworks like EJBs or Spring allow demarcating transactions declaratively: you don't need to explicitely start and commit trannsactions, handle exceptions and rollback exceptions. All you have to do is mark a service method as transactional using an annotation.

JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
  • I've seen EJB's used and their methods marked as requiring a new transaction. If this was a standard java application and not a j2ee application, would it be ok to manually handle the rollbacks? – user1154644 Apr 09 '15 at 12:31
  • I'd use Spring in that case. – JB Nizet Apr 09 '15 at 13:36
0

Actually DAO's themselves became an anti pattern and shouldn't really be used anymore.

http://www.adam-bien.com/roller/abien/entry/how_to_deal_with_j2ee

http://www.adam-bien.com/roller/abien/entry/jpa_ejb3_killed_the_dao

Java EE Architecture - Are DAO's still recommended when using an ORM like JPA 2?

Community
  • 1
  • 1
GregD
  • 1,884
  • 2
  • 28
  • 55