skipValidate logic in GormValidateable causes missed validations and is inconsistent

[longwa]

GormValidateable has a property skipValidate that is trying to skip calling validate multiple times when an entity is saved and then later flushed.

However, it has the potential to cause validations to be skipped under some scenarios. It's also completely ignored for explicit calls to validate() without a list of properties, which seems inconsistent.

The attached project's AuthorSpec test does the following:

1. Create a new valid Author and call save but don't flush
2. Update the name attribute on the Author instance to an invalid state
3. Call validate(['name']) to re-validate the author instance
4. Validation should fail, however step 1 left the Author with skipValidate = true so validation is skipped instead and the validate returns true

If you call validate without any properties, the validation will fail as expected, which seems like a weird side effect as well.

The problem is that this finally block:

https://github.com/grails/grails-data-mapping/blob/ef1730a42bc515cfb96cf9175d81e344d8a08a61/grails-datastore-gorm-hibernate-core/src/main/groovy/org/grails/orm/hibernate/AbstractHibernateGormInstanceApi.groovy#L162

Incorrectly restores the skipValidate based on the shouldFlush flag, which leaves the non-flushed object in this state.

It almost seems like it should just restore the skipValidate to false after everything is finished instead of tying it to !shouldFlush.

• **GORM Version: 6.1.10.RELEASE
• **Grails Version (if using Grails): 3.3.8
• **JDK Version: 1.8.0_111

Example Application

https://github.com/longwa/gorm-skipValidate-bug

[longwa]

Actually, after testing some changes, I think the correct fix is to remove the check from the two validate(...) calls that take a List and Map in the GormValidateable. The ClosureEventListener already explicitly checks the flag before calling validate as part of the save listeners so there's no reason to use the flag for explicit calls to validate. This also makes it consistent as calling validate with or without parameters yields the same result.

The check is already made here:
https://github.com/grails/grails-data-mapping/blob/a0ba37f0fbc41fc883498ab88ad720c670abeade/grails-datastore-gorm-hibernate-core/src/main/groovy/org/grails/orm/hibernate/support/ClosureEventListener.java#L278

[longwa]

Actually, playing with it more it seems like the code isn't actually accomplishing what it is trying to do in all cases.

Specifically, if you call save without a flush, it can actually end up validating twice anyway as hibernate will still call the preInsert event listeners when it does the key generation for this sequence type (even without a flush).

Once you are past the block at:
https://github.com/grails/grails-data-mapping/blob/a0ba37f0fbc41fc883498ab88ad720c670abeade/grails-datastore-gorm-hibernate-core/src/main/groovy/org/grails/orm/hibernate/AbstractHibernateGormInstanceApi.groovy#L117

You can always setup skipValidate(true) for the duration of the try/finally block since you've already either validated the object (inside the shouldValidate) or you've skipped validation explicitly; either way, you don't need to validate in the ClosureEvent callbacks.

However, you can't make any assumptions about whether you need to validate based on the flush parameter since it's possible that the object could change prior to flush being called. For example, this code will manage to bypass the GORM validations completely:

        // Save but don't flush, this causes the new author to have skipValidate = true
        Author author = new Author(name: 'Aaron').save(failOnError: true)

        when:
        // Author has a maxSize constraint of 8 as does the DB
        author.name = "ThisNameIsTooLong"
        Author.withSession { session ->
            // This flush will skip validation, however the object needs validation as it's dirty since the last
            // save or validate. 
            session.flush()
        }

        then:
        author.hasErrors()  // Won't get here in 6.1.10

The fix is to always try/finally to ensure the skipValidate is only enabled for the duration of a single save call and never persists outside of it.

[longwa]

It's not really directly part of this change, but in testing I've realized that deepValidate is not really that useful in practice. Only the explicit up-front validation in the save() call uses the deepValidate flag to stop cascading down the graph. However, any objects that are actually dirty will still be validated as part of the ClosureEventListener handlers when they are flushed. This is true whether you pass flush: true to save() or whether the session is flushed via some other means such as on commit. This is because the ClosureEventListener doesn't really have the context it would need to know whether the object needed to be validated.

In those cases, it might make sense to call skipValidation(true) explicitly on the GormValidateable instance for any object that is dirty in the session and you want to exclude from validation on flush.

If we wanted to fix the deepValidate: true, flush: true scenario for save, I think the only solution would be to try and setup some additional context that the ClosureEventListener could use to know it needs to skip everything. Currently (and with my change as well), only the root entity has the skipValidate flag true, but other associated entities that might be dirty do not. Without trying to iterate the entire object graph and set everything skipValidate = true, another option would be to setup some ThreadLocal context or something to pass that context into the event listener. Just sort of brainstorming.

[longwa]

I think the ultimate solution needs to include having the DirtyCheckable implementation reset the skipValidate flag if the entity is GormValidateable.

With that logic in place, an entity safely be saved (without flush) and have the skipValidate flag left as true, as in the test case above. If the entity is unchanged when the flush occurs, it will not validate again. However, if the entity changes before flush, the dirty checking logic can also reset skipValidate to false to ensure that it is validated again on the flush.

[longwa]

DirtyCheckable is in core and GormValidateable is in gorm so DirtyCheckable can't directly reference GormValidateable, which would be ideal. I think the two are pretty closely related (or should be when it comes to skipping validations) so it doesn't seem that weird to have a dependency here. Without adding a project dependency, adding this to DirtyCheckable and calling for each markDirty method fixes all of the scenarios in my sample project:

    /**
     * If this entity is GormValidateable, then make sure that we reset the skip validation to false
     */
    private void resetValidationState() {
        if (this.respondsTo("skipValidation")) {
            this.invokeMethod("skipValidation", false)
        }
    }

I guess there could be a small performance hit, but it seems negligible.

Also, not sure the solution would work if a project elects to use hibernate dirty checking (I'm not sure if the grails dirty checking methods are still called or not).

[demon101]

Dear @longwa is it finished?

[longwa]

@demon101 I've been running a custom build with this for awhile in Gorm 6.1.x, but once Grails 4 came out I guess I need to see if this fix is needed/relevant for G4 and try to get it merged.

[jdaugherty]

https://github.com/jdaugherty/gorm-hibernate5/commit/01d9cf81e876179b4ee626f686b703ba1c23ebd3 will fix the second part of this ticket. Will open this change after the test fixes from Grails 7 get merged & the part 1 fix is merged.