Constraint API + concurrency fixes #18

Open
wants to merge 2 commits into
from

Projects

None yet

2 participants

@samcday
Contributor
samcday commented Apr 5, 2012

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

Sam Day added some commits Apr 5, 2012
Sam Day introduced ConstraintApi, which is used by MetaClassEnhancer to provi…
…de the metaclass magic to validate closure. The RequestConstraintApi uses current web request to get a handle on the CustomConstraintClass being executed. This new functionality obseletes the manual metaclass stuff in ConstraintGrailsPlugin.setupConstaint, so it and its supporting stuff in CustomConstraintFactory + DefaultGrailsConstraintClass have been removed.
17dced6
Sam Day MockConstraintApi implemented, which simply delegates to a unit test …
…that has been injected with ConstraintUnitTestMixin. Aforementioned class updated to use this Api for enhancing constraint under test (CUT, hehe).
167d0ab
@geofflane
Owner

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
Contributor
samcday commented Jul 5, 2012

@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
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment