Details

    • Improvement
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 2.2.0
    • JSR-344
    • None

    Description

      The current code for server side state saving creates one key per request to store the view state. This is ok, but it is not necessary for ajax requests.

      The reason why is not necessary is because you can never go back to a page when using ajax. If you are on page A and the current request is an ajax request and it returns to the same page and the view is the same that the one that has been restored, the key or the token sent does not need to change, what changes is the internal state of the view. From the client side the page is the same. We can take advantage of this fact and just update the state stored in SerializedViewCollection for the view.

      The challenge here is detect when this strategy is applicable. For example, what happen if there is an ajax redirect? It looks is a good idea for implement in 2.2, because it avoids to store unnecessary information into session and optimize the use of each view slot.

      Attachments

        1. ajaxviewkeytest2.patch
          2 kB
          Dora Rajappan
        2. ajaxviewkeytest.patch
          6 kB
          Dora Rajappan
        3. ajaxviewsamekey2.patch
          1 kB
          Dora Rajappan
        4. ajaxviewsamekey3.patch
          0.5 kB
          Dora Rajappan
        5. ajaxviewsamekey.patch
          11 kB
          Dora Rajappan
        6. MYFACES-3804-4.patch
          32 kB
          Leonardo Uribe

        Issue Links

        Activity

          dorarajappan Dora Rajappan added a comment -
          1. 1. This key generation per request for ajax is also fine. Instead of changing the logic for ajax just create a key, its the same over head. Depending on the max count of NumberOfSequentialViewsInSession these previous keys/values are removed from the map. Hence the map size is limited to this count. If new key is generated for ajax number of views in the collection goes up to max count. Of course using same key can save a few objects in this map. Current algorithm is good and you can avoid ViewExpiredException by making ajax specific logic.

          #2. Ajax redirect will not add a view in this map. Cause partial rendering of the current page is avoided.

          @RequestScoped
          public class RedirectBean {

          public String redirect() {

          FacesContext ctx = FacesContext.getCurrentInstance();

          ExternalContext extContext = ctx.getExternalContext();
          String url = extContext.encodeActionURL(ctx.getApplication().getViewHandler().getActionURL(ctx, "/ajax/redirecttarget.xhtml"));
          try

          { extContext.redirect(url); }

          catch (IOException ioe)

          { throw new FacesException(ioe); }

          return null;

          }

          <h:head>
          <title>Ajax Redirect</title>
          </h:head>
          <h:body>
          <h:form id="form">
          <h:commandButton id="redirect" value="Redirect" action="#

          {redirectBean.redirect}

          ">
          <f:ajax execute="@this" render="@none"/>
          </h:commandButton>
          </h:form>
          </h:body>

          dorarajappan Dora Rajappan added a comment - 1. This key generation per request for ajax is also fine. Instead of changing the logic for ajax just create a key, its the same over head. Depending on the max count of NumberOfSequentialViewsInSession these previous keys/values are removed from the map. Hence the map size is limited to this count. If new key is generated for ajax number of views in the collection goes up to max count. Of course using same key can save a few objects in this map. Current algorithm is good and you can avoid ViewExpiredException by making ajax specific logic. #2. Ajax redirect will not add a view in this map. Cause partial rendering of the current page is avoided. @RequestScoped public class RedirectBean { public String redirect() { FacesContext ctx = FacesContext.getCurrentInstance(); ExternalContext extContext = ctx.getExternalContext(); String url = extContext.encodeActionURL(ctx.getApplication().getViewHandler().getActionURL(ctx, "/ajax/redirecttarget.xhtml")); try { extContext.redirect(url); } catch (IOException ioe) { throw new FacesException(ioe); } return null; } <h:head> <title>Ajax Redirect</title> </h:head> <h:body> <h:form id="form"> <h:commandButton id="redirect" value="Redirect" action="# {redirectBean.redirect} "> <f:ajax execute="@this" render="@none"/> </h:commandButton> </h:form> </h:body>
          dorarajappan Dora Rajappan added a comment -

          One good aspect of using the same key for ajax is that it will retain valid views in map than redundant ajax views.

          dorarajappan Dora Rajappan added a comment - One good aspect of using the same key for ajax is that it will retain valid views in map than redundant ajax views.
          dorarajappan Dora Rajappan added a comment -

          SerializedViewCollection and ServerSideStateCacheImpl updated. Works well for web configuration

          <context-param>
          <param-name>javax.faces.STATE_SAVING_METHOD</param-name>
          <param-value>server</param-value>
          </context-param>
          <context-param>
          <param-name>org.apache.myfaces.HANDLE_STATE_CACHING_MECHANICS</param-name>
          <param-value>true</param-value>
          </context-param>
          <context-param>
          <param-name>org.apache.myfaces.NUMBER_OF_SEQUENTIAL_VIEWS_IN_SESSION</param-name>
          <param-value>3</param-value>
          </context-param>

          dorarajappan Dora Rajappan added a comment - SerializedViewCollection and ServerSideStateCacheImpl updated. Works well for web configuration <context-param> <param-name>javax.faces.STATE_SAVING_METHOD</param-name> <param-value>server</param-value> </context-param> <context-param> <param-name>org.apache.myfaces.HANDLE_STATE_CACHING_MECHANICS</param-name> <param-value>true</param-value> </context-param> <context-param> <param-name>org.apache.myfaces.NUMBER_OF_SEQUENTIAL_VIEWS_IN_SESSION</param-name> <param-value>3</param-value> </context-param>
          dorarajappan Dora Rajappan added a comment -

          Also for for web configuration below the fix is good.
          <context-param>
          <param-name>javax.faces.STATE_SAVING_METHOD</param-name>
          <param-value>client</param-value>
          </context-param>

          dorarajappan Dora Rajappan added a comment - Also for for web configuration below the fix is good. <context-param> <param-name>javax.faces.STATE_SAVING_METHOD</param-name> <param-value>client</param-value> </context-param>
          dorarajappan Dora Rajappan added a comment -

          Including the test case for ajax requests and view saved with same key.

          dorarajappan Dora Rajappan added a comment - Including the test case for ajax requests and view saved with same key.
          dorarajappan Dora Rajappan added a comment -

          Including relevant comments from 3816
          What have you decided about the current behaviour of ajax redirect loosing state information when changes are executed with redirect? Have you decided to flag in redirect and write it in response after save state in rendering phase?

          And this behaviour true for redirect via navigation handler. In render phase it goes to response complete and state saving is avoided.

          <h:commandButton action="#

          {bean.action}" value="Invoke and redirect">
          <f:ajax execute="@this"/>
          </h:commandButton>

          <navigation-rules>
          <navigation-rule>
          <from-view-id>/form.xhtml</from-view-id>
          <navigation-case>
          <from-action>#{bean.action}

          </from-action>
          <from-outcome>success</from-outcome>
          <to-view-id>/target.xhtml</to-view-id>
          <redirect/>
          </navigation-case>
          </navigation-rule>
          </navigation-rules>

          @RequestScoped @ManagedBean
          public class Bean

          { public String action() { return "success"; }


          }

          I think its a good idea to follow client side state saving behaviour for ajax redirect.

          dorarajappan Dora Rajappan added a comment - Including relevant comments from 3816 What have you decided about the current behaviour of ajax redirect loosing state information when changes are executed with redirect? Have you decided to flag in redirect and write it in response after save state in rendering phase? And this behaviour true for redirect via navigation handler. In render phase it goes to response complete and state saving is avoided. <h:commandButton action="# {bean.action}" value="Invoke and redirect"> <f:ajax execute="@this"/> </h:commandButton> <navigation-rules> <navigation-rule> <from-view-id>/form.xhtml</from-view-id> <navigation-case> <from-action>#{bean.action} </from-action> <from-outcome>success</from-outcome> <to-view-id>/target.xhtml</to-view-id> <redirect/> </navigation-case> </navigation-rule> </navigation-rules> @RequestScoped @ManagedBean public class Bean { public String action() { return "success"; } } I think its a good idea to follow client side state saving behaviour for ajax redirect.
          dorarajappan Dora Rajappan added a comment -

          I have opened a jira issue 3817 for state saving problem with ajax redirect. Once that is fixed the view key fix automatically applies to redirect scenario. Shall I commit this code. This address when there is no key initially in client window mode and an ajax request is made.

          dorarajappan Dora Rajappan added a comment - I have opened a jira issue 3817 for state saving problem with ajax redirect. Once that is fixed the view key fix automatically applies to redirect scenario. Shall I commit this code. This address when there is no key initially in client window mode and an ajax request is made.
          lu4242 Leonardo Uribe added a comment -

          The code contains tab characters and does not follow the code conventions enforced by checkstyle. How are you writing the code? are you using maven? svn? I receive checkstyle errors when I apply the code. Are you compiling the code using maven?. The patch must not contain tab characters, and the brackets must be properly set.

          About the patch, I think reuse add() method of SerializedViewCollection is not the right thing to do. Instead, the best way is create a method called for example update(...) and call it from outside. Also the condition in restoreSerializedView looks suspicious, because with that you are bypassing all the logic related to generate the next token. Suppose you have an ajax request and then there is a navigation, not caused by a redirect but normal navigation. In that theorical case, the key will not be generated again.

          If you want to discuss this issue please send a mail to dev likes with topic starting with [core] .... as topic.

          lu4242 Leonardo Uribe added a comment - The code contains tab characters and does not follow the code conventions enforced by checkstyle. How are you writing the code? are you using maven? svn? I receive checkstyle errors when I apply the code. Are you compiling the code using maven?. The patch must not contain tab characters, and the brackets must be properly set. About the patch, I think reuse add() method of SerializedViewCollection is not the right thing to do. Instead, the best way is create a method called for example update(...) and call it from outside. Also the condition in restoreSerializedView looks suspicious, because with that you are bypassing all the logic related to generate the next token. Suppose you have an ajax request and then there is a navigation, not caused by a redirect but normal navigation. In that theorical case, the key will not be generated again. If you want to discuss this issue please send a mail to dev likes with topic starting with [core] .... as topic.
          dorarajappan Dora Rajappan added a comment -

          Normal Navigation for ajax is saved with new key. Update method included in SerializedViewCollection. Server state id is put in SERVER_STATE_ID instead of SEQUENCE_PARAM.

          dorarajappan Dora Rajappan added a comment - Normal Navigation for ajax is saved with new key. Update method included in SerializedViewCollection. Server state id is put in SERVER_STATE_ID instead of SEQUENCE_PARAM.
          dorarajappan Dora Rajappan added a comment -

          Thanks for your reply. Let this discussion go in here for ease.
          You are talking about the tab characters in testcase. I will submit new patch for that. I had not saved the format.
          I am not using checkstyle for now. I will configure it.
          I use maven for build/test, svn yes but outside of eclipse.

          add() can change to update. I will make that change.

          In normal navigation you have a key and next key for the new view built which passed to add of SerialisedViewCollection.
          But in the case of ajax and normal navigation same thing happens new view built is passed to add of SerialisedViewCollection. What happens now is in invoke application executor the action is processed and navigation handled and next view is created and this view is in same execute cycle and this overwrites the ajax view and gets rendered with ajax view id in the page.

          Not altering this behaviour above ajaxwithsameviewkey fix is extended to normal navigation by means of add instead of update of SerialisedViewCollection.

          Regarding the condition in restoreSerializedView looks suspicious, this bypass is only for ajax request and hence its affects only ajax requests. But I have changed code to put the state id in SERVER_STATE_ID and not in SEQUENCE_PARAM.

          Is this fine tuning required for ajax since redirect and navigation are avoided

          dorarajappan Dora Rajappan added a comment - Thanks for your reply. Let this discussion go in here for ease. You are talking about the tab characters in testcase. I will submit new patch for that. I had not saved the format. I am not using checkstyle for now. I will configure it. I use maven for build/test, svn yes but outside of eclipse. add() can change to update. I will make that change. In normal navigation you have a key and next key for the new view built which passed to add of SerialisedViewCollection. But in the case of ajax and normal navigation same thing happens new view built is passed to add of SerialisedViewCollection. What happens now is in invoke application executor the action is processed and navigation handled and next view is created and this view is in same execute cycle and this overwrites the ajax view and gets rendered with ajax view id in the page. Not altering this behaviour above ajaxwithsameviewkey fix is extended to normal navigation by means of add instead of update of SerialisedViewCollection. Regarding the condition in restoreSerializedView looks suspicious, this bypass is only for ajax request and hence its affects only ajax requests. But I have changed code to put the state id in SERVER_STATE_ID and not in SEQUENCE_PARAM. Is this fine tuning required for ajax since redirect and navigation are avoided
          mkienenb Mike Kienenberger added a comment - - edited

          Dora,

          We need to move discussion to the developer mailing list, and reserve the JIRA for resolving issues.
          There are a number of reasons why the JIRA issue is not the best place for discussion, which I wrote about here:

          http://mail-archives.apache.org/mod_mbox/myfaces-dev/201311.mbox/%3CCAM1yOjZHhSRwrAQ2Va_Em0yAcPD9mA9D-7daCG7z-eHfr%3DZ0-g%40mail.gmail.com%3E

          I'm sorry that I didn't speak up a while back when I first noticed it happening, but we need to resume using the developer mailing list for its intended purpose. Please repost your comments from here directly on the developer mailing list.

          mkienenb Mike Kienenberger added a comment - - edited Dora, We need to move discussion to the developer mailing list, and reserve the JIRA for resolving issues. There are a number of reasons why the JIRA issue is not the best place for discussion, which I wrote about here: http://mail-archives.apache.org/mod_mbox/myfaces-dev/201311.mbox/%3CCAM1yOjZHhSRwrAQ2Va_Em0yAcPD9mA9D-7daCG7z-eHfr%3DZ0-g%40mail.gmail.com%3E I'm sorry that I didn't speak up a while back when I first noticed it happening, but we need to resume using the developer mailing list for its intended purpose. Please repost your comments from here directly on the developer mailing list.
          dorarajappan Dora Rajappan added a comment -

          Can I checkin this code? This checkstyle compliant now.

          dorarajappan Dora Rajappan added a comment - Can I checkin this code? This checkstyle compliant now.
          lu4242 Leonardo Uribe added a comment -

          I have checked the patch provided, but I didn't see anything new. If you provide a solution and you need to do some improvements, please include the complete solution in a single patch (including tests and everything). The common convention is put the issue number on the patch and another number to indicate the iteration.

          I did some cleanup on the server side state saving algorithm and on the way I have studied this problem and the one in MYFACES-3806 and I have realized both are related somehow, so I fixed both in a single patch. I have attached it, so you can see the difference. The fundamental idea that was not in the first patches was change the code but keep ServerSideStateCacheImpl.getNextViewSequence(...) and nextViewSequence(...) logic in place. The trick was very simple: when the view is restored, if there is an ajax request, save the viewId and the viewKey of the current request. Then when a view sequence is needed, check if the current request is still on the same view and if that so reuse the current viewKey.

          lu4242 Leonardo Uribe added a comment - I have checked the patch provided, but I didn't see anything new. If you provide a solution and you need to do some improvements, please include the complete solution in a single patch (including tests and everything). The common convention is put the issue number on the patch and another number to indicate the iteration. I did some cleanup on the server side state saving algorithm and on the way I have studied this problem and the one in MYFACES-3806 and I have realized both are related somehow, so I fixed both in a single patch. I have attached it, so you can see the difference. The fundamental idea that was not in the first patches was change the code but keep ServerSideStateCacheImpl.getNextViewSequence(...) and nextViewSequence(...) logic in place. The trick was very simple: when the view is restored, if there is an ajax request, save the viewId and the viewKey of the current request. Then when a view sequence is needed, check if the current request is still on the same view and if that so reuse the current viewKey.

          People

            lu4242 Leonardo Uribe
            lu4242 Leonardo Uribe
            Votes:
            1 Vote for this issue
            Watchers:
            Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Slack

                In order to see discussions, first confirm access to your Slack account(s) in the following workspace(s): ASF