[rfc][icedtea-web] Archive URL wrongly resolved - PR1204

[Andrew Azores]

On 09/25/2013 03:55 PM, Omair Majid wrote:
> On 09/25/2013 02:49 PM, Andrew Azores wrote:
>
>> Changelog:
>> * netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java:
>> (getVersionedUrlUsingQuery) renamed to getVersionedUrl, refactored
>> construction of URL
>> * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java:
>> (requestPluginProxyInfo) extracted proxy URI logic. (processProxyUri)
>> new method for finding proxy URIs, handles absolute resource paths
>> correctly
>> *
>> tests/netx/unit/net/sourceforge/jnlp/cache/ResourceUrlCreatorTest.java:
>> added tests for ResourceUrlCreator#getVersionedUrl
>> * tests/netx/unit/sun/applet/PluginAppletViewerTest.java: added tests
>> for PluginAppletViewer.processProxyUri
>> *
>> tests/reproducers/simple/AbsolutePathsAndQueryStrings/testcase/AbsolutePathsAndQueryStrings.java:
>> new reproducer checks that absolute paths and query strings in resource
>> URLs are properly handled, and caching still works
>> *
>> tests/reproducers/simple/AbsolutePathsAndQueryStrings/resource/AbsolutePathsAndQueryStrings.html:
>> same
>> *
>> tests/reproducers/simple/AbsolutePathsAndQueryStrings/resource/AbsolutePathsAndQueryStrings.jnlp:
>> same
> One request for future patches: please keep the changelog in the format
> that you will add to the ChangeLog file.

Hmm? Did the formatting come out weird?

>
>> Here's a new version of this change set, now with more testing! The
>> actual fix is the same. The reproducer is a little bit lame but I think
>> I captured most of the cases with the unit tests anyway already.
> Thanks for updating this patch and adding tests. I have some comments
> and suggestions in-line below.
>
>> +++ b/netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java
>> +            URI uri = new URI(resourceUrl.getProtocol(), null, resourceUrl.getHost(), resourceUrl.getPort(), resourceUrl.getPath(), query, null);
> No resourceUrl.getUserInfo() ?

Apparently. I don't know what the reasoning was for that, but it only 
makes sense to include it. Thanks.

>
>> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
>> +    public static String processProxyUri(URI uri) throws URISyntaxException, UnsupportedEncodingException {
> It might be better to name this method that is a little less ambiguous.
> Maybe convertUriSchemeForProxyQuery?

Sure thing.

>
>> reproducer.patch
> I am hoping someone more familiar with reproducers will comment on this.
>
>> --- a/tests/netx/unit/sun/applet/PluginAppletViewerTest.java
>> +++ b/tests/netx/unit/sun/applet/PluginAppletViewerTest.java
>> +    @Test
>> +    public void testPluginProxyInfoUrl() throws Exception {
>> +        URI[] testUris = {
>> +                new URI("http", "foo.com", "/bar", null),
>> +                new URI("https", "foo.com", "/bar", null),
>> +                new URI("ftp", "foo.com", "/app/res/pub/channel.jar?i=1234", null),
>> +                new URI("socks", "foo.co.uk", "/bar/pub/ale.jar", null),
> This will be 'socket' (not 'socks') actually [1].

Okay, thanks, I didn't know this.

>
>> +        };
>> +
>> +        for (URI uri : testUris) {
>> +            URI result = new URI(PluginAppletViewer.processProxyUri(uri));
>> +            if (uri.getScheme().equals("ftp") || uri.getScheme().startsWith("http")) {
>> +                Assert.assertTrue(result.getScheme().equals(uri.getScheme()));
>> +            } else {
>> +                Assert.assertTrue("Scheme should have been \"http\" but was " + result.getScheme(),
>> +                        result.getScheme().equals("http"));
>> +            }
>> +            Assert.assertTrue(! (result.getAuthority() + result.getPath()).contains("//"));
> assertFalse() might be easier to read than assertTrue(! (condition)).

Uhh... I don't even know what that was. Wow. That was very silly. Thanks :)

>
> Maybe you can make it even easier to read by doing something like:
>
>      String hierarchicalPath = result.getAuthority() + result.getPath();
>      assertFalse(hierarchicalPath.contains("//"));
>
>
>> +            Assert.assertEquals(result.getQuery(), uri.getQuery());
>> +        }
>> +    }
> It might be easier to read if this test was split into multiple tests:
>
> // check that https/http/ftp scheme is accept and socks is replaced
> // with something else
> testQueryForBrowserProxyOnlyUsesHttpAndFtpScheme();
>
> // test double '//' is replace with single '/'
> testQueryForBrowserProxyContainsNoDoubleSlashes();
>
> // test query portion is unchanged
> testQueryForBrowserProxyDoesNotChangeQuery();

Mm sure, makes sense.

>
> Cheers,
> Omair
>
> [1]
> http://download.java.net/jdk8/docs/api/java/net/ProxySelector.html#select(java.net.URI)

Thanks for review!

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix.patch
Type: text/x-patch
Size: 5255 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130925/78b152d1/fix-0001.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reproducer.patch
Type: text/x-patch
Size: 10717 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130925/78b152d1/reproducer-0001.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unit_tests.patch
Type: text/x-patch
Size: 6596 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130925/78b152d1/unit_tests-0001.patch