Martijn and I discussed some more about this, and we've came to the conclusion that we probably should not try to fix this in Wicket 6. The cause of this issue (and several others) is a mismatch between Page, PageParameters and statefulness that cannot be fixed without breaking the API in several ways. The reason Component.urlFor checks isPageStateless is that a stateful page cannot be recreated from a bookmarkable url. If you do, you will loose (some of) its state. However, looking at it again, I'm starting to think we should at least to try improve the situation for Wicket 6.
At the moment, there is a discrepancy between mounted and bookmarkable pages. Consider MyPage, a bookmarkable but stateful page with a Link. The href for this link will be something like /wicket/page?2-1.ILinkListener-link. Now, if I mount this page under /mypage, the href for the link will suddenly become /mypage?2-1.ILinkListener-link. I don't see why a mounted stateful page can and should be treated differently than a bookmarkable stateful page. IMHO, they are 2 forms of the same pattern. Therefore, I've just pushed an update to the wicket-4997 that tries to improve the situation by using bookmarkable urls for stateless pages (as it was), but also for bookmarkable pages that were created bookmarkable. This seems to give much better results in the testcases. I would really appreciate it if some more people could take a look at this, because url rendering is quite a sensitive subject. For Wicket 7, we might want to take a look at the PageParameters and mounts because they hold several caveats. The most important is that the lifecycle of PageParameters if different than that of the Page they belong to. PageParameters are bound to a request, whereas a page instance can live for many requests. Also, it is possible to modify the PageParameters passed to the super contructor, but those changes will be overridden by Wicket after constructing the page. Finally, you can override getPageParameters in your page, which will confuse Wicket even more. Also, PageParameters only handle strings, which is quite limited. I don't know how to improve this. Perhaps we can borrow some ideas from JAX-RS? On the other hand, they don't have to deal with annoying things like state :) Best regards, Emond On Wed, Aug 21, 2013 at 8:54 AM, Bernard <[email protected]> wrote: > Hi, > > I think that WebResponseExceptionsTest#expirePage must fail > because clicking the link should re-create the page because > TestExpirePage is bookmarkable. > > I have made different changes to Wicket which I am not so sure about > but I am ready to share, and in order to get this test to pass, I had > to use a non-bookmarkable TestExpirePage: > > public class TestExpirePage extends WebPage > { > private static final long serialVersionUID = 1L; > > private int state; > > /** > * Construct. > */ > public TestExpirePage(int state) > { > this.state = state; > ... > then in the test: > tester.startPage(new TestExpirePage(1)); > > and this test must succeed. > > More importantly we still need an additional test that positively > verifies that a bookmarkable page does NOT expire with > PageExpiredException but with the same page re-created after session > expiry. > > Kind Regards > > Bernard > > > On Mon, 19 Aug 2013 17:32:45 +0300, you wrote: > > >Hi Emond, > > > > > >On Mon, Aug 19, 2013 at 12:10 PM, <[email protected]> wrote: > > > >> WICKET-4997: render bookmarkable urls for bookmarkable pages (not > >> stateless) > >> > >> > >> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo > >> Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/e99bf147 > >> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/e99bf147 > >> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/e99bf147 > >> > >> Branch: refs/heads/wicket-4997 > >> Commit: e99bf1476403ff13e409ba28331a8291a03ca25f > >> Parents: 7d8313f > >> Author: Emond Papegaaij <[email protected]> > >> Authored: Mon Aug 19 11:07:41 2013 +0200 > >> Committer: Emond Papegaaij <[email protected]> > >> Committed: Mon Aug 19 11:10:02 2013 +0200 > >> > >> ---------------------------------------------------------------------- > >> .../main/java/org/apache/wicket/Component.java | 4 +- > >> .../mapper/AbstractBookmarkableMapper.java | 13 +++++-- > >> .../core/request/mapper/MountedMapper.java | 41 > ++++++++------------ > >> 3 files changed, 29 insertions(+), 29 deletions(-) > >> ---------------------------------------------------------------------- > >> > >> > >> > >> > http://git-wip-us.apache.org/repos/asf/wicket/blob/e99bf147/wicket-core/src/main/java/org/apache/wicket/Component.java > >> ---------------------------------------------------------------------- > >> diff --git a/wicket-core/src/main/java/org/apache/wicket/Component.java > >> b/wicket-core/src/main/java/org/apache/wicket/Component.java > >> index 8fa63a3..116eb86 100644 > >> --- a/wicket-core/src/main/java/org/apache/wicket/Component.java > >> +++ b/wicket-core/src/main/java/org/apache/wicket/Component.java > >> @@ -3336,7 +3336,7 @@ public abstract class Component > >> Page page = getPage(); > >> PageAndComponentProvider provider = new > >> PageAndComponentProvider(page, this, parameters); > >> IRequestHandler handler; > >> - if (page.isPageStateless()) > >> + if (page.isBookmarkable()) > >> { > >> handler = new > >> BookmarkableListenerInterfaceRequestHandler(provider, listener, id); > >> } > >> @@ -3379,7 +3379,7 @@ public abstract class Component > >> Page page = getPage(); > >> PageAndComponentProvider provider = new > >> PageAndComponentProvider(page, this, parameters); > >> IRequestHandler handler; > >> - if (page.isPageStateless()) > >> + if (page.isBookmarkable()) > >> > > > >I think this change is OK. > >Maybe we can improve it a bit by using > Application.get().getPageSettings(). > >getRecreateMountedPagesAfterExpiry() in the checks above ? > > > >With the new check as you can see the produced urls contain the class > name. > >Some users don't like this. > >Application.get().getPageSettings().getRecreateMountedPagesAfterExpiry() > >return true by default. If someone doesn't like the extra info in the > >produced urls then she can disable it this way. > > > > > >> { > >> handler = new > >> BookmarkableListenerInterfaceRequestHandler(provider, listener); > >> } > >> > >> > >> > http://git-wip-us.apache.org/repos/asf/wicket/blob/e99bf147/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/AbstractBookmarkableMapper.java > >> ---------------------------------------------------------------------- > >> diff --git > >> > a/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/AbstractBookmarkableMapper.java > >> > b/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/AbstractBookmarkableMapper.java > >> index 93c22d2..bbe2e1c 100644 > >> --- > >> > a/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/AbstractBookmarkableMapper.java > >> +++ > >> > b/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/AbstractBookmarkableMapper.java > >> @@ -210,8 +210,7 @@ public abstract class AbstractBookmarkableMapper > >> extends AbstractComponentMapper > >> PageProvider provider = new > >> PageProvider(pageInfo.getPageId(), pageClass, pageParameters, > >> renderCount); > >> provider.setPageSource(getContext()); > >> - if (provider.isNewPageInstance() && > >> - > >> > !WebApplication.get().getPageSettings().getRecreateMountedPagesAfterExpiry()) > >> + if (provider.isNewPageInstance() && > >> !getRecreateMountedPagesAfterExpiry()) > >> { > >> throw new > >> PageExpiredException(String.format("Bookmarkable page id '%d' has > expired.", > >> pageInfo.getPageId())); > >> @@ -222,6 +221,11 @@ public abstract class AbstractBookmarkableMapper > >> extends AbstractComponentMapper > >> } > >> } > >> > >> + boolean getRecreateMountedPagesAfterExpiry() > >> + { > >> + return > >> > WebApplication.get().getPageSettings().getRecreateMountedPagesAfterExpiry(); > >> + } > >> + > >> /** > >> * Creates a {@code IRequestHandler} that processes a listener > >> request. > >> * > >> @@ -420,8 +424,11 @@ public abstract class AbstractBookmarkableMapper > >> extends AbstractComponentMapper > >> > >> requestListenerInterfaceToString(handler.getListenerInterface()), > >> handler.getComponentPath(), > >> handler.getBehaviorIndex()); > >> > >> + PageParameters parameters = > >> getRecreateMountedPagesAfterExpiry() ? new PageParameters( > >> + > >> > handler.getPage().getPageParameters()).mergeWith(handler.getPageParameters()) > >> + : handler.getPageParameters(); > >> UrlInfo urlInfo = new UrlInfo(new > >> PageComponentInfo(pageInfo, componentInfo), > >> - pageClass, handler.getPageParameters()); > >> + pageClass, parameters); > >> return buildUrl(urlInfo); > >> } > >> > >> > >> > >> > http://git-wip-us.apache.org/repos/asf/wicket/blob/e99bf147/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/MountedMapper.java > >> ---------------------------------------------------------------------- > >> diff --git > >> > a/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/MountedMapper.java > >> > b/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/MountedMapper.java > >> index 9b1db28..19212b6 100644 > >> --- > >> > a/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/MountedMapper.java > >> +++ > >> > b/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/MountedMapper.java > >> @@ -19,7 +19,6 @@ package org.apache.wicket.core.request.mapper; > >> import java.util.ArrayList; > >> import java.util.List; > >> > >> -import org.apache.wicket.Application; > >> import org.apache.wicket.RequestListenerInterface; > >> import > >> org.apache.wicket.core.request.handler.ListenerInterfaceRequestHandler; > >> import org.apache.wicket.request.IRequestHandler; > >> @@ -49,21 +48,21 @@ import org.apache.wicket.util.string.Strings; > >> * are matched before optional parameters, and optional parameters > eager > >> (from left to right). > >> * <p> > >> * Decodes and encodes the following URLs: > >> - * > >> + * > >> * <pre> > >> * Page Class - Render (BookmarkablePageRequestHandler for mounted > pages) > >> * /mount/point > >> * (these will redirect to hybrid alternative if page is not > stateless) > >> - * > >> + * > >> * IPage Instance - Render Hybrid (RenderPageRequestHandler for > mounted > >> pages) > >> * /mount/point?2 > >> - * > >> + * > >> * IPage Instance - Bookmarkable Listener > >> (BookmarkableListenerInterfaceRequestHandler for mounted pages) > >> * /mount/point?2-click-foo-bar-baz > >> * /mount/point?2-5.click.1-foo-bar-baz (1 is behavior index, 5 is > >> render count) > >> * (these will redirect to hybrid if page is not stateless) > >> * </pre> > >> - * > >> + * > >> * @author Matej Knopp > >> */ > >> public class MountedMapper extends AbstractBookmarkableMapper > >> @@ -143,7 +142,7 @@ public class MountedMapper extends > >> AbstractBookmarkableMapper > >> > >> /** > >> * Construct. > >> - * > >> + * > >> * @param mountPath > >> * @param pageClass > >> */ > >> @@ -154,32 +153,32 @@ public class MountedMapper extends > >> AbstractBookmarkableMapper > >> > >> /** > >> * Construct. > >> - * > >> + * > >> * @param mountPath > >> * @param pageClassProvider > >> */ > >> @Deprecated > >> public MountedMapper(String mountPath, > >> - ClassProvider<? extends IRequestablePage> > >> pageClassProvider) > >> + ClassProvider<? extends IRequestablePage> > >> pageClassProvider) > >> { > >> this(mountPath, new > >> ClassReference(pageClassProvider.get()), new PageParametersEncoder()); > >> } > >> > >> /** > >> * Construct. > >> - * > >> + * > >> * @param mountPath > >> * @param pageClassProvider > >> */ > >> public MountedMapper(String mountPath, > >> - IProvider<Class<? extends > IRequestablePage>> > >> pageClassProvider) > >> + IProvider<Class<? extends IRequestablePage>> > >> pageClassProvider) > >> { > >> this(mountPath, pageClassProvider, new > >> PageParametersEncoder()); > >> } > >> > >> /** > >> * Construct. > >> - * > >> + * > >> * @param mountPath > >> * @param pageClass > >> * @param pageParametersEncoder > >> @@ -192,7 +191,7 @@ public class MountedMapper extends > >> AbstractBookmarkableMapper > >> > >> /** > >> * Construct. > >> - * > >> + * > >> * @param mountPath > >> * @param pageClassProvider > >> * @param pageParametersEncoder > >> @@ -202,20 +201,19 @@ public class MountedMapper extends > >> AbstractBookmarkableMapper > >> ClassProvider<? extends IRequestablePage> > >> pageClassProvider, > >> IPageParametersEncoder pageParametersEncoder) > >> { > >> - this(mountPath, new > >> ClassReference(pageClassProvider.get()), > >> - pageParametersEncoder); > >> + this(mountPath, new > >> ClassReference(pageClassProvider.get()), pageParametersEncoder); > >> } > >> > >> /** > >> * Construct. > >> - * > >> + * > >> * @param mountPath > >> * @param pageClassProvider > >> * @param pageParametersEncoder > >> */ > >> public MountedMapper(String mountPath, > >> - IProvider<Class<? extends > IRequestablePage>> > >> pageClassProvider, > >> - IPageParametersEncoder > pageParametersEncoder) > >> + IProvider<Class<? extends IRequestablePage>> > >> pageClassProvider, > >> + IPageParametersEncoder pageParametersEncoder) > >> { > >> Args.notEmpty(mountPath, "mountPath"); > >> Args.notNull(pageClassProvider, "pageClassProvider"); > >> @@ -427,11 +425,6 @@ public class MountedMapper extends > >> AbstractBookmarkableMapper > >> return url; > >> } > >> > >> - boolean getRecreateMountedPagesAfterExpiry() > >> - { > >> - return > >> > Application.get().getPageSettings().getRecreateMountedPagesAfterExpiry(); > >> - } > >> - > >> /** > >> * @see > >> AbstractBookmarkableMapper#buildUrl(AbstractBookmarkableMapper.UrlInfo) > >> */ > >> @@ -483,7 +476,7 @@ public class MountedMapper extends > >> AbstractBookmarkableMapper > >> /** > >> * Check if the URL is for home page and the home page class > match > >> mounted class. If so, > >> * redirect to mounted URL. > >> - * > >> + * > >> * @param url > >> * @return request handler or <code>null</code> > >> */ > >> @@ -504,7 +497,7 @@ public class MountedMapper extends > >> AbstractBookmarkableMapper > >> * If this method returns <code>true</code> and application home > >> page class is same as the class > >> * mounted with this encoder, request to home page will result > in > >> a redirect to the mounted > >> * path. > >> - * > >> + * > >> * @return whether this encode should respond to home page > request > >> when home page class is same > >> * as mounted class. > >> */ > >> > >> > >
