Add a URLHelper factory API to ParsingUtils #1421

jrobinso · 2019-10-03T01:15:59Z

This PR adds a URLHelper factory interface and function on ParsingUtils to set the factory. It is a replacement or alternative to the previous implementation which used reflection to accomplish the same thing. Reflection won't work for Java 11 clients, its illegal across module boundaries for the most part, and bad practice as well. The reflection API was left in place in case anyone is using it with Java 8.

…for the previous implementation which used reflection to accomplish the same thing. Reflection won't work for Java 11 clients, its illegal across module boundaries for the most part, and bad practice as well. The reflection API was left in place in case anyone is using it with Java 8.

codecov-io · 2019-10-03T01:27:45Z

Codecov Report

Merging #1421 into master will decrease coverage by 0.004%.
The diff coverage is 27.273%.

@@               Coverage Diff               @@
##              master     #1421       +/-   ##
===============================================
- Coverage     68.122%   68.118%   -0.004%     
+ Complexity      8382      8381        -1     
===============================================
  Files            573       573               
  Lines          33989     33988        -1     
  Branches        5668      5669        +1     
===============================================
- Hits           23154     23152        -2     
- Misses          8645      8646        +1     
  Partials        2190      2190

Impacted Files	Coverage Δ	Complexity Δ
...main/java/htsjdk/tribble/util/RemoteURLHelper.java	`46.154% <ø> (ø)`	`3 <0> (ø)`	⬇️
src/main/java/htsjdk/tribble/util/FTPHelper.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
src/main/java/htsjdk/tribble/util/HTTPHelper.java	`47.917% <0%> (-3.194%)`	`4 <0> (ø)`
...rc/main/java/htsjdk/tribble/util/ParsingUtils.java	`74.863% <37.5%> (+1.179%)`	`44 <1> (-1)`	⬇️
...samtools/util/AsyncBlockCompressedInputStream.java	`72% <0%> (-4%)`	`12% <0%> (-1%)`
src/main/java/htsjdk/variant/vcf/VCFHeader.java	`90.116% <0%> (+0.058%)`	`76% <0%> (ø)`	⬇️
...n/java/htsjdk/variant/vcf/VCFContigHeaderLine.java	`80.556% <0%> (+4.085%)`	`11% <0%> (+1%)`	⬆️

…ative to using reflection for URLHelper, forbidden across modules in Java 11.

lbergelson · 2019-10-04T19:41:46Z

@jrobinso Is reflection really not working for java 11? I was under the impression that java 11 was giving a nasty warning but allowing it and it would be disabled at some time in the future.

jrobinso · 2019-10-04T20:18:13Z

Not between modules.

On Fri, Oct 4, 2019 at 12:41 PM Louis Bergelson ***@***.***> wrote: @jrobinso <https://github.com/jrobinso> Is reflection really not working for java 11? I was under the impression that java 11 was giving a nasty warning but allowing it and it would be disabled at some time in the future. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1421?email_source=notifications&email_token=AAHD2HDVBGUJCWDZFILLJX3QM6L75A5CNFSM4I45F7VKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAMWHPQ#issuecomment-538534846>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHD2HHNFS2GFLNV7LD6WVLQM6L75ANCNFSM4I45F7VA> .

-- Sent from Gmail Mobile

lbergelson

@jrobinso This seems like a good improvement for java 8 users as well. The current system is really ancient and gross. Since I think this is not called by many users, and it should be pretty obvious how to upgrade, I wouldn't be opposed to just making a breaking change here and removing the old setter mechanism.

As it is, if we deprecate it we still have to default to the old mechanism (Which is the very reasonable choice that you made.) in order to not override existing code, or add a bunch of extra complication where setting the class nulls out the factory etc.
It's made worse by that publicly visible and settable static class field which makes it really hard to change this in an absolutely safe way since someone could be setting that directly.

In either case, a bit more documentation would be helpful for future viewers.

lbergelson · 2019-10-04T19:44:14Z

src/main/java/htsjdk/tribble/util/URLHelperFactory.java

