Grails pitfalls: Don't do flush:true when you actually want a transaction

Putting persistence code in controllers leads to poor maintenance, reduces the chance of reusing code, and makes controllers extremely hard to test.

One of the first problems a junior programmer experience is to realize the domain class hasn't been saved when he/she expected.

After explaining to them the best way of approaching this type of situations is to put the persistence code in the service layer, they ignore you because they find they can force the commit doing:

  product.save(flush:true)

For what else would be this parameter for? (Sarcasm ;)) If you were only saving a given instance without any relationship...still I wouldn't do that. At least forget about the use of flush:true in that context and use withTransaction:

  Product.withTransaction{
    product.save()
  }

I can see a transaction going on here. Let's say we have two given transactions trying to do something at once.

  Product.withTransaction{
    product.status = ACTIVE
    product.save(flush:true) // without this line the total number will be all of them but this one
    Product.countByByStatus(ACTIVE)
  }

In the previous code without forcing flush:true we would have been omitting the product we were saving in our transaction. First time you see this code you could be thinking, OMG they are committing before the end of the transaction.

Sort answer would be: we ARE NOT committing anything yet, we are sending to the database all instructions until this point (flushing) allowing other transactions in their way to see the our transaction status. If something goes wrong before the transaction ends then changes won't persist.

The problem is that when there's no transaction boundaries the default behavior of flush:true is to commit changes. But if you do establish the boundaries, we would expect the commit/rollback to happen once we exit the transaction's scope.

UPDATE: Somebody at work asked me how to prove this statement. I've created an entry to explain this a little bit more.

Using flush:true the way people normally use it is not right, but is even funnier when they realize they can't do

product.delete(flush:true)

All these could have been avoided if people were used to put their persistence code in services. There are a couple of benefits when using services:

• You no longer have to worry about opening and closing a transaction. Convention over configuration.

A simplistic view of transactions in Grail's services would be that by default the code within a service's method is executed within a transaction. If the method throws an exception then that code will be rolled back. If everything went ok then all changes will be commited. Although you could but now you don't have to declare explicitly the boundaries of the transaction.

class ProductService{

   def addProductToCart(Product p, Cart c){ // transaction starts here

      //... code within a transaction

   } // transaction ends here (if everything went ok ;) )

}

EXPLANATORY UPDATE:

To be accurate the transaction begins not at the beginning of the method but at the service's invocation time. What does it means? It means that when you're invoking a service's method let's say from the controller, a Spring interceptor, intercepts the call and wraps that call inside a given transaction scope.

One more thing to keep in mind is that the default propagation of transactions is REQUIRED, that means that if we're calling a transactional service's method from another transactional method the latter uses the former transaction scope, or in cause there were none it creates a new one. That explains why we can call some other service methods from our service method without committing previous statements, because there were a pre-existent transaction.

You can always change the default propagation policy using the @Transactional annotation from Spring in your service methods, or using Spring AOP advices to configure transactional scopes.

Anyway if you want to dive into this topic please read "Spring Declarative Transaction Management".

• It keeps separated your view logic (controllers) from your persistence code (services)

It's clear that less code is always easier to review. Hence if you could keep separated view logic from business logic everything starts looking clearer.  Let's see an example:

If you have the following code:

class ContractController{

  def saveContract(Contract contract,Address address){

    if(contract.validate()){       

       contract.deliveryAddress = address.save(flush:true)   
       contract.save(flush:true,failOnError:true)  

       render(view:'ok')    

    } else {

       render(view:'ko')

    }

  }

}

If you had to unit test this code you should have to mock every domain class behavior (Contract and Address), and sometimes that's really a pain in the ass. Besides the fact that the address could have been saved without succeeding on doing the same with the contract instance. That type of inconsistencies are really dangerous.

Wouldn't be better to split the code like this:

class ContractController{

  def contractService 

  def saveContract(Contract contract,Address address){

    if(c.validate()){   

       contractService.saveContract(contract,address)
       render(view:'ok')    

    } else {

       render(view:'ko')

    }

  }

}

class ContractService{

   def saveContract(Contract c,Address address){

       c.deliveryAddress = address.save()
       c.save(failOnError:true) 

   }

}

Now is easier to mock the service's method, and you only have to focus on testing the view logic.