Wicket
  1. Wicket
  2. WICKET-4997

Mounted bookmarkable Page not recreated on Session Expiry

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 6.4.0
    • Fix Version/s: 6.13.0, 7.0.0
    • Component/s: wicket
    • Labels:
      None
    • Environment:
      JDK 7, GlassFish 3.1.2.2

      Description

      With the default true of org.apache.wicket.settings.IPageSettings#getRecreateMountedPagesAfterExpiry() PageExpiryException is thrown when a Link on a page is clicked.

      I find it very useful and in fact indispensible to rely on RecreateMountedPagesAfterExpiry especially on logout links.

      But it doesn't seem to work for me in 6.4.0. I think this is because Link#getUrl() calls Component#utlFor() which requires a stateless page for this to work:

      if (page.isPageStateless())

      { handler = new BookmarkableListenerInterfaceRequestHandler(provider, listener); }

      else

      { handler = new ListenerInterfaceRequestHandler(provider, listener); }

      With a stateless page a url is:

      http://localhost:8080/wicket/HomePage?-1.ILinkListener-toolBar-signout

      With a non stateless but bookmarkable page a url is:

      http://localhost:8080/wicket/page?1-1.ILinkListener-toolBar-signout

      So I guess that a stateful page cannot be recovered because after session expiry Wicket cannot find out what the page is. It is not coded in the URL.

      Looking at the semantics of UrlFor(), I thought this might be a bug and I copied and changed the code in my Link subclass from

      // if (page.isPageStateless()) {
      to:
      if (page.isBookmarkable()) {

      It works as expected but I don't know whether it would break other things if implemented in Wicket.

      I guess I am not the only one who needs recovery for bookmarkable pages in this way. Would it be possible to change Wicket to fix this so it becomes possible?

      1. TestCase.zip
        12 kB
        bernard
      2. WICKET-4997-bernard-sources.zip
        44 kB
        bernard

        Issue Links

          Activity

          Hide
          bernard added a comment -

          Test case

          Show
          bernard added a comment - Test case
          Hide
          bernard added a comment -

          I changed Component#urlFor() in 6.9.2 with good success. This eliminates PageExpiredException for bookmarkable pages.

          There are many cosmetic test failures due to a different expectation of the target URL in the generated responses: Instead of "wicket/page?0-1.IBehaviorListener.0-test" we should expect a more specific value "wicket/bookmarkable/org.apache.wicket.ajax.DomReadyOrderPage?0-1.IBehaviorListener.0-test", or a mounted path.

          I have made real pages in a quickstart out of some test cases - they work fine. Test cases that expect PageExpiredException should no longer do so because of the improvement.

          We can expect a performance improvement because Page#isPageStateless(), called for every generated URL on the page is very expensive compared with Page#isBookmarkable().

          Show
          bernard added a comment - I changed Component#urlFor() in 6.9.2 with good success. This eliminates PageExpiredException for bookmarkable pages. There are many cosmetic test failures due to a different expectation of the target URL in the generated responses: Instead of "wicket/page?0-1.IBehaviorListener.0-test" we should expect a more specific value "wicket/bookmarkable/org.apache.wicket.ajax.DomReadyOrderPage?0-1.IBehaviorListener.0-test", or a mounted path. I have made real pages in a quickstart out of some test cases - they work fine. Test cases that expect PageExpiredException should no longer do so because of the improvement. We can expect a performance improvement because Page#isPageStateless(), called for every generated URL on the page is very expensive compared with Page#isBookmarkable().
          Hide
          Emond Papegaaij added a comment -

          I've pushed a branch (wicket-4997) against 6.x with a proposed fix for this issue. A problem I'm facing is that urls for mounted pages with required page parameters are rendered as bookmarkable urls (wicket/bookmarkable) when recreateMountedPagesAfterExpiry is disabled. This can be prevented by overriding isBookmarkable on the page (see PageWithLink), but I'm not sure if that's a good thing or not.

          Show
          Emond Papegaaij added a comment - I've pushed a branch (wicket-4997) against 6.x with a proposed fix for this issue. A problem I'm facing is that urls for mounted pages with required page parameters are rendered as bookmarkable urls (wicket/bookmarkable) when recreateMountedPagesAfterExpiry is disabled. This can be prevented by overriding isBookmarkable on the page (see PageWithLink), but I'm not sure if that's a good thing or not.
          Hide
          bernard added a comment -

          Thanks Emond.

          Yes, I agree with your concerns. Have you tried Page#wasCreatedBookmarkable instead? I have just discoverd this on line 384 in AbstractBookmarkableMapper#mapHandler

          if (checkPageInstance(page) &&
          (!pageMustHaveBeenCreatedBookmarkable() || page.wasCreatedBookmarkable()))
          {

          I was getting second thoughts about using Page#isBookmarkable() because it creates a bookmarkable URL for an unmounted page that was in fact created by a non-bookmarkable constructor i.e. with IModel parameter as provided with the test case. Sorry if I first suggested the wrong approach.

          Show
          bernard added a comment - Thanks Emond. Yes, I agree with your concerns. Have you tried Page#wasCreatedBookmarkable instead? I have just discoverd this on line 384 in AbstractBookmarkableMapper#mapHandler if (checkPageInstance(page) && (!pageMustHaveBeenCreatedBookmarkable() || page.wasCreatedBookmarkable())) { I was getting second thoughts about using Page#isBookmarkable() because it creates a bookmarkable URL for an unmounted page that was in fact created by a non-bookmarkable constructor i.e. with IModel parameter as provided with the test case. Sorry if I first suggested the wrong approach.
          Hide
          bernard added a comment -

          On the other hand expiry in the non-bookmarkable constructor case can be handled nicely in the constructor with a customised response so Page#isBookmarkable() is more flexible and less disrupting to the user. We just don't want PageExpiredException. That weakens the point in my last comment.

          In other words if the developer codes bookmarkable constructors then these constructors must be supported properly i.e. validated.

          This is the kind of scenario that makes me I think that Wicket should call a no-args page constructor instead of creating bogus PageParameters, if no PageParameters are provided.

          Also, regarding your concern, there is still an outstanding issue WICKET-5068. When we get the PageParameters back, then there is less motivation to prevent the page from being re-created.

          BTW this fixes WICKET-5043 which is essentially a duplicate of this issue.

          Show
          bernard added a comment - On the other hand expiry in the non-bookmarkable constructor case can be handled nicely in the constructor with a customised response so Page#isBookmarkable() is more flexible and less disrupting to the user. We just don't want PageExpiredException. That weakens the point in my last comment. In other words if the developer codes bookmarkable constructors then these constructors must be supported properly i.e. validated. This is the kind of scenario that makes me I think that Wicket should call a no-args page constructor instead of creating bogus PageParameters, if no PageParameters are provided. Also, regarding your concern, there is still an outstanding issue WICKET-5068 . When we get the PageParameters back, then there is less motivation to prevent the page from being re-created. BTW this fixes WICKET-5043 which is essentially a duplicate of this issue.
          Hide
          bernard added a comment -

          Some source files in a zip archive for consideration.

          I think that WebResponseExceptionsTest#expirePage must fail with a bookmarkable TestExpirePage because clicking the link should re-create the page. That is why I changed it to be non-bookmarkable.

          We still need a trivial additional test that positively verifies that a bookmarkable page does NOT expire with PageExpiredException but with the same page re-created after session expiry.

          Show
          bernard added a comment - Some source files in a zip archive for consideration. I think that WebResponseExceptionsTest#expirePage must fail with a bookmarkable TestExpirePage because clicking the link should re-create the page. That is why I changed it to be non-bookmarkable. We still need a trivial additional test that positively verifies that a bookmarkable page does NOT expire with PageExpiredException but with the same page re-created after session expiry.
          Hide
          Emond Papegaaij added a comment -

          Please provide code changes in unified diff format and in this case against the wicket-4997 branch. The easiest way to create a diff is using git diff.

          Show
          Emond Papegaaij added a comment - Please provide code changes in unified diff format and in this case against the wicket-4997 branch. The easiest way to create a diff is using git diff.
          Hide
          bernard added a comment -

          I can see a lot of remote branches but can't see that 4997 remote branch in my git client. I don't have much experience with git. What else can I do?

          Show
          bernard added a comment - I can see a lot of remote branches but can't see that 4997 remote branch in my git client. I don't have much experience with git. What else can I do?
          Hide
          Emond Papegaaij added a comment -

          The branch is there (you can also see it on the github mirror). Perhaps you need to fetch it.

          Show
          Emond Papegaaij added a comment - The branch is there (you can also see it on the github mirror). Perhaps you need to fetch it.
          Hide
          bernard added a comment -

          I have fetched the branch. What is the diff command please?

          Show
          bernard added a comment - I have fetched the branch. What is the diff command please?
          Hide
          bernard added a comment -

          Please see the attached 6 files. I changed only 4 main source files in addition to the obvious Component. MountedMapper#mapHandler now additionally handles BookmarkableListenerInterfaceRequestHandler via a new interface IListenerInterfaceRequestHandler which is implemented by BookmarkableListenerInterfaceRequestHandler and ListenerInterfaceRequestHandler.

          Wicket needs this to add the bookmarkable URL component to link URLs to avoid PageExpiredException for bookmarkable pages.

          Show
          bernard added a comment - Please see the attached 6 files. I changed only 4 main source files in addition to the obvious Component. MountedMapper#mapHandler now additionally handles BookmarkableListenerInterfaceRequestHandler via a new interface IListenerInterfaceRequestHandler which is implemented by BookmarkableListenerInterfaceRequestHandler and ListenerInterfaceRequestHandler. Wicket needs this to add the bookmarkable URL component to link URLs to avoid PageExpiredException for bookmarkable pages.
          Hide
          bernard added a comment -

          Hi Emond,

          Many thanks again. The results of your changes look good to me. The tests look fine, too. I may have overlooked a couple of things or looked at an older version.

          Show
          bernard added a comment - Hi Emond, Many thanks again. The results of your changes look good to me. The tests look fine, too. I may have overlooked a couple of things or looked at an older version.
          Hide
          Emond Papegaaij added a comment -

          We now use the following condition to render bookmarkable urls:
          getApplication().getPageSettings().getRecreateMountedPagesAfterExpiry() && ((page.isBookmarkable() && page.wasCreatedBookmarkable()) || page.isPageStateless())

          This results in bookmarkable urls for pages that are bookmarkable and were created bookmarkable. These pages can be recreated after session expiry.

          Show
          Emond Papegaaij added a comment - We now use the following condition to render bookmarkable urls: getApplication().getPageSettings().getRecreateMountedPagesAfterExpiry() && ((page.isBookmarkable() && page.wasCreatedBookmarkable()) || page.isPageStateless()) This results in bookmarkable urls for pages that are bookmarkable and were created bookmarkable. These pages can be recreated after session expiry.
          Hide
          bernard added a comment -

          Emond, my big thanks for that!

          Show
          bernard added a comment - Emond, my big thanks for that!

            People

            • Assignee:
              Emond Papegaaij
              Reporter:
              bernard
            • Votes:
              3 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development