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

[Omair Majid]

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.

Cheers,
Omair