Hi Emond and Martijn, Thanks for your great work. This is really tricky time-consuming stuff, and I am glad that you follow this up all the way. I am not a Wicket framework specialist and don't have as much inside knowledge as I would like, so please excuse my ignorance in some cases.
On Wed, 21 Aug 2013 21:10:37 +0200, you wrote: >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. I agree. This needs time and refinement, and while you are at it, perhaps some other issues should be addressed with it to get a better return for the changes. >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. But this is not a problem. From my limited perspective, I was only trying to address the survivability of a page after expiry. So we can only gain never lose. I am fully aware that some if not all state can be lost if the page is re-created using a link URL that is created with a different urlFor(). That is until we start discussing client state. Often all is required is that the page is created without state, and obviously a bookmarkable constructor must exist because of the isBookmarkable() switch. This bookmarkable constructor should guarantee that a page is valid otherwise what is the point of coding it and what is the point of defining what bookmarkable is. >However, looking at it again, I'm >starting to think we should at least to try improve the situation for >Wicket 6. Great. I wasn't really expecting that. > >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. This is also my understanding. This is a sound concept, and it should be one of our guidelines. > >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 :) Obviously, this is very tricky, and to understand this, I simplify this a little to not get confused. I am considering PageParameters client state that gets passed between pages via links and requests. Although they might not be strictly needed if the page becomes stateful. But we need them so that people can bookmark the page in the browser, and for recovery. So if there is for example ?orderId=123&lineItemId=5 then I would expect that these parameters never cease to exist. Your comments about the complexity remind me of WICKET-4441: "PageProvider should create a new Page instance if PageParameters are changed, even if a stored page exists." So what you are raising is the fact that client state competes with server state. We would need a policy for that. That policy would perhaps help to create a bridge to some viable client state. Who is the genious to solve this? I don't have any ideas yet. I just hoped that we get incremental improvements by means of clarity and consistency. Therefore I thought that WICKET-5068 could be the next step. Kind Regards, Bernard > >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. >> >> */ >> >> >> >> >> >>
