geofflane / grails-constraints

Custom constraints plugin for Grails applications
Apache License 2.0
35 stars 11 forks source link

Constraint API + concurrency fixes #18

Closed samcday closed 6 years ago

samcday commented 12 years ago

Hi Geoff,

This patch is a little bigger than the other one I just submitted. This one focuses on the way we enhance the metaClass for the purposes of the "magic" stuff available in validate closure (params/hibernateTemplate/etc).

The issue is, the way it's currently implemented is inherently not thread-safe. This is because a single instance of DefaultGrailsConstraintClass (and the actual constraint class being wrapped) is shared across any number of applied constraints. Consider this example:

class AwesomeConstraint {
    def validate = { val ->
        return params == "foo"
    }
}

class FirstCommandObject {
    String blah

    static constraints = {
        blah(awesome: "foo")
    }
}

class SecondCommandObject {
    String blah

    static constraints = {
        blah(awesome: "bar")
    }
}

We would assume in this example that any time SecondCommandObject is used, it would be guaranteed to fail, however this is currently not the case. Instead, FirstCommandObject will sometimes sporadically fail, and SecondCommandObject will succeed. This is because the way setupConstraint was dishing out params and such was not thread safe.

So I've opted to implement the expected constraints validate api by stashing the CustomConstraintClass currently being executed in the request attributes of the current ServletRequest. The new api proxies requests for params/owningClass/propertyName/etc through the stashed CustomConstraintClass.

While I was doing this, I also refactored the api to be an interface, and the implementation will be applied to constraints using the Grails-y MetaClassEnhancer.

I also introduced another implementation of the ConstraintApi for test-mode, which will just proxy params and friends over to the test method that is currently executing. This means that accessing params/constraintOwningClass/etc will just access the same var that is injected into test classes via ConstraintsUnitTestMixin.

Please let me know if you have any questions!

-Sam

geofflane commented 12 years ago

Thanks for this, I want to look into this a bit more to see if there's another way to fix this that doesn't involve Request state, etc.

samcday commented 12 years ago

@geofflane Did you ever get a chance to review this? We've been using it in production for a while now and haven't had any issues. I agree it would be nice to avoid needing state tracking in ServletRequests, but I couldn't come up with a better way at the time. Would be interested to see if you come up with a better way. Let me know! :)

geofflane commented 12 years ago

Sorry I haven't gotten to it yet. I started digging into this more tonight and I see what you're saying in that we only have one "params" value per Constraint instead of per "ConstrainedProperty". That currently really is a fatal flaw... I need to think on this a bit more.