You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by do...@gmail.com on 2009/01/30 15:49:50 UTC
Trailing "/" in generated urls considered redundant/harmful
Is it OK (ie "by design" as opposed to "by mistake") that the urls
generated for the mounted pages end up with the "/"?
Provided that there's a page that expects single parameter
(here: "content")...
public class HelpPage extends WebPage {
public HelpPage(PageParameters p) {
super(p);
add(new DynamicContentPanel("contentPanel", new
Model<String>(p.getString("content"))));
}
}
...and it is mounted in the Application#init()
mount(new BookmarkablePageRequestTargetUrlCodingStrategy("help",
HelpPage.class, null));
...and further referred to somewhere else as:
add(new BookmarkablePageLink("helpPage", HelpPage.class, new
PageParameters("content=a")));
the url in the generated markup is in the following form:
http://localhost:8080/dummy-web/help/content/a/;jsessionid=11624C6125F8DF4867E3218676D79A29
While IMHO it should read:
http://localhost:8080/dummy-web/help/content/a;jsessionid=11624C6125F8DF4867E3218676D79A29
The page parameter for both cases is resolved correctly by the HelpPage's
constructor, so it seems that even though there's an extra "/" at the end
of the url it gets omitted.
Then why bother generating it?
I looked up in the sources and found that it is the
AbstractRequestTargetUrlCodingStrategy#appendValue(AppendingStringBuffer
url, String key, String value) that is responsible for adding an extra
trailing "/" at the end of the generated url
It is invoked in the loop, and possibly there's no corner case implemented
for the last parameter to be encoded.
/regz,
Dominik Drzewiecki
Re: Trailing "/" in generated urls considered redundant/harmful
Posted by Dominik Drzewiecki <do...@gmail.com>.
It seems that the fix is as easy as applying the following patch.
Index: src/main/java/org/apache/wicket/request/target/coding/AbstractRequestTargetUrlCodingStrategy.java
===================================================================
--- src/main/java/org/apache/wicket/request/target/coding/AbstractRequestTargetUrlCodingStrategy.java (revision
739413)
+++ src/main/java/org/apache/wicket/request/target/coding/AbstractRequestTargetUrlCodingStrategy.java (working
copy)
@@ -117,7 +119,7 @@
{
url.append("/");
}
- url.append(key).append("/").append(escapedValue).append("/");
+ url.append(key).append("/").append(escapedValue);
}
}
Applying this however results in several test failures:
Failed tests:
testStatelessDefaultUrlCodingStrategy(org.apache.wicket.request.target.coding.StatelessStatefullUrlCodingStrategyTest)
testStatefullDefaultUrlCodingStrategy(org.apache.wicket.request.target.coding.StatelessStatefullUrlCodingStrategyTest)
testStatelessHybridUrlCodingStrategy(org.apache.wicket.request.target.coding.StatelessStatefullUrlCodingStrategyTest)
testStatefullHybridUrlCodingStrategy(org.apache.wicket.request.target.coding.StatelessStatefullUrlCodingStrategyTest)
testStatelessComponentPageWithMount(org.apache.wicket.stateless.StatelessComponentTest)
testStatelessComponentPageWithParamsWithMount(org.apache.wicket.stateless.StatelessComponentTest)
But in fact those tests should pass.
For the first 4, the generated markup is semantically identical (but
it's textual representation differs) to the expected one.
The last 2 tests fail, but removing trailing slashes from the
generated urls in the expected markup does the trick.
regz,
/dd
/dd
2009/1/30 Igor Vaynberg <ig...@gmail.com>:
> can you open a jira issue please...feel free to attach any tests you have.
>
> -igor
>
> On Fri, Jan 30, 2009 at 12:33 PM, Dominik Drzewiecki
> <do...@gmail.com> wrote:
>> Yep, the problem persists for the wicket built from trunk.
>>
>> /dd
>>
>> 2009/1/30 Igor Vaynberg <ig...@gmail.com>:
>>> what wicket version are you using and have you tried with latest from svn?
>>>
>>> -igor
>>>
>>> On Fri, Jan 30, 2009 at 6:49 AM, <do...@gmail.com> wrote:
>>>> Is it OK (ie "by design" as opposed to "by mistake") that the urls generated
>>>> for the mounted pages end up with the "/"?
>>>>
>>>> Provided that there's a page that expects single parameter (here:
>>>> "content")...
>>>> public class HelpPage extends WebPage {
>>>> public HelpPage(PageParameters p) {
>>>> super(p);
>>>> add(new DynamicContentPanel("contentPanel", new
>>>> Model<String>(p.getString("content"))));
>>>> }
>>>> }
>>>>
>>>> ...and it is mounted in the Application#init()
>>>> mount(new BookmarkablePageRequestTargetUrlCodingStrategy("help",
>>>> HelpPage.class, null));
>>>>
>>>> ...and further referred to somewhere else as:
>>>> add(new BookmarkablePageLink("helpPage", HelpPage.class, new
>>>> PageParameters("content=a")));
>>>>
>>>> the url in the generated markup is in the following form:
>>>> http://localhost:8080/dummy-web/help/content/a/;jsessionid=11624C6125F8DF4867E3218676D79A29
>>>>
>>>> While IMHO it should read:
>>>> http://localhost:8080/dummy-web/help/content/a;jsessionid=11624C6125F8DF4867E3218676D79A29
>>>>
>>>> The page parameter for both cases is resolved correctly by the HelpPage's
>>>> constructor, so it seems that even though there's an extra "/" at the end of
>>>> the url it gets omitted.
>>>> Then why bother generating it?
>>>>
>>>> I looked up in the sources and found that it is the
>>>> AbstractRequestTargetUrlCodingStrategy#appendValue(AppendingStringBuffer
>>>> url, String key, String value) that is responsible for adding an extra
>>>> trailing "/" at the end of the generated url
>>>> It is invoked in the loop, and possibly there's no corner case implemented
>>>> for the last parameter to be encoded.
>>>>
>>>> /regz,
>>>> Dominik Drzewiecki
>>>>
>>>
>>
>
Re: Trailing "/" in generated urls considered redundant/harmful
Posted by Dominik Drzewiecki <do...@gmail.com>.
I further investigated the reason *why* there was an extra "/" at the
end and stumbled upon an issue
https://issues.apache.org/jira/browse/WICKET-765.
Apart from the compatibility with wicket 1.2 I see no rationale for
trailing "/". There are even more Strategies that need to be changed.
Looking at them I came to the conclusion that the "append("/")" is
being overused and redundant especially when it is preceded by the
following code which makes sure that the "/" is in place before adding
another parameter name-value pair:
if (!url.endsWith("/"))
{
url.append("/");
}
I opened a jira issue on that and attached proposed patch addressing
all (hopefully) possible places in code that generate extra trailing
"/" in the urls. See:
https://issues.apache.org/jira/browse/WICKET-2065
regz,
/dd
2009/1/30 Igor Vaynberg <ig...@gmail.com>:
> can you open a jira issue please...feel free to attach any tests you have.
>
> -igor
>
> On Fri, Jan 30, 2009 at 12:33 PM, Dominik Drzewiecki
> <do...@gmail.com> wrote:
>> Yep, the problem persists for the wicket built from trunk.
>>
>> /dd
>>
>> 2009/1/30 Igor Vaynberg <ig...@gmail.com>:
>>> what wicket version are you using and have you tried with latest from svn?
>>>
>>> -igor
>>>
>>> On Fri, Jan 30, 2009 at 6:49 AM, <do...@gmail.com> wrote:
>>>> Is it OK (ie "by design" as opposed to "by mistake") that the urls generated
>>>> for the mounted pages end up with the "/"?
>>>>
>>>> Provided that there's a page that expects single parameter (here:
>>>> "content")...
>>>> public class HelpPage extends WebPage {
>>>> public HelpPage(PageParameters p) {
>>>> super(p);
>>>> add(new DynamicContentPanel("contentPanel", new
>>>> Model<String>(p.getString("content"))));
>>>> }
>>>> }
>>>>
>>>> ...and it is mounted in the Application#init()
>>>> mount(new BookmarkablePageRequestTargetUrlCodingStrategy("help",
>>>> HelpPage.class, null));
>>>>
>>>> ...and further referred to somewhere else as:
>>>> add(new BookmarkablePageLink("helpPage", HelpPage.class, new
>>>> PageParameters("content=a")));
>>>>
>>>> the url in the generated markup is in the following form:
>>>> http://localhost:8080/dummy-web/help/content/a/;jsessionid=11624C6125F8DF4867E3218676D79A29
>>>>
>>>> While IMHO it should read:
>>>> http://localhost:8080/dummy-web/help/content/a;jsessionid=11624C6125F8DF4867E3218676D79A29
>>>>
>>>> The page parameter for both cases is resolved correctly by the HelpPage's
>>>> constructor, so it seems that even though there's an extra "/" at the end of
>>>> the url it gets omitted.
>>>> Then why bother generating it?
>>>>
>>>> I looked up in the sources and found that it is the
>>>> AbstractRequestTargetUrlCodingStrategy#appendValue(AppendingStringBuffer
>>>> url, String key, String value) that is responsible for adding an extra
>>>> trailing "/" at the end of the generated url
>>>> It is invoked in the loop, and possibly there's no corner case implemented
>>>> for the last parameter to be encoded.
>>>>
>>>> /regz,
>>>> Dominik Drzewiecki
>>>>
>>>
>>
>
Re: Trailing "/" in generated urls considered redundant/harmful
Posted by Igor Vaynberg <ig...@gmail.com>.
can you open a jira issue please...feel free to attach any tests you have.
-igor
On Fri, Jan 30, 2009 at 12:33 PM, Dominik Drzewiecki
<do...@gmail.com> wrote:
> Yep, the problem persists for the wicket built from trunk.
>
> /dd
>
> 2009/1/30 Igor Vaynberg <ig...@gmail.com>:
>> what wicket version are you using and have you tried with latest from svn?
>>
>> -igor
>>
>> On Fri, Jan 30, 2009 at 6:49 AM, <do...@gmail.com> wrote:
>>> Is it OK (ie "by design" as opposed to "by mistake") that the urls generated
>>> for the mounted pages end up with the "/"?
>>>
>>> Provided that there's a page that expects single parameter (here:
>>> "content")...
>>> public class HelpPage extends WebPage {
>>> public HelpPage(PageParameters p) {
>>> super(p);
>>> add(new DynamicContentPanel("contentPanel", new
>>> Model<String>(p.getString("content"))));
>>> }
>>> }
>>>
>>> ...and it is mounted in the Application#init()
>>> mount(new BookmarkablePageRequestTargetUrlCodingStrategy("help",
>>> HelpPage.class, null));
>>>
>>> ...and further referred to somewhere else as:
>>> add(new BookmarkablePageLink("helpPage", HelpPage.class, new
>>> PageParameters("content=a")));
>>>
>>> the url in the generated markup is in the following form:
>>> http://localhost:8080/dummy-web/help/content/a/;jsessionid=11624C6125F8DF4867E3218676D79A29
>>>
>>> While IMHO it should read:
>>> http://localhost:8080/dummy-web/help/content/a;jsessionid=11624C6125F8DF4867E3218676D79A29
>>>
>>> The page parameter for both cases is resolved correctly by the HelpPage's
>>> constructor, so it seems that even though there's an extra "/" at the end of
>>> the url it gets omitted.
>>> Then why bother generating it?
>>>
>>> I looked up in the sources and found that it is the
>>> AbstractRequestTargetUrlCodingStrategy#appendValue(AppendingStringBuffer
>>> url, String key, String value) that is responsible for adding an extra
>>> trailing "/" at the end of the generated url
>>> It is invoked in the loop, and possibly there's no corner case implemented
>>> for the last parameter to be encoded.
>>>
>>> /regz,
>>> Dominik Drzewiecki
>>>
>>
>
Re: Trailing "/" in generated urls considered redundant/harmful
Posted by Dominik Drzewiecki <do...@gmail.com>.
Yep, the problem persists for the wicket built from trunk.
/dd
2009/1/30 Igor Vaynberg <ig...@gmail.com>:
> what wicket version are you using and have you tried with latest from svn?
>
> -igor
>
> On Fri, Jan 30, 2009 at 6:49 AM, <do...@gmail.com> wrote:
>> Is it OK (ie "by design" as opposed to "by mistake") that the urls generated
>> for the mounted pages end up with the "/"?
>>
>> Provided that there's a page that expects single parameter (here:
>> "content")...
>> public class HelpPage extends WebPage {
>> public HelpPage(PageParameters p) {
>> super(p);
>> add(new DynamicContentPanel("contentPanel", new
>> Model<String>(p.getString("content"))));
>> }
>> }
>>
>> ...and it is mounted in the Application#init()
>> mount(new BookmarkablePageRequestTargetUrlCodingStrategy("help",
>> HelpPage.class, null));
>>
>> ...and further referred to somewhere else as:
>> add(new BookmarkablePageLink("helpPage", HelpPage.class, new
>> PageParameters("content=a")));
>>
>> the url in the generated markup is in the following form:
>> http://localhost:8080/dummy-web/help/content/a/;jsessionid=11624C6125F8DF4867E3218676D79A29
>>
>> While IMHO it should read:
>> http://localhost:8080/dummy-web/help/content/a;jsessionid=11624C6125F8DF4867E3218676D79A29
>>
>> The page parameter for both cases is resolved correctly by the HelpPage's
>> constructor, so it seems that even though there's an extra "/" at the end of
>> the url it gets omitted.
>> Then why bother generating it?
>>
>> I looked up in the sources and found that it is the
>> AbstractRequestTargetUrlCodingStrategy#appendValue(AppendingStringBuffer
>> url, String key, String value) that is responsible for adding an extra
>> trailing "/" at the end of the generated url
>> It is invoked in the loop, and possibly there's no corner case implemented
>> for the last parameter to be encoded.
>>
>> /regz,
>> Dominik Drzewiecki
>>
>
Re: Trailing "/" in generated urls considered redundant/harmful
Posted by Dominik Drzewiecki <do...@gmail.com>.
I've built wicket from the trunk about Tuesday, can't remember which
particular revision it was (Can't check it either now as it was at my
office and I'm at home now, but AFAIR it was just after all issues for
RC2 were resolved and the trunk was declared stable). I've just
checked out from trunk and it seems that
AbstractRequestTargetUrlCodingStrategy#appendValue(AppendingStringBuffer
url, String key, String value) hasn't changed a bit. I'm setting up
the test case against the trunk and will update you soon.
/dd
2009/1/30 Igor Vaynberg <ig...@gmail.com>:
> what wicket version are you using and have you tried with latest from svn?
>
> -igor
>
> On Fri, Jan 30, 2009 at 6:49 AM, <do...@gmail.com> wrote:
>> Is it OK (ie "by design" as opposed to "by mistake") that the urls generated
>> for the mounted pages end up with the "/"?
>>
>> Provided that there's a page that expects single parameter (here:
>> "content")...
>> public class HelpPage extends WebPage {
>> public HelpPage(PageParameters p) {
>> super(p);
>> add(new DynamicContentPanel("contentPanel", new
>> Model<String>(p.getString("content"))));
>> }
>> }
>>
>> ...and it is mounted in the Application#init()
>> mount(new BookmarkablePageRequestTargetUrlCodingStrategy("help",
>> HelpPage.class, null));
>>
>> ...and further referred to somewhere else as:
>> add(new BookmarkablePageLink("helpPage", HelpPage.class, new
>> PageParameters("content=a")));
>>
>> the url in the generated markup is in the following form:
>> http://localhost:8080/dummy-web/help/content/a/;jsessionid=11624C6125F8DF4867E3218676D79A29
>>
>> While IMHO it should read:
>> http://localhost:8080/dummy-web/help/content/a;jsessionid=11624C6125F8DF4867E3218676D79A29
>>
>> The page parameter for both cases is resolved correctly by the HelpPage's
>> constructor, so it seems that even though there's an extra "/" at the end of
>> the url it gets omitted.
>> Then why bother generating it?
>>
>> I looked up in the sources and found that it is the
>> AbstractRequestTargetUrlCodingStrategy#appendValue(AppendingStringBuffer
>> url, String key, String value) that is responsible for adding an extra
>> trailing "/" at the end of the generated url
>> It is invoked in the loop, and possibly there's no corner case implemented
>> for the last parameter to be encoded.
>>
>> /regz,
>> Dominik Drzewiecki
>>
>
Re: Trailing "/" in generated urls considered redundant/harmful
Posted by Igor Vaynberg <ig...@gmail.com>.
what wicket version are you using and have you tried with latest from svn?
-igor
On Fri, Jan 30, 2009 at 6:49 AM, <do...@gmail.com> wrote:
> Is it OK (ie "by design" as opposed to "by mistake") that the urls generated
> for the mounted pages end up with the "/"?
>
> Provided that there's a page that expects single parameter (here:
> "content")...
> public class HelpPage extends WebPage {
> public HelpPage(PageParameters p) {
> super(p);
> add(new DynamicContentPanel("contentPanel", new
> Model<String>(p.getString("content"))));
> }
> }
>
> ...and it is mounted in the Application#init()
> mount(new BookmarkablePageRequestTargetUrlCodingStrategy("help",
> HelpPage.class, null));
>
> ...and further referred to somewhere else as:
> add(new BookmarkablePageLink("helpPage", HelpPage.class, new
> PageParameters("content=a")));
>
> the url in the generated markup is in the following form:
> http://localhost:8080/dummy-web/help/content/a/;jsessionid=11624C6125F8DF4867E3218676D79A29
>
> While IMHO it should read:
> http://localhost:8080/dummy-web/help/content/a;jsessionid=11624C6125F8DF4867E3218676D79A29
>
> The page parameter for both cases is resolved correctly by the HelpPage's
> constructor, so it seems that even though there's an extra "/" at the end of
> the url it gets omitted.
> Then why bother generating it?
>
> I looked up in the sources and found that it is the
> AbstractRequestTargetUrlCodingStrategy#appendValue(AppendingStringBuffer
> url, String key, String value) that is responsible for adding an extra
> trailing "/" at the end of the generated url
> It is invoked in the loop, and possibly there's no corner case implemented
> for the last parameter to be encoded.
>
> /regz,
> Dominik Drzewiecki
>