[rfc][icedtea-web] Extract URL processing in TinyHttpdImpl

[Adam Domurad]

Thanks for the refactoring!

On 06/10/2013 12:00 PM, Andrew Azores wrote:
> Changelog:
> * tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java:
> extracted some lines out of run() into new method urlToFilePath()

You should use list symbols you changed in the ChangeLog if its only a 
few. Eg:
* tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
(run): Calls urlToFilePath
(urlToFilePath): New, extract path resolution from (run)

> *
> tests/test-extensions-tests/net/sourceforge/jnlp/ServerAccessTest.java:
> unit tests added for new urlToFilePath()
>
> As per Jiri's request/suggestion a few days ago.

Fix comments:

> diff --git a/tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java b/tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
> --- a/tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
> +++ b/tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
> @@ -42,6 +42,7 @@ import java.io.DataOutputStream;
>  import java.io.File;
>  import java.io.FileInputStream;
>  import java.io.InputStreamReader;
> +import java.io.UnsupportedEncodingException;
>  import java.net.HttpURLConnection;
>  import java.net.Socket;
>  import java.net.SocketException;
> @@ -120,14 +121,8 @@ public class TinyHttpdImpl extends Threa
>                          t.nextToken();
>                          String op = t.nextToken();
>                          String p = op;
> -                        if (p.startsWith(XSX)) {
> -                            p = p.replace(XSX, "/");
> -                        }
>                          ServerAccess.logNoReprint("Getting: " + p);
> -                        p = URLDecoder.decode(p, "UTF-8");
> -                        p = p.replaceAll("\\?.*", "");
> -                        p = (".".concat((p.endsWith("/")) ? p.concat("index.html") : p)).replace('/', File.separatorChar);
> -                        p = stripHttpPathParams(p);
> +                        p = urlToFilePath(p);
>                          ServerAccess.logNoReprint("Serving: " + p);
>                          File pp = new File(dir, p);
>                          int l = (int) pp.length();
> @@ -206,6 +201,28 @@ public class TinyHttpdImpl extends Threa
>      }
>
>      /**
> +     * This function transforms a request URL into a path to a file which the server
> +     * will return to the requester.
> +     * @param url - the request URL
> +     * @return a String representation of the local path to the file
> +     * @throws UnsupportedEncodingException
> +     */
> +	public static String urlToFilePath(String url) throws UnsupportedEncodingException {
> +		url = URLDecoder.decode(url, "UTF-8");		// Decode URL encoded charaters, eg "%3B" becomes ';'

Please fix the indentation in this function (also make sure you aren't 
using any tabs, see 
http://icedtea.classpath.org/wiki/IcedTea-Web#Code_style)

> +        if (url.startsWith(XSX)) {
> +            url = url.replace(XSX, "/");
> +        }
> +		url = url.replaceAll("\\?.*", "");  		// Remove query string from URL
> +		url = ".".concat(url);						// Change path into relative path

I would just place comments one space away from the line instead of 
trying to line them up, but its up to you.

> +		if (url.endsWith("/")) {
> +			url += "index.html";
> +		}
> +		url = url.replace('/', File.separatorChar); // If running on Windows, replace '/' in path with "\\"
> +		url = stripHttpPathParams(url);
> +		return url;
> +	}
> +
> +    /**
>       * This function removes the HTTP Path Parameter from a given JAR URL, assuming that the
>       * path param delimiter is a semicolon
>       * @param url - the URL from which to remove the path parameter

Otherwise looks good :-)

Test commments:

> diff --git a/tests/test-extensions-tests/net/sourceforge/jnlp/ServerAccessTest.java b/tests/test-extensions-tests/net/sourceforge/jnlp/ServerAccessTest.java
> --- a/tests/test-extensions-tests/net/sourceforge/jnlp/ServerAccessTest.java
> +++ b/tests/test-extensions-tests/net/sourceforge/jnlp/ServerAccessTest.java
> @@ -39,6 +39,8 @@ package net.sourceforge.jnlp;
>  import java.io.File;
>  import java.io.FileInputStream;
>  import java.net.URL;
> +import java.net.URLDecoder;
> +
>  import org.junit.Assert;
>  import org.junit.Test;
>
> @@ -217,6 +219,41 @@ public class ServerAccessTest {
>          Assert.assertArrayEquals(b2, bb[1]);
>          Assert.assertArrayEquals(b3, bb[2]);
>      }
> +
> +    private static final String[] filePathTestUrls = {
> +    		"/foo.html",
> +    		"/foo/",
> +    		"/foo/bar.jar",
> +    		"/foo/bar.jar;path_param",
> +    		"/foo/bar.jar%3Bpath_param",
> +    		"/foo/bar?query=string&red=hat"
> +    };
> +    @Test
> +    public void urlToFilePathTest() throws Exception {
> +    	for (String url : filePathTestUrls) {
> +    		String newUrl = TinyHttpdImpl.urlToFilePath(url);
> +    		
> +    		Assert.assertFalse("File path should not contain query string: " + newUrl, newUrl.contains("?"));
> +    		Assert.assertTrue("File path should be relative: " + newUrl, newUrl.startsWith("./"));
> +    		Assert.assertFalse("File path should not contain \"/XslowX\":" + newUrl,
> +    				newUrl.toLowerCase().contains("/XslowX".toLowerCase()));
> +    		
> +    		if (url.endsWith("/")) {
> +    			Assert.assertTrue(newUrl.endsWith("/index.html"));
> +    		}
> +    	}
> +    }
> +
> +    @Test
> +	public void urlToFilePathUrlDecodeTest() throws Exception {
> +		// This test may fail with strange original URLs, eg those containing the substring "%253B",
> +    	// which can be decoded into "%3B", then decoded again into ';'.

Indentation should be fixed here, easy way is to just autoformat in 
eclipse (make sure your formatting profile uses spaces only).

> +    	
> +    	for (String url : filePathTestUrls) {
> +    		String newUrl = TinyHttpdImpl.urlToFilePath(url);
> +    		Assert.assertEquals(newUrl, URLDecoder.decode(newUrl, "UTF-8"));
> +    	}
> +	}
>
>      @Test
>      public void stripHttpPathParamTest() {


Otherwise looks good, though.

Thanks again!
-Adam