Uploaded image for project: 'Struts 2'
  1. Struts 2
  2. WW-4605

Refactor MessageStoreInterceptor and use PreResultListener to store messages

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.24
    • Fix Version/s: 2.3.28, 2.5
    • Component/s: Core Interceptors
    • Labels:
      None

      Description

      See WW-4600 as this issue is similar, when response was already comitted (redirect was send, JSP was rendered), logic in after method won't work as it isn't possible to modify session when response was comitted.

        Issue Links

          Activity

          Hide
          ghuber Greg Huber added a comment -

          I think we are actually comparing strings here, as the redirect is from the struts.xml

          <result-type name="redirect" class="org.apache.struts2.result.ServletRedirectResult"/>

          ....if we would want to decouple from the actual classes, a ResultConfig reference to the redirect (from the configuration), and check getClassName() against the string value on the invocation.

          Probably an overkill.

          Show
          ghuber Greg Huber added a comment - I think we are actually comparing strings here, as the redirect is from the struts.xml <result-type name="redirect" class="org.apache.struts2.result.ServletRedirectResult"/> ....if we would want to decouple from the actual classes, a ResultConfig reference to the redirect (from the configuration), and check getClassName() against the string value on the invocation. Probably an overkill.
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Thanks, I will wait a few more days/weeks and will push out one more 2.3.x series - it's basically a regression which must be fixed

          Show
          lukaszlenart Lukasz Lenart added a comment - Thanks, I will wait a few more days/weeks and will push out one more 2.3.x series - it's basically a regression which must be fixed
          Hide
          jtdevos Jim deVos added a comment -

          Greg, Lukasz: thanks for the correction! In the future I should probably vet my snippets in an IDE...

          Show
          jtdevos Jim deVos added a comment - Greg, Lukasz: thanks for the correction! In the future I should probably vet my snippets in an IDE...
          Hide
          jtdevos Jim deVos added a comment - - edited

          I don't think I have permissions to reopen a ticket but I don't mind making a new one.

          As for fixversion, I'll defer to your judgement. We were able to work-around the issue, so just wanted to give you folks a heads-up.

          edit: WW-4618 created

          Show
          jtdevos Jim deVos added a comment - - edited I don't think I have permissions to reopen a ticket but I don't mind making a new one. As for fixversion, I'll defer to your judgement. We were able to work-around the issue, so just wanted to give you folks a heads-up. edit: WW-4618 created
          Hide
          ghuber Greg Huber added a comment -

          OK, that works also...

          Show
          ghuber Greg Huber added a comment - OK, that works also...
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Yes, but you can simply resolve that by

          isRedirect = ServletRedirectResult.class.isAssignableFrom(Class.forName(resultConfig.getClassName()));

          Show
          lukaszlenart Lukasz Lenart added a comment - Yes, but you can simply resolve that by isRedirect = ServletRedirectResult.class.isAssignableFrom(Class.forName(resultConfig.getClassName()));
          Hide
          ghuber Greg Huber added a comment - - edited

          The interceptor logic has changed, now need to get the isRedirect from the invocation, not from invocation.getResult() which is null.

          isRedirect = ServletRedirectResult.class.getName().equals(resultConfig.getClassName());

          The isAssignableFrom wants a class:

          The method isAssignableFrom(Class<?>) in the type Class<ServletRedirectResult> is not applicable for the arguments (String)

          ######

          		Map<String, ResultConfig> results = null;
          		ResultConfig resultConfig = null;
          
          		try {
          
          			results = invocation.getProxy().getConfig().getResults();
          			resultConfig = results.get(resultCode);
          
          		} catch (NullPointerException e) {
          			LOG.debug("Got NPE trying to read result configuration for resultCode [{}]", resultCode);
          		}
          
          		boolean isRedirect = false;
          		try {
          			// isRedirect = invocation.getResult() instanceof ServletRedirectResult;
          			isRedirect = ServletRedirectResult.class.getName().equals(resultConfig.getClassName());
          		} catch (Exception e) {
          			LOG.warn("Cannot read result configuration!", e);
          		}
          
          Show
          ghuber Greg Huber added a comment - - edited The interceptor logic has changed, now need to get the isRedirect from the invocation, not from invocation.getResult() which is null. isRedirect = ServletRedirectResult.class.getName().equals(resultConfig.getClassName()); The isAssignableFrom wants a class: The method isAssignableFrom(Class<?>) in the type Class<ServletRedirectResult> is not applicable for the arguments (String) ###### Map< String , ResultConfig> results = null ; ResultConfig resultConfig = null ; try { results = invocation.getProxy().getConfig().getResults(); resultConfig = results.get(resultCode); } catch (NullPointerException e) { LOG.debug( "Got NPE trying to read result configuration for resultCode [{}]" , resultCode); } boolean isRedirect = false ; try { // isRedirect = invocation.getResult() instanceof ServletRedirectResult; isRedirect = ServletRedirectResult.class.getName().equals(resultConfig.getClassName()); } catch (Exception e) { LOG.warn( "Cannot read result configuration!" , e); }
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Sure but please reopen the issue or create a new one - do you want to to be fixed in 2.3.x series or fixing in 2.5 is ok for you?

          Show
          lukaszlenart Lukasz Lenart added a comment - Sure but please reopen the issue or create a new one - do you want to to be fixed in 2.3.x series or fixing in 2.5 is ok for you?
          Hide
          jtdevos Jim deVos added a comment -

          Hi Lukasz! I noticed your commit replaces using the 'instanceof' operator on invocation.getResult(), so i'm guessing there was a problem doing it that way, too. If you're looking to avoid referencing the result object, could you do something like this?

          isRedirect = ServletRedirectResult.class.isAssignableFrom(resultConfig.getClassName());
          

          I ask because my team uses a custom subclass of ServletRedirectResult, e.g. "org.tdar.struts.ServletRedirectResult". After updating to 2.3.8 our messages no longer survive a redirect, and I suspect that this string comparison is the root cause. Thanks for your time --Jim

          Show
          jtdevos Jim deVos added a comment - Hi Lukasz! I noticed your commit replaces using the 'instanceof' operator on invocation.getResult(), so i'm guessing there was a problem doing it that way, too. If you're looking to avoid referencing the result object, could you do something like this? isRedirect = ServletRedirectResult.class.isAssignableFrom(resultConfig.getClassName()); I ask because my team uses a custom subclass of ServletRedirectResult, e.g. "org.tdar.struts.ServletRedirectResult". After updating to 2.3.8 our messages no longer survive a redirect, and I suspect that this string comparison is the root cause. Thanks for your time --Jim
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Struts-JDK7-master #444 (See https://builds.apache.org/job/Struts-JDK7-master/444/)
          WW-4605 Reverts to previous flow when result is created just before (lukaszlenart: rev 88526180375f958ea57eceedb3017f4b7637ef68)

          • core/src/test/java/org/apache/struts2/interceptor/MessageStorePreResultListenerTest.java
          • core/src/main/java/com/opensymphony/xwork2/DefaultActionInvocation.java
          • core/src/test/java/org/apache/struts2/views/jsp/ActionTagTest.java
          • core/src/main/java/org/apache/struts2/interceptor/MessageStorePreResultListener.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Struts-JDK7-master #444 (See https://builds.apache.org/job/Struts-JDK7-master/444/ ) WW-4605 Reverts to previous flow when result is created just before (lukaszlenart: rev 88526180375f958ea57eceedb3017f4b7637ef68) core/src/test/java/org/apache/struts2/interceptor/MessageStorePreResultListenerTest.java core/src/main/java/com/opensymphony/xwork2/DefaultActionInvocation.java core/src/test/java/org/apache/struts2/views/jsp/ActionTagTest.java core/src/main/java/org/apache/struts2/interceptor/MessageStorePreResultListener.java
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 88526180375f958ea57eceedb3017f4b7637ef68 in struts's branch refs/heads/master from Lukasz Lenart
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=8852618 ]

          WW-4605 Reverts to previous flow when result is created just before executing it

          Show
          jira-bot ASF subversion and git services added a comment - Commit 88526180375f958ea57eceedb3017f4b7637ef68 in struts's branch refs/heads/master from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=8852618 ] WW-4605 Reverts to previous flow when result is created just before executing it
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Struts-JDK6-support-2.3 #1000 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/1000/)
          WW-4605 Reverts to previous flow when result is created just before (lukaszlenart: rev 6b497ef8f7091224b4e87a825fdbc50b02a21c3d)

          • core/src/main/java/org/apache/struts2/interceptor/MessageStorePreResultListener.java
          • core/src/test/java/org/apache/struts2/views/jsp/ActionTagTest.java
          • xwork-core/src/main/java/com/opensymphony/xwork2/DefaultActionInvocation.java
          • core/src/test/java/org/apache/struts2/interceptor/MessageStorePreResultListenerTest.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK6-support-2.3 #1000 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/1000/ ) WW-4605 Reverts to previous flow when result is created just before (lukaszlenart: rev 6b497ef8f7091224b4e87a825fdbc50b02a21c3d) core/src/main/java/org/apache/struts2/interceptor/MessageStorePreResultListener.java core/src/test/java/org/apache/struts2/views/jsp/ActionTagTest.java xwork-core/src/main/java/com/opensymphony/xwork2/DefaultActionInvocation.java core/src/test/java/org/apache/struts2/interceptor/MessageStorePreResultListenerTest.java
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 6b497ef8f7091224b4e87a825fdbc50b02a21c3d in struts's branch refs/heads/support-2-3 from Lukasz Lenart
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=6b497ef ]

          WW-4605 Reverts to previous flow when result is created just before executing it

          Show
          jira-bot ASF subversion and git services added a comment - Commit 6b497ef8f7091224b4e87a825fdbc50b02a21c3d in struts's branch refs/heads/support-2-3 from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=6b497ef ] WW-4605 Reverts to previous flow when result is created just before executing it
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Great! There were no more other changes in that area.

          Show
          lukaszlenart Lukasz Lenart added a comment - Great! There were no more other changes in that area.
          Hide
          ghuber Greg Huber added a comment -

          I think we are OK, I reinstated the "result = createResult();" in the DefaultActionInvocation.executeResult() method and included the modified MessageStoreInterceptor and MessageStorePreResultListener classes and everything seems to work, the messages now show on redirects. Were there any other changes made that I missed? Maybe I can check/test for, what does not now work.

          Show
          ghuber Greg Huber added a comment - I think we are OK, I reinstated the "result = createResult();" in the DefaultActionInvocation.executeResult() method and included the modified MessageStoreInterceptor and MessageStorePreResultListener classes and everything seems to work, the messages now show on redirects. Were there any other changes made that I missed? Maybe I can check/test for, what does not now work.
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          I have no better idea, reverting back to previous order of creating a Result will break MessageStore implementation. Another idea is to revert WW-4600 as well, thus will remove the need of refactoring to PreResultListener and this can be done in 2.5.

          Show
          lukaszlenart Lukasz Lenart added a comment - I have no better idea, reverting back to previous order of creating a Result will break MessageStore implementation. Another idea is to revert WW-4600 as well, thus will remove the need of refactoring to PreResultListener and this can be done in 2.5.
          Hide
          ghuber Greg Huber added a comment - - edited

          I extend MethodFilterInterceptor as not all my actions have mobile screens eg cancel, so there is quite a lot of filtering. Why I ended up with a PreResultListener(), I guess it was a good (or bad) day coding.

          Does it use the result in the code anyway? If I swap them back it still works with no npe.

          if (preResultListeners != null) {
                    LOG.trace("Executing PreResultListeners for result [{}]", result);
          
                    for (Object preResultListener : preResultListeners) {
                      PreResultListener listener = (PreResultListener) preResultListener;
          
                      String _profileKey = "preResultListener: ";
                      try {
                        UtilTimerStack.push(_profileKey);
                        listener.beforeResult(this, resultCode);
                      }
                      finally {
                        UtilTimerStack.pop(_profileKey);
                      }
                    }
                  }
          
          Show
          ghuber Greg Huber added a comment - - edited I extend MethodFilterInterceptor as not all my actions have mobile screens eg cancel, so there is quite a lot of filtering. Why I ended up with a PreResultListener(), I guess it was a good (or bad) day coding. Does it use the result in the code anyway? If I swap them back it still works with no npe. if (preResultListeners != null ) { LOG.trace( "Executing PreResultListeners for result [{}]" , result); for ( Object preResultListener : preResultListeners) { PreResultListener listener = (PreResultListener) preResultListener; String _profileKey = "preResultListener: " ; try { UtilTimerStack.push(_profileKey); listener.beforeResult( this , resultCode); } finally { UtilTimerStack.pop(_profileKey); } } }
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Result must be created before PreResultListener gets executed because in other case it will be null and MessageStoreInterceptor's PreResultListener won't work.

          Btw. why do you use PreResultListener to modify result? Basing on interceptor should be enough.

          Show
          lukaszlenart Lukasz Lenart added a comment - Result must be created before PreResultListener gets executed because in other case it will be null and MessageStoreInterceptor 's PreResultListener won't work. Btw. why do you use PreResultListener to modify result? Basing on interceptor should be enough.
          Hide
          ghuber Greg Huber added a comment - - edited

          Is it necessary to move the position of the

          result = createResult(); in DefaultActionInvocation

          as it stops my Interceptor from working. I am modifying the result code in a PreResultListener() interceptor and need the result to be created after applying the interceptor otherwise I have the wrong value.

          ......
          public String intercept(ActionInvocation invocation) throws Exception {
          
              if (applyInterceptor(invocation)) {
          
                invocation.addPreResultListener(new PreResultListener() {
                  public void beforeResult(ActionInvocation invocation,
                      String resultCode) {
          
                    final ActionContext context = invocation
                        .getInvocationContext();
          
                    final HttpServletRequest request = (HttpServletRequest) context
                        .get(StrutsStatics.HTTP_REQUEST);
          
                    if (DeviceStore.isMobileDevice(request, true)
                        && applyResult(resultCode)) {
          
                      invocation.setResultCode(resultCode + "-"
                          + DeviceType.mobile);
          
                      Object action = invocation.getAction();
          
                      if (action instanceof UIAction) {
          
                        UIAction theAction = (UIAction) action;
                        theAction.setMobileAwareResult(true);
          
                      }
                    }
          
                  }
                });
          
              }
          
              return doIntercept(invocation);
            }
          ....
          

          My interceptor appends to the result the device type, so if its a mobile I get list-mobile

          <result name="list" type="tiles">.MyView</result>
          <result name="list-mobile" type="tiles">.MyView-mobile</result>

          Calling it where it currently is, all I get is list even though I am setting the ResultCode using the above.

          Show
          ghuber Greg Huber added a comment - - edited Is it necessary to move the position of the result = createResult(); in DefaultActionInvocation as it stops my Interceptor from working. I am modifying the result code in a PreResultListener() interceptor and need the result to be created after applying the interceptor otherwise I have the wrong value. ...... public String intercept(ActionInvocation invocation) throws Exception { if (applyInterceptor(invocation)) { invocation.addPreResultListener( new PreResultListener() { public void beforeResult(ActionInvocation invocation, String resultCode) { final ActionContext context = invocation .getInvocationContext(); final HttpServletRequest request = (HttpServletRequest) context .get(StrutsStatics.HTTP_REQUEST); if (DeviceStore.isMobileDevice(request, true ) && applyResult(resultCode)) { invocation.setResultCode(resultCode + "-" + DeviceType.mobile); Object action = invocation.getAction(); if (action instanceof UIAction) { UIAction theAction = (UIAction) action; theAction.setMobileAwareResult( true ); } } } }); } return doIntercept(invocation); } .... My interceptor appends to the result the device type, so if its a mobile I get list-mobile <result name="list" type="tiles">.MyView</result> <result name="list-mobile" type="tiles">.MyView-mobile</result> Calling it where it currently is, all I get is list even though I am setting the ResultCode using the above.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Struts-JDK6-support-2.3 #976 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/976/)
          WW-4600 Ports solution with PreResultListener from WW-4605 (lukaszlenart: rev 9c7b8336685d810a657f3f3c56ad8662dcc85dbf)

          • core/src/test/java/org/apache/struts2/interceptor/MessageStorePreResultListenerTest.java
          • xwork-core/src/main/java/com/opensymphony/xwork2/DefaultActionInvocation.java
          • core/src/main/java/org/apache/struts2/interceptor/MessageStorePreResultListener.java
          • core/src/main/java/org/apache/struts2/interceptor/MessageStoreInterceptor.java
          • xwork-core/src/test/java/com/opensymphony/xwork2/DefaultActionInvocationTest.java
          • core/src/test/java/org/apache/struts2/interceptor/MessageStoreInterceptorTest.java
          • core/src/test/java/org/apache/struts2/views/jsp/ActionTagTest.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK6-support-2.3 #976 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/976/ ) WW-4600 Ports solution with PreResultListener from WW-4605 (lukaszlenart: rev 9c7b8336685d810a657f3f3c56ad8662dcc85dbf) core/src/test/java/org/apache/struts2/interceptor/MessageStorePreResultListenerTest.java xwork-core/src/main/java/com/opensymphony/xwork2/DefaultActionInvocation.java core/src/main/java/org/apache/struts2/interceptor/MessageStorePreResultListener.java core/src/main/java/org/apache/struts2/interceptor/MessageStoreInterceptor.java xwork-core/src/test/java/com/opensymphony/xwork2/DefaultActionInvocationTest.java core/src/test/java/org/apache/struts2/interceptor/MessageStoreInterceptorTest.java core/src/test/java/org/apache/struts2/views/jsp/ActionTagTest.java
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 9c7b8336685d810a657f3f3c56ad8662dcc85dbf in struts's branch refs/heads/support-2-3 from Lukasz Lenart
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=9c7b833 ]

          WW-4600 Ports solution with PreResultListener from WW-4605

          Show
          jira-bot ASF subversion and git services added a comment - Commit 9c7b8336685d810a657f3f3c56ad8662dcc85dbf in struts's branch refs/heads/support-2-3 from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=9c7b833 ] WW-4600 Ports solution with PreResultListener from WW-4605
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 857195c1b1fef638bcdb7fbb8f212a7b384ecbaf in struts's branch refs/heads/master from Lukasz Lenart
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=857195c ]

          WW-4605 Refactors MessageStoreInterceptor to use listener

          Show
          jira-bot ASF subversion and git services added a comment - Commit 857195c1b1fef638bcdb7fbb8f212a7b384ecbaf in struts's branch refs/heads/master from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=857195c ] WW-4605 Refactors MessageStoreInterceptor to use listener

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development