Grails

Make binding errors behave more like validation errors

Details

  • Type: Sub-task Sub-task
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 1.0.3
  • Fix Version/s: 1.1.1
  • Component/s: None
  • Labels:
    None

Description

An explanation of the current behaviour and why it's not exactly ideal is given here:

http://www.cacoethes.co.uk/blog/index.php?p=38

One possible solution is to use the Hibernate's Session.setReadOnly() method to ensure that changes to a domain instance are not persisted at flush time if there are any binding or validation errors.

Hopefully it would also be possible to call Session.setReadOnly(false) to make the changes persist if, for example, the save() succeeds after any binding errors have been manually rectified. It might be worth making the method available on domain instances directly too.

Activity

Hide
Mos added a comment -

I added some thoughts from the mailinglist to this issue (as suggested from Peter).:

I'm wondering whether the "dirty check" feature of hibernate is a reasonable default for Grails at all.
Sure it's a cool feature: Whenever the session get flushed, hibernate checks all known objects if
some properties changed. Those objects get updated automatically to the DB.

But from my experience this feature often makes more trouble then benefits.
Many times people wondering why objects get saved 'magically'. They are used to
save objects explicitly.

Setting the FlushMode to MANUAL is a nice workaround in your example.
But from my understanding it prevents all objects from being flushed to DB?
What are you doing if another object, for example Author, needs to be updated in the same action?

Is there an easy way in Grails to deactivate hibernates dirty checking for all domain object by default?
(--> Changes on Objects (and cascaded children) get only stored if the save() method is called explicitly)

Show
Mos added a comment - I added some thoughts from the mailinglist to this issue (as suggested from Peter).: I'm wondering whether the "dirty check" feature of hibernate is a reasonable default for Grails at all. Sure it's a cool feature: Whenever the session get flushed, hibernate checks all known objects if some properties changed. Those objects get updated automatically to the DB. But from my experience this feature often makes more trouble then benefits. Many times people wondering why objects get saved 'magically'. They are used to save objects explicitly. Setting the FlushMode to MANUAL is a nice workaround in your example. But from my understanding it prevents all objects from being flushed to DB? What are you doing if another object, for example Author, needs to be updated in the same action? Is there an easy way in Grails to deactivate hibernates dirty checking for all domain object by default? (--> Changes on Objects (and cascaded children) get only stored if the save() method is called explicitly)
Hide
Graeme Rocher added a comment -

Grails now uses setReadOnly(true) when a binding or validation error occurs

Show
Graeme Rocher added a comment - Grails now uses setReadOnly(true) when a binding or validation error occurs
Hide
Roshan Shrestha added a comment -

The URL http://www.cacoethes.co.uk/blog/index.php?p=38 no longer seems to be valid. Maybe the description of the bug can be updated so that it is clear what the problem is?

Show
Roshan Shrestha added a comment - The URL http://www.cacoethes.co.uk/blog/index.php?p=38 no longer seems to be valid. Maybe the description of the bug can be updated so that it is clear what the problem is?
Hide
Graeme Rocher added a comment - - edited

ok it was fixed in one of the earlier betas but there seems to have been an unfortunate regression. We are working on a functional test to prevent it happening in the future, but as it stands 1.1 has the same behavior as 1.0.x at the moment. Meaning you have to call discard() on the entity or set it to read-only upon a validation error as a workaround:

foo.properties = params
if(foo.hasErrors())
   Foo.withSession { session -> session.setReadOnly foo, true }

We will get it resolved for 1.1.1. The issue is actually remarkably simple. The changeset to fix the problem is:

Index: src/groovy/org/codehaus/groovy/grails/plugins/orm/hibernate/HibernatePluginSupport.groovy
===================================================================
--- src/groovy/org/codehaus/groovy/grails/plugins/orm/hibernate/HibernatePluginSupport.groovy	(revision 8822)
+++ src/groovy/org/codehaus/groovy/grails/plugins/orm/hibernate/HibernatePluginSupport.groovy	(working copy)
@@ -312,7 +312,7 @@
         metaClass.setProperties = {Object o ->
             originalPropertiesProperty.setProperty delegate, o
             if(delegate.hasErrors()) {
-                GrailsHibernateUtil.setObjectToReadyOnly o,sessionFactory
+                GrailsHibernateUtil.setObjectToReadyOnly delegate,sessionFactory
             }
         }

Criminal that our tests didn't pick this up. As an immediate workaround for 1.1 users we could issue a 1.1.1 release of the hibernate plugin soonish if it is felt that is appropriate

Show
Graeme Rocher added a comment - - edited ok it was fixed in one of the earlier betas but there seems to have been an unfortunate regression. We are working on a functional test to prevent it happening in the future, but as it stands 1.1 has the same behavior as 1.0.x at the moment. Meaning you have to call discard() on the entity or set it to read-only upon a validation error as a workaround:
foo.properties = params
if(foo.hasErrors())
   Foo.withSession { session -> session.setReadOnly foo, true }
