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

[Omair Majid]

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.

> 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() ?

> +++ 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?

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

> +        };
> +
> +        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)).

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();

Cheers,
Omair

[1]
http://download.java.net/jdk8/docs/api/java/net/ProxySelector.html#select(java.net.URI)
-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681