Description
Suppose I have a package structure like this:
com.mycompany.myproject
— module1
------- page
--------- Page1.html
— module2
------- page
--------- Page2.html
If I want to autolink from Page1.html to Page2.html, it would look like:
<wicket:link>
<a href="../../module2/page/Page2.html">Click Here!</a>
</wicket:link>
This is not working, however. The AutoLinkResolver spits out a warning message:
"WARN - AutoLinkResolver - Did not find corresponding java class: .....module2.page.Page2"
Attachments
Attachments
- wicket-1428-patch-with-test-cases-for-wicket-1.4.x.patch
- 11 kB
- Peter Ertl
- wicket-1428-patch-with-test-cases-for-wicket-1.3.x.patch
- 11 kB
- Peter Ertl
- wicket-link-outside-of-package.zip
- 3 kB
- Peter Ertl
- WICKET-1428.patch
- 2 kB
- James Carman
Issue Links
- is depended upon by
-
WICKET-1526 issue 1428 seems not to be fixed properly - please reopen it
- Closed
Activity
Attachments:
- wicket-1428-patch-with-test-cases-for-wicket-1.3.x.patch
- wicket-1428-patch-with-test-cases-for-wicket-1.4.x.patch
I decided to pick
$up$
as a replacement string for '..' because based on RFC 1738 (Uniform Resource Locators) it needs no url encoding at all. The previous suggestion "$^" was converted into "$%5E" in Firefox which is kind of ugly for my taste and would need explicit url encoding and decoding. I wanted to avoid this unnecessary encoding/decoding stuff at all to not bother with different character sets or browser quirks of whatever kind. This should be more reliable and robust in the 100.000 browsers that currently exist. anyway, you can change that sequence using
Application.getResourceSettings().setParentFolderPlaceholder("my-favorite-escape-sequence")
I did check the patch with ms explorer 6, ms explorer 7, opera 2 and safari 3 which worked well.
An example url could now look like
/resources/my.great.and.wonderful.AnchorClass/$up$/$up$/base.css
and not be garbled by some browser anymore...
There's also a bunch of test cases included in the uploaded patches to test this behavior during regression tests.
Would be great if this stuff would make it into 1.4 and 1.3 soon
Regards
Peter
can we also apply this patch for 1.3, please?
I think having an different internal representation of resourceKeys will not be critical to applications. also, the internal representation only changes for resources that are placed in a higher level package than the scope class.
picking the right placeholder for '..' seems not as easy
taken from the RFC for URL's:
http://www.faqs.org/rfcs/rfc1738.html
> Thus, only alphanumerics, the special characters "$-_.+!*'(),", and
> reserved characters used for their reserved purposes may be used
> unencoded within a URL.
so these options have been suggested so far:
"wicket:up", "...", "+", "-", "(up)", "_", "^"
it's all a matter of personal taste and whatever we choose, some people will not like it.
so I would definitely make this a customizable setting
I uploaded another patch that includes
org.apache.wicket.settings.IResourceSettings#getParentFolderPlaceholder()
and
org.apache.wicket.settings.IResourceSettings#setParentFolderPlaceholder(CharSequence)
to modify that value.
the default for now is 'wicket:up'
Peter,
I'm not a big fan of the "wicket:up" string, since it's too verbose. But, I agree we should pick something that means "go up one level" and just use that. You didn't like the idea of using two colons "::"? Colons are already used in Wicket, so it might kind of stick with the "theme" better. I'm glad you did some of the legwork on this (I had no idea where to look). It seems like an easy fix. We'd just need to unit test the heck out of it so that we don't break stuff (and nobody else breaks it in the future).
I try to summon up what people found out so far and explain my patch using this example:
resource url = resources/markup.level1.level2.level3.page.TestPage/../../../../base.js
the path splits up into:
wicket resource prefix = "resources/"
class anchor for resource = "markup.level1.level2.level3.page.TestPage"
resource key = "../../../../base.js
the problem is that the browser tries to normalize the resource key by removing the '..' out of it and shortening the path (but, hey, the resource key is_no_path). as an effect wicket obviously will not recognize the resource key anymore. using a query parameter 'path=resourceKey' would work, but this can cause problems with browser caching, default mime type handling, etc. and also doesn't look quite as pretty (we all know beauty counts in wicket
it's preferrable to have an url that looks like a static resource from the browsers point of view.
so the patch tried to avoid the '..' at all by using a unambiguous placeholder. I decided for
public static final String PARENT_FOLDER_ESCAPE = "wicket:up";
as Igor suggested in a previous post.
It might be a good idea to make this customizable for example by a setter called
Application.getMarkupSettings().setParentFolderPlaceholder()
or whatever you would like it to name.
so resource key generation is only mimimally affected by my patch in
org.apache.wicket.SharedResources#resourceKey(final String path, final Locale locale, final String style):
before :
final String basePath = Files.basePath(path, extension);
after:
final String basePath = Files.basePath(path, extension).replace("../", PARENT_FOLDER_ESCAPE + "/");
this means any occurences of '../' in the resource key will be replace by 'wicket:up/'
the other place that needs to be changed is the part where the url gets sent to wicket and the resource key is not defined in Application.getSharedResources() yet. this can for example happen when the session changes the server node in a cluster. wicket takes the resourceKey out of the url, looks up the file in the corresponding package and adds it to the application's shared resources. that's the only place we need a real file system relative path. so we have to change back the 'wicket:up' to '..' and all is fine.
SharedResourceRequestTarget#respond(RequestCycle requestCycle)
before:
String path = resourceKey.substring(ix + 1);
after:
String path = resourceKey.substring(ix + 1).replace(SharedResources.PARENT_FOLDER_ESCAPE, "..");
the patch will only affect resource url's that go up the path hierarchy and are broken anyway. so applying it should not be dangerous at all for resource urls in general or change expected wicket behavior.
would be great if you could put that patch into trunk
Could we use "..." (three dots) or ",," (two commas) or "::" (two colons)? We just need to trick the browser into not interpreting the "go up" part of the links.
we chould use a param, but then only for these situations.
if you dont want the classname you can set an alias for that thats already possible for a long time. (or really mount it)
and no we cant just generate that and remember it because a shared resource is dynamically generated on the fly
so
/resources/xxxxx.Yyyyy/foo.js
has to work also if the app server just restarted without any hit yet to the resource yet (so it isn't rendered through a page yet)
Then purely that url has to be able to resolve the resource.
So if you make something make sure that that is working, that the resource can be created and found by just the information that you have in the url and nothing more.
Could you use a URL parameter to specify the path?
resources/markup.level1.level2.level3.page.TestPage?path=/../../../../base.js
Also, I've never really liked the fact that we have the classname in the URL. Could we use something to replace the "scope" of the resource URL with some constant (and it would be remembered for subsequent invocations)? Basically, we'd have a bidi-map that goes from some generated string <-> class name or something?
Oh, but then it won't know how to go up the class hierarchy to look for the resource with respect to the parent class. Darn it!
> but if you get this:
>
> resources/markup/base.js
>
> how on earth do we know what to get?
> Because we lost everything that is related how to get it
> We lost the classname that we want to search for.
You can assume that the path excluding the "resources/" prefix and up to the last "/" character is a package name, or "markup" in this case. So, you look within the scope of the "markup" package for a resource named "base.js"
yeah, i know. we cant win. for what its worth "wicket:" can come from the namespace prefix that you will make configurable in 1.5
if we do wicket:up:wicketup then i can already hear the complains again
how can we change that...
@johan: exactly! that is why an alternate solution i proposed is to encode
com.foo.Bar/../../base.js
into
com.foo.Bar/wicket:up/wicket:up/base.js
but if you get this:
resources/markup/base.js
how on earth do we know what to get?
Because we lost everything that is related how to get it
We lost the classname that we want to search for.
right, this is what i suggested in my earlier comment, however then sharedresources need to be rewritten to use the /foo/bar keys instead of foo.bar and to be able to properly resolve resources where scope is a package not a class...
Ooooooh! Yuck! I didn't realize the browser would be that "smart" when it saw a weird URL like that. Would it help if we spit out something like this instead?
resources/markup/level1/level2/level3/page/../../../../base.js
So, if the browser wanted to be smart about it, it would send up (if I counted correctly):
resources/markup/base.js
when you have a url like
resources/markup.level1.level2.level3.page.TestPage/../../../../base.js
the browser will convert it to a canonical url before performing the request, so what is actually requested from the server is
/base.js because obviously you cannot go 4 levels up since the url is only two levels deep
I don't see a problem with URLs like:
resources/markup.level1.level2.level3.page.TestPage/../../../../base.js
The ../ when dealing with a class name (such as we have here) means to go up to the parent package to look for the package-relative resource. Correct? To me, it's just a matter of making the resource handling logic understand resource URLs with this ../ stuff in it. Again, I'm no expert on the internals of Wicket like you folks. This is just my view from the outside.
ok, the reason your anchor works is that you actually go all the way up to the root and construct what is essentially an absolute path from root to the resource. this isnt really complete support for "relative urls"
the reason relative urls are hard with this is that the url ends up looking like this
/context/resources/some.package.in.my.app.SomeClass/../../../../someResource.js
so there is a mismatch between our encoding of the class path with "." and encoding of the relative path with "../"
the problem here is that the browser will act on ../../ and transform the path before it even hits the server.
there are two ways i see to fix this. either encode "../" into something like "wicket:up" in the url and then decode it back, or key the resources as some/package/in/my/app/SomeClass in sharedresources instead of using class name. both of these changes are quiet major and wont make it into 1.3x branch i dont think. so we can put this off until 1.5 and see if anyone interested provides a patch.
Anchor as in <a> tags, not (necessarily) anchors on a page. So, I guess I tested it with hyperlinks would be a better way to say it. The original description shows the use case I used to test it.
Perhaps the "thing" that responds to "resource" URLs needs to be able to understand parent-relative links now? The AutolinkResolver is doing its best to spit out a URL, but the thing that handles the request doesn't know how to interpret it. Can you point me in the direction of that "thing" (sorry, but I don't understand the internal workings all that well yet, but I'm glad to give it a whirl fixing it).
Was this broken in 1.3.2? Or, is this now broken because of my patch?
this is the structure of the sample:
--markup
contains file: base.js
– markup/level1/level2/level3/page/TestPage.html
contains this HTML
<wicket:link>
<script type="text/javascript" src="../../../../base.js"></script>
</wicket:link>
the HTML will evaluate to:
<script type="text/javascript" src="resources/markup.level1.level2.level3.page.TestPage/../../../../base.js"></script>
which seems wrong from just looking at the url in "src"
go : resources
go : markup.level1.level2.level3.page.TestPage
go : up
go : up
go : up
go : up
file : base.js
I have a template that includes a <wicket:link> to a javascript file outside of the current package. It does not resolve to a valid url in the browser but instead you get back a '404 - not found'.
I also attached a sample that shows the structure of that markup.
Ok, you're right, the Javadocs specifically say "the href URL must be relative to the package containing the associated page." However, one might argue that "../../blah/blah" is indeed a relative URL. Either way, I've attached the patch. I don't care if you treat it as a bug or an enhancement request. It'd be nice if this worked properly. Thanks.
../ has never been supported, and the main reason for that is/ was that it is more difficult to assume which class loader is used. I'm not sure if it really is a problem we can't overcome though, so we could make this a feature request.
thanks