Community
Participate
Working Groups
The WeavingAdaptor requires the aspect path to be composed by URLs. The URL of a file is encoded, for example if it contains spaces they will be represented with %20. It then converts these file:// urls to simple string paths, and then tries to access files pointed by those paths. This is done inside the FileUtil.makeClasspath(URL[]) . This method uses URL.getPath() to obtain the path. But this method does not decode the string, it returns it as it is in the URL. When later this string is used to create a new File instance, that file contains an invalid path, and the weaver fails as follows : Caused by: org.aspectj.bridge.AbortException: bad aspect library: '/home/sym/path%20with%20space/aspect-library.jar' at org.aspectj.weaver.tools.WeavingAdaptor$WeavingAdaptorMessageHolder.handleMessage(WeavingAdaptor.java:624) at org.aspectj.bridge.MessageUtil.error(MessageUtil.java:80) at org.aspectj.weaver.tools.WeavingAdaptor.error(WeavingAdaptor.java:504) at org.aspectj.weaver.tools.WeavingAdaptor.addAspectLibrary(WeavingAdaptor.java:472) at org.aspectj.weaver.tools.WeavingAdaptor.registerAspectLibraries(WeavingAdaptor.java:447) at org.aspectj.weaver.tools.WeavingAdaptor.init(WeavingAdaptor.java:177) at org.aspectj.weaver.tools.WeavingAdaptor.<init>(WeavingAdaptor.java:112) This issue is quite important, because on older windows "Documents and Settings" is an unfortunately common path, for example Maven stores there its repository. Multiple solutions are possible for this simple bug, in order of impact : - Decode the string obtained by URL.getPath() using URLEncoder.decode() - Use Files instead of Strings and let Java handle the URL, using the File(URI) constructor - Don't assume that aspect libraries are files, and hence that urls are file url, and use URLConnection to fetch aspect library contents.
A simple workaround for this bug, from the user code, is to add the following snippet in the getAspectURLs of the WeavingClassloader passed to the WeavingAdaptor : for (int i = 0; i < aspects.length; i++) { try { aspects[i] = new URL(URLDecoder.decode(aspects[i].toExternalForm(), "UTF-8")); } catch (Exception e) { throw new RuntimeException("Error sanitizing the aspect URL", e); } } Surprisingly, new URL(String) does not encode it. I've tested this under Linux, with a path with spaces. Will test it under windows XP soon.
i'm surprised this hasn't been reported before now, hmmm
It would be easier if you gave me a patch as I don't understand where you want that code snippet to be put. It mentions a field called aspects that I don't see in getAspectsURLs() - and surely putting it in there will cause the decode to happen on every call to getAspectsURLs() when it should only be done once.
Hi Andy, yes, that code was merely a workaround, forget about it. The simplest patch would be to use URLEncoder.decode() in FileUtil.java line 1422. In fact the problem is that the file url may be "file:///path%20with%20space/something.jar", calling URL.getPath() (as it happens in line 1422 of FileUtil.java) returns "/path%20with%20space/something.jar", and trying to build a new File() with that string will search for that path literaly, without reconverting %20 to " ".
I changed FileUtil to use try { ret.add(URLDecoder.decode(urls[i].toExternalForm(), "UTF-8")); } catch (UnsupportedEncodingException e) { // TODO Auto-generated catch block e.printStackTrace(); } and got lots of test failures.
Seen by Martin Lippert with tc server and insight running on the mac under STS.
Created attachment 188381 [details] patch I am attaching a patch. I confirmed that it fixes the problem in Tc Server. I am currently rerunning all the tests and seem to have no additional test failures (I did have two tests failures initially without making any changes). After applying this patch, there's actually only one test failure now. Since you said you prefer a 'minimal' change. This change impacts only the place where it matters for the TcServer problem. Also I've included a 'fallback' for when the 'correct' method of converting a URL to a path string might fail because the input contains bad stuff (such as unescaped spaces, which would not occur in correctly formatted URLs). In that case we will assume that some other ill-behaved code has passed us a URL created while not properly escaping stuff. It runs fine on Unix, but you may have some problems with tests in Windows because of '\' characters in the paths. I might try and run this on my Windows view later. With some luck the 'fallback' for broken URLs should work. Kris PS: I still don't like that there seem to be many more places that incorrectly use the URL.getPath method. PS2: I will try and create a regression test somehow later.
I tried on Windows last night. I did have one test failure *before* applying my patch. I think it was the same test that failed on Linux, a test called 'testIncompletAspectPath' or something like that. Also on Windows it seems that applying the patch makes that test pass. I did have my workspace under 'Documents and Settings' somewhere so there were spaces in the path. After applying the patch, all tests ran clean for me on Windows.
This the test failed before patch appied: org.aspectj.weaver.loadtime.WeavingURLClassLoaderTest.testIncompletePath()
patch is in, thanks Kris
