[RFC][icedtea-web] Fix handling absolute paths passed into jnlp_href's value.

[Jiri Vanek]

On 05/30/2012 11:36 PM, Omair Majid wrote:
> On 05/30/2012 03:01 PM, Danesh Dadachanji wrote:
>>
>> Attached a new patch with the refactored code and a unit test.
>
> I have a few comments on the patch below.
>
>> I added a static method that just wraps around the new URL assignment. I
>> couldn't figure out how to test PluginBridge directly without having it
>> open an actual JNLP file. :/ I made it static to make the unit test
>> cleaner (did not have to deal with creating a new PluginBridge var etc)
>> since it is quite the simple method and really only serves for unit
>> testing purposes. If others prefer, I can make it non-static.
>
> I think it's fine as long as it's implied clearly that this static
> method is meant to be private (it can't be made private since it's used
> in the test class).
>
>> diff --git a/netx/net/sourceforge/jnlp/PluginBridge.java b/netx/net/sourceforge/jnlp/PluginBridge.java
>> --- a/netx/net/sourceforge/jnlp/PluginBridge.java
>> +++ b/netx/net/sourceforge/jnlp/PluginBridge.java
>> @@ -171,6 +171,18 @@ public class PluginBridge extends JNLPFi
>>           codeBaseLookup = cbl == null || (Boolean.valueOf(cbl));
>>       }
>>
>> +    /**
>> +     * Evaluate if the location is a relative or absolute path. Use the codeBase
>> +     * as the URL context if it is relative.
>> +     * @param codeBase The URL used for context if needed.
>> +     * @param location The address pointing to the JNLP.
>> +     * @return A URL to the location of the JNLP file.
>> +     * @throws MalformedURLException
>> +     */
>
> I would prefer a comment saying this method should be considered private
> rather than cleanly documenting it.
>
>> +    static URL getEvaluatedJNLPHref(URL codeBase, String location) throws MalformedURLException {
>> +        return new URL(codeBase, location);
>> +    }
>> +
>>       public boolean codeBaseLookup() {
>>       	return codeBaseLookup;
>>       }
>> diff --git a/tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java b/tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java
>> new file mode 100644
>> --- /dev/null
>> +++ b/tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java
>> @@ -0,0 +1,27 @@
>> +package net.sourceforge.jnlp;
>
> You are missing the license here.
>
>> +    @Test
>> +    public void testAbsoluteJNLPHref() throws MalformedURLException, Exception {
>> +        URL codeBase = new URL("http://undesired.absolute.codebase.com");
>> +        String absoluteLocation = "http://absolute.href.com/test.jnlp";
>> +        assertEquals(absoluteLocation,
>> +                PluginBridge.getEvaluatedJNLPHref(codeBase, absoluteLocation).toExternalForm());
>> +    }
>> +
>> +    @Test
>> +    public void testARelativeJNLPHref() throws MalformedURLException, Exception {
>> +        URL codeBase = new URL("http://desired.absolute.codebase.com");
>> +        String relativeLocation = "sub/dir/test.jnlp";
>> +        assertEquals(codeBase + "/" + relativeLocation,
>> +                PluginBridge.getEvaluatedJNLPHref(codeBase, relativeLocation).toExternalForm());
>> +    }
>> +}
>
> If you want to be extra pedantic, one more test:
>
> URL codeBase = new URL("http://desired.absolute.codebase.com/foo");
> String relativeLocation = "/sub/dir/test.jnlp";
>
> Result should be "http://desired.absolute.codebase.com/sub/dir/test.jnlp"
>
> But I am feeling sort of uncomfortable about this. These tests are just
> testing that 'new URL(a,b)' works sanely in java. If it doesn't, then
> the JVM is broken and there is nothing we can do about it. It doesn't
> help us in any way if these tests pass because these tests aren't
> verifying that PluginBridge uses this correct url.

Although I'm sharing Omair's  concerns, I think the tests for method  for purpose of its future modifications and as behaviour demonstrations are enough.

After fulfilling  Omair's requests (including one test O:)  I'm ok both with changeset and tests.


Thanx both of you
   J.