+
+import java.net.URL;
+
+public interface URLHelperFactory {


Could you add javadoc explaining what this interface is for and what the contract of getHelper is.

Added, although not much to say. What else would you like to know here?

Yeah, thank you, it's basically boilerplate but it lets you know where to look for more information and it makes it clear that someone at least cared enough to write SOMETHING. Maybe it's just me, but I'm always suspicious of library code with 0 documentation.

I think the updated doc in URLHelper saying that there is a 1-1 relationship between urls and helpers is a useful addition. Naively I would have assumed that a helper could be used for many URL's so it's good to spell that out.

lbergelson · 2019-10-04T19:45:04Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

@@ -466,6 +474,11 @@ public static void registerHelperClass(Class helperClass) {
        urlHelperClass = helperClass;
    }

+
+    public static void registerHelperFactory(URLHelperFactory factory) {


Could you add a getter for this as well. There's always someone who comes along afterwards and needs one. Javadoc would be good too.

Done, and renamed to set/get per convention

lbergelson · 2019-10-04T20:02:54Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

@@ -429,10 +430,16 @@ public static boolean resourceExists(String resource) throws IOException{
     * @return
     */


Update the javadoc here to point to the new method instead of the deprecated.

The deprecated (reflection) method has been removed.

lbergelson · 2019-10-04T20:04:55Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

+        if(urlHelperFactory != null) {
+            return urlHelperFactory.getHelper(url);
+        }
+        else {


It would be good to add a comment here saying that this path is necessary for backwards compatibility with the old deprecated code path.

lbergelson · 2019-10-04T20:05:48Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

@@ -454,6 +461,7 @@ private static URLHelper getURLHelper(Class helperClass, URL url) {
     * The default helper class is {@link RemoteURLHelper}, which delegates to FTP/HTTP
     * helpers as appropriate.
     *
+     * @deprecated use {@code registerHelperFactory}


If you use an @link or @see tag instead the javadoc and IDE can provide links which is nice.

jrobinso · 2019-10-05T01:00:05Z

@lbergelson Thanks for the quick review and comments! I'll take care these early next week, but first would you prefer a breaking change that removes the old reflection stuff? As you say its ugly and not forward compatible with Java 11 and on, so maybe we should just get rid of it. Ditto removing or making private that public static variable. Let me know.

lbergelson · 2019-10-07T15:04:55Z

I vote for removing the old reflection code and that public static variable.

jrobinso · 2019-10-07T18:03:13Z

@lbergelson OK I'll give that a try and update the PR.

jrobinso · 2019-10-11T18:51:24Z

@lbergelson working on this now, but slightly off-topic I stumbled on a deprecation warming for the method htsjdk.tribble.util.URLHelper> openInputStreamForRange. This method is used extensively by IGV for tribble-indexed files. The IGV implementation is essentially the same as the htsjdk, with no reports of bugs traced to it, so I think its stable.

    /**
     * May throw an OperationUnsupportedException
     * @deprecated Will be removed in a future release, as is somewhat fragile
     * and not used.
     * @param start
     * @param end
     * @return
     * @throws IOException
     */
    @Deprecated
    InputStream openInputStreamForRange(long start, long end) throws IOException;

lbergelson · 2019-10-11T18:57:46Z

@jrobinso That was deprecated in 2013 by Jacob Siltera, so I figured that IGV didn't need it. We can undeprecate it if it's useful and well used.

* More javadoc * Remove old reflection-based code and global * add getter for helper factory

jrobinso · 2019-10-11T19:59:00Z

@lbergelson updated, have a look I'm using this version in IGV now (in my dev environment)

lbergelson

@jrobinso A few comments, mostly nitpicky. Thank you for adding the extra documentation. I appreciate it even if it's mostly boilerplate. 👍 after the last few comments.

lbergelson · 2019-10-11T20:11:36Z

src/main/java/htsjdk/tribble/util/URLHelper.java

@@ -48,7 +64,7 @@
     * @return
     * @throws IOException
     */
-    @Deprecated
+


nitpick, unecessary newline here.

lbergelson · 2019-10-11T20:11:57Z

src/main/java/htsjdk/tribble/util/URLHelper.java

    InputStream openInputStream() throws IOException;

    /**
+     * Open an InputStream to stream a slice (range)  of the resource.
+     *
     * May throw an OperationUnsupportedException
     * @deprecated Will be removed in a future release, as is somewhat fragile


This line saying it's deprecated should be removed too.

lbergelson · 2019-10-11T20:17:50Z

src/main/java/htsjdk/tribble/util/URLHelperFactory.java

+     * @throws
+     */
+    default URLHelper getHelper(URL url)   {
+        return new RemoteURLHelper(url);


I would probably not make this a default method, since we probably want to require that anyone who implements this to actually override this method. If this is non-default it lets us easily provide a lambda in place of a factory class as well since this would be a functional interface.

lbergelson · 2019-10-11T20:19:36Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

@@ -46,11 +46,10 @@

    public static final Map<Object, Color> colorCache = new WeakHashMap<>(100);

-    // HTML 4.1 color table,  + orange and magenta
-    static Map<String, String> colorSymbols = new HashMap();
+    private static URLHelperFactory urlHelperFactory = new URLHelperFactory() {};


It seems strange to have this interface have default implementation of its only method. Lets remove that and just replace this with

private static URLHelperFactory urlHelperFactory = RemoteURLHelper::new;

Sure, I thought it strange too but didn't want to make a concrete implementation. I wasn't aware of the syntax you used, nice.

Yeah! In java 8+ if you have an interface with exactly 1 abstract method you can use a lambda instead of a named or anonymous class.

lbergelson · 2019-10-11T20:20:02Z

src/main/java/htsjdk/tribble/util/HTTPHelper.java

    public InputStream openInputStreamForRange(long start, long end) throws IOException {

        HttpURLConnection connection = openConnection();
        String byteRange = "bytes=" + start + "-" + end;
        connection.setRequestProperty("Range", byteRange);
+
+        if (connection.getResponseCode() != 206) {


Good check to add if this is important. I definitely did not know this detail.

If servers don't support range requests they are free according to the spec to return the entire file with response code 200. This is bad when its a 300 gb BAM

lbergelson · 2019-10-11T20:21:15Z

src/main/java/htsjdk/tribble/util/URLHelperFactory.java

+
+import java.net.URL;
+
+public interface URLHelperFactory {


Yeah, thank you, it's basically boilerplate but it lets you know where to look for more information and it makes it clear that someone at least cared enough to write SOMETHING. Maybe it's just me, but I'm always suspicious of library code with 0 documentation.

I think the updated doc in URLHelper saying that there is a 1-1 relationship between urls and helpers is a useful addition. Naively I would have assumed that a helper could be used for many URL's so it's good to spell that out.

lbergelson · 2019-10-11T20:21:58Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

-            //TODO check that it has 1 arg constructor of proper type
-        }
-        urlHelperClass = helperClass;
+    public static void setURLHelperFactory(URLHelperFactory factory) {


This should probably throw if the value is null.

lbergelson · 2019-10-11T20:23:30Z

src/main/java/htsjdk/tribble/util/URLHelperFactory.java

+    /**
+     * @param url
+     * @return a {@link URLHelper} object for the given URL
+     * @throws


I'd drop the throws unless there's something useful to say about what it throws.

* default method in URLHelperFactory interface is removed * update some comments

lbergelson

Thank you. 👍

jrobinso referenced this pull request in igvteam/igv Oct 4, 2019

patch the htsjdk -- current release is broken for java 11. Add altern…

4f80fb1

…ative to using reflection for URLHelper, forbidden across modules in Java 11.

lbergelson requested changes Oct 4, 2019

View reviewed changes

jrobinso added 3 commits October 11, 2019 12:33

Updates to PR based on comments

d84e083

* More javadoc * Remove old reflection-based code and global * add getter for helper factory

update comment

05dd043

Update comment

eec9348

lbergelson requested changes Oct 11, 2019

View reviewed changes

Update PR based on comments

6090ee9

* default method in URLHelperFactory interface is removed * update some comments

lbergelson approved these changes Oct 11, 2019

View reviewed changes

lbergelson merged commit c33b695 into samtools:master Oct 11, 2019

lbergelson mentioned this pull request Oct 25, 2019

Replace IndexFactory reflection. #1431

Merged

5 tasks

		@@ -429,10 +430,16 @@ public static boolean resourceExists(String resource) throws IOException{
		* @return
		*/

Add a URLHelper factory API to ParsingUtils #1421

Add a URLHelper factory API to ParsingUtils #1421

Conversation

jrobinso commented Oct 3, 2019

codecov-io commented Oct 3, 2019 • edited Loading

Codecov Report

lbergelson commented Oct 4, 2019

jrobinso commented Oct 4, 2019 via email

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrobinso commented Oct 5, 2019

lbergelson commented Oct 7, 2019

jrobinso commented Oct 7, 2019

jrobinso commented Oct 11, 2019

lbergelson commented Oct 11, 2019

jrobinso commented Oct 11, 2019

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 3, 2019 •

edited

Loading