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

Danesh Dadachanji ddadacha at redhat.com
Mon Jun 4 08:42:32 PDT 2012


On 04/06/12 06:33 AM, Jiri Vanek wrote:
> 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.
>>

Thanks, fixed!

>>> + @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"
>>

Added, works as expected.

>> 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.
>

Too late =) I wasn't too happy with this change either so I hunted down a better solution. I noticed some of the tests used JNLPs 
directly so I attempted to integrate that into my test and use the first patch (without the evaluation wrapper). However, Omair pointed 
out that using JNLPs is not ideal because this adds test coverage of the parser. So to avoid this, I've added the strategy class 
JNLPCreator that wraps around the creation of a JNLPFile. Under netx, it will simply do what it did before, create a normal JNLPFile. 
In the test however, it creates a mock JNLP, one that only has the bits my test in particular cares about (e.g. location). The 
additional class in the unit test is there to ensure PluginBridge's constructor can run without throwing exceptions.

+2012-06-04  Danesh Dadachanji  <ddadacha at redhat.com>
+
+	Fix to handle absolute paths passed into jnlp_href's value.
+	* netx/net/sourceforge/jnlp/PluginBridge.java
+	(PluginBridge): Uses context of codebase to evaluate jnlp_href's value.
+	Uses JNLPCreator's create method to make new JNLPFile variables.
+	New constructor that wraps around the original one, creating a new
+	JNLPCreator to use.
+	* netx/net/sourceforge/jnlp/JNLPCreator.java: New strategy pattern class
+	to be used to wrap around the creation of a JNLPFile. Replace this creator
+	when unit testing to skip running parsing code.
+	* tests/netx/unit/net/sourceforge/jnlp/PluginBridgeTest.java:
+	New class to unit test getEvaluatedJNLPHref.
+

Thanks to both of you for commenting! Any more would be appreciated.

Cheers,
Danesh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: absolute-url-jnlp_href-03.patch
Type: text/x-patch
Size: 9072 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120604/f7ab1acf/absolute-url-jnlp_href-03.patch 


More information about the distro-pkg-dev mailing list