We will get it resolved for 1.1.1. The issue is actually remarkably simple. The changeset to fix the problem is:
Index: src/groovy/org/codehaus/groovy/grails/plugins/orm/hibernate/HibernatePluginSupport.groovy
===================================================================
--- src/groovy/org/codehaus/groovy/grails/plugins/orm/hibernate/HibernatePluginSupport.groovy	(revision 8822)
+++ src/groovy/org/codehaus/groovy/grails/plugins/orm/hibernate/HibernatePluginSupport.groovy	(working copy)
@@ -312,7 +312,7 @@
         metaClass.setProperties = {Object o ->
             originalPropertiesProperty.setProperty delegate, o
             if(delegate.hasErrors()) {
-                GrailsHibernateUtil.setObjectToReadyOnly o,sessionFactory
+                GrailsHibernateUtil.setObjectToReadyOnly delegate,sessionFactory
             }
         }
Criminal that our tests didn't pick this up. As an immediate workaround for 1.1 users we could issue a 1.1.1 release of the hibernate plugin soonish if it is felt that is appropriate
Hide
John Allison added a comment -

Thanks for the comments, Graeme. I personally can live with the workarounds or patch on my own, without an official release, as long as I know it's a priority for the next release.

Show
John Allison added a comment - Thanks for the comments, Graeme. I personally can live with the workarounds or patch on my own, without an official release, as long as I know it's a priority for the next release.
Hide
Bradley Beddoes added a comment -

Wondering if anyone can advise a suitable method for testing this (would like to prove that it goes away for us once patch is applied and ensure it doesn't re-introduce in the future). Have tried several methods with an Integration test including modifying hibernate session flush mode to no avail, just can't seem to make it trigger. Problem does exist with same code path when doing manual testing via our webapp so I am obviously missing something that the full stack sets/introduces.

Show
Bradley Beddoes added a comment - Wondering if anyone can advise a suitable method for testing this (would like to prove that it goes away for us once patch is applied and ensure it doesn't re-introduce in the future). Have tried several methods with an Integration test including modifying hibernate session flush mode to no avail, just can't seem to make it trigger. Problem does exist with same code path when doing manual testing via our webapp so I am obviously missing something that the full stack sets/introduces.
Hide
Graeme Rocher added a comment -

I personally plan to write a functional test so that this behavior doesn't regress.

Show
Graeme Rocher added a comment - I personally plan to write a functional test so that this behavior doesn't regress.
Hide
Graeme Rocher added a comment -

fixed now for good, with functional test

Show
Graeme Rocher added a comment - fixed now for good, with functional test
Hide
Ayub Malik added a comment -

We are currently using grails 1.1.1 and are still experiencing this behaviour, when a domain object has a binding error the edit page is displayed with a validation error however the record saves when the flush occurs. The code where this fails is from the controller generated from the grails:generate-all command, the line where the error occurs is shown below:
 

if(!employeeInstance.hasErrors() && employeeInstance.save()) {
 ....
}

This results in an object being saved in a partial state or in a state that fails the grails validation.

Can you please advise if this has definetly been fixed in 1.1.1 and if not whether it is fixed in 1.2M1

Show
Ayub Malik added a comment - We are currently using grails 1.1.1 and are still experiencing this behaviour, when a domain object has a binding error the edit page is displayed with a validation error however the record saves when the flush occurs. The code where this fails is from the controller generated from the grails:generate-all command, the line where the error occurs is shown below:  
if(!employeeInstance.hasErrors() && employeeInstance.save()) {
 ....
}
This results in an object being saved in a partial state or in a state that fails the grails validation. Can you please advise if this has definetly been fixed in 1.1.1 and if not whether it is fixed in 1.2M1
Hide
Rob Kirkbride added a comment -

Can this be reopened as I believe the problem still occurs with Grails 1.1.1

It seems to be related to http://jira.codehaus.org/browse/GRAILS-1793 which also is marked as resolved but we have to do Marc Guillemots suggestion to make things work. However, this still produces a WARN and stack trace.

Thanks.

Show
Rob Kirkbride added a comment - Can this be reopened as I believe the problem still occurs with Grails 1.1.1 It seems to be related to http://jira.codehaus.org/browse/GRAILS-1793 which also is marked as resolved but we have to do Marc Guillemots suggestion to make things work. However, this still produces a WARN and stack trace. Thanks.
Hide
Graeme Rocher added a comment -

We have implemented a different strategy to deal with this for 1.2-M1, i suggest you try that release before we reopening the issue

Show
Graeme Rocher added a comment - We have implemented a different strategy to deal with this for 1.2-M1, i suggest you try that release before we reopening the issue
Hide
Graeme Rocher added a comment -

Bulk closing bunch of resolved issues

Show
Graeme Rocher added a comment - Bulk closing bunch of resolved issues

People

Vote (3)
Watch (10)

Dates

  • Created:
    Updated:
    Resolved: