Hi,
On Wed, Aug 21, 2013 at 10:10 PM, Emond Papegaaij <[email protected] > 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. 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 > PageParameters has a confusing name, e.g. PageParameters are passed to IResource#respond(Attributes#getPageParameters). Here is how I understand the lifecycle - when a page is instantiated Wicket may pass PageParameters to its constructor. Those PageParameters have the same lifecycle as the page itself. They are reachable via Page#getPageParameters. When a link is clicked its parameters are reachable in #onClick via getRequest.getRequestParameters. Using getPage.getPageParameters will return the parameters for the page creation, not for the link's click, or form submit. > the super contructor, but those changes will be overridden by Wicket after > I don't see what you mean here. > constructing the page. Finally, you can override getPageParameters in your > Yes. Overriding this method may return wrong data later. See https://cwiki.apache.org/confluence/display/WICKET/Wicket+7.0+Roadmap#Wicket7.0Roadmap-PageParametersimprovements - here I suggest to make the Page's parameters immutable. I like this change A LOT, but as you can see in the patch in https://issues.apache.org/jira/browse/WICKET-4774 it requires too many API breaks :-/ > 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 > What do you mean by this ? The data coming from the client is not typed, so using String is the most natural choice. You can use StringValue#toXyz() to convert it to other primitives. > we can borrow some ideas from JAX-RS? On the other hand, they don't have to > deal with annoying things like state :) > I don't quite like the repetitive approach in JAX-RS - @ParamValue("number") int number -, note that there is "number" twice, but if you like it then you can create a new IPageFactory impl that does this for you. > > 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. > > >> */ > > >> > > >> > > > > >
