[rfc][icedtea-web] Stripping semicolon tags from jar urls

[Jiri Vanek]

Hi!

At first several global hint:

Following of http://icedtea.classpath.org/wiki/CommitPolicy is moreover a must, especially:

"IcedTea-Web code changes/new feature should be accompanied with appropriate tests (JUnit class 
and/or reproducer). If no tests are added/modified, changes should be accompanied with an 
explanation as to why. "

This is exactly the situation when both of them is worthy.

To add an unittest you need one or more method to be added into same package and SameFileTest  in 
tests/unit/sampe_package/SameFile.Test
In case that your method will remain where it is now then into 
tests/netx/unit/net/sourceforge/jnlp/ParserTest.java will be added test method(s) for getURL(..) 
where you will try to torture this method as much as possible (....;.)(...;)(;....) as much corner 
cases as possible.. and of course some normal cases. Also include please some filing cases (where 
failure is en expectation). If you will include tests also for already existing code, even better! 
By this many bugs have been already cough and found.

Maybe some refactoring will be needed in current code to make it testable
eg:
  - your new code will be moved to separate method, so you will be able to test it atomically
  - maybe method getURL will jsut call static getURL with some more parameters. static getUrl may 
have better testability. (but it is you decision whether make static getURL or create Parser 
instances in tests - choose wise!-))


To add an reproducer is a little bit more work. Pelase read 
http://icedtea.classpath.org/wiki/Reproducers. Me, jfabriko or adam may provide xchat-help if needed :)
Basically you will provide one or more jnlp files which will be testing the ";" appearences  and 
class with main method to be jarred and launched via your jnlp. Maybe your exact case will need also 
second jar as with extension. Then yoou create testcase where you will call individual jnlp.
If your case is affecting also applets, then html file9s) have to be provided.
The main class(es) is mostly very simple - just printing out :aplet name launched" "service name 
used". In testcase you are then just examining the output.

Don't get scared if you will found that more code is needed to test your change then to achieve the 
desired fix. I'm sorry for "redundant" work, but it is really necessary.


To code itself:
On 05/24/2013 04:06 PM, Andrew Azores wrote:
> As per this bug <https://bugzilla.redhat.com/show_bug.cgi?id=905074>, added some logic to JNLP
> Parser to "strip" any characters after and including semicolons following file extension for
> resource files. eg if there is a file specified as "lib/application.jar;no_javaws_cheat" then it
> will be parsed as "lib/application.jar" instead.
>
> Changelog:
> netx/net/sourceforge/jnlp/Parser.java
>
> Andrew A
>
> jar_name_semicolon_stripping.patch
>
>
> diff --git a/netx/net/sourceforge/jnlp/Parser.java b/netx/net/sourceforge/jnlp/Parser.java
> --- a/netx/net/sourceforge/jnlp/Parser.java
> +++ b/netx/net/sourceforge/jnlp/Parser.java
> @@ -1064,6 +1064,15 @@ class Parser {
>        */
>       public URL getURL(Node node, String name, URL base) throws ParseException {
>           String href = getAttribute(node, name, null);
> +
> +        if (href != null) {
Maybe this can be moved behinf the if (href == null) block, and so avid this check.
Please if so, add the brackets to  if (href == null) check:

 >           if (href == null){
 >               return null; // so that code can throw an exception if attribute was required
 >            }



> +        	int lastPeriod = href.lastIndexOf('.');

I'm not 100% sure what this means - you are checking of last appearence of dot, why? You wan to 
avoid case like this;is;my.jar;and_this_not ?

Then maybe little bit of sophistication like href.toUpperCase.lastIndexof(".JAR") is worhty.

If so, are you really trying to avoid case like starngejar;ignored;ignored;ignored ? I must say I do 
not like this, but maybe your aporach is good middle way.
If nothing else comment would be nice.


> +        	int firstSemiColon = lastPeriod == -1 ? -1 : href.indexOf(';', lastPeriod);


Please avoid ?: construction as much as possible. This is exactly the case where it should not be.

> +        	href = firstSemiColon == -1 ? href : href.substring(0, firstSemiColon);

Again... not shortest possible code, most readable code :)

> +        	// Strip any characters after the file extension iff there is a semicolon after the extension.
> +        	// eg "lib/applet.jar;no_javaws_cheat" should become "lib/applet.jar"
> +        }
> +
>           if (href == null)
>               return null; // so that code can throw an exception if attribute was required
>

Anyway I'm quite wondering if this is best place for this. This is transforming all urls which 
appears in jnlp file. Cant there be some case where ";" is valid part?
I can imagine some obscure http://my.url.net/myPage?my.script_returning;jar;version;15


I would maybe rather see this as fall back when jar is not found, then try to cut it later. But I do 
not insist, and am open to rfc as I'm not sure here :)

J.