Skip to content

Commit

Permalink
Merge pull request #913 from jdaugherty/feature/9.0.x/1155-skip-validate
Browse files Browse the repository at this point in the history
fix #1155 - skipValidate logic in GormValidateable causes missed validations and is inconsistent
jdaugherty authored Oct 31, 2024
2 parents 97aefb3 + 16ccfd4 commit 63881c9
Showing 2 changed files with 112 additions and 9 deletions.
Original file line number Diff line number Diff line change
@@ -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) {
Original file line number Diff line number Diff line change
@@ -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
})
}
}

0 comments on commit 63881c9

Please sign in to comment.