Struts 2
  1. Struts 2
  2. WW-4071

ValidationAware add callable method, called from DefaultWorkflowInterceptor

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.14
    • Fix Version/s: 2.3.15
    • Component/s: Core Interceptors
    • Labels:
      None

      Description

      When a form error occurs, the action is unaware of this event. There are situations whereby one should know that a form error has occurred. For example, one may wish to populate the ActionStack with special handling variables or set a flag denoting special logic that the JSP should handle. In short, it isn't unreasonable for one to want to know that a form error occurred and be able to respond to it with more than just a single JSP result attached to "input."

      My suggestion would be within ValidationAware, add a method:
      void actionError()

      Within DefaultWorkflowInterceptor.doIntercept, there is already an if condition of:
      if (validationAwareAction.hasErrors()) {

      at the end of processing that if block, just before, "return resultName;" call validationAwareAction.actionError();

      By the way, this line:
      LOG.debug("Errors on action " + validationAwareAction + ", returning result name 'input'");

      should be:
      LOG.debug("Errors on action " + validationAwareAction + ", returning result name '" + inputResultName + "'");

        Activity

        Hide
        pbenedict added a comment -

        I second this feature, except the callback method should return a String so the developer can choose their own forward.

        I actually wanted this feature for a bit but was too lazy to submit it My use case, however, is a bit different. I have several Actions that support AJAX and I want validation error to trigger an HTTP 400 (Bad Request). With this sort of call back, I can easily set the status code and return null.

        Show
        pbenedict added a comment - I second this feature, except the callback method should return a String so the developer can choose their own forward. I actually wanted this feature for a bit but was too lazy to submit it My use case, however, is a bit different. I have several Actions that support AJAX and I want validation error to trigger an HTTP 400 (Bad Request). With this sort of call back, I can easily set the status code and return null.
        Hide
        Lukasz Lenart added a comment - - edited

        The fix is easy but adding it directly to ValidationAware will break backward compatibility - each action will have to be updated :\

        I would rather add another interface - ValidationErrorAware - and use it to mark actions that need to be notified about validation errors.

        Show
        Lukasz Lenart added a comment - - edited The fix is easy but adding it directly to ValidationAware will break backward compatibility - each action will have to be updated :\ I would rather add another interface - ValidationErrorAware - and use it to mark actions that need to be notified about validation errors.
        Hide
        Eric Lentz added a comment -

        Good call Lukasz. Thanks!

        Show
        Eric Lentz added a comment - Good call Lukasz. Thanks!
        Hide
        Lukasz Lenart added a comment -

        I have added possibility to return null from actionError() to allow keep the previous result. So you can return a new result or null to keep what was previously calculated.

        Show
        Lukasz Lenart added a comment - I have added possibility to return null from actionError() to allow keep the previous result. So you can return a new result or null to keep what was previously calculated.
        Hide
        Lukasz Lenart added a comment -
        Show
        Lukasz Lenart added a comment - Implemented also docs updated https://cwiki.apache.org/confluence/display/WW/Default+Workflow+Interceptor
        Hide
        Eric Lentz added a comment -

        Lukasz,

        The code is looking really nice. I like what you did. One question though. I've received mixed information about interceptor life-cycles, but I believe that an interceptor can span multiple requests? In other words, it isn't created anew with each request. If that's true, then I'm wondering if permanently changing resultName as a side-effect of executing processValidationErrorAware is a good policy? In other words, I could see one action executing in ValidationErrorAware mode, changing the result type and then another action coming along, not executing in ValidationErrorAware mode, and expecting "input" for the resultName. If interceptors only live for 1 request and then are re-created for the next request, then I'm way off and never mind.

        If, on the other hand, it isn't created each time, then there could also be a thread-safety / race condition between ValidationErrorAware executions coming in at the same time.

        Thanks,
        Eric

        Show
        Eric Lentz added a comment - Lukasz, The code is looking really nice. I like what you did. One question though. I've received mixed information about interceptor life-cycles, but I believe that an interceptor can span multiple requests? In other words, it isn't created anew with each request. If that's true, then I'm wondering if permanently changing resultName as a side-effect of executing processValidationErrorAware is a good policy? In other words, I could see one action executing in ValidationErrorAware mode, changing the result type and then another action coming along, not executing in ValidationErrorAware mode, and expecting "input" for the resultName. If interceptors only live for 1 request and then are re-created for the next request, then I'm way off and never mind. If, on the other hand, it isn't created each time, then there could also be a thread-safety / race condition between ValidationErrorAware executions coming in at the same time. Thanks, Eric
        Hide
        Lukasz Lenart added a comment -

        Interceptors are singletons per stack, which means the same interceptor instance will be used for each request (per stack).

        I don't get it - method params are thread-safe, even if many threads are reusing the same instance of interceptor, each thread has its own parameter reference. Secondly ActionInvocation / Action is recreated per request so there is no option to share the state.

        Basically operating just on ActionInvocation / Action in methods is thread-safe. Interceptor doesn't change any global state (like itself) so I don't see where the race condition can occur.

        Show
        Lukasz Lenart added a comment - Interceptors are singletons per stack, which means the same interceptor instance will be used for each request (per stack). I don't get it - method params are thread-safe, even if many threads are reusing the same instance of interceptor, each thread has its own parameter reference. Secondly ActionInvocation / Action is recreated per request so there is no option to share the state. Basically operating just on ActionInvocation / Action in methods is thread-safe. Interceptor doesn't change any global state (like itself) so I don't see where the race condition can occur.
        Hide
        Eric Lentz added a comment -

        I'm sorry, you're right. I confused inputResultName (global) with resultName (local to method). Okay, in that case, totally beautiful code! Sorry for the confusion. Thanks again for implementing this.

        Show
        Eric Lentz added a comment - I'm sorry, you're right. I confused inputResultName (global) with resultName (local to method). Okay, in that case, totally beautiful code! Sorry for the confusion. Thanks again for implementing this.
        Hide
        Lukasz Lenart added a comment -

        Good to hear that, you're welcome

        Show
        Lukasz Lenart added a comment - Good to hear that, you're welcome
        Hide
        Hudson added a comment -

        Integrated in Struts2-JDK6 #708 (See https://builds.apache.org/job/Struts2-JDK6/708/)
        WW-4071 Adds some comments about the supported *Aware interfaces (Revision 1485153)
        WW-4071 Adds new ValidationErrorAware interface to allow notify action about action/field errors (Revision 1485149)

        Result = SUCCESS
        lukaszlenart :
        Files :

        • /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/DefaultWorkflowInterceptor.java

        lukaszlenart :
        Files :

        • /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/DefaultWorkflowInterceptor.java
        • /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ValidationErrorAware.java
        • /struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ValidationErrorAwareTest.java
        Show
        Hudson added a comment - Integrated in Struts2-JDK6 #708 (See https://builds.apache.org/job/Struts2-JDK6/708/ ) WW-4071 Adds some comments about the supported *Aware interfaces (Revision 1485153) WW-4071 Adds new ValidationErrorAware interface to allow notify action about action/field errors (Revision 1485149) Result = SUCCESS lukaszlenart : Files : /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/DefaultWorkflowInterceptor.java lukaszlenart : Files : /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/DefaultWorkflowInterceptor.java /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ValidationErrorAware.java /struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ValidationErrorAwareTest.java
        Hide
        pbenedict added a comment -

        Returning null to keep the current result is insufficient for my needs. It also is a surprise since null usually means the action handled the result itself. I need to return null because I am setting the HTTP response code; I can't do that using the current return semantics.

        Show
        pbenedict added a comment - Returning null to keep the current result is insufficient for my needs. It also is a surprise since null usually means the action handled the result itself. I need to return null because I am setting the HTTP response code; I can't do that using the current return semantics.
        Hide
        Lukasz Lenart added a comment -

        Paul Benedict you are right, I overlooked that and basically you can return the same result as calculated by the interceptor (method signature has to be changed a bit):

        
        String actionErrorOccured(final String originalResultName);
        
        
        Show
        Lukasz Lenart added a comment - Paul Benedict you are right, I overlooked that and basically you can return the same result as calculated by the interceptor (method signature has to be changed a bit): String actionErrorOccured( final String originalResultName);
        Hide
        pbenedict added a comment -

        Nods. Yes, that's exactly the signature I was hoping for!

        Show
        pbenedict added a comment - Nods. Yes, that's exactly the signature I was hoping for!
        Hide
        Lukasz Lenart added a comment -

        Done

        Show
        Lukasz Lenart added a comment - Done
        Hide
        Hudson added a comment -

        Integrated in Struts2-JDK6 #709 (See https://builds.apache.org/job/Struts2-JDK6/709/)
        WW-4071 Changes interface a bit after review (Revision 1485311)

        Result = SUCCESS
        lukaszlenart :
        Files :

        • /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/DefaultWorkflowInterceptor.java
        • /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ValidationErrorAware.java
        • /struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ValidationErrorAwareTest.java
        Show
        Hudson added a comment - Integrated in Struts2-JDK6 #709 (See https://builds.apache.org/job/Struts2-JDK6/709/ ) WW-4071 Changes interface a bit after review (Revision 1485311) Result = SUCCESS lukaszlenart : Files : /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/DefaultWorkflowInterceptor.java /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ValidationErrorAware.java /struts/struts2/trunk/xwork-core/src/test/java/com/opensymphony/xwork2/interceptor/ValidationErrorAwareTest.java
        Hide
        Alfredo Osorio added a comment -

        Actually, these lines should be just before returning the result and not at the start because at that moment you still don't know what result will be returned:
        if (LOG.isDebugEnabled())

        { LOG.debug("Errors on action [#0], returning result name [#1]", validationAwareAction, resultName); }
        Show
        Alfredo Osorio added a comment - Actually, these lines should be just before returning the result and not at the start because at that moment you still don't know what result will be returned: if (LOG.isDebugEnabled()) { LOG.debug("Errors on action [#0], returning result name [#1]", validationAwareAction, resultName); }
        Hide
        Alfredo Osorio added a comment -

        Never mind, I just saw that there are other loggings to notify that the result changed if that is the case.

        Show
        Alfredo Osorio added a comment - Never mind, I just saw that there are other loggings to notify that the result changed if that is the case.

          People

          • Assignee:
            Lukasz Lenart
            Reporter:
            Eric Lentz
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development