Great review Carlos!
Simone, I haven't had time to look thoroughly at the code, but it looks
like it uses BVAL correctly and is a good complement module. I was able
to build the module and it passed rat:check, so I'd vote for bringing it
over into trunk.
-Donald
On 5/9/10 6:10 PM, Carlos Vara wrote:
> Hi Simone,
>
> I took a look at the code, very good work there!
>
> I'm afraid I can't comment much about the guice part as I'm more of a spring
> man myself, but I can comment a little on the validation part.
>
> First, just to check that I understood what it does right:
> - It creates an interceptor for method calls annotated with @Validate and
> validates all the arguments. In case any validation error is found, it
> throws a CVE.
>
> If the above is right, a few pointers on the validation part:
>
> - If you validate the arguments individually, maybe having an extra
> annotation in them can help improve the speed of the code. Take care that
> calling validator.validate(..) for all basic types (Strings, Integers, etc.)
> will always output an empty set, and those arguments are very common.
> You can use @Valid as a marker for the arguments that you want to validate,
> this way, a validated method call would be like this:
>
> @Validate
> public String myMethod(String notValidated, @Valid Person validatedPerson) {
> ... }
>
> You may decide to validate all arguments in case @Validate is present and no
> argument is annotated with @Valid.
>
> - Although it is not portable, you may also want to take a look at the
> method validation API that the Apache implementation has. It works quite
> well in this scenario. I posted a quick implementation I did with AspectJ
> here:
> http://carinae.net/2010/04/automatic-validation-method-calls-with-jsr-303-appendix-c/
> If you are interested, you could do a second interceptor that takes
> advantage of it in case the JSR-303 implementation is the apache one.
>
> - One last thing, in ValidateMethodInterceptor.invoke(), you may want to
> also validate the return value before returning.
>
> Oh, and my non-binding opinion about moving it out of the sandbox would be a
> yes. All the above comments are just possible improvements, IMHO code uses
> the validation API correctly to solve an interesting problem.
>
> Regards,
> Carlos
>
>
>
> On Sat, May 8, 2010 at 1:07 PM, Simone Tripodi
> <[email protected]>wrote:
>
>> Hi all guys,
>> I'd like to ask you what you think about moving the actual google
>> guice integration support from sandbox to current modules.
>> I just integrated the groups support in the AOP interceptor and it
>> seems working nicely, at least for me, I'd really would like if
>> someone could provide feedbacks.
>> Every kind of help will be more than appreciated, thanks in advance!
>> Simo
>>
>> http://people.apache.org/~simonetripodi/<http://people.apache.org/%7Esimonetripodi/>
>>
>
