MyFaces Tomahawk
  1. MyFaces Tomahawk
  2. TOMAHAWK-503

Capture and restore saveState Beans and messages when using redirect navigation rule

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.1.5-SNAPSHOT
    • Fix Version/s: 1.1.5
    • Component/s: None
    • Labels:
      None

      Description

      Ok, this is a try to get what is described in the summary.

      The heart of this patch is the RedirectTracker.

      How it works:
      *) In NavigationHandlerImpl when it comes to the redirect the RedirectTracker will capture all required (save-state beans/messages) data. The tracker appends a special parameter (_rtid=) to the redirect url so the saved state can be looked up in an map.
      *) In LifeCycleImpl in restoreView the RedirectTracker will check for the special parameter and lookup the map using its value. If an entry was found the data will be restored.
      *) In UISaveState every bean rematerialized in restoreState will put it in a request-scoped map

      Known limitations:
      *) need to make the number of tracked redirects configureable (back-button) currently its hard-coded to 20
      *) some, as I didnt tested it very well

      I added this patch as a base to discuss what could be made better, though the goal is to add this functionality to myFaces.
      I hope some else jump in and we get a vital discussion about this topic.

      1. impl.diff
        35 kB
        Mario Ivankovits
      2. myfaces-extension.zip
        5 kB
        Kevin Galligan
      3. RedirectTracker.java
        6 kB
        Mario Ivankovits
      4. sandbox.patch
        3 kB
        Kevin Galligan
      5. sandbox2.patch
        4 kB
        Kevin Galligan
      6. tom.diff
        2 kB
        Mario Ivankovits

        Issue Links

          Activity

          Hide
          Martin Marinschek added a comment -

          I like the idea - and I am assigning this to me as to not forget about this one.

          Another very nice proposal, Mario!

          Any other committer willing to comment on this?

          regards,

          Martin

          Show
          Martin Marinschek added a comment - I like the idea - and I am assigning this to me as to not forget about this one. Another very nice proposal, Mario! Any other committer willing to comment on this? regards, Martin
          Hide
          Mario Ivankovits added a comment -

          We should save the requestParameterMap too.

          This should be done to save parameters like in

          <h:commandLink .... action="nextPage">
          <f:param name="abc" value="123" />
          </h:commandLink/>

          else if the navigationRule for "nextPage" uses "<redirect" this parameter will be lost.

          I would like to extend my patch, but only if there is a chance to get all this stuff commited.
          Its a little bit silent around this topic, so if you think we dont need it I wouldnt want to waste my time on it.

          I think it would be VERY nice if we get myfaces to work with <redirect the same way as without.

          Thanks!

          Show
          Mario Ivankovits added a comment - We should save the requestParameterMap too. This should be done to save parameters like in <h:commandLink .... action="nextPage"> <f:param name="abc" value="123" /> </h:commandLink/> else if the navigationRule for "nextPage" uses "<redirect" this parameter will be lost. I would like to extend my patch, but only if there is a chance to get all this stuff commited. Its a little bit silent around this topic, so if you think we dont need it I wouldnt want to waste my time on it. I think it would be VERY nice if we get myfaces to work with <redirect the same way as without. Thanks!
          Hide
          Mike Kienenberger added a comment -

          Guy Bashan (http://issues.apache.org/jira/secure/ViewProfile.jspa?name=bashan) writes,
          > Using the navigation mechanism with redirect disables the following important functionallity:
          > 1) Passing parameters between pages (with param tag).

          Show
          Mike Kienenberger added a comment - Guy Bashan ( http://issues.apache.org/jira/secure/ViewProfile.jspa?name=bashan ) writes, > Using the navigation mechanism with redirect disables the following important functionallity: > 1) Passing parameters between pages (with param tag).
          Hide
          Mario Ivankovits added a comment -

          solved in tomahawk-sandbox (RedirectTracker) except for the param thing

          Show
          Mario Ivankovits added a comment - solved in tomahawk-sandbox (RedirectTracker) except for the param thing
          Hide
          Hendrik Ebel added a comment -

          Thanks for this proposal, it's very useful!

          But I have found a little bug in the sandbox code: after 10 requests no restore will be executed. This problem can by solved with usage of

          String mapKey = Long.toString(rtid);

          instead of

          String mapKey = Long.toString(rtid, Character.MAX_RADIX);

          in RedirectTrackerManager.trackRedirect(...)

          Show
          Hendrik Ebel added a comment - Thanks for this proposal, it's very useful! But I have found a little bug in the sandbox code: after 10 requests no restore will be executed. This problem can by solved with usage of String mapKey = Long.toString(rtid); instead of String mapKey = Long.toString(rtid, Character.MAX_RADIX); in RedirectTrackerManager.trackRedirect(...)
          Hide
          Kevin Galligan added a comment -

          I've been playing around with this for a little bit and have some thoughts. The first thought is this is great and I've felt something like this is needed. However, I think the functionality needs to be configurable. There are several general use cases where this would be very helpful, but also situations where this becomes a problem.

          The first general issue I'm concerned with is related to the following:

          "I think it would be VERY nice if we get myfaces to work with <redirect the same way as without"

          I think this would be true is certain situations, but just making '<redirect/>' the same seems like it would actually break the JSF implementation. In concept, myfaces sits on top of and extends the standard jsf implementation. This, however, alters the base of functionality. I would think it would be better to allow redirects to carry state information, but it should be explicitly turned on rather than just happening.

          1) Want to break state completely. This is the "standard" situation. You redirect, and all page and request level state is lost. This is useful if the user has completed a full "page cycle" and they are being redirected to a home page of sorts. In fact, with are recent application I had some issues with using a redirect and expecting the state to be completely refreshed. Bad coding? Maybe, but still. I was expecting it to work a different way.

          2) Want to post a message, but otherwise drop state. For example, adding a new "Account" record. When its done, the user would be returned to the "Account List" page, but a message like "Account added" should be posted to the top. All other request state, however, should be lost.

          3) Full requst state retained. This allows the "correct" url to show up in the browser, and allows the user to hit "Refresh" without the double post problem.

          I think there should be some way of turning this on and off for particular requests. In fact, I think it should be more like by default it functions as it does currently, and must be explicitly turned on to carry state. I'm not sure how that would be configured. The "natural" place would seem to be in the config file, but I don't really like the idea of either changing the config file format or adding custom info to the url.

          Another method, that would be a little more "hacky", but functional would be to add a special key/value to the request map, and test for it in the 'redirect' function of the 'RedirectTrackerExternalContextWrapper'. So, if you have a function that performs something, at the end just put something like the following:

          public String someFunction()

          { .... (the code) RedirectTrackerUtils.trackRedirect(FacesContext.getCurrentInstance()); return "success"; }

          'trackRedirect' would just be something like

          public static void trackRedirect(FacesContext context)

          { context.getExternalContext().getRequestMap().put("redirectTrackerEnabled", Boolean.TRUE); }

          This of course clutters the code with UI specifc issues, but I think its workable from a purely functional perspective. Thoughts?

          Also, I think it might be a good idea to use an id number that's a little more obfuscated. Having a low numberd sequential id means somebody is going to start messing around with it manually.

          Show
          Kevin Galligan added a comment - I've been playing around with this for a little bit and have some thoughts. The first thought is this is great and I've felt something like this is needed. However, I think the functionality needs to be configurable. There are several general use cases where this would be very helpful, but also situations where this becomes a problem. The first general issue I'm concerned with is related to the following: "I think it would be VERY nice if we get myfaces to work with <redirect the same way as without" I think this would be true is certain situations, but just making '<redirect/>' the same seems like it would actually break the JSF implementation. In concept, myfaces sits on top of and extends the standard jsf implementation. This, however, alters the base of functionality. I would think it would be better to allow redirects to carry state information, but it should be explicitly turned on rather than just happening. 1) Want to break state completely. This is the "standard" situation. You redirect, and all page and request level state is lost. This is useful if the user has completed a full "page cycle" and they are being redirected to a home page of sorts. In fact, with are recent application I had some issues with using a redirect and expecting the state to be completely refreshed. Bad coding? Maybe, but still. I was expecting it to work a different way. 2) Want to post a message, but otherwise drop state. For example, adding a new "Account" record. When its done, the user would be returned to the "Account List" page, but a message like "Account added" should be posted to the top. All other request state, however, should be lost. 3) Full requst state retained. This allows the "correct" url to show up in the browser, and allows the user to hit "Refresh" without the double post problem. I think there should be some way of turning this on and off for particular requests. In fact, I think it should be more like by default it functions as it does currently, and must be explicitly turned on to carry state. I'm not sure how that would be configured. The "natural" place would seem to be in the config file, but I don't really like the idea of either changing the config file format or adding custom info to the url. Another method, that would be a little more "hacky", but functional would be to add a special key/value to the request map, and test for it in the 'redirect' function of the 'RedirectTrackerExternalContextWrapper'. So, if you have a function that performs something, at the end just put something like the following: public String someFunction() { .... (the code) RedirectTrackerUtils.trackRedirect(FacesContext.getCurrentInstance()); return "success"; } 'trackRedirect' would just be something like public static void trackRedirect(FacesContext context) { context.getExternalContext().getRequestMap().put("redirectTrackerEnabled", Boolean.TRUE); } This of course clutters the code with UI specifc issues, but I think its workable from a purely functional perspective. Thoughts? Also, I think it might be a good idea to use an id number that's a little more obfuscated. Having a low numberd sequential id means somebody is going to start messing around with it manually.
          Hide
          Kevin Galligan added a comment -

          Here's a patch that has changes I posted the comment about. A little util class that puts and gets to the request map. Little changes to 'RedirectTrackerManager' and 'RedirectTrackerExternalContextWrapper'.

          To use it, call either

          RedirectTrackerUtils.trackMessages(FacesContext)

          or...

          RedirectTrackerUtils.trackMessagesAndBeans(FacesContext)

          The patch was created with TortoiseSVN. The root repository path of the patch is...

          http://svn.apache.org/repos/asf/myfaces/tomahawk/trunk/sandbox

          Show
          Kevin Galligan added a comment - Here's a patch that has changes I posted the comment about. A little util class that puts and gets to the request map. Little changes to 'RedirectTrackerManager' and 'RedirectTrackerExternalContextWrapper'. To use it, call either RedirectTrackerUtils.trackMessages(FacesContext) or... RedirectTrackerUtils.trackMessagesAndBeans(FacesContext) The patch was created with TortoiseSVN. The root repository path of the patch is... http://svn.apache.org/repos/asf/myfaces/tomahawk/trunk/sandbox
          Hide
          Kevin Galligan added a comment -

          Includes my last patch, plus the key used to track the state is a little more obscured. Ideally it would be like a hash of some kind, but I just took the request number and appended a dash and the current timestamp. So, its fairly unlikely that a user could just type in a different number.

          Show
          Kevin Galligan added a comment - Includes my last patch, plus the key used to track the state is a little more obscured. Ideally it would be like a hash of some kind, but I just took the request number and appended a dash and the current timestamp. So, its fairly unlikely that a user could just type in a different number.
          Hide
          Mario Ivankovits added a comment -

          Hi!

          I added a policy configuration to customize the data being saved between redirects

          New context configuration parameters:

          org.apache.myfaces.redirectTracker.MAX_REDIRECTS - Default: 20
          Number of redirects to track - required to allow the back-button to work.

          org.apache.myfaces.redirectTracker.POLICY - Default: org.apache.myfaces.custom.redirectTracker.policy.NoopRedirectTrackPolicy

          The policy used to describe which data to save between requests. This can be any FQN to a class implementing the RedirectTrackerPolicy.

          Current implementations are:

          • org.apache.myfaces.custom.redirectTracker.policy.FullRedirectTrackPolicy
            Captures request beans, local and messages
          • org.apache.myfaces.custom.redirectTracker.policy.MessagesRedirectTrackPolicy
            Captures FacesMessages only
          • org.apache.myfaces.custom.redirectTracker.policy.NoopRedirectTrackPolicy
            Captures nothing (the default for JSF backward compatiblity)
          Show
          Mario Ivankovits added a comment - Hi! I added a policy configuration to customize the data being saved between redirects New context configuration parameters: org.apache.myfaces.redirectTracker.MAX_REDIRECTS - Default: 20 Number of redirects to track - required to allow the back-button to work. org.apache.myfaces.redirectTracker.POLICY - Default: org.apache.myfaces.custom.redirectTracker.policy.NoopRedirectTrackPolicy The policy used to describe which data to save between requests. This can be any FQN to a class implementing the RedirectTrackerPolicy. Current implementations are: org.apache.myfaces.custom.redirectTracker.policy.FullRedirectTrackPolicy Captures request beans, local and messages org.apache.myfaces.custom.redirectTracker.policy.MessagesRedirectTrackPolicy Captures FacesMessages only org.apache.myfaces.custom.redirectTracker.policy.NoopRedirectTrackPolicy Captures nothing (the default for JSF backward compatiblity)
          Hide
          Kevin Galligan added a comment -

          Took a look at this. The config is on the entire app, correct? In the time that I've used it, I've found the ability to selectively apply redirect tracking to be quite useful. Genernally, no tracking. Sometimes message only, occasionally all.

          Show
          Kevin Galligan added a comment - Took a look at this. The config is on the entire app, correct? In the time that I've used it, I've found the ability to selectively apply redirect tracking to be quite useful. Genernally, no tracking. Sometimes message only, occasionally all.
          Hide
          Mario Ivankovits added a comment -

          Yes, for the entire app as it is a context parameter, though, you can create whatever policy you want to, even one which can simulate the "manual redirect tracking" behaviour, I'll help you out to build such a policy if required, maybe, when I find some time I'll add such a policy to the supported ones.

          Show
          Mario Ivankovits added a comment - Yes, for the entire app as it is a context parameter, though, you can create whatever policy you want to, even one which can simulate the "manual redirect tracking" behaviour, I'll help you out to build such a policy if required, maybe, when I find some time I'll add such a policy to the supported ones.
          Hide
          Kevin Galligan added a comment -

          Ok. Added a new policy that does the manual thing. Lightly tested!!!

          If you want to include it in the standard sandbox code, feel free to cut and paste.

          In the web.xml file...

          <context-param>
          <param-name>org.apache.myfaces.redirectTracker.POLICY</param-name>
          <param-value>
          com.kgalligan.commons.myfaces.sandbox.ManualRedirectTrackerPolicy
          </param-value>
          </context-param>

          Show
          Kevin Galligan added a comment - Ok. Added a new policy that does the manual thing. Lightly tested !!! If you want to include it in the standard sandbox code, feel free to cut and paste. In the web.xml file... <context-param> <param-name>org.apache.myfaces.redirectTracker.POLICY</param-name> <param-value> com.kgalligan.commons.myfaces.sandbox.ManualRedirectTrackerPolicy </param-value> </context-param>
          Hide
          Kevin Galligan added a comment -

          PS. The two patch attachments I put up should be removed as they're old. Not sure how to do that.

          Show
          Kevin Galligan added a comment - PS. The two patch attachments I put up should be removed as they're old. Not sure how to do that.
          Hide
          Kevin Galligan added a comment -

          PPS. The policy I added, mentioned above, is in the zip file called 'myfaces-extension.zip' attached to this bug. Not sure that that was clear.

          As stated in a much earlier comment, you use it by calling into the utility methods...

          RedirectTrackerUtils.trackMessages(FacesContext)

          or...

          RedirectTrackerUtils.trackMessagesAndBeans(FacesContext)

          I wasn't sure what to do with 'redirectContext.saveLocale()', so it calls this if either 'trackMessages' or 'trackMessagesAndBeans' is called.

          Show
          Kevin Galligan added a comment - PPS. The policy I added, mentioned above, is in the zip file called 'myfaces-extension.zip' attached to this bug. Not sure that that was clear. As stated in a much earlier comment, you use it by calling into the utility methods... RedirectTrackerUtils.trackMessages(FacesContext) or... RedirectTrackerUtils.trackMessagesAndBeans(FacesContext) I wasn't sure what to do with 'redirectContext.saveLocale()', so it calls this if either 'trackMessages' or 'trackMessagesAndBeans' is called.

            People

            • Assignee:
              Mario Ivankovits
              Reporter:
              Mario Ivankovits
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development