[jira] Commented: (WW-2587) @SkipValidation not found on superclass method

["Brad Cupit (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/struts/browse/WW-2587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=44017#action_44017 ] 

Brad Cupit commented on WW-2587:
--------------------------------

I think this is going to fix the bug just fine, but I have a concern about the test.

The original bug relates to annotated super class methods that are overridden by the subclass.
The current test seems to test
a) normal (un-annotated) super class methods overridden by a subclass method, and the subclass method has the annotation
b) annotated super class methods not overridden in the subclass

Both of the above conditions are good, and should be tested. I wonder if another test should be added though, to catch methods in the superclass which are annotated and overridden in the subclass (but the subclass method is not annotated). This test is necessary to prove that @SkipValidation works the same way as the other validation annotations.

How about this, add the below method to TestActionBase:
@SkipValidation
public String skipMeOverride() {
    throw new IllegalArgumentException("i should have been overridden by a subclass");
}

Add this to TestAction:
public String skipMeOverride() {
    return "override an annotated method, but don't add any new annotations (superclass annotations should still apply)";
}

And finally, add the following test to AnnotationValidationInterceptorTest:
public void testShouldSkipOverride() throws Exception {
    mockActionProxy.expectAndReturn("getMethod", "skipMeOverride");
    interceptor.doIntercept((ActionInvocation)mockActionInvocation.proxy());
    mockActionProxy.verify();
}

Note: i don't really understand the tests 100%. It seems that if the mock ActionProxy only has getMethod() called once, and nothing else happens, then the method was skipped. If getActionName() is called on the mock ActionProxy though, then the method is not skipped. If that's a true assumption, then the above test should work and prove that the bug is fixed for good.

> @SkipValidation not found on superclass method
> ----------------------------------------------
>
>                 Key: WW-2587
>                 URL: https://issues.apache.org/struts/browse/WW-2587
>             Project: Struts 2
>          Issue Type: Bug
>          Components: Core Interceptors
>            Reporter: Brad Cupit
>             Fix For: 2.1.3
>
>
> The SkipValidation annotation is a great addition, but it would be nice if it was found on super class methods (i.e. the method being overridden) and/or interfaces.
> I have a CGLIB-generated proxy for my Action (created by Spring) so the SkipValidation annotation is never found, since CGLIB-proxies extend the class they proxy.
> Admittedly, this is more of a problem with CGLIB, than with Struts2, however if @SkipValidation were allowed on methods in interfaces or superclasses, my problem would be solved.
> Unfortunately, the validation annotations (like @RequiredStringValidator), are found on super classes. So the net effect is that my SkipValidation annotation is not found, but the validation annotations are, which means validation is occurring when it shouldn't.
> potential workarounds:
> a) use one action method per Action so validation can always be done for that entire class, or
> b) use excludeMethods param when configuring the validation interceptor in struts.xml
> Note that for option b) to work with zero-configuration, the @ParentPackage annotation must be applied to the Action class that will use the package defined in struts.xml
> Ideal fix:
> The AnnotationValidationInterceptor#doIntercept(ActionInvocation) method would check for the SkipValidation annotation on the method being overridden. This would not only fix the CGLIB-proxy issue, but would also allow SkipValidation to be used on interfaces and superclasses.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.