fix #1155 - skipValidate logic in GormValidateable causes missed validations and is inconsistent

Author: jdaugherty

grails-datastore-gorm-hibernate5/src/main/groovy/org/grails/orm/hibernate/AbstractHibernateGormInstanceApi.groovy

@@ -23,8 +23,6 @@ import org.grails.datastore.mapping.core.Datastore
 import org.grails.datastore.mapping.dirty.checking.DirtyCheckable
 import org.grails.datastore.mapping.model.config.GormProperties
 import org.grails.datastore.mapping.model.types.Embedded
-import org.grails.datastore.mapping.model.types.EmbeddedCollection
-import org.grails.datastore.mapping.model.types.ToMany
 import org.grails.datastore.mapping.proxy.ProxyHandler
 import org.grails.datastore.mapping.reflect.ClassUtils
 import org.grails.datastore.mapping.reflect.EntityReflector
@@ -143,12 +141,12 @@ abstract class AbstractHibernateGormInstanceApi<D> extends GormInstanceApi<D> {
         // this piece of code will retrieve a persistent instant
         // of a domain class property is only the id is set thus
         // relieving this burden off the developer
-        autoRetrieveAssocations datastore, domainClass, target
+        autoRetrieveAssociations datastore, domainClass, target
 
-        // If validation is disabled, skip it or if a flush:true is passed then disable it too to avoid duplicate validation
+        // Once we get here we've either validated this object or skipped validation, either way
+        // we don't need to validate again for the rest of this save.
         GormValidateable validateable = (GormValidateable) target
-        boolean shouldSkipValidation = !shouldValidate || shouldFlush
-        validateable.skipValidation(shouldSkipValidation)
+        validateable.skipValidation(true)
 
         try {
             if (shouldInsert(arguments)) {
@@ -164,7 +162,10 @@ abstract class AbstractHibernateGormInstanceApi<D> extends GormInstanceApi<D> {
                 return performSave(target, shouldFlush)
             }
         } finally {
-            validateable.skipValidation(!shouldFlush)
+            // After save, we have to make sure this entity is setup to validate again. It's possible it will
+            // be validated again if this save didn't flush, but without checking it's dirty state we can't really
+            // know for sure that it hasn't changed and need to err on the side of caution.
+            validateable.skipValidation(false)
         }
     }
 
@@ -284,7 +285,7 @@ abstract class AbstractHibernateGormInstanceApi<D> extends GormInstanceApi<D> {
         try {
             session.flush()
         } catch (HibernateException e) {
-            // session should not be flushed again after a data acccess exception!
+            // session should not be flushed again after a data access exception!
             session.setFlushMode FlushMode.MANUAL
             throw e
         }
@@ -295,7 +296,7 @@ abstract class AbstractHibernateGormInstanceApi<D> extends GormInstanceApi<D> {
      * @param target The target object
      */
     @SuppressWarnings("unchecked")
-    private void autoRetrieveAssocations(Datastore datastore, PersistentEntity entity, Object target) {
+    private void autoRetrieveAssociations(Datastore datastore, PersistentEntity entity, Object target) {
         EntityReflector reflector = datastore.mappingContext.getEntityReflector(entity)
         IHibernateTemplate t = this.hibernateTemplate
         for (PersistentProperty prop in entity.associations) {

grails-datastore-gorm-hibernate5/src/test/groovy/grails/gorm/tests/validation/SkipValidationSpec.groovy

@@ -0,0 +1,102 @@
+package grails.gorm.tests.validation
+
+import grails.gorm.annotation.Entity
+import grails.gorm.transactions.Rollback
+import grails.validation.ValidationException
+import org.grails.orm.hibernate.HibernateDatastore
+import spock.lang.AutoCleanup
+import spock.lang.Shared
+import spock.lang.Specification
+
+class SkipValidationSpec extends Specification {
+    @Shared @AutoCleanup HibernateDatastore hibernateDatastore = new HibernateDatastore(Author)
+
+    // For whatever reason it may be valid to flush & save without validation (database would obviously fail if the field is too long, but maybe the object is expected to only have an invalid validator?) so continue to support this scenario
+    @Rollback
+    void "calling save with flush with validate false should skip validation"() {
+        when:
+        new Author(name: 'false').save(failOnError: true, validate: false, flush: true)
+
+        then:
+        noExceptionThrown()
+    }
+
+    @Rollback
+    void "calling save with flush and invalid attribute"() {
+        when:
+        new Author(name: 'ThisNameIsTooLong').save(failOnError: true, flush: true)
+
+        then:
+        thrown(ValidationException)
+    }
+
+    @Rollback
+    void "calling validate with property list after save should validate again"() {
+        // Save but don't flush, this causes the new author to have skipValidate = true
+        Author author = new Author(name: 'Aaron').save(failOnError: true)
+
+        when: "validate is called again with a property list"
+        author.name = "ThisNameIsTooLong"
+        def isValid = author.validate(['name'])
+
+        then: "it should be invalid but it skips validation instead"
+        !isValid
+    }
+
+    @Rollback
+    void "calling validate with property list after save with flush should validate again"() {
+        // Save but don't flush, this causes the new author to have skipValidate = true
+        Author author = new Author(name: 'Aaron').save(failOnError: true, flush: true)
+
+        when: "validate is called again with a property list"
+        author.name = "ThisNameIsTooLong"
+        def isValid = author.validate(['name'])
+
+        then: "it should be invalid but it skips validation instead"
+        !isValid
+    }
+
+    @Rollback
+    void "calling validate with property list after save should validate again on explicit flush"() {
+        // Save but don't flush, this causes the new author to have skipValidate = true
+        Author author = new Author(name: 'Aaron').save(failOnError: true)
+
+        when: "validate is called again with a property list"
+        author.name = "ThisNameIsTooLong"
+        Author.withSession { session ->
+            session.flush()
+        }
+
+        then:
+        author.hasErrors()
+    }
+
+    @Rollback
+    void "calling validate with no list after save should validate again"() {
+        // Save but don't flush, this causes the new author to have skipValidate = true
+        Author author = new Author(name: 'Aaron').save(failOnError: true)
+
+        when: "validate is called again without any parameters"
+        author.name = "ThisNameIsTooLong"
+        def isValid = author.validate()
+
+        then: "this works since validate without params doesn't honor skipValidate for some reason"
+        !isValid
+    }
+}
+
+@Entity
+class Author {
+    String name
+
+    static constraints = {
+        name(nullable: false, maxSize: 8, validator: { val, obj ->
+            if(val == "false") {
+                return "name.invalid"
+            }
+
+            println "Validate called"
+            true
+        })
+    }
